Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Avoid erroring when getQueryArgs processes a malformed URL #45561

Merged
merged 8 commits into from
Jan 13, 2023
Merged

Avoid erroring when getQueryArgs processes a malformed URL #45561

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a85e3d9
Ignore malformed url params in get-query-args function
kozer Nov 4, 2022
4669991
Merge branch 'WordPress:trunk' into fix/get_query_args_ignore_malformed
kozer Jan 3, 2023
f311dcb
Merge branch 'WordPress:trunk' into fix/get_query_args_ignore_malformed
kozer Jan 10, 2023
3550425
Merge branch 'WordPress:trunk' into fix/get_query_args_ignore_malformed
kozer Jan 10, 2023
8a3d7b0
Return malformed param instead of discard it
kozer Jan 10, 2023
cbb9a3d
chore: use saveDecodeURIComponent instead of try catch
kozer Jan 13, 2023
9a803c1
tests: Update test to ensure that getQueryArgs does not throw
kozer Jan 13, 2023
e5b754b
Import from the actual module to avoid dependency issues
danielbachhuber Jan 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 12 additions & 10 deletions packages/url/src/get-query-args.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,17 +80,19 @@ export function getQueryArgs( url ) {
.replace( /\+/g, '%20' )
.split( '&' )
.reduce( ( accumulator, keyValue ) => {
const [ key, value = '' ] = keyValue
.split( '=' )
// Filtering avoids decoding as `undefined` for value, where
// default is restored in destructuring assignment.
.filter( Boolean )
.map( decodeURIComponent );
try {
const [ key, value = '' ] = keyValue
.split( '=' )
// Filtering avoids decoding as `undefined` for value, where
// default is restored in destructuring assignment.
.filter( Boolean )
.map( decodeURIComponent );

if ( key ) {
const segments = key.replace( /\]/g, '' ).split( '[' );
setPath( accumulator, segments, value );
}
if ( key ) {
const segments = key.replace( /\]/g, '' ).split( '[' );
setPath( accumulator, segments, value );
}
} catch ( uriComponentError ) {}

return accumulator;
}, Object.create( null ) )
Expand Down
8 changes: 8 additions & 0 deletions packages/url/src/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -723,6 +723,14 @@ describe( 'getQueryArgs', () => {
)
).toEqual( data );
} );

it( 'should ignore malformed params', () => {
const url = 'https://andalouses.example/beach?foo=bar&baz=%E0%A4%A';

expect( getQueryArgs( url ) ).toEqual( {
kozer marked this conversation as resolved.
Show resolved Hide resolved
foo: 'bar',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PHP's parse_str() returns the malformed query var:

wp shell
wp> parse_str( 'foo=bar&baz=%E0%A4%A', $args );
=> NULL
$ wp> $args
=> array(2) {
  ["foo"]=>
  string(3) "bar"
  ["baz"]=>
  string(4) "�%A"
}

getQueryArgs() was originally designed to emulate parse_str() (#20693). Would it be possible to return the malformed value instead, for consistent behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @danielbachhuber for pointing this out. I wasn't familiar with that.
I did the changes as requested.

Please note, that although this is a step to the right direction, will minimally affect the issue we have in calypso.

Calypso uses two ways to parse URLs:

  1. qs library
  2. Using the following import:
import { getQueryArgs } from 'calypso/lib/query-args';

This internally uses the get-query-args I'm about to fix here, but to fix the issue related to this pr, we need to stop using qs and start using our function as stated above.

I proposed to add that inside the calypso README file for other developers to be aware.

We can create a ticket to remove qs library of course, but this is a huge take and I think it's better if we do it incrementally.

Maybe we should communicate that also to the calypso channel as well?

Any thoughts?

Thank you!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This internally uses the get-query-args I'm about to fix here, but to fix the issue related to this pr, we need to stop using qs and start using our function as stated above.

I proposed to add that inside the calypso README file for other developers to be aware.

We can create a ticket to remove qs library of course, but this is a huge take and I think it's better if we do it incrementally.

@kozer I'm not sure README is quite the right approach. Can you create a new issue in https://github.com/Automattic/wp-calypso and we can discuss with the Calypso team?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getQueryArgs() was originally designed to emulate parse_str() (#20693). Would it be possible to return the malformed value instead, for consistent behavior?

I kind of agree with this. Is there is a reason not to return the value? You could just replace decodeURIComponent with safeDecodeURIComponent then.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Raising another hand of agreement. I think using safeDecodeURIComponent was the intention here since it does exactly what we're aiming for.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ntsekouras , @tyxla I did the changes you proposed. Thanks so much!
I already have created an issue here. Do you think it make sense to open a new one?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I already have created an issue here. Do you think it make sense to open a new one?

@kozer No, that's sufficient.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking care of that, @kozer, looks great 🙌

} );
} );
} );
} );

Expand Down