Fix/relative url proxy handler#1220
Conversation
Resolves SpectoLabs#774. When goproxy routes a relative-URL HTTP/1.1 request to NonproxyHandler, reconstruct the absolute URL from the Host header and re-dispatch to the proxy. Return 400 Bad Request if Host is absent, 500 if Host points to the proxy itself (self-address guard to prevent infinite recursion). Add regression tests covering all three paths. Extract duplicated string literals into constants.
|
hi @scovl thanks for your contributions! Just let you know that I will be offline for the next few days in case you're wondering why no one picks up the review. no sure if @marcoioco is around, otherwise i will check these out as soon as I can! |
Np, dude! |
|
@scovl thanks. Will review this in a few days. |
| // Guard: if the Host header targets the proxy's own port the client is connecting directly to | ||
| // Hoverfly (not using it as a proxy). In that case keep the original 500 "is a proxy server" | ||
| // response — forwarding to ourselves would cause infinite recursion or a panic. | ||
| proxy.NonproxyHandler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { |
There was a problem hiding this comment.
does it override the default goproxy implementation?
There was a problem hiding this comment.
Yes, it overrides the default goproxy NonproxyHandler, which returns HTTP 500 for all non-proxy requests. The new handler reconstructs the absolute URL from the Host header and re-dispatches the request through proxy.ServeHTTP, enabling HTTP/1.1 clients that send relative URLs to be handled correctly.
The original 500 behavior is preserved only when the
Hosttargets the proxy's own port, to prevent infinite recursion.
There was a problem hiding this comment.
Great! I think this should fix the issue while improving the existing non-proxy request check.
tommysitu
left a comment
tommysitu
left a comment
There was a problem hiding this comment.
Nice fix — the approach is sound and the placement right after goproxy.NewProxyHttpServer() is correct, since it overrides the default NonproxyHandler before any other handlers are wired.
RFC compliance. HTTP/1.1 mandates a Host header (RFC 7230 §5.4), so the 400 for missing Host is correct. HTTP/1.0 clients without Host genuinely can't be reconstructed — 400 is the only sane response there too.
HTTPS scope. Hard-coding scheme = "http" is fine — raw TCP clients like telnet can't do TLS anyway, and HTTPS on the proxy port requires CONNECT.
Good test coverage too. Well done!
3 participants
Problem
When an HTTP/1.1 client sends a request with a relative URL (path only, no scheme/host) and a
Hostheader a valid pattern per RFC 7230 goproxy routes it toNonproxyHandler. The default implementation returns500 "This is a proxy server", making Hoverfly appear broken for clients that rely on this behaviour (e.g. some HTTP libraries and CLI tools).Reproduces with:
Fix
Override
NonproxyHandlerinNewProxyto reconstruct the absolute URL from theHostheader and re-dispatch the request through the normal proxy pipeline:400 Bad Requestif theHostheader is absent (HTTP/1.0 style request with no host information cannot be forwarded).500(original behaviour) if theHostport matches the proxy's own port, preventing infinite recursion when a client connects directly to Hoverfly instead of using it as a proxy.r.URL.Scheme = "http"andr.URL.Host = r.Host, then callsproxy.ServeHTTPthe same pattern already used byNewWebserverProxy.Testing
Three regression tests added to
core/proxy_test.gousing raw TCP (required becausehttp.DefaultClientalways produces absolute URLs and would bypassNonproxyHandler):Test_NewProxy_ShouldReturn400ForRelativeURLWithNoHostHeaderTest_NewProxy_ShouldHandleRelativeURLWithHostHeaderTest_NewProxy_ShouldReturn500WhenRelativeURLHostIsProxyItselfCloses #774