From 9f57ab4517e2edb5629e787b50d9da76c3fbf641 Mon Sep 17 00:00:00 2001 From: Maurice Date: Mon, 20 Apr 2026 21:04:09 +0200 Subject: [PATCH] security: address second-pass audit findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - CI-C1 false positive: actions/{checkout,setup-node,upload-artifact} @v6 do exist (v6.0.0 releases published Oct-Dec 2025). Restore the @v6 refs — the earlier batch-1 commit downgraded them unnecessarily. - Widen idempotency_keys primary key to (key, user_id, method, path) via new migration. Batch 1 widened the middleware lookup but left the table PK at (key, user_id), so `INSERT OR IGNORE` silently skipped the second endpoint that reused a key — the cache was never populated for it and a replay re-ran the handler. The migration rebuilds the table preserving existing rows (the old narrower PK guarantees no conflicts against the new looser key). - HSTS: keep `includeSubDomains` OFF by default. Enabling it for every NODE_ENV=production install would break apex-domain setups where siblings still serve HTTP. Operators who want the stricter policy opt in with HSTS_INCLUDE_SUBDOMAINS=true. - Extend the idempotency unit tests to cover the (method, path) dimension — same user+key on different path no longer replays. --- .github/workflows/test.yml | 12 ++--- server/src/app.ts | 10 ++-- server/src/db/migrations.ts | 30 +++++++++++ .../tests/unit/middleware/idempotency.test.ts | 53 ++++++++++++++----- 4 files changed, 83 insertions(+), 22 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index d2dffc65..6c11b481 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -17,9 +17,9 @@ jobs: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v6 - - uses: actions/setup-node@v4 + - uses: actions/setup-node@v6 with: node-version: 22 cache: npm @@ -33,7 +33,7 @@ jobs: - name: Upload coverage if: success() - uses: actions/upload-artifact@v4 + uses: actions/upload-artifact@v6 with: name: backend-coverage path: server/coverage/ @@ -44,9 +44,9 @@ jobs: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v6 - - uses: actions/setup-node@v4 + - uses: actions/setup-node@v6 with: node-version: 22 cache: npm @@ -60,7 +60,7 @@ jobs: - name: Upload coverage if: success() - uses: actions/upload-artifact@v4 + uses: actions/upload-artifact@v6 with: name: frontend-coverage path: client/coverage/ diff --git a/server/src/app.ts b/server/src/app.ts index 655f3424..bae6a820 100644 --- a/server/src/app.ts +++ b/server/src/app.ts @@ -79,10 +79,14 @@ export function createApp(): express.Application { // Caddy / Cloudflare Tunnel typically leave FORCE_HTTPS unset (the // proxy handles the redirect for them), and the previous "HSTS off by // default" meant those instances never advertised HSTS at all. - // `HSTS_INCLUDE_SUBDOMAINS=false` lets operators with sibling - // subdomains on the same apex opt back out. + // + // `includeSubDomains` stays OFF by default on purpose: an instance + // running on an apex domain would otherwise force HTTPS on every + // sibling subdomain the same operator may still be running over plain + // HTTP. Operators who want the stricter policy opt in with + // `HSTS_INCLUDE_SUBDOMAINS=true`. const hstsActive = shouldForceHttps || process.env.NODE_ENV === 'production'; - const hstsIncludeSubdomains = process.env.HSTS_INCLUDE_SUBDOMAINS !== 'false'; + const hstsIncludeSubdomains = process.env.HSTS_INCLUDE_SUBDOMAINS === 'true'; // RFC 8414 / RFC 9728: discovery docs are world-readable — open CORS regardless of deployment config app.use( diff --git a/server/src/db/migrations.ts b/server/src/db/migrations.ts index fabc58dc..4994ec20 100644 --- a/server/src/db/migrations.ts +++ b/server/src/db/migrations.ts @@ -1837,6 +1837,36 @@ function runMigrations(db: Database.Database): void { } } catch { /* notifications table may not exist on very old installs */ } }, + // Migration: widen idempotency_keys primary key to (key, user_id, + // method, path). The middleware lookup was widened in the same audit + // batch so a reused X-Idempotency-Key against a different endpoint + // does not replay the cached body of an unrelated request. The old + // PK was only (key, user_id), so the `INSERT OR IGNORE` on the + // second endpoint silently skipped — the cache never stored request + // B's response and replays re-executed the handler. Rebuild the + // table with the widened PK, preserving existing rows (the old PK + // guarantees no conflicts in the new, strictly looser unique key). + () => { + const hasTable = db.prepare("SELECT name FROM sqlite_master WHERE type = 'table' AND name = 'idempotency_keys'").get(); + if (!hasTable) return; + db.exec(` + CREATE TABLE idempotency_keys_new ( + key TEXT NOT NULL, + user_id INTEGER NOT NULL REFERENCES users(id) ON DELETE CASCADE, + method TEXT NOT NULL, + path TEXT NOT NULL, + status_code INTEGER NOT NULL, + response_body TEXT NOT NULL, + created_at INTEGER NOT NULL DEFAULT (strftime('%s','now')), + PRIMARY KEY (key, user_id, method, path) + ); + INSERT INTO idempotency_keys_new (key, user_id, method, path, status_code, response_body, created_at) + SELECT key, user_id, method, path, status_code, response_body, created_at FROM idempotency_keys; + DROP TABLE idempotency_keys; + ALTER TABLE idempotency_keys_new RENAME TO idempotency_keys; + CREATE INDEX IF NOT EXISTS idx_idempotency_keys_created ON idempotency_keys(created_at); + `); + }, ]; if (currentVersion < migrations.length) { diff --git a/server/tests/unit/middleware/idempotency.test.ts b/server/tests/unit/middleware/idempotency.test.ts index 237939d0..e45a9f17 100644 --- a/server/tests/unit/middleware/idempotency.test.ts +++ b/server/tests/unit/middleware/idempotency.test.ts @@ -8,12 +8,12 @@ const { rows, dbMock } = vi.hoisted(() => { db: { prepare: vi.fn((sql: string) => ({ get: vi.fn((...args: unknown[]) => { - const [key, userId] = args; - return rows[`${key}:${userId}`] ?? undefined; + const [key, userId, method, path] = args; + return rows[`${key}:${userId}:${method}:${path}`] ?? undefined; }), run: vi.fn((...args: unknown[]) => { - const [key, userId, , , status_code, response_body] = args as [string, number, string, string, number, string]; - const k = `${key}:${userId}`; + const [key, userId, method, path, status_code, response_body] = args as [string, number, string, string, number, string]; + const k = `${key}:${userId}:${method}:${path}`; if (!rows[k]) rows[k] = { status_code, response_body }; }), })), @@ -28,8 +28,8 @@ vi.mock('../../../src/db/database', () => dbMock); import { applyIdempotency } from '../../../src/middleware/idempotency'; import type { Request, Response, NextFunction } from 'express'; -function makeReq(method = 'POST', headers: Record = {}): Request { - return { method, path: '/api/test', headers } as unknown as Request; +function makeReq(method = 'POST', headers: Record = {}, path = '/api/test'): Request { + return { method, path, headers } as unknown as Request; } function makeRes(statusCode = 200): Response { @@ -64,8 +64,8 @@ describe('applyIdempotency', () => { expect(next).toHaveBeenCalledOnce(); }); - it('replays cached response when key+user already stored', () => { - rows['cached-key:42'] = { status_code: 201, response_body: JSON.stringify({ id: 99 }) }; + it('replays cached response when key+user+method+path already stored', () => { + rows['cached-key:42:POST:/api/test'] = { status_code: 201, response_body: JSON.stringify({ id: 99 }) }; const req = makeReq('POST', { 'x-idempotency-key': 'cached-key' }); const res = makeRes(); const next = vi.fn(); @@ -75,7 +75,7 @@ describe('applyIdempotency', () => { }); it('different user same key does NOT replay', () => { - rows['cached-key:1'] = { status_code: 200, response_body: JSON.stringify({ ok: true }) }; + rows['cached-key:1:POST:/api/test'] = { status_code: 200, response_body: JSON.stringify({ ok: true }) }; const req = makeReq('POST', { 'x-idempotency-key': 'cached-key' }); const res = makeRes(); const next = vi.fn(); @@ -83,6 +83,33 @@ describe('applyIdempotency', () => { expect(next).toHaveBeenCalledOnce(); }); + it('same key+user on different path does NOT replay (scoped cache)', () => { + // Key 'dual-key' is cached under /api/a but reused against /api/b. + // Without the (key, user_id, method, path) scoping, /api/b would + // have replayed /api/a's body — a silent cross-endpoint leak. + rows['dual-key:7:POST:/api/a'] = { status_code: 200, response_body: JSON.stringify({ from: 'a' }) }; + const req = makeReq('POST', { 'x-idempotency-key': 'dual-key' }, '/api/b'); + const res = makeRes(); + const next = vi.fn(() => { + (res.json as ReturnType)({ from: 'b' }); + }); + applyIdempotency(req, res, next, 7); + expect(next).toHaveBeenCalledOnce(); + expect(rows['dual-key:7:POST:/api/b']).toBeDefined(); + expect(JSON.parse(rows['dual-key:7:POST:/api/b'].response_body)).toEqual({ from: 'b' }); + // /api/a's row is untouched. + expect(JSON.parse(rows['dual-key:7:POST:/api/a'].response_body)).toEqual({ from: 'a' }); + }); + + it('same key+user+path but different method does NOT replay', () => { + rows['m-key:3:POST:/api/x'] = { status_code: 201, response_body: JSON.stringify({ m: 'post' }) }; + const req = makeReq('PATCH', { 'x-idempotency-key': 'm-key' }, '/api/x'); + const res = makeRes(); + const next = vi.fn(); + applyIdempotency(req, res, next, 3); + expect(next).toHaveBeenCalledOnce(); + }); + it('stores 2xx response on first execution via wrapped res.json', () => { const req = makeReq('POST', { 'x-idempotency-key': 'new-key' }); const res = makeRes(201); @@ -92,9 +119,9 @@ describe('applyIdempotency', () => { }); applyIdempotency(req, res, next, 7); expect(next).toHaveBeenCalledOnce(); - expect(rows['new-key:7']).toBeDefined(); - expect(rows['new-key:7'].status_code).toBe(201); - expect(JSON.parse(rows['new-key:7'].response_body)).toEqual({ id: 5 }); + expect(rows['new-key:7:POST:/api/test']).toBeDefined(); + expect(rows['new-key:7:POST:/api/test'].status_code).toBe(201); + expect(JSON.parse(rows['new-key:7:POST:/api/test'].response_body)).toEqual({ id: 5 }); }); it('does NOT store 4xx responses', () => { @@ -104,7 +131,7 @@ describe('applyIdempotency', () => { (res.json as ReturnType)({ error: 'Invalid' }); }); applyIdempotency(req, res, next, 3); - expect(rows['fail-key:3']).toBeUndefined(); + expect(rows['fail-key:3:POST:/api/test']).toBeUndefined(); }); it('handles PUT, PATCH, and DELETE the same as POST', () => {