-
Notifications
You must be signed in to change notification settings - Fork 109
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
Prevent URL in Link
header from including invalid characters
#1802
Prevent URL in Link
header from including invalid characters
#1802
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## trunk #1802 +/- ##
==========================================
+ Coverage 65.92% 65.97% +0.04%
==========================================
Files 88 88
Lines 6885 6895 +10
==========================================
+ Hits 4539 4549 +10
Misses 2346 2346
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Thanks for the PR! I left a couple alternative suggestions.
Please add some test coverage for the lines not covered be tests.
What about when an internationalized domain name is used? Couldn't this also cause problems with encoding? |
Additionally, on multisite subdirectory installs, in theory the path before
|
@AhmarZaidi Hey, are you intending to pick this up again? |
@westonruter Yes, I'll be working on the changes right away. |
@westonruter I've implemented the changes: If the path contains any non-ascii characters then we encode the whole URL. However we can use |
What you did looks good. Could you add some test cases for international domain names and non-ASCII paths to ensure the expected output? |
@westonruter The old solution was encoding the Potential Issue: This solution converts the slashes ( If we use something like:
Then Please feel free to let me know if we should do any further changes to the approach. |
That looks good! |
Implemented the changes. |
So this is no longer a draft and is ready for review, correct? |
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @amitay-elementor. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
plugins/optimization-detective/tests/test-class-od-link-collection.php
Outdated
Show resolved
Hide resolved
plugins/optimization-detective/tests/test-class-od-link-collection.php
Outdated
Show resolved
Hide resolved
Could you update the description based on the latest revisions? |
Link
header from including invalid characters
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.
Thank you!
@felixarntz Anything we missed here?
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.
Thank you for the PR @AhmarZaidi, LGTM!
Summary
This PR addresses an issue where non-ASCII characters in URL filenames caused HTTP headers to break reverse proxies and violate standards. The solution encodes only the filename part of URLs, ensuring compliance with ISO-8859-1 character requirements. This change maintains URL integrity while preventing potential issues with reverse proxies.
Example URL :
https://testsite.com/wp-content/uploads/2025/01/חנות-scaled.avif
Corrected URL:
https://testsite.com/wp-content/uploads/2025/01/%D7%97%D7%A0%D7%95%D7%AA-scaled.avif
Fixes #1775
Relevant technical choices