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

fix the misinterpretation of the % character in a query string #1998

Closed
wants to merge 2 commits into from

Conversation

eilgin
Copy link

@eilgin eilgin commented Jan 18, 2017

Note: new features are to be opened against next, hotfixes against master.

Summary

We've observed that Kong doesn't handle the '%' character when it's not part of a percent-encoding value (e.g. "foo=%bar%"). Since encode_args is only used in kong/core/handler.lua and that query params are already decoded and moreover since we cannot at the same time deal with percent-encoded value and a decoded value containing the % character without misinterpret it, we've remove the uncesseray unescape call as we judged that encode_args should be called with the raw option in the presence of percent-encoded value.

Full changelog

  • [fix] remove the unescape call in encode_args
  • [tests] handling of the % character with the encode_args method

Issues resolved

Fix #1975
Link #1480 (comment)

thomas jouannic added 2 commits January 18, 2017 16:29
Since we cannot at the same time encoded correctly query parameters
that were properly unescaped before and those which aren't because
in this case we cannot predicted if the `%` character was part of
the percent-encoding or just a character (e.g. "foo=%bar%")
in the presence of the `%` character.
@thibaultcha
Copy link
Member

I believe this makes sense, thanks for looking into it. The changes and tests look ok.

However, Kong 0.10 does not proxy requests in the exact same way (it is now more transparent and avoids manipulating the request URI on the Lua land) and thus does not call encode_args() anymore, making this patch unnecessary. Would you mind then trying out Kong 0.10.0rc1 (or the upcoming RC2 today or this week at the most) and let us know if the behavior is correct?

Ultimately, I think this patch is still necessary if utils.encode_args() is still preferred over ngx.encode_args(), and that might be the case in the request-transformer plugin from the top of my head, among others, maybe.

@thibaultcha
Copy link
Member

This can also make it into 0.9.9 @thefosk

@thibaultcha
Copy link
Member

I have created #2040, which includes a squashed version of your fix (with slight modifications in the commit message) and some new regression tests to ensure the URL encoding/decoding behavior by the proxy.

I am closing this and we'll be focused on approving #2040 instead. Thank you!

@thibaultcha thibaultcha closed this Feb 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong decoding for some query strings
2 participants