Skip to content
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

Kong is adding URL encoding before sending upstream #2512

Closed
tecnobrat opened this issue May 10, 2017 · 13 comments · Fixed by #2519
Closed

Kong is adding URL encoding before sending upstream #2512

tecnobrat opened this issue May 10, 2017 · 13 comments · Fixed by #2519
Labels

Comments

@tecnobrat
Copy link

Summary

In Kong 0.10.2 it is adding additional URL encoding before sending a request upstream.

Steps To Reproduce

  1. Make a request to kong with /auth%7C123 (/auth|123, url-encoded).
  2. Kong will make the request to your upstream as: /auth%257C123

Additional Details & Logs

  • Kong version 0.10.2

This was not the case in Kong 0.9.9, it didn't further encode these URLs.

@thibaultcha
Copy link
Member

Hi,

Thanks for the report. That might indeed very well be the case, we'll investigate.

@bungle Did you see this?

@thibaultcha thibaultcha added the task/needs-investigation Requires investigation and reproduction before classifying it as a bug or not. label May 10, 2017
@bungle
Copy link
Member

bungle commented May 11, 2017

The relevant tests:
https://github.com/Mashape/kong/blob/master/spec/01-unit/11-router_spec.lua#L819-L833
https://github.com/Mashape/kong/blob/master/spec/02-integration/05-proxy/01-router_spec.lua#L175-L184

The possible bug:
https://github.com/Mashape/kong/blob/master/kong/core/router.lua#L687

I guess that you may need to url decode the uri before supplying it to ngx.req.set_uri.

Docs say:

Rewrite the current request's (parsed) URI by the uri argument. 

So either it is a bug in OpenResty, or it is expected behavior and requires you to decode before setting it. I suspect it is the latter.

@bungle
Copy link
Member

bungle commented May 11, 2017

@tecnobrat If you have time, mind you changing the line:
https://github.com/Mashape/kong/blob/master/kong/core/router.lua#L687

that has code:

ngx.req.set_uri(new_uri)

with

ngx.req.set_uri(ngx.unescape_uri(new_uri))

And report back if it had any effect.

@tecnobrat
Copy link
Author

@bungle I'll try giving it a shot, I'll need to do some docker-fu and report back :)

@tecnobrat
Copy link
Author

Hey @bungle yep, that worked.

@bungle bungle added task/bug and removed task/needs-investigation Requires investigation and reproduction before classifying it as a bug or not. labels May 11, 2017
@3dbrows
Copy link

3dbrows commented May 12, 2017

@bungle Your fix also worked for me, thanks.

@thibaultcha
Copy link
Member

I have created a regression test at #2519, but this test exposes that the currently proposed fix is incomplete. I believe that with this fix, Kong is making the upstream request with unescaped URIs (/auth|123) instead. Could you check? Thanks!

@p0pr0ck5
Copy link
Contributor

@thibaultcha isn't the point here that the upstream request should be unescaped? @tecnobrat isn't that what you're looking for?

@thibaultcha
Copy link
Member

No, the point is that Kong needs to be as transparent as possible, and an escaped request from downstream should be forwarded upstream as an escaped request as well. This is the expectation we set when we qualify Kong as a "transparent proxy".

@tecnobrat
Copy link
Author

@thibaultcha is correct.

The request was pre-escaped. It should be escaped when it hits the resulting upstream. Currently its escaping it TWICE. But it most certainly should not be unescaped completely.

However the request was made to kong, it should be made to the upstream.

@thibaultcha
Copy link
Member

Thanks for confirming that @tecnobrat.

However the request was made to kong, it should be made to the upstream.

That is indeed the expectation one might have, and certainly the one we'd like to promise, but we are limited by the current APIs that Nginx/OpenResty are offering us. We are investigating fixes, but so far we don't have a definitive one.

@pklingem
Copy link

pklingem commented Jun 1, 2017

is there any news on this? it's preventing us from upgrading from 0.9.x.

thibaultcha added a commit that referenced this issue Jun 6, 2017
* The router now returns a 4th value: the URI to use for the upstream
  call.
* The `proxy_pass` directive now inlines a new `$upstream_uri` variable
  which contains a raw string with both the Nginx `$request_uri` and
  `$args` variables.
* Update the router value return test
* New test to ensure we proxy the request's querystring
* New regression test to ensure we do not double percent-encode request
  URIs.

Fix #2512
@thibaultcha
Copy link
Member

I just updated #2519 with a fix that ensures we transparently proxy request URIs (without further escaping/un-escaping). Could you give it a try on your side and let us know? Thanks!

thibaultcha added a commit that referenced this issue Jun 9, 2017
* The router now returns a 4th value: the URI to use for the upstream
  call.
* The `proxy_pass` directive now inlines a new `$upstream_uri` variable
  which contains a raw string with both the Nginx `$request_uri` and
  `$args` variables.
* Update the router value return test
* New test to ensure we proxy the request's querystring
* New regression test to ensure we do not double percent-encode request
  URIs.

Fix #2512
thibaultcha added a commit that referenced this issue Jun 9, 2017
* The router now returns a 4th value: the URI to use for the upstream
  call.
* The `proxy_pass` directive now inlines a new `$upstream_uri` variable
  which contains a raw string with both the Nginx `$request_uri` and
  `$args` variables.
* Update the router value return test
* New test to ensure we proxy the request's querystring
* New regression test to ensure we do not double percent-encode request
  URIs.

Fix #2512
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants