From a048cb3553f4853a64358ed276a84d0a0a431a90 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Wed, 4 Nov 2026 03:10:26 +0100 Subject: [PATCH 1/3] fix: stop an approved mail server sending the desktop app to a server nobody approved --- lib/auth/verify-jmap-auth.ts | 7 +- lib/desktop/__tests__/server-redirect.test.ts | 200 ++++++++++++++++++ lib/desktop/server-redirect.ts | 31 +++ lib/stalwart/jmap-api.ts | 3 +- lib/stalwart/server-fetch.ts | 18 +- 5 files changed, 250 insertions(+), 9 deletions(-) create mode 100644 lib/desktop/__tests__/server-redirect.test.ts create mode 100644 lib/desktop/server-redirect.ts diff --git a/lib/auth/verify-jmap-auth.ts b/lib/auth/verify-jmap-auth.ts index 6c455cdf4..b565de6e0 100644 --- a/lib/auth/verify-jmap-auth.ts +++ b/lib/auth/verify-jmap-auth.ts @@ -6,6 +6,7 @@ import { } from '@/lib/security/url-guard'; import { forwardedClientHeaders } from '@/lib/auth/client-address'; import { observeUpstream } from '@/lib/metrics/upstream'; +import { desktopRedirectRefused } from '@/lib/desktop/server-redirect'; const VERIFY_TIMEOUT_MS = 10000; const MAX_REDIRECTS = 3; @@ -453,7 +454,11 @@ async function fetchVerifiedSession( if (!location) { throw new JmapAuthVerificationError('Failed to verify JMAP session', 502); } - currentUrl = new URL(location, currentUrl).toString(); + const next = new URL(location, currentUrl).toString(); + if (desktopRedirectRefused(currentUrl, next, options.trusted === true)) { + throw new JmapAuthVerificationError('The mail server redirected to a server that has not been approved', 502); + } + currentUrl = next; continue; } break; diff --git a/lib/desktop/__tests__/server-redirect.test.ts b/lib/desktop/__tests__/server-redirect.test.ts new file mode 100644 index 000000000..8442de02b --- /dev/null +++ b/lib/desktop/__tests__/server-redirect.test.ts @@ -0,0 +1,200 @@ +// @vitest-environment node +import { createServer, type IncomingMessage, type Server, type ServerResponse } from 'node:http'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +/** + * An approved server sits inside the SSRF guard, so where it redirects to is the + * whole question. Real sockets: what counts is who received the request, and + * whether it carried the user's credentials. + */ + +const TRUST_KEY = Symbol.for('bulwark.desktop.trustedJmapOrigins'); +const POLICY_KEY = Symbol.for('bulwark.desktop.serverPolicy'); +const holder = globalThis as unknown as Record; +const AUTH = `Basic ${Buffer.from('probe@example.org:not-a-real-password').toString('base64')}`; + +interface Host { + origin: string; + hits: { url: string; auth: boolean }[]; + answer: (req: IncomingMessage, res: ServerResponse) => void; +} + +const servers: Server[] = []; + +function session(res: ServerResponse) { + res.writeHead(200, { 'content-type': 'application/json' }); + res.end(JSON.stringify({ apiUrl: '/jmap/', accounts: { a: {} }, primaryAccounts: { 'urn:ietf:params:jmap:mail': 'a' }, username: 'probe@example.org' })); +} + +async function host(answer: Host['answer'] = (_req, res) => session(res)): Promise { + const self: Host = { origin: '', hits: [], answer }; + const server = createServer((req, res) => { + self.hits.push({ url: req.url ?? '', auth: !!req.headers.authorization }); + self.answer(req, res); + }); + await new Promise((resolve) => server.listen(0, '127.0.0.1', resolve)); + servers.push(server); + self.origin = `http://127.0.0.1:${(server.address() as { port: number }).port}`; + return self; +} + +const redirectTo = (location: string, status = 307): Host['answer'] => (_req, res) => { + res.writeHead(status, { location }); + res.end(); +}; + +function approve(...origins: string[]) { + holder[TRUST_KEY] = new Set(origins); +} + +beforeEach(() => { + vi.resetModules(); + delete holder[TRUST_KEY]; + delete holder[POLICY_KEY]; +}); + +afterEach(async () => { + vi.unstubAllEnvs(); + delete holder[TRUST_KEY]; + delete holder[POLICY_KEY]; + await Promise.all(servers.splice(0).map((server) => new Promise((resolve) => server.close(resolve)))); +}); + +async function verify(server: string) { + const { fetchJmapSessionIdentity } = await import('@/lib/auth/verify-jmap-auth'); + return fetchJmapSessionIdentity(server, AUTH, { trusted: true }); +} + +async function fetchVia(url: string, init: RequestInit = {}) { + const { fetchJmapServer } = await import('@/lib/stalwart/server-fetch'); + return fetchJmapServer(url, { headers: { Authorization: AUTH }, ...init }, true); +} + +describe('the desktop build, signing in to an approved server', () => { + beforeEach(() => vi.stubEnv('NEXT_PUBLIC_BULWARK_DESKTOP', '1')); + + it('follows a redirect on the same origin, as Stalwart answers /.well-known/jmap', async () => { + const a = await host((req, res) => (req.url === '/.well-known/jmap' ? redirectTo('/jmap/session')(req, res) : session(res))); + approve(a.origin); + await expect(verify(a.origin)).resolves.toBeTruthy(); + expect(a.hits).toEqual([{ url: '/.well-known/jmap', auth: true }, { url: '/jmap/session', auth: true }]); + }); + + it('refuses a redirect to a server nobody approved, which gets nothing', async () => { + const b = await host(); + const a = await host(redirectTo('')); + a.answer = redirectTo(`${b.origin}/.well-known/jmap`); + approve(a.origin); + await expect(verify(a.origin)).rejects.toMatchObject({ name: 'JmapAuthVerificationError', status: 502 }); + expect(b.hits).toEqual([]); + }); + + it('counts another name for the same machine as another server', async () => { + const b = await host(); + const a = await host(redirectTo(`${b.origin.replace('127.0.0.1', 'localhost')}/x`)); + approve(a.origin); + await expect(verify(a.origin)).rejects.toMatchObject({ status: 502 }); + expect(b.hits).toEqual([]); + }); + + it('follows one to another approved server', async () => { + const b = await host(); + const a = await host(redirectTo(`${b.origin}/jmap/session`)); + approve(a.origin, b.origin); + await expect(verify(a.origin)).resolves.toBeTruthy(); + expect(b.hits).toEqual([{ url: '/jmap/session', auth: true }]); + }); + + it('holds AllowedJmapServers across the redirect, even to an approved server', async () => { + const b = await host(); + const a = await host(redirectTo(`${b.origin}/jmap/session`)); + approve(a.origin, b.origin); + holder[POLICY_KEY] = { pluginsMode: 'allowed', pluginAllowlist: [], allowedJmapServers: [a.origin] }; + await expect(verify(a.origin)).rejects.toMatchObject({ status: 502 }); + expect(b.hits).toEqual([]); + }); +}); + +describe('the desktop build, other requests to an approved server', () => { + beforeEach(() => vi.stubEnv('NEXT_PUBLIC_BULWARK_DESKTOP', '1')); + + it('follows the same origin', async () => { + const a = await host((req, res) => (req.url === '/.well-known/jmap' ? redirectTo('/jmap/session')(req, res) : session(res))); + approve(a.origin); + expect((await fetchVia(`${a.origin}/.well-known/jmap`)).status).toBe(200); + expect(a.hits.map((hit) => hit.auth)).toEqual([true, true]); + }); + + it('refuses a server nobody approved, which gets nothing', async () => { + const b = await host(); + const a = await host(redirectTo(`${b.origin}/x`)); + approve(a.origin); + await expect(fetchVia(`${a.origin}/y`)).rejects.toThrow(); + expect(b.hits).toEqual([]); + }); + + it('still hands back the redirect when asked to', async () => { + const b = await host(); + const a = await host(redirectTo(`${b.origin}/x`)); + approve(a.origin); + expect((await fetchVia(`${a.origin}/y`, { redirect: 'manual' })).status).toBe(307); + expect(b.hits).toEqual([]); + }); + + it('postJmap keeps to the approved set too', async () => { + const b = await host(); + const a = await host(redirectTo(`https://127.0.0.1:${new URL(b.origin).port}/jmap/`)); + approve(a.origin); + const { postJmap } = await import('@/lib/stalwart/jmap-api'); + await expect(postJmap(`${a.origin}/jmap`, AUTH, '{}', { trusted: true })).rejects.toThrow(/untrusted host/); + expect(b.hits).toEqual([]); + }); +}); + +describe('which hops count', () => { + beforeEach(() => vi.stubEnv('NEXT_PUBLIC_BULWARK_DESKTOP', '1')); + + async function refused(from: string, to: string, approved = true) { + const { desktopRedirectRefused } = await import('../server-redirect'); + return desktopRedirectRefused(from, to, approved); + } + + it('lets the same host move up to https on the standard ports only', async () => { + expect(await refused('http://mail.lan/x', 'https://mail.lan/x')).toBe(false); + expect(await refused('http://mail.lan:8080/x', 'https://mail.lan/x')).toBe(true); + expect(await refused('http://mail.lan/x', 'https://mail.lan:8443/x')).toBe(true); + expect(await refused('http://mail.lan/x', 'https://other.lan/x')).toBe(true); + expect(await refused('https://mail.lan/x', 'http://mail.lan/x')).toBe(true); + }); + + it('leaves a public server to the hop-by-hop guard, but not to the policy', async () => { + expect(await refused('https://a.example/x', 'https://b.example/x', false)).toBe(false); + holder[POLICY_KEY] = { pluginsMode: 'allowed', pluginAllowlist: [], allowedJmapServers: ['https://a.example'] }; + expect(await refused('https://a.example/x', 'https://b.example/x', false)).toBe(true); + expect(await refused('https://a.example/x', 'https://a.example/y', false)).toBe(false); + }); +}); + +describe('a server install', () => { + it('follows an admin-configured server as it always did', async () => { + const b = await host(); + const a = await host(redirectTo(`${b.origin}/jmap/session`)); + approve(a.origin); + await expect(verify(a.origin)).resolves.toBeTruthy(); + expect(b.hits).toEqual([{ url: '/jmap/session', auth: true }]); + }); + + it('other requests too, where fetch takes the credentials off', async () => { + const b = await host(); + const a = await host(redirectTo(`${b.origin}/x`)); + expect((await fetchVia(`${a.origin}/y`)).status).toBe(200); + expect(b.hits).toEqual([{ url: '/x', auth: false }]); + }); + + it('has no opinion on any hop', async () => { + const { desktopRedirectRefused } = await import('../server-redirect'); + approve(); + holder[POLICY_KEY] = { pluginsMode: 'allowed', pluginAllowlist: [], allowedJmapServers: [] }; + expect(desktopRedirectRefused('http://a.lan/', 'http://169.254.169.254/', true)).toBe(false); + }); +}); diff --git a/lib/desktop/server-redirect.ts b/lib/desktop/server-redirect.ts new file mode 100644 index 000000000..64223a61d --- /dev/null +++ b/lib/desktop/server-redirect.ts @@ -0,0 +1,31 @@ +/** + * Where a mail server may send the desktop app's own server with a redirect. + * + * An approved server gets past the SSRF guard, and the guard only knows about the + * address the user approved. Following its redirects anywhere would let it aim the + * app at the LAN or at loopback, with the user's credentials on the request. So a + * hop stays on its origin, goes to another approved server, or moves the same host + * from http to https on the standard ports. Nothing else counts as the same server. + * + * A public server's hops go through the rebinding-safe fetch, which checks each of + * them. AllowedJmapServers holds for every hop, wherever it comes from. + * + * Every other build keeps its rules: a server an admin configured is the admin's. + */ + +import { mailServerRefusedByDevicePolicy } from './server-policy'; +import { isDesktopTrustedServer } from './trusted-servers'; + +function movesUpToHttps(from: URL, to: URL): boolean { + return from.protocol === 'http:' && to.protocol === 'https:' && from.hostname === to.hostname && from.port === '' && to.port === ''; +} + +/** `approved`: the request was let past the guard as an approved server. */ +export function desktopRedirectRefused(from: string | URL, to: string | URL, approved: boolean): boolean { + if (process.env.NEXT_PUBLIC_BULWARK_DESKTOP !== '1') return false; + const source = new URL(from); + const target = new URL(to); + if (mailServerRefusedByDevicePolicy(target.href)) return true; + if (!approved || source.origin === target.origin) return false; + return !isDesktopTrustedServer(target.href) && !movesUpToHttps(source, target); +} diff --git a/lib/stalwart/jmap-api.ts b/lib/stalwart/jmap-api.ts index dfe1cd0e7..660267896 100644 --- a/lib/stalwart/jmap-api.ts +++ b/lib/stalwart/jmap-api.ts @@ -17,6 +17,7 @@ * this process — see calendar-agenda's session handling). */ +import { desktopRedirectRefused } from '@/lib/desktop/server-redirect'; import { fetchJmapServer } from '@/lib/stalwart/server-fetch'; import { UPSTREAM_JSON_MAX_BYTES, readJsonCapped } from '@/lib/security/capped-body'; import { forwardedClientHeaders } from '@/lib/auth/client-address'; @@ -88,7 +89,7 @@ export async function postJmap( if (!location) return response; const next = new URL(location, url); - if (!isTrustedRedirect(url, next)) { + if (!isTrustedRedirect(url, next) || desktopRedirectRefused(url, next, options.trusted !== false)) { throw new JmapRedirectError( `JMAP endpoint redirected to an untrusted host: ${next.host}`, ); diff --git a/lib/stalwart/server-fetch.ts b/lib/stalwart/server-fetch.ts index 7e0296cf2..d5e657732 100644 --- a/lib/stalwart/server-fetch.ts +++ b/lib/stalwart/server-fetch.ts @@ -20,6 +20,7 @@ import { logger } from '@/lib/logger'; import { parseJmapServers, resolveTrustedJmapUrl } from '@/lib/admin/jmap-servers'; import { fetchPublicUrl, type PublicFetchResponse } from '@/lib/security/url-guard'; import { observeUpstream } from '@/lib/metrics/upstream'; +import { desktopRedirectRefused } from '@/lib/desktop/server-redirect'; import { isStoredCredentialRefused, recordStoredCredentialStatus, @@ -100,7 +101,9 @@ async function fetchUnbudgeted( // endpoints on, one the user chose) must not pin a connection for as long // as it likes. Callers with their own signal keep it. const init: ServerFetchInit = givenInit.signal ? givenInit : { ...givenInit, signal: AbortSignal.timeout(UPSTREAM_TIMEOUT_MS) }; - if (trusted !== false) { + // The desktop app's approved server gets its hops checked by hand as well. + const approvedOnDesktop = process.env.NEXT_PUBLIC_BULWARK_DESKTOP === '1' && trusted !== false; + if (trusted !== false && !approvedOnDesktop) { return fetch(url, init); } @@ -111,12 +114,10 @@ async function fetchUnbudgeted( let body = rest.body; for (let hop = 0; hop <= MAX_REDIRECTS; hop++) { - const response: PublicFetchResponse = await fetchPublicUrl(currentUrl, { - ...(rest as Omit), - method, - body: body as never, - headers, - }); + const hop = { ...(rest as Omit), method, body: body as never, headers }; + const response: PublicFetchResponse = approvedOnDesktop + ? ((await fetch(currentUrl, { ...hop, redirect: 'manual' } as RequestInit)) as unknown as PublicFetchResponse) + : await fetchPublicUrl(currentUrl, hop); if (redirect === 'manual' || response.status < 300 || response.status >= 400) { return response as unknown as Response; @@ -129,6 +130,9 @@ async function fetchUnbudgeted( await response.body?.cancel().catch(() => {}); const next = new URL(location, currentUrl); + if (desktopRedirectRefused(currentUrl, next, approvedOnDesktop)) { + throw new Error(`${currentUrl} redirected to a server that has not been approved`); + } if (next.origin !== new URL(currentUrl).origin) { for (const key of Object.keys(headers)) { if (key.toLowerCase() === 'authorization' || key.toLowerCase() === 'cookie') { From a9e33ec2d6f6c917b35d80b88ac0d1c5f757e513 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Wed, 4 Nov 2026 03:11:25 +0100 Subject: [PATCH 2/3] fix: stop a page wiping another account's plugin secrets by naming it in the desktop app A wipe by name took any username and server with no session behind it. It now needs a sealed cookie the account got while it was signed in, since a dropped account has no session cookie left. The old wipe test asserted that a name alone was enough, which is the behaviour being removed. --- app/api/plugin-secrets/__tests__/wipe.test.ts | 62 ++++++++++++++++++- app/api/plugin-secrets/route.ts | 24 +++++-- lib/auth/crypto.ts | 5 +- lib/offline/evicted-secrets.ts | 7 ++- lib/plugin-secrets/wipe-proof.ts | 42 +++++++++++++ 5 files changed, 127 insertions(+), 13 deletions(-) create mode 100644 lib/plugin-secrets/wipe-proof.ts diff --git a/app/api/plugin-secrets/__tests__/wipe.test.ts b/app/api/plugin-secrets/__tests__/wipe.test.ts index 075138ac9..cc9bcaecc 100644 --- a/app/api/plugin-secrets/__tests__/wipe.test.ts +++ b/app/api/plugin-secrets/__tests__/wipe.test.ts @@ -16,8 +16,16 @@ vi.mock('@/lib/stalwart/credentials', () => ({ vi.mock('@/lib/admin/config-manager', () => ({ configManager: { ensureLoaded: async () => {}, get: () => undefined, getPolicy: () => ({ features: { pluginsEnabled: true } }) }, })); +vi.mock('@/lib/plugin-secrets/known-plugins', () => ({ + declaredUserCredential: async () => ({ inject: { type: 'bearer' } }), +})); +vi.mock('@/lib/plugin-secrets/proxy', () => ({ + adminCredentialOrigins: async () => [], + adminCredentialStatus: async () => [], +})); vi.mock('@/lib/plugin-secrets/vault', () => ({ VaultFullError: class extends Error {}, + userKey: (user: { username: string; serverUrl: string }) => `${Buffer.from(`${user.username}:${user.serverUrl}`).toString('hex')}0000000000000000`.slice(0, 64), deleteAllUserCredentials: vi.fn(async (user: unknown) => { deleted.push(user); }), deleteUserCredentials: vi.fn(async () => {}), listUserCredentials: vi.fn(async () => []), @@ -31,10 +39,23 @@ async function remove(query: string, headers: Record = { 'sec-fe } const EVICTED = 'all=true&username=alice&server=https%3A%2F%2Fmail.example.org'; +const SAME_ORIGIN = { 'sec-fetch-site': 'same-origin' }; + +/** The proof cookie an account gets while signed in, as `name=value`. */ +async function proofFor(username: string): Promise { + vi.resetModules(); + const { setWipeProof } = await import('@/lib/plugin-secrets/wipe-proof'); + const { NextResponse } = await import('next/server'); + const answer = NextResponse.json({}); + setWipeProof(answer, { username, serverUrl: 'https://mail.example.org' }); + const cookie = answer.cookies.getAll()[0]; + return `${cookie.name}=${cookie.value}`; +} beforeEach(() => { deleted.length = 0; session.signedIn = false; + vi.stubEnv('SESSION_SECRET', 'wipe-test-session-secret-wipe-test-session-secret'); }); afterEach(() => { @@ -42,23 +63,58 @@ afterEach(() => { }); describe('DELETE /api/plugin-secrets?all=true', () => { - it('in the desktop app takes a dropped account by who it was, with no cookie', async () => { + it('in the desktop app takes a dropped account by who it was, with the proof it got while signed in', async () => { vi.stubEnv('NEXT_PUBLIC_BULWARK_DESKTOP', '1'); - expect((await remove(EVICTED)).status).toBe(200); + const answer = await remove(EVICTED, { ...SAME_ORIGIN, cookie: await proofFor('alice') }); + expect(answer.status).toBe(200); expect(deleted).toEqual([{ username: 'alice', serverUrl: 'https://mail.example.org' }]); + expect(answer.headers.get('set-cookie')).toMatch(/Max-Age=0/i); + }); + + it('in the desktop app won\'t take any name on its own', async () => { + vi.stubEnv('NEXT_PUBLIC_BULWARK_DESKTOP', '1'); + expect((await remove(EVICTED)).status).toBe(401); + expect(deleted).toEqual([]); + }); + + it('in the desktop app won\'t take another account\'s proof, or the signed-in account\'s session, for that name', async () => { + vi.stubEnv('NEXT_PUBLIC_BULWARK_DESKTOP', '1'); + session.signedIn = true; + const bob = await proofFor('bob'); + const aliceName = (await proofFor('alice')).split('=')[0]; + expect((await remove(EVICTED, { ...SAME_ORIGIN, cookie: `${aliceName}=${bob.split('=').slice(1).join('=')}` })).status).toBe(401); + expect((await remove(EVICTED, { ...SAME_ORIGIN, cookie: `${aliceName}=forged` })).status).toBe(401); + expect(deleted).toEqual([]); }); it('won\'t take it from another site', async () => { vi.stubEnv('NEXT_PUBLIC_BULWARK_DESKTOP', '1'); - expect((await remove(EVICTED, { 'sec-fetch-site': 'cross-site' })).status).toBe(403); + const cookie = await proofFor('alice'); + expect((await remove(EVICTED, { 'sec-fetch-site': 'cross-site', cookie })).status).toBe(403); expect(deleted).toEqual([]); }); + it('gives the proof when an account lists what it has, in the desktop app only', async () => { + session.signedIn = true; + const list = async () => { + vi.resetModules(); + const vault = await import('@/lib/plugin-secrets/vault'); + vi.mocked(vault.listUserCredentials).mockResolvedValueOnce([{ pluginId: 'p' } as never]); + const { GET } = await import('../route'); + return GET(new NextRequest('https://app.local/api/plugin-secrets', { headers: SAME_ORIGIN })); + }; + expect((await list()).headers.get('set-cookie')).toBeNull(); + vi.stubEnv('NEXT_PUBLIC_BULWARK_DESKTOP', '1'); + expect((await list()).headers.get('set-cookie')).toMatch(/^jmap_psp_/); + }); + it('on an instance deletes nobody\'s, with a cookie or without', async () => { expect((await remove(EVICTED)).status).toBe(401); session.signedIn = true; await remove(EVICTED); expect(deleted).toEqual([]); + await remove(EVICTED, { ...SAME_ORIGIN, cookie: await proofFor('alice') }); + expect(deleted).toEqual([]); }); it('in the desktop app with a cookie and no name takes the signed-in account', async () => { diff --git a/app/api/plugin-secrets/route.ts b/app/api/plugin-secrets/route.ts index b767e0dbe..9ccce655b 100644 --- a/app/api/plugin-secrets/route.ts +++ b/app/api/plugin-secrets/route.ts @@ -15,6 +15,7 @@ import { } from '@/lib/plugin-secrets/vault'; import { storedDataUnavailableResponse } from '@/lib/stored-data-errors'; import { IS_DESKTOP_BUILD } from '@/lib/desktop/flag'; +import { clearWipeProof, hasWipeProof, setWipeProof } from '@/lib/plugin-secrets/wipe-proof'; const PLUGIN_ID_RE = /^[a-z0-9][a-z0-9-]*[a-z0-9]$/; const MAX_SECRET_LENGTH = 4096; @@ -52,7 +53,9 @@ export async function GET(request: NextRequest) { const [admin, adminOrigins] = pluginId ? await Promise.all([adminCredentialStatus(pluginId), adminCredentialOrigins(pluginId)]) : [[], []]; - return NextResponse.json({ credentials, admin, adminOrigins }, { headers: NO_STORE }); + const answer = NextResponse.json({ credentials, admin, adminOrigins }, { headers: NO_STORE }); + if (IS_DESKTOP_BUILD && credentials.length > 0) setWipeProof(answer, user); + return answer; } /** @@ -107,21 +110,28 @@ export async function POST(request: NextRequest) { if (unavailable) return unavailable; throw error; } - return NextResponse.json({ ok: true }, { headers: NO_STORE }); + const answer = NextResponse.json({ ok: true }, { headers: NO_STORE }); + if (IS_DESKTOP_BUILD) setWipeProof(answer, user); + return answer; } /** DELETE /api/plugin-secrets?plugin=[&credential=] - disconnect. */ export async function DELETE(request: NextRequest) { const crossOrigin = rejectCrossOriginRequest(request); if (crossOrigin) return crossOrigin; - // An account dropped earlier, in the desktop app: no cookie left, and this server serves this device alone. + // An account dropped earlier, in the desktop app: no session cookie left, so it + // takes the proof that account got while it had one (lib/plugin-secrets/wipe-proof.ts). const { searchParams } = request.nextUrl; const owner = searchParams.get('username'); const server = searchParams.get('server'); if (IS_DESKTOP_BUILD && searchParams.get('all') === 'true' && owner && server) { + const named = { username: owner, serverUrl: server }; + if (!hasWipeProof(request, named)) return NextResponse.json({ error: 'Not authenticated' }, { status: 401, headers: NO_STORE }); try { - await deleteAllUserCredentials({ username: owner, serverUrl: server }); - return NextResponse.json({ ok: true }, { headers: NO_STORE }); + await deleteAllUserCredentials(named); + const answer = NextResponse.json({ ok: true }, { headers: NO_STORE }); + clearWipeProof(answer, named); + return answer; } catch (error) { const unavailable = storedDataUnavailableResponse(error, NO_STORE); if (unavailable) return unavailable; @@ -135,7 +145,9 @@ export async function DELETE(request: NextRequest) { if (IS_DESKTOP_BUILD && request.nextUrl.searchParams.get('all') === 'true') { try { await deleteAllUserCredentials(user); - return NextResponse.json({ ok: true }, { headers: NO_STORE }); + const answer = NextResponse.json({ ok: true }, { headers: NO_STORE }); + clearWipeProof(answer, user); + return answer; } catch (error) { const unavailable = storedDataUnavailableResponse(error, NO_STORE); if (unavailable) return unavailable; diff --git a/lib/auth/crypto.ts b/lib/auth/crypto.ts index a16fd7958..102c4bd1b 100644 --- a/lib/auth/crypto.ts +++ b/lib/auth/crypto.ts @@ -123,7 +123,9 @@ export type PayloadPurpose = /** A parked mobile hand-off sign-in in the shared store (lib/auth/handoff-store.ts). */ | 'handoff-store' /** Which server a desktop account's refresh token belongs to (lib/oauth/custom-upstream.ts). */ - | 'custom-upstream'; + | 'custom-upstream' + /** Proof of an earlier session, for wiping a dropped account's plugin secrets (lib/plugin-secrets/wipe-proof.ts). */ + | 'plugin-wipe-proof'; /** * Whether a blob minted before purposes were bound may be re-admitted for @@ -148,6 +150,7 @@ const LEGACY_PAYLOAD_MARKERS: Record false, 'handoff-store': () => false, 'custom-upstream': () => false, + 'plugin-wipe-proof': () => false, }; export function encryptPayload(payload: Record, purpose: PayloadPurpose): string { diff --git a/lib/offline/evicted-secrets.ts b/lib/offline/evicted-secrets.ts index 490992645..4929a7f34 100644 --- a/lib/offline/evicted-secrets.ts +++ b/lib/offline/evicted-secrets.ts @@ -1,8 +1,9 @@ /** * Plugin secrets of an account the app dropped, in the desktop app, where they sit - * on this device's own server. The account has no cookie left to ask with, so the - * route takes who it was, from what was written down at the time. On an instance - * they aren't the device's to remove, and nothing is written down. + * on this device's own server. The account has no session cookie left to ask with, so + * the route takes who it was, from what was written down at the time, with the proof + * the account got while it was signed in. On an instance they aren't the device's to + * remove, and nothing is written down. */ import { apiFetch } from '@/lib/browser-navigation'; diff --git a/lib/plugin-secrets/wipe-proof.ts b/lib/plugin-secrets/wipe-proof.ts new file mode 100644 index 000000000..cb8a32feb --- /dev/null +++ b/lib/plugin-secrets/wipe-proof.ts @@ -0,0 +1,42 @@ +/** + * Proof that this browser had a session for an account, for the desktop app. + * + * An account the app dropped has no session cookie left, and its plugin secrets + * still sit on this device until a wipe. So when a signed-in account stores or lists + * them, it also gets this sealed cookie, and a wipe by name needs the one for that + * name. Nobody can then clear an account's secrets by knowing its address. + * + * It opens nothing else, and only the desktop build writes or reads it. + */ + +import type { NextRequest, NextResponse } from 'next/server'; +import { decryptPayload, encryptPayload } from '@/lib/auth/crypto'; +import { getCookieOptions } from '@/lib/oauth/cookie-config'; +import { userKey, type VaultUser } from './vault'; + +/** Chromium's upper limit. A dropped account can stay dropped for a long time. */ +const MAX_AGE_SECONDS = 400 * 24 * 60 * 60; + +function cookieName(key: string): string { + return `jmap_psp_${key.slice(0, 16)}`; +} + +export function setWipeProof(response: NextResponse, user: VaultUser): void { + const key = userKey(user); + response.cookies.set(cookieName(key), encryptPayload({ u: key }, 'plugin-wipe-proof'), { ...getCookieOptions(), maxAge: MAX_AGE_SECONDS }); +} + +export function clearWipeProof(response: NextResponse, user: VaultUser): void { + response.cookies.set(cookieName(userKey(user)), '', { ...getCookieOptions(), maxAge: 0 }); +} + +export function hasWipeProof(request: NextRequest, user: VaultUser): boolean { + const key = userKey(user); + const value = request.cookies.get(cookieName(key))?.value; + if (!value) return false; + try { + return decryptPayload(value, 'plugin-wipe-proof')?.u === key; + } catch { + return false; + } +} From 466fa7f97463bb9514dff721ec63fa4b455e1419 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Wed, 4 Nov 2026 03:15:42 +0100 Subject: [PATCH 3/3] fix: keep the desktop app's session secret and admin password out of its server's environment --- lib/admin/password.ts | 3 +- lib/auth/session-secret.ts | 4 ++ lib/desktop/__tests__/server-secrets.test.ts | 57 ++++++++++++++++++++ lib/desktop/__tests__/trust-flag.test.ts | 14 +++++ lib/desktop/server-secrets.ts | 20 +++++++ 5 files changed, 97 insertions(+), 1 deletion(-) create mode 100644 lib/desktop/__tests__/server-secrets.test.ts create mode 100644 lib/desktop/server-secrets.ts diff --git a/lib/admin/password.ts b/lib/admin/password.ts index bae73d0b5..a2bfdce13 100644 --- a/lib/admin/password.ts +++ b/lib/admin/password.ts @@ -6,6 +6,7 @@ import { assertWritable, isConfigReadOnly } from './paths'; import { adminConfigDocs, adminStateDocs, isStoreUnavailable } from './admin-store'; import type { AdminConfigData, AdminStateData } from './types'; import { readFileEnv } from '../read-file-env'; +import { desktopServerSecret } from '@/lib/desktop/server-secrets'; const SCRYPT_KEYLEN = 64; const SCRYPT_COST = 16384; // 2^14 @@ -269,7 +270,7 @@ export async function initAdminPassword(): Promise { return true; } - const envPassword = process.env.ADMIN_PASSWORD; + const envPassword = desktopServerSecret('adminPassword') || process.env.ADMIN_PASSWORD; if (!envPassword) { if (!cache.initialized) logger.info('Admin dashboard disabled (no ADMIN_PASSWORD set)'); cache.initialized = true; diff --git a/lib/auth/session-secret.ts b/lib/auth/session-secret.ts index cd3b6cee7..d8faeea3c 100644 --- a/lib/auth/session-secret.ts +++ b/lib/auth/session-secret.ts @@ -1,6 +1,7 @@ import { randomBytes } from 'node:crypto'; import { writeFile } from 'node:fs/promises'; import { configManager } from '@/lib/admin/config-manager'; +import { desktopServerSecret } from '@/lib/desktop/server-secrets'; import { ensureStateDir, getStatePath } from '@/lib/admin/paths'; import { readFileEnv } from '@/lib/read-file-env'; import { getStore, isSharedMode } from '@/lib/shared-store'; @@ -25,6 +26,9 @@ function sharedSecret(): string { /** A secret the operator set: env, file, or the setup wizard's config. */ function configuredSessionSecret(): string { + const fromShell = desktopServerSecret('sessionSecret'); + if (fromShell) return fromShell; + const fromEnv = process.env.SESSION_SECRET; if (fromEnv) return fromEnv; diff --git a/lib/desktop/__tests__/server-secrets.test.ts b/lib/desktop/__tests__/server-secrets.test.ts new file mode 100644 index 000000000..ff7016879 --- /dev/null +++ b/lib/desktop/__tests__/server-secrets.test.ts @@ -0,0 +1,57 @@ +// @vitest-environment node +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +/** + * The desktop app's server gets its secrets by message, so they stay out of its + * environment block. No other build looks there. + */ + +vi.mock('@/lib/admin/config-manager', () => ({ + configManager: { get: (_key: string, fallback: unknown) => fallback }, +})); +vi.mock('@/lib/shared-store', () => ({ isSharedMode: () => false, getStore: () => ({}) })); +vi.mock('@/lib/admin/paths', () => ({ ensureStateDir: async () => {}, getStatePath: () => 'C:/nowhere/.session-secret' })); + +const KEY = Symbol.for('bulwark.desktop.serverSecrets'); +const holder = globalThis as unknown as Record; +const FROM_SHELL = 'from-the-shell-from-the-shell-from-the-shell'; + +async function sessionSecret() { + vi.resetModules(); + return (await import('@/lib/auth/session-secret')).getSessionSecret(); +} + +beforeEach(() => { + holder[KEY] = { sessionSecret: FROM_SHELL, adminPassword: 'admin-from-the-shell' }; +}); + +afterEach(() => { + vi.unstubAllEnvs(); + delete holder[KEY]; +}); + +describe('the session secret', () => { + it('comes from the shell in the desktop build, with nothing in the environment', async () => { + vi.stubEnv('NEXT_PUBLIC_BULWARK_DESKTOP', '1'); + vi.stubEnv('SESSION_SECRET', ''); + expect(await sessionSecret()).toBe(FROM_SHELL); + }); + + it('never from there in a server build', async () => { + vi.stubEnv('SESSION_SECRET', ''); + expect(await sessionSecret()).toBe(''); + vi.stubEnv('SESSION_SECRET', 'from-the-environment'); + expect(await sessionSecret()).toBe('from-the-environment'); + }); +}); + +describe('the admin password', () => { + it('is only readable from the shell in the desktop build', async () => { + const { desktopServerSecret } = await import('../server-secrets'); + expect(desktopServerSecret('adminPassword')).toBe(''); + vi.stubEnv('NEXT_PUBLIC_BULWARK_DESKTOP', '1'); + expect(desktopServerSecret('adminPassword')).toBe('admin-from-the-shell'); + delete holder[KEY]; + expect(desktopServerSecret('adminPassword')).toBe(''); + }); +}); diff --git a/lib/desktop/__tests__/trust-flag.test.ts b/lib/desktop/__tests__/trust-flag.test.ts index 0edd41532..ad56705eb 100644 --- a/lib/desktop/__tests__/trust-flag.test.ts +++ b/lib/desktop/__tests__/trust-flag.test.ts @@ -134,6 +134,20 @@ describe('the flag is fixed at build time', () => { expect(await trustCheckOfBuild('1')).toContain(DESKTOP_TRUST_MARKER); }); + it('the secrets from the shell are read in the desktop build only', async () => { + const compile = async (flag: string) => (await build({ + entryPoints: [path.join(ROOT, 'lib/desktop/server-secrets.ts')], + bundle: true, + write: false, + minify: true, + format: 'esm', + platform: 'node', + define: { 'process.env.NEXT_PUBLIC_BULWARK_DESKTOP': JSON.stringify(flag) }, + })).outputFiles[0].text; + expect(await compile('')).not.toContain('bulwark.desktop.serverSecrets'); + expect(await compile('1')).toContain('bulwark.desktop.serverSecrets'); + }); + it('no image build sets it', () => { for (const file of ['Dockerfile', 'Dockerfile.lite']) { expect(readFileSync(path.join(ROOT, file), 'utf8')).not.toContain('NEXT_PUBLIC_BULWARK_DESKTOP'); diff --git a/lib/desktop/server-secrets.ts b/lib/desktop/server-secrets.ts new file mode 100644 index 000000000..ee8e76189 --- /dev/null +++ b/lib/desktop/server-secrets.ts @@ -0,0 +1,20 @@ +/** + * The session secret and local admin password of the desktop app's own server. + * + * The Electron main process sends them by message, and src/server/host.ts in the + * desktop repo puts them on globalThis, which all of Next's bundles share. They stay + * out of the environment block, where a child process would inherit them. + * + * Every other build reads its secrets the usual way, and this compiles to nothing. + */ + +interface ShellSecrets { + sessionSecret?: string; + adminPassword?: string; +} + +export function desktopServerSecret(name: keyof ShellSecrets): string { + if (process.env.NEXT_PUBLIC_BULWARK_DESKTOP !== '1') return ''; + const holder = globalThis as unknown as Record; + return holder[Symbol.for('bulwark.desktop.serverSecrets')]?.[name] ?? ''; +}