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

net/http: allow user-defined behaviour on malformed HTTP requests #25116

Open
mjwbyrne opened this Issue Apr 27, 2018 · 8 comments

Comments

Projects
None yet
4 participants
@mjwbyrne

mjwbyrne commented Apr 27, 2018

The net/http HTTP server handles certain malformed HTTP requests completely autonomously, without ever passing control to non-library code. This means that the application is never made aware of them, so it cannot take any action, e.g. logging that a malformed request was received. It also prevents the application from customising the response.

For example, running a minimal Go HTTP server at localhost:8000 and doing this:

curl -v "http://localhost:8000/foo bar"

results in

*   Trying 127.0.0.1...
* TCP_NODELAY set
* Connected to localhost (127.0.0.1) port 8000 (#0)
> GET /foo bar HTTP/1.1
> Host: localhost:8000
> User-Agent: curl/7.55.1
> Accept: */*
> 
< HTTP/1.1 400 Bad Request
< Content-Type: text/plain; charset=utf-8
< Connection: close
< 
* Closing connection 0
400 Bad Request

(The error is caused by the whitespace in the first line of the request - the parser naively splits it on space and expects [method, URI, protocol]).

This is a problem because:

  • The application can't log the malformed request.
  • I can't make a guarantee like "my API always returns JSON", because "400 Bad Request" is not valid JSON.
  • The error message is very little help to the end user, and when the end user contacts me for support I have nothing to go on when diagnosing the problem because I couldn't log the request.
  • Certain proxy_pass directives in Nginx do a silent (and probably unexpected) URL-decode on the request URI. So if you run a Go HTTP server behind an Nginx reverse proxy (surely a common scenario), the error above will be triggered by a correct-looking request with a %20 somewhere in it.

I propose adding an exported variable of type

func(error, http.ResponseWriter, []byte)

to http.Server. It can be set by the application before ListenAndServe or ListenAndServeTLS is called. If this is nil (the default), the current behaviour stays. If it is not nil, then it is called (with the error, responseWriter and, optionally, a byte slice containing the offending part of the request - or even all of it?) when the server encounters a malformed request which it cannot pass on to the handler.

@bcmills bcmills added this to the Unplanned milestone Apr 27, 2018

@bcmills

This comment has been minimized.

Member

bcmills commented Apr 27, 2018

CC: @bradfitz

@bradfitz

This comment has been minimized.

Member

bradfitz commented May 4, 2018

Related bug: #14952 (net/http: add switches to accept/send bogus HTTP?)

The signature func(error, http.ResponseWriter, []byte) is a bit bizarre, especially that last argument. If it's malformed, that means we don't really understand it, so it's not clear we even know what's wrong. And how does this work with HTTP/2? Give you the whole binary frame? Or is this an HTTP/1-only thing, with the idea that HTTP/2 requests are all well formed since they haven't degraded over years yet?

I think I'd be fine adding in some HTTP1-specific hook for malformed requests, but I don't think I'd want to involve ResponseWriter at all. I'd prefer to just provide the net.Conn and the bytes consumed. It'd probably look more like the Hijacker interface. i.e. "On bogus HTTP/1.x request, the connection is all yours... have fun!"

@bradfitz

This comment has been minimized.

Member

bradfitz commented May 4, 2018

If somebody wants to make a prototype along those lines and send a CL we can take a look.

@mjwbyrne

This comment has been minimized.

mjwbyrne commented May 4, 2018

Regarding the signature, the idea is that control is passed to the application to do whatever it wants. It might just discard the []byte (if, as you say, it just doesn't know what to do with a malformed request), but why not let the application decide whether the request is so broken it can't be dealt with? For example, in the "literal space in the URL" example I gave originally, it's conceivable that application code could in fact rescue the request.

I can't comment on how this would interact with HTTP/2 because I don't know enough (i.e. anything, really) about HTTP/2.

The Hijacker-style solution sounds great to me. Happy to knock out a first draft and send it for review.

@bradfitz

This comment has been minimized.

Member

bradfitz commented May 4, 2018

it's conceivable that application code could in fact rescue the request.

Maybe, but you won't be able to pass it back to net/http to continue using. But yeah, you might be able to fire back a response that the client is happy with & then close the TCP connection. If you're cool with that.

@mjwbyrne

This comment has been minimized.

mjwbyrne commented May 4, 2018

Yes, that's what I had in mind - when a malformed request comes in, the hook gets called and from then on it's up to the application to handle it entirely - if it wants to rescue the request it has to do all the header parsing and so on by itself.

I expect the real-life use cases would simply be to respond with a 400 Bad Request, but in a more customisable way than literally putting "400 Bad Request" on the wire. Also logging.

@gopherbot

This comment has been minimized.

gopherbot commented May 5, 2018

Change https://golang.org/cl/111695 mentions this issue: net/http: add a malformed HTTP request callback to http.Server

@mjwbyrne

This comment has been minimized.

mjwbyrne commented May 5, 2018

Proposed change submitted. This is a somewhat simpler design than what we've been discussing - it does the minimum required to give the application a reasonable say in how malformed requests are handled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment