Small bug with forwarder when path changes

[fotinakis]

Wanted to check the best way to fix this bug before starting a PR.

I'm using Forwarder as a hijacking reverse proxy which rewrites the destination URL (so, it doesn't just reverse proxy like normal to a new destination host, it changes the host and path).

I thought I could just do something like replace req.URL:

import (
	"github.com/vulcand/oxy/forward"
	"net/http"
	"net/url"
)

func proxyHandler(w http.ResponseWriter, req *http.Request) {
        // Replace every request with example.com.
	newUrl, _ := url.ParseRequestURI("http://example.com")
	req.URL = newUrl

	fwd, _ := forward.New()
	fwd.ServeHTTP(w, req)
}

func main() {
	http.HandleFunc("/", proxyHandler)
	http.ListenAndServe("127.0.0.1:8080", nil)
}

...except, that attempting to use this always returns a 400 Bad Request:

$ curl -I -x localhost:8080 http://foo/
HTTP/1.1 400 Bad Request

I was able to trace the bug to this line:

oxy/forward/fwd.go

Line 252 in cf724ef

outReq.URL.Opaque = req.RequestURI

which does:

outReq.URL.Opaque = req.RequestURI

The bug: If you call outReq.URL.String() after that line you get: http:http://foo/, which obviously will die.

I've read the docs on how Opaque is used but don't quite understand what the best global fix is for this. I was able to work around it by just removing that line entirely — and now everything works, both normal reverse-proxying and hijacking the request URL. :)

$ curl -I -x localhost:8080 http://foo/
HTTP/1.1 200 OK

But some path tests break (but they also set some Opaque things manually), so I'm not sure what the right fix is here.

[klizhentas]

Thanks for the detailed write-up. I remember I had some bugs because of the way Opaque part works as well. At this stage I'm thinking may be take a more low-level approach to constructing the URI after all to make it explicit.

[klizhentas]

So not use standard URI, but just construct it manually from pieces

[fotinakis]

Sounds good! I'm sure you've seen it but this doc seems to have the rules for how Opaque is used to construct the URL: https://golang.org/pkg/net/url/#URL.String

[fotinakis]

So, maybe the bug here is that you're setting the opaque to RequestURI, which includes the scheme also, so the scheme gets double-included by URL.String(). Not sure if that's the only bug, but it seems to be what's happening.

[dereulenspiegel]

I stumbled upon the same thing here and was also about to create a bug report. Looking at the behavior of the methods of the URL struct I would say setting Opaque when using the URL in HTTP request is a bug and shouldn't happen. Right now I simply set Opaque to an empty value in my ReqRewriter, because otherwise all requests fail.

[leearmstrong]

Was there a fix for this at all?

[fotinakis]

Not an official one, I think it just needs a tiny bit of work to avoid setting outReq.URL.Opaque in some cases. Here's my current workaround, same as @dereulenspiegel above: https://github.com/percy/jackproxy/blob/e67c36b1ca97bc3f32f3e16d3b928eb17c8c97bc/roundtripper.go#L43

[fotinakis]

I believe this might be closed or obsoleted by https://github.com/vulcand/oxy/pull/108/files