Fix: getQueryArgs discards everything after the second = in a query argument value - #81066
Fix: getQueryArgs discards everything after the second = in a query argument value#81066hbhalodia wants to merge 3 commits into
Conversation
|
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: @konnen916. 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. |
|
The fix reads correctly to me. I checked out the branch and ran the package suite against it. One gap worth filling: there is no test for the round trip through Both of these already pass on your branch, so this is a regression guard rather than anything wrong with the change: it( 'should not truncate an existing value containing equals signs', () => {
const url =
'https://andalouses.example/beach?cursor=eyJvZmZzZXQiOjIwfQ==';
const args = { sun: 'true' };
expect( addQueryArgs( url, args ) ).toBe(
'https://andalouses.example/beach?cursor=eyJvZmZzZXQiOjIwfQ%3D%3D&sun=true'
);
} );
it( 'should not truncate a nested URL passed as an existing value', () => {
const url = 'https://andalouses.example/beach?redirect=/watch?v=abc';
const args = { sun: 'true' };
expect( addQueryArgs( url, args ) ).toBe(
'https://andalouses.example/beach?redirect=%2Fwatch%3Fv%3Dabc&sun=true'
);
} );The four new tests all sit in the Minor one too: the changelog entry links to |
|
Warning: Type of PR label mismatch To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task. |
|
I wish I still wonder if we could keep the split and assign the value parts as a spread like Something like this isn't the prettiest code, but it's close to what we have: let [ key, ...valueParts ] = keyValue.split( '=' );
let value = valueParts.join( '=' );
key = key && safeDecodeURIComponent( key );
value = value && safeDecodeURIComponent( value );Although I think the proposed implementation has some advantages in terms of doing the least work necessary to manipulate and decode the parts, which seems nice 👍 I'd be curious about your feedback to @konnen916 's comment, but otherwise this looks good to me. |
What?
Closes #81062
Why?
@wordpress/urlpackage.How?
=sign correctly.Testing Instructions
wp.url.getQueryArgs( 'https://example.com/?a=1&b=x=y&c=3' )or multiple cases mentioned in issue.Testing Instructions for Keyboard
Screenshots or screencast
Use of AI Tools
AI Summary
What?
Closes #81062
getQueryArgs()splits each query argument on the first=only. Previously it split on every=and kept just the first two segments, silently discarding everything after the second one.Why?
packages/url/src/get-query-args.tsparsed eachkey=valuepair with:'b=x=y'.split( '=' )yields[ 'b', 'x', 'y' ], and the destructuring reads only the first two entries — so'y'was dropped. Any value legitimately containing an unencoded=was truncated:?b=x=y{ b: 'x' }{ b: 'x=y' }?token=eyJhbGciOiJIUzI1NiJ9=={ token: 'eyJhbGciOiJIUzI1NiJ9' }{ token: 'eyJhbGciOiJIUzI1NiJ9==' }?redirect=/watch?v=abc{ redirect: '/watch?v' }{ redirect: '/watch?v=abc' }?=orphan{ orphan: '' }{}The blast radius is wider than one function, because
getQueryArgs()is the shared parser:addQueryArgs()merges existing args viagetQueryArgs(), so appending a parameter to a URL rewrote the truncated value back out — silent data loss rather than a read-only glitch.@wordpress/api-fetchcallsaddQueryArgs()from its defaultuser-localemiddleware, so every request whose URL carried a=in a value was affected.getQueryArg(),hasQueryArg(), andremoveQueryArgs()all delegate to it too.Base64 (
==padding), JWTs, and redirect targets are exactly the values that carry=, so this hit auth tokens and redirect URLs hardest.The last row is the secondary defect from the issue:
.filter( Boolean )dropped the empty string from[ '', 'orphan' ], leaving[ 'orphan' ], which promoted a keyless value into a key.How?
Find the first
=withindexOfand slice around it, instead of splitting on all of them:Dropping
.filter( Boolean )is what fixes the orphan case: an empty key now reaches the pre-existingif ( key )guard and the malformed pair is skipped.valuemoved inside that guard because it is only needed there.packages/url/src/get-query-args.ts=split; keyless pairs ignoredpackages/url/src/test/index.jsgetQueryArgsdescribepackages/url/CHANGELOG.mdBehavior deliberately preserved
=at all —?foostill gives{ foo: '' }.hasValueisfalse, so the empty-string default applies exactly as the old destructuring default did.=—?foo=still gives{ foo: '' }, now because the slice is empty rather than because the filter removed it.?&fooand?still skip the blanks, covered by the existing "should gracefully handle empty keys and values" test.foo[]=,foo[0]=,user[name]=, and the percent-encodeduser%5Bname%5D=forms are untouched. The key is still decoded before being split on[, which is what makes the encoded variants work.safeDecodeURIComponent, so?baz=%E0%A4%Areturns the raw string instead of throwing.buildQueryString()encodes=in values as%3D, soaddQueryArgs()output stays correctly encoded, and the two "reverses buildQueryString" tests are unaffected.No API surface, signature, or type changed —
QueryArgsand the exported signature are identical.Testing Instructions
Unit tests:
Manual verification in the browser console
On any wp-admin screen (
@wordpress/urlis exposed aswp.url):Every line above except the
addQueryArgsone can also be checked againsttrunkto see the truncation.