Skip to content

Commit fe72186

Browse files
atscottatscott
committed
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.
1 parent 0a9ff4e commit fe72186

2 files changed

Lines changed: 192 additions & 9 deletions

File tree

‎packages/router/src/statemanager/navigation_state_manager.ts‎

Lines changed: 35 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import {
1515
} from '@angular/core';
1616

1717
import {
18+
Location,
1819
PlatformLocation,
1920
PlatformNavigation,
2021
ɵPRECOMMIT_HANDLER_SUPPORTED as PRECOMMIT_HANDLER_SUPPORTED,
@@ -65,8 +66,7 @@ export class NavigationStateManager extends StateManager {
6566
/** The base origin of the application, extracted from PlatformLocation. */
6667
private readonly base = new URL(inject(PlatformLocation).href).origin;
6768
/** The root URL of the Angular application, considering the base href. */
68-
private readonly appRootURL = new URL(this.location.prepareExternalUrl?.('/') ?? '/', this.base)
69-
.href;
69+
private readonly appRootUrl = new URL(this.location.prepareExternalUrl?.('/') ?? '/', this.base);
7070
private readonly precommitHandlerSupported = inject(PRECOMMIT_HANDLER_SUPPORTED);
7171
/**
7272
* The `NavigationHistoryEntry` from the Navigation API that corresponds to the last successfully
@@ -399,6 +399,17 @@ export class NavigationStateManager extends StateManager {
399399
}
400400
const isTriggeredByRouterTransition = !!routerInfo;
401401
if (!isTriggeredByRouterTransition) {
402+
const {pathname: destPathname, origin: destOrigin} = new URL(event.destination.url);
403+
const {pathname: rootPathname, origin: appOrigin} = this.appRootUrl;
404+
const rootPath = rootPathname.endsWith('/') ? rootPathname : rootPathname + '/';
405+
406+
if (
407+
destOrigin !== appOrigin ||
408+
(destPathname !== rootPathname && !destPathname.startsWith(rootPath))
409+
) {
410+
return;
411+
}
412+
402413
// If there's an ongoing navigation in the Angular Router, abort it. This new navigation
403414
// supersedes it. If the navigation was triggered by the Router, it may be the navigation
404415
// happening from _inside_ the navigation transition, or a separate Router.navigate call
@@ -523,11 +534,9 @@ export class NavigationStateManager extends StateManager {
523534
* @param event The `NavigateEvent` from the Navigation API.
524535
*/
525536
private handleNavigateEventTriggeredOutsideRouterAPIs(event: NavigateEvent) {
526-
// TODO(atscott): Consider if the destination URL doesn't start with `appRootURL`.
527-
// Should we ignore it or not intercept in the first place?
528-
529537
// Extract the application-relative path from the full destination URL.
530-
const path = event.destination.url.substring(this.appRootURL.length - 1);
538+
// The url will always start with the appRootUrl because of the boundary check in handleNavigate.
539+
const path = event.destination.url.substring(this.appRootUrl.href.length - 1);
531540
const state = event.destination.getState() as RestoredState | null | undefined;
532541
this.nonRouterCurrentEntryChangeSubject.next({path, state});
533542
}
@@ -539,8 +548,26 @@ export class NavigationStateManager extends StateManager {
539548
const internalPath = this.createBrowserPath(transition);
540549
const eventDestination = new URL(navigateEvent.destination.url);
541550
// this might be a path or an actual URL depending on the baseHref
542-
const routerDestination = this.location.prepareExternalUrl(internalPath);
543-
return new URL(routerDestination, eventDestination.origin).href === eventDestination.href;
551+
const routerDestination = new URL(
552+
this.location.prepareExternalUrl(internalPath),
553+
eventDestination.origin,
554+
);
555+
556+
eventDestination.searchParams.sort();
557+
routerDestination.searchParams.sort();
558+
559+
const {pathname: destPathname, search: destSearch, hash: hashDest} = routerDestination;
560+
const {
561+
pathname: eventDestPathname,
562+
search: eventDestSearch,
563+
hash: eventDestHash,
564+
} = eventDestination;
565+
566+
return (
567+
destSearch === eventDestSearch &&
568+
hashDest === eventDestHash &&
569+
Location.stripTrailingSlash(destPathname) === Location.stripTrailingSlash(eventDestPathname)
570+
);
544571
}
545572

546573
private generateNgRouterState(transition: RouterNavigation) {

‎packages/router/test/with_platform_navigation.spec.ts‎

Lines changed: 157 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,16 @@
77
*/
88

99
import {TestBed} from '@angular/core/testing';
10-
import {NavigationStart, provideRouter, Event, Router} from '../src';
10+
import {
11+
NavigationStart,
12+
provideRouter,
13+
Event,
14+
Router,
15+
UrlSerializer,
16+
DefaultUrlSerializer,
17+
UrlTree,
18+
Params,
19+
} from '../src';
1120
import {withExperimentalPlatformNavigation, withRouterConfig} from '../src/provide_router';
1221
import {withBody, useAutoTick, timeout} from '@angular/private/testing';
1322
import {
@@ -21,6 +30,7 @@ import {
2130
ɵFakeNavigation as FakeNavigation,
2231
ɵFakeNavigationPlatformLocation as FakeNavigationPlatformLocation,
2332
provideLocationMocks,
33+
MOCK_PLATFORM_LOCATION_CONFIG,
2434
} from '@angular/common/testing';
2535
import {inject} from '@angular/core';
2636

@@ -227,6 +237,152 @@ describe('withPlatformNavigation feature', () => {
227237
await expectAsync(finished).toBeResolved();
228238
});
229239
});
240+
241+
class TrailingSlashNormalizingUrlSerializer extends DefaultUrlSerializer {
242+
override parse(url: string): UrlTree {
243+
if (url !== '/' && url.endsWith('/')) {
244+
url = url.slice(0, -1);
245+
}
246+
return super.parse(url);
247+
}
248+
override serialize(tree: UrlTree): string {
249+
let url = super.serialize(tree);
250+
if (url !== '/' && url.endsWith('/')) {
251+
url = url.slice(0, -1);
252+
}
253+
return url;
254+
}
255+
}
256+
257+
class QueryParamSortingUrlSerializer extends DefaultUrlSerializer {
258+
override parse(url: string): UrlTree {
259+
const tree = super.parse(url);
260+
const sorted: Params = {};
261+
for (const key of Object.keys(tree.queryParams).sort()) {
262+
sorted[key] = tree.queryParams[key];
263+
}
264+
tree.queryParams = sorted;
265+
return tree;
266+
}
267+
override serialize(tree: UrlTree): string {
268+
const sorted: Params = {};
269+
for (const key of Object.keys(tree.queryParams).sort()) {
270+
sorted[key] = tree.queryParams[key];
271+
}
272+
const newTree = new UrlTree(tree.root, sorted, tree.fragment);
273+
return super.serialize(newTree);
274+
}
275+
}
276+
277+
describe('URL comparison and extraction bugs', () => {
278+
useAutoTick();
279+
let router: Router;
280+
let navigation: PlatformNavigation;
281+
282+
it('should not trigger new navigation when traversing back to a URL with trailing slash mismatch (with custom serializer)', async () => {
283+
TestBed.resetTestingModule();
284+
TestBed.configureTestingModule({
285+
providers: [
286+
{provide: UrlSerializer, useClass: TrailingSlashNormalizingUrlSerializer},
287+
{provide: PRECOMMIT_HANDLER_SUPPORTED, useValue: false},
288+
provideRouter(
289+
[
290+
{path: 'foo', children: []},
291+
{path: 'bar', children: []},
292+
],
293+
withExperimentalPlatformNavigation(),
294+
),
295+
],
296+
});
297+
navigation = TestBed.inject(PlatformNavigation);
298+
299+
navigation.navigate('/foo/');
300+
await timeout();
301+
navigation.navigate('/bar');
302+
await timeout();
303+
304+
router = TestBed.inject(Router);
305+
router.initialNavigation();
306+
await navigation.transition?.finished;
307+
308+
const navigateSpy = spyOn(navigation, 'navigate').and.callThrough();
309+
310+
await navigation.back().finished;
311+
await timeout();
312+
313+
expect(navigateSpy).not.toHaveBeenCalled();
314+
});
315+
316+
it('should not trigger new navigation when traversing back to a URL with query param order mismatch (with custom serializer)', async () => {
317+
TestBed.resetTestingModule();
318+
TestBed.configureTestingModule({
319+
providers: [
320+
{provide: UrlSerializer, useClass: QueryParamSortingUrlSerializer},
321+
{provide: PRECOMMIT_HANDLER_SUPPORTED, useValue: false},
322+
provideRouter(
323+
[
324+
{path: 'foo', children: []},
325+
{path: 'bar', children: []},
326+
],
327+
withExperimentalPlatformNavigation(),
328+
),
329+
],
330+
});
331+
navigation = TestBed.inject(PlatformNavigation);
332+
333+
navigation.navigate('/foo?b=2&a=1');
334+
await timeout();
335+
navigation.navigate('/bar');
336+
await timeout();
337+
338+
router = TestBed.inject(Router);
339+
router.initialNavigation();
340+
await navigation.transition?.finished;
341+
342+
const navigateSpy = spyOn(navigation, 'navigate').and.callThrough();
343+
344+
await navigation.back().finished;
345+
await timeout();
346+
347+
expect(navigateSpy).not.toHaveBeenCalled();
348+
});
349+
350+
it('should not intercept navigations outside the app root', async () => {
351+
TestBed.resetTestingModule();
352+
TestBed.configureTestingModule({
353+
providers: [
354+
{
355+
provide: MOCK_PLATFORM_LOCATION_CONFIG,
356+
useValue: {
357+
startUrl: 'http://localhost/my-app/',
358+
appBaseHref: '/my-app/',
359+
},
360+
},
361+
provideRouter([{path: '**', children: []}], withExperimentalPlatformNavigation()),
362+
],
363+
});
364+
navigation = TestBed.inject(PlatformNavigation);
365+
366+
let interceptCalled = false;
367+
navigation.addEventListener('navigate', (e: any) => {
368+
const originalIntercept = e.intercept;
369+
e.intercept = function (...args: any[]) {
370+
interceptCalled = true;
371+
originalIntercept.apply(this, args);
372+
};
373+
});
374+
375+
router = TestBed.inject(Router);
376+
router.initialNavigation();
377+
await navigation.transition?.finished;
378+
379+
interceptCalled = false;
380+
navigation.navigate('http://localhost/other-app/foo');
381+
await timeout();
382+
383+
expect(interceptCalled).toBeFalse();
384+
});
385+
});
230386
});
231387

232388
describe('configuration error', () => {

0 commit comments

Comments
 (0)