Fix URL encoding in Link HTTP response header #1907
Conversation
Signed-off-by: Shyamsundar Gadde <[email protected]>
Signed-off-by: Shyamsundar Gadde <[email protected]>
ShyamGadde
added
[Type] Bug
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## trunk #1907 +/- ##
==========================================
- Coverage 69.59% 69.59% -0.01%
==========================================
Files 86 86
Lines 6983 6982 -1
==========================================
- Hits 4860 4859 -1
Misses 2123 2123
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -489,7 +489,7 @@ public function data_provider_to_test_add_link(): array { | |||
| 'expected_html' => ' | |||
| <link data-od-added-tag rel="preload" href="https://example.com/חנות/wp-content/uploads/2025/01/example.jpg?ver=1+2" as="image"> | |||
| ', | |||
| 'expected_header' => 'Link: <https://example.com/%D7%97%D7%A0%D7%95%D7%AA/wp-content/uploads/2025/01/example.jpg?ver=1%202>; rel="preload"; as="image"', | |||
| 'expected_header' => 'Link: <https://example.com/%D7%97%D7%A0%D7%95%D7%AA/wp-content/uploads/2025/01/example.jpg?ver=1+2>; rel="preload"; as="image"', | |||
There was a problem hiding this comment.
Could you add test cases for the example URLs I have in the issue description as well?
Signed-off-by: Shyamsundar Gadde <[email protected]>
|
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 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. |
| // Encode characters not allowed in a URL per RFC 3986 (anything that is not among the reserved and unreserved characters). | ||
| $encoded_url = (string) preg_replace_callback( | ||
| '/[^A-Za-z0-9\-._~:\/?#\[\]@!$&\'()*+,;=]/', | ||
| '/[^A-Za-z0-9\-._~:\/?#\[\]@!$&\'()*+,;=%]/', |
There was a problem hiding this comment.
One thing that comes to mind. What if someone has a URL like: https://example.com/price.jpg?amount=100%-free. What happens here? Will browsers force that % to get encoded as if they encountered non-ASCII characters?
There was a problem hiding this comment.
After looking into how WordPress handles standalone % characters in URLs via esc_url_raw(), I found that:
- WordPress allows standalone
%characters in URLs since they are part of the allowed character set. - It only strips truly invalid characters, not malformed percent encodings.
Additionally, modern browsers (tested on Edge, Chrome, and Firefox) accept URLs with standalone % characters. For example, given a URL like https://example.com/price.jpg?amount=100%-free:
- Both browsers and WordPress preserve the standalone
%. - The image loads correctly without errors.
- The URL is not rejected as invalid.
I considered encoding standalone % with something like:
preg_replace_callback(
'/%(?![0-9A-Fa-f]{2})/',
static function () {
return '%25';
},
$url
);However, this would create inconsistency between how the URL appears in HTML vs. HTTP headers, potentially causing the same double-request issue we're trying to fix.
There was a problem hiding this comment.
I can confirm that in trunk right now, a bare % is resulting in the incorrect behavior where the % is getting percent-encoded:
Warning
The resource https://upload.wikimedia.org/wikipedia/commons/8/8d/American_bison_k5680-1.jpg?amount=100%25-free was preloaded using link preload but not used within a few seconds from the window's load event. Please make sure it has an appropriate as value and it is preloaded intentionally.
Whereas on this branch it is working correctly with only one image is getting preloaded:
And no warning appears in the console.
Summary
Fixes #1906
Relevant technical choices
urldecode()to prevent double encoding.encode_url_for_response_header()to preserve existing percent-encoded sequences while properly encoding invalid characters.