Skip to content
This repository has been archived by the owner on Jan 24, 2019. It is now read-only.

Encoded slashes are expanded by the proxy #17

Merged
merged 1 commit into from
Mar 19, 2015

Conversation

jehiah
Copy link
Member

@jehiah jehiah commented Mar 17, 2015

To reproduce, hit this URL on the proxy: http://proxy/a%2Fb/

It is passed to the backend as http://backend/a/b/

The expected result is for the backend to receive http://backend/a%2Fb/

This is a problem for Jenkins and is described here: https://wiki.jenkins-ci.org/display/JENKINS/Jenkins+says+my+reverse+proxy+setup+is+broken

@ploxiln
Copy link
Contributor

ploxiln commented Jun 6, 2014

Thanks for the report, that's illuminating. We've actually seen that error message on our own jenkins instance, though we just ignored it (because we were pretty sure the basic host and path parts of rewriting were correct, and it's worked OK for our projects and integrations for many months).

@bradisbell
Copy link

Any chance of a fix for this?

The google_auth_proxy is actually decoding the URL and then doing a 301 redirect to the decoded version. Not only that, but a pair of slashes // encoded as %2F%2F get converted into a single slash /.

@bradisbell
Copy link

I don't know Golang, but I think I found the root cause within Golang's httputil package.

http://golang.org/src/pkg/net/http/httputil/reverseproxy.go#L66

req.URL.Path = singleJoiningSlash(target.Path, req.URL.Path)

When you use req.URL.Path, you get the decoded form. When you use req.RequestURI, you get the unmodified request URI, as it was requested. http://golang.org/pkg/net/http/#Request

I don't really know where to proceed from here. I'm going to dig around to see if anyone has reported this issue in httputil previously.

@ploxiln
Copy link
Contributor

ploxiln commented Oct 16, 2014

It appears to me that you'd have to not use NewSingleHostReverseProxy(), and make your own reverse proxy, to fix this.

@jehiah jehiah added the bug label Nov 8, 2014
@willbryant
Copy link

I guess that's doable if this doesn't get fixed upstream - NewSingleHostReverseProxy is a relatively small method. Looks like singleJoiningSlash is the only non-public method it calls, so not too much code to copy?

@parabolic
Copy link

Hi Is there any fix for this issue, today I've upgraded jenkins and I see the same error although we are still not using it in production so I am not sure whether it will cause problems or not.
Thanks.

@jehiah
Copy link
Member

jehiah commented Mar 17, 2015

@ploxiln mind taking a look at this?

@adrian-gomez I saw you also had a similar patch for this in 3af83e0 so If you have any comments I'd appreciate it.

@adrian-gomez
Copy link

Hi, yeah i had the issue but with gerrit. The fix as someone mentioned is to use a custom proxy instead of NewSingleHostReverseProx for now at least. The real issue is with NewSingleHostReverseProxy that is not doing a great job as a proxy.

I have fixed this in the commit you mentioned i can do a PR ir you want

@jehiah
Copy link
Member

jehiah commented Mar 18, 2015

@adrian-gomez Yeah your commit was helpful. I've updated this PR w/ a patch and I was hoping you could help verify that a build from this branch works as expected for you.

@jehiah jehiah self-assigned this Mar 18, 2015
@adrian-gomez
Copy link

Yeah np, i'll test and comment back before the end of the day.

}
expected := backend.URL + encodedPath
if seen != expected {
t.Errorf("got bad requst %q expected %q", seen, expected)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/requst/request/

@ploxiln
Copy link
Contributor

ploxiln commented Mar 18, 2015

I'm actually not using google_auth_proxy at the present moment, so take this with a grain of salt, but this LGTM.

jehiah added a commit that referenced this pull request Mar 19, 2015
Encoded slashes are expanded by the proxy
@jehiah jehiah merged commit 748247d into bitly:master Mar 19, 2015
@jehiah jehiah deleted the encoded_slashes_17 branch March 20, 2015 03:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

None yet

6 participants