Skip to content

fix: allow special characters in uri params#11788

Merged
shreemaan-abhishek merged 2 commits into
apache:masterfrom
shreemaan-abhishek:allow-spcl-chars-in-parameter
Nov 29, 2024
Merged

fix: allow special characters in uri params#11788
shreemaan-abhishek merged 2 commits into
apache:masterfrom
shreemaan-abhishek:allow-spcl-chars-in-parameter

Conversation

@shreemaan-abhishek

Copy link
Copy Markdown
Contributor

Description

The underlying lua-resty-radixtree library used a regex that would cause route matching to fail when there are special characters in the uri parameters. This has been fixed in the radixtree router library, so we upgrade the version and add a test case that ensures special characters are allowed in uri parameters when using radixtree_uri_with_parameter.

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

@dosubot dosubot Bot added size:L This PR changes 100-499 lines, ignoring generated files. bug Something isn't working labels Nov 28, 2024
@Revolyssup

Copy link
Copy Markdown
Contributor

@shreemaan-abhishek lint issue

@mikyll

mikyll commented Nov 28, 2024

Copy link
Copy Markdown
Contributor

I tested this change and it doesn't seem to work for %2F (encoded slash or solidus /)...

In my opinion we should add more test cases for different special characters. I ran some local test on a subset of URL-encoded special characters with api7/lua-resty-radixtree v2.9.1 and v2.9.2 and that's what I got:

encoded char value v2.9.1 v2.9.2
%20 🔴 🟢
%21 ! 🟢 🟢
%22 " 🔴 🟢
%23 # 🔴 🟢
%24 $ 🟢 🟢
%25 % 🟢 🟢
%26 & 🟢 🟢
%27 ' 🟢 🟢
%28 ( 🟢 🟢
%29 ) 🟢 🟢
%2A * 🟢 🟢
%2B + 🟢 🟢
%2C , 🟢 🟢
%2D - 🟢 🟢
%2E . 🟢 🟢
%2F / 🔴 🔴 ❗❗
%3A : 🟢 🟢
%3B ; 🟢 🟢
%3C < 🔴 🟢
%3D = 🟢 🟢
%3E > 🔴 🟢
%3F ? 🔴 🟢
%40 @ 🟢 🟢
%5B [ 🔴 🟢
%5C \ 🔴 🟢
%5D ] 🔴 🟢
%5E ^ 🔴 🟢
%5F _ 🟢 🟢
%60 ` 🔴 🟢
%7A { 🟢 🟢
%7B | 🔴 🟢
%7C } 🔴 🟢
%7D ~ 🔴 🟢

%2F (/) appears to be the only character that still breaks the radixtree parser 🤔

@shreemaan-abhishek

Copy link
Copy Markdown
Contributor Author

@mikyll, this is expected behaviour as / is a delimiter for urls. If / is allowed as a special character, there will be no regularity in the boundary of the url parameter. Consider the following example:

route uri: /v1/:id/products/:type/list
request uri: /v1/123/products/footwear/list

If / is also accepted as a special character then the id url parameter would be incorrectly evaluated to 123/products/footwear/list.

@shreemaan-abhishek
shreemaan-abhishek merged commit 9f80311 into apache:master Nov 29, 2024
@shreemaan-abhishek
shreemaan-abhishek deleted the allow-spcl-chars-in-parameter branch November 29, 2024 08:20
@mikyll

mikyll commented Dec 4, 2024

Copy link
Copy Markdown
Contributor

@shreemaan-abhishek thank you for the reply, but I think you misunderstand my previous comment... I agree that / should only be treated as a path separator, it wouldn't make sense otherwise and it would also lead to security issues.
However, that's exactly the reason why we need URL-encoding: when we URL-encode ("percent-encoding") characters, they are treated as raw data.

Currently, APISIX with radixtree_uri_with_parameter doesn't fully support this, since the router fails to route requests that contain %2F in a path parameter and returns 404: Not Found.

This PR fixed the problem for many special characters but not for %2F (encoded /)!
See the comparison table above.

Please see issue #11810 for further information.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants