Skip to content

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

Merged
deansheather merged 2 commits into
mainfrom
dean/non-canonical-app-headers
Dec 7, 2022
Merged

fix: do not canonicalize Sec-WebSocket-* headers in apps#5334
deansheather merged 2 commits into
mainfrom
dean/non-canonical-app-headers

Conversation

@deansheather
Copy link
Copy Markdown
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
Copy Markdown
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.

Comment thread coderd/workspaceapps.go
// 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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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