§9 whole-project arch+product review pass (todo §9); ran systems-architect + product-owner on the whole project (no Critical/High — a converged scaffold) and addressed the in-scope §9 customer-facing/security findings. (1) return_to deep-link login, open-redirect-safe: a gated request hit while signed out (plugin-route gate, requireSession, requireAdmin) bounces to /login?return_to=<host-relative path> via new loginRedirect(ctx) (GET/HEAD, skips /); /login bakes it into the Kratos flow — a host-relative target is wrapped through <origin>/auth/complete?return_to=<path> so the JWT mints before landing, an absolute target (§6 OAuth2 login challenge) passes to Kratos as-is; /auth/complete redirects to the requested page. (2) safeUrl()+localPath() in new pure src/safe-url.ts: safeUrl sanitises an untrusted href/src to relative-or-http(s) (else "#"), exported via plugin-api.ts (closes the contract's "planned for §9" pointer); localPath is the host-relative redirect-allowlist guard for return_to, re-checked at both /login and /auth/complete. (3) honest 503 on Ory-unreachable sign-in (views/503.ejs) instead of the misattributed catch-all 500; expired-flow 4xx still restarts. Tests-first throughout; stability-reviewer APPROVE (addressed its Medium — scoped the 503 catch so a template bug hits the 500 with a stack, not a 503). typecheck + 339 units + full scripts/ci.sh gate green. Deferred with justification: the app.ts route-table refactor (standalone change + §10 prereq), mock dashboard + public-page blessing (§10 lines 139/140), success-flash (known).

2026-06-20 16:38:57 +02:00
parent 1118d7a9f7
commit 66ea68a91b
16 changed files with 277 additions and 48 deletions
--- a/src/admin-nav.test.ts
+++ b/src/admin-nav.test.ts
@@ -58,7 +58,7 @@ test("adminNav: prepends Dashboard and role-filters the section (admin sees it,
 // ---- auth gates ----

 test("requireAdmin: anonymous → 401→/login, signed-in non-admin → 403, admin → the user", () => {
-  assert.throws(() => requireAdmin(reqCtx({ user: null })), (e: unknown) => e instanceof GuardError && e.status === 401 && e.location === "/login");
+  assert.throws(() => requireAdmin(reqCtx({ user: null })), (e: unknown) => e instanceof GuardError && e.status === 401 && e.location === "/login?return_to=%2Fadmin%2Fusers"); // anonymous bounce remembers the page
  assert.throws(() => requireAdmin(reqCtx({ user: member })), (e: unknown) => e instanceof GuardError && e.status === 403);
  assert.equal(requireAdmin(reqCtx({ user: admin })), admin);
 });
--- a/src/admin-nav.ts
+++ b/src/admin-nav.ts
@@ -7,7 +7,7 @@
 import { readFormBody } from "./body.ts";
 import type { RequestContext, User } from "./context.ts";
 import { CSRF_FIELD, verifyCsrfRequest } from "./csrf.ts";
-import { GuardError } from "./guards.ts";
+import { GuardError, loginRedirect } from "./guards.ts";
 import { type MenuConfig } from "./menu-config.ts";
 import { composeNav, type NavNode } from "./nav.ts";
 import { buildShellContext } from "./shell-context.ts";
@@ -51,7 +51,7 @@ export function adminNav(roles: string[], menu: MenuConfig, current: AdminScreen
 // The shared gate for every admin screen: a signed-in admin only. Throws GuardError that app.ts maps
 // (anonymous → /login, non-admin → 403). Returns the (non-null) user for the handler to thread on.
 export function requireAdmin(ctx: RequestContext): User {
-  if (!ctx.user) throw new GuardError(401, "authentication required", "/login");
+  if (!ctx.user) throw new GuardError(401, "authentication required", loginRedirect(ctx));
  if (!ctx.roles.includes(ADMIN_PERMISSION)) throw new GuardError(403, "admin role required");
  return ctx.user;
 }
--- a/src/app.test.ts
+++ b/src/app.test.ts
@@ -311,10 +311,11 @@ test("mounts plugin routes: params, html/json/redirect/view results, and the per
  assert.match(await css.text(), /\.demo/);
  assert.equal((await fetch(url + "/public/demo/..%2f..%2fplugin.ts")).status, 403); // traversal still blocked

-  // gated route, anonymous → redirect to sign in (like the built-in screens), not a dead-end 403
+  // gated route, anonymous → redirect to sign in (like the built-in screens), not a dead-end 403;
+  // the requested page is preserved as return_to so login lands the user back there.
  const denied = await fetch(url + "/demo/secret", { redirect: "manual" });
  assert.equal(denied.status, 303);
-  assert.equal(denied.headers.get("location"), "/login");
+  assert.equal(denied.headers.get("location"), "/login?return_to=%2Fdemo%2Fsecret");

  // known path + wrong method → 405 with Allow; unknown path → 404
  const wrong = await fetch(url + "/demo/data", { method: "DELETE" });
@@ -398,10 +399,11 @@ test("a verified session JWT authorizes a role-gated route; no cookie / expired
  assert.equal(ok.status, 200);
  assert.equal(await ok.text(), "secret");

-  // No cookie and an expired token both render anonymous → the gate bounces to sign in (303 → /login).
+  // No cookie and an expired token both render anonymous → the gate bounces to sign in (303 → /login,
+  // remembering the gated page as return_to).
  const noCookie = await secret();
  assert.equal(noCookie.status, 303);
-  assert.equal(noCookie.headers.get("location"), "/login");
+  assert.equal(noCookie.headers.get("location"), "/login?return_to=%2Fdemo%2Fsecret");
  assert.equal((await secret(`${SESSION_COOKIE}=${mintJwt({ email: "a@b.c", exp: nowSec - 600, roles: ["demo:read"], sub: "u1" })}`)).status, 303);

  // The home menu wires in the permission-gated Admin section: an admin's roles surface the links.
@@ -449,7 +451,7 @@ test("session re-mint: an expired JWT backed by a live Kratos session is silentl
  t.after(() => dead.close());
  const denied = await fetch(`http://localhost:${(dead.address() as AddressInfo).port}/demo/secret`, { headers: { cookie: expired }, redirect: "manual" });
  assert.equal(denied.status, 303);
-  assert.equal(denied.headers.get("location"), "/login");
+  assert.equal(denied.headers.get("location"), "/login?return_to=%2Fdemo%2Fsecret");
  assert.match(denied.headers.get("set-cookie") ?? "", /^plainpages_jwt=;.*Max-Age=0/);

  // Ory unreachable (not a dead session): whoami throws → degrade to anonymous (bounce to /login, not 500),
@@ -459,7 +461,7 @@ test("session re-mint: an expired JWT backed by a live Kratos session is silentl
  t.after(() => down.close());
  const outage = await fetch(`http://localhost:${(down.address() as AddressInfo).port}/demo/secret`, { headers: { cookie: expired }, redirect: "manual" });
  assert.equal(outage.status, 303);
-  assert.equal(outage.headers.get("location"), "/login");
+  assert.equal(outage.headers.get("location"), "/login?return_to=%2Fdemo%2Fsecret");
  assert.equal(outage.headers.get("set-cookie"), null);
 });

@@ -482,10 +484,10 @@ test("guards map to responses: requireSession → /login, a failed can/check →
  const nowSec = Math.floor(Date.now() / 1000);
  const auth = (roles: string[]) => ({ headers: { cookie: `${SESSION_COOKIE}=${mintJwt({ email: "a@b.c", exp: nowSec + 600, roles, sub: "u1" })}` } });

-  // requireSession: anonymous bounces to /login; a signed-in user reaches the handler.
+  // requireSession: anonymous bounces to /login (remembering the page); a signed-in user reaches the handler.
  const anon = await fetch(url + "/guarded/me", { redirect: "manual" });
  assert.equal(anon.status, 303);
-  assert.equal(anon.headers.get("location"), "/login");
+  assert.equal(anon.headers.get("location"), "/login?return_to=%2Fguarded%2Fme");
  const me = await fetch(url + "/guarded/me", auth([]));
  assert.equal(me.status, 200);
  assert.match(await me.text(), /hi a@b\.c/);
@@ -501,7 +503,7 @@ test("guards map to responses: requireSession → /login, a failed can/check →
  // declarative route `permission` gate: anonymous → sign in, signed-in-without-role → the 403 page, with → 200.
  const gAnon = await fetch(url + "/guarded/gated", { redirect: "manual" });
  assert.equal(gAnon.status, 303);
-  assert.equal(gAnon.headers.get("location"), "/login");
+  assert.equal(gAnon.headers.get("location"), "/login?return_to=%2Fguarded%2Fgated");
  const gDenied = await fetch(url + "/guarded/gated", auth([]));
  assert.equal(gDenied.status, 403);
  assert.match(await gDenied.text(), /403/); // the rendered 403.ejs over HTTP
@@ -588,6 +590,57 @@ test("themed auth GET: anonymous inits a flow (CSRF relay, stale→restart); a s
  assert.equal((await fetch(url + "/settings", signedIn)).headers.get("location"), "/settings?flow=new1");
 });

+// return_to (§9): a deep-link login lands back on the requested page. The gate redirects to
+// /login?return_to=<host-relative path>; /login bakes that into the Kratos flow so completion
+// returns there — but a first-party path must route via /auth/complete first (to mint the JWT).
+test("login return_to: a first-party deep link is wrapped through /auth/complete; an absolute target passes through as-is", async (t) => {
+  let lastReturnTo: string | undefined;
+  const kratos: KratosPublic = {
+    ...mockKratos(async (_t, id) => loginFlow(id)),
+    initBrowserFlow: async (_t: FlowType, opts = {}) => { lastReturnTo = opts.returnTo; return { flow: { id: "new1", ui: { action: "", method: "post", nodes: [] } }, setCookie: [] }; },
+  };
+  const app = createApp({ kratos });
+  await new Promise<void>((r) => app.listen(0, r));
+  t.after(() => app.close());
+  const url = `http://localhost:${(app.address() as AddressInfo).port}`;
+
+  // A host-relative deep link → wrapped: Kratos returns to <origin>/auth/complete?return_to=<path>,
+  // so the JWT is minted before the user lands on the page (query preserved, re-encoded).
+  await fetch(url + "/login?return_to=" + encodeURIComponent("/admin/users?q=1"), { redirect: "manual" });
+  assert.match(lastReturnTo ?? "", /^http:\/\/[^/]+\/auth\/complete\?return_to=%2Fadmin%2Fusers%3Fq%3D1$/);
+
+  // An absolute target (the §6 OAuth2 login challenge) is passed to Kratos unchanged — Kratos
+  // allow-lists it. A protocol-relative "//evil.com" is likewise not wrapped (Kratos rejects it).
+  const abs = "http://localhost/oauth2/login?login_challenge=abc";
+  await fetch(url + "/login?return_to=" + encodeURIComponent(abs), { redirect: "manual" });
+  assert.equal(lastReturnTo, abs);
+  await fetch(url + "/login?return_to=" + encodeURIComponent("//evil.com"), { redirect: "manual" });
+  assert.equal(lastReturnTo, "//evil.com");
+});
+
+// "Ory down ⇒ no logins" is documented; the auth path should say so honestly (503), not the
+// generic "error on our end" 500 the catch-all renders.
+test("auth flow when Ory is unreachable → an honest 503, not the catch-all 500", async (t) => {
+  const boom = () => { throw new KratosError("kratos down", 503, ""); };
+  const down: KratosPublic = { ...mockKratos(async () => boom()), initBrowserFlow: async () => boom() };
+  const app = createApp({ kratos: down });
+  await new Promise<void>((r) => app.listen(0, r));
+  t.after(() => app.close());
+  const url = `http://localhost:${(app.address() as AddressInfo).port}`;
+
+  const init = await fetch(url + "/login", { redirect: "manual" }); // init (no ?flow=) with Kratos down
+  assert.equal(init.status, 503);
+  assert.match(await init.text(), /unavailable/i);
+  assert.equal((await fetch(url + "/login?flow=f1")).status, 503); // fetching a flow, Kratos down
+
+  // A network-level throw (refused/timeout — not a KratosError) is treated the same way.
+  const refused: KratosPublic = { ...mockKratos(async () => { throw new Error("ECONNREFUSED"); }), initBrowserFlow: async () => { throw new Error("ECONNREFUSED"); } };
+  const app2 = createApp({ kratos: refused });
+  await new Promise<void>((r) => app2.listen(0, r));
+  t.after(() => app2.close());
+  assert.equal((await fetch(`http://localhost:${(app2.address() as AddressInfo).port}/login`, { redirect: "manual" })).status, 503);
+});
+
 test("renders a fetched flow as the themed auth page: fields post straight to Kratos, errors surface", async (t) => {
  const app = createApp({ kratos: mockKratos(async (_t, id) => loginFlow(id)) });
  await new Promise<void>((r) => app.listen(0, r));
@@ -660,7 +713,7 @@ async function adminHarness(t: TestContext, opts: AppOptions = {}) {
 async function assertAdminGate(url: string, get: (path: string, roles?: string[]) => Promise<Response>, path: string) {
  const anon = await fetch(url + path, { redirect: "manual" });
  assert.equal(anon.status, 303);
-  assert.equal(anon.headers.get("location"), "/login");
+  assert.equal(anon.headers.get("location"), `/login?return_to=${encodeURIComponent(path)}`); // remembers the page
  assert.equal((await get(path, [])).status, 403);
 }

@@ -670,10 +723,11 @@ test("login completion (/auth/complete): a live session mints the JWT cookie; no
  const kratos = withWhoami(async (o) => (o?.tokenizeAs ? { active: true, identity, tokenized: "h.p.s" } : { active: true, identity }) as Session);
  const kratosAdmin = stubAdmin({ updateMetadataPublic: async (_id, meta) => { projected = meta; return identity; } });
  const keto = fakeKeto([], { check: async () => true, listRelations: async () => ({ nextPageToken: null, tuples: [{ namespace: "Role", object: "admin", relation: "members", subject_id: `user:${identity.id}` }] }) });
-  const complete = async (app: ReturnType<typeof createApp>, cookie?: string) => {
+  const complete = async (app: ReturnType<typeof createApp>, cookie?: string, returnTo?: string) => {
    await new Promise<void>((r) => app.listen(0, r));
    t.after(() => app.close());
-    return fetch(`http://localhost:${(app.address() as AddressInfo).port}/auth/complete`, { headers: cookie ? { cookie } : {}, redirect: "manual" });
+    const q = returnTo ? `?return_to=${encodeURIComponent(returnTo)}` : "";
+    return fetch(`http://localhost:${(app.address() as AddressInfo).port}/auth/complete${q}`, { headers: cookie ? { cookie } : {}, redirect: "manual" });
  };

  // Live Kratos session: roles from Keto → projection → tokenize → JWT cookie, land on /.
@@ -683,6 +737,11 @@ test("login completion (/auth/complete): a live session mints the JWT cookie; no
  assert.match(ok.headers.get("set-cookie") ?? "", /^plainpages_jwt=h\.p\.s;.*HttpOnly/);
  assert.deepEqual(projected, { roles: ["admin"] }); // Keto roles projected onto the identity for the tokenizer

+  // return_to (§9): a safe host-relative target lands the user back where they were headed; an
+  // off-origin one is ignored (open-redirect guard) and falls back to /.
+  assert.equal((await complete(createApp({ keto, kratos, kratosAdmin }), "plainpages_session=s", "/admin/users?q=1")).headers.get("location"), "/admin/users?q=1");
+  assert.equal((await complete(createApp({ keto, kratos, kratosAdmin }), "plainpages_session=s", "//evil.com")).headers.get("location"), "/");
+
  // No Kratos session: nothing minted, bounce to /login with no cookie.
  const none = await complete(createApp({ keto: fakeKeto(), kratos: withWhoami(async () => null), kratosAdmin: stubAdmin({}) }));
  assert.equal(none.status, 303);
--- a/src/app.ts
+++ b/src/app.ts
@@ -15,7 +15,7 @@ import { CSRF_FIELD, csrfCookie, ensureCsrfToken, verifyCsrfRequest } from "./cs
 import type { Denylist } from "./denylist.ts";
 import { buildDashboardModel } from "./dashboard.ts";
 import { PLUGINS_DIR } from "./discovery.ts";
-import { GuardError } from "./guards.ts";
+import { GuardError, loginRedirect } from "./guards.ts";
 import { AUTH_FLOWS, buildFlowView } from "./flow-view.ts";
 import { runRequestHooks, runResponseHooks } from "./hooks.ts";
 import { HydraError, type HydraAdmin } from "./hydra-admin.ts";
@@ -23,7 +23,7 @@ import type { JwksProvider } from "./jwks.ts";
 import { resolveSession, type VerifyOptions } from "./jwt-middleware.ts";
 import type { KetoClient } from "./keto-client.ts";
 import type { KratosAdmin } from "./kratos-admin.ts";
-import { KratosError, type KratosPublic } from "./kratos-public.ts";
+import { type Flow, KratosError, type KratosPublic } from "./kratos-public.ts";
 import { createLogger, type Log, requestLogger, runWithLog } from "./logger.ts";
 import { clearSessionCookie, completeLogin, remintSession, sessionCookie } from "./login.ts";
 import { resolveLoginChallenge } from "./oauth-login.ts";
@@ -32,6 +32,7 @@ import { DEFAULT_MENU, type MenuConfig } from "./menu-config.ts";
 import type { Plugin, RouteResult } from "./plugin.ts";
 import { allowedMethods, isAuthorized, matchRoute } from "./router.ts";
 import { securityHeaders } from "./security-headers.ts";
+import { localPath } from "./safe-url.ts";
 import { routePublic, serveStatic } from "./static.ts";
 import { renderPluginView } from "./view-resolver.ts";

@@ -188,9 +189,9 @@ export function createApp(options: AppOptions = {}): Server {
      if (match) {
        const routeCtx = buildContext(req, res, { chrome: chrome(), log: reqLog, params: match.params, user, verifyCsrf });
        if (!isAuthorized(match.route, routeCtx.roles)) {
-          // Anonymous → sign in (like the built-in screens' requireSession); a signed-in user who
-          // simply lacks the role gets the 403 page.
-          if (!routeCtx.user) { res.writeHead(303, { location: "/login" }).end(); return; }
+          // Anonymous → sign in (like the built-in screens' requireSession), remembering the page as
+          // return_to; a signed-in user who simply lacks the role gets the 403 page.
+          if (!routeCtx.user) { res.writeHead(303, { location: loginRedirect(routeCtx) }).end(); return; }
          reqLog.warn("forbidden: missing role", { path: pathname, required: match.route.permission ?? "", sub: routeCtx.user.id });
          sendHtml(res, 403, await render("403", { title: "Forbidden" }));
          return;
@@ -249,25 +250,48 @@ export function createApp(options: AppOptions = {}): Server {
        }
        const cookie = req.headers.cookie;
        const flowId = ctx.url.searchParams.get("flow");
-        if (!flowId) {
-          // No flow yet: init one server-side, relay Kratos' CSRF cookie, bounce to ?flow=<id>.
-          // A `return_to` (e.g. the OAuth2 login challenge bouncing here, §6) is baked into the
-          // flow so Kratos lands back there after login instead of the default completion route.
-          const returnTo = ctx.url.searchParams.get("return_to") ?? undefined;
-          const { flow, setCookie } = await kratos.initBrowserFlow(flowType, { ...(cookie ? { cookie } : {}), ...(returnTo ? { returnTo } : {}) });
-          if (setCookie.length) res.appendHeader("set-cookie", setCookie);
-          res.writeHead(303, { location: `${pathname}?flow=${flow.id}` }).end();
-          return;
-        }
+        // Only the Kratos calls are in the try, so a render/buildFlowView bug below falls through to
+        // the catch-all 500 (with a stack), not the "Ory unreachable" 503.
+        let flow: Flow;
        try {
-          const flow = await kratos.getFlow(flowType, flowId, cookie ? { cookie } : {});
-          sendHtml(res, 200, await render("auth", { brand: menu.branding.name, flow: buildFlowView(flow, flowType) }));
+          if (!flowId) {
+            // No flow yet: init one server-side, relay Kratos' CSRF cookie, bounce to ?flow=<id>.
+            // A `return_to` is baked into the flow so Kratos lands there after login instead of the
+            // default completion route. A first-party deep link (host-relative, from the gate's
+            // return_to) is wrapped through /auth/complete so the session JWT is minted before the
+            // user reaches the page; an absolute target (the §6 OAuth2 login challenge) is passed
+            // as-is — Kratos allow-lists it. localPath rejects an off-origin "//evil.com".
+            const raw = ctx.url.searchParams.get("return_to");
+            const local = localPath(raw);
+            let returnTo: string | undefined;
+            if (local) {
+              const origin = `${secureCookies ? "https" : "http"}://${req.headers.host ?? "127.0.0.1:3000"}`;
+              const complete = new URL(`${origin}/auth/complete`);
+              complete.searchParams.set("return_to", local);
+              returnTo = complete.toString();
+            } else if (raw) returnTo = raw;
+            const { flow: initiated, setCookie } = await kratos.initBrowserFlow(flowType, { ...(cookie ? { cookie } : {}), ...(returnTo ? { returnTo } : {}) });
+            if (setCookie.length) res.appendHeader("set-cookie", setCookie);
+            res.writeHead(303, { location: `${pathname}?flow=${initiated.id}` }).end();
+            return;
+          }
+          flow = await kratos.getFlow(flowType, flowId, cookie ? { cookie } : {});
        } catch (err) {
          // Expired/unknown flow → restart by re-initialising (drop the stale ?flow=).
          if (err instanceof KratosError && [403, 404, 410].includes(err.status)) {
            res.writeHead(303, { location: pathname }).end();
-          } else throw err;
+            return;
+          }
+          // Ory unreachable (Kratos 5xx / connection refused / timeout): "Ory down ⇒ no logins" is
+          // documented, so render an honest 503 rather than the catch-all "error on our end" 500.
+          if (!(err instanceof KratosError) || err.status >= 500) {
+            reqLog.warn("auth flow failed (Ory unreachable?)", { error: String(err), path: pathname });
+            sendHtml(res, 503, await render("503", { title: "Sign-in unavailable" }));
+            return;
+          }
+          throw err; // any other Kratos 4xx → the catch-all (genuinely unexpected)
        }
+        sendHtml(res, 200, await render("auth", { brand: menu.branding.name, flow: buildFlowView(flow, flowType) }));
        return;
      }

@@ -381,7 +405,9 @@ export function createApp(options: AppOptions = {}): Server {
          return;
        }
        res.appendHeader("set-cookie", sessionCookie(completed.jwt, { secure: secureCookies }));
-        res.writeHead(303, { location: "/" }).end();
+        // Land on the deep link the user was headed to (return_to, validated host-relative so a
+        // crafted ?return_to= can't make this an open redirect), else home (§9).
+        res.writeHead(303, { location: localPath(ctx.url.searchParams.get("return_to")) ?? "/" }).end();
        return;
      }

--- a/src/guards.test.ts
+++ b/src/guards.test.ts
@@ -6,23 +6,28 @@ import { buildContext, type RequestContext, type User } from "./context.ts";
 import { can, check, GuardError, requireSession } from "./guards.ts";
 import type { KetoClient, RelationTuple } from "./keto-client.ts";

-function ctxFor(user: User | null): RequestContext {
+function ctxFor(user: User | null, url = "/"): RequestContext {
  const req = new IncomingMessage(new Socket());
-  req.url = "/";
+  req.url = url;
  return buildContext(req, new ServerResponse(req), { user });
 }

 const alice: User = { email: "a@b.c", id: "u1", roles: ["admin", "scheduling:read"] };

-test("requireSession returns the user, or throws GuardError(401)→/login when anonymous", () => {
+test("requireSession returns the user, or throws GuardError(401)→/login (preserving return_to) when anonymous", () => {
  assert.equal(requireSession(ctxFor(alice)), alice);

+  // On the home path there is nothing worth returning to → a bare /login.
  assert.throws(() => requireSession(ctxFor(null)), (err: unknown) => {
    assert.ok(err instanceof GuardError);
    assert.equal(err.status, 401);
    assert.equal(err.location, "/login"); // app.ts turns this into a 303 to sign in
    return true;
  });
+
+  // A deep link is remembered so login returns the user there (host-relative, encoded).
+  assert.throws(() => requireSession(ctxFor(null, "/scheduling/shifts?q=1")), (err: unknown) =>
+    err instanceof GuardError && err.location === "/login?return_to=%2Fscheduling%2Fshifts%3Fq%3D1");
 });

 test("can reads a coarse role from the JWT claims; anonymous has none", () => {
--- a/src/guards.ts
+++ b/src/guards.ts
@@ -5,6 +5,17 @@
 // Keto call — the fine-grained "may I?" tier (README), reserved for relationship rules.
 import type { RequestContext, User } from "./context.ts";
 import type { KetoClient } from "./keto-client.ts";
+import { localPath } from "./safe-url.ts";
+
+// Build the sign-in redirect for a gated request, preserving where the user was headed as
+// `return_to` so login can land them back there (§9). Only a safe GET/HEAD navigation to a
+// non-home, host-relative path is remembered (a POST or "/" ⇒ a bare /login); the target is
+// validated host-relative (localPath) so it can't become an open redirect.
+export function loginRedirect(ctx: RequestContext): string {
+  const method = (ctx.req.method ?? "GET").toUpperCase();
+  const target = method === "GET" || method === "HEAD" ? localPath(ctx.url.pathname + ctx.url.search) : null;
+  return target && target !== "/" ? `/login?return_to=${encodeURIComponent(target)}` : "/login";
+}

 // Thrown by an asserting guard; app.ts maps it to a response. `location` ⇒ a 303 redirect (an
 // anonymous browser bounces to /login); otherwise `status` renders an error page (403 Forbidden).
@@ -20,9 +31,9 @@ export class GuardError extends Error {
  }
 }

-// Assert a signed-in session and return the user. Anonymous ⇒ GuardError → /login.
+// Assert a signed-in session and return the user. Anonymous ⇒ GuardError → /login (return_to kept).
 export function requireSession(ctx: RequestContext): User {
-  if (!ctx.user) throw new GuardError(401, "authentication required", "/login");
+  if (!ctx.user) throw new GuardError(401, "authentication required", loginRedirect(ctx));
  return ctx.user;
 }

--- a/src/plugin-api.test.ts
+++ b/src/plugin-api.test.ts
@@ -6,10 +6,11 @@ import test from "node:test";
 import * as api from "./plugin-api.ts";

 test("plugin-api re-exports the stable author value surface", () => {
-  for (const name of ["definePlugin", "can", "check", "GuardError", "requireSession", "parseListQuery", "readFormBody", "CSRF_FIELD", "tracedFetch", "Log"]) {
+  for (const name of ["definePlugin", "can", "check", "GuardError", "requireSession", "parseListQuery", "readFormBody", "CSRF_FIELD", "tracedFetch", "Log", "safeUrl"]) {
    assert.ok(name in api && api[name as keyof typeof api] !== undefined, `missing export: ${name}`);
  }
  assert.equal(typeof api.definePlugin, "function");
  assert.equal(typeof api.tracedFetch, "function"); // the request-trace-aware fetch a plugin uses for upstream calls
+  assert.equal(api.safeUrl("javascript:alert(1)"), "#"); // the URL sanitiser for rendering untrusted hrefs
  assert.equal(api.definePlugin({ apiVersion: "1.0.0" }).apiVersion, "1.0.0"); // identity helper works through the barrel
 });
--- a/src/plugin-api.ts
+++ b/src/plugin-api.ts
@@ -13,6 +13,9 @@ export { can, check, GuardError, requireSession } from "./guards.ts";
 export { parseListQuery } from "./list-query.ts";
 export { readFormBody } from "./body.ts";
 export { CSRF_FIELD } from "./csrf.ts";
+// Sanitise an untrusted URL (upstream/user data) before rendering it in an href/src — partials
+// escape text but not URL schemes, so a `javascript:`/`data:` URL would be live XSS (see docs).
+export { safeUrl } from "./safe-url.ts";
 // Observability (§9): `ctx.log` (RequestContext) is the request logger; `tracedFetch` is a drop-in
 // `fetch` a plugin uses for upstream calls so they join the request's trace (client span + traceparent).
 // The `Log` class is exported so a plugin can type/construct one (e.g. `new Log("none")` in a test).
--- a/src/safe-url.test.ts
+++ b/src/safe-url.test.ts
@@ -0,0 +1,47 @@
+import assert from "node:assert/strict";
+import { test } from "node:test";
+import { localPath, safeUrl } from "./safe-url.ts";
+
+test("safeUrl: passes relative + http(s) through, neutralises dangerous schemes", () => {
+  // Relative forms (no scheme) and http(s) are rendered as-is.
+  assert.equal(safeUrl("/admin/users?q=1#f"), "/admin/users?q=1#f");
+  assert.equal(safeUrl("?q=1"), "?q=1");
+  assert.equal(safeUrl("#frag"), "#frag");
+  assert.equal(safeUrl("shifts/edit"), "shifts/edit");
+  assert.equal(safeUrl("http://example.com/x"), "http://example.com/x");
+  assert.equal(safeUrl("https://example.com/x"), "https://example.com/x");
+  assert.equal(safeUrl("HTTPS://EXAMPLE.com"), "HTTPS://EXAMPLE.com"); // scheme match is case-insensitive
+  // Any other scheme (the contract is: relative or http(s) only) ⇒ neutralised to "#".
+  assert.equal(safeUrl("javascript:alert(1)"), "#");
+  assert.equal(safeUrl("data:text/html,<script>alert(1)</script>"), "#");
+  assert.equal(safeUrl("vbscript:msgbox(1)"), "#");
+  assert.equal(safeUrl("mailto:x@y.z"), "#");
+  // Control-char / leading-whitespace obfuscation can't slip a scheme past the check (browsers
+  // strip TAB/CR/LF and leading controls before resolving the scheme).
+  assert.equal(safeUrl("java\tscript:alert(1)"), "#");
+  assert.equal(safeUrl("java\nscript:alert(1)"), "#");
+  assert.equal(safeUrl("  javascript:alert(1)"), "#");
+  // Empty / control-only ⇒ a safe no-op href.
+  assert.equal(safeUrl(""), "#");
+  // Leading Unicode whitespace above U+0020 (NBSP/NEL/LS) is left as-is on purpose: a browser only
+  // strips C0+space when resolving an href, so a NBSP-prefixed "javascript:" is an invalid scheme to
+  // it too \u2014 it resolves as a relative reference, not script. Documented so the strip set isn't widened later.
+  assert.equal(safeUrl("\u00a0javascript:alert(1)"), "\u00a0javascript:alert(1)");
+});
+
+test("localPath: accepts host-relative paths, rejects absolute / protocol-relative / odd input", () => {
+  assert.equal(localPath("/admin/users?q=1&page=2"), "/admin/users?q=1&page=2");
+  assert.equal(localPath("/"), "/");
+  // Protocol-relative and backslash variants are off-origin → rejected (open-redirect guard).
+  assert.equal(localPath("//evil.com"), null);
+  assert.equal(localPath("/\\evil.com"), null);
+  assert.equal(localPath("https://evil.com"), null);
+  assert.equal(localPath("javascript:alert(1)"), null);
+  assert.equal(localPath("relative/no-leading-slash"), null);
+  // Control chars / whitespace (a return_to is a server-built path) ⇒ rejected.
+  assert.equal(localPath("/x\nSet-Cookie: y"), null);
+  assert.equal(localPath("/a b"), null);
+  assert.equal(localPath(""), null);
+  assert.equal(localPath(null), null);
+  assert.equal(localPath(undefined), null);
+});
--- a/src/safe-url.ts
+++ b/src/safe-url.ts
@@ -0,0 +1,35 @@
+// URL safety helpers (todo §9). Two pure, dependency-free guards:
+//
+//   safeUrl(value)  — sanitise an untrusted URL before rendering it in an href/src attribute.
+//                     Partials escape *text*, but a URL field is emitted verbatim, so a
+//                     `javascript:`/`data:` URL from upstream/user data would be live XSS. The
+//                     contract (docs/plugin-contract.md) is: a relative or http(s) URL is allowed,
+//                     anything else collapses to "#". Exported to plugins via plugin-api.ts.
+//
+//   localPath(value) — validate a redirect target is a *same-origin* path (the redirect-URI
+//                     allowlist). Used for `return_to`: a host-relative "/a/b?x=1" passes, an
+//                     absolute or protocol-relative ("//evil.com", "https://evil.com") is rejected
+//                     so a crafted ?return_to= can't turn login completion into an open redirect.
+
+// ASCII control chars + space that browsers strip/ignore when resolving a URL — strip them before
+// the scheme check so "java\tscript:" / a leading space can't masquerade as relative.
+const CONTROL_G = /[\u0000-\u0020\u007f]/g;
+const CONTROL = /[\u0000-\u0020\u007f]/;
+const HAS_SCHEME = /^[a-z][a-z0-9+.-]*:/i; // a URL scheme prefix, e.g. "javascript:", "http:"
+const HTTP_SCHEME = /^https?:/i;
+
+export function safeUrl(value: string): string {
+  const cleaned = value.replace(CONTROL_G, "");
+  if (!cleaned) return "#";
+  // A scheme present? Allow only http(s). No scheme ⇒ relative ⇒ safe. Return the original once
+  // deemed safe (EJS still HTML-escapes it into the attribute; the inert control chars don't matter).
+  if (HAS_SCHEME.test(cleaned) && !HTTP_SCHEME.test(cleaned)) return "#";
+  return value;
+}
+
+export function localPath(value: string | null | undefined): string | null {
+  if (!value || CONTROL.test(value)) return null;
+  if (!value.startsWith("/")) return null; // must be host-relative
+  if (value.startsWith("//") || value.startsWith("/\\")) return null; // protocol-relative ⇒ off-origin
+  return value;
+}