From fe721868a6e3b87abc112e49911da42e8c8f17ad Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Mon, 8 Jun 2026 14:27:44 -0700 Subject: [PATCH] fix(router): use native URL object for navigation boundary and comparison Previously, `NavigationStateManager` relied on string-based comparisons and `.substring()` to match `NavigateEvent` URLs against internal router transitions or the application root boundary. This was brittle against trailing slashes, query parameter order variations, and sibling application URLs. This commit updates the logic to: - Use the native `URL` object to strictly compare `origin` and `pathname` for `appRootURL` boundaries. - Sort `searchParams` and use `Location.stripTrailingSlash()` to robustly compare the router destination against the event destination. - Pre-compute and store `appRootUrl` as a `URL` object to avoid redundant parsing on every navigation. --- .../statemanager/navigation_state_manager.ts | 43 ++++- .../test/with_platform_navigation.spec.ts | 158 +++++++++++++++++- 2 files changed, 192 insertions(+), 9 deletions(-) diff --git a/packages/router/src/statemanager/navigation_state_manager.ts b/packages/router/src/statemanager/navigation_state_manager.ts index ad04a1a9350..990e9d96ff5 100644 --- a/packages/router/src/statemanager/navigation_state_manager.ts +++ b/packages/router/src/statemanager/navigation_state_manager.ts @@ -15,6 +15,7 @@ import { } from '@angular/core'; import { + Location, PlatformLocation, PlatformNavigation, ɵPRECOMMIT_HANDLER_SUPPORTED as PRECOMMIT_HANDLER_SUPPORTED, @@ -65,8 +66,7 @@ export class NavigationStateManager extends StateManager { /** The base origin of the application, extracted from PlatformLocation. */ private readonly base = new URL(inject(PlatformLocation).href).origin; /** The root URL of the Angular application, considering the base href. */ - private readonly appRootURL = new URL(this.location.prepareExternalUrl?.('/') ?? '/', this.base) - .href; + private readonly appRootUrl = new URL(this.location.prepareExternalUrl?.('/') ?? '/', this.base); private readonly precommitHandlerSupported = inject(PRECOMMIT_HANDLER_SUPPORTED); /** * The `NavigationHistoryEntry` from the Navigation API that corresponds to the last successfully @@ -399,6 +399,17 @@ export class NavigationStateManager extends StateManager { } const isTriggeredByRouterTransition = !!routerInfo; if (!isTriggeredByRouterTransition) { + const {pathname: destPathname, origin: destOrigin} = new URL(event.destination.url); + const {pathname: rootPathname, origin: appOrigin} = this.appRootUrl; + const rootPath = rootPathname.endsWith('/') ? rootPathname : rootPathname + '/'; + + if ( + destOrigin !== appOrigin || + (destPathname !== rootPathname && !destPathname.startsWith(rootPath)) + ) { + return; + } + // If there's an ongoing navigation in the Angular Router, abort it. This new navigation // supersedes it. If the navigation was triggered by the Router, it may be the navigation // happening from _inside_ the navigation transition, or a separate Router.navigate call @@ -523,11 +534,9 @@ export class NavigationStateManager extends StateManager { * @param event The `NavigateEvent` from the Navigation API. */ private handleNavigateEventTriggeredOutsideRouterAPIs(event: NavigateEvent) { - // TODO(atscott): Consider if the destination URL doesn't start with `appRootURL`. - // Should we ignore it or not intercept in the first place? - // Extract the application-relative path from the full destination URL. - const path = event.destination.url.substring(this.appRootURL.length - 1); + // The url will always start with the appRootUrl because of the boundary check in handleNavigate. + const path = event.destination.url.substring(this.appRootUrl.href.length - 1); const state = event.destination.getState() as RestoredState | null | undefined; this.nonRouterCurrentEntryChangeSubject.next({path, state}); } @@ -539,8 +548,26 @@ export class NavigationStateManager extends StateManager { const internalPath = this.createBrowserPath(transition); const eventDestination = new URL(navigateEvent.destination.url); // this might be a path or an actual URL depending on the baseHref - const routerDestination = this.location.prepareExternalUrl(internalPath); - return new URL(routerDestination, eventDestination.origin).href === eventDestination.href; + const routerDestination = new URL( + this.location.prepareExternalUrl(internalPath), + eventDestination.origin, + ); + + eventDestination.searchParams.sort(); + routerDestination.searchParams.sort(); + + const {pathname: destPathname, search: destSearch, hash: hashDest} = routerDestination; + const { + pathname: eventDestPathname, + search: eventDestSearch, + hash: eventDestHash, + } = eventDestination; + + return ( + destSearch === eventDestSearch && + hashDest === eventDestHash && + Location.stripTrailingSlash(destPathname) === Location.stripTrailingSlash(eventDestPathname) + ); } private generateNgRouterState(transition: RouterNavigation) { diff --git a/packages/router/test/with_platform_navigation.spec.ts b/packages/router/test/with_platform_navigation.spec.ts index 7fdacc51643..cfbe4fd68c9 100644 --- a/packages/router/test/with_platform_navigation.spec.ts +++ b/packages/router/test/with_platform_navigation.spec.ts @@ -7,7 +7,16 @@ */ import {TestBed} from '@angular/core/testing'; -import {NavigationStart, provideRouter, Event, Router} from '../src'; +import { + NavigationStart, + provideRouter, + Event, + Router, + UrlSerializer, + DefaultUrlSerializer, + UrlTree, + Params, +} from '../src'; import {withExperimentalPlatformNavigation, withRouterConfig} from '../src/provide_router'; import {withBody, useAutoTick, timeout} from '@angular/private/testing'; import { @@ -21,6 +30,7 @@ import { ɵFakeNavigation as FakeNavigation, ɵFakeNavigationPlatformLocation as FakeNavigationPlatformLocation, provideLocationMocks, + MOCK_PLATFORM_LOCATION_CONFIG, } from '@angular/common/testing'; import {inject} from '@angular/core'; @@ -227,6 +237,152 @@ describe('withPlatformNavigation feature', () => { await expectAsync(finished).toBeResolved(); }); }); + + class TrailingSlashNormalizingUrlSerializer extends DefaultUrlSerializer { + override parse(url: string): UrlTree { + if (url !== '/' && url.endsWith('/')) { + url = url.slice(0, -1); + } + return super.parse(url); + } + override serialize(tree: UrlTree): string { + let url = super.serialize(tree); + if (url !== '/' && url.endsWith('/')) { + url = url.slice(0, -1); + } + return url; + } + } + + class QueryParamSortingUrlSerializer extends DefaultUrlSerializer { + override parse(url: string): UrlTree { + const tree = super.parse(url); + const sorted: Params = {}; + for (const key of Object.keys(tree.queryParams).sort()) { + sorted[key] = tree.queryParams[key]; + } + tree.queryParams = sorted; + return tree; + } + override serialize(tree: UrlTree): string { + const sorted: Params = {}; + for (const key of Object.keys(tree.queryParams).sort()) { + sorted[key] = tree.queryParams[key]; + } + const newTree = new UrlTree(tree.root, sorted, tree.fragment); + return super.serialize(newTree); + } + } + + describe('URL comparison and extraction bugs', () => { + useAutoTick(); + let router: Router; + let navigation: PlatformNavigation; + + it('should not trigger new navigation when traversing back to a URL with trailing slash mismatch (with custom serializer)', async () => { + TestBed.resetTestingModule(); + TestBed.configureTestingModule({ + providers: [ + {provide: UrlSerializer, useClass: TrailingSlashNormalizingUrlSerializer}, + {provide: PRECOMMIT_HANDLER_SUPPORTED, useValue: false}, + provideRouter( + [ + {path: 'foo', children: []}, + {path: 'bar', children: []}, + ], + withExperimentalPlatformNavigation(), + ), + ], + }); + navigation = TestBed.inject(PlatformNavigation); + + navigation.navigate('/foo/'); + await timeout(); + navigation.navigate('/bar'); + await timeout(); + + router = TestBed.inject(Router); + router.initialNavigation(); + await navigation.transition?.finished; + + const navigateSpy = spyOn(navigation, 'navigate').and.callThrough(); + + await navigation.back().finished; + await timeout(); + + expect(navigateSpy).not.toHaveBeenCalled(); + }); + + it('should not trigger new navigation when traversing back to a URL with query param order mismatch (with custom serializer)', async () => { + TestBed.resetTestingModule(); + TestBed.configureTestingModule({ + providers: [ + {provide: UrlSerializer, useClass: QueryParamSortingUrlSerializer}, + {provide: PRECOMMIT_HANDLER_SUPPORTED, useValue: false}, + provideRouter( + [ + {path: 'foo', children: []}, + {path: 'bar', children: []}, + ], + withExperimentalPlatformNavigation(), + ), + ], + }); + navigation = TestBed.inject(PlatformNavigation); + + navigation.navigate('/foo?b=2&a=1'); + await timeout(); + navigation.navigate('/bar'); + await timeout(); + + router = TestBed.inject(Router); + router.initialNavigation(); + await navigation.transition?.finished; + + const navigateSpy = spyOn(navigation, 'navigate').and.callThrough(); + + await navigation.back().finished; + await timeout(); + + expect(navigateSpy).not.toHaveBeenCalled(); + }); + + it('should not intercept navigations outside the app root', async () => { + TestBed.resetTestingModule(); + TestBed.configureTestingModule({ + providers: [ + { + provide: MOCK_PLATFORM_LOCATION_CONFIG, + useValue: { + startUrl: 'http://localhost/my-app/', + appBaseHref: '/my-app/', + }, + }, + provideRouter([{path: '**', children: []}], withExperimentalPlatformNavigation()), + ], + }); + navigation = TestBed.inject(PlatformNavigation); + + let interceptCalled = false; + navigation.addEventListener('navigate', (e: any) => { + const originalIntercept = e.intercept; + e.intercept = function (...args: any[]) { + interceptCalled = true; + originalIntercept.apply(this, args); + }; + }); + + router = TestBed.inject(Router); + router.initialNavigation(); + await navigation.transition?.finished; + + interceptCalled = false; + navigation.navigate('http://localhost/other-app/foo'); + await timeout(); + + expect(interceptCalled).toBeFalse(); + }); + }); }); describe('configuration error', () => {