mirror of
https://github.com/angular/angular.git
synced 2026-09-14 13:54:52 +08:00
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.
This commit is contained in:
@@ -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) {
|
||||
|
||||
@@ -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', () => {
|
||||
|
||||
Reference in New Issue
Block a user