diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d0d2cb6..03a7280 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -75,13 +75,16 @@ jobs: - name: Install frontend dependencies run: npm ci - # TEMPORARILY relaxed from --audit-level=high: react-router-dom is flagged - # by GHSA-qwww-vcr4-c8h2 (RSC-mode CSRF bypass) and no patched release - # exists yet. The app is not affected (classic BrowserRouter, no RSC or - # server actions), so failing every CI run over it would only mask real - # regressions. Restore --audit-level=high once Dependabot brings the fix. - - name: Audit frontend dependencies (production, critical severity) - run: npm audit --omit=dev --audit-level=critical + # High-severity gate with a SCOPED allowlist (scripts/audit-gate.mjs) + # instead of a blanket audit-level drop: each waived advisory carries its + # justification and an expiry date, and the gate fails when an entry + # expires or becomes stale — so a "temporary" exception can never + # silently become permanent, and any NEW high/critical advisory still + # fails the build. Currently waived: GHSA-qwww-vcr4-c8h2 (react-router + # RSC-mode CSRF bypass — app uses classic BrowserRouter, not affected; + # entry expires 2026-09-30). + - name: Audit frontend dependencies (production, high severity, allowlisted) + run: node scripts/audit-gate.mjs - name: Lint frontend run: npm run lint diff --git a/backend/src/services/twoFactor.service.js b/backend/src/services/twoFactor.service.js index b6c586b..122b0c5 100644 --- a/backend/src/services/twoFactor.service.js +++ b/backend/src/services/twoFactor.service.js @@ -44,15 +44,32 @@ const generateRecoveryCode = () => { // far more entropy than any bcrypt work factor meaningfully adds to). const hashRecoveryCode = (code) => hashOtp(code.toUpperCase()); -/** Mints RECOVERY_CODE_COUNT fresh codes and stores only their hashes. */ +/** + * Mints RECOVERY_CODE_COUNT fresh codes and stores only their hashes. + * Delete + insert run in one transaction (same pattern as + * replaceProjectPalette): a crash between the two must never leave a 2FA + * account with zero recovery codes — either the old set is still intact or + * the new one is fully in place. + */ const storeRecoveryCodes = async (userId) => { const codes = Array.from({ length: RECOVERY_CODE_COUNT }, generateRecoveryCode); - await db.query('DELETE FROM user_recovery_codes WHERE user_id = ?', [userId]); - await db.query( - `INSERT INTO user_recovery_codes (user_id, code_hash) VALUES ${codes.map(() => '(?, ?)').join(', ')}`, - codes.flatMap((code) => [userId, hashRecoveryCode(code)]), - ); - return codes; + let connection; + try { + connection = await db.getConnection(); + await connection.beginTransaction(); + await connection.query('DELETE FROM user_recovery_codes WHERE user_id = ?', [userId]); + await connection.query( + `INSERT INTO user_recovery_codes (user_id, code_hash) VALUES ${codes.map(() => '(?, ?)').join(', ')}`, + codes.flatMap((code) => [userId, hashRecoveryCode(code)]), + ); + await connection.commit(); + return codes; + } catch (error) { + if (connection) await connection.rollback(); + throw error; + } finally { + if (connection) connection.release(); + } }; // Starts enrollment: generates a fresh secret, stages it (not yet active) so diff --git a/backend/tests/unit/twoFactor.service.test.js b/backend/tests/unit/twoFactor.service.test.js index d6e190f..7b87d6c 100644 --- a/backend/tests/unit/twoFactor.service.test.js +++ b/backend/tests/unit/twoFactor.service.test.js @@ -21,6 +21,21 @@ jest.mock('../../src/services/user.service', () => ({ verifyUserIdentity: jest.fn(), })); +// The recovery-code rotation runs on a dedicated pooled connection inside a +// transaction; this wires db.getConnection to a fully-mocked connection whose +// queries all succeed, and returns it for per-test assertions/overrides. +const mockRecoveryCodesConnection = () => { + const connection = { + beginTransaction: jest.fn(), + commit: jest.fn(), + rollback: jest.fn(), + release: jest.fn(), + query: jest.fn().mockResolvedValue([{}]), + }; + db.getConnection.mockResolvedValue(connection); + return connection; +}; + describe('twoFactor service', () => { beforeEach(() => { jest.resetAllMocks(); @@ -62,12 +77,12 @@ describe('twoFactor service', () => { .mockResolvedValueOnce([ [{ email: 'axelle@example.com', totp_pending_secret_encrypted: encryptSecret(secret) }], ]) // lookup - .mockResolvedValueOnce([{}]) // activate UPDATE - .mockResolvedValueOnce([{}]) // DELETE old recovery codes - .mockResolvedValueOnce([{}]); // INSERT new recovery codes + .mockResolvedValueOnce([{}]); // activate UPDATE + const connection = mockRecoveryCodesConnection(); const result = await twoFactorService.confirmTotpSetup(1, code); + expect(connection.commit).toHaveBeenCalled(); expect(result.recoveryCodes).toHaveLength(twoFactorService.RECOVERY_CODE_COUNT); // Each code is unique and shaped like XXXXX-XXXXX-XXXXX-XXXXX. expect(new Set(result.recoveryCodes).size).toBe(twoFactorService.RECOVERY_CODE_COUNT); @@ -112,9 +127,8 @@ describe('twoFactor service', () => { .mockResolvedValueOnce([ [{ email: 'a@b.com', totp_pending_secret_encrypted: encryptSecret(secret) }], ]) - .mockResolvedValueOnce([{}]) - .mockResolvedValueOnce([{}]) .mockResolvedValueOnce([{}]); + mockRecoveryCodesConnection(); mailService.sendMail.mockRejectedValueOnce(new Error('smtp down')); const onMailError = jest.fn(); @@ -169,12 +183,10 @@ describe('twoFactor service', () => { describe('regenerateRecoveryCodes', () => { it('mints a fresh set after re-auth, wiping the previous codes first', async () => { - db.query - .mockResolvedValueOnce([ - [{ email: 'axelle@example.com', password: 'hashed', google_id: null, totp_enabled: 1 }], - ]) // lookup - .mockResolvedValueOnce([{}]) // DELETE old recovery codes - .mockResolvedValueOnce([{}]); // INSERT new recovery codes + db.query.mockResolvedValueOnce([ + [{ email: 'axelle@example.com', password: 'hashed', google_id: null, totp_enabled: 1 }], + ]); // lookup + const connection = mockRecoveryCodesConnection(); userService.verifyUserIdentity.mockResolvedValueOnce(undefined); const { recoveryCodes } = await twoFactorService.regenerateRecoveryCodes(1, { @@ -183,8 +195,12 @@ describe('twoFactor service', () => { expect(recoveryCodes).toHaveLength(twoFactorService.RECOVERY_CODE_COUNT); expect(new Set(recoveryCodes).size).toBe(twoFactorService.RECOVERY_CODE_COUNT); - expect(db.query.mock.calls[1][0]).toMatch(/DELETE FROM user_recovery_codes/); - expect(db.query.mock.calls[2][0]).toMatch(/INSERT INTO user_recovery_codes/); + // Delete-then-insert, inside one transaction. + expect(connection.beginTransaction).toHaveBeenCalled(); + expect(connection.query.mock.calls[0][0]).toMatch(/DELETE FROM user_recovery_codes/); + expect(connection.query.mock.calls[1][0]).toMatch(/INSERT INTO user_recovery_codes/); + expect(connection.commit).toHaveBeenCalled(); + expect(connection.release).toHaveBeenCalled(); expect(mailService.sendMail).toHaveBeenCalledWith( expect.objectContaining({ to: 'axelle@example.com', @@ -193,6 +209,28 @@ describe('twoFactor service', () => { ); }); + it('rolls back (keeping the old codes) when the insert fails mid-rotation', async () => { + db.query.mockResolvedValueOnce([ + [{ email: 'axelle@example.com', password: 'hashed', google_id: null, totp_enabled: 1 }], + ]); // lookup + const connection = mockRecoveryCodesConnection(); + connection.query + .mockReset() + .mockResolvedValueOnce([{}]) // DELETE succeeds + .mockRejectedValueOnce(new Error('db down')); // INSERT fails + userService.verifyUserIdentity.mockResolvedValueOnce(undefined); + + await expect( + twoFactorService.regenerateRecoveryCodes(1, { currentPassword: 'Password1' }), + ).rejects.toThrow('db down'); + + expect(connection.rollback).toHaveBeenCalled(); + expect(connection.commit).not.toHaveBeenCalled(); + expect(connection.release).toHaveBeenCalled(); + // No "regenerated" alert goes out for a rotation that didn't happen. + expect(mailService.sendMail).not.toHaveBeenCalled(); + }); + it('refuses when 2FA is not enabled', async () => { db.query.mockResolvedValueOnce([ [{ email: 'a@b.com', password: 'hashed', google_id: null, totp_enabled: 0 }], diff --git a/backend/tests/unit/user.controller.test.js b/backend/tests/unit/user.controller.test.js index d5117ff..4399ed8 100644 --- a/backend/tests/unit/user.controller.test.js +++ b/backend/tests/unit/user.controller.test.js @@ -22,6 +22,21 @@ jest.mock('../../src/database'); jest.mock('../../src/services/mail.service'); jest.mock('../../src/services/googleIdentity.service'); +// The recovery-code rotation runs on a dedicated pooled connection inside a +// transaction (see twoFactor.service.js); this stands in for it with a +// connection whose queries all succeed. +const mockRecoveryCodesConnection = () => { + const connection = { + beginTransaction: jest.fn(), + commit: jest.fn(), + rollback: jest.fn(), + release: jest.fn(), + query: jest.fn().mockResolvedValue([{}]), + }; + db.getConnection.mockResolvedValue(connection); + return connection; +}; + describe('user controller', () => { beforeEach(() => { jest.resetAllMocks(); @@ -383,9 +398,8 @@ describe('user controller', () => { .mockResolvedValueOnce([ [{ email: 'axelle@example.com', totp_pending_secret_encrypted: encryptSecret(secret) }], ]) - .mockResolvedValueOnce([{}]) - .mockResolvedValueOnce([{}]) - .mockResolvedValueOnce([{}]); + .mockResolvedValueOnce([{}]); // activate UPDATE + mockRecoveryCodesConnection(); // transactional delete+insert of the codes const req = { user: { id: 1 }, body: { code } }; const res = { json: jest.fn(), status: jest.fn().mockReturnThis() }; @@ -441,12 +455,10 @@ describe('user controller', () => { it('regenerateRecoveryCodes returns a fresh set after a correct current password', async () => { const hashedPassword = await bcrypt.hash('Password1', 4); - db.query - .mockResolvedValueOnce([ - [{ email: 'a@b.com', password: hashedPassword, google_id: null, totp_enabled: 1 }], - ]) - .mockResolvedValueOnce([{}]) // DELETE old codes - .mockResolvedValueOnce([{}]); // INSERT new codes + db.query.mockResolvedValueOnce([ + [{ email: 'a@b.com', password: hashedPassword, google_id: null, totp_enabled: 1 }], + ]); + mockRecoveryCodesConnection(); // transactional delete+insert of the codes const req = { user: { id: 1 }, body: { currentPassword: 'Password1' } }; const res = { json: jest.fn(), status: jest.fn().mockReturnThis() }; diff --git a/frontend/scripts/audit-gate.mjs b/frontend/scripts/audit-gate.mjs new file mode 100644 index 0000000..bc331e9 --- /dev/null +++ b/frontend/scripts/audit-gate.mjs @@ -0,0 +1,115 @@ +/** + * Dependency-audit gate for CI: enforces `npm audit --omit=dev` at HIGH + * severity, minus an explicit allowlist. Unlike lowering --audit-level (which + * waives every future advisory too), each waiver here is scoped to one GHSA + * id, carries its justification, and EXPIRES: past its date the gate fails + * again, so a "temporary" exception can never silently become permanent. + * + * Run: `node scripts/audit-gate.mjs` (used by .github/workflows/ci.yml). + */ +import { spawnSync } from 'node:child_process'; +import { pathToFileURL } from 'node:url'; + +export const ALLOWLIST = [ + { + id: 'GHSA-qwww-vcr4-c8h2', + reason: + 'react-router RSC-mode CSRF bypass — not exploitable here: classic BrowserRouter SPA, ' + + 'no React Server Components and no server actions. No patched release exists yet ' + + '(the only "fix" npm offers is downgrading to 7.11.0).', + // Re-evaluate by this date: check for a patched react-router release and + // either upgrade or consciously renew this entry. + expires: '2026-09-30', + }, +]; + +const GATED_SEVERITIES = new Set(['high', 'critical']); + +// Extracts the GHSA ids of every high/critical advisory from `npm audit +// --json` output (npm v10 shape: vulnerabilities..via[] where direct +// advisories are objects carrying url/severity, and transitive references are +// plain strings to be resolved through their own package entry). +export const collectGatedAdvisories = (auditReport) => { + const advisories = new Map(); // GHSA id -> { package, severity, title } + const vulnerabilities = auditReport?.vulnerabilities || {}; + for (const [pkg, info] of Object.entries(vulnerabilities)) { + for (const via of info?.via || []) { + if (typeof via !== 'object' || via === null) continue; + if (!GATED_SEVERITIES.has(via.severity)) continue; + const match = String(via.url || '').match(/GHSA-[a-z0-9-]+/i); + if (!match) continue; + const id = match[0]; + if (!advisories.has(id)) { + advisories.set(id, { package: pkg, severity: via.severity, title: via.title || '' }); + } + } + } + return advisories; +}; + +/** + * Pure decision logic (unit-tested): given the audit report, the allowlist + * and "now", returns { failures: string[] } — empty means the gate passes. + */ +export const evaluateAudit = (auditReport, allowlist, now = new Date()) => { + const failures = []; + const advisories = collectGatedAdvisories(auditReport); + const allowlistById = new Map(allowlist.map((entry) => [entry.id, entry])); + + for (const [id, advisory] of advisories) { + const waiver = allowlistById.get(id); + if (!waiver) { + failures.push( + `${advisory.severity.toUpperCase()} advisory ${id} on "${advisory.package}" is not allowlisted: ${advisory.title}`, + ); + } else if (now > new Date(`${waiver.expires}T23:59:59Z`)) { + failures.push( + `Allowlist entry ${id} EXPIRED on ${waiver.expires} — re-evaluate it (upgrade the package, or consciously renew the entry with a new date). Reason on file: ${waiver.reason}`, + ); + } + } + + // A waiver for an advisory npm no longer reports is stale: fail so the + // allowlist shrinks back instead of accumulating dead exceptions. + for (const entry of allowlist) { + if (!advisories.has(entry.id)) { + failures.push( + `Allowlist entry ${entry.id} no longer matches any reported advisory — the fix landed, remove the entry.`, + ); + } + } + + return { failures, advisories }; +}; + +const isMainModule = process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href; +if (isMainModule) { + // npm audit exits non-zero whenever it finds anything; the JSON on stdout is + // the real signal, so the exit code is ignored here. Single-string + shell + // (the command is a constant, nothing user-supplied) so the same line works + // on Windows (npm.cmd) and on the Linux CI runners. + const result = spawnSync('npm audit --omit=dev --json', { + encoding: 'utf8', + shell: true, + }); + let report; + try { + report = JSON.parse(result.stdout); + } catch { + console.error('audit-gate: could not parse `npm audit --json` output.'); + console.error(result.stderr || result.stdout); + process.exit(1); + } + + const { failures, advisories } = evaluateAudit(report, ALLOWLIST); + const waived = [...advisories.keys()].filter((id) => ALLOWLIST.some((e) => e.id === id)); + if (waived.length > 0) { + console.log(`audit-gate: ${waived.length} allowlisted advisory(ies): ${waived.join(', ')}`); + } + if (failures.length > 0) { + console.error('audit-gate: FAILED'); + failures.forEach((failure) => console.error(` - ${failure}`)); + process.exit(1); + } + console.log('audit-gate: OK (no non-allowlisted high/critical advisories in production deps).'); +} diff --git a/frontend/tests/unit/auditGate.test.js b/frontend/tests/unit/auditGate.test.js new file mode 100644 index 0000000..afdad19 --- /dev/null +++ b/frontend/tests/unit/auditGate.test.js @@ -0,0 +1,90 @@ +// The CI dependency-audit gate: scoped waivers with expiry, instead of a +// blanket audit-level drop. Tests exercise the pure decision logic against +// synthetic `npm audit --json` shapes. +import { evaluateAudit, collectGatedAdvisories, ALLOWLIST } from '../../scripts/audit-gate.mjs'; + +const reportWith = (advisories) => ({ + vulnerabilities: Object.fromEntries( + advisories.map(({ pkg, id, severity, title }, index) => [ + pkg || `pkg-${index}`, + { + severity, + via: [{ url: `https://github.com/advisories/${id}`, severity, title: title || 'x' }], + }, + ]), + ), +}); + +describe('audit gate', () => { + it('passes when the only advisories are allowlisted and unexpired', () => { + const report = reportWith([ + { pkg: 'react-router', id: 'GHSA-aaaa-bbbb-cccc', severity: 'high' }, + ]); + const allowlist = [ + { id: 'GHSA-aaaa-bbbb-cccc', reason: 'not exploitable', expires: '2999-01-01' }, + ]; + + const { failures } = evaluateAudit(report, allowlist, new Date('2026-08-03')); + + expect(failures).toEqual([]); + }); + + it('fails on any high/critical advisory that is not allowlisted', () => { + const report = reportWith([ + { pkg: 'evil-dep', id: 'GHSA-dddd-eeee-ffff', severity: 'critical' }, + ]); + + const { failures } = evaluateAudit(report, [], new Date('2026-08-03')); + + expect(failures).toHaveLength(1); + expect(failures[0]).toContain('GHSA-dddd-eeee-ffff'); + expect(failures[0]).toContain('not allowlisted'); + }); + + it('fails once a waiver expires, forcing a dated re-evaluation', () => { + const report = reportWith([ + { pkg: 'react-router', id: 'GHSA-aaaa-bbbb-cccc', severity: 'high' }, + ]); + const allowlist = [ + { id: 'GHSA-aaaa-bbbb-cccc', reason: 'was fine in July', expires: '2026-07-01' }, + ]; + + const { failures } = evaluateAudit(report, allowlist, new Date('2026-08-03')); + + expect(failures).toHaveLength(1); + expect(failures[0]).toContain('EXPIRED'); + }); + + it('fails on a stale waiver whose advisory is no longer reported (fix landed)', () => { + const allowlist = [{ id: 'GHSA-gone-gone-gone', reason: 'old', expires: '2999-01-01' }]; + + const { failures } = evaluateAudit({ vulnerabilities: {} }, allowlist, new Date('2026-08-03')); + + expect(failures).toHaveLength(1); + expect(failures[0]).toContain('remove the entry'); + }); + + it('ignores moderate/low advisories and transitive string references', () => { + const report = { + vulnerabilities: { + 'some-pkg': { + severity: 'moderate', + via: [ + { url: 'https://github.com/advisories/GHSA-mmmm-nnnn-oooo', severity: 'moderate' }, + 'other-pkg', // transitive reference, not an advisory object + ], + }, + }, + }; + + expect(collectGatedAdvisories(report).size).toBe(0); + expect(evaluateAudit(report, [], new Date()).failures).toEqual([]); + }); + + it('the real allowlist stays scoped: single entry, justified, with a real expiry', () => { + expect(ALLOWLIST).toHaveLength(1); + expect(ALLOWLIST[0].id).toBe('GHSA-qwww-vcr4-c8h2'); + expect(ALLOWLIST[0].reason.length).toBeGreaterThan(20); + expect(ALLOWLIST[0].expires).toMatch(/^\d{4}-\d{2}-\d{2}$/); + }); +});