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

http3: fix check for content length of the response #3998

Merged
merged 2 commits into from
Jul 29, 2023

Conversation

imroc
Copy link
Contributor

@imroc imroc commented Jul 27, 2023

No description provided.

@marten-seemann
Copy link
Member

What are you trying to achieve with this change?

@imroc
Copy link
Contributor Author

imroc commented Jul 28, 2023

What are you trying to achieve with this change?

Just like the logic written in comments: Check that the server doesn't send more data in DATA frames than indicated by the Content-Length header (if set).

@imroc
Copy link
Contributor Author

imroc commented Jul 28, 2023

A simple example to reproduce the bug:

package main

import (
	"github.com/quic-go/quic-go/http3"
	"io"
	"net/http"
	"strings"
)

func main() {
	client := &http.Client{
		Transport: &http3.RoundTripper{},
	}
	req, err := http.NewRequest(http.MethodPost, "https://www.cloudflare.com/", strings.NewReader("test"))
	if err != nil {
		panic(err)
	}
	req.Header.Set("Content-Length", "4")
	res, err := client.Do(req)
	if err != nil {
		panic(err)
	}
	_, err = io.ReadAll(res.Body)
	if err != nil {
		panic(err)
	}
}

Run the code:

$ go run .
panic: peer sent too much data

goroutine 1 [running]:
main.main()
        /Users/roc/dev/test/quic-go/main.go:25 +0x2d4
exit status 2

@imroc
Copy link
Contributor Author

imroc commented Jul 28, 2023

Current logic: Check if response body content-length if greater than Content-Length header in request.

Should change to: Check if response body content-length if greater than Content-Length header in response.

@marten-seemann
Copy link
Member

Oh, I see! I missed that you're using res and not req. I don't think the textproto conversion is needed though, headers are already normalized after we retrieve them from qpack.

@imroc imroc force-pushed the fix-length-check branch from fcc258e to 87fc2c9 Compare July 28, 2023 06:07
@imroc
Copy link
Contributor Author

imroc commented Jul 28, 2023

Oh, I see! I missed that you're using res and not req. I don't think the textproto conversion is needed though, headers are already normalized after we retrieve them from qpack.

OK, textproto is removed.

http3/client.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jul 28, 2023

Codecov Report

Merging #3998 (4925daa) into master (469a615) will decrease coverage by 0.04%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #3998      +/-   ##
==========================================
- Coverage   82.86%   82.82%   -0.04%     
==========================================
  Files         146      146              
  Lines       14729    14729              
==========================================
- Hits        12204    12198       -6     
- Misses       2025     2030       +5     
- Partials      500      501       +1     
Files Changed Coverage Δ
http3/client.go 85.89% <0.00%> (ø)

... and 2 files with indirect coverage changes

@marten-seemann marten-seemann changed the title fix: check response content-length other than request content-length http3: fix check for response content-length Jul 28, 2023
@marten-seemann marten-seemann added this to the v0.37.1 milestone Jul 28, 2023
@marten-seemann marten-seemann changed the title http3: fix check for response content-length http3: fix check for content length of the response Jul 29, 2023
@marten-seemann marten-seemann merged commit 32f8b20 into quic-go:master Jul 29, 2023
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.

2 participants