Skip to content

fix: do not canonicalize Sec-WebSocket-* headers in apps #5334

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

Merged
merged 2 commits into from
Dec 7, 2022

Conversation

deansheather
Copy link
Member

@deansheather deansheather commented Dec 7, 2022

Browsers write Sec-WebSocket-* headers, Golang canonicalizes the headers to become Sec-Websocket-* and passes them on when we reverse proxy. This changes the reverse proxy to always send those headers as Sec-WebSocket-* with a capital S.

A customer was affected by this when running an app that didn't perform case-insensitive header checks (GTK broadwayd).

TODO:

  • Tests (we'll have to use raw TCP instead of HTTP to do this I think, which is annoying but doable)

@deansheather deansheather requested a review from mtojek December 7, 2022 11:11
@deansheather deansheather marked this pull request as ready for review December 7, 2022 11:11
Copy link
Member

@mtojek mtojek left a comment

Choose a reason for hiding this comment

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

LGTM. Left one nit-pick.

// Some apps our customers use are sensitive to the case of these headers.
//
// https://github.com/golang/go/issues/18495
var nonCanonicalHeaders = map[string]string{
Copy link
Member

Choose a reason for hiding this comment

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

nit: Only these keys or every Sec-Websocket-*? If the latter one, maybe you can refactor it to the future-proof logic (Sec-Websocket-Foo, Sec-Websocket-Bar, etc.)

Copy link
Member Author

Choose a reason for hiding this comment

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

I considered using a regex or some custom function to do it but decided against it as the websocket spec was made in 2011 and they haven't added any new headers that require this level of massaging since. Hashmap is the fastest so I think it's a fine trade off

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough, feel free to ship it

@deansheather deansheather merged commit 161465d into main Dec 7, 2022
@deansheather deansheather deleted the dean/non-canonical-app-headers branch December 7, 2022 12:55
@github-actions github-actions bot locked and limited conversation to collaborators Dec 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants