-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Rewrite: use escaped path #5504
Conversation
Signed-off-by: TP-O <letranphong2k1@gmail.com>
Thanks; this will need careful review, and in consideration with #5438. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, I like the change. It's simpler code and all the exists tests still pass, along with two new test cases.
Do the test cases represent the original issue sufficiently?
Additionally, I wonder if @wlonkly could chime in and confirm that this patch works for you.
This is one of the few areas of the code that is more heavily tested, so I guess we can be confident with this, I'm just wary of unexpected edge cases especially if they relate to security issues 🙃
I think these test cases make sure the |
Thanks again for the contribution! If there are any issues reported we'll let you know 👍 |
Fix #5278
Using unescaped path in rewriting makes me confuse which part after
?
is a query if the path contains%3F
, so I thought it would be better to use an escaped path.