Visitar URL original
fix(router): require outlets to match a route before processing child… · angular/angular@b0d2ba0 · GitHub
Skip to content

Commit b0d2ba0

Browse files
atscottatscott
committed
fix(router): require outlets to match a route before processing child segments
Previously, empty-path outlet groups in a request URL (such as `x:/(detail:2)`, `///(detail:2)`, or `x:/()`) were allowed to match without a corresponding route for that outlet name in the route configuration: 1. `processChildren` called `processSegmentGroup` for each child outlet, which immediately delegated any zero-segment group with children back to `processChildren` against the same parent `config` and `parentRoute` without matching a route. Calling `processSegment` directly in `processChildren` ensures only the root `UrlSegmentGroup` short-circuits to `processChildren`. 2. `processSegmentAgainstRoute` allowed named outlets to pierce empty-path routes for other outlets even when the route had no children to pierce into or the URL segment group had no segments or children to pierce with. 3. `matchSegmentAgainstRoute` branched into `processChildren` or returned a completed leaf match before `getOutlet(route) === outlet` was satisfied, and did not require a descendant `TreeNode` match when piercing an empty-path route for a different outlet. 4. `noLeftoversInUrl` only checked `!segmentGroup.children[outlet]` rather than `!segmentGroup.hasChildren()`, allowing unmatched zero-segment outlet groups with child outlets for other names to be treated as having no leftovers instead of throwing `NoMatch`. Together, these allowed crafted URLs with unconfigured zero-segment outlet groups to fan out across empty-path routes (#70932) or repeatedly evaluate guards and construct duplicate route snapshot subtrees across sibling outlets (#70997). Requiring every child segment group to match a route for its outlet before unwrapping children or completing a match prevents the invalid matches at the source. Fixes #70932 Fixes #70997
1 parent 83944fc commit b0d2ba0

3 files changed

Lines changed: 184 additions & 17 deletions

File tree

‎packages/router/src/recognize.ts‎

Lines changed: 29 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -223,12 +223,22 @@ export class Recognizer {
223223
// appear first, followed by routes for other outlets, which might match if they have
224224
// an empty path.
225225
const sortedConfig = sortByMatchingOutlets(config, childOutlet);
226-
return this.processSegmentGroup(injector, sortedConfig, child, childOutlet, parentRoute);
226+
return this.processSegment(
227+
injector,
228+
sortedConfig,
229+
child,
230+
child.segments,
231+
childOutlet,
232+
true,
233+
parentRoute,
234+
);
227235
}),
228-
scan((children, outletChildren) => {
229-
children.push(...outletChildren);
236+
scan((children, outletChild) => {
237+
if (outletChild instanceof TreeNode) {
238+
children.push(outletChild);
239+
}
230240
return children;
231-
}),
241+
}, [] as TreeNode<ActivatedRouteSnapshot>[]),
232242
defaultIfEmpty(null as TreeNode<ActivatedRouteSnapshot>[] | null),
233243
last(),
234244
mergeMap((children) => {
@@ -308,7 +318,12 @@ export class Recognizer {
308318
// This should only match if the url is `/(x:b)`.
309319
if (
310320
getOutlet(route) !== outlet &&
311-
(outlet === PRIMARY_OUTLET || !emptyPathMatch(rawSegment, segments, route))
321+
(outlet === PRIMARY_OUTLET ||
322+
!emptyPathMatch(rawSegment, segments, route) ||
323+
// the route has no children to pierce into.
324+
(!route.children?.length && !route.loadChildren) ||
325+
// the URL segment group has no segments or children to pierce with.
326+
(segments.length === 0 && !rawSegment.hasChildren()))
312327
) {
313328
return noMatch(rawSegment);
314329
}
@@ -457,19 +472,20 @@ export class Recognizer {
457472
childConfig,
458473
);
459474

460-
if (slicedSegments.length === 0 && segmentGroup.hasChildren()) {
475+
const matchedOnOutlet = getOutlet(route) === outlet;
476+
477+
if (matchedOnOutlet && slicedSegments.length === 0 && segmentGroup.hasChildren()) {
461478
return this.processChildren(childInjector, childConfig, segmentGroup, snapshot).pipe(
462479
map((children) => {
463480
return new TreeNode(snapshot, children);
464481
}),
465482
);
466483
}
467484

468-
if (childConfig.length === 0 && slicedSegments.length === 0) {
485+
if (matchedOnOutlet && childConfig.length === 0 && slicedSegments.length === 0) {
469486
return of(new TreeNode(snapshot, []));
470487
}
471488

472-
const matchedOnOutlet = getOutlet(route) === outlet;
473489
// If we matched a config due to empty path match on a different outlet, we need to
474490
// continue passing the current outlet for the segment rather than switch to PRIMARY.
475491
// Note that we switch to primary when we have a match because outlet configs look like
@@ -487,8 +503,11 @@ export class Recognizer {
487503
true,
488504
snapshot,
489505
).pipe(
490-
map((child) => {
491-
return new TreeNode(snapshot, child instanceof TreeNode ? [child] : []);
506+
switchMap((child) => {
507+
if (!matchedOnOutlet && !(child instanceof TreeNode)) {
508+
return noMatch(rawSegment);
509+
}
510+
return of(new TreeNode(snapshot, child instanceof TreeNode ? [child] : []));
492511
}),
493512
);
494513
}),

‎packages/router/src/utils/config_matching.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -215,5 +215,5 @@ export function noLeftoversInUrl(
215215
segments: UrlSegment[],
216216
outlet: string,
217217
): boolean {
218-
return segments.length === 0 && !segmentGroup.children[outlet];
218+
return segments.length === 0 && !segmentGroup.hasChildren();
219219
}

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

Lines changed: 154 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -578,6 +578,155 @@ describe('recognize', () => {
578578
.toPromise();
579579
await expectAsync(recognizePromise).toBeRejected();
580580
});
581+
582+
it('does not evaluate sibling routes under unconfigured zero-segment outlets', async () => {
583+
let canMatchCalls = 0;
584+
const config = [
585+
{
586+
path: 'a',
587+
children: [
588+
{
589+
path: '',
590+
outlet: 'detail',
591+
children: [
592+
{
593+
path: ':id',
594+
component: ComponentA,
595+
canMatch: [
596+
() => {
597+
canMatchCalls++;
598+
return true;
599+
},
600+
],
601+
},
602+
],
603+
},
604+
],
605+
},
606+
];
607+
await expectAsync(
608+
recognize(config, 'a/(detail:1//x0:/(detail:2)//x1:/(detail:3)//x2:/(detail:4))'),
609+
).toBeRejectedWithError(/Cannot match any routes/);
610+
expect(canMatchCalls).toBe(1);
611+
612+
canMatchCalls = 0;
613+
await expectAsync(
614+
recognize(config, 'a/(detail:1///(detail:2///(detail:3)))'),
615+
).toBeRejectedWithError(/Cannot match any routes/);
616+
expect(canMatchCalls).toBe(0);
617+
});
618+
619+
it('does not match empty-path primary routes against unconfigured outlets', async () => {
620+
let canMatchCalls = 0;
621+
const s1 = await recognize(
622+
[
623+
{
624+
path: 'a',
625+
children: [
626+
{
627+
path: '',
628+
component: ComponentA,
629+
canMatch: [
630+
() => {
631+
canMatchCalls++;
632+
return true;
633+
},
634+
],
635+
},
636+
],
637+
},
638+
],
639+
'a/(x0:/()//x1:/()//x2:/())',
640+
);
641+
expect(canMatchCalls).toBe(1);
642+
expect(s1.root.firstChild!.children.length).toBe(1);
643+
644+
canMatchCalls = 0;
645+
const s2 = await recognize(
646+
[
647+
{
648+
path: 'shop',
649+
children: [
650+
{
651+
path: '',
652+
canMatch: [
653+
() => {
654+
canMatchCalls++;
655+
return true;
656+
},
657+
],
658+
children: [{path: '', component: ComponentA}],
659+
},
660+
],
661+
},
662+
],
663+
'shop/(a:/()//b:/()//c:/())',
664+
);
665+
expect(canMatchCalls).toBe(1);
666+
expect(s2.root.firstChild!.children.length).toBe(1);
667+
});
668+
669+
it('allows navigation when an empty-path named outlet child fails canMatch', async () => {
670+
const config = [
671+
{
672+
path: 'dashboard',
673+
component: ComponentA,
674+
children: [
675+
{path: 'home', component: ComponentC},
676+
{
677+
path: '',
678+
outlet: 'aux',
679+
component: ComponentB,
680+
canMatch: [() => false],
681+
},
682+
],
683+
},
684+
];
685+
686+
const s1 = await recognize(config, 'dashboard');
687+
checkActivatedRoute(s1.root.firstChild!, 'dashboard', {}, ComponentA);
688+
expect(s1.root.firstChild!.children.length).toBe(0);
689+
690+
const s2 = await recognize(config, 'dashboard/home');
691+
checkActivatedRoute(s2.root.firstChild!, 'dashboard', {}, ComponentA);
692+
expect(s2.root.firstChild!.children.length).toBe(1);
693+
checkActivatedRoute(s2.root.firstChild!.firstChild!, 'home', {}, ComponentC);
694+
695+
const s3 = await recognize(
696+
[
697+
{path: '', outlet: 'aux', canMatch: [() => false], component: ComponentA},
698+
{path: '', children: [{path: '', outlet: 'aux', component: ComponentB}]},
699+
],
700+
'',
701+
);
702+
expect(s3.root.children.length).toBe(1);
703+
expect(s3.root.firstChild!.children.length).toBe(1);
704+
checkActivatedRoute(s3.root.firstChild!.firstChild!, '', {}, ComponentB, 'aux');
705+
});
706+
707+
it('allows matching only the first level of a named outlet config with children', async () => {
708+
const s = await recognize(
709+
[
710+
{
711+
path: 'dashboard',
712+
component: ComponentA,
713+
children: [
714+
{
715+
path: 'abc',
716+
outlet: 'detail',
717+
component: ComponentB,
718+
children: [{path: 'sub', component: ComponentC}],
719+
},
720+
],
721+
},
722+
],
723+
'dashboard/(detail:abc)',
724+
);
725+
checkActivatedRoute(s.root.firstChild!, 'dashboard', {}, ComponentA);
726+
expect(s.root.firstChild!.children.length).toBe(1);
727+
checkActivatedRoute(s.root.firstChild!.firstChild!, 'abc', {}, ComponentB, 'detail');
728+
expect(s.root.firstChild!.firstChild!.children.length).toBe(0);
729+
});
581730
});
582731
});
583732

@@ -810,7 +959,7 @@ describe('recognize', () => {
810959
it('shares one queryParams object across every snapshot', async () => {
811960
const s = await recognize(
812961
[{path: 'a', children: [{path: '', children: [{path: '', component: ComponentA}]}]}],
813-
'a/(x:/()//y:/()//z:/())?p=1&q=2',
962+
'a?p=1&q=2',
814963
);
815964
const snapshots = collectSnapshots(s.root);
816965
expect(snapshots.length).toBeGreaterThan(1);
@@ -836,7 +985,6 @@ describe('recognize', () => {
836985
);
837986
const a = s.root.firstChild!;
838987
expect(a.params).toEqual({x: '1'});
839-
// An empty path route matches once for every outlet the URL names.
840988
expect(a.children.length).toBe(2);
841989
for (const child of a.children) {
842990
expect(child.params).toBe(a.params);
@@ -846,21 +994,21 @@ describe('recognize', () => {
846994
});
847995

848996
describe('outlet name uniqueness', () => {
849-
// The two `detail` matches merge into one node, so the duplicate is among its children.
997+
// The two empty-path matches merge into one node, so the duplicate `detail` outlet is among its children.
850998
it('rejects duplicates that appear only after empty path matches are merged', async () => {
851999
await expectAsync(
8521000
recognize(
8531001
[
8541002
{
8551003
path: 'a',
8561004
children: [
857-
{path: '', outlet: 'detail', children: [{path: ':id', component: ComponentA}]},
1005+
{path: '', children: [{path: ':id', outlet: 'detail', component: ComponentA}]},
8581006
],
8591007
},
8601008
],
861-
'a/(detail:1//x:/(detail:2))',
1009+
'a/(detail:1///(detail:2))',
8621010
),
863-
).toBeRejectedWithError(/Two segments cannot have the same outlet name: '1' and '2'/);
1011+
).toBeRejectedWithError(/Two segments cannot have the same outlet name: '2' and '1'/);
8641012
});
8651013

8661014
it('does not treat an outlet named after an Object member as a duplicate', async () => {

0 commit comments

Comments
 (0)