Skip to content

url: convert setter values before reading current href - #67356

Closed
hakarrr wants to merge 2 commits into
nodejs:mainfrom
hakarrr:fix-url-setter-coercion-order
Closed

hakarrr wants to merge 2 commits into
nodejs:mainfrom
hakarrr:fix-url-setter-coercion-order

Conversation

@hakarrr

@hakarrr hakarrr commented Nov 9, 2026 •

Copy link
Copy Markdown

Edit: removed Fixes: https://github.com/nodejs/node/issues/67345

Web IDL converts an assigned value to the attribute's type before the setter steps run. The URL component setters passed this.#context.href as the first argument to bindingUrl.update(), so it was evaluated before the assigned value's string conversion. A toString() that mutates the URL during conversion therefore had its effects overwritten by the stale href captured beforehand:

const url = new URL('https://before.example/old');
url.pathname = {
  toString() {
    url.href = 'https://after.example/base';
    return '/new';
  },
};
// before: https://before.example/new
// after:  https://after.example/new

The change hoists the conversion above the href read in the protocol, username, password, host, hostname, port, pathname, search and hash setters, matching the existing href setter, which already converts first.

Adds test/parallel/test-whatwg-url-setters-coercion-order.js covering all nine setters (WPT's setters_tests.json has no mutation-during-conversion cases).

Local verification: the new test fails on the unpatched code (all nine setters) and passes with the fix; I additionally ran a differential check of 25 ordinary URL operations (assignments, port edge cases, URLSearchParams sync, percent-encoding) with identical outputs before and after the change. Note my sandbox can't build Node, so I verified the patched lib/internal/url.js by loading it through --expose-internals on v22.23.4 against the real ada binding - CI should confirm on a full build.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes - sandbox here can't build Node; new test verified via the --expose-internals route described above, CI run appreciated
  • tests and/or benchmarks are included
  • documentation is changed or added (not user-facing-documented behavior beyond WHATWG spec compliance)
  • commit message follows commit guidelines

generaltony007 and others added 2 commits November 9, 2026 10:23
Web IDL converts an assigned value to the attribute's type before the setter steps run. The URL component setters passed `this.#context.href` as the first argument to `bindingUrl.update()`, evaluating it before the assigned value's string conversion. A `toString()` that mutates the URL during conversion therefore had its effects overwritten by the stale href captured beforehand.

Hoist the conversion above the href read in the protocol, username, password, host, hostname, port, pathname, search and hash setters, matching the existing `href` setter.

Fixes: nodejs#67345

Co-authored-by: generaltony007 <144252170+GeneralTony007@users.noreply.github.com>
Regression test: every URL component setter must apply to the URL state after value conversion, not to stale state captured before it.

Fixes: nodejs#67345

Co-authored-by: generaltony007 <144252170+GeneralTony007@users.noreply.github.com>
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/url

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Nov 9, 2026
@github-actions

github-actions Bot commented Nov 9, 2026

Copy link
Copy Markdown
Contributor

Welcome to Node.js, and thank you for your first contribution!

Before review, please take a moment to read:

Please make sure every commit is signed off. For a first pull request, GitHub Actions require collaborator approval and Jenkins CI must be started by a collaborator or triager, so an initial wait is normal.

Caution

AgentScan found account activity patterns that may be consistent with automation. This is a heuristic, not proof that this pull request was opened by an agent or violates policy. AI-assisted contributions are permitted, but automated tooling must not open pull requests without advance approval, and contributors must personally understand, test, verify, and take responsibility for every submitted change. See the AgentScan analysis, AI use policy, and automation policy for additional context.

@MikeMcC399

Copy link
Copy Markdown
Contributor

See Commit message guidelines according to the Pull requests documentation.

Your commit must contain the Signed-off-by line with your name and email address as an acknowledgement that you agree to the Developer Certificate of Origin.

This is repeated also in AGENTS.md where it says:

  • All commits must be signed off by the human user using Signed-off-by: Your Name <your@email.example>
    as an attestation to the Developer Certificate of Origin.
  • AI-assistance must be acknowledged using the Assisted-by: <agent name> annotation.
  • AI-authored code contributions must be compatible with the project's licensing and
    contribution guidelines.

@MikeMcC399

Copy link
Copy Markdown
Contributor

Note my sandbox can't build Node, so I verified the patched lib/internal/url.js by loading it through --expose-internals on v22.23.4 against the real ada binding - CI should confirm on a full build.

PRs are required to be fully tested before submission:

See Pull requests > Step 6: Test

Before submitting your changes in a pull request, always run the full Node.js test suite.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agentscan:automated-account needs-ci PRs that need a full CI run. whatwg-url Issues and PRs related to the WHATWG URL implementation.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants