contribsys / faktory

Language-agnostic persistent background job server
https://contribsys.com/faktory/
Other
5.73k stars 227 forks source link

Fix URL Path for Proxied Requests #352

Closed lokiwins closed 3 years ago

lokiwins commented 3 years ago

Relates to: https://github.com/contribsys/faktory/issues/351

I found that the issue is when Proxy is configured r.URL.Path is being set to r.RequestURI which results in some escaping behavior by URL causing a bad URL to be formed.

Before Proxy Rewrite
Path: /faktory/retries
URI: /faktory/retries?page=2
After Proxy Rewrite
Path: /retries?page=2
URI: /retries?page=2

Example of this in a request with and without the proxy:

Request with Proxy:
&{GET /retries%3Fpage=2?page=2 HTTP/1.1 1 1 map[Accept:[text/html,application/xhtml+xml,application/xml;q=0.9,image/avif,image/webp,image/apng,*/*;q=0.8,application/signed-exchange;v=b3;q=0.9] Accept-Encoding:[gzip, deflate, br] Accept-Language:[en-US,en;q=0.9] Connection:[close] Cookie:[csrf_token=mIlWhjEHfh5C/IyXl2V8wsOtJCx5clacBMLO1UmvNK8=] Sec-Ch-Ua:[" Not A;Brand";v="99", "Chromium";v="90", "Google Chrome";v="90"] Sec-Ch-Ua-Mobile:[?0] Sec-Fetch-Dest:[document] Sec-Fetch-Mode:[navigate] Sec-Fetch-Site:[none] Sec-Fetch-User:[?1] Upgrade-Insecure-Requests:[1] User-Agent:[Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/90.0.4430.93 Safari/537.36] X-Script-Name:[/faktory]] {} <nil> 0 [] true faktory-ui map[] map[] <nil> map[] 127.0.0.1:49432 /retries?page=2 <nil> <nil> <nil> 0xc000090280}
I 2021-05-05T21:09:11.994Z GET /retries?page=2 2.285647ms
Request No Proxy:
&{GET /retries?page=2 HTTP/1.1 1 1 map[Accept:[text/html,application/xhtml+xml,application/xml;q=0.9,image/avif,image/webp,image/apng,*/*;q=0.8,application/signed-exchange;v=b3;q=0.9] Accept-Encoding:[gzip, deflate, br] Accept-Language:[en-US,en;q=0.9] Connection:[keep-alive] Cookie:[csrf_token=+v6+UvZNkp4NHanHU/mByw5xQBTXAgSFlK5L0lu18fs=] Sec-Ch-Ua:[" Not A;Brand";v="99", "Chromium";v="90", "Google Chrome";v="90"] Sec-Ch-Ua-Mobile:[?0] Sec-Fetch-Dest:[document] Sec-Fetch-Mode:[navigate] Sec-Fetch-Site:[none] Sec-Fetch-User:[?1] Upgrade-Insecure-Requests:[1] User-Agent:[Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/90.0.4430.93 Safari/537.36]] {} <nil> 0 [] false localhost:7420 map[] map[] <nil> map[] 127.0.0.1:49436 /retries?page=2 <nil> <nil> <nil> 0xc00012ed80}
I 2021-05-05T21:09:19.510Z GET /retries?page=2 582.405µs

r.URL.Path should just be the target path without the query params. The resulting Path and RequestURI with the fix:

Before Proxy Rewrite
Path: /faktory/retries
URI: /faktory/retries?page=2
After Proxy Rewrite
Path: /retries
URI: /retries?page=2

Be happy to discuss more or provide any other info 😄

mperham commented 3 years ago

Top notch work, Chris. I really appreciate the time and effort!

I don't have a release date for 1.5.2 with this fix. Are you able to run your own custom binary for the time being?

lokiwins commented 3 years ago

Top notch work, Chris. I really appreciate the time and effort!

I don't have a release date for 1.5.2 with this fix. Are you able to run your own custom binary for the time being?

Yea I can do that. I'll keep an eye for the 1.5.2 release. Thanks!