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(bind body): content-length can be -1 #2710

Merged
merged 2 commits into from
Nov 22, 2024
Merged

Conversation

phamvinhdat
Copy link

@phamvinhdat phamvinhdat commented Nov 22, 2024

I am writing a unit test and using httptest.NewRequestWithContext to create an http request, it will return a new http request with content-length = -1 with body as http.NoBody

httpReq := httptest.NewRequest(http.MethodGet, "/", http.NoBody)
httpReq.Header.Set(echo.HeaderContentType, "application/json")

https://github.com/golang/go/blob/master/src/net/http/httptest/httptest.go#L46C1-L77C3

func NewRequestWithContext(ctx context.Context, method, target string, body io.Reader) *http.Request {
	if method == "" {
		method = "GET"
	}
	req, err := http.ReadRequest(bufio.NewReader(strings.NewReader(method + " " + target + " HTTP/1.0\r\n\r\n")))
	if err != nil {
		panic("invalid NewRequest arguments; " + err.Error())
	}
	req = req.WithContext(ctx)

	// HTTP/1.0 was used above to avoid needing a Host field. Change it to 1.1 here.
	req.Proto = "HTTP/1.1"
	req.ProtoMinor = 1
	req.Close = false

	if body != nil {
		switch v := body.(type) {
		case *bytes.Buffer:
			req.ContentLength = int64(v.Len())
		case *bytes.Reader:
			req.ContentLength = int64(v.Len())
		case *strings.Reader:
			req.ContentLength = int64(v.Len())
		default:
			req.ContentLength = -1
		}
		if rc, ok := body.(io.ReadCloser); ok {
			req.Body = rc
		} else {
			req.Body = io.NopCloser(body)
		}
	}

this is inconsistent in go-source-code as http.NewRequestWithContext with body as http.NoBody will have content-length as 0. i don't know if this should be fixed in go-source-code. golang/go#18117
https://github.com/golang/go/blob/master/src/net/http/request.go#L946-L951

func NewRequestWithContext(ctx context.Context, method, url string, body io.Reader) (*Request, error) {
...
      if body != nil {
      ...
		case *bytes.Reader:
			req.ContentLength = int64(v.Len())
			snapshot := *v
			req.GetBody = func() (io.ReadCloser, error) {
				r := snapshot
				return io.NopCloser(&r), nil
			}
		case *strings.Reader:
			req.ContentLength = int64(v.Len())
			snapshot := *v
			req.GetBody = func() (io.ReadCloser, error) {
				r := snapshot
				return io.NopCloser(&r), nil
			}
		default:
			// This is where we'd set it to -1 (at least
			// if body != NoBody) to mean unknown, but
			// that broke people during the Go 1.8 testing
			// period. People depend on it being 0 I
			// guess. Maybe retry later. See Issue 18117.

@aldas
Copy link
Contributor

aldas commented Nov 22, 2024

Please add testcase for it and add some reference (links) to this PR descrtiption for when -1 occurs.

@phamvinhdat
Copy link
Author

added

@aldas
Copy link
Contributor

aldas commented Nov 22, 2024

Thank you. Description what is being fixed and when it occurs is important from maintenance perspective, so thank you for adding it and making someones life easier in (potentially) future.

@aldas
Copy link
Contributor

aldas commented Nov 22, 2024

Some background for readers in future.

ChatGPT describes when Content-Length: -1 can happen.


Yes, there are situations where an HTTP POST request might have a Content-Length header set to -1, although it's not compliant with the HTTP specification. According to the HTTP/1.1 protocol, the Content-Length header should be a non-negative integer representing the size of the message body in bytes. Setting it to -1 is considered invalid.

However, you might encounter Content-Length: -1 in the following scenarios:

Misconfigured Clients or Servers: Software bugs or misconfigurations can cause clients or servers to incorrectly set the Content-Length to -1.

Placeholder Values in Code: In some programming languages or frameworks, -1 is used as a placeholder to indicate an unknown or unset length. If this placeholder is not properly handled before sending the HTTP request, it might result in Content-Length: -1.

Testing and Debugging: Developers may deliberately set Content-Length to -1 when testing how servers handle malformed or invalid requests.

Proxy or Middleware Issues: Intermediate systems like proxies, gateways, or load balancers might unintentionally modify headers incorrectly, resulting in an invalid Content-Length.

Misuse of Transfer-Encoding: In HTTP/1.1, when using Transfer-Encoding: chunked, the Content-Length header should be omitted. Misunderstandings about this can lead to setting Content-Length: -1 to indicate an unspecified length, which is incorrect.

Implications:

Server Behavior: Servers receiving a request with Content-Length: -1 should treat it as a protocol error. The typical response is a 400 Bad Request error.

Security Concerns: Incorrect Content-Length headers can lead to security vulnerabilities like request smuggling, where an attacker sends malformed requests to confuse intermediary servers.

Recommendations:

For Developers: Ensure that your applications correctly set the Content-Length header or use Transfer-Encoding: chunked as appropriate. Avoid using placeholder values like -1 in production code.

For Server Administrators: Implement strict validation for incoming requests. Reject requests with invalid Content-Length headers to prevent potential security risks.

Conclusion:

While Content-Length: -1 is not valid according to HTTP standards, such situations can arise due to errors or intentional testing. It's important to handle these cases appropriately to maintain protocol compliance and ensure security.

@aldas aldas merged commit a973e3b into labstack:master Nov 22, 2024
14 checks passed
@aldas aldas mentioned this pull request Dec 11, 2024
@178inaba
Copy link
Contributor

The issue where ContentLength becomes -1 when using http.NoBody with httptest.NewRequest is being fixed and is expected to be released in version 1.24.
golang/go#68476
https://go.dev/cl/599815

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.

3 participants