feat(router) add decode_uri_captures parameter #8064

yankun-li-kong · 2021-11-10T09:30:47Z

Add a new parameter decode_uri_captures to decide whether to normalize the captured request URI after route matching.

New feature for #7913

Summary

Add a new parameter decode_uri_captures to decide whether to normalize the captured request URI after route matching.

Only allow setting on or off:

on: Captured request URI decoding is enabled.
off: Captured request URI decoding is disabled.

By default, this value is set as on.

Examples of decode_uri_captures = on behavior:

Assuming that the route path is /plain/(a%2Eb%20c)/(.*) and the request URI is /plain/a%2Eb%20c/a%2Eb%20c, the captured request URI is decoded as /plain/a.b c/a.b c.
Assuming that the route path is /plain/(a.b c)/(.*) and the request URI is /plain/a%2Eb%20c/a%2Eb%20c, the captured request URI is decoded as /plain/a.b c/a.b c.

Examples of decode_uri_captures = off

Assuming that the route path is /plain/(a%2Eb%20c)/(.*) and the request URI is /plain/a%2Eb%20c/a%2Eb%20c, the captured request URI is /plain/a%2Eb%20c/a%2Eb%20c without URI decoding.
Assuming that the route path is /plain/(a.b c)/(.*) and the request URI is /plain/a%2Eb%20c/a%2Eb%20c, the request URI can not match the route path without URI decoding. As a result, the captured request URI is decoded as /plain/a.b c/a.b c.

Full changelog

Add a parameter decode_uri_captures to only decide whether to decode captured request URI or not (Do not have affect for route matching and upstream request)
Use a self parameter(decode_uri_captures) of kong.router to load the new parameter.
Add unit tests

Issues resolved

Fix #7913

CLAassistant · 2021-11-10T09:30:52Z

All committers have signed the CLA.

kong/conf_loader/init.lua

javierguerragiraldez · 2021-11-24T12:17:36Z

kong.conf.default

+                                        # - `on`: Request URI normalization is enabled.
+                                        # - `off`: Request URI normalization is disabled.
+                                        #
+                                        # By default this value is set to `on`.


normalization and decoding are two different things and have different consequences.

this description doesn't explain if the process is done only for route matching or if it affects the upstream request.

thanks for your advice,
now I am considering only using the unnormalized req uri to get the match_t.matches.uri_captures instead.

Then it should not affect route matching and upstream request

Replace normalize_req_uri with normalize_uri_captures to only affect captured request URI.
Please help check again, thanks!

this is still calling the process "normalization", but it's decoding. i guess the function was badly named, but this change makes the error visible in the documentation and parameter name.

Thanks for your review!
Just submit a commit to rename normalize_uri_captures to decode_uri_captures.

yankun-li-kong · 2021-11-28T11:23:36Z

Replace normalize_req_uri with decode_uri_captures to only affect captured request URI.

When decode_uri_captures is set as on, the behavior is the same as previous.

When decode_uri_captures is set as off,
kong will first try to get undecoded uri_captures by using unnormalized req_uri and unnormalized regex like below example-1:
Regex: /plain/(a%2Eb%20c)/(.*)
Request URI: /plain/a%2Eb%20c/a%2Eb%20c
then the uri_captures is /plain/a%2Eb%20c/a%2Eb%20c without URI decoding.

If kong failed to get uri_captures, it will nomalize req_uri and regex to get decoded uri_captures instead
like below example-2:
Regex: /plain/(a.b c)/(.*)
Request URI: /plain/a%2Eb%20c/a%2Eb%20c
The request URI can not match the regex without URI decoding.
As a result, the uri_captures is decoded as /plain/a.b c/a.b c.

Now customers are able to get undecoded uri_captures by setting decode_uri_captures as off
and using a correct regex for a route path.
It means customers have to consider what is the actual request URI passed to Kong,
then try to match that request URI with route path without URI decoding like the example-1.

Add a new parameter normalize-req-uri to decide whether to normalize the request URI or not. New feature for Kong#7913

Use a self parameter(normalize_req_uri) of kong.router to load the new parameter. It will be easier to add a new route parameter to control the URI normalization behavior in the future. New feature for Kong#7913

Modify the 'if condition' for the case self.normalize_req_uri is nil to pass the original unit test. Add unit tests. New feature for Kong#7913

The normalize-req-uri may affect route matching and the upstream request. Replace normalize-req-uri with normalize_uri_captures to only affect captured request URI. If normalize_uri_captures is set to on, the behavior is same as previous. If normalize_uri_captures is set to off, it will try to the capture the request URI without URI normalization. Check more details in normalize_uri_captures part from kong.conf.default. New feature for Kong#7913

yankun-li-kong · 2021-11-28T12:35:22Z

Rebased

Rename normalize_uri_captures to decode_uri_captures New feature for Kong#7913

yankun-li-kong · 2021-12-02T02:58:56Z

Renamed normalize_uri_captures to decode_uri_captures

bungle · 2021-12-07T16:50:26Z

I don't think we want this to be configuration option. We just need to do proper thing.

hbagdi · 2022-10-26T18:09:28Z

@dndx We need to make a decision on if we want to proceed or not here.

hanshuebner · 2023-02-07T13:03:37Z

Is this pull request still relevant with Kong Gateway 3.x?

kikito · 2023-02-09T22:11:04Z

We decided to not go this way, instead we did #8140

yankun-li-kong changed the title ~~[WIP] feat(router) add normalize-req-uri parameter~~ feat(router) add normalize-req-uri parameter Nov 14, 2021

kikito added the core/router label Nov 16, 2021

javierguerragiraldez reviewed Nov 24, 2021

View reviewed changes

yankun-li-kong changed the title ~~feat(router) add normalize-req-uri parameter~~ feat(router) add normalize_uri_captures parameter Nov 28, 2021

yankun-li-kong added 4 commits November 28, 2021 21:31

feat(router) add normalize-req-uri parameter

34b2a3c

Add a new parameter normalize-req-uri to decide whether to normalize the request URI or not. New feature for Kong#7913

feat(router) add normalize-req-uri parameter

9f0f6cb

Use a self parameter(normalize_req_uri) of kong.router to load the new parameter. It will be easier to add a new route parameter to control the URI normalization behavior in the future. New feature for Kong#7913

feat(router) add normalize-req-uri parameter

8a4c7c0

Modify the 'if condition' for the case self.normalize_req_uri is nil to pass the original unit test. Add unit tests. New feature for Kong#7913

yankun-li-kong force-pushed the feat/add-normalize-req-uri-param branch from 7edbb03 to f8ceec8 Compare November 28, 2021 12:32

feat(router) add decode_uri_captures parameter

102b19f

Rename normalize_uri_captures to decode_uri_captures New feature for Kong#7913

yankun-li-kong changed the title ~~feat(router) add normalize_uri_captures parameter~~ feat(router) add decode_uri_captures parameter Dec 2, 2021

bungle added the pr/do not merge label Dec 7, 2021

guanlan requested a review from a team as a August 25, 2022 01:38

guanlan force-pushed the master branch from f089a5b to 62d113d Compare August 25, 2022 01:40

hanshuebner added the pending author feedback Waiting for the issue author to get back to a maintainer with findings, more details, etc... label Feb 7, 2023

kikito closed this Feb 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(router) add decode_uri_captures parameter #8064

feat(router) add decode_uri_captures parameter #8064

yankun-li-kong commented Nov 10, 2021 •

edited

Loading

CLAassistant commented Nov 10, 2021 •

edited

Loading

javierguerragiraldez Nov 24, 2021

yankun-li-kong Nov 25, 2021

yankun-li-kong Nov 28, 2021

javierguerragiraldez Dec 1, 2021

yankun-li-kong Dec 2, 2021

yankun-li-kong commented Nov 28, 2021 •

edited

Loading

yankun-li-kong commented Nov 28, 2021

yankun-li-kong commented Dec 2, 2021 •

edited

Loading

bungle commented Dec 7, 2021 •

edited

Loading

hbagdi commented Oct 26, 2022

hanshuebner commented Feb 7, 2023

kikito commented Feb 9, 2023

feat(router) add decode_uri_captures parameter #8064

feat(router) add decode_uri_captures parameter #8064

Conversation

yankun-li-kong commented Nov 10, 2021 • edited Loading

Summary

Full changelog

Issues resolved

CLAassistant commented Nov 10, 2021 • edited Loading

javierguerragiraldez Nov 24, 2021

Choose a reason for hiding this comment

yankun-li-kong Nov 25, 2021

Choose a reason for hiding this comment

yankun-li-kong Nov 28, 2021

Choose a reason for hiding this comment

javierguerragiraldez Dec 1, 2021

Choose a reason for hiding this comment

yankun-li-kong Dec 2, 2021

Choose a reason for hiding this comment

yankun-li-kong commented Nov 28, 2021 • edited Loading

yankun-li-kong commented Nov 28, 2021

yankun-li-kong commented Dec 2, 2021 • edited Loading

bungle commented Dec 7, 2021 • edited Loading

hbagdi commented Oct 26, 2022

hanshuebner commented Feb 7, 2023

kikito commented Feb 9, 2023

yankun-li-kong commented Nov 10, 2021 •

edited

Loading

CLAassistant commented Nov 10, 2021 •

edited

Loading

yankun-li-kong commented Nov 28, 2021 •

edited

Loading

yankun-li-kong commented Dec 2, 2021 •

edited

Loading

bungle commented Dec 7, 2021 •

edited

Loading