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.

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

Fix Query Params Parsing #3012

Merged
merged 2 commits into from
Jan 6, 2025
Merged

Fix Query Params Parsing #3012

merged 2 commits into from
Jan 6, 2025

Conversation

coldlink
Copy link
Member

@coldlink coldlink commented Dec 19, 2024

What does this change?

In #2875 we fixed an issue where there were duplicate query params appearing in the URL.

This only fixes this issue from the client side, the server side issue still remains if duplicate query params were to be provided.

Essentially express query parameter parsing will convert a value of a query parameter into an array if it sees multiple keys for that parameter.

e.g.

?foo=bar&foo=baz&hello=world

would get parsed into

{
 foo: ['bar', 'baz'],
 hello: 'world'
}

In Gateway we don’t use duplicate query parameters and they should never exist.

By default, express will use https://www.npmjs.com/package/qs to parse query params, which is more powerful than what we need (called 'extended' in the express docs, see https://expressjs.com/en/api.html#app.settings.table).

Even the "simple" mode in express uses node's querystring api doesn't work. This will also parse duplicate query params into an array, which is not what we want, as shown by the example: https://nodejs.org/api/querystring.html#querystringparsestr-sep-eq-options

However, we can use a custom query parser to use the native URLSearchParams API, which will parse duplicate query params into the last value, which is exactly what we want.

So that ?foo=bar&foo=baz&hello=world => { foo: 'baz', hello: 'world' }.

This can be tested in a node repl or browser console by running:

Object.fromEntries(new URLSearchParams(`foo=bar&foo=baz&hello=world`).entries())

which returns the expected result.

Tested

  • DEV
    • inc. node repl
  • CODE

@coldlink coldlink force-pushed the mm/fix-query-params branch from 188f0c5 to 9991a28 Compare December 19, 2024 10:20
@coldlink coldlink marked this pull request as ready for review December 19, 2024 10:20
@coldlink coldlink requested a review from a team as a code owner December 19, 2024 10:20
Copy link
Contributor

@akinsola-guardian akinsola-guardian left a comment

Choose a reason for hiding this comment

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

LGTM

@coldlink coldlink requested a review from guardian-ci January 6, 2025 08:20
@coldlink coldlink enabled auto-merge January 6, 2025 08:41
@coldlink coldlink merged commit 1137eae into main Jan 6, 2025
21 checks passed
@coldlink coldlink deleted the mm/fix-query-params branch January 6, 2025 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants