fix(router): Allow redirects after an absolute redirect (#51731)

The router currently restricts all further redirects after an absolute
redirect. Because there's no documented reason for _why_ this
restriction is in place, I'm now deeming this unnecessary. Developers
should not be restricted in this manner. Instead, configs that may
have caused infinite redirects in the past should be updated to not be
infinite. It is confusing to ignore configs with redirects after an
absolute redirect occurred because it creates different matching rules
depending on the whether an absolute redirect has happened or not.

For additional context on why I believe removing this restriction is
necessary, #13373 asks for allowing `redirectTo` to be a function. It
would make sense to allow this function to return a `UrlTree` like other
guards in the Router. When guards in the `Router` return `UrlTree`, they
cancel the current navigation and start a new one to re-do the route
matching. Since we're already in the router matching part, we don't need
to cancel the navigation. However, the restriction on absolute redirects
here then creates a weird situation where developers wouldn't see any
other redirects if they returned a `UrlTree` as an absolute redirect
from `redirectTo`.

resolves #39770

BREAKING CHANGE: Absolute redirects no longer prevent further redirects.
Route configurations may need to be adjusted to prevent infinite
redirects where additional redirects were previously ignored after an
absolute redirect occurred.

PR Close #51731

goldens/public-api/router/errors.md

@@ -11,6 +11,8 @@ export const enum RuntimeErrorCode {
     // (undocumented)
     FOR_ROOT_CALLED_TWICE = 4007,
     // (undocumented)
+    INFINITE_REDIRECT = 4016,
+    // (undocumented)
     INVALID_DOUBLE_DOTS = 4005,
     // (undocumented)
     INVALID_ROOT_URL_SEGMENT = 4015,

packages/router/src/errors.ts

@@ -26,4 +26,5 @@ export const enum RuntimeErrorCode {
   OUTLET_ALREADY_ACTIVATED = 4013,
   INVALID_ROUTE_CONFIG = 4014,
   INVALID_ROOT_URL_SEGMENT = 4015,
+  INFINITE_REDIRECT = 4016,
 }

packages/router/src/models.ts

@@ -519,7 +519,6 @@ export interface Route {
    * A URL to redirect to when the path matches.
    *
    * Absolute if the URL begins with a slash (/), otherwise relative to the path URL.
-   * Note that no further redirects are evaluated after an absolute redirect.
    *
    * When not present, router does not redirect.
    */

packages/router/src/recognize.ts

@@ -10,7 +10,7 @@ import {EnvironmentInjector, Type, ɵRuntimeError as RuntimeError} from '@angula
 import {from, Observable, of} from 'rxjs';
 import {catchError, concatMap, defaultIfEmpty, first, last as rxjsLast, map, mergeMap, scan, switchMap, tap} from 'rxjs/operators';
 
-import {absoluteRedirect, AbsoluteRedirect, ApplyRedirects, canLoadFails, noMatch, NoMatch} from './apply_redirects';
+import {AbsoluteRedirect, ApplyRedirects, canLoadFails, noMatch, NoMatch} from './apply_redirects';
 import {createUrlTreeFromSnapshot} from './create_url_tree';
 import {RuntimeErrorCode} from './errors';
 import {Data, LoadedRouterConfig, ResolveData, Route, Routes} from './models';
@@ -44,9 +44,12 @@ export function recognize(
       .recognize();
 }
 
+const MAX_ALLOWED_REDIRECTS = 31;
+
 export class Recognizer {
-  allowRedirects = true;
   private applyRedirects = new ApplyRedirects(this.urlSerializer, this.urlTree);
+  private absoluteRedirectCount = 0;
+  allowRedirects = true;
 
   constructor(
       private injector: EnvironmentInjector, private configLoader: RouterConfigLoader,
@@ -86,13 +89,11 @@ export class Recognizer {
   }
 
 
-  private match(root: UrlSegmentGroup): Observable<TreeNode<ActivatedRouteSnapshot>[]> {
-    const expanded$ = this.processSegmentGroup(this.injector, this.config, root, PRIMARY_OUTLET);
+  private match(rootSegmentGroup: UrlSegmentGroup): Observable<TreeNode<ActivatedRouteSnapshot>[]> {
+    const expanded$ =
+        this.processSegmentGroup(this.injector, this.config, rootSegmentGroup, PRIMARY_OUTLET);
     return expanded$.pipe(catchError((e: any) => {
       if (e instanceof AbsoluteRedirect) {
-        // After an absolute redirect we do not apply any more redirects!
-        // If this implementation changes, update the documentation note in `redirectTo`.
-        this.allowRedirects = false;
         this.urlTree = e.urlTree;
         return this.match(e.urlTree.root);
       }
@@ -216,7 +217,7 @@ export class Recognizer {
       return this.matchSegmentAgainstRoute(injector, rawSegment, route, segments, outlet);
     }
 
-    if (allowRedirects && this.allowRedirects) {
+    if (this.allowRedirects && allowRedirects) {
       return this.expandSegmentAgainstRouteUsingRedirect(
           injector, rawSegment, routes, route, segments, outlet);
     }
@@ -237,6 +238,22 @@ export class Recognizer {
                               match(segmentGroup, route, segments);
     if (!matched) return noMatch(segmentGroup);
 
+    // TODO(atscott): Move all of this under an if(ngDevMode) as a breaking change and allow stack
+    // size exceeded in production
+    if (route.redirectTo!.startsWith('/')) {
+      this.absoluteRedirectCount++;
+      if (this.absoluteRedirectCount > MAX_ALLOWED_REDIRECTS) {
+        if (ngDevMode) {
+          throw new RuntimeError(
+              RuntimeErrorCode.INFINITE_REDIRECT,
+              `Detected possible infinite redirect when redirecting from '${this.urlTree}' to '${
+                  route.redirectTo}'.\n` +
+                  `This is currently a dev mode only error but will become a` +
+                  ` call stack size exceeded error in production in a future major version.`);
+        }
+        this.allowRedirects = false;
+      }
+    }
     const newTree = this.applyRedirects.applyRedirectCommands(
         consumedSegments, route.redirectTo!, positionalParamSegments);
 

packages/router/test/apply_redirects.spec.ts

@@ -176,6 +176,19 @@ describe('redirects', () => {
         });
   });
 
+  it('should throw an error on infinite absolute redirect', () => {
+    recognize(
+        TestBed.inject(EnvironmentInjector), TestBed.inject(RouterConfigLoader), null,
+        [{path: '**', redirectTo: '/404'}], tree('/'), new DefaultUrlSerializer())
+        .subscribe({
+          next: () => fail('expected infinite redirect error'),
+          error: e => {
+            expect((e as Error).message).toMatch(/infinite redirect/);
+          }
+        });
+  });
+
+
   it('should support absolute redirects', () => {
     checkRedirect(
         [
@@ -1216,8 +1229,9 @@ describe('redirects', () => {
     it('should work when using absolute redirects (wildcard)', () => {
       checkRedirect(
           [
-            {path: '**', redirectTo: '/b(aux:c)'}, {path: 'b', component: ComponentB},
-            {path: 'c', component: ComponentC, outlet: 'aux'}
+            {path: 'b', component: ComponentB},
+            {path: 'c', component: ComponentC, outlet: 'aux'},
+            {path: '**', redirectTo: '/b(aux:c)'},
           ],
           'a/1', (t: UrlTree) => {
             expectTreeToBe(t, '/b(aux:c)');