DEV Community

Daniel Pertu
Daniel Pertu

Posted on

Three signals competed for the post-login redirect, and ranking them was the whole fix

Somebody finishes signing in. Where do they go?

The answer sounds like one line of code and it is three signals of wildly different ages arguing with each other. Getting the argument wrong does not produce an error, it produces a user who clicked a link to one page and arrived at another, which is the kind of bug that gets reported as "the site is weird".

The three signals

A next query parameter. A deep link the user clicked seconds ago: an email confirmation link, or a bounce off a page that required sign-in. This is the most recent and most deliberate thing we know about them.

A ?provider= on the signup URL. Also seconds old, also deliberate, but only present on links we deliberately built that way.

A first touch entry intent. Recorded in localStorage the first time somebody arrives from a provider page or the assessment centre page, and deliberately never overwritten afterwards, so the earliest signal in a visit is the one that sticks. It survives 30 days.

That third one is the interesting one, because it is simultaneously the signal that covers the common case and the signal most likely to be wrong.

Most visitors do not click a link carrying ?provider=. They browse /games/shl, then click the ordinary "Sign up" in the header. Before the first touch fallback existed they arrived with no intent at all and got dropped on a generic dashboard, despite us knowing exactly what they came for. So it earns its place.

But it is up to a month old. Here is my own browser, right now, after opening a Hogan page:

{"kind":"provider","id":"arctic-shores","at":1789036080966}
Enter fullscreen mode Exit fullscreen mode

I was looking at Hogan. The record says Arctic Shores, because that is where I first arrived, weeks ago, and first touch never overwrites. That is correct behaviour for a first touch record and it is a terrible basis for deciding where to send me next.

The precedence, and the bug it prevents

export function postAuthDestination({ next, landingIntent }: {
  next?: string | null;
  landingIntent?: string | null;
}): string {
  return internalPath(next) ?? destinationForLandingIntent(landingIntent, DEFAULT_DESTINATION);
}
Enter fullscreen mode Exit fullscreen mode

next wins outright. The whole function is that one line, and the ordering is the entire content.

Without it, the failure is specific and quiet: a returning visitor who browsed a provider page a fortnight ago clicks a password reset link, or an email confirmation, and gets dropped into a practice game for a provider they were not thinking about. Their next is silently discarded. Nothing throws, nothing logs, and the person who reports it will describe it as the link being broken.

The general shape, which I think applies well beyond redirects: when two sources of intent disagree, rank them by recency of deliberate action, not by specificity. The stale signal is often the more specific one, and specificity is exactly what makes a wrong answer feel confident.

One pure module, because two callers needed the same answer

The function lives in its own file with no server-only imports, and that constraint is doing real work.

Three different things need to agree about this:

  1. the OAuth callback, a route handler, which clears the cookie on its own NextResponse;
  2. the email signup server action, which clears it through cookies();
  3. a client component, which is what writes the cookie in the first place.

Those are three different execution contexts. They cannot share a cookie helper. What they can share is the decision, and that is what this module exports: the cookie name, the encoder, the decoder and the precedence rule, all as plain functions over strings.

The two server paths previously each had their own copy and had already drifted on what to fall back to. The point worth taking is that sharing the lookup is not enough. If one caller reads the cookie and then applies its own idea of precedence, you have shared the cheap part and duplicated the part that was actually hard.

The client component imports LANDING_INTENT_COOKIE and encodeLandingIntent from the same file the servers read with. The writer and the readers cannot disagree about the name or the format, because there is only one of each.

The encoding carries the kind explicitly

const ASSESSMENT_CENTRE_VALUE = 'assessment-centre';
const PROVIDER_VALUE_PREFIX = 'provider:';

export function encodeLandingIntent(intent: EntryIntentInput): string {
  return intent.kind === 'provider'
    ? `${PROVIDER_VALUE_PREFIX}${intent.id}`
    : ASSESSMENT_CENTRE_VALUE;
}
Enter fullscreen mode Exit fullscreen mode

The tempting version writes a bare provider slug and treats one magic string as the other case. That works until somebody onboards a provider whose slug happens to be the sentinel, which is a bug nobody will find by reading the code, only by shipping the provider.

The provider: prefix means the reader never has to rely on a collision not happening.

function destinationForLandingIntent(value: string | null | undefined, fallback: string): string {
  if (!value) return fallback;
  if (value === ASSESSMENT_CENTRE_VALUE) return ASSESSMENT_CENTRE_DESTINATION;
  if (value.startsWith(PROVIDER_VALUE_PREFIX)) {
    return getFirstFreeGameRoute(value.slice(PROVIDER_VALUE_PREFIX.length)) ?? fallback;
  }
  return fallback;
}
Enter fullscreen mode Exit fullscreen mode

Everything unrecognised falls through to the fallback. That covers forged values, and it also covers values written by an older deployment before the encoding changed, which matters because a cookie in somebody's browser is a piece of state from a previous version of your code that you cannot migrate. During any rollout, both formats are live at once. A decoder that throws on an unknown shape turns a deploy into an outage for everyone mid-flow.

The open redirect, and the character that causes it

function internalPath(next: string | null | undefined): string | null {
  if (!next || !next.startsWith('/')) return null;
  if (next.startsWith('//') || next.startsWith('/\\')) return null;
  return next;
}
Enter fullscreen mode Exit fullscreen mode

Three rejections, and the third is the one worth the post.

Rejecting anything not starting with / is obvious: it stops https://evil.com.

Rejecting // is the well known one: //evil.com is a protocol relative URL. It starts with a slash, so the naive check passes, and the browser reads it as "same scheme, different host".

/\evil.com is the same attack wearing a different first character, and it defeats a check that only looks for //. The URL parser normalises backslashes to forward slashes in the authority position of an http or https URL, which is a compatibility behaviour inherited from Windows-era path handling and is specified, not a quirk. So /\evil.com is parsed exactly as //evil.com, and a redirect to it leaves your site.

If you have a next, redirect, returnTo or continue parameter anywhere, that is the test case to add. It is a single character away from the one everybody already blocks.

The cookie is never a path

Worth stating plainly, because it is the property that makes the rest safe: the landing intent cookie is never used as a destination. It selects between a fixed set of internal constants, and the provider branch runs the slug through getFirstFreeGameRoute, which only returns routes that exist in the game library.

So the worst an attacker can do by writing whatever they like into that cookie is send themselves to the dashboard.

The slug gets validated twice on purpose, once on the client against the provider list before it is written, and once on the server before it is used. The client check is not security, localStorage is user writable and the user owns their own browser. It is there so a stale or corrupt value is dropped early rather than becoming a cookie that the server then has to reject. The server check is the one that counts.

Two lifetimes, chosen separately

The localStorage record lives 30 days. The cookie derived from it lives 30 minutes:

document.cookie = `${LANDING_INTENT_COOKIE}=${encodeURIComponent(
  encodeLandingIntent(intent)
)}; path=/; max-age=1800; samesite=lax`;
Enter fullscreen mode Exit fullscreen mode

Different jobs, so different durations. The localStorage record answers "what did this person originally come here for", which is a fact about them that stays true. The cookie answers "this person is signing up right now, and this is what for", which is only true for the length of a signup, and half an hour is generous for that.

SameSite=Lax rather than Strict is not a default, it is a requirement. OAuth sends the user to a provider and back to /auth/callback as a top level navigation from another site. Strict withholds the cookie on exactly that navigation, so the signal would survive everything except the flow it exists for.

See it

Signed out, in a private window:

  1. Open cogniprep.app/games/hogan. In DevTools, Application, Local Storage, look for cogniprep:entry-intent. You will find {"kind":"provider","id":"hogan","at":...}.
  2. Now open cogniprep.app/games/shl and look again. Still Hogan. That is first touch refusing to be overwritten, and it is the reason the precedence rule in this post has to exist.
  3. Open cogniprep.app/signup and check Application, Cookies. A landing_intent cookie appears with the value provider:hogan, with a 30 minute expiry. That is the handoff from browser storage the server cannot read to a cookie it can.

Then edit the cookie to provider:not-a-real-provider and complete a signup. You get the dashboard, because the slug does not resolve to a route. The cookie is a hint the server is allowed to ignore, which is the only kind of hint a user-writable value is allowed to be.

Top comments (0)