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.

Sign up for GitHub

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
Merged

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

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
343ce35
fix: do not canonicalize Sec-WebSocket-* headers in apps
deansheather Dec 7, 2022
a566830
chore: test for non-canonical header name subst
deansheather Dec 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
fix: do not canonicalize Sec-WebSocket-* headers in apps
  • Loading branch information
@deansheather
deansheather committed Dec 7, 2022
commit 343ce3568a3cad162373f9ef2eb31ce55df90e98
29 changes: 28 additions & 1 deletion coderd/workspaceapps.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,23 @@ const (
redirectURIQueryParam = "redirect_uri"
)

// nonCanonicalHeaders is a map from "canonical" headers to the actual header we
// should send to the app in the workspace. Some headers (such as the websocket
// upgrade headers from RFC 6455) are not canonical according to the HTTP/1
// spec. Golang has said that they will not add custom cases for these headers,
// so we need to do it ourselves.
//
// 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

"Sec-Websocket-Accept": "Sec-WebSocket-Accept",
"Sec-Websocket-Extensions": "Sec-WebSocket-Extensions",
"Sec-Websocket-Key": "Sec-WebSocket-Key",
"Sec-Websocket-Protocol": "Sec-WebSocket-Protocol",
"Sec-Websocket-Version": "Sec-WebSocket-Version",
}

func (api *API) appHost(rw http.ResponseWriter, r *http.Request) {
host := api.AppHostname
if api.AccessURL.Port() != "" {
Expand Down Expand Up @@ -708,14 +725,24 @@ func (api *API) proxyWorkspaceApplication(proxyApp proxyApplication, rw http.Res
return
}
defer release()
proxy.Transport = conn.HTTPTransport()

// This strips the session token from a workspace app request.
cookieHeaders := r.Header.Values("Cookie")[:]
r.Header.Del("Cookie")
for _, cookieHeader := range cookieHeaders {
r.Header.Add("Cookie", httpapi.StripCoderCookies(cookieHeader))
}
proxy.Transport = conn.HTTPTransport()

// Convert canonicalized headers to their non-canonicalized counterparts.
// See the comment on `nonCanonicalHeaders` for more information on why this
// is necessary.
for k, v := range r.Header {
if n, ok := nonCanonicalHeaders[k]; ok {
r.Header.Del(k)
r.Header[n] = v
}
}

// end span so we don't get long lived trace data
tracing.EndHTTPSpan(r, http.StatusOK, trace.SpanFromContext(ctx))
Expand Down