FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix(router): limit protocol-relative URL handling to serialization by SkyZeroZx · Pull Request #69874 · angular/angular · GitHub

fix(router): limit protocol-relative URL handling to serialization - #69874

Merged
amishne merged 1 commit into
angular:mainfrom
SkyZeroZx:fix/router-relative-url
Aug 11, 2026
Merged

fix(router): limit protocol-relative URL handling to serialization#69874
amishne merged 1 commit into
angular:mainfrom
SkyZeroZx:fix/router-relative-url

Conversation

SkyZeroZx commented Jul 21, 2026
edited
Loading

Copy link
Copy Markdown
Contributor

Preserve createUrlTree command semantics, including custom serializer inputs, while keeping the single-leading-slash guarantee at the default serialization boundary.

Expand coverage for command forms, public UrlTree values, secondary outlets, and preserved query parameters and fragments.

Fixes #69700

ngbot Bot added this to the Backlog milestone Jul 21, 2026
SkyZeroZx force-pushed the fix/router-relative-url branch from e45cf01 to e559339 Compare July 21, 2026 14:50
SkyZeroZx force-pushed the fix/router-relative-url branch 2 times, most recently from 2ebb61a to cb1d70b Compare July 22, 2026 17:47
SkyZeroZx marked this pull request as ready for review July 22, 2026 18:05
pullapprove Bot requested a review from kirjs July 22, 2026 18:05
JeanMeche requested review from atscott and removed request for kirjs July 30, 2026 15:42
Comment thread packages/router/src/url_tree.ts Outdated
Preserve createUrlTree command semantics, including custom serializer inputs, while keeping the single-leading-slash guarantee at the default serialization boundary.

Expand coverage for command forms, public UrlTree values, secondary outlets, and preserved query parameters and fragments.

Fixes angular#69700
SkyZeroZx force-pushed the fix/router-relative-url branch 2 times, most recently from cb1d70b to 0c99701 Compare August 10, 2026 21:57
pullapprove Bot requested review from JeanMeche and kirjs August 10, 2026 22:12
atscott added the target: patch This PR is targeted for the next patch release label Aug 10, 2026

JeanMeche left a comment

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

reviewed-for: public-api

JeanMeche removed the request for review from kirjs August 11, 2026 06:54
atscott added the action: merge The PR is ready for merge by the caretaker label Aug 11, 2026
alan-agius4 added action: merge The PR is ready for merge by the caretaker and removed action: merge The PR is ready for merge by the caretaker labels Aug 11, 2026
amishne merged commit 435f8b2 into angular:main Aug 11, 2026
40 of 47 checks passed

amishne commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

This PR was merged into the repository. The changes were merged into the following branches:

Copy link
Copy Markdown
Member

Note, we'll have to revert this fix, I was breaking inside G3. Not sure yet if it was a legit breakage or not.

SkyZeroZx commented Aug 18, 2026
edited
Loading

Copy link
Copy Markdown
Contributor Author

Note, we'll have to revert this fix, I was breaking inside G3. Not sure yet if it was a legit breakage or not.

@JeanMeche Could we investigate the cause? I understand we should send another PR to try to see which tests failed or which cases broke.

JeanMeche commented Aug 18, 2026
edited
Loading

Copy link
Copy Markdown
Member

I'd had a hard time understanding what was the issue, but I believe it was a [routerLink]="['/'+ someVar, someId]" that was throwing. Unsure why.

Copy link
Copy Markdown
Contributor Author

Well, according to GPT-5.6, I'm getting something like this; is there possibly some kind of "mock" that generates this behavior?

 it('rejects question-mark-prefixed relative commands with an explicit empty root segment', async () => {
    @Component({
      template: `<a [routerLink]="['?' + someVar, someId]">commands</a>`,
      imports: [RouterLink],
    })
    class WithLink {
      readonly someVar = 'search';
      readonly someId = '123';
    }

    const snapshot: any = {
      url: [new UrlSegment('', {})],
      children: [],
      outlet: PRIMARY_OUTLET,
    };
    snapshot.root = snapshot;
    TestBed.configureTestingModule({
      providers: [
        provideRouter([{path: '', component: WithLink}]),
        {provide: ActivatedRoute, useValue: {snapshot}},
      ],
    });
    const fixture = TestBed.createComponent(WithLink);

    await expectAsync(fixture.whenStable()).toBeRejectedWithError(
      /NG04019: Cannot serialize a UrlTree that would produce a protocol-relative URL/,
    );
  });

Copy link
Copy Markdown
Member

Fwiw, I wrote a typo, ? was a /. I doesn't look like they were mocks of such sort.

SkyZeroZx commented Aug 18, 2026
edited
Loading

Copy link
Copy Markdown
Contributor Author

From GPT:

I could not reproduce NG04019 directly from the reported two-command RouterLink with scalar values. However, with an empty ID it creates /source/, and a positional redirect such as source/:id → /:id/target moves the empty segment to the front, producing //target and triggering NG04019. Tests expecting NG04019 were only characterization tests; a landing regression test should assert that serialization/navigation does not throw.

it("can fail after a redirect promotes the link's trailing empty segment", async () => {
  @Component({
    template: `<a [routerLink]="['/' + someVar, someId]">commands</a>`,
    imports: [RouterLink],
  })
  class WithLink {
    readonly someVar = 'source';
    readonly someId = '';
  }

  @Component({template: ''})
  class Target {}

  TestBed.configureTestingModule({
    providers: [
      provideRouter([
        {path: '', pathMatch: 'full', component: WithLink},
        {path: 'source/:id', redirectTo: '/:id/target'},
        {path: '**', component: Target},
      ]),
    ],
  });

  const harness = await RouterTestingHarness.create('/');
  const anchor = harness.fixture.nativeElement.querySelector('a');

  // RouterLink itself produces a valid href.
  expect(anchor.getAttribute('href')).toBe('/source/');

  anchor.click();

  // The redirect promotes the empty :id and produces //target.
  await expectAsync(harness.fixture.whenStable()).toBeRejectedWithError(
    /NG04019: Cannot serialize a UrlTree that would produce a protocol-relative URL/,
  );
});

// Short variant
it('rejects an empty dynamic segment passed separately after the root command', async () => {
  @Component({
    template: `<a [routerLink]="['/', someVar, someId]">commands</a>`,
    imports: [RouterLink],
  })
  class WithLink {
    readonly someVar = '';
    readonly someId = '123';
  }

  TestBed.configureTestingModule({
    providers: [provideRouter([])],
  });

  const fixture = TestBed.createComponent(WithLink);

  await expectAsync(fixture.whenStable()).toBeRejectedWithError(
    /NG04019: Cannot serialize a UrlTree that would produce a protocol-relative URL/,
  );
});

As I understand it, the validation was apparently done too early before it could be confirmed that it was indeed a relative URL.

atscott commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

I think generally the error is going to mostly surface issues with test data and potentially mistakes in the command construction. I still have extreme doubts that any of these are actual security issues. To avoid a breaking change here, I wonder if a better approach would be a dev mode warning (instead of throwing), coupled with modifying the serialization to simply return '/'. There is precedence for this in Router.parseUrl already:

try {
return this.urlSerializer.parse(url);
} catch (e) {
this.console.warn(
formatRuntimeError(
RuntimeErrorCode.ERROR_PARSING_URL,
ngDevMode && `Error parsing URL ${url}. Falling back to '/' instead. \n` + e,
),
);
return this.urlSerializer.parse('/');

SkyZeroZx added a commit to SkyZeroZx/angular that referenced this pull request Aug 18, 2026
Avoid throwing when DefaultUrlSerializer encounters a UrlTree that would serialize to a protocol-relative URL. The exception surfaced existing test data and mistaken command construction as a breaking change after angular#69874.

Warn in development mode and serialize the tree as "/" instead. Keep the browser-facing URL root-relative without changing command or route-recognition semantics.

Follow the compatibility approach discussed in PR angular#69874:
angular#69874 (comment)

Address the g3 regression reported at:
angular#69874 (comment)

Fixes angular#69700
SkyZeroZx added a commit to SkyZeroZx/angular that referenced this pull request Aug 18, 2026
Avoid throwing when DefaultUrlSerializer encounters a UrlTree that would serialize to a protocol-relative URL. The exception surfaced existing test data and mistaken command construction as a breaking change after angular#69874.

Warn in development mode and serialize the tree as "/" instead. Keep the browser-facing URL root-relative without changing command or route-recognition semantics.

The original fix had to be reverted after a regression surfaced in g3:
angular#69874 (comment)

This uses the less disruptive fallback proposed in the follow-up discussion:
angular#69874 (comment)

Fixes angular#69700
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

action: merge The PR is ready for merge by the caretaker area: router target: patch This PR is targeted for the next patch release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RouterLink can serialize internal route commands into protocol-relative external hrefs

5 participants


Back | FazBrowse Home | New Git URL