mirror of
https://github.com/mauriceboe/TREK.git
synced 2026-06-19 13:21:46 +00:00
security: address second-pass audit findings
- 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.
This commit is contained in:
+7
-3
@@ -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(
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -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<string, string> = {}): Request {
|
||||
return { method, path: '/api/test', headers } as unknown as Request;
|
||||
function makeReq(method = 'POST', headers: Record<string, string> = {}, 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<typeof vi.fn>)({ 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<typeof vi.fn>)({ 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', () => {
|
||||
|
||||
Reference in New Issue
Block a user