net/http/httptest: NewRequest easily misused as client side requests with full URLs

[patrickod]

httptest.NewRequest can create HTTP requests that differ subtly from that of production HTTP servers resulting in misleading test behavior.

Specifically, when httptest.NewRequest is passed a complete URL as its second argument, it will parse the URL and return a http.Request whose URL fields are populated with all of the data in that URL.

However in production per the http.Request Go documentation the http.Request objects passed to handlers will have URLs with only Path and RawQuery populated.

// URL specifies either the URI being requested (for server
// requests) or the URL to access (for client requests).
//
// For server requests, the URL is parsed from the URI
// supplied on the Request-Line as stored in RequestURI.  For
// most requests, fields other than Path and RawQuery will be
// empty. (See [RFC 7230, Section 5.3](https://rfc-editor.org/rfc/rfc7230.html#section-5.3))
//
// For client requests, the URL's Host specifies the server to
// connect to, while the Request's Host field optionally
// specifies the Host header value to send in the HTTP
// request.
URL *[url](https://pkg.go.dev/net/url).[URL](https://pkg.go.dev/net/url#URL)

The result is that tests that consume httptest.NewRequest and that rely on the presence of URL fields other than Path and RawQuery will pass under go test but the behavior that they exercise will never actually execute in production.

An example of this misleading behavior can be found in the gorilla/csrf library whose Referer header checks were reliant on r.URL.Scheme being set to https.

/cc @golang/security

[seankhliao]

As the docs for httptest.NewRequestWithContext state, it is for creating server side requests, which can see the full url in some requests such as CONNECT. It also points out that client requests should use the standard http.NewRequest.

I don't think there's anything for the Go project to do, gorilla/csrf should fix their docs.

[rolandshoemaker]

I think it is worth thinking about how we can make it less easy to misuse this API, whether that be by improving the documentation, or by adding guardrails that prevent people who have overlooked that documentation from doing something that can cause the type of problem that gorilla/csrf had.

[seankhliao]

I guess it's a really common mistake https://github.com/search?q=language%3Ago%20%2Fhttptest.NewRequest(WithContext)%3F%5C(.*%2C%20%22http%2F&type=code

perhaps a vet check that full URLs should not be used with GET / POST / etc? not sure if it's accurate enough.

[gabyhelp]

Related Issues

• net/http/httptest: make easier way to create Requests for testing #14915 (closed)
• net/http: NewRequest and NewRequestWithContext do not ensure a valid path #71565 (closed)
• net/http: Inconsistent output with req.URL.RequestURI() #67666 (closed)
• net/http: *Request.URL.Scheme returns an empty string. No alternative way present to get request url scheme. #28940 (closed)

Related Documentation

• Package httptest > func NewRequestWithContext ¶ 1.23

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[FiloSottile]

The docs are really clear that the returned value is an "incoming server Request, suitable for passing to an http.Handler for testing" which for non-CONNECT methods (which NewRequest knows) should not have those URL fields set. IMHO, it's outright a bug.

Making the target argument a relative path would lose the ability to specify the host and HTTPS use, so it doesn't sound like the correct thing for the application to do.

[rolandshoemaker]

I think there is some confusion about the Request.URL value in general.

It will usually only have the Path and RawQuery set, but this is entirely based on what the client sends the server. If they send a request-target of the absolute-form, then the scheme and host will be set, if they send the authority-form then the host will be set, if they send the origin-form then only the path and query will be set.

The server cannot predict what form they will send, and per RFC 7230 basically has to support all of them, even if it's expected that the client typically should only ever send the origin-form.

httptest.NewRequest just uses http.ReadRequest under the hood, and therefore handles request-target values in the same way as Server does, and it's therefore up to the user to provide values that they expect to see in non-testing contexts. Probably they should be testing with all request-target forms, but this is clearly not widely understood.

[FiloSottile]

Right, but as a testing interface the current httptest.NewRequest is problematic: the average user wants to test the average request, which is the origin-form, and instead they are surprised with the absolute-form.

Worse, there is no good way to get a origin-form Request with Request.TLS and Request.Host set, AFAICT, because httptest.NewRequest sets them based on the target.

[rolandshoemaker]

Right, although I think the httptest.NewRequest documentation is rather clear on this, you pass the request-target form that you want the request to represent. But you're right, it's somewhat unclear (and perhaps misleading) how you would get a origin-form request that was sent over TLS, or set any Request parameters that are based on header values such as Host.

I don't think we can really change the behavior of httptest.NewRequest without breaking normal usages of it. Perhaps this needs a new API.