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

Fix connect response that prevented proper working of websockets in FireFox #956

Merged
merged 3 commits into from
Nov 13, 2024

Conversation

mmatczuk
Copy link
Contributor

@mmatczuk mmatczuk commented Nov 12, 2024

Fixes #938

When using http.Response.Write() the resulting payload contains additional Connection: close header.
Firefox treats that proxy response as server response preventing proper working of a connect tunnel.

Fixes #938
…sponses

Add writeConnectOKResponse() and modify proxyConn.writeResponse() to use it.
@mmatczuk mmatczuk changed the title Fix connect response that prevented proper working of websockets Fix connect response that prevented proper working of websockets in FireFox Nov 12, 2024
Comment on lines -244 to -247
if res.ContentLength != -1 {
log.Errorf(ctx, "CONNECT response with Content-Length: %d, ignoring content length", res.ContentLength)
res.ContentLength = -1
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't remove that. Even though, the proxy will now use raw bytes for connectOKResponse, It does not hurt us to keep this condition. That could print a useful log we are doing something wrong in some other place.

Comment on lines +174 to +179
pc := proxyConn{
Proxy: p.Proxy,
brw: brw,
conn: conn,
}
pc.writeResponse(res)
Copy link
Contributor

Choose a reason for hiding this comment

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

Fine, but it starts to look odd. Maybe there is some better way to share that code then 🤔.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks ok to me tbh

Copy link
Contributor

@Choraden Choraden left a comment

Choose a reason for hiding this comment

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

LGTM with minor suggestions

@mmatczuk mmatczuk merged commit 1579e2e into main Nov 13, 2024
6 checks passed
@mmatczuk mmatczuk deleted the mmt/ws_error branch November 13, 2024 10:34
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.

Websockets do not connect on first attempt on Firefox
2 participants