From d32c8c373060d5d8aa3f2558e7c1510e0901668e Mon Sep 17 00:00:00 2001 From: Claude Fable 5 Date: Thu, 30 Jul 2026 09:34:44 +0200 Subject: [PATCH] #189: make the CSRF origin check fail closed A cookie-carrying mutation without Origin and Referer (or with an unparsable one) is now rejected with 403 csrf_origin_mismatch instead of passing unchecked. The exception for non-browser clients stays structural: PAT/bearer requests carry no session cookie and never reach the check, and a request that does carry the cookie is always checked. The test harness injects the matching Origin (supertest simulates a browser page of this instance) with an explicit suppression header for the negative cases; the Playwright fixture contexts send the header on their manual seeding calls; release-qa.sh pins APP_BASE_URL and sends the matching Origin. Dedicated spec covers: missing headers 403, mismatch 403, unparsable 403, match passes, GETs untouched, PAT mutation without headers passes, cookie+bearer still checked. Refs #189 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_0168Ph5uBmHm8X28CSVpbpnJ --- apps/api/src/auth/auth.guard.ts | 22 +++- apps/api/src/auth/csrf.e2e.db.test.ts | 178 ++++++++++++++++++++++++++ apps/api/src/testing/test-app.ts | 24 ++++ apps/web/e2e/helpers.ts | 9 +- deploy/release-qa.sh | 7 +- docs/architecture/security.md | 7 + docs/vs-nfd/20-massnahmenplan.md | 2 +- 7 files changed, 241 insertions(+), 8 deletions(-) create mode 100644 apps/api/src/auth/csrf.e2e.db.test.ts diff --git a/apps/api/src/auth/auth.guard.ts b/apps/api/src/auth/auth.guard.ts index 3f850e1..7c84c20 100644 --- a/apps/api/src/auth/auth.guard.ts +++ b/apps/api/src/auth/auth.guard.ts @@ -105,13 +105,27 @@ export class AuthGuard implements CanActivate { return true; } + /** + * Fail closed (#189): a cookie-carrying mutation must prove its origin — + * browsers always send `Origin` on cross- and same-origin mutations, so a + * missing header means "not a browser page of ours" and is rejected like a + * mismatch. Non-browser clients (curl, scripts) either send a matching + * `Origin` explicitly or authenticate with a PAT/bearer token and no + * cookie, which never reaches this check — the exception for them is + * structural (bound to the cookie), never a header loophole. + */ private assertSameOrigin(request: Request): void { const origin = request.headers.origin ?? request.headers.referer; - // Non-browser clients (curl, supertest) send neither header; SameSite - // cookies already stop cross-site browser requests without Origin. - if (!origin) return; + if (!origin) throw new ForbiddenException({ code: 'csrf_origin_mismatch' }); const expected = new URL(this.config.env.APP_BASE_URL).origin; - if (new URL(origin).origin !== expected) { + let actual: string; + try { + actual = new URL(origin).origin; + } catch { + // An unparsable Origin/Referer is a broken or hostile client, not ours. + throw new ForbiddenException({ code: 'csrf_origin_mismatch' }); + } + if (actual !== expected) { throw new ForbiddenException({ code: 'csrf_origin_mismatch' }); } } diff --git a/apps/api/src/auth/csrf.e2e.db.test.ts b/apps/api/src/auth/csrf.e2e.db.test.ts new file mode 100644 index 0000000..ec405e7 --- /dev/null +++ b/apps/api/src/auth/csrf.e2e.db.test.ts @@ -0,0 +1,178 @@ +import { INestApplication } from '@nestjs/common'; +import { PrismaClient } from '@prisma/client'; +import request from 'supertest'; +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; + +import { InstanceSettingsService } from '../settings/instance-settings.service'; +import { SUPPRESS_ORIGIN_HEADER, createTestApp, sessionCookieOf } from '../testing/test-app'; +import { createTestPrisma, hasTestDb, uniqueSuffix } from '../testing/test-db'; +import { UsersService } from '../users/users.service'; + +/** + * CSRF origin check, fail closed (issue #189): a cookie-carrying mutation + * without `Origin` and `Referer` is rejected exactly like a mismatch, and + * the exception for non-browser clients is structural — PAT/bearer requests + * carry no cookie and never reach the check. Cookie-authenticated requests + * never benefit from any header-based bypass. + */ +describe.skipIf(!hasTestDb)('csrf origin check (e2e, issue #189)', () => { + let app: INestApplication; + let prisma: PrismaClient; + const suffix = uniqueSuffix(); + const password = 'csrf fail closed pass 1'; + const ids: Record = {}; + const cookies: Record = {}; + let pondId: string; + let pondSlug: string; + let patToken: string; + + const api = () => request(app.getHttpServer()); + + async function makeUser(handle: string): Promise { + const users = app.get(UsersService); + const username = `csrf-${handle}-${suffix}`; + const user = await users.createUser({ + username, + email: `${username}@example.org`, + displayName: `Csrf ${handle}`, + password, + locale: 'en', + }); + await users.markEmailVerified(user.id); + ids[handle] = user.id; + cookies[handle] = sessionCookieOf( + await api() + .post('/api/v1/auth/login') + .send({ usernameOrEmail: username, password }) + .expect(200), + ); + } + + beforeAll(async () => { + prisma = createTestPrisma(); + await prisma.rateLimit.deleteMany({}); + app = await createTestApp(); + for (const handle of ['owner', 'siteadmin']) { + await makeUser(handle); + } + await prisma.user.update({ where: { id: ids.siteadmin! }, data: { isSiteAdmin: true } }); + // Per-user quota override, never the instance default (shared database). + await api() + .put(`/api/v1/admin/quotas/user/${ids.owner!}/additional_ponds`) + .set('Cookie', cookies.siteadmin!) + .send({ value: 5 }) + .expect(200); + + // A pond opted into the public API, and a write-scope PAT for it. + const pond = await api() + .post('/api/v1/ponds') + .set('Cookie', cookies.owner!) + .send({ name: `CSRF Pond ${suffix}` }) + .expect(201); + pondId = pond.body.id; + pondSlug = pond.body.slug; + await app.get(InstanceSettingsService).set('api.enabled', true, ids.siteadmin!); + await api() + .patch(`/api/v1/ponds/${pondId}`) + .set('Cookie', cookies.owner!) + .send({ apiEnabled: true }) + .expect(200); + const pat = await api() + .post('/api/v1/users/me/api-tokens') + .set('Cookie', cookies.owner!) + .send({ name: 'csrf-write', scope: 'write' }) + .expect(201); + patToken = pat.body.token; + }); + + afterAll(async () => { + const all = Object.values(ids); + await prisma.instanceSetting.deleteMany({ where: { key: 'api.enabled' } }); + await prisma.quotaOverride.deleteMany({ where: { subjectId: { in: all } } }); + await prisma.auditEntry.deleteMany({ where: { actorId: { in: all } } }); + await prisma.apiToken.deleteMany({ where: { userId: { in: all } } }); + const ponds = await prisma.pond.findMany({ + where: { ownerId: { in: all } }, + select: { id: true }, + }); + const pondIds = ponds.map((p) => p.id); + await prisma.pageVersion.deleteMany({ where: { page: { pondId: { in: pondIds } } } }); + await prisma.page.deleteMany({ where: { pondId: { in: pondIds } } }); + await prisma.roleGrant.deleteMany({ where: { pondId: { in: pondIds } } }); + await prisma.pondUsage.deleteMany({ where: { pondId: { in: pondIds } } }); + await prisma.pond.deleteMany({ where: { id: { in: pondIds } } }); + await prisma.session.deleteMany({ where: { userId: { in: all } } }); + await prisma.userIdentity.deleteMany({ where: { userId: { in: all } } }); + await prisma.rateLimit.deleteMany({}); + await prisma.user.deleteMany({ where: { id: { in: all } } }); + await prisma.$disconnect(); + await app.close(); + }); + + it('rejects a cookie mutation that sends neither Origin nor Referer', async () => { + const res = await api() + .patch(`/api/v1/ponds/${pondId}`) + .set('Cookie', cookies.owner!) + .set(SUPPRESS_ORIGIN_HEADER, '1') + .send({ name: `CSRF Pond ${suffix}` }) + .expect(403); + expect(res.body.code).toBe('csrf_origin_mismatch'); + }); + + it('rejects a cookie mutation from a mismatching origin (kept behaviour)', async () => { + const res = await api() + .patch(`/api/v1/ponds/${pondId}`) + .set('Cookie', cookies.owner!) + .set('Origin', 'https://evil.example') + .send({ name: `CSRF Pond ${suffix}` }) + .expect(403); + expect(res.body.code).toBe('csrf_origin_mismatch'); + }); + + it('rejects a cookie mutation with an unparsable Origin instead of erroring', async () => { + const res = await api() + .patch(`/api/v1/ponds/${pondId}`) + .set('Cookie', cookies.owner!) + .set('Origin', 'not a url') + .send({ name: `CSRF Pond ${suffix}` }) + .expect(403); + expect(res.body.code).toBe('csrf_origin_mismatch'); + }); + + it('accepts a cookie mutation from the matching origin', async () => { + await api() + .patch(`/api/v1/ponds/${pondId}`) + .set('Cookie', cookies.owner!) + .send({ name: `CSRF Pond ${suffix}` }) + .expect(200); + }); + + it('leaves cookie reads untouched — the check binds to mutations', async () => { + await api() + .get('/api/v1/auth/me') + .set('Cookie', cookies.owner!) + .set(SUPPRESS_ORIGIN_HEADER, '1') + .expect(200); + }); + + it('lets a PAT mutation through without either header — no cookie, no check', async () => { + await api() + .post(`/api/public/v1/ponds/${pondSlug}/pages`) + .set('Authorization', `Bearer ${patToken}`) + .set(SUPPRESS_ORIGIN_HEADER, '1') + .send({ title: `CSRF PAT page ${suffix}` }) + .expect(201); + }); + + it('enforces the check when a request carries both cookie and bearer token', async () => { + // Cookie-authenticated requests never benefit from the bearer exception. + const res = await api() + .patch(`/api/v1/ponds/${pondId}`) + .set('Cookie', cookies.owner!) + .set('Authorization', `Bearer ${patToken}`) + .set(SUPPRESS_ORIGIN_HEADER, '1') + .send({ name: `CSRF Pond ${suffix}` }) + .expect(403); + expect(res.body.code).toBe('csrf_origin_mismatch'); + }); +}); diff --git a/apps/api/src/testing/test-app.ts b/apps/api/src/testing/test-app.ts index 71c219f..76091e5 100644 --- a/apps/api/src/testing/test-app.ts +++ b/apps/api/src/testing/test-app.ts @@ -6,8 +6,17 @@ import { INestApplication } from '@nestjs/common'; import { Test, TestingModuleBuilder } from '@nestjs/testing'; import type { NestExpressApplication } from '@nestjs/platform-express'; import cookieParser from 'cookie-parser'; +import type { NextFunction, Request, Response } from 'express'; import { AppModule } from '../app.module'; +import { AppConfig } from '../config/app-config.service'; + +/** + * Suppresses the test harness's automatic `Origin` header (below) for a + * single request — the way the CSRF spec simulates a client that sends + * neither `Origin` nor `Referer` (issue #189). + */ +export const SUPPRESS_ORIGIN_HEADER = 'x-test-suppress-origin'; /** * Boots the full application for e2e tests, mirroring main.ts middleware. @@ -32,6 +41,21 @@ export async function createTestApp( const moduleRef = await (customize ? customize(base) : base).compile(); const app = moduleRef.createNestApplication(); app.use(cookieParser()); + // Browsers always send `Origin` on mutations, and since #189 the CSRF + // check fails closed without it. supertest simulates a browser page of + // this instance, so the harness injects the matching header — absence is + // simulated explicitly via SUPPRESS_ORIGIN_HEADER, never by accident. + const expectedOrigin = new URL(moduleRef.get(AppConfig).env.APP_BASE_URL).origin; + app.use((req: Request, _res: Response, next: NextFunction) => { + if (SUPPRESS_ORIGIN_HEADER in req.headers) { + delete req.headers[SUPPRESS_ORIGIN_HEADER]; + delete req.headers.origin; + delete req.headers.referer; + } else { + req.headers.origin ??= expectedOrigin; + } + next(); + }); // Mirrors main.ts: base64 Yjs page state needs more than Express's 100kb default. app.useBodyParser('json', { limit: '8mb' }); // Mirrors main.ts: the public API (issue #104) declares its full path. diff --git a/apps/web/e2e/helpers.ts b/apps/web/e2e/helpers.ts index 02f2793..280d539 100644 --- a/apps/web/e2e/helpers.ts +++ b/apps/web/e2e/helpers.ts @@ -22,7 +22,14 @@ export async function contextForUser( } const storageState = await api.storageState(); await api.dispose(); - return browser.newContext({ baseURL, storageState }); + // The CSRF check fails closed (#189): cookie mutations must carry a + // matching Origin. The browser sends it on its own fetches; this default + // covers the specs' manual `context.request.*` seeding calls too. + return browser.newContext({ + baseURL, + storageState, + extraHTTPHeaders: { origin: new URL(baseURL).origin }, + }); } /** Latest mail for an address from the Mailpit REST api. */ diff --git a/deploy/release-qa.sh b/deploy/release-qa.sh index a73367b..c11f460 100755 --- a/deploy/release-qa.sh +++ b/deploy/release-qa.sh @@ -33,6 +33,9 @@ NET="$P-net" PGPASS="rqa-$(date +%s)" ADMIN_PASS="release qa admin pass 1" SESSION="" +# Cookie mutations must prove their origin since #189 (CSRF fail-closed). +# The api pins APP_BASE_URL to this value and every api_curl sends it. +QA_ORIGIN="http://release-qa.local" log() { echo "release-qa: $*"; } fail() { echo "release-qa: FAILED — $*" >&2; exit 1; } @@ -50,7 +53,7 @@ trap cleanup EXIT api_curl() { # api_curl [json-body] METHOD=$1; APIPATH=$2; BODY=${3:-} docker run --rm --network "$NET" curlimages/curl:8.10.1 \ - -s ${SESSION:+-H "Cookie: $SESSION"} -X "$METHOD" \ + -s ${SESSION:+-H "Cookie: $SESSION"} -H "Origin: $QA_ORIGIN" -X "$METHOD" \ ${BODY:+-H 'Content-Type: application/json' --data "$BODY"} \ "http://$P-api:3000/api/v1$APIPATH" } @@ -58,7 +61,7 @@ api_curl() { # api_curl [json-body] start_api() { # start_api docker rm -f "$P-api" >/dev/null 2>&1 || true docker run -d --name "$P-api" --network "$NET" \ - -e DATABASE_URL="$1" \ + -e DATABASE_URL="$1" -e APP_BASE_URL="$QA_ORIGIN" \ -e SETUP_ADMIN_USERNAME=qa-admin -e SETUP_ADMIN_EMAIL=qa-admin@example.org \ -e SETUP_ADMIN_PASSWORD="$ADMIN_PASS" \ -v "$P-uploads":/data/uploads -v "$P-plugins":/data/plugins -v "$P-backups":/data/backups:ro \ diff --git a/docs/architecture/security.md b/docs/architecture/security.md index fe32def..d616dba 100644 --- a/docs/architecture/security.md +++ b/docs/architecture/security.md @@ -15,6 +15,13 @@ or sloppy plugin authors, compromised dependencies. Secure, SameSite=Lax cookies; CSRF protected by SameSite + origin checks on mutating requests (double-submit token for the file-download edge cases). +- The origin check **fails closed** (issue #189): a cookie-carrying + mutation without `Origin` and `Referer` (or with an unparsable one) is + rejected with `403 csrf_origin_mismatch`. Non-browser clients + authenticate with a PAT/bearer token and no cookie, which never reaches + the check — the exception is structural, not a header loophole; a + request that does carry the session cookie is always checked. Scripted + cookie clients must send `Origin: `. - E-mail verification (double opt-in) before an account can create content; password reset via single-use hashed tokens; both rate-limited. - Rate limiting (DB-backed) on login, signup, reset, and API; lockout diff --git a/docs/vs-nfd/20-massnahmenplan.md b/docs/vs-nfd/20-massnahmenplan.md index 3c08fcc..62e360d 100644 --- a/docs/vs-nfd/20-massnahmenplan.md +++ b/docs/vs-nfd/20-massnahmenplan.md @@ -91,7 +91,7 @@ chain`_ (`packages/shared/src/token-crypto.ts`). Einzeln wären es 5–6 AT._ Achtung: Unsubscribe-Tokens leben lang in versandten Mails → Dual-Verify-Fenster einplanen. -- [ ] **CSRF fail-closed** — fehlendes Origin _und_ Referer wird derzeit +- [x] **CSRF fail-closed** — fehlendes Origin _und_ Referer wird derzeit durchgelassen · 1 AT · #189 - [ ] **Session-Timeout konfigurierbar**, Default deutlich unter 30 Tagen, separates Idle-Timeout · 1–2 AT · #190