mirror of
https://github.com/ionic-team/ionic-framework.git
synced 2026-03-13 10:22:08 +08:00
fix(react-router): prevent tail-slice false positives for parameterized routes
This commit is contained in:
@@ -452,12 +452,35 @@ export class ReactRouterViewStack extends ViewStacks {
|
||||
if (routePath === '*' || routePath === '') {
|
||||
// Check if any other view in this outlet has a match for the current route
|
||||
const outletViews = this.getViewItemsForOutlet(viewItem.outletId);
|
||||
|
||||
// When parent path context is available, compute the relative pathname once
|
||||
// outside the loop since both routeInfo.pathname and parentPath are invariant.
|
||||
const relativePathname = parentPath
|
||||
? computeRelativeToParent(routeInfo.pathname, parentPath)
|
||||
: null;
|
||||
|
||||
let hasSpecificMatch = outletViews.some((v) => {
|
||||
if (v.id === viewItem.id) return false; // Skip self
|
||||
const vRoutePath = v.reactElement?.props?.path || '';
|
||||
if (vRoutePath === '*' || vRoutePath === '') return false; // Skip other wildcard/empty routes
|
||||
|
||||
// Check if this view item would match the current route
|
||||
// When parent path context is available and the route is relative, use
|
||||
// parent-path-aware matching. This avoids false positives from
|
||||
// derivePathnameToMatch's tail-slice heuristic, which can incorrectly
|
||||
// match route literals that appear at the wrong position in the pathname.
|
||||
// Example: pathname /parent/extra/details/99 with route details/:id —
|
||||
// the tail-slice extracts ["details","99"] producing a false match.
|
||||
if (parentPath && vRoutePath && !vRoutePath.startsWith('/')) {
|
||||
if (relativePathname === null) {
|
||||
return false; // Pathname is outside this outlet's parent scope
|
||||
}
|
||||
return !!matchPath({
|
||||
pathname: relativePathname,
|
||||
componentProps: v.reactElement.props,
|
||||
});
|
||||
}
|
||||
|
||||
// Fallback to matchComponent when no parent path context is available
|
||||
const vMatch = v.reactElement ? matchComponent(v.reactElement, routeInfo.pathname) : null;
|
||||
return !!vMatch;
|
||||
});
|
||||
|
||||
@@ -68,17 +68,6 @@ const matchesEmbeddedWildcardRoute = (route: React.ReactElement, pathname: strin
|
||||
return !!matchPath({ pathname, componentProps: route.props });
|
||||
};
|
||||
|
||||
/**
|
||||
* Checks if a route path consists entirely of parameterized segments (e.g., ":slug", ":category/:id").
|
||||
* These routes match any single segment and should not drive the parent path deeper
|
||||
* than the outlet's established mount point.
|
||||
*/
|
||||
const isPurelyParameterized = (routePath: string | undefined): boolean => {
|
||||
if (!routePath) return false;
|
||||
const segments = routePath.split('/').filter(Boolean);
|
||||
return segments.length > 0 && segments.every((segment) => segment.startsWith(':'));
|
||||
};
|
||||
|
||||
/**
|
||||
* Checks if a route is a specific match (not wildcard-only or index).
|
||||
*/
|
||||
@@ -180,12 +169,21 @@ const couldSpecificRouteMatch = (
|
||||
// When mount path is established, skip this lookahead: the parent depth is known,
|
||||
// and purely parameterized routes (e.g., :slug) matching the last segment should
|
||||
// not prevent the wildcard from claiming the full remaining path.
|
||||
//
|
||||
// Only allow purely literal routes (no :params) in the lookahead. Routes with
|
||||
// parameters are positionally ambiguous — their params match any segment, so a
|
||||
// coincidental match at a deeper level (e.g., details/:id matching "details/99"
|
||||
// inside "extra/details/99") should not prevent the wildcard from capturing the
|
||||
// full remaining path.
|
||||
if (!outletMountPath) {
|
||||
for (let j = 1; j < segments.length; j++) {
|
||||
const futureRemaining = segments.slice(j).join('/');
|
||||
const futureMatch = findFirstSpecificMatchingRoute(routeChildren, futureRemaining);
|
||||
if (futureMatch && !isPurelyParameterized(futureMatch.props.path as string)) {
|
||||
return true;
|
||||
if (futureMatch) {
|
||||
const futurePath = futureMatch.props.path as string | undefined;
|
||||
if (futurePath && !futurePath.includes(':')) {
|
||||
return true;
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -314,13 +312,12 @@ export const computeParentPath = (options: ComputeParentPathOptions): ParentPath
|
||||
|
||||
// Check for specific route match (highest priority)
|
||||
if (!firstSpecificMatch && findSpecificMatch(routeChildren, remainingPath)) {
|
||||
// Don't let purely parameterized routes (e.g., :slug, :id) drive the
|
||||
// parent deeper than where a wildcard already matched. A :slug route
|
||||
// matching the last segment of "deep/nested/path" shouldn't pull the
|
||||
// parent to /parent/deep/nested — the wildcard at the correct depth
|
||||
// should catch the full remaining path instead.
|
||||
// Literal routes (e.g., "settings", "redirect") can still match beyond
|
||||
// the wildcard depth to support redirect scenarios.
|
||||
// Don't let routes containing parameter segments (e.g., :slug, details/:id)
|
||||
// drive the parent deeper than where a wildcard already matched. Parameter
|
||||
// segments match any value, making tail-slice matches positionally ambiguous:
|
||||
// e.g., "details/:id" matching "details/99" inside "extra/details/99" is a
|
||||
// coincidental match at the wrong depth. Only purely literal routes (e.g.,
|
||||
// "settings", "redirect") can override the wildcard at deeper levels.
|
||||
//
|
||||
// Also don't let empty/default path routes (path="" or undefined) drive
|
||||
// the parent deeper than a wildcard match. An empty path route matching
|
||||
@@ -334,7 +331,13 @@ export const computeParentPath = (options: ComputeParentPathOptions): ParentPath
|
||||
if (matchingRoute) {
|
||||
const matchingPath = matchingRoute.props.path as string | undefined;
|
||||
const isEmptyPath = !matchingPath || matchingPath === '';
|
||||
if (shouldSkipParameterized && (isPurelyParameterized(matchingPath as string) || isEmptyPath)) {
|
||||
// When the parent path is deeper than expected (shouldSkipParameterized),
|
||||
// skip routes containing ANY parameterized segments. Parameters make tail-
|
||||
// slice matches positionally ambiguous: e.g., "details/:id" matching
|
||||
// "details/99" inside "extra/details/99" is a coincidental match at the
|
||||
// wrong depth. Only purely literal routes (e.g., "settings") can override
|
||||
// the wildcard at deeper levels.
|
||||
if (shouldSkipParameterized && (matchingPath?.includes(':') || isEmptyPath)) {
|
||||
continue;
|
||||
}
|
||||
if (firstWildcardMatch && isEmptyPath) {
|
||||
|
||||
@@ -52,6 +52,7 @@ import PrefixMatchWildcard from './pages/prefix-match-wildcard/PrefixMatchWildca
|
||||
import StaleViewCleanup from './pages/stale-view-cleanup/StaleViewCleanup';
|
||||
import IndexParamPriority from './pages/index-param-priority/IndexParamPriority';
|
||||
import IndexRouteReuse from './pages/index-route-reuse/IndexRouteReuse';
|
||||
import TailSliceAmbiguity from './pages/tail-slice-ambiguity/TailSliceAmbiguity';
|
||||
|
||||
setupIonicReact();
|
||||
|
||||
@@ -93,6 +94,7 @@ const App: React.FC = () => {
|
||||
<Route path="/stale-view-cleanup/*" element={<StaleViewCleanup />} />
|
||||
<Route path="/index-param-priority/*" element={<IndexParamPriority />} />
|
||||
<Route path="/index-route-reuse/*" element={<IndexRouteReuse />} />
|
||||
<Route path="/tail-slice-ambiguity/*" element={<TailSliceAmbiguity />} />
|
||||
</IonRouterOutlet>
|
||||
</IonReactRouter>
|
||||
</IonApp>
|
||||
|
||||
@@ -101,6 +101,9 @@ const Main: React.FC = () => {
|
||||
<IonItem routerLink="/index-route-reuse">
|
||||
<IonLabel>Index Route Reuse</IonLabel>
|
||||
</IonItem>
|
||||
<IonItem routerLink="/tail-slice-ambiguity">
|
||||
<IonLabel>Tail Slice Ambiguity</IonLabel>
|
||||
</IonItem>
|
||||
</IonList>
|
||||
</IonContent>
|
||||
</IonPage>
|
||||
|
||||
@@ -0,0 +1,97 @@
|
||||
import {
|
||||
IonButton,
|
||||
IonContent,
|
||||
IonHeader,
|
||||
IonLabel,
|
||||
IonPage,
|
||||
IonRouterOutlet,
|
||||
IonTitle,
|
||||
IonToolbar,
|
||||
} from '@ionic/react';
|
||||
import React from 'react';
|
||||
import { Route, useLocation, useParams } from 'react-router-dom';
|
||||
|
||||
/**
|
||||
* Test page for tail-slice ambiguity in derivePathnameToMatch.
|
||||
*
|
||||
* Route structure:
|
||||
* /tail-slice-ambiguity/*
|
||||
* └── <IonRouterOutlet>
|
||||
* ├── index → ListPage
|
||||
* ├── details/:id → DetailsPage
|
||||
* └── * → CatchAllPage
|
||||
*
|
||||
* Bug scenario:
|
||||
* 1. Navigate to /tail-slice-ambiguity/details/42 → creates details/:id view
|
||||
* 2. Navigate to /tail-slice-ambiguity/extra/details/99
|
||||
* 3. derivePathnameToMatch tail-slices the last 2 segments ["details", "99"]
|
||||
* and matchComponent falsely matches details/:id
|
||||
* 4. The catch-all * view is incorrectly deactivated because hasSpecificMatch
|
||||
* finds the false positive from details/:id
|
||||
* 5. User sees nothing instead of the catch-all page
|
||||
*/
|
||||
const TailSliceAmbiguity: React.FC = () => (
|
||||
<IonRouterOutlet id="tail-slice-outlet">
|
||||
<Route index element={<ListPage />} />
|
||||
<Route path="details/:id" element={<DetailsPage />} />
|
||||
<Route path="*" element={<CatchAllPage />} />
|
||||
</IonRouterOutlet>
|
||||
);
|
||||
|
||||
const ListPage: React.FC = () => (
|
||||
<IonPage data-pageid="tail-slice-list">
|
||||
<IonHeader>
|
||||
<IonToolbar>
|
||||
<IonTitle>List</IonTitle>
|
||||
</IonToolbar>
|
||||
</IonHeader>
|
||||
<IonContent>
|
||||
<IonButton routerLink="/tail-slice-ambiguity/details/42" id="go-to-details">
|
||||
Details 42
|
||||
</IonButton>
|
||||
<IonButton routerLink="/tail-slice-ambiguity/extra/details/99" id="go-to-ambiguous">
|
||||
Ambiguous Path
|
||||
</IonButton>
|
||||
</IonContent>
|
||||
</IonPage>
|
||||
);
|
||||
|
||||
const DetailsPage: React.FC = () => {
|
||||
const { id } = useParams<{ id: string }>();
|
||||
return (
|
||||
<IonPage data-pageid="tail-slice-details">
|
||||
<IonHeader>
|
||||
<IonToolbar>
|
||||
<IonTitle>Details</IonTitle>
|
||||
</IonToolbar>
|
||||
</IonHeader>
|
||||
<IonContent>
|
||||
<IonLabel data-testid="details-id">Details ID: {id}</IonLabel>
|
||||
<IonButton routerLink="/tail-slice-ambiguity" id="back-to-list">
|
||||
Back to List
|
||||
</IonButton>
|
||||
</IonContent>
|
||||
</IonPage>
|
||||
);
|
||||
};
|
||||
|
||||
const CatchAllPage: React.FC = () => {
|
||||
const location = useLocation();
|
||||
return (
|
||||
<IonPage data-pageid="tail-slice-catchall">
|
||||
<IonHeader>
|
||||
<IonToolbar>
|
||||
<IonTitle>Catch All</IonTitle>
|
||||
</IonToolbar>
|
||||
</IonHeader>
|
||||
<IonContent>
|
||||
<IonLabel data-testid="catchall-path">Catch-all: {location.pathname}</IonLabel>
|
||||
<IonButton routerLink="/tail-slice-ambiguity" id="catchall-back-to-list">
|
||||
Back to List
|
||||
</IonButton>
|
||||
</IonContent>
|
||||
</IonPage>
|
||||
);
|
||||
};
|
||||
|
||||
export default TailSliceAmbiguity;
|
||||
@@ -0,0 +1,74 @@
|
||||
const port = 3000;
|
||||
|
||||
/**
|
||||
* Tests that derivePathnameToMatch's tail-slice heuristic does not produce
|
||||
* false positive matches that incorrectly deactivate catch-all routes.
|
||||
*
|
||||
* Route structure:
|
||||
* /tail-slice-ambiguity/*
|
||||
* ├── index → ListPage
|
||||
* ├── details/:id → DetailsPage
|
||||
* └── * → CatchAllPage
|
||||
*
|
||||
* Bug: navigating to /tail-slice-ambiguity/extra/details/99 after visiting
|
||||
* /tail-slice-ambiguity/details/42 causes the tail-slice to extract
|
||||
* ["details", "99"] which falsely matches details/:id, deactivating the
|
||||
* catch-all page.
|
||||
*/
|
||||
describe('Tail-Slice Ambiguity', () => {
|
||||
it('should show details page for /details/:id', () => {
|
||||
cy.visit(`http://localhost:${port}/tail-slice-ambiguity`);
|
||||
cy.ionPageVisible('tail-slice-list');
|
||||
|
||||
cy.get('#go-to-details').click();
|
||||
cy.ionPageVisible('tail-slice-details');
|
||||
cy.get('[data-testid="details-id"]').should('contain', 'Details ID: 42');
|
||||
});
|
||||
|
||||
it('should show catch-all when path has extra segments before details', () => {
|
||||
cy.visit(`http://localhost:${port}/tail-slice-ambiguity`);
|
||||
cy.ionPageVisible('tail-slice-list');
|
||||
|
||||
// First create the details/:id view
|
||||
cy.get('#go-to-details').click();
|
||||
cy.ionPageVisible('tail-slice-details');
|
||||
cy.get('[data-testid="details-id"]').should('contain', 'Details ID: 42');
|
||||
|
||||
// Go back to list
|
||||
cy.get('#back-to-list').click();
|
||||
cy.ionPageVisible('tail-slice-list');
|
||||
|
||||
// Navigate to ambiguous path - should show catch-all, NOT details
|
||||
cy.get('#go-to-ambiguous').click();
|
||||
cy.ionPageVisible('tail-slice-catchall');
|
||||
cy.get('[data-testid="catchall-path"]').should('contain', '/tail-slice-ambiguity/extra/details/99');
|
||||
});
|
||||
|
||||
it('should show catch-all for ambiguous path on direct navigation', () => {
|
||||
cy.visit(`http://localhost:${port}/tail-slice-ambiguity/extra/details/99`);
|
||||
cy.ionPageVisible('tail-slice-catchall');
|
||||
cy.get('[data-testid="catchall-path"]').should('contain', '/tail-slice-ambiguity/extra/details/99');
|
||||
});
|
||||
|
||||
it('should correctly navigate: details → list → ambiguous → back to list', () => {
|
||||
cy.visit(`http://localhost:${port}/tail-slice-ambiguity`);
|
||||
cy.ionPageVisible('tail-slice-list');
|
||||
|
||||
// Visit details
|
||||
cy.get('#go-to-details').click();
|
||||
cy.ionPageVisible('tail-slice-details');
|
||||
cy.get('[data-testid="details-id"]').should('contain', 'Details ID: 42');
|
||||
|
||||
// Back to list
|
||||
cy.get('#back-to-list').click();
|
||||
cy.ionPageVisible('tail-slice-list');
|
||||
|
||||
// Visit ambiguous path (catch-all)
|
||||
cy.get('#go-to-ambiguous').click();
|
||||
cy.ionPageVisible('tail-slice-catchall');
|
||||
|
||||
// Back to list via browser back
|
||||
cy.go('back');
|
||||
cy.ionPageVisible('tail-slice-list');
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user