Fix URL signing api for URLs containing special characters - #12435
Fix URL signing api for URLs containing special characters#12435ErykKul wants to merge 31 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Test Results404 tests 389 ✅ 32m 52s ⏱️ Results for commit 4fef358. ♻️ This comment has been updated with latest results. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
|
@qqmyers Heads up before QA: a few more commits landed since my comments above. While testing against a real client I also made validation verbatim-first — the signature is checked against the raw request URI first, with the URL-decoded check kept as fallback — so a URL signed exactly as it will be used now works as received, and everything that validated before still validates. A final review pass added a couple of fixes on top: crafted |
This comment has been minimized.
This comment has been minimized.
|
@ErykKul - can we coordinate in slack and discuss this PR? |
|
@ErykKul just a reminder that we'd like your input and code freeze for 6.12 is next Thursday! 🥶 |
|
We think only @ErykKul is actively using this but we'd like to get it into 6.12. |
|
@pdurbin I am on it. Trying to wrap it up. Not ready yet, I will try to have it ready by tomorrow evening. |
…eserved params instead of stripping, constant-time signature check #12435
…, forging without the server secret fails #12435
This comment has been minimized.
This comment has been minimized.
…chronized, no per-node cache), 400s instead of 500s for smuggled or manifest reserved params, constructor injection, coverage tests #12435
This comment has been minimized.
This comment has been minimized.
2 similar comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…h, including bulk PUT #12435
This comment has been minimized.
This comment has been minimized.
|
|
📦 Pushed preview images as 🚢 See on GHCR. Use by referencing with full name as printed above, mind the registry name. |


What this PR does / why we need it:
Bug fix; URL signing for URLs containing special characters broken in 6.10, this PR fixes the introduced bug.
Which issue(s) this PR closes:
Issue not created, bug fixed directly here.
Special notes for your reviewer:
Added tests to prevent regression. Also, API requires now the signing secret to be set, otherwise it will not work.
Suggestions on how to test this:
No special testing needed, the unit tests should be sufficient.
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
No
Is there a release notes update needed for this change?:
Yes, included.
Additional documentation:
No