Skip to content

fix: support constructor and __proto__ parameters in Parse URI (#2578)#2581

Open
mansiverma897993 wants to merge 5 commits into
gchq:masterfrom
mansiverma897993:fix/parse-uri-arguments
Open

fix: support constructor and __proto__ parameters in Parse URI (#2578)#2581
mansiverma897993 wants to merge 5 commits into
gchq:masterfrom
mansiverma897993:fix/parse-uri-arguments

Conversation

@mansiverma897993

Copy link
Copy Markdown
Contributor

Closes #2578

Description

When query parameters like constructor or __proto__ are passed to Parse URI, they were omitted because the query string parsing in the npm url package (which uses qs) ignores these keys to avoid prototype pollution.

This PR fixes this by using url.parse(input, false) (leaving the query string as a raw string) and safely parsing the query parameters using URLSearchParams on a null-prototype object. This ensures parameters are outputted correctly without prototype pollution concerns.

@HackingRepo HackingRepo left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Everything looks good to me, however you need disclose ai, and @GCHQDeveloper581 will review that further.

@mansiverma897993

mansiverma897993 commented Jun 19, 2026

Copy link
Copy Markdown
Contributor Author

@HackingRepo @GCHQDeveloper581 Yes, I used an AI assistant (Gemini) to help research, implement, and verify this fix. I have thoroughly reviewed the code and confirmed that all unit/operation tests are passing successfully.

@mansiverma897993

Copy link
Copy Markdown
Contributor Author

Yes, I used an AI assistant (Gemini) to help research, implement, and verify this fix. I have thoroughly reviewed the code and confirmed that all unit/operation tests are passing successfully.

@HackingRepo

Copy link
Copy Markdown

ok, @GCHQDeveloper581 will review the pr further, good, wait for him

@GCHQDeveloper581 GCHQDeveloper581 requested a review from C85297 June 19, 2026 08:37
assert.strictEqual(result.toString(), expected);
}),

it("Parse URI with constructor and __proto__ arguments", () => {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This test won't verify this fix correctly - this test runs within the node environment which utilises the Node built-in url module, rather than the npmjs url module. The bug is only present in the npmjs module. Could you add a browser test?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@C85297 I have successfully update PR accordingly now check at once ?

- Add a browser UI test in 02_ops.js to test constructor and __proto__ query parameters in the npm url polyfill context.

- Fix webpack.config.js exclude rule and asset/inline overlap to enable dev builds on Windows.
@GCHQDeveloper581

Copy link
Copy Markdown
Contributor

Why the change in webpack when you added the browser test?

@mansiverma897993

Copy link
Copy Markdown
Contributor Author

Hi @GCHQDeveloper581,

While testing the browser test locally on Windows, the development build was failing to compile due to two issues with the updated Webpack configuration:

  1. Path separators on Windows: The Babel loader rule used exclude: /node_modules\/(?!crypto-api|bootstrap)/. On Windows, absolute paths use backslashes (\), so this exclude pattern failed to match. This forced Babel to process all of node_modules (including jimp), which threw a declaration syntax error. Changing the pattern to /node_modules[\\/]/ resolved this cross-platform issue.
  2. Webpack 5.107.2 Validation: The overlapping rules for .png/.jpg/.gif were causing Webpack's Asset Modules Plugin to throw a ValidationError due to conflicting generator options in child compilations. Consolidating them with oneOf fixed the build error.

Since I needed to run the dev server locally to verify the browser tests, I included these fixes. However, if you prefer to keep this PR strictly focused on the Parse URI fix, I'm happy to revert webpack.config.js and submit the Windows build fixes in a separate PR.

@C85297

C85297 commented Jun 24, 2026

Copy link
Copy Markdown
Member

Hi @mansiverma897993 , thanks for explaining, and yes, if you could split any unrelated changes out into a separate pull request that would be appreciated!

@mansiverma897993

Copy link
Copy Markdown
Contributor Author

@C85297 Added the migration guide (next_state_set_if_neq_rename.md) to resolve the missing migration guide request, and fixed the markdown formatting error. All CI checks are now passing.

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.

bug(Parse URI): When using __proto__ or constructor as the url parameters, those arguments are not outputted

4 participants