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

fix: allow special characters in uri params #11788

Conversation

shreemaan-abhishek
Copy link
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
Contributor

@shreemaan-abhishek lint issue

@mikyll
Copy link
Contributor

mikyll commented Nov 28, 2024

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
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
30 checks passed
@shreemaan-abhishek shreemaan-abhishek deleted the allow-spcl-chars-in-parameter branch November 29, 2024 08:20
@mikyll
Copy link
Contributor

mikyll commented Dec 4, 2024

@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