From bd064e44bde7702be6d12adc3d1ce681c656bcd2 Mon Sep 17 00:00:00 2001 From: Mikko Numminen Date: Fri, 24 Jul 2026 04:22:52 +0300 Subject: [PATCH 1/4] fix(2fa): fail closed in production without TOTP_ENCRYPTION_KEY; accept AUTH_SECRET in dev MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The encryption-key fallback read NEXTAUTH_SECRET — the NextAuth v4 name. This app is on v5 and configures AUTH_SECRET, so the "dev convenience" fallback was dead: on Vercel neither TOTP_ENCRYPTION_KEY nor NEXTAUTH_SECRET is set, and every 2FA setup/verify threw at runtime. Rework getEncryptionKey to mirror getHmacSecret in auditHashChain.ts: - TOTP_ENCRYPTION_KEY first, always. - NEXTAUTH_SECRET stays honored in EVERY environment — the k8s templates still set it, and TOTP secrets encrypted under that fallback must remain decryptable. Dropping it would strand them. - AUTH_SECRET now works as the dev/test fallback, but deliberately does NOT count in production: 2FA fails closed with an actionable message until a dedicated key is set, instead of silently deriving 2FA crypto from the session-signing secret. No production ciphertexts are affected: with both lookup names unset, encryptSecret always threw, so nothing was ever encrypted under a key this change replaces. Tests pin all four behaviors (v4 fallback, v5 dev fallback, production fail-closed with AUTH_SECRET set, production NEXTAUTH_SECRET back-compat), and .env.example now documents the variable — it was documented nowhere. Co-Authored-By: Claude Fable 5 --- .env.example | 6 ++- src/lib/totpCrypto.ts | 23 ++++++++-- src/tests/server/totpCrypto.test.ts | 69 +++++++++++++++++++++++++++-- 3 files changed, 91 insertions(+), 7 deletions(-) diff --git a/.env.example b/.env.example index fb41f480..0fdcc723 100644 --- a/.env.example +++ b/.env.example @@ -11,9 +11,13 @@ AUTH_GITHUB_SECRET="" # CRON_SECRET gates /api/cron/* (Vercel Cron sends it automatically once set). # AUDIT_HMAC_SECRET keys the audit-log hash chain — generate fresh, never reuse, # and never rotate casually (old entries verify against the key that wrote them). -# Generate both with: openssl rand -base64 32 +# TOTP_ENCRYPTION_KEY encrypts stored 2FA secrets (AES-256-GCM). Without it, 2FA +# setup/verify fails closed in production (dev falls back to AUTH_SECRET). Never +# rotate casually — stored secrets decrypt only with the key that encrypted them. +# Generate all three with: openssl rand -base64 32 CRON_SECRET="" AUDIT_HMAC_SECRET="" +TOTP_ENCRYPTION_KEY="" # Optional: enables `npm run i18n:translate` (auto-translation via Claude API). # Without it, translate new keys by hand — see the i18n skill. diff --git a/src/lib/totpCrypto.ts b/src/lib/totpCrypto.ts index aad5cc12..d7054712 100644 --- a/src/lib/totpCrypto.ts +++ b/src/lib/totpCrypto.ts @@ -8,12 +8,29 @@ const ISSUER = "HRManager"; /** * Derives a 256-bit AES key from the TOTP_ENCRYPTION_KEY env var. - * Falls back to a deterministic key derived from NEXTAUTH_SECRET for dev convenience. + * + * Fallback order matters for decryptability of existing secrets: + * - NEXTAUTH_SECRET (the NextAuth v4 name) is honored in every environment — + * the k8s templates still set it, and any deployment that encrypted TOTP + * secrets under that fallback must keep decrypting them. + * - AUTH_SECRET (the v5 name this app actually configures) is a dev/test + * convenience only. In production it deliberately does NOT count: 2FA + * fails closed until a dedicated TOTP_ENCRYPTION_KEY is set, mirroring + * getHmacSecret() in auditHashChain.ts. */ function getEncryptionKey(): Buffer { - const envKey = process.env.TOTP_ENCRYPTION_KEY ?? process.env.NEXTAUTH_SECRET; + let envKey = process.env.TOTP_ENCRYPTION_KEY ?? process.env.NEXTAUTH_SECRET; + if (!envKey && process.env.NODE_ENV !== "production") { + envKey = process.env.AUTH_SECRET; + } if (!envKey) { - throw new Error("TOTP_ENCRYPTION_KEY or NEXTAUTH_SECRET must be set"); + throw new Error( + process.env.NODE_ENV === "production" + ? "TOTP_ENCRYPTION_KEY is not set. Two-factor secrets cannot be encrypted or " + + "decrypted without a dedicated key in production. Set TOTP_ENCRYPTION_KEY " + + "to a strong random value (openssl rand -base64 32)." + : "TOTP_ENCRYPTION_KEY, NEXTAUTH_SECRET, or AUTH_SECRET must be set", + ); } // Derive a 32-byte key via SHA-256 so any-length secret works return createHash("sha256").update(envKey).digest(); diff --git a/src/tests/server/totpCrypto.test.ts b/src/tests/server/totpCrypto.test.ts index ac46bd8d..1646f9c5 100644 --- a/src/tests/server/totpCrypto.test.ts +++ b/src/tests/server/totpCrypto.test.ts @@ -32,23 +32,86 @@ describe("TOTP Crypto Utilities", () => { process.env.TOTP_ENCRYPTION_KEY = saved; }); - // Throws when both TOTP_ENCRYPTION_KEY and NEXTAUTH_SECRET are absent — covers line 16. - it("throws when neither TOTP_ENCRYPTION_KEY nor NEXTAUTH_SECRET is set", () => { + // Falls back to AUTH_SECRET (the NextAuth v5 name) outside production — + // previously only the v4 NEXTAUTH_SECRET counted, which nothing v5 sets. + it("should use AUTH_SECRET as fallback when the other two are missing", () => { const savedTotp = process.env.TOTP_ENCRYPTION_KEY; const savedNext = process.env.NEXTAUTH_SECRET; + const savedAuth = process.env.AUTH_SECRET; delete process.env.TOTP_ENCRYPTION_KEY; delete process.env.NEXTAUTH_SECRET; + process.env.AUTH_SECRET = "auth-secret-v5-fallback-for-testing-32-chars"; + + const { encryptSecret, decryptSecret } = require("@/lib/totpCrypto"); + const secret = "JBSWY3DPEHPK3PXP"; + expect(decryptSecret(encryptSecret(secret))).toBe(secret); + + process.env.TOTP_ENCRYPTION_KEY = savedTotp; + process.env.NEXTAUTH_SECRET = savedNext; + if (savedAuth === undefined) delete process.env.AUTH_SECRET; + else process.env.AUTH_SECRET = savedAuth; + }); + + // Throws when all three key sources are absent — covers the dev error branch. + it("throws when no key source is set", () => { + const savedTotp = process.env.TOTP_ENCRYPTION_KEY; + const savedNext = process.env.NEXTAUTH_SECRET; + const savedAuth = process.env.AUTH_SECRET; + delete process.env.TOTP_ENCRYPTION_KEY; + delete process.env.NEXTAUTH_SECRET; + delete process.env.AUTH_SECRET; jest.resetModules(); const { encryptSecret: encryptFresh } = require("@/lib/totpCrypto"); expect(() => encryptFresh("JBSWY3DPEHPK3PXP")).toThrow( - "TOTP_ENCRYPTION_KEY or NEXTAUTH_SECRET must be set", + "TOTP_ENCRYPTION_KEY, NEXTAUTH_SECRET, or AUTH_SECRET must be set", ); process.env.TOTP_ENCRYPTION_KEY = savedTotp; process.env.NEXTAUTH_SECRET = savedNext; + if (savedAuth !== undefined) process.env.AUTH_SECRET = savedAuth; jest.resetModules(); }); + + // In production AUTH_SECRET deliberately does NOT satisfy the key lookup — + // 2FA fails closed until a dedicated TOTP_ENCRYPTION_KEY is set (mirrors + // the AUDIT_HMAC_SECRET pattern in auditHashChain.ts). + it("throws in production even when AUTH_SECRET is set", () => { + const savedTotp = process.env.TOTP_ENCRYPTION_KEY; + const savedNext = process.env.NEXTAUTH_SECRET; + const savedAuth = process.env.AUTH_SECRET; + const savedNodeEnv = process.env.NODE_ENV; + delete process.env.TOTP_ENCRYPTION_KEY; + delete process.env.NEXTAUTH_SECRET; + process.env.AUTH_SECRET = "auth-secret-v5-fallback-for-testing-32-chars"; + (process.env as Record).NODE_ENV = "production"; + + const { encryptSecret: encryptFresh } = require("@/lib/totpCrypto"); + expect(() => encryptFresh("JBSWY3DPEHPK3PXP")).toThrow(/TOTP_ENCRYPTION_KEY is not set/); + + (process.env as Record).NODE_ENV = savedNodeEnv; + process.env.TOTP_ENCRYPTION_KEY = savedTotp; + process.env.NEXTAUTH_SECRET = savedNext; + if (savedAuth === undefined) delete process.env.AUTH_SECRET; + else process.env.AUTH_SECRET = savedAuth; + }); + + // The legacy NEXTAUTH_SECRET fallback keeps working in production — the k8s + // templates still set it, and TOTP secrets encrypted under it must stay + // decryptable. + it("honors NEXTAUTH_SECRET in production (k8s back-compat)", () => { + const savedTotp = process.env.TOTP_ENCRYPTION_KEY; + const savedNodeEnv = process.env.NODE_ENV; + delete process.env.TOTP_ENCRYPTION_KEY; + (process.env as Record).NODE_ENV = "production"; + + const { encryptSecret, decryptSecret } = require("@/lib/totpCrypto"); + const secret = "JBSWY3DPEHPK3PXP"; + expect(decryptSecret(encryptSecret(secret))).toBe(secret); + + (process.env as Record).NODE_ENV = savedNodeEnv; + process.env.TOTP_ENCRYPTION_KEY = savedTotp; + }); }); describe("encryptSecret / decryptSecret", () => { From 5a40b083ae098a6d7c4b390b9595afcab3db6219 Mon Sep 17 00:00:00 2001 From: Mikko Numminen Date: Fri, 24 Jul 2026 04:34:01 +0300 Subject: [PATCH 2/4] docs(readme): refresh test table after the 2FA key-fallback tests (2906 -> 2909, branches 87.38) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Generated with `npm run readme:test-table` — its first run on this Windows machine, via the portability fix from #41. Both CI gates verified locally: check-coverage-claims and check-test-count pass against the fresh run. Co-Authored-By: Claude Fable 5 --- README.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 3989ff5c..65769c60 100644 --- a/README.md +++ b/README.md @@ -304,15 +304,15 @@ HRManager is designed as a standalone, independently deployable application — | Feature component tests | 981 | Per-feature UI + actions: persons, teams, departments, admin, reviews, realtime, leave, employee, 2FA, dashboard, reports | | Shared component tests | 105 | Reusable components shared across features: LeaveManager tabs (types, balances, requests) | | Shared suites (a11y, schemas, permissions) | 828 | Cross-cutting: axe-core WCAG AA, Zod schemas, RBAC permissions, i18n, themes, telemetry, tutorial, middleware | -| Server integration | 896 | Real PostgreSQL + MongoDB — server actions, queries, auth, audit hash chain, rate limiting, health, sessions | +| Server integration | 899 | Real PostgreSQL + MongoDB — server actions, queries, auth, audit hash chain, rate limiting, health, sessions | | Jobs | 21 | pg-boss queue setup, worker registration, and the serverless fetch-based drain | | E2E (Playwright) | 75 | Auth, CRUD, detail editing, dashboard, profile, data I/O, form validation, full workflow | -| **Total** | **2906** (2831 Jest + 75 Playwright) | **92.2% line coverage · 92.0% function coverage** | +| **Total** | **2909** (2834 Jest + 75 Playwright) | **92.2% line coverage · 92.0% function coverage** | ``` -Statements : 90.45% Branches : 87.36% +Statements : 90.45% Branches : 87.38% Functions : 91.97% Lines : 92.22% ``` From f440a73f8ed38a49b5669f4b2827f749a9c43a2c Mon Sep 17 00:00:00 2001 From: Mikko Numminen Date: Fri, 24 Jul 2026 05:27:15 +0300 Subject: [PATCH 3/4] fix(2fa): treat an empty TOTP_ENCRYPTION_KEY as unset (review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review finding on the previous commit: the first lookup used ?? — but .env.example now ships TOTP_ENCRYPTION_KEY="" and an empty string is not nullish, so a copied-but-unfilled template value would block the NEXTAUTH_SECRET fallback entirely. In production on k8s (where NEXTAUTH_SECRET is the configured key) that combination would throw even though a valid legacy key was present. Use || instead: an empty string is never a valid key, and truthiness is what the mirrored getHmacSecret pattern already uses (`if (secret) return secret`). Pinned with a test that fails under ??. Co-Authored-By: Claude Fable 5 --- src/lib/totpCrypto.ts | 5 ++++- src/tests/server/totpCrypto.test.ts | 14 ++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/src/lib/totpCrypto.ts b/src/lib/totpCrypto.ts index d7054712..d70f4882 100644 --- a/src/lib/totpCrypto.ts +++ b/src/lib/totpCrypto.ts @@ -19,7 +19,10 @@ const ISSUER = "HRManager"; * getHmacSecret() in auditHashChain.ts. */ function getEncryptionKey(): Buffer { - let envKey = process.env.TOTP_ENCRYPTION_KEY ?? process.env.NEXTAUTH_SECRET; + // || not ??: .env.example ships TOTP_ENCRYPTION_KEY="" and an empty string is + // not nullish, so ?? would let an unfilled template value block the fallbacks. + // Truthiness also matches getHmacSecret's `if (secret) return secret`. + let envKey = process.env.TOTP_ENCRYPTION_KEY || process.env.NEXTAUTH_SECRET; if (!envKey && process.env.NODE_ENV !== "production") { envKey = process.env.AUTH_SECRET; } diff --git a/src/tests/server/totpCrypto.test.ts b/src/tests/server/totpCrypto.test.ts index 1646f9c5..f83ff0cc 100644 --- a/src/tests/server/totpCrypto.test.ts +++ b/src/tests/server/totpCrypto.test.ts @@ -32,6 +32,20 @@ describe("TOTP Crypto Utilities", () => { process.env.TOTP_ENCRYPTION_KEY = saved; }); + // An unfilled template value must not block the fallbacks: .env.example + // ships TOTP_ENCRYPTION_KEY="" and "" is not nullish, so with ?? a copied + // template would break k8s deployments whose key is NEXTAUTH_SECRET. + it("treats an empty TOTP_ENCRYPTION_KEY as unset (falls through to NEXTAUTH_SECRET)", () => { + const savedTotp = process.env.TOTP_ENCRYPTION_KEY; + process.env.TOTP_ENCRYPTION_KEY = ""; + + const { encryptSecret, decryptSecret } = require("@/lib/totpCrypto"); + const secret = "JBSWY3DPEHPK3PXP"; + expect(decryptSecret(encryptSecret(secret))).toBe(secret); + + process.env.TOTP_ENCRYPTION_KEY = savedTotp; + }); + // Falls back to AUTH_SECRET (the NextAuth v5 name) outside production — // previously only the v4 NEXTAUTH_SECRET counted, which nothing v5 sets. it("should use AUTH_SECRET as fallback when the other two are missing", () => { From f46bbb1638e5791de5cb9f1a0dfdc19775203b09 Mon Sep 17 00:00:00 2001 From: Mikko Numminen Date: Fri, 24 Jul 2026 05:27:18 +0300 Subject: [PATCH 4/4] docs(readme): test table 2909 -> 2910 after the empty-key regression test Verified against a fresh full run: 2835 Jest green, both README gates pass, coverage percentages unchanged (the || swap adds no branch counts). Co-Authored-By: Claude Fable 5 --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 65769c60..26168ac5 100644 --- a/README.md +++ b/README.md @@ -304,10 +304,10 @@ HRManager is designed as a standalone, independently deployable application — | Feature component tests | 981 | Per-feature UI + actions: persons, teams, departments, admin, reviews, realtime, leave, employee, 2FA, dashboard, reports | | Shared component tests | 105 | Reusable components shared across features: LeaveManager tabs (types, balances, requests) | | Shared suites (a11y, schemas, permissions) | 828 | Cross-cutting: axe-core WCAG AA, Zod schemas, RBAC permissions, i18n, themes, telemetry, tutorial, middleware | -| Server integration | 899 | Real PostgreSQL + MongoDB — server actions, queries, auth, audit hash chain, rate limiting, health, sessions | +| Server integration | 900 | Real PostgreSQL + MongoDB — server actions, queries, auth, audit hash chain, rate limiting, health, sessions | | Jobs | 21 | pg-boss queue setup, worker registration, and the serverless fetch-based drain | | E2E (Playwright) | 75 | Auth, CRUD, detail editing, dashboard, profile, data I/O, form validation, full workflow | -| **Total** | **2909** (2834 Jest + 75 Playwright) | **92.2% line coverage · 92.0% function coverage** | +| **Total** | **2910** (2835 Jest + 75 Playwright) | **92.2% line coverage · 92.0% function coverage** |