Public: anyone can read this; the crew can change it.

/login and /sign-in answer 308 to /signin, keeping a same-origin next #7728

Merged opened this

Check · Review · Land · Live done — Landed and live in production.

Merged

`GET /login` and `GET /sign-in` now answer 308 to `/signin`, so the addresses people type to sign in work.

shipped production is serving it deploy a67717b

opened this

GET /login and GET /sign-in now answer 308 to /signin, so the addresses people type to sign in work.

  • next handling: next is carried over only when safeNext from src/web/pages/site.ts accepts it. That is the same check /signin already uses, so there is no second validator. ?next=/settings becomes /signin?next=%2Fsettings. //evil.test, https://evil.test and /\evil.test are dropped and the redirect goes to plain /signin, which closes the open-redirect risk.
  • Where it lives: one small signinAlias handler, registered next to /signin in src/http/server.ts.
  • Server-Timing sweep: both paths are in REDIRECTS in test/server-timing-sweep.test.ts, kept in alphabetical order. Nothing was added to UNNAMED.
  • New test: test/signin-alias.test.ts checks, for each alias, the plain 308, that next=/settings survives, and that the three off-site values are dropped. It also checks that /signup is not redirected.

Not done: /signup is untouched and no other alias was added. No scorecard, proof or looks files were changed.

Checks run here: bun x tsc --noEmit passes, and bun test test/server-timing-sweep.test.ts test/signin-alias.test.ts passes (9 tests). scripts/fast.ts named 1059 test files, well over its 5-minute budget, so I chose the ones that touch routes or sign-in myself: guest-crawl, hollow, system-telemetry, signin-page, forge-signin and signin-edge. All 74 tests in them pass. The full suite has not been run here; the forge's check runs it on this push.

To try it on the preview, open /login?next=/settings with redirects off: it should answer 308 with Location: /signin?next=%2Fsettings. /login?next=//evil.test should go to /signin with no next.

🤖 Generated with Claude Code

Closes #7721

commented

GET /login and GET /sign-in now answer 308 to /signin, so the addresses people type to sign in work.

  • next handling: next is carried over only when safeNext from src/web/pages/site.ts accepts it. That is the same check /signin already uses, so there is no second validator. ?next=/settings becomes /signin?next=%2Fsettings. //evil.test, https://evil.test and /\evil.test are dropped and the redirect goes to plain /signin, which closes the open-redirect risk.
  • Where it lives: one small signinAlias handler, registered next to /signin in src/http/server.ts.
  • Server-Timing sweep: both paths are in REDIRECTS in test/server-timing-sweep.test.ts, kept in alphabetical order. Nothing was added to UNNAMED.
  • New test: test/signin-alias.test.ts checks, for each alias, the plain 308, that next=/settings survives, and that the three off-site values are dropped. It also checks that /signup is not redirected.

Not done: /signup is untouched and no other alias was added. No scorecard, proof or looks files were changed.

Checks run here: bun x tsc --noEmit passes, and bun test test/server-timing-sweep.test.ts test/signin-alias.test.ts passes (9 tests). scripts/fast.ts named 1059 test files, well over its 5-minute budget, so I chose the ones that touch routes or sign-in myself: guest-crawl, hollow, system-telemetry, signin-page, forge-signin and signin-edge. All 74 tests in them pass. The full suite has not been run here; the forge's check runs it on this push.

To try it on the preview, open /login?next=/settings with redirects off: it should answer 308 with Location: /signin?next=%2Fsettings. /login?next=//evil.test should go to /signin with no next.

🤖 Generated with Claude Code

commented

GET /login and GET /sign-in now answer 308 to /signin, so the addresses people type to sign in work.

  • next handling: next is carried over only when safeNext from src/web/pages/site.ts accepts it. That is the same check /signin already uses, so there is no second validator. ?next=/settings becomes /signin?next=%2Fsettings. //evil.test, https://evil.test and /\evil.test are dropped and the redirect goes to plain /signin, which closes the open-redirect risk.
  • Where it lives: one small signinAlias handler, registered next to /signin in src/http/server.ts.
  • Server-Timing sweep: both paths are in REDIRECTS in test/server-timing-sweep.test.ts. Nothing was added to UNNAMED.
  • Design audit: docs/design/audit.md lists every static GET route, so it now has entries for /login and /sign-in, written the way the /big redirect entry is. Its page count goes from 64 to 66. The first push failed test/design-audit.test.ts because these entries were missing; the test was right and is unchanged.
  • New test: test/signin-alias.test.ts checks, for each alias, the plain 308, that next=/settings survives, and that the three off-site values are dropped. It also checks that /signup is not redirected.

Not done: /signup is untouched and no other alias was added. No scorecard, proof or looks files were changed.

Checks run here: bun x tsc --noEmit passes, and bun test test/design-audit.test.ts test/server-timing-sweep.test.ts test/signin-alias.test.ts passes (13 tests). The route and sign-in suites I ran before this fix also passed: guest-crawl, hollow, system-telemetry, signin-page, forge-signin and signin-edge. The full suite has not been run here; the forge's check runs it on this push.

To try it on the preview, open /login?next=/settings with redirects off: it should answer 308 with Location: /signin?next=%2Fsettings. /login?next=//evil.test should go to /signin with no next.

🤖 Generated with Claude Code

Approved — before the latest push

Clean. /login and /sign-in answer 308 to /signin on the preview. next is validated by the existing safeNext (rejects //, backslash, CR/LF, absolute URLs) and re-encoded via URLSearchParams, so no open redirect or header injection. GET-only, no identity or body involved. Tests cover each alias, next kept, off-site next dropped, and /signup untouched. The audit doc and Server-Timing sweep are updated. No new dependency.

commented

/login and /sign-in now answer 308 to /signin. They keep next only when it points to this same origin, so a typed or bookmarked "login" URL lands on the real sign-in form without becoming an open redirect.

  • Change: src/http/server.ts; tests in test/signin-alias.test.ts. Both aliases are added to test/server-timing-sweep.test.ts, and docs/design/audit.md lists them, making 66 top-level pages.
  • Check fix: the first push failed one test, "app.looks is the router's top-level pages minus the not-a-page list". The test was right: every new top-level route must be either pictured or listed with a reason. The two aliases are now in its NOT_A_PAGE list in test/forge-looks.test.ts as "redirects to /signin", like /asks and /flow. No app code changed in this fix.
  • Not done: the aliases are not added to app.looks, since a redirect has nothing to picture.

Tests: bun x tsc --noEmit is clean. I ran seven test files: forge-looks, signin-alias, server-timing-sweep, route-timing-coverage, security-floor-routes, design-audit and looks. 39 tests pass. Two fail in test/looks.test.ts: they capture pages with a real Chromium, which fails in the sandbox here. That file passed in the forge's own check and this pull doesn't touch it.

Try it: on the preview, curl -sI 'https://lab--7728--forge-trevato-apps.open-platform.sh/login?next=/map' should answer 308 with location: /signin?next=%2Fmap. With next=https://evil.example the location is a bare /signin.

🤖 Generated with Claude Code

Approved

Clean. Aliases reuse safeNext (no second validator), off-site next values are dropped, tests cover 308, kept next and dropped next, and the looks/audit/timing lists are updated (the earlier check failures are fixed). The preview answers 308 as described. No earlier asks outstanding.

Sign in to join this conversation.

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs