fix(plugins) urldecode path fragments which accept spaces

[hishamhm]

In some plugins (basic-auth, hmac-auth, jwt, key-auth, oauth2) we have path fragments which represent plugin fields that may contain spaces in their definitions. For example, if a username for basic-auth contains a space, for example, then it is reasonable to perform this request:

http ":8001/consumers/bob/basic-auth/John Doe/"

which, due to URL-encoding, results in a request to
http://localhost:8001/consumers/bob/basic-auth/John%20Doe/ --
we are currently returning 404 for those queries.

This patch performs URL-decoding on those path fragments, using ngx.unescape_uri.

It also modifies the test suites for these plugins using values with spaces for the relevant fields, and using the URL-encoded version of these fields when querying the Admin API.

Fixes #3247.

[p0pr0ck5]

Apologies if I'm missing something, but any reason not to use https://github.com/openresty/lua-nginx-module#ngxunescape_uri?

[hishamhm]

@p0pr0ck5 the only reason was my lack of knowledge of that function! :) I'll modify the PR accordingly, thank you for the heads-up!

[hishamhm]

Updated to use ngx.unescape_uri!

[thibaultcha]

Could we double check that all of the reserved characters from RFC 3986 are percent-decoded? In the past, we've had issues with ngx_lua's encoding/decoding APIs (ngx.encode_args() specifically, but those APIs rely on the same encoding/decoding tables iirc) not encoding characters such as ,$@|, which are characters that are likely to be used in a key/password/email address...).

Not only did they not encode/decode all reserved characters, but we also encountered pushback back then to fix this in ngx_lua (I recall another report/contribution some months later that maybe made it in?). Those APIs were also not encoding/decoding the same set of characters as that of NGINX itself, which made things even worse, and forced us to roll out our own flavor (based on url.escape).

I did not go again through the entire threads (and they are quite old so I don't remember all the details), but see:

#749
openresty/lua-nginx-module#1124
https://github.com/openresty/lua-nginx-module/search?p=1&q=escape&type=Issues&utf8=%E2%9C%93

Again, just wanting to make sure that all reserved characters are properly decoded - we should probably have a test for it.

[hishamhm]

@thibaultcha tests amended to verify that all reserved characters from RFC 3986 decode properly (good news: they do)

[thibaultcha]

@hishamhm Sweet!

[thibaultcha]

Great stuff 👍

The only issue I am seeing with this is that we aren't testing any endpoint with non-percent-encoded paths anymore, which kinds of feels like we are not providing a regression for this? How about adding a couple tests, for values without any reserved characters or values that are not properly encoded, if anything only to assert the behavior?
I would have maybe tried to organize those tests to be mostly running without special characters still, and then adding a few edge-cases with such those special characters. Mostly because if the latter is broken and fails, doesn't cause the former to fail as well, thus making the culprit more obvious. But well, no biggie

[hishamhm]

Oh, I see what you mean! I considered the tests to be covering the regression aspect because running these tests in the pre-fixed version would mean they fail and in the fixed version they succeed, but indeed if a future bug is specific to the encoding having all the tests exercise the encoding may make it harder to spot.

What would be a sufficient way of covering this? Adding a new entry with a simpler name/key like foo on each plugin testsuite and making one GET request for it?

[thibaultcha]

What would be a sufficient way of covering this? Adding a new entry with a simpler name/key like foo on each plugin testsuite and making one GET request for it?

I think this would be best indeed (we can loop over the GET tests with 2 values, one with and one without special characters maybe)

[thibaultcha]

Awesome!