Unify the unlock redirect onto one builder #150

New issue

Closed

opened 2026-09-15 18:23:17 +02:00 by lz · 0 comments

lz commented

2026-09-15 18:23:17 +02:00

Owner

Four sites decide where the operator lands after unlocking, and they disagree. OAuth consent arrives as a cold navigation, which is exactly the path that loses the destination — so this is a prerequisite for MCP. It is worth doing on its own merits regardless.

Verified against the live locked instance

/oauth/authorize?client_id=abc&state=xyz  ->  303  https://a.lck.sh/unlock
/settings?tab=tokens                      ->  303  https://a.lck.sh/unlock

The bounce drops the entire destination, not just the query string. Navigate to /settings?tab=tokens while locked and after unlocking you land on /.

The four sites

Site	Today	Carries destination?
`lib/api/unlock-redirect.ts:50`	hand-built template literal	yes, sanitized
`lib/components/Unlock.svelte:23`	consumer via `sanitizeNext`	reads it
`routes/+layout.server.ts:21`	`redirect(303, required)`	no
`lib/components/Header.svelte:35`	`goto('/unlock')` after Lock	no

Plus a fifth leak in the other direction: +layout.server.ts:25 bounces off the lock screens with redirect(303, '/'), discarding any next already present.

The complete implementation is the client 401 path; the incomplete one runs on a cold navigation.

Once the server bounce learns next, sites 3 and 4 actively fight. lock() calls invalidateAll(), the layout re-runs and bounces to /unlock?next=…, and then the explicit goto('/unlock') overwrites it with a bare one. The duplication stops being a smell and becomes a bug.

Work

One builder in lib/api/unlock-redirect.ts — already outside $lib/server, so both sides run one implementation, the same deliberate choice as lib/git-ref.ts:

export function unlockUrl(dest: string): string

All four callers route through it. +layout.server.ts:25 honours sanitizeNext(url.searchParams.get('next')) instead of hardcoding /.

NO_REDIRECT_FROM moves into the builder, so no caller can emit next=/unlock and loop — subsuming the layout's separate url.pathname !== required guard rather than sitting beside it.

Decisions

next is carried for the unlock screen only, not for every PHASE_ROUTE bounce. needs-setup hands off to needs-unlock immediately, so a next threaded through /setup would be dropped at the next hop — machinery that looks like it works and does not.

Deliberate small UX change: the Lock button now returns you where you were after unlocking. Making it the exception would be a second behaviour to remember.

Done when

A cold navigation to any page while locked returns you to that page after unlocking, query string intact — and a meta-test fails if a '/unlock' literal appears outside unlock-redirect.ts and phase-routes.ts. That test is the guard against a fifth path appearing, so prove it fails by adding one.

Four sites decide where the operator lands after unlocking, and they disagree. OAuth consent arrives as a **cold navigation**, which is exactly the path that loses the destination — so this is a prerequisite for MCP. It is worth doing on its own merits regardless. ## Verified against the live locked instance ``` /oauth/authorize?client_id=abc&state=xyz -> 303 https://a.lck.sh/unlock /settings?tab=tokens -> 303 https://a.lck.sh/unlock ``` The bounce drops the **entire destination**, not just the query string. Navigate to `/settings?tab=tokens` while locked and after unlocking you land on `/`. ## The four sites | Site | Today | Carries destination? | |---|---|---| | `lib/api/unlock-redirect.ts:50` | hand-built template literal | yes, sanitized | | `lib/components/Unlock.svelte:23` | consumer via `sanitizeNext` | reads it | | `routes/+layout.server.ts:21` | `redirect(303, required)` | **no** | | `lib/components/Header.svelte:35` | `goto('/unlock')` after Lock | **no** | Plus a fifth leak in the other direction: `+layout.server.ts:25` bounces *off* the lock screens with `redirect(303, '/')`, discarding any `next` already present. The complete implementation is the client 401 path; the incomplete one runs on a cold navigation. **Once the server bounce learns `next`, sites 3 and 4 actively fight.** `lock()` calls `invalidateAll()`, the layout re-runs and bounces to `/unlock?next=…`, and then the explicit `goto('/unlock')` overwrites it with a bare one. The duplication stops being a smell and becomes a bug. ## Work One builder in `lib/api/unlock-redirect.ts` — already outside `$lib/server`, so both sides run one implementation, the same deliberate choice as `lib/git-ref.ts`: ```ts export function unlockUrl(dest: string): string ``` All four callers route through it. `+layout.server.ts:25` honours `sanitizeNext(url.searchParams.get('next'))` instead of hardcoding `/`. `NO_REDIRECT_FROM` moves **into** the builder, so no caller can emit `next=/unlock` and loop — subsuming the layout's separate `url.pathname !== required` guard rather than sitting beside it. ## Decisions `next` is carried for the **unlock screen only**, not for every `PHASE_ROUTE` bounce. `needs-setup` hands off to `needs-unlock` immediately, so a `next` threaded through `/setup` would be dropped at the next hop — machinery that looks like it works and does not. Deliberate small UX change: the Lock button now returns you where you were after unlocking. Making it the exception would be a second behaviour to remember. ## Done when A cold navigation to any page while locked returns you to that page after unlocking, query string intact — and a meta-test fails if a `'/unlock'` literal appears outside `unlock-redirect.ts` and `phase-routes.ts`. That test is the guard against a fifth path appearing, so prove it fails by adding one.

lz added this to the MCP support (#140) milestone

2026-09-15 18:23:17 +02:00

lz referenced this issue from a pull request that will close it,

2026-09-15 23:02:12 +02:00

Unify the unlock redirect onto one builder #165

lz closed this issue

2026-09-15 23:05:19 +02:00