From 2ab21c1b32ac4069773d402557bd7d61f2a08fc2 Mon Sep 17 00:00:00 2001 From: Codeman maintainer Date: Fri, 4 Sep 2026 15:21:13 +0200 Subject: [PATCH] fix(webview): revoke proxy capabilities on logout and stamp Referrer-Policy WebviewCapabilityStore.revokeOwner() shipped for two releases with a docstring claiming logout called it and no caller at all. The capability is a bearer credential exempt from cookie auth with a rolling TTL refreshed on every use, so a proxy URL that leaked (browser history, a screenshot, a dashboard with a loose referrer policy) stayed valid for as long as anything kept polling it. - POST /api/logout revokes the caller's capabilities (all of them in single-user mode), the admin forced logout revokes the target user's, and user deletion revokes whatever that user had open. revokeOwner returns the count for the admin audit line. - Proxied responses carry `Referrer-Policy: same-origin` and the upstream's own policy is dropped: every URL inside the frame carries the capability, and a dashboard on no-referrer-when-downgrade or unsafe-url handed it to any third-party host it linked. Verified with Playwright that a sandboxed frame under an upstream `unsafe-url` sends no Referer to a third party while the root-absolute fetch and the CSS-triggered 404 fallback still reach the dashboard. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01WKtW48T1UjAaecHAJxKobE --- src/web/routes/admin-routes.ts | 7 +- src/web/routes/session-routes.ts | 5 + src/web/webview-proxy.ts | 13 ++ src/webview-capabilities.ts | 25 +++- test/webview-capability-revocation.test.ts | 132 +++++++++++++++++++++ test/webview-proxy.test.ts | 31 +++++ 6 files changed, 209 insertions(+), 4 deletions(-) create mode 100644 test/webview-capability-revocation.test.ts diff --git a/src/web/routes/admin-routes.ts b/src/web/routes/admin-routes.ts index 2de62764..8f9294c9 100644 --- a/src/web/routes/admin-routes.ts +++ b/src/web/routes/admin-routes.ts @@ -35,6 +35,7 @@ import { UserStoreError, } from '../../user-store.js'; import { getAuthUser, requireAdmin, revokeUserSessions } from '../route-helpers.js'; +import { webviewCapabilities } from '../../webview-capabilities.js'; import { appendAdminAudit } from '../admin-audit.js'; import { SseEvent } from '../sse-events.js'; import type { AuthPort } from '../ports/auth-port.js'; @@ -179,7 +180,10 @@ export function registerAdminRoutes(app: FastifyInstance, ctx: SessionPort & Aut if (!gate(req, reply)) return; const { username } = req.params as { username: string }; const revoked = revokeUserSessions(ctx.authSessions, username); - audit(req, 'user.logout', username, { revoked }); + // Web-tab proxy capabilities are a second credential the cookie purge does not + // touch; a forced logout that left them alive would not be a logout. + const revokedWebviews = webviewCapabilities.revokeOwner(normalizeUsername(username)); + audit(req, 'user.logout', username, { revoked, revokedWebviews }); return { success: true, data: { revoked } }; }); @@ -201,6 +205,7 @@ export function registerAdminRoutes(app: FastifyInstance, ctx: SessionPort & Aut await ctx.cleanupSession(id, true, 'admin_delete_user').catch(() => {}); } revokeUserSessions(ctx.authSessions, username); + webviewCapabilities.revokeOwner(normalizeUsername(username)); if (deleteSpace) await deleteUserSpace(username); audit(req, 'user.delete', username, { deleteSpace, killedSessions: owned.length }); ctx.broadcast(SseEvent.AdminUsersChanged, {}); diff --git a/src/web/routes/session-routes.ts b/src/web/routes/session-routes.ts index 21195c42..8ef5ccaa 100644 --- a/src/web/routes/session-routes.ts +++ b/src/web/routes/session-routes.ts @@ -31,6 +31,7 @@ import { } from '../../types.js'; import { Session, isAltScreenStripMode, isExternalCliMode, isMuxAltScreenOnlyStripMode } from '../../session.js'; import { SseEvent } from '../sse-events.js'; +import { webviewCapabilities } from '../../webview-capabilities.js'; import { CreateSessionSchema, SessionNameSchema, @@ -821,6 +822,10 @@ export function registerSessionRoutes( if (sessionToken) { ctx.authSessions?.delete(sessionToken); } + // The web-tab proxy authenticates on capabilities, not on this cookie, so a + // logout has to retire them too or every dashboard URL opened during this + // login keeps relaying without one (WebviewCapabilityStore.revokeOwner). + webviewCapabilities.revokeOwner(ownerFor(req)); reply.clearCookie(AUTH_COOKIE_NAME, { path: '/' }); return {}; }); diff --git a/src/web/webview-proxy.ts b/src/web/webview-proxy.ts index bc0633f9..4856b34b 100644 --- a/src/web/webview-proxy.ts +++ b/src/web/webview-proxy.ts @@ -93,6 +93,10 @@ const DROP_RESPONSE_HEADERS = new Set([ 'access-control-allow-headers', 'access-control-expose-headers', 'access-control-max-age', + // The capability rides in every proxied URL, so the upstream's own referrer + // policy must not decide whether third parties receive it. Ours is stamped in + // buildDownstreamResponseHeaders. + 'referrer-policy', ]); /** The same-origin path prefix an iframe loads for a given capability. */ @@ -352,6 +356,15 @@ export function buildDownstreamResponseHeaders( headers[lower] = value; } + // Every URL inside the frame carries the capability, and a dashboard that sets + // `no-referrer-when-downgrade` or `unsafe-url` would hand it to any third-party + // host it links or embeds. `same-origin` keeps the Referer on requests back to + // Codeman (the 404 fallback and `refererPath` rely on it; both compare URL + // origins, which an opaque-origin frame still satisfies) and strips it for + // everyone else. A `` inside the document can still + // override this; that is the dashboard author's own decision about their page. + headers['referrer-policy'] = 'same-origin'; + const setCookie = setCookies.map((cookie) => rewriteSetCookie(cookie, capability, secureContext)); return { headers, setCookie, csp }; diff --git a/src/webview-capabilities.ts b/src/webview-capabilities.ts index fa4ccc90..84263ccd 100644 --- a/src/webview-capabilities.ts +++ b/src/webview-capabilities.ts @@ -13,7 +13,8 @@ * * - 128 bits of `randomBytes` entropy, base64url, never derived from anything. * - Held in memory only. A restart invalidates every outstanding capability. - * - Rolling TTL: refreshed on use, expired after inactivity. + * - Rolling TTL: refreshed on use, expired after inactivity, and revoked outright + * on logout, admin logout and user deletion (`revokeOwner`). * - Bound to the minting user, so multi-user ownership survives the exemption. * - Grants exactly one thing: relaying bytes to that one saved URL. It reaches no * session, no file, no API surface. @@ -81,15 +82,33 @@ export class WebviewCapabilityStore { } } - /** Revoke every capability minted by a user (called on logout / user deletion). */ - revokeOwner(owner: string): void { + /** + * Revoke every capability bound to an identity. Called from `POST /api/logout` + * (the caller's own identity, which in single-user mode is `undefined`, i.e. + * every capability there is), from the admin logout route, and from user + * deletion. + * + * ⚠️ This method shipped for two releases with NO caller while its docstring + * claimed logout invoked it. The rolling TTL is refreshed on every use, so a + * proxy URL that leaked (browser history, a shared screenshot, a dashboard with + * a loose referrer policy) stayed valid indefinitely as long as something kept + * polling it. Logging out is the user's one deliberate "invalidate what I + * opened" gesture, and it has to reach here; `test/webview-capability-revocation.test.ts` + * pins each call site. + * + * @returns how many capabilities were revoked (for the admin audit line). + */ + revokeOwner(owner: string | undefined): number { + let revoked = 0; for (const [webviewId, token] of [...this.byWebview]) { const record = this.capabilities.peek(token); if (record?.owner === owner) { this.capabilities.delete(token); this.byWebview.delete(webviewId); + revoked++; } } + return revoked; } get size(): number { diff --git a/test/webview-capability-revocation.test.ts b/test/webview-capability-revocation.test.ts new file mode 100644 index 00000000..8be83f41 --- /dev/null +++ b/test/webview-capability-revocation.test.ts @@ -0,0 +1,132 @@ +/** + * Web-tab proxy capabilities must die with the login that minted them. + * + * `WebviewCapabilityStore.revokeOwner()` shipped for two releases with a docstring + * saying logout called it and NO caller. The capability is a bearer credential + * exempt from cookie auth, with a rolling TTL refreshed on every use, so a leaked + * proxy URL stayed valid indefinitely. These tests pin every call site: + * `POST /api/logout` (own identity), the admin forced logout, and user deletion. + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { WebviewCapabilityStore, webviewCapabilities } from '../src/webview-capabilities.js'; +import { createRouteTestHarness, type RouteTestHarness } from './routes/_route-test-utils.js'; +import { registerSessionRoutes } from '../src/web/routes/session-routes.js'; +import { registerAdminRoutes } from '../src/web/routes/admin-routes.js'; +import { createUser, invalidateUsersCache } from '../src/user-store.js'; + +const PASSWORD = 'correct-horse-battery-staple'; + +describe('WebviewCapabilityStore.revokeOwner', () => { + it('revokes exactly the identity asked for, the single-user `undefined` identity included', () => { + const store = new WebviewCapabilityStore(); + const solo = store.mint('wv-solo', undefined); + const alice = store.mint('wv-alice', 'alice'); + const bob = store.mint('wv-bob', 'bob'); + + expect(store.revokeOwner('alice')).toBe(1); + expect(store.resolve(alice)).toBeUndefined(); + expect(store.resolve(bob)).toBeDefined(); + expect(store.resolve(solo)).toBeDefined(); + + expect(store.revokeOwner(undefined)).toBe(1); + expect(store.resolve(solo)).toBeUndefined(); + expect(store.resolve(bob)).toBeDefined(); + + // A later open mints a NEW token rather than resurrecting the revoked one. + expect(store.mint('wv-alice', 'alice')).not.toBe(alice); + expect(store.revokeOwner('nobody')).toBe(0); + store.dispose(); + }); +}); + +describe('POST /api/logout', () => { + let harness: RouteTestHarness; + + beforeAll(async () => { + harness = await createRouteTestHarness(registerSessionRoutes); + }); + + afterAll(async () => { + await harness.app.close(); + }); + + it('single-user: every outstanding capability dies with the login', async () => { + const cap = webviewCapabilities.mint('wv-logout-solo', undefined); + expect(webviewCapabilities.resolve(cap)).toBeDefined(); + + const res = await harness.app.inject({ method: 'POST', url: '/api/logout' }); + expect(res.statusCode).toBe(200); + expect(webviewCapabilities.resolve(cap)).toBeUndefined(); + }); +}); + +describe('POST /api/logout in multi-user mode', () => { + let harness: RouteTestHarness; + let savedMode: string | undefined; + + beforeAll(async () => { + savedMode = process.env.CODEMAN_MULTIUSER; + process.env.CODEMAN_MULTIUSER = '1'; + harness = await createRouteTestHarness(registerSessionRoutes, { authUser: { username: 'peon', role: 'user' } }); + }); + + afterAll(async () => { + await harness.app.close(); + if (savedMode === undefined) delete process.env.CODEMAN_MULTIUSER; + else process.env.CODEMAN_MULTIUSER = savedMode; + }); + + it("revokes only the caller's capabilities, never another user's", async () => { + const mine = webviewCapabilities.mint('wv-peon-own', 'peon'); + const theirs = webviewCapabilities.mint('wv-boss-own', 'boss'); + + const res = await harness.app.inject({ method: 'POST', url: '/api/logout' }); + expect(res.statusCode).toBe(200); + expect(webviewCapabilities.resolve(mine)).toBeUndefined(); + expect(webviewCapabilities.resolve(theirs)).toBeDefined(); + webviewCapabilities.revokeWebview('wv-boss-own'); + }); +}); + +describe('admin routes (multi-user)', () => { + let harness: RouteTestHarness; + let savedMode: string | undefined; + + // The temp HOME from test/setup.ts is per-FILE, so users.json persists across + // the tests in this block. + beforeAll(async () => { + savedMode = process.env.CODEMAN_MULTIUSER; + process.env.CODEMAN_MULTIUSER = '1'; + invalidateUsersCache(); + await createUser({ username: 'boss', role: 'admin', password: PASSWORD }); + await createUser({ username: 'peon', role: 'user', password: PASSWORD }); + harness = await createRouteTestHarness(registerAdminRoutes, { authUser: { username: 'boss', role: 'admin' } }); + }); + + afterAll(async () => { + await harness.app.close(); + if (savedMode === undefined) delete process.env.CODEMAN_MULTIUSER; + else process.env.CODEMAN_MULTIUSER = savedMode; + invalidateUsersCache(); + }); + + it('a forced logout revokes the target user (normalised) and leaves the admin alone', async () => { + const peon = webviewCapabilities.mint('wv-peon-forced', 'peon'); + const boss = webviewCapabilities.mint('wv-boss-forced', 'boss'); + + const res = await harness.app.inject({ method: 'POST', url: '/api/admin/users/PEON/logout' }); + expect(res.statusCode).toBe(200); + expect(webviewCapabilities.resolve(peon)).toBeUndefined(); + expect(webviewCapabilities.resolve(boss)).toBeDefined(); + webviewCapabilities.revokeWebview('wv-boss-forced'); + }); + + it('deleting a user revokes whatever that user had open', async () => { + const peon = webviewCapabilities.mint('wv-peon-deleted', 'peon'); + + const res = await harness.app.inject({ method: 'DELETE', url: '/api/admin/users/peon' }); + expect(res.statusCode).toBe(200); + expect(webviewCapabilities.resolve(peon)).toBeUndefined(); + }); +}); diff --git a/test/webview-proxy.test.ts b/test/webview-proxy.test.ts index 6815f889..bd020f34 100644 --- a/test/webview-proxy.test.ts +++ b/test/webview-proxy.test.ts @@ -653,3 +653,34 @@ describe('misc helpers', () => { expect(proxyPrefixFor(CAP)).toBe(PREFIX); }); }); + +describe('referrer policy on proxied responses', () => { + const CAP = 'c'.repeat(32); + const requestUrl = new URL('http://127.0.0.1:4000/'); + + it('stamps same-origin and drops the upstream policy, so the capability in the URL never reaches a third party', () => { + const { headers } = buildDownstreamResponseHeaders( + [ + ['referrer-policy', 'unsafe-url'], + ['content-type', 'text/html'], + ], + [], + CAP, + requestUrl, + false + ); + expect(headers['referrer-policy']).toBe('same-origin'); + expect(headers['content-type']).toBe('text/html'); + }); + + it('stamps it even when the upstream sent none (the browser default would still leak on a downgrade-style policy)', () => { + const { headers } = buildDownstreamResponseHeaders( + [['content-type', 'application/json']], + [], + CAP, + requestUrl, + false + ); + expect(headers['referrer-policy']).toBe('same-origin'); + }); +});