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

bugfix:ngx.encode_args() not escaped "|" to "%7c". see: https://en.wi… #542

Closed
wants to merge 4 commits into from

Conversation

goecho
Copy link
Contributor

@goecho goecho commented Aug 5, 2015

…kipedia.org/wiki/Percent-encoding

static uint32_t *map[] =
{ uri, args, html, refresh, memcached, memcached };

static uint32_t *map[] =
Copy link
Member

Choose a reason for hiding this comment

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

Please fix the indentation.

@agentzh
Copy link
Member

agentzh commented Aug 5, 2015

@goecho From what I can see. You're escaping not only | but also quite some others like @, *, (, ), !, and etc. So your commit log message is inaccurate.

It's worth mentioning that even Google Chrome's JavaScript API function encodeURIComponent() does not escape all your new candidates:

> encodeURIComponent(",*()'$!@`|")
"%2C*()'%24!%40%60%7C"

We needn't be harsher than Google Chrome I think :)

@@ -2424,7 +2443,7 @@ ngx_http_lua_process_args_option(ngx_http_request_t *r, lua_State *L,

if (total_escape) {
p = (u_char *) ngx_http_lua_escape_uri(p, key, key_len,
NGX_ESCAPE_URI);
NGX_ESCAPE_URI_COMPONENT);
Copy link
Member

Choose a reason for hiding this comment

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

This line exceeds 80 columns (so do the following changes from you). Please fix them. Thanks.

@agentzh
Copy link
Member

agentzh commented Aug 5, 2015

@goecho Will you add a simple test case to the existing test case to cover these changes? Thanks!

@goecho
Copy link
Contributor Author

goecho commented Aug 10, 2015

@agentzh I had added a simple test case to the existing test case to cover these changes.

100% compatible with What Google Chrome's JavaScript API function encodeURIComponent() does.

see:https://tools.ietf.org/html/rfc2396.

>encodeURIComponent(",$@|`")
"%2C%24%40%7C%60"
>encodeURIComponent("-_.!~*'()")
"-_.!~*'()"

@agentzh
Copy link
Member

agentzh commented Aug 11, 2015

@goecho Cool, thanks. I'll look into this when I have a chance.

thibaultcha added a commit to Kong/kong that referenced this pull request Dec 1, 2015
Percent-encode query args when re-attaching them to the `upstream_uri`.
Since `ngx.encode_args` does not perform percent-encoding on various
reserved characters, this implements a custom `utils.encode_args`
function which uses LuaSocket's `url.encode` function. It tries to mimic
the `ngx.encode_uri` behaviour 100%.

Ideally, `ngx.encode_args` would proceed to the percent-encoding itself (see
openresty/lua-nginx-module#542).

This also makes some perf and style changes.

Fix #749
@agentzh agentzh force-pushed the master branch 2 times, most recently from e7ac10c to cfd4f90 Compare January 31, 2016 19:04
@Tieske
Copy link
Contributor

Tieske commented Aug 18, 2016

What's the status on this? We still got workarounds in place waiting for this to gets fixed....

@toruneko
Copy link

toruneko commented Jul 4, 2017

2 years ago...
What's the status on this?

@toruneko
Copy link

toruneko commented Jul 4, 2017

It seems that the same code with function ngx_escape_uri from file ngx_string.c.

@agentzh
Copy link
Member

agentzh commented Jul 4, 2017

OK, I've merged a slightly modified version of this patch. Sorry for the long delay on my side.

@agentzh agentzh closed this Jul 4, 2017
@snpcp
Copy link

snpcp commented Dec 13, 2021

Hello, I using the ngx.escape_uri and ngx.encode_args function doesn't look right.


This is RFC 3986 : https://datatracker.ietf.org/doc/html/rfc3986#appendix-A
[[
reserved = gen-delims / sub-delims
gen-delims = ":" / "/" / "?" / "#" / "[" / "]" / "@"
sub-delims = "!" / "$" / "&" / "'" / "(" / ")"
/ "*" / "+" / "," / ";" / "="
]]

This is my code:
print(ngx.encode_args({a="1,2!3^4_5;6"}));
print(ngx.escape_uri("1,2!3^4_5"));

This is output:
a=1%2C2!3%5E4_5%3B6
a%3D1%2C2!3%5E4_5%3B6

This is Openresty Version:
resty 0.23
nginx version: openresty/1.15.8.2
built by gcc 4.8.5 20150623 (Red Hat 4.8.5-36) (GCC)
built with OpenSSL 1.0.2k-fips 26 Jan 2017

The expected output is '1,2!3%5E_5;6' by RFC 3986, however that's not true.

@zhuizhuhaomeng
Copy link
Contributor

@snpcp according to the rfc you given, the "," should be escaped.
but the expected output is not escaped. They are contradictory

@snpcp
Copy link

snpcp commented Dec 14, 2021

In the fact, Many software doesn't strictly follow this standard(about gen-delims and sub-delims), And the Openresty look like the same strategy as encodeURIComponent of JavaScript.
That being the case, I accepted. thanks for your response to me.

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.

6 participants