Angular Router state committed before activation can make a valid error-handler redirect loop forever #71126

Which @angular/* package(s) are the source of the bug?

router

Is this a regression?

No

Description

An app that sends navigation failures to an error page the documented way never finishes the redirect:

withNavigationErrorHandler(() => new RedirectCommand(inject(Router).parseUrl('/error')));

/error renders fine on its own. But GET /(//) makes the SSR worker stop answering everything, while it spins the event loop.

It comes down to an ordering problem. commitTransition() runs on BeforeActivateRoutes, before the browser URL is written:

else if (e instanceof BeforeActivateRoutes) {
  this.commitTransition(currentTransition);
  if (this.urlUpdateStrategy === 'deferred' && !currentTransition.extras.skipLocationChange) {
    this.setBrowserUrl(this.createBrowserPath(currentTransition), currentTransition);
  }
}

ActivatedRoute.snapshot is only assigned later, by advanceActivatedRoute() during activation. So a throw in between leaves a committed routerState whose routes have no snapshot.

Normally that gets repaired, because NavigationError calls restoreHistory(t, true). A RedirectCommand takes the other branch instead, and that branch is excluded:

else if (e instanceof NavigationCancel && !isRedirectingEvent(e)) {
  this.restoreHistory(currentTransition);
}

So the redirect runs with the half-committed state as its previous state, and DefaultRouteReuseStrategy reads it unguarded:

shouldReuseRoute(future, curr) {
  return future.routeConfig === curr.routeConfig;   // curr is undefined
}

That throws, the handler answers with another RedirectCommand, and it never ends.

The URL is not the only way into that window. ActivateRoutes.activate() advances one route at a time, and the component is created in between:

activateRoutes(futureNode, currNode, parentContexts) {
  advanceActivatedRoute(future);                 // this route gets its snapshot
  // ...then context.outlet.activateWith() creates the component, and that is what throws
}

With one outlet the route is already advanced when the component throws, so nothing is left behind. With two, the primary advances, its component throws, and the sibling outlet never advances. Same half-committed state, no odd URL involved.

Minimal Reproduction

@Component({selector: 'app-login', template: '<p>LOGIN-OK</p>'})
export class Login {
  // guards pass returnUrl around encoded, so it gets decoded again here
  protected readonly returnUrl = decodeURIComponent(
    inject(ActivatedRoute).snapshot.queryParamMap.get('returnUrl') ?? '/',
  );
}

export const routes: Routes = [
  { path: 'shell', component: Shell },
  { path: 'error', component: ErrorPage },
  // control: an ordinary resolver failure, no commit-window throw
  {
    path: 'boom',
    component: Shell,
    resolve: {
      x: () => {
        throw new Error('nope');
      },
    },
  },
  { path: 'login', component: Login },
  { path: 'room/:id', outlet: 'chat', component: Chat },
  { path: '**', component: Shell },
];

<router-outlet /><router-outlet name="chat" /> in the shell, <base href="/"> and urlUpdateStrategy: 'deferred' left at their defaults.

Case A, the browser URL write fails:

localhost:4000/boom         # 302, recovers in one hop
localhost:4000/(//)         # Never answers, spins the event loop

/boom is the control: it hits the same handler and the same /error, and recovers in one hop. /(//) never answers.

Case B, a component fails next to a named outlet. ?returnUrl=%25 is a correctly encoded %, so the URL itself is valid. The Router hands the component a decoded value already, so decodeURIComponent gets % and throws URIError: URI malformed:

localhost:4000/login?returnUrl=%25                    # 302, throws and recovers in one hop
localhost:4000/login(chat:room/42)?returnUrl=%25      # Never answers

The first line is the control. Same throw, one hop, worker fine. The only difference on the second is (chat:room/42), and that comes from the request, not from the configuration.

The error chain, written synchronously because nothing async flushes once the loop starts:

ERRCHAIN #1 nope                                                   <- /boom, stops here
ERRCHAIN #2 NG05701: //                                            <- /(//)
ERRCHAIN #3 Cannot read properties of undefined (reading 'routeConfig')
ERRCHAIN #4 Cannot read properties of undefined (reading 'routeConfig')
...

Case B produces the same chain, with URI malformed in place of NG05701.

Please provide a link to a minimal reproduction of the bug

See https://github.com/SkyZeroZx/angular-ssr-router-redirect-loop

Anything else?

I narrowed this down to RedirectCommand. With the same setup, no handler or a handler returning undefined gives a normal 404, and router.navigate('/error') is noisy but settles. Only returning a RedirectCommand never completes, through either withNavigationErrorHandler() or RouterModule.forRoot({errorHandler}).

The setup is otherwise default. Case A needs the ** route, because without it /(//) fails before the router commits anything. Case B needs the named outlet in the shell template instead, and the unactivated route has to be a sibling rather than a descendant.

#71094 fixes the /(//) trigger by preventing DefaultUrlSerializer from producing protocol relative paths (reported at #69700), but it only removes one way into the window above.

Case B still reproduces with it applied. A try/catch around setBrowserUrl() has the same gap: it handles one source of the throw, but the state may already be committed.

What covers both is restoring the internal state when a redirecting navigation is canceled, but only when that transition actually committed (this.routerState === targetRouterState).

The browser URL does not need restoring, since the redirect replaces it. Adding curr?.routeConfig in shouldReuseRoute() stops the crash but leaves the state inconsistent.