fix(tools.uri) normalization decodes as much as possible

[bungle]

We decide to let normalize function to decode URL-encoded string as much as possible.

PLEASE REFERER TO: #8140 (comment)

Issues resolved

Fix #7913, FTI-2904

Outdated discussion:

---

This is alternative to PR #8139 where we actually fix the normalization function to not do excessive percent-decoding on normalization.

When we added normalization kong.tools.uri.normalize, that function does percent-decoding on everything, except for the reserved characters.

That means that we basically percent-decode more than just the ranges of ALPHA (%41–%5A and %61–%7A), DIGIT (%30–%39), hyphen (%2D), period (%2E), underscore (%5F), or tilde (%7E). (so called Unreserved Characters)

Alternative Implementation: See #8139

[flrgh]

Hmm. RFCs are always a chore to interpret. It seems like the original mistake was that normalization was implemented with the assumption that unreserved chars == (all chars - reserved chars), which, as you know, turns out to be incorrect (according to the RFC).

IMO instead of over-decoding and re-encoding we should just fix the logic to be more selective in our decoding, according to the chars in section 2.3:

unreserved = ALPHA / DIGIT / "-" / "." / "_" / "~"

I put a draft/poc of this in another branch (fix/uri-normalize-unreserved 6313dd2). Maybe take a look and tell me what you think?

[bungle]

Hmm. RFCs are always a chore to interpret. It seems like the original mistake was that normalization was implemented with the assumption that unreserved chars == (all chars - reserved chars), which, as you know, turns out to be incorrect (according to the RFC).

IMO instead of over-decoding and re-encoding we should just fix the logic to be more selective in our decoding, according to the chars in section 2.3:

unreserved = ALPHA / DIGIT / "-" / "." / "_" / "~"

I put a draft/poc of this in another branch (fix/uri-normalize-unreserved 6313dd2). Maybe take a look and tell me what you think?

@flrgh yes, we discussed this with @dndx. The thing is that I am not sure which one is better. More selective percent decoding or not. I am not sure can there ever be things already percent decoded in ngx.var.request_uri or not. Do you? If there could be, then those need to be normalized back to percent encoded form. Similar question goes to route.paths. Also as this is kong.tools.uri.normalize, it should support normalizing this: "/ä/a/%2e./a%2E%5f%99%af" should produce "/%C3%A4/a._%99%AF" which is done decoding, process dots, encoding approach I took here (it is difficult to do in one pass). The performance of this PR should be same as before, aka it does not do more work than before. We have already seen usages on other PRs, like yours: https://github.com/Kong/kong/pull/8129/files (if this is about to be tool for normalization it needs to do correct thing).

Also the escaping includes % in non-escape chars which is rather questionable:
https://github.com/Kong/kong/blob/master/kong/tools/uri.lua#L38

[kikito]

I have spoken with @bungle and for now we're removing this from the 2.7 milestone

[bungle]

If someone wants to work or try different approaches, here is a list that need to be taken in account:

definitions:
reserved chars: ! * ' ( ) ; : @ & = + $ , / ? % # [ ]
unreserved: - _ . ~
alphanumeric: a b c d e f g h i j k l m n o p q r s t u v w x y z A B C D E F G H I J K L M N O P Q R S T U V W X Y Z 0 1 2 3 4 5 6 7 8 9
other: * (meaning any char that is not mentioned above)

dot processing:
/. AND /./
/.. AND /../
/%2e AND /%2e/
/.%2e AND /.%2e/
/.%2E AND /.%2E/
/%2e%2E AND /%2e%2E/
etc.

merge slashes (this is generally thought to be good thing to do, but it might change semantics in some cases):
//
/a//b
etc.

given this:
/ä/a/%2e./a%2E//a/%2e/./a/../a/%2e%2E/%5f%99%af%2f%2F

should normalize (I think) to this:
/%C3%A4/a./a/_%99%AF%2F%2F

What happens there?
ä -> being percent encoded to %C3%A4
%2e -> being uppercased to %2E
// -> slashes being merged
%5f -> being percent decoded to _
/../ -> dot segment being processed
/%2e/ -> dot segment being processed
/a%2E/ -> dot being percent decoded to /a./

In general:

• unreserved and alphanumeric should be percent decoded
• reserved should be kept as is (no percent decoding, no percent encoding) BUT percent encoded forms should be turned to uppercase
• rest should be percent encoded

Then you need to implement dot processing.

Current issue with kong.tools.uri.normalize (as a generic tool):

• it over percent decodes (basically it percent decodes everything)
• it does not percent encode

[bungle]

I looked at it, and it looks good to me. @flrgh, do you agree?

[flrgh]

Code-wise, I reviewed and left a couple nitpicks. The table.new thing should probably be fixed, but the other comment on chars_to_decode is mostly just a readability gripe, so not a blocker if anyone disagrees with me about it.

Behavior-wise, there's been a whole bunch of activity on the path handling/normalization discussion since I last participated, so as long as everyone else here is in agreement about this change, it gets my 👍.