Fix content length in CONNECT requests and responses#866
Merged
Conversation
This is for compatibility between conn and handler versions. Also, handler version sets content length for http/2 to indicate that there will be DATA frames.
83ece87 to
5659193
Compare
Choraden
reviewed
Jul 25, 2024
internal/martian/proxy_connect.go
Outdated
|
|
||
| func newConnectResponse(req *http.Request) *http.Response { | ||
| return &http.Response{ | ||
| StatusCode: http.StatusOK, |
Contributor
There was a problem hiding this comment.
Please set Status too. The response log lacks it, you can see it in commit message.
sc-1 | HTTP/2.0
vs
sc-server-1 | HTTP/1.1 200 OK
Contributor
There was a problem hiding this comment.
Wrong 🤔. That's how it's done in proxyutil
Status: fmt.Sprintf("%d %s", code, http.StatusText(code)),
Contributor
Author
There was a problem hiding this comment.
That is the best way of doing it, fixed.
50184fd to
0e79abf
Compare
Contributor
res.ContentLength = -1 // Do we still need that?
if err := p.tunnel("CONNECT", res, crw); err != nil {
log.Errorf(ctx, "CONNECT tunnel: %v", err)
}I wonder if we still need that, since we set the ContentLength in read. I understand this is just a sanity check if the ContentLength was modified in Martian req/res modifiers? |
Contributor
Author
I'd leave that to be on the safe side, we can add some logging. |
Contributor
|
Sure, let's log it. |
Without this patch we (the net/http package) assume any request content length without Content-Length header is 0. This is not the case for CONNECT requests.
Make CONNECT response use http.NoBody and ConetentLenth = -1 Logs from Sauce Connect showing clean CONNECT requests and responses when using this patch. sc-1 | CONNECT //cdn.cookielaw.org:443 HTTP/2.0 sc-1 | Host: cdn.cookielaw.org:443 sc-1 | Sl-Forwarded-Host: cdn.cookielaw.org:443 sc-1 | Sl-Forwarded-Proto: sc-1 | User-Agent: curl/8.6.0 sc-1 | Via: 1.1 sc_server-159f7f75742d33630cfc, 2.0 sauce_connect-e31d884000ef4e50a1a0 sc-1 | sc-1 | HTTP/2.0 OK sc-1 | sc-1 | ... sc-server-1 | CONNECT https://cdn.cookielaw.org:443 HTTP/1.1 sc-server-1 | Host: cdn.cookielaw.org:443 sc-server-1 | Sl-Forwarded-Host: cdn.cookielaw.org:443 sc-server-1 | Sl-Forwarded-Proto: sc-server-1 | User-Agent: curl/8.6.0 sc-server-1 | Via: 1.1 sc_server-159f7f75742d33630cfc sc-server-1 | sc-server-1 | HTTP/1.1 200 OK sc-server-1 | sc-server-1 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Make sure that we correctly read and send Content-Length header value for CONNECT requests.