Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

unsupported permessage-deflate parameter: "client_max_window_bits=15" from client #443

Open
HMaker opened this issue Apr 9, 2024 · 8 comments

Comments

@HMaker
Copy link

HMaker commented Apr 9, 2024

If a websocket server reply with extension permessage-deflate; client_max_window_bits=15 an error happens:

unsupported permessage-deflate parameter: "client_max_window_bits=15"

at

websocket/dial.go

Lines 268 to 300 in bd07a64

func verifyServerExtensions(copts *compressionOptions, h http.Header) (*compressionOptions, error) {
exts := websocketExtensions(h)
if len(exts) == 0 {
return nil, nil
}
ext := exts[0]
if ext.name != "permessage-deflate" || len(exts) > 1 || copts == nil {
return nil, fmt.Errorf("WebSocket protcol violation: unsupported extensions from server: %+v", exts[1:])
}
_copts := *copts
copts = &_copts
for _, p := range ext.params {
switch p {
case "client_no_context_takeover":
copts.clientNoContextTakeover = true
continue
case "server_no_context_takeover":
copts.serverNoContextTakeover = true
continue
}
if strings.HasPrefix(p, "server_max_window_bits=") {
// We can't adjust the deflate window, but decoding with a larger window is acceptable.
continue
}
return nil, fmt.Errorf("unsupported permessage-deflate parameter: %q", p)
}
return copts, nil
}

@nhooyr
Copy link
Owner

nhooyr commented Apr 9, 2024

Since we don't include it in the handshake request whatever server you're trying to use isn't implementing the RFC correctly.

See https://datatracker.ietf.org/doc/html/rfc7692

   If a received extension negotiation offer doesn't have the
   "client_max_window_bits" extension parameter, the corresponding
   extension negotiation response to the offer MUST NOT include the
   "client_max_window_bits" extension parameter.

The library is correct to error on it.

@nhooyr nhooyr closed this as completed Apr 9, 2024
@HMaker
Copy link
Author

HMaker commented Apr 9, 2024

I did include the client_max_window_bits parameter because that's what the Chrome browser does. I fixed it in my fork.

@HMaker
Copy link
Author

HMaker commented Apr 9, 2024

related #351

@nhooyr
Copy link
Owner

nhooyr commented Apr 9, 2024

The situation is different from #351.

There the server is hinting us on its window size but that doesn't matter to use as we always use the largest window size. See #258 (comment)

Here if you include client_max_window_bits you're telling the server it can reply with client_max_window_bits with a value less than 15. But we don't support that as we cannot adjust the window size to be less than 2**15.

   If a received extension negotiation offer has the
   "client_max_window_bits" extension parameter, the server MAY include
   the "client_max_window_bits" extension parameter in the corresponding
   extension negotiation response to the offer.  If the
   "client_max_window_bits" extension parameter in a received extension
   negotiation offer has a value, the server may either ignore this
   value or use this value to avoid allocating an unnecessarily big LZ77
   sliding window by including the "client_max_window_bits" extension
   parameter in the corresponding extension negotiation response to the
   offer with a value equal to or smaller than the received value.

If you know a way to adjust the window size dynamically with the standard library deflate package I'd be happy to add support and then we can support these extensions parameters fully.

@HMaker
Copy link
Author

HMaker commented Apr 9, 2024

In my case the server always reply with client_max_window_bits=15, I made websocket accept that

switch p {
  case "client_no_context_takeover":
	  copts.clientNoContextTakeover = true
	  continue
  case "server_no_context_takeover":
	  copts.serverNoContextTakeover = true
	  continue
  case "client_max_window_bits=15":
	  continue
}

@nhooyr
Copy link
Owner

nhooyr commented Apr 9, 2024

But you just said

I did include the client_max_window_bits parameter because that's what the Chrome browser does. I fixed it in my fork.

If you didn't include it and the server is always replying then the server is broken. It's violating the RFC.

@HMaker
Copy link
Author

HMaker commented Apr 9, 2024

I meant the server replies with client_max_window_bits=15 when the client sends client_max_window_bits, so it won't break this library

Probably most servers do that

@nhooyr
Copy link
Owner

nhooyr commented Apr 9, 2024

Ok fair enough I'll add a bypass here too.

@nhooyr nhooyr reopened this Apr 9, 2024
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

No branches or pull requests

2 participants