lz/agent-nexus

Fork 0

Unify the unlock redirect onto one builder #165

Merged

lz merged 3 commits from fix/unlock-redirect into main

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

lz commented

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

Owner

Closes #150.

Four sites decided where a locked operator lands and they disagreed. The client 401 bounce carried a sanitized destination; the phase gate and the Lock button carried nothing, and the bounce back off the lock screens hardcoded /. A cold navigation — the path OAuth consent will arrive on (#151) — therefore dropped the whole destination.

What changed

lib/api/unlock-redirect.ts is the only place that builds an unlock URL or reads one back:

	before	now
`lib/api/unlock-redirect.ts`	hand-built template literal	`unlockUrl(dest)`
`routes/+layout.server.ts` phase gate	`redirect(303, required)`	`unlockUrl(url.pathname + url.search)`
`routes/+layout.server.ts` bounce out	`redirect(303, '/')`	`destFromSearch(url.search)`
`lib/components/Header.svelte` Lock	`goto('/unlock')`	nothing — `invalidateAll()` lets the layout decide
`lib/components/Unlock.svelte`	`sanitizeNext(...)`	`destFromSearch(window.location.search)`

UNLOCK_ROUTE is the single literal and phase-routes.ts imports it. destFromSearch is the one reader of next, so the server (url.search) and the client (window.location.search) agree about an absent, empty, repeated or differently-cased parameter by construction rather than by luck. The next is read only on the screen that writes one.

The Lock button stops navigating entirely. invalidateAll() already follows the layout's redirect (_invalidate() awaits _goto() on a redirect result, kit 2.59.1), so an explicit goto was a second decision to keep in step — and it pushed a history entry over the redirect's replaceState, so Back returned to the URL you were already on. Setup.svelte already makes the /setup → /unlock hop on a bare refresh().

The sites-3-and-4 fight in the issue is real, and measured rather than reasoned: with the new layout and the old goto('/unlock') left in place, clicking Lock on /settings?tab=tokens produced /unlock?next=%2Fsettings%3Ftab%3Dtokens and then overwrote it with a bare /unlock, landing on / after unlocking. The fix is not ordering — it is that the second decision is gone.

NO_REDIRECT_FROM did NOT subsume the layout's loop guard, and could not. They answer different questions: url.pathname !== required asks "am I already at this phase's route" and all three gated phases need it, while the builder's rule governs what a next may point at. Drop the pathname test and unlockUrl on the unlock screen returns a bare /unlock — its own fixed point, so an infinite redirect. Both are stated where they live rather than left as silent neighbours.

A security defect the review caught

The third commit fixes an open redirect that the first commit made reachable. Browsers strip tab, CR and LF from a URL before parsing, so /<tab>/evil.com passes "starts with / and is not //" and then resolves to //evil.com; a tab is a legal HTTP field-value character, so it survives a Location header. Measured against the previous commit on a live instance:

GET /unlock?next=%2F%09%2Fevil.com   ->   303  Location: /<TAB>/evil.com

On main this was latent: sanitizeNext's only consumer was Unlock.svelte's goto, and SvelteKit's public goto rejects a cross-origin URL. Teaching the bounce to honour next put it in a header, where nothing checks anything. The same shape walked /./unlock, /x/../unlock and /unlock/ past the lock-screen rule, and /%75nlock past it in a way that parked an unlocked operator back on the passphrase form — a small regression from main.

Fix: sanitizeNext refuses anything the URL parser would rewrite (refuse, never repair — the rule the module already had), and the lock-screen comparison resolves the route the way the router does, decodeURI included, per fact #23. This deviates from the issue's "keep sanitizeNext as is" — deliberately, and only by strengthening it; every existing rule is untouched.

Verified live after the fix (/unlock?next=… while unlocked): %2F%09%2Fevil.com → /, %2F%2575nlock → /, %2F.%2Funlock → /, %2Fsettings%3Ftab%3Dtokens → /settings?tab=tokens.

Gates

1825 tests / 157 files green, typecheck 0 errors across 4960 files, lint clean.

Guards proven able to fail

Each mutation applied, the named test confirmed red, then restored from a copy (never git checkout):

broken	went red
phase gate back to `redirect(303, required)`	carries the whole destination
bounce back to `redirect(303, '/')`	returns an already-unlocked operator to their destination
loop guard → `if (true)`	does not redirect a gated phase already on its route; leaves a destination parked on the unlock screen alone
lock-screen rule removed	13 tests across both files
query/fragment strip removed	7 tests
`isCanonical` removed	20 tests — tab/CR/LF and dot-segment cases, unit and layout
`decodeURI` removed	7 tests, including the property test — the proof it no longer restates the implementation
trailing-slash trim removed	6 tests
`next` read widened to all gate routes	ignores a next on the gate screens that never write one
`303` → `307`	21 tests
a stray `'/unlock'` added to `lib/keepalive.ts`	the meta-test's offender check
a dormant `ALLOWED` entry re-added	the new bidirectional allowlist test

The meta-test caught two real hits I did not know about on its first run (hooks.server.ts, lib/api/client.ts — both /api/auth/unlock, which is why the regex has a leading lookbehind).

Browser-verified

A real instance (fake claude-session volume → needs-provider, so the whole app renders), driven with playwright-cli on the final code: cold /settings?tab=tokens while locked → /unlock?next=%2Fsettings%3Ftab%3Dtokens → unlock → back on /settings?tab=tokens; Lock → /unlock?next=%2Fsettings%3Ftab%3Dtokens → unlock → back again; both attack URLs → /, same origin.

Rejected review findings

Derive the lock-screen set from PHASE_ROUTE. It would add /claude-session, which is a gate but not a lock screen, and stop honouring next=/claude-session. The sets differ on purpose.
Carry next through every gated phase. Getting the operator back needs each gate screen's completion to honour it; none does, so it would look like it works. This is the issue's decision 1 and it holds.
A source-text assertion that Header.svelte contains no goto. Pinning an absence is the weakest kind of guard and would fire on any legitimate navigation added later.
Extend the literal ban to /setup and /claude-session. Defensible, but this issue is scoped to the unlock route and the meta-test's blast radius grows with it.
Log every refused next. They are hostile input, not upstream bugs, and a universal load would log on both sides of the wire.
Unify stripComments with the two other copies (sealed-columns.test.ts, tabs-render.test.ts). They are deliberately different functions; sharing one changes two tests this branch does not touch.
Batch the meta-test's file reads. ~25× faster cold, ~2× slower warm. Not worth the async.
Make sanitizeNext module-local. It has no caller outside its own test, which is a fair observation, but the issue asked for it kept and its corpus is pinned against the function that implements the rule. Its docblock now points a caller at destAfterUnlock.

Not verified

That Chrome follows the pre-fix tab redirect off-origin. The header emission is measured (Location: /<TAB>/evil.com) and new URL('/\t/evil.com', base).href === 'http://evil.com/' in the same WHATWG parser family, but I did not drive the pre-fix build in a browser to watch it leave the origin. The post-fix behaviour is browser-verified.
/%75nlock typed directly (not as a next) by an already-unlocked operator still renders a stale unlock form: GATING_ROUTES.includes(url.pathname) compares the encoded pathname while the router matched the decoded one. Pre-existing, cosmetic, and the real fix is keying that test on route.id (fact #23) — a separate change to a guard AGENTS.md documents as pathname-based by design.
No Svelte component test covers Header.svelte or Unlock.svelte; the suite is node-only. Those two are covered by the browser pass above and nothing else.

Conflicts to expect

Branched from 8a7e36e like fix/unlock-rate-limit (#160) and chore/zod-v4 (#148).

#160 touches routes/api/auth/unlock/+server.ts (untouched here) and may touch Unlock.svelte. This branch changes two lines there — the import and the goto argument — both in the <script> block. A textual conflict is possible if #160 edits the same region; there is no semantic overlap.
#148 (zod repo-wide) touches nothing this branch does.
main has moved to accb51f since branching; that commit is docs-only.

Closes #150. Four sites decided where a locked operator lands and they disagreed. The client 401 bounce carried a sanitized destination; the phase gate and the Lock button carried nothing, and the bounce back off the lock screens hardcoded `/`. A cold navigation — the path OAuth consent will arrive on (#151) — therefore dropped the whole destination. ## What changed `lib/api/unlock-redirect.ts` is the only place that builds an unlock URL or reads one back: | | before | now | |---|---|---| | `lib/api/unlock-redirect.ts` | hand-built template literal | `unlockUrl(dest)` | | `routes/+layout.server.ts` phase gate | `redirect(303, required)` | `unlockUrl(url.pathname + url.search)` | | `routes/+layout.server.ts` bounce out | `redirect(303, '/')` | `destFromSearch(url.search)` | | `lib/components/Header.svelte` Lock | `goto('/unlock')` | nothing — `invalidateAll()` lets the layout decide | | `lib/components/Unlock.svelte` | `sanitizeNext(...)` | `destFromSearch(window.location.search)` | `UNLOCK_ROUTE` is the single literal and `phase-routes.ts` imports it. `destFromSearch` is the one reader of `next`, so the server (`url.search`) and the client (`window.location.search`) agree about an absent, empty, repeated or differently-cased parameter by construction rather than by luck. The `next` is read only on the screen that writes one. **The Lock button stops navigating entirely.** `invalidateAll()` already follows the layout's redirect (`_invalidate()` awaits `_goto()` on a redirect result, kit 2.59.1), so an explicit `goto` was a second decision to keep in step — and it pushed a history entry over the redirect's `replaceState`, so Back returned to the URL you were already on. `Setup.svelte` already makes the `/setup → /unlock` hop on a bare `refresh()`. **The sites-3-and-4 fight in the issue is real**, and measured rather than reasoned: with the new layout and the old `goto('/unlock')` left in place, clicking Lock on `/settings?tab=tokens` produced `/unlock?next=%2Fsettings%3Ftab%3Dtokens` and then overwrote it with a bare `/unlock`, landing on `/` after unlocking. The fix is not ordering — it is that the second decision is gone. **`NO_REDIRECT_FROM` did NOT subsume the layout's loop guard, and could not.** They answer different questions: `url.pathname !== required` asks "am I already at this phase's route" and all three gated phases need it, while the builder's rule governs what a `next` may point *at*. Drop the pathname test and `unlockUrl` on the unlock screen returns a bare `/unlock` — its own fixed point, so an infinite redirect. Both are stated where they live rather than left as silent neighbours. ## A security defect the review caught The third commit fixes an **open redirect that the first commit made reachable**. Browsers strip tab, CR and LF from a URL before parsing, so `/<tab>/evil.com` passes "starts with `/` and is not `//`" and then resolves to `//evil.com`; a tab is a legal HTTP field-value character, so it survives a `Location` header. Measured against the previous commit on a live instance: ``` GET /unlock?next=%2F%09%2Fevil.com -> 303 Location: /<TAB>/evil.com ``` On `main` this was latent: `sanitizeNext`'s only consumer was `Unlock.svelte`'s `goto`, and SvelteKit's public `goto` rejects a cross-origin URL. Teaching the bounce to honour `next` put it in a header, where nothing checks anything. The same shape walked `/./unlock`, `/x/../unlock` and `/unlock/` past the lock-screen rule, and `/%75nlock` past it in a way that parked an unlocked operator back on the passphrase form — a small regression from `main`. Fix: `sanitizeNext` refuses anything the URL parser would rewrite (refuse, never repair — the rule the module already had), and the lock-screen comparison resolves the route the way the router does, `decodeURI` included, per fact #23. This deviates from the issue's "keep `sanitizeNext` as is" — deliberately, and only by strengthening it; every existing rule is untouched. Verified live after the fix (`/unlock?next=…` while unlocked): `%2F%09%2Fevil.com` → `/`, `%2F%2575nlock` → `/`, `%2F.%2Funlock` → `/`, `%2Fsettings%3Ftab%3Dtokens` → `/settings?tab=tokens`. ## Gates 1825 tests / 157 files green, typecheck 0 errors across 4960 files, lint clean. ## Guards proven able to fail Each mutation applied, the named test confirmed red, then restored from a copy (never `git checkout`): | broken | went red | |---|---| | phase gate back to `redirect(303, required)` | carries the whole destination | | bounce back to `redirect(303, '/')` | returns an already-unlocked operator to their destination | | loop guard → `if (true)` | does not redirect a gated phase already on its route; leaves a destination parked on the unlock screen alone | | lock-screen rule removed | 13 tests across both files | | query/fragment strip removed | 7 tests | | `isCanonical` removed | 20 tests — tab/CR/LF and dot-segment cases, unit and layout | | `decodeURI` removed | 7 tests, including the property test — the proof it no longer restates the implementation | | trailing-slash trim removed | 6 tests | | `next` read widened to all gate routes | ignores a next on the gate screens that never write one | | `303` → `307` | 21 tests | | a stray `'/unlock'` added to `lib/keepalive.ts` | the meta-test's offender check | | a dormant `ALLOWED` entry re-added | the new bidirectional allowlist test | The meta-test caught two real hits I did not know about on its first run (`hooks.server.ts`, `lib/api/client.ts` — both `/api/auth/unlock`, which is why the regex has a leading lookbehind). ## Browser-verified A real instance (fake `claude-session` volume → `needs-provider`, so the whole app renders), driven with playwright-cli on the final code: cold `/settings?tab=tokens` while locked → `/unlock?next=%2Fsettings%3Ftab%3Dtokens` → unlock → back on `/settings?tab=tokens`; Lock → `/unlock?next=%2Fsettings%3Ftab%3Dtokens` → unlock → back again; both attack URLs → `/`, same origin. ## Rejected review findings - **Derive the lock-screen set from `PHASE_ROUTE`.** It would add `/claude-session`, which is a gate but not a lock screen, and stop honouring `next=/claude-session`. The sets differ on purpose. - **Carry `next` through every gated phase.** Getting the operator back needs each gate screen's *completion* to honour it; none does, so it would look like it works. This is the issue's decision 1 and it holds. - **A source-text assertion that `Header.svelte` contains no `goto`.** Pinning an absence is the weakest kind of guard and would fire on any legitimate navigation added later. - **Extend the literal ban to `/setup` and `/claude-session`.** Defensible, but this issue is scoped to the unlock route and the meta-test's blast radius grows with it. - **Log every refused `next`.** They are hostile input, not upstream bugs, and a universal load would log on both sides of the wire. - **Unify `stripComments` with the two other copies** (`sealed-columns.test.ts`, `tabs-render.test.ts`). They are deliberately different functions; sharing one changes two tests this branch does not touch. - **Batch the meta-test's file reads.** ~25× faster cold, ~2× slower warm. Not worth the async. - **Make `sanitizeNext` module-local.** It has no caller outside its own test, which is a fair observation, but the issue asked for it kept and its corpus is pinned against the function that implements the rule. Its docblock now points a caller at `destAfterUnlock`. ## Not verified - **That Chrome follows the pre-fix tab redirect off-origin.** The header emission is measured (`Location: /<TAB>/evil.com`) and `new URL('/\t/evil.com', base).href === 'http://evil.com/'` in the same WHATWG parser family, but I did not drive the pre-fix build in a browser to watch it leave the origin. The post-fix behaviour *is* browser-verified. - **`/%75nlock` typed directly** (not as a `next`) by an already-unlocked operator still renders a stale unlock form: `GATING_ROUTES.includes(url.pathname)` compares the encoded pathname while the router matched the decoded one. Pre-existing, cosmetic, and the real fix is keying that test on `route.id` (fact #23) — a separate change to a guard AGENTS.md documents as pathname-based by design. - No Svelte component test covers `Header.svelte` or `Unlock.svelte`; the suite is node-only. Those two are covered by the browser pass above and nothing else. ## Conflicts to expect Branched from `8a7e36e` like `fix/unlock-rate-limit` (#160) and `chore/zod-v4` (#148). - **#160** touches `routes/api/auth/unlock/+server.ts` (untouched here) and may touch `Unlock.svelte`. This branch changes two lines there — the import and the `goto` argument — both in the `<script>` block. A textual conflict is possible if #160 edits the same region; there is no semantic overlap. - **#148** (zod repo-wide) touches nothing this branch does. - `main` has moved to `accb51f` since branching; that commit is docs-only.

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

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

lz added 3 commits

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

Unify the unlock redirect onto one builder 0828c1af86

Four sites decided where a locked operator lands and they disagreed. The
client 401 bounce carried a sanitized destination; the server phase gate and
the Lock button carried nothing, and the bounce back off the lock screens
hardcoded '/'. So a cold navigation — the one path OAuth consent will arrive
on — dropped the whole destination: /settings?tab=tokens 303'd to a bare
/unlock and landed on / after unlocking.

unlockUrl() is now the only way to build that URL and destAfterUnlock() the
only way to read it back. Both refuse a lock screen as a destination, which
is what makes next=/unlock unrepresentable rather than merely unlikely, and
both strip the query before that comparison because /unlock?foo is still the
unlock screen. UNLOCK_ROUTE is the single literal; phase-routes.ts imports it.

The Lock button builds its URL BEFORE invalidateAll(). That ordering is the
whole fix for the interaction the two sites now have: invalidateAll re-runs
the layout, and _invalidate() awaits _goto() when a load returns a redirect,
so by the time the explicit goto runs the browser is already at
/unlock?next=… and window.location would hand back a bare one. Measured in a
browser — with the old goto('/unlock') left in place the destination is
overwritten and unlocking lands on /.

The layout keeps its `url.pathname !== required` test. It is the loop guard
and it is generic: /setup and /claude-session need it too, and the builder's
lock-screen rule answers a different question — what a next may point at, not
whether to make the hop. Dropping it and letting the builder decide returns a
bare /unlock while already on /unlock, which is the loop.

next rides the unlock hop only. Threading it through the other gated phases
would look like it works and would not: both hand off to another phase that
redirects from scratch.

Let the Lock button stop deciding, and close the dormant exemption 913d8ed9c6

Four cleanup passes over the first commit. Three findings were worth taking.

The Lock button's explicit goto is gone. invalidateAll() already follows the
layout's redirect — _invalidate() awaits _goto() when a re-run load returns
one — so the handler was making a second decision that had to be kept in step
with the first, which is the shape of the bug this branch exists to fix. It
also pushed a history entry on top of the redirect's replaceState, so Back
returned to the URL you were already on. Setup.svelte already makes the
/setup -> /unlock hop on a bare refresh() and nothing else. Verified in a
browser: Lock from /settings?tab=tokens still lands on
/unlock?next=%2Fsettings%3Ftab%3Dtokens and unlocking returns there.

The meta-test's allowlist named lib/phase-routes.ts, which after the import
change holds no literal at all — an exemption nothing used, silently keeping
a hole open for the day that file hand-writes the route again. It is now one
entry, and a second test reads the list the other way: an entry whose file
does not name the route fails. That is the rule hooks.server.test.ts already
applies to AGENT_CALLBACKS, and this branch had shipped the exact stale entry
it exists to catch. Proven by re-adding the entry and watching it go red.

The loop test iterated 18 inputs and asserted on one: every refused input
returns a bare /unlock, so the guard continued before reaching an assertion.
It now states the fixed point over destinations that really are carried, and
the corpus is composed from its two halves rather than written out twice.

Rejected: deriving the lock-screen set from PHASE_ROUTE (it would add
/claude-session, which is a gate but not a lock screen, and stop honouring
next=/claude-session); carrying next through every gated phase (getting the
operator back needs each gate screen's completion to honour it, and none
does); batching the meta-test's file reads (2x worse warm, and the walk is
already prefiltered on the raw text); and unifying stripComments with the two
other copies, which are deliberately different functions in tests this branch
does not touch.

Refuse a next the URL parser would rewrite

ci / images (pull_request) Has been cancelled

Details

ci / nexus (pull_request) Has been cancelled

Details

pr-image-cleanup / delete-pr-images (pull_request) Successful in 7s

Details

977e9dea82

The review pass found an open redirect that the previous commit made
reachable, and it is the same class of mistake as the bug this branch fixes:
a check that agrees with itself rather than with the thing it models.

Browsers STRIP tab, CR and LF from a URL before parsing it, so
`/<tab>/evil.com` passes "starts with / and is not //" and then resolves to
`//evil.com`. A tab is a legal HTTP field-value character, so it survives a
Location header where a CR makes Node's Headers throw. Measured end to end
against the previous commit on a live instance:

  GET /unlock?next=%2F%09%2Fevil.com   ->   303  Location: /<TAB>/evil.com

On main this was latent — sanitizeNext's only consumer was Unlock.svelte's
goto, and SvelteKit's public goto rejects a cross-origin URL. Teaching the
layout bounce to honour `next` put it in a Location header, where nothing
checks anything. The same shape let `/./unlock`, `/x/../unlock` and `/unlock/`
past the lock-screen rule, and `/%75nlock` past it in a way that parked an
unlocked operator back on the passphrase form — a small regression from main,
which sent them to `/`.

Both holes came from comparing strings the browser has not finished editing
yet. So sanitizeNext now refuses anything the URL parser would rewrite
(refuse, never repair — the rule this module already had), and the
lock-screen comparison resolves the route the way the router does, decodeURI
included, per fact #23. The property test no longer restates the
implementation's own comparison: it asks where a browser lands, which is why
it goes red on the decode.

Also from the review, each verified before acting:

- `destFromSearch` is now the single reader of `next`, used by the layout and
  by Unlock.svelte. The two used to parse the query separately and agreed by
  luck about an empty, repeated or differently-cased parameter; that is now
  structural and tested.
- The layout reads `next` only on the unlock screen, which is the only screen
  that writes one. Honouring one on /setup or /claude-session was honouring a
  hand-crafted parameter, and it made a comment on the line above false.
- The Lock handler no longer answers a failed invalidateAll() with "Lock
  failed". Past api.lock() the session is gone whatever happens next, so
  saying it failed is a lie told immediately after "Locked".
- Five comments of mine claimed things the code does not do: that the
  post-unlock path reloads (it is a fetch — Unlock.svelte said the opposite
  four lines away), that folding the two reducers would change the 401 bounce
  (it never calls sanitizeNext), that `https://host/unlock` would be a missed
  hit (ROUTE cannot match it), that the layout is "the one place" that decides
  (the 401 bounce also does), and a file count that was wrong. Fixed rather
  than kept, along with the prefilter whose justification was false: deleting
  characters CAN splice a match into existence.

Rejected: an eslint rule or a wider literal ban covering /setup and
/claude-session (this issue is scoped to the unlock route, and the meta-test's
blast radius grows with it); a source-text assertion that Header.svelte
contains no goto (pinning an absence is the weakest kind of guard and would
fire on any legitimate navigation added later); logging every refused `next`
(they are hostile input, not upstream bugs, and a universal load logs on both
sides of the wire); and switching the layout's pathname test to route.id,
which would close the last encoded-path wart but changes a gate AGENTS.md
documents as pathname-based by design — worth its own issue, not this one.

Gates: 1825 tests over 157 files, typecheck 0 errors across 4960 files, lint
clean. Every new guard was proven able to fail by breaking what it protects.

lz merged commit 3446ca33e7 into main

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

lz deleted branch fix/unlock-redirect

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

lz referenced this pull request from a commit

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

Merge pull request 'Unify the unlock redirect onto one builder' (#165) from fix/unlock-redirect into main