Skip to content

Conversation

@Jeremy-Boyle
Copy link

Implement functionality to send and access custom headers during WebSocket handshake and server communication. This enhancement improves authentication and client-server interactions.

Closes: #1342

To Do:

  • Update readme with new examples
  • Update websocket api to allow optional headers
  • Update websocket server to be able to handle headers
    • Implement way to get the headers
  • Update Tests
  • Update c-api (Can you please help implement this, not sure what would be best here, for the new functions / names)

Copy link
Owner

@paullouisageneau paullouisageneau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good overall, thank you. I just have a few comments, the main one being about introducing two maps whereas it seems to me that one would do the job.

WebSocket::~WebSocket() { PLOG_VERBOSE << "Destroying WebSocket"; }

void WebSocket::open(const string &url) {
void WebSocket::open(const string &url, const std::map<string, string> &headers) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick, but since you plan to store the map, you should pass it by value and move it into the WsHandshake() constructor.

Comment on lines +82 to +84
for (const auto& [headerName, headerValue] : mCustomHeaders) {
out += headerName + ": " + headerValue + "\r\n";
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should at least sanitize headerValue:

Suggested change
for (const auto& [headerName, headerValue] : mCustomHeaders) {
out += headerName + ": " + headerValue + "\r\n";
}
for (auto [headerName, headerValue] : mCustomHeaders) {
headerValue.erase(std::remove(headerValue.begin(), headerValue.end(), '\r'), headerValue.end());
std::replace(headerValue.begin(), headerValue.end(), '\n', ' ');
out += headerName + ": " + headerValue + "\r\n";
}

Comment on lines +61 to +62
std::map<string, string> mCustomHeaders;
std::multimap<string, string> mRequestHeaders;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to use two different maps here? You could use only one multimap.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey ! It's to add support for the websocket server.

But good suggestion. Rather than custom and request we can just rename them to request headers or just headers up to you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Custom headers for websocket

2 participants