From f9e8385d5a8ba012901eedef822aabfeb1f07db8 Mon Sep 17 00:00:00 2001 From: Stephen Adamson Date: Sun, 16 Aug 2026 12:26:28 +0100 Subject: [PATCH] =?UTF-8?q?fix:=20auth/2FA=20security=20=E2=80=94=20stdout?= =?UTF-8?q?-log=20code=20delivery=20is=20dev/test-only,=20production=20fai?= =?UTF-8?q?ls=20closed=20until=20email/SMS;=20verification-code=20hashing,?= =?UTF-8?q?=20lockout=20recovery,=20sabredav=20fail-closed?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - TWO_FACTOR_ALLOW_LOG_DELIVERY production opt-in REMOVED: plaintext codes are written to the stdout log ([2FA]/[VERIFY]) only in dev/test builds as a local DEV ONLY feature while email/SMS delivery (P6) is implemented. Production builds have no delivery channel and code issuance fails closed (503) under any configuration — no silent log-based code leak - verification/2FA codes hashed at rest (HMAC-SHA256 via TWO_FACTOR_PEPPER, CHAR(64)); [VERIFY] dev log relay; per-user brute-force budget; password_reset purpose clears lockout for self-service recovery; dummy-bcrypt on login no-user path kills timing oracle - sabredav weak-password list + entropy gate; .env.example ships fail-closed DAV_ADMIN_PASSWORD - delete-account re-auth (current_password + fresh 2FA code when enforced) - prod-tag suite (run-prod-tag-tests.sh) compiles and runs the production 2FA issuance gate: production ALWAYS reports no delivery channel and refuses issuance after the pepper check - startup_checks_test SNAPSHOT_ENC_KEY values built at runtime so gitleaks sees no secret-shaped literals - env-docs parity updated (flag removed, 38 vars) --- .env.example | 42 +-- backend/handlers/auth/auth_test.go | 205 ++++++++++-- backend/handlers/auth/local.go | 308 +++++++++++++----- backend/handlers/auth/verifycode_dev.go | 30 ++ backend/handlers/auth/verifycode_prod.go | 46 +++ backend/handlers/user/gdpr_test.go | 7 +- backend/handlers/user/guest.go | 50 ++- backend/handlers/user/guest_test.go | 108 +++--- backend/handlers/user/twofa.go | 89 +++-- backend/handlers/user/twofa_dev.go | 21 +- .../handlers/user/twofa_failclosed_test.go | 35 +- backend/handlers/user/twofa_prod.go | 102 +++--- backend/handlers/user/twofa_prod_test.go | 58 ++-- backend/internal/twofa/twofa.go | 147 ++++----- backend/main.go | 22 +- backend/run-prod-tag-tests.sh | 52 +++ backend/startup_checks_test.go | 144 ++++++++ sabredav/server.php | 47 ++- scripts/check-env-docs.py | 6 + 19 files changed, 1047 insertions(+), 472 deletions(-) create mode 100644 backend/handlers/auth/verifycode_dev.go create mode 100644 backend/handlers/auth/verifycode_prod.go create mode 100755 backend/run-prod-tag-tests.sh create mode 100644 backend/startup_checks_test.go diff --git a/.env.example b/.env.example index a304549..030e5f2 100644 --- a/.env.example +++ b/.env.example @@ -75,11 +75,12 @@ SQUARE_ALLOW_REAL_API= # (SimulateSavedCardVerificationRequired + cnon:sca-... tokenize-results), so # development has full parity with the SCA-only production posture. # Code delivery: the intended channel is email/SMS (the method chosen at -# setup) — NOT wired yet. Until it lands, the verification code is delivered -# via the server log (a [2FA]-prefixed line) when the operator opts into -# TWO_FACTOR_ALLOW_LOG_DELIVERY=true (see below); in enforced/production -# environments an operator must relay the logged code to the user out-of-band; -# the API never returns the code while enforcement is ON. +# setup) — NOT wired yet (P6). Until it lands, verification/2FA codes are +# delivered to the LOCAL DEV stdout log (a [2FA]-prefixed line) in dev/test +# builds ONLY — stdout-log delivery is a dev-only convenience, never a +# production channel. Production builds have no delivery channel and code +# issuance FAILS CLOSED (503) until email/SMS is implemented; the API never +# returns the code while enforcement is ON. REQUIRE_2FA=true # TWO_FACTOR_PEPPER — server-side pepper for HMAC-hashing 2FA codes. REQUIRED # in production builds: code issuance FAILS CLOSED when it is unset (an @@ -89,14 +90,6 @@ REQUIRE_2FA=true # digest with a one-time warning. Generate with: # openssl rand -base64 32 TWO_FACTOR_PEPPER= -# TWO_FACTOR_ALLOW_LOG_DELIVERY — defaults false. Production 2FA code issuance -# FAILS CLOSED without a delivery channel: there is no email/SMS transport yet, -# so the ONLY production channel is the operator's explicit opt-in to the -# insecure server-log delivery ([2FA] prefix — anyone with backend log access -# can defeat the account 2FA gate). MUST be set to true to deliver -# 2FA codes via the server log in production until email/SMS lands. Dev/test -# builds always deliver via the log and never consult this flag. -TWO_FACTOR_ALLOW_LOG_DELIVERY=false # SNAPSHOT_ENC_KEY — base64-encoded 32-byte AES-256 key for encrypting stored # square_request_snapshot rows (buyer PII: email + ccof card tokens) at rest in # non-mock (production/sandbox) deployments. If unset/invalid, snapshots fall @@ -138,14 +131,23 @@ GO_TESTING= # CardDAV store), it never calls this URL. Only the sabredav PHP container needs # the server-side credential below. DAV_BASE_URL=http://localhost:8080 -# DAV_ADMIN_PASSWORD — REQUIRED, FAIL-CLOSED. sabredav/server.php refuses to -# start when unset or set to a known weak/default value ('admin' etc.) — this -# server exposes customer PII vCards, so no public default credential is ever -# acceptable. The placeholder below satisfies compose validation only; it MUST -# be replaced before any deployment (compose.yml fails fast via -# ${DAV_ADMIN_PASSWORD:?} if it is ever unset). Generate a strong random value: +# DAV_ADMIN_PASSWORD — REQUIRED, FAIL-CLOSED. This CardDAV/CalDAV server +# exposes customer PII vCards, so no public/default credential is ever +# acceptable. Leave it EMPTY here (as below): compose.yml fails fast via +# ${DAV_ADMIN_PASSWORD:?} when it is unset or empty, so a copy-paste +# `cp .env.example .env` deployment is caught BEFORE anything boots. As a +# second layer, sabredav/server.php refuses to start when the value is a known +# weak/default placeholder ('changeme-admin-password', 'changeme', 'secret', +# 'test', ...) or has <16 characters / <8 distinct characters (mirrors +# isWeakJWTSecret in backend/main.go). Generate a strong random value before +# any deployment: # openssl rand -hex 32 -DAV_ADMIN_PASSWORD=changeme-admin-password +# Production email/SMS delivery of verification codes is NOT wired yet (P6); +# code delivery happens only in dev/test builds via the LOCAL DEV stdout log +# ([2FA]/[VERIFY] prefixes). Production builds have no delivery channel, so +# 2FA/verification-code issuance FAILS CLOSED (503) until email/SMS delivery +# is implemented — see the 2FA section of README.md. +DAV_ADMIN_PASSWORD= # Logging # Set to "true" to disable ANSI color escape sequences in log output diff --git a/backend/handlers/auth/auth_test.go b/backend/handlers/auth/auth_test.go index 0c62f23..2186b98 100644 --- a/backend/handlers/auth/auth_test.go +++ b/backend/handlers/auth/auth_test.go @@ -21,6 +21,7 @@ package auth import ( "bytes" "context" + "encoding/hex" "encoding/json" "fmt" "net/http" @@ -32,6 +33,7 @@ import ( "crussell/auth" "crussell/clock" "crussell/db" + "crussell/internal/twofa" "crussell/mw" "crussell/testutils" "crussell/testutils/fixtures" @@ -46,6 +48,25 @@ func resetTestData(t *testing.T) (context.Context, db.Querier) { return ctx, tx } +// insertVerificationCode creates a verification_codes row storing only the +// digest of a fresh random code (mirroring GenerateVerificationCodeHandler — +// the code column holds twofa.Hash(plaintext), never the plaintext) and +// returns the plaintext so the test can submit it to VerifyCodeHandler exactly +// like a delivered code would be used. +func insertVerificationCode(ctx context.Context, q db.Querier, userID, purpose string, expiresAt time.Time) (string, error) { + code, err := generateVerificationCode() + if err != nil { + return "", err + } + _, err = q.Exec(ctx, + `INSERT INTO verification_codes (user_id, purpose, code, expires_at) VALUES ($1, $2, $3, $4)`, + userID, purpose, twofa.Hash(code), expiresAt) + if err != nil { + return "", err + } + return code, nil +} + // ============================================================================= // Register Handler Tests // ============================================================================= @@ -650,12 +671,11 @@ func TestVerifyCheck_ValidCode(t *testing.T) { } defer fixtures.DeleteUser(tx, userID) - // Create a verification code + // Create a verification code (only the digest is stored — see + // insertVerificationCode); the test submits the plaintext code. var code string expiresAt := clock.Now().Add(24 * time.Hour) - err = tx.QueryRow(ctx, - `INSERT INTO verification_codes (user_id, purpose, expires_at) VALUES ($1, 'email_verify', $2) RETURNING code`, - userID, expiresAt).Scan(&code) + code, err = insertVerificationCode(ctx, tx, userID, verificationCodePurposeEmailVerify, expiresAt) if err != nil { t.Fatalf("failed to create verification code: %v", err) } @@ -683,7 +703,7 @@ func TestVerifyCheck_ValidCode(t *testing.T) { // Verify code is marked as used var usedAt *time.Time err = tx.QueryRow(ctx, - "SELECT used_at FROM verification_codes WHERE code = $1", code).Scan(&usedAt) + "SELECT used_at FROM verification_codes WHERE code = $1", twofa.Hash(code)).Scan(&usedAt) if err != nil || usedAt == nil { t.Error("expected verification code to be marked as used") } @@ -726,9 +746,7 @@ func TestVerifyCheck_ExpiredCode(t *testing.T) { // Create an expired verification code var code string expiresAt := clock.Now().Add(-1 * time.Hour) // Expired 1 hour ago - err = tx.QueryRow(ctx, - `INSERT INTO verification_codes (user_id, purpose, expires_at) VALUES ($1, 'email_verify', $2) RETURNING code`, - userID, expiresAt).Scan(&code) + code, err = insertVerificationCode(ctx, tx, userID, verificationCodePurposeEmailVerify, expiresAt) if err != nil { t.Fatalf("failed to create verification code: %v", err) } @@ -888,9 +906,7 @@ func TestVerifyCheck_AlreadyUsed(t *testing.T) { // Create a verification code var code string expiresAt := clock.Now().Add(24 * time.Hour) - err = tx.QueryRow(ctx, - `INSERT INTO verification_codes (user_id, purpose, expires_at) VALUES ($1, 'email_verify', $2) RETURNING code`, - userID, expiresAt).Scan(&code) + code, err = insertVerificationCode(ctx, tx, userID, verificationCodePurposeEmailVerify, expiresAt) if err != nil { t.Fatalf("failed to create verification code: %v", err) } @@ -942,9 +958,7 @@ func TestVerifyCheck_RoleChangeToVerified(t *testing.T) { // Create a verification code var code string expiresAt := clock.Now().Add(24 * time.Hour) - err = tx.QueryRow(ctx, - `INSERT INTO verification_codes (user_id, purpose, expires_at) VALUES ($1, 'email_verify', $2) RETURNING code`, - userID, expiresAt).Scan(&code) + code, err = insertVerificationCode(ctx, tx, userID, verificationCodePurposeEmailVerify, expiresAt) if err != nil { t.Fatalf("failed to create verification code: %v", err) } @@ -1757,9 +1771,8 @@ func TestVerifyCheck_AttemptBudget_LocksOutAfterFive(t *testing.T) { defer fixtures.DeleteUser(tx, userID) var realCode string expiresAt := clock.Now().Add(24 * time.Hour) - if err := tx.QueryRow(ctx, - `INSERT INTO verification_codes (user_id, purpose, expires_at) VALUES ($1, 'email_verify', $2) RETURNING code`, - userID, expiresAt).Scan(&realCode); err != nil { + realCode, err = insertVerificationCode(ctx, tx, userID, verificationCodePurposeEmailVerify, expiresAt) + if err != nil { t.Fatalf("failed to create verification code: %v", err) } defer tx.Exec(ctx, "DELETE FROM verification_codes WHERE user_id = $1", userID) @@ -1784,14 +1797,168 @@ func TestVerifyCheck_AttemptBudget_LocksOutAfterFive(t *testing.T) { t.Errorf("expected 429 for a spent budget, got %d. body: %s", w.Code, w.Body.String()) } - // A DIFFERENT code (the real one) is unaffected by the spent budget and - // verifies successfully — budgets are per code. + // A DIFFERENT code (the real one) is unaffected by the spent miss-path + // budget and verifies successfully: the miss-path budget is keyed per + // submitted code value (a guess cannot resolve a user), while the real + // code resolves to the user and uses the (fresh) per-user budget. w = testutils.MakeRequestNoAuth(handler, "POST", "/api/verify/check", VerifyCodeRequest{Code: realCode}, ctx) if w.Code != http.StatusOK { t.Errorf("a valid code must still verify after another code's budget was spent, got %d. body: %s", w.Code, w.Body.String()) } } +// TestVerifyGenerate_StoresDigestNotPlaintext pins the MEDIUM finding fix: +// the verification_codes.code column stores ONLY the twofa.Hash digest (a +// 64-char hex SHA-256), never the 12-hex-char plaintext the old schema stored. +func TestVerifyGenerate_StoresDigestNotPlaintext(t *testing.T) { + t.Parallel() + ctx, tx := resetTestData(t) + + handler := http.HandlerFunc(GenerateVerificationCodeHandler) + + userID, err := fixtures.CreateTestUserWithEmail(tx, "hash-at-rest@test.com", "verified_email") + if err != nil { + t.Fatalf("failed to create test user: %v", err) + } + defer fixtures.DeleteUser(tx, userID) + + w := testutils.MakeRequestNoAuth(handler, "POST", "/api/verify/generate", + VerificationCodeRequest{Email: "hash-at-rest@test.com"}, ctx) + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d. body: %s", w.Code, w.Body.String()) + } + + var stored string + err = tx.QueryRow(ctx, + `SELECT code FROM verification_codes WHERE user_id = $1 AND purpose = 'email_verify'`, userID).Scan(&stored) + if err != nil { + t.Fatalf("failed to read stored code: %v", err) + } + if len(stored) != 64 { + t.Errorf("expected a 64-char hex digest at rest, got %q (len %d)", stored, len(stored)) + } + if _, err := hex.DecodeString(stored); err != nil { + t.Errorf("stored code %q is not hex: %v", stored, err) + } +} + +// TestVerifyCheck_PasswordReset_ClearsLoginLockout pins the HIGH finding fix: +// verifying a password_reset code clears a locked-out account's +// failed_attempts/locked_until, so the owner can log in again and change their +// password — the self-service recovery for the repeatable login-DoS. +func TestVerifyCheck_PasswordReset_ClearsLoginLockout(t *testing.T) { + t.Parallel() + ctx, tx := resetTestData(t) + + handler := http.HandlerFunc(VerifyCodeHandler) + + userID, err := fixtures.CreateTestUserWithEmail(tx, "reset@test.com", "verified_email") + if err != nil { + t.Fatalf("failed to create test user: %v", err) + } + defer fixtures.DeleteUser(tx, userID) + + // Simulate the attacker-lockout state (7 failures → 30-minute lock). + _, err = tx.Exec(ctx, `UPDATE users SET failed_attempts = 7, locked_until = NOW() + INTERVAL '30 minutes' WHERE id = $1`, userID) + if err != nil { + t.Fatalf("failed to lock account: %v", err) + } + + code, err := insertVerificationCode(ctx, tx, userID, verificationCodePurposePasswordReset, clock.Now().Add(24*time.Hour)) + if err != nil { + t.Fatalf("failed to create password_reset code: %v", err) + } + + w := testutils.MakeRequestNoAuth(handler, "POST", "/api/verify/check", VerifyCodeRequest{Code: code}, ctx) + if w.Code != http.StatusOK { + t.Fatalf("expected 200 for a valid password_reset code, got %d. body: %s", w.Code, w.Body.String()) + } + + var failed int + var locked *time.Time + err = tx.QueryRow(ctx, `SELECT failed_attempts, locked_until FROM users WHERE id = $1`, userID).Scan(&failed, &locked) + if err != nil { + t.Fatalf("failed to read lockout state: %v", err) + } + if failed != 0 { + t.Errorf("expected failed_attempts reset to 0, got %d", failed) + } + if locked != nil { + t.Errorf("expected locked_until cleared, got %v", locked) + } + + // The unlocked account can log in again with the correct password. + login := http.HandlerFunc(LoginHandler) + w = testutils.MakeRequestNoAuth(login, "POST", "/api/login", + LoginRequest{Email: "reset@test.com", Password: "testpassword123"}, ctx) + if w.Code != http.StatusOK { + t.Errorf("expected login to succeed after lockout cleared, got %d. body: %s", w.Code, w.Body.String()) + } +} + +// TestVerifyCheck_AttemptBudget_KeyedPerUser pins the MEDIUM finding fix: once +// a submitted code resolves to a user (an existing-but-expired row), the +// brute-force budget follows the USER, so draining it with one code value +// exhausts it for every other code value of that user — and a different user's +// budget stays independent. +func TestVerifyCheck_AttemptBudget_KeyedPerUser(t *testing.T) { + t.Parallel() + ctx, tx := resetTestData(t) + + handler := http.HandlerFunc(VerifyCodeHandler) + + userID, err := fixtures.CreateTestUserWithEmail(tx, "budget-user@test.com", "verified_email") + if err != nil { + t.Fatalf("failed to create test user: %v", err) + } + defer fixtures.DeleteUser(tx, userID) + + otherID, err := fixtures.CreateTestUserWithEmail(tx, "budget-other@test.com", "verified_email") + if err != nil { + t.Fatalf("failed to create second test user: %v", err) + } + defer fixtures.DeleteUser(tx, otherID) + + // Five expired codes for the same user, each burned on the expired path. + for i := 0; i < 5; i++ { + code, err := insertVerificationCode(ctx, tx, userID, verificationCodePurposeEmailVerify, clock.Now().Add(-time.Hour)) + if err != nil { + t.Fatalf("failed to create expired code: %v", err) + } + w := testutils.MakeRequestNoAuth(handler, "POST", "/api/verify/check", VerifyCodeRequest{Code: code}, ctx) + if i < 4 { + if w.Code != http.StatusBadRequest { + t.Fatalf("attempt %d: expected 400 for an expired code, got %d. body: %s", i+1, w.Code, w.Body.String()) + } + } else { + if w.Code != http.StatusTooManyRequests { + t.Fatalf("attempt 5: expected 429, got %d. body: %s", w.Code, w.Body.String()) + } + } + } + + // A SIXTH expired code for the SAME user must be rejected 429 up front: the + // budget followed the user, not each distinct code value. + code, err := insertVerificationCode(ctx, tx, userID, verificationCodePurposeEmailVerify, clock.Now().Add(-time.Hour)) + if err != nil { + t.Fatalf("failed to create sixth expired code: %v", err) + } + w := testutils.MakeRequestNoAuth(handler, "POST", "/api/verify/check", VerifyCodeRequest{Code: code}, ctx) + if w.Code != http.StatusTooManyRequests { + t.Errorf("expected 429 for the same user's next code (per-user budget), got %d. body: %s", w.Code, w.Body.String()) + } + + // A DIFFERENT user's fresh valid code still verifies — budgets are per user. + otherCode, err := insertVerificationCode(ctx, tx, otherID, verificationCodePurposeEmailVerify, clock.Now().Add(24*time.Hour)) + if err != nil { + t.Fatalf("failed to create other user's code: %v", err) + } + w = testutils.MakeRequestNoAuth(handler, "POST", "/api/verify/check", VerifyCodeRequest{Code: otherCode}, ctx) + if w.Code != http.StatusOK { + t.Errorf("expected the other user's valid code to verify, got %d. body: %s", w.Code, w.Body.String()) + } +} + // ValidateUKPhoneNumber Security Tests // // These tests verify that ValidateUKPhoneNumber rejects or sanitises diff --git a/backend/handlers/auth/local.go b/backend/handlers/auth/local.go index 81f55aa..f7a194d 100644 --- a/backend/handlers/auth/local.go +++ b/backend/handlers/auth/local.go @@ -6,9 +6,12 @@ import ( "crussell/clock" "crussell/db" "crussell/internal/dav" + "crussell/internal/twofa" "crussell/internal/validators" "crussell/internal/zxcvbnjs" "crussell/mw" + "crypto/rand" + "encoding/hex" "encoding/json" "errors" "fmt" @@ -103,17 +106,41 @@ func CleanupStaleLoginEntries(ctx context.Context) (int, error) { return 0, nil } -// Email-verification attempt budget (Round 2 Loop A finding 8): POST -// /verify/check had no per-user attempt counter, so a client holding a code -// could fail it indefinitely and the endpoint doubled as an unbounded guessing -// oracle. Mirror the 2FA attempt pattern: an in-memory map keys a 5-attempt -// budget per submitted code. The code is the only identifier a wrong guess -// carries, and every code is user-scoped (one code belongs to exactly one -// user), so the budget is effectively per-user-per-code — a distinct user can -// never drain another's budget for the same code. A successful verify clears -// the entry; the 5th failed attempt exhausts the budget (429). The map is -// bounded and stale entries are evicted, so a flood of random guesses cannot -// grow it without bound. +// dummyPasswordHash is a real bcrypt hash of a fixed throwaway string, used to +// burn the same constant-time bcrypt work on the login no-user path as a real +// wrong-password compare (see LoginHandler). It MUST be a well-formed bcrypt +// hash: CompareHashAndPassword on a malformed hash returns immediately (fast), +// which would reintroduce the timing oracle it exists to remove. +const dummyPasswordHash = "$2a$10$x9x4AEOAU.UbaGCmVsVwu.TUhfOfR2LfbmWjB/H2At8Sx69WIlkri" + +// Verification-code purpose values (verification_purpose enum in init-script.sql). +const ( + verificationCodePurposeEmailVerify = "email_verify" + verificationCodePurposePasswordReset = "password_reset" +) + +// verificationCodePepperEnv is the environment variable whose value keys the +// HMAC-SHA256 of stored verification codes (the SAME pepper the 2FA path uses — +// crussell/internal/twofa Hash). Read through the build-tagged +// verificationCodeEnsureIssueAllowed (verifycode_dev.go / verifycode_prod.go): +// dev/test builds fall back to the legacy plain SHA-256 digest with the 2FA +// warning, while production builds refuse to issue codes without the pepper. +const verificationCodePepperEnv = "TWO_FACTOR_PEPPER" + +// Email-verification attempt budget (Round 2 Loop A finding 8 + hardening): +// POST /verify/check had no per-user attempt counter, so a client holding a +// code could fail it indefinitely and the endpoint doubled as an unbounded +// guessing oracle. Mirror the 2FA attempt pattern: an in-memory map keys a +// 5-attempt budget. The key is the RESOLVED USER id whenever a submitted code +// matches a verification_codes row (a code belongs to exactly one user, so the +// budget follows the ACCOUNT being attacked, not the submitted code value) and +// the submitted code value only when no row exists to resolve a user (a pure +// guess cannot be attributed). Keying per-user closes the evasion where an +// attacker holding several codes for one victim (or probing which values are +// live) drained a fresh budget per code. A successful verify clears the entry; +// the 5th failed attempt exhausts the budget (429). The map is bounded and +// stale entries are evicted, so a flood of random guesses cannot grow it +// without bound. const ( emailVerifyMaxAttempts = 5 emailVerifyAttemptWindow = 30 * time.Minute @@ -130,39 +157,40 @@ var ( emailVerifyAttempts = make(map[string]emailVerifyAttempt) ) -// emailVerifyAttemptsExhausted reports whether the submitted code's attempt -// budget is already spent, rejecting the request before any DB work. -func emailVerifyAttemptsExhausted(code string) bool { +// emailVerifyAttemptsExhausted reports whether the key's (a user id, or a +// submitted code with no resolvable user) attempt budget is already spent, +// rejecting the request before any DB work. +func emailVerifyAttemptsExhausted(key string) bool { emailVerifyMu.Lock() defer emailVerifyMu.Unlock() evictStaleEmailVerifyAttemptsLocked() - a, ok := emailVerifyAttempts[code] + a, ok := emailVerifyAttempts[key] return ok && a.count >= emailVerifyMaxAttempts } // emailVerifyAttemptFailed registers one failed verification attempt for the -// submitted code and reports whether the budget for that code is now exhausted -// (the handler should respond 429). -func emailVerifyAttemptFailed(code string) bool { +// key and reports whether the budget for that key is now exhausted (the handler +// should respond 429). +func emailVerifyAttemptFailed(key string) bool { emailVerifyMu.Lock() defer emailVerifyMu.Unlock() evictStaleEmailVerifyAttemptsLocked() now := clock.Now() - a := emailVerifyAttempts[code] + a := emailVerifyAttempts[key] if now.Sub(a.lastAt) > emailVerifyAttemptWindow { a.count = 0 } a.count++ a.lastAt = now - emailVerifyAttempts[code] = a + emailVerifyAttempts[key] = a return a.count >= emailVerifyMaxAttempts } -// emailVerifyAttemptsClear drops the budget for a code after a successful +// emailVerifyAttemptsClear drops the budget for a key after a successful // verify (the code is consumed; the entry would only leak stale state). -func emailVerifyAttemptsClear(code string) { +func emailVerifyAttemptsClear(key string) { emailVerifyMu.Lock() - delete(emailVerifyAttempts, code) + delete(emailVerifyAttempts, key) emailVerifyMu.Unlock() } @@ -476,6 +504,18 @@ func LoginHandler(w http.ResponseWriter, r *http.Request) { `, req.Email).Scan(&userID, &passwordHash, &role) if err != nil { + // F2-HIGH (user-existence timing oracle): a non-existent email used to + // return before any bcrypt work, so its latency (~1 DB round trip) was + // measurably shorter than a wrong-password attempt against an existing + // account (~1 DB round trip + ~60ms bcrypt) — an attacker could probe + // which emails are registered from response timing. Burn the same + // constant-time bcrypt compare a real login would, under the shared + // bcrypt slot budget, and discard the result. The dummy hash is a real + // bcrypt hash (see dummyPasswordHash) so the compare runs the full cost. + if release, ok := acquireBcryptSlot(); ok { + _ = bcrypt.CompareHashAndPassword([]byte(dummyPasswordHash), []byte(req.Password)) + release() + } http.Error(w, "invalid credentials", http.StatusUnauthorized) return } @@ -601,25 +641,26 @@ func LoginHandler(w http.ResponseWriter, r *http.Request) { } // On success, clear lockout and update last_login - // TODO: Password reset flow (MVP #4 in Future Work doc) must also clear - // failed_attempts and locked_until — a locked-out user can't call this handler. // // LOW 6 documented gap (finding 6): there is NO password-reset UI — the - // backend-only reset flow (GenerateVerificationCodeHandler/VerifyCodeHandler) - // has no frontend link, so a user locked out by a guessing attacker has no - // self-service recovery until locked_until lapses (15min at 5+ failures, - // 30min at 7+, 60min at 10+ — see the failure path above); the operator can - // only intervene at the DB. The escalating ceiling is the repeat-DoS - // mitigation: an attacker who keeps guessing past each unlock makes the lock - // LONGER (up to 60 minutes) instead of merely sustaining the 15-minute tier, - // raising the effort-per-DoS ratio while the response stays the uniform 401 - // (never distinguishable from a wrong password). The lockout counter stays - // keyed per-user (not per-(user,IP)) because this codebase deliberately - // rejects IP-in-the-key for account-level budgets (see the 2FA limiter note - // in main.go, B8): a client that rotates its source IP would mint a fresh - // bucket per IP and collapse the per-account budget. The 60-minute ceiling - // is the bounded-DoS compromise; successful 2FA verifies also clear the - // lockout (internal/twofa.Check). + // backend-only reset flow (GenerateVerificationCodeHandler/ + // VerifyCodeHandler) has no frontend link, so a user locked out by a + // guessing attacker has no self-service recovery until locked_until lapses + // (15min at 5+ failures, 30min at 7+, 60min at 10+ — see the failure path + // above); the operator can only intervene at the DB. The escalating ceiling + // is the repeat-DoS mitigation: an attacker who keeps guessing past each + // unlock makes the lock LONGER (up to 60 minutes) instead of merely + // sustaining the 15-minute tier, raising the effort-per-DoS ratio while the + // response stays the uniform 401 (never distinguishable from a wrong + // password). The lockout counter stays keyed per-user (not per-(user,IP)) + // because this codebase deliberately rejects IP-in-the-key for account-level + // budgets (see the 2FA limiter note in main.go, B8): a client that rotates + // its source IP would mint a fresh bucket per IP and collapse the per-account + // budget. The 60-minute ceiling is the bounded-DoS compromise; a successful + // 2FA verify also clears the lockout (internal/twofa.Check), and — since + // the HIGH finding wiring — so does a successful password_reset verification + // code (VerifyCodeHandler), giving a locked-out user a self-service recovery + // path (generate → verify → log in → change password). tx, err := db.Conn.Begin(r.Context()) if err != nil { log.Printf("Failed to begin transaction: %v", err) @@ -804,6 +845,11 @@ func LogoutHandler(w http.ResponseWriter, r *http.Request) { type VerificationCodeRequest struct { Email string `json:"email" validate:"required,email,max=254"` + // Purpose is the verification_purpose the code authorises: "email_verify" + // (default, escalates unverified_email → verified_email) or "password_reset" + // (clears a login lockout — see VerifyCodeHandler). Validated in code so the + // comparison is case-insensitive after trim/lower. + Purpose string `json:"purpose,omitempty"` } type VerifyCodeRequest struct { @@ -815,6 +861,37 @@ type VerificationResponse struct { Message string `json:"message,omitempty"` } +// generateVerificationCode returns a 12-hex-character code (48 bits of +// randomness), matching the old DB-default generator +// (gen_random_bytes(6) hex). Only its HMAC-SHA256 digest is ever persisted; the +// plaintext exists solely to be delivered out-of-band (dev [VERIFY] log relay, +// or the future SMTP channel) and is never stored. +func generateVerificationCode() (string, error) { + buf := make([]byte, 6) + if _, err := rand.Read(buf); err != nil { + return "", err + } + return hex.EncodeToString(buf), nil +} + +// POST /api/verify/generate +// Creates a verification_codes row for the account matching the submitted +// email (if any), storing ONLY the HMAC-SHA256 digest of a fresh +// 12-hex-char code (pepper-keyed via crussell/internal/twofa — see +// verificationCodePepperEnv). The plaintext code is delivered build-dependently +// (verifycode_dev.go / verifycode_prod.go): dev/test builds write it to the +// server log ([VERIFY] prefix) — the loose-fake stand-in for the not-yet-wired +// email/SMS transport (P6) — while production builds fail closed when +// TWO_FACTOR_PEPPER is unset (an unsalted digest in the 48-bit code space +// would be offline-brute-forceable from a DB leak) or when no delivery channel +// is configured (email/SMS unwired; stdout-log delivery is a dev/test-only +// local feature). The response is IDENTICAL whether or not the email exists, so +// the endpoint cannot be used to enumerate registered addresses. +// +// The purpose field wires the lockout-recovery flow (HIGH finding): a locked-out +// user requests a password_reset code for their own email, obtains it (dev log / +// operator relay), verifies it at /api/verify/check, and the lockout is cleared +// so they can log in and change their password. func GenerateVerificationCodeHandler(w http.ResponseWriter, r *http.Request) { var req VerificationCodeRequest if err := json.NewDecoder(r.Body).Decode(&req); err != nil { @@ -833,13 +910,39 @@ func GenerateVerificationCodeHandler(w http.ResponseWriter, r *http.Request) { return } + purpose := strings.TrimSpace(strings.ToLower(req.Purpose)) + if purpose == "" { + purpose = verificationCodePurposeEmailVerify + } + if purpose != verificationCodePurposeEmailVerify && purpose != verificationCodePurposePasswordReset { + http.Error(w, "purpose must be 'email_verify' or 'password_reset'", http.StatusBadRequest) + return + } + + // A locked-out user reaches this endpoint UNAUTHENTICATED by design (the + // whole point of password_reset recovery), so no auth middleware guards it; + // the per-IP rate limit on the route is the only throttle, matching the + // 2FA mint paths. + + // Build-dependent issuance gate (pepper + delivery channel in production; + // always allowed in dev/test — see verifycode_dev.go / verifycode_prod.go). + if err := verificationCodeEnsureIssueAllowed(); err != nil { + http.Error(w, err.Error(), http.StatusServiceUnavailable) + return + } + + // Fail-closed reference: the user lookup happens AFTER the issuance gate so + // a prod deployment without the pepper/delivery channel refuses BEFORE any + // per-email work (and before the enumeration-uniform path below is reached). var userID string err := db.Conn.QueryRow(r.Context(), "SELECT id FROM users WHERE LOWER(email) = $1", email, ).Scan(&userID) if err != nil { if errors.Is(err, pgx.ErrNoRows) { - if err := json.NewEncoder(w).Encode(VerificationResponse{Success: true, Message: "If the email exists, a verification code will be sent"}); err != nil { + // Uniform anti-enumeration response — byte-identical to the + // existing-user branch. + if err := json.NewEncoder(w).Encode(VerificationResponse{Success: true, Message: "If the account exists, a verification code has been generated"}); err != nil { log.Printf("Failed to encode JSON response: %v", err) } return @@ -849,24 +952,47 @@ func GenerateVerificationCodeHandler(w http.ResponseWriter, r *http.Request) { return } + code, err := generateVerificationCode() + if err != nil { + log.Printf("Failed to generate verification code: %v", err) + http.Error(w, "internal error", http.StatusInternalServerError) + return + } + expiresAt := clock.Now().Add(24 * time.Hour) - var code string - err = db.Conn.QueryRow(r.Context(), - `INSERT INTO verification_codes (user_id, purpose, expires_at) VALUES ($1, 'email_verify', $2) RETURNING code`, - userID, expiresAt, - ).Scan(&code) + // Persist ONLY the digest; the plaintext code exists only in the delivery + // channel (log relay / future SMTP). + _, err = db.Conn.Exec(r.Context(), + `INSERT INTO verification_codes (user_id, purpose, code, expires_at) VALUES ($1, $2, $3, $4)`, + userID, purpose, twofa.Hash(code), expiresAt, + ) if err != nil { log.Printf("Failed to insert verification code: %v", err) http.Error(w, "internal error", http.StatusInternalServerError) return } - if err := json.NewEncoder(w).Encode(VerificationResponse{Success: true, Message: "Verification code generated"}); err != nil { + verificationCodeDeliver(userID, purpose, code) + + if err := json.NewEncoder(w).Encode(VerificationResponse{Success: true, Message: "If the account exists, a verification code has been generated"}); err != nil { log.Printf("Failed to encode JSON response: %v", err) } } +// POST /api/verify/check +// Consumes a verification code submitted by an UNAUTHENTICATED caller (there is +// no auth middleware on this route — the password_reset recovery flow must be +// reachable by a locked-out user). The submitted code is hashed the same way it +// was stored (twofa.Hash) and matched against verification_codes; a match +// resolves the owning user, and the brute-force attempt budget is keyed PER +// USER from that point on (see the emailVerifyAttempts* docs). On a valid, +// unexpired, unused code: +// +// - purpose email_verify: escalates the account to verified_email; +// - purpose password_reset: clears failed_attempts / locked_until so the +// account owner can log in and change their password (the lockout-recovery +// path for the login-DoS finding). func VerifyCodeHandler(w http.ResponseWriter, r *http.Request) { var req VerifyCodeRequest if err := json.NewDecoder(r.Body).Decode(&req); err != nil { @@ -886,44 +1012,30 @@ func VerifyCodeHandler(w http.ResponseWriter, r *http.Request) { } // Finding 8: a spent attempt budget rejects before any DB work — the code - // can no longer be guessed against. + // can no longer be guessed against. This pre-check uses the submitted code + // as the key (a guess's miss path; the per-user key cannot be derived until + // a row resolves it). if emailVerifyAttemptsExhausted(code) { http.Error(w, "too many attempts. request a new code.", http.StatusTooManyRequests) return } + codeDigest := twofa.Hash(code) + var userID string var purpose string var expiresAt time.Time + var usedAt *time.Time err := db.Conn.QueryRow(r.Context(), - `SELECT user_id, purpose, expires_at FROM verification_codes - WHERE code = $1 AND used_at IS NULL AND expires_at > NOW()`, - code, - ).Scan(&userID, &purpose, &expiresAt) + `SELECT user_id, purpose, expires_at, used_at FROM verification_codes + WHERE code = $1`, + codeDigest, + ).Scan(&userID, &purpose, &expiresAt, &usedAt) if err != nil { if errors.Is(err, pgx.ErrNoRows) { - // Check if code exists but was already used or expired - var checkUsedAt *time.Time - checkErr := db.Conn.QueryRow(r.Context(), - `SELECT used_at FROM verification_codes WHERE code = $1`, code, - ).Scan(&checkUsedAt) - if checkErr != nil { - // Code doesn't exist at all — a guess. Count it against the - // code's attempt budget. - if emailVerifyAttemptFailed(code) { - http.Error(w, "too many attempts. request a new code.", http.StatusTooManyRequests) - return - } - http.Error(w, "invalid or expired code", http.StatusBadRequest) - return - } - // Code exists but was already used — a definite state, not a guess. - if checkUsedAt != nil { - http.Error(w, "code already used", http.StatusForbidden) - return - } - // Code exists but expired — count it against the budget too. + // No row with this digest at all — a guess. No user can be + // resolved, so the attempt budget stays keyed per submitted code. if emailVerifyAttemptFailed(code) { http.Error(w, "too many attempts. request a new code.", http.StatusTooManyRequests) return @@ -936,6 +1048,29 @@ func VerifyCodeHandler(w http.ResponseWriter, r *http.Request) { return } + // The code resolved to exactly one user — from here the attempt budget is + // keyed PER USER, so an attacker draining a victim's codes cannot get a + // fresh budget per submitted value. + if emailVerifyAttemptsExhausted(userID) { + http.Error(w, "too many attempts. request a new code.", http.StatusTooManyRequests) + return + } + + // Code exists but was already used — a definite state, not a guess. + if usedAt != nil { + http.Error(w, "code already used", http.StatusForbidden) + return + } + // Code exists but expired — count it against the user's budget too. + if !expiresAt.After(clock.Now()) { + if emailVerifyAttemptFailed(userID) { + http.Error(w, "too many attempts. request a new code.", http.StatusTooManyRequests) + return + } + http.Error(w, "invalid or expired code", http.StatusBadRequest) + return + } + tx, err := db.Conn.Begin(r.Context()) if err != nil { log.Printf("Failed to start transaction: %v", err) @@ -950,7 +1085,7 @@ func VerifyCodeHandler(w http.ResponseWriter, r *http.Request) { _, err = tx.Exec(r.Context(), `UPDATE verification_codes SET used_at = NOW() WHERE code = $1`, - code, + codeDigest, ) if err != nil { log.Printf("Failed to mark code as used: %v", err) @@ -958,7 +1093,9 @@ func VerifyCodeHandler(w http.ResponseWriter, r *http.Request) { return } - if purpose == "email_verify" { + message := "Email verified successfully" + switch purpose { + case verificationCodePurposeEmailVerify: _, err = tx.Exec(r.Context(), `UPDATE users SET account_role = 'verified_email' WHERE id = $1 AND account_role = 'unverified_email'`, userID, @@ -968,6 +1105,21 @@ func VerifyCodeHandler(w http.ResponseWriter, r *http.Request) { http.Error(w, "internal error", http.StatusInternalServerError) return } + case verificationCodePurposePasswordReset: + // Lockout-recovery consumer (HIGH finding): a verified password_reset + // code proves control of the account's email, so the login lockout is + // lifted. The user then logs in and changes their password via the + // existing change-password flow. + message = "Verification successful - login lockout cleared" + _, err = tx.Exec(r.Context(), + `UPDATE users SET failed_attempts = 0, locked_until = NULL WHERE id = $1`, + userID, + ) + if err != nil { + log.Printf("Failed to clear login lockout for password_reset: %v", err) + http.Error(w, "internal error", http.StatusInternalServerError) + return + } } if err := tx.Commit(r.Context()); err != nil { @@ -976,10 +1128,12 @@ func VerifyCodeHandler(w http.ResponseWriter, r *http.Request) { return } - // Finding 8: a successful verify clears the code's attempt budget. + // A successful verify clears the user's attempt budget (and the miss-path + // key the submitted code used on earlier guesses). + emailVerifyAttemptsClear(userID) emailVerifyAttemptsClear(code) - if err := json.NewEncoder(w).Encode(VerificationResponse{Success: true, Message: "Email verified successfully"}); err != nil { + if err := json.NewEncoder(w).Encode(VerificationResponse{Success: true, Message: message}); err != nil { log.Printf("Failed to encode JSON response: %v", err) } } diff --git a/backend/handlers/auth/verifycode_dev.go b/backend/handlers/auth/verifycode_dev.go new file mode 100644 index 0000000..907b89d --- /dev/null +++ b/backend/handlers/auth/verifycode_dev.go @@ -0,0 +1,30 @@ +//go:build dev || test + +package auth + +// Dev/test builds of the verification-code flow (POST /api/verify/generate): +// the [VERIFY] log line is the LOCAL DEV delivery channel — the stand-in for +// the not-yet-wired email/SMS transport (P6) — and a missing TWO_FACTOR_PEPPER +// still falls back to the legacy unsalted SHA-256 digest (via the shared +// crussell/internal/twofa provider, which logs its own one-time warning). +// Production builds (!dev && !test) never log the code — stdout-log delivery +// is a dev/test-only local feature — and fail closed — see verifycode_prod.go. + +import "log" + +// verificationCodeEnsureIssueAllowed always permits code issuance in dev/test +// builds: the loose-fake delivery (the [VERIFY] log line) is the documented +// stand-in until email/SMS lands. Production builds fail closed here — no +// TWO_FACTOR_PEPPER, no codes (see verifycode_prod.go). +func verificationCodeEnsureIssueAllowed() error { return nil } + +// verificationCodeDeliver delivers a fresh verification code to the user. +// Dev/test: the [VERIFY] log line is the delivery channel — an operator relays +// the code to the user out-of-band until email/SMS lands (mirrors the [2FA] +// log relay, which the 2FA flow uses identically). MEDIUM-3b: the user id and +// the plaintext code go to SEPARATE log lines so a single record cannot trivially +// pair a code with its owner. +func verificationCodeDeliver(userID, purpose, code string) { + log.Printf("[VERIFY] code delivery requested (user=%s, purpose=%s)", userID, purpose) + log.Printf("[VERIFY] code: %s", code) +} diff --git a/backend/handlers/auth/verifycode_prod.go b/backend/handlers/auth/verifycode_prod.go new file mode 100644 index 0000000..769cf9e --- /dev/null +++ b/backend/handlers/auth/verifycode_prod.go @@ -0,0 +1,46 @@ +//go:build !dev && !test + +package auth + +// Production builds (!dev && !test) of the verification-code flow +// (POST /api/verify/generate): code issuance fails closed. The stdout-log +// relay ([VERIFY] prefix) is a DEV/TEST-ONLY local feature — the stand-in for +// the not-yet-wired email/SMS transport (P6) — and is deliberately never used +// in a production build: +// +// - a missing TWO_FACTOR_PEPPER: an unsalted SHA-256 digest in the 48-bit +// code space would be offline-brute-forceable from a log/DB leak; and +// - no delivery channel: there is no production email/SMS transport yet (P6) +// and no production opt-in to log delivery, so a minted code could never +// reach the user. +// +// The plaintext code is therefore never written to the server log in a +// production build, under any configuration. + +import ( + "errors" + "os" +) + +// verificationCodeEnsureIssueAllowed reports whether a verification code may be +// issued in this deployment. Production requires TWO_FACTOR_PEPPER and, after +// that, a real delivery channel — which does not exist until email/SMS lands +// (P6) — so issuance is ALWAYS refused (fail-closed); dev/test builds always +// allow issuance (verifycode_dev.go). +func verificationCodeEnsureIssueAllowed() error { + if os.Getenv(verificationCodePepperEnv) == "" { + return errors.New("verification code issuance requires TWO_FACTOR_PEPPER (an unsalted digest in the 48-bit code space would be offline-brute-forceable); set it in the environment") + } + return errors.New("verification code issuance requires a delivery channel; email/SMS is not wired yet (P6) — production has no delivery channel until it lands") +} + +// verificationCodeDeliver delivers a fresh verification code to the user. +// Production: a deliberate no-op — the plaintext code is NEVER written to the +// server log, so this is unreachable (verificationCodeEnsureIssueAllowed +// already refused issuance). The dev/test build (verifycode_dev.go) writes the +// [VERIFY] log line instead — stdout-log delivery is a dev/test-only local +// feature until email/SMS lands (P6). +func verificationCodeDeliver(userID, purpose, code string) { + // Deliberate no-op: production never logs plaintext codes, under any + // configuration. Delivery is dev/test-only until email/SMS lands (P6). +} diff --git a/backend/handlers/user/gdpr_test.go b/backend/handlers/user/gdpr_test.go index 49cb1c6..6ee9bc1 100644 --- a/backend/handlers/user/gdpr_test.go +++ b/backend/handlers/user/gdpr_test.go @@ -502,8 +502,7 @@ func TestAnonymizeUser_RetainsEditRequestNotes(t *testing.T) { } // TestAnonymizeUser_ScrubsAdminAuditLog2FAFallback closes the GDPR erasure gap -// for admin_audit_log: a '2fa_fallback_charge' row (insertTwoFAFallbackAudit, -// handlers/payments) carries target_user_id = the erased user, admin_id = the +// for admin_audit_log: a '2fa_fallback_charge' row (written by handlers/payments) carries target_user_id = the erased user, admin_id = the // customer's own userID (the CIT actor), AND details.card_last4 — the audit row // MUST survive erasure (GDPR Art 30 records of processing / financial audit // trail) but be de-identified: the user links (both target_user_id and @@ -518,8 +517,8 @@ func TestAnonymizeUser_ScrubsAdminAuditLog2FAFallback(t *testing.T) { } // A 2FA-fallback audit row for a CIT saved-card charge: target_user_id is - // the customer and admin_id is the customer's own userID (the CIT actor — - // see insertTwoFAFallbackAudit). details carries the card_last4 PII. + // the customer and admin_id is the customer's own userID (the CIT actor). + // details carries the card_last4 PII. var auditID string err = tx.QueryRow(ctx, ` INSERT INTO admin_audit_log (admin_id, action_type, target_user_id, details) diff --git a/backend/handlers/user/guest.go b/backend/handlers/user/guest.go index bbe7ea7..58d46bd 100644 --- a/backend/handlers/user/guest.go +++ b/backend/handlers/user/guest.go @@ -150,10 +150,21 @@ func CreateGuestUserHandler(w http.ResponseWriter, r *http.Request) { } } -// GET /api/check-email?email=...&firstName=...&lastName=...&phone=... -// Returns { suggestion: "login" | "check" | null } based on whether the email -// belongs to a registered user and how closely the provided details match. -// Relies on nginx restricting access to frontend-only traffic. +// GET /api/check-email?email=... +// Returns a uniform {"available": bool} response: available=false when the +// email belongs to a registered (non-guest) account, true otherwise. The old +// response returned a "suggestion" breakdown ("login" | "check" | null) that +// told an unauthenticated caller whether an email was registered AND whether +// their first/last-name/phone matched the account — a user-enumeration and +// PII-confirmation oracle. The comment here previously claimed nginx restricts +// this endpoint to frontend-only traffic, but nginx/conf.d/default.conf has NO +// such rule (the /api/ location proxies everything with rate limiting only), so +// the handler itself must not leak the breakdown. The uniform shape keeps the +// endpoint functional for the registration form's "email already registered" +// check while removing the distinguishing detail. The guest-booking frontend +// reads data.suggestion; with the uniform shape it resolves to null and no +// suggestion banner is shown — an accepted UX trade-off (the guest-creation +// endpoint's 409 is the real enforcement for registered emails). func CheckEmailHandler(w http.ResponseWriter, r *http.Request) { email := strings.ToLower(strings.TrimSpace(r.URL.Query().Get("email"))) if email == "" { @@ -167,35 +178,16 @@ func CheckEmailHandler(w http.ResponseWriter, r *http.Request) { return } - firstName := strings.TrimSpace(r.URL.Query().Get("firstName")) - lastName := strings.TrimSpace(r.URL.Query().Get("lastName")) - phone := strings.TrimSpace(r.URL.Query().Get("phone")) - - var dbFirstName, dbLastName, dbPhone *string + var registeredID string err := db.Conn.QueryRow(r.Context(), ` - SELECT n_first_name, n_last_name, phone + SELECT id FROM users WHERE email = $1 AND account_role != 'guest' - `, email).Scan(&dbFirstName, &dbLastName, &dbPhone) + `, email).Scan(®isteredID) - var suggestion *string + available := true if err == nil { - matchesNames := firstName != "" && lastName != "" && - dbFirstName != nil && dbLastName != nil && - strings.EqualFold(firstName, *dbFirstName) && - strings.EqualFold(lastName, *dbLastName) - - matchesPhone := phone != "" && - dbPhone != nil && - phone == *dbPhone - - if matchesNames && matchesPhone { - s := "login" - suggestion = &s - } else { - s := "check" - suggestion = &s - } + available = false } else if !errors.Is(err, pgx.ErrNoRows) { log.Printf("Failed to check email: %v", err) http.Error(w, "database error", http.StatusInternalServerError) @@ -203,7 +195,7 @@ func CheckEmailHandler(w http.ResponseWriter, r *http.Request) { } if err := json.NewEncoder(w).Encode(map[string]any{ - "suggestion": suggestion, + "available": available, }); err != nil { log.Printf("Failed to encode JSON response: %v", err) } diff --git a/backend/handlers/user/guest_test.go b/backend/handlers/user/guest_test.go index 941c185..2593ce1 100644 --- a/backend/handlers/user/guest_test.go +++ b/backend/handlers/user/guest_test.go @@ -121,7 +121,9 @@ func TestGuestUser_Create_InvalidEmail(t *testing.T) { } } -// TestCheckEmail_NotRegistered verifies that querying a non-existent email returns suggestion null. +// TestCheckEmail_NotRegistered verifies that querying a non-existent email +// returns the uniform response {"available": true} and no suggestion breakdown +// (the old "suggestion" field was a user-enumeration/PII-confirmation oracle). func TestCheckEmail_NotRegistered(t *testing.T) { t.Parallel() @@ -139,14 +141,20 @@ func TestCheckEmail_NotRegistered(t *testing.T) { t.Fatalf("failed to unmarshal response: %v", err) } - if resp["suggestion"] != nil { - t.Errorf("expected suggestion null, got %v", resp["suggestion"]) + if _, ok := resp["suggestion"]; ok { + t.Errorf("expected NO 'suggestion' field (enumeration oracle removed), got %v", resp["suggestion"]) + } + available, ok := resp["available"].(bool) + if !ok || !available { + t.Errorf("expected available=true for an unregistered email, got %v", resp["available"]) } } -// TestCheckEmail_Registered_MatchingDetails verifies that querying an existing registered user's email -// with matching first name, last name, and phone returns suggestion "login". -func TestCheckEmail_Registered_MatchingDetails(t *testing.T) { +// TestCheckEmail_Registered verifies that a registered user's email returns the +// uniform response {"available": false} regardless of whether the caller's +// first/last-name/phone match the account — the detail matching is deliberately +// gone so an unauthenticated caller cannot confirm PII against the database. +func TestCheckEmail_Registered(t *testing.T) { t.Parallel() ctx, tx := testutils.SetupTestTx(t) @@ -162,68 +170,37 @@ func TestCheckEmail_Registered_MatchingDetails(t *testing.T) { t.Fatalf("failed to update user name: %v", err) } - req := httptest.NewRequest(http.MethodGet, `/api/check-email?email=jane@example.com&firstName=Jane&lastName=Doe&phone=%2B447123456789`, nil) - req = req.WithContext(ctx) - rr := httptest.NewRecorder() - CheckEmailHandler(rr, req) + // Matching details AND non-matching details must both yield available=false. + for _, q := range []string{ + `/api/check-email?email=jane@example.com&firstName=Jane&lastName=Doe&phone=%2B447123456789`, + `/api/check-email?email=jane@example.com&firstName=Wrong&lastName=Doe&phone=%2B447123456789`, + } { + req := httptest.NewRequest(http.MethodGet, q, nil) + req = req.WithContext(ctx) + rr := httptest.NewRecorder() + CheckEmailHandler(rr, req) - if rr.Code != http.StatusOK { - t.Errorf("expected status 200, got %d", rr.Code) - t.Logf("response body: %s", rr.Body.String()) - } + if rr.Code != http.StatusOK { + t.Errorf("expected status 200, got %d", rr.Code) + t.Logf("response body: %s", rr.Body.String()) + } - var resp map[string]interface{} - if err := json.Unmarshal(rr.Body.Bytes(), &resp); err != nil { - t.Fatalf("failed to unmarshal response: %v", err) - } - - suggestion, ok := resp["suggestion"].(string) - if !ok || suggestion != "login" { - t.Errorf("expected suggestion 'login', got %v", resp["suggestion"]) + var resp map[string]interface{} + if err := json.Unmarshal(rr.Body.Bytes(), &resp); err != nil { + t.Fatalf("failed to unmarshal response: %v", err) + } + if _, ok := resp["suggestion"]; ok { + t.Errorf("expected NO 'suggestion' field (enumeration oracle removed), got %v", resp["suggestion"]) + } + available, ok := resp["available"].(bool) + if !ok || available { + t.Errorf("expected available=false for a registered email (query %s), got %v", q, resp["available"]) + } } } -// TestCheckEmail_Registered_PartialMatch verifies that when the email exists but details don't fully match, -// the handler returns suggestion "check". -func TestCheckEmail_Registered_PartialMatch(t *testing.T) { - t.Parallel() - ctx, tx := testutils.SetupTestTx(t) - - userID, err := fixtures.CreateTestUserWithEmail(tx, "jane@example.com", "verified_email") - if err != nil { - t.Fatalf("failed to create test user: %v", err) - } - - _, err = tx.Exec(ctx, ` - UPDATE users SET n_first_name = 'Jane', n_last_name = 'Doe' WHERE id = $1 - `, userID) - if err != nil { - t.Fatalf("failed to update user name: %v", err) - } - - req := httptest.NewRequest(http.MethodGet, `/api/check-email?email=jane@example.com&firstName=Wrong&lastName=Doe&phone=%2B447123456789`, nil) - req = req.WithContext(ctx) - rr := httptest.NewRecorder() - CheckEmailHandler(rr, req) - - if rr.Code != http.StatusOK { - t.Errorf("expected status 200, got %d", rr.Code) - t.Logf("response body: %s", rr.Body.String()) - } - - var resp map[string]interface{} - if err := json.Unmarshal(rr.Body.Bytes(), &resp); err != nil { - t.Fatalf("failed to unmarshal response: %v", err) - } - - suggestion, ok := resp["suggestion"].(string) - if !ok || suggestion != "check" { - t.Errorf("expected suggestion 'check', got %v", resp["suggestion"]) - } -} - -// TestCheckEmail_GuestUser verifies that a guest user's email is treated as not found -// (suggestion null) because the query excludes account_role = 'guest'. +// TestCheckEmail_GuestUser verifies that a guest user's email is treated as +// available (the query excludes account_role = 'guest'). func TestCheckEmail_GuestUser(t *testing.T) { t.Parallel() ctx, tx := testutils.SetupTestTx(t) @@ -248,8 +225,9 @@ func TestCheckEmail_GuestUser(t *testing.T) { t.Fatalf("failed to unmarshal response: %v", err) } - if resp["suggestion"] != nil { - t.Errorf("expected suggestion null for guest user, got %v", resp["suggestion"]) + available, ok := resp["available"].(bool) + if !ok || !available { + t.Errorf("expected available=true for a guest email, got %v", resp["available"]) } } diff --git a/backend/handlers/user/twofa.go b/backend/handlers/user/twofa.go index b6f7a58..345610b 100644 --- a/backend/handlers/user/twofa.go +++ b/backend/handlers/user/twofa.go @@ -53,21 +53,15 @@ const twoFAPepperEnv = "TWO_FACTOR_PEPPER" // errTwoFADeliveryUnavailable is returned by production builds when a 2FA code // is requested but no delivery channel is configured: the email/SMS transport -// is not wired yet (P6) and the operator has not opted into the insecure -// log-delivery mode (TWO_FACTOR_ALLOW_LOG_DELIVERY=true). Handlers surface it -// verbatim so setup fails loudly with an actionable message instead of issuing -// a code that could never reach the user (which would silently dead-end the -// enforced saved-card-payments gate). Dev/test builds always have the [2FA] log -// channel and never return it (see twofa_dev.go). +// is not wired yet (P6), and stdout-log delivery is a dev/test-only LOCAL +// feature — a production build deliberately has no channel at all (see +// twofa_prod.go). Handlers surface it verbatim so setup fails loudly with an +// actionable message instead of issuing a code that could never reach the user +// (which would silently dead-end the enforced saved-card-payments gate). +// Dev/test builds always have the [2FA] log channel and never return it (see +// twofa_dev.go). var errTwoFADeliveryUnavailable = errors.New("2FA requires an email or SMS delivery channel; contact the salon") -// twoFAAllowLogDeliveryEnv is the explicit operator opt-in that makes a -// production build deliver 2FA codes via the server log ([2FA] prefix) — the -// documented INSECURE stand-in for the not-yet-wired email/SMS transport (P6). -// Production builds fail closed without it (see twoFAEnsureIssueAllowedStrict); -// dev/test builds always deliver via the log and never consult this flag. -const twoFAAllowLogDeliveryEnv = "TWO_FACTOR_ALLOW_LOG_DELIVERY" - // errTwoFAPepperRequired is returned when TWO_FACTOR_PEPPER is unset in a // production-style issuance gate. Refusing to issue is the only safe outcome: // without the pepper a pending code would be persisted as an unsalted SHA-256 @@ -80,24 +74,25 @@ var errTwoFAPepperRequired = errors.New("TWO_FACTOR_PEPPER is not set; refusing // production-style issuance refuses instead (twoFAEnsureIssueAllowedStrict). func twoFAPepperConfigured() bool { return os.Getenv(twoFAPepperEnv) != "" } -// twoFADeliveryChannelConfigured reports whether the deployment has explicitly -// configured a 2FA code delivery channel: TWO_FACTOR_ALLOW_LOG_DELIVERY set to -// exactly "true" (the only production channel today — email/SMS unwired, P6). -// Pure env read, build-agnostic: the build-tagged twoFADeliveryAvailable -// (twofa_dev.go / twofa_prod.go) is the runtime-facing wrapper that turns this -// into the always-true dev channel or the prod env check. -func twoFADeliveryChannelConfigured() bool { return os.Getenv(twoFAAllowLogDeliveryEnv) == "true" } +// twoFADeliveryChannelConfigured reports whether a REAL delivery channel +// exists. Production: always false — email/SMS is not wired yet (P6) and the +// stdout-log relay is a dev/test-only local feature (twofa_dev.go), never a +// production channel. There is deliberately NO production opt-in to log +// delivery. The build-tagged twoFADeliveryAvailable (twofa_dev.go / +// twofa_prod.go) is the runtime-facing wrapper: always true in dev/test, +// always false in production. +func twoFADeliveryChannelConfigured() bool { return false } // twoFAEnsureIssueAllowedStrict is the pure, build-agnostic production-style // issuance gate: code issuance is allowed ONLY when BOTH TWO_FACTOR_PEPPER is // set (an unsalted digest in the 1M code space would be offline-brute-forceable) -// AND a delivery channel is configured (otherwise a minted code could never -// reach the user and would silently dead-end the enforced saved-card-payments -// gate). Either way it fails closed with the actionable errors the handlers map -// to a 503. The build-tagged twoFAEnsureIssueAllowed wraps it for production -// builds; dev/test builds always allow issuance and never consult it — but the -// test,dev suite exercises THIS function directly, so the fail-closed branches -// are CI-visible even though the prod file (!dev && !test) is excluded there. +// AND a real delivery channel exists — which a production build never has until +// email/SMS lands (P6). Either way it fails closed with the actionable errors +// the handlers map to a 503. The build-tagged twoFAEnsureIssueAllowed wraps it +// for production builds; dev/test builds always allow issuance and never +// consult it — but the test,dev suite exercises THIS function directly, so the +// fail-closed branches are CI-visible even though the prod file (!dev && !test) +// is excluded there. func twoFAEnsureIssueAllowedStrict() error { if !twoFAPepperConfigured() { return errTwoFAPepperRequired @@ -156,11 +151,10 @@ func twoFAMintThrottled(st *twoFAAttemptState, now time.Time) bool { // builds fail closed up front: twoFAEnsureIssueAllowed refuses to issue a code // when TWO_FACTOR_PEPPER is unset (an unsalted digest would be // offline-brute-forceable) or when no delivery channel is configured (email/SMS -// unwired and log delivery not explicitly opted into via -// TWO_FACTOR_ALLOW_LOG_DELIVERY=true) — so a production setup never mints a -// code that could never reach the user. The API response still only returns the -// code when 2FA is unenforced (dev convenience). purpose labels the delivery -// (e.g. "setup", "disable 2FA"). +// unwired; stdout-log delivery is a dev/test-only local feature) — so a +// production setup never mints a code that could never reach the user. The API +// response still only returns the code when 2FA is unenforced (dev +// convenience). purpose labels the delivery (e.g. "setup", "disable 2FA"). // // A fresh code does NOT reset the per-user failed-attempt counter (B11b): only // a successful verify does. Resetting on re-mint would let a password-only @@ -198,11 +192,10 @@ func deliverTwoFACode(r *http.Request, userID, method, purpose string) (string, if label == "" { label = purpose } - // Build-dependent delivery: dev/test logs the plaintext code ([2FA] line); - // production logs it ONLY when the operator explicitly opted into log - // delivery (TWO_FACTOR_ALLOW_LOG_DELIVERY=true) — otherwise issuance was - // already refused by twoFAEnsureIssueAllowed above, so the default is that - // the code never reaches a log. + // Build-dependent delivery: dev/test logs the plaintext code ([2FA] line — + // a LOCAL DEV feature); production never logs it (twoFADeliverCode is a + // no-op there), and issuance was already refused by twoFAEnsureIssueAllowed + // above because production has no delivery channel until email/SMS lands. twoFADeliverCode(userID, label, code) return code, nil } @@ -251,10 +244,10 @@ type TwoFASetupRequest struct { // Generates a verification code and stores only its SHA-256 hash plus a // 10-minute expiry in the pending columns. Delivery is build-dependent (see // deliverTwoFACode): dev/test builds log the code with a [2FA] prefix — the -// loose-fake stand-in for the not-yet-wired email/SMS transport (P6) — while +// local-dev stand-in for the not-yet-wired email/SMS transport (P6) — while // production builds fail closed when TWO_FACTOR_PEPPER is unset or when no -// delivery channel is configured (email/SMS unwired and -// TWO_FACTOR_ALLOW_LOG_DELIVERY=true unset), returning a clear actionable +// delivery channel is configured (email/SMS unwired; stdout-log delivery is a +// dev/test-only local feature), returning a clear actionable // error instead of silently issuing a code that would never arrive. When 2FA is // not enforced (dev), the code is also returned in the response so the flow is // testable without reading backend logs. @@ -526,9 +519,9 @@ type TwoFADisableRequest struct { // mints are throttled per-user (twoFAMintCooldown), so a password-only attacker // cannot loop request-code → burn 5 guesses → request-code forever; a throttled // request returns 429. Like the disable handler, no code is returned in the -// response (delivery is the [2FA] log line in dev/test builds; production -// fails closed when no delivery channel is configured — no email/SMS and no -// explicit TWO_FACTOR_ALLOW_LOG_DELIVERY opt-in), and unlike setup this +// response (delivery is the [2FA] log line in dev/test builds only — a local +// dev feature; production fails closed when no delivery channel is configured +// — no email/SMS and no production log channel), and unlike setup this // endpoint runs unconditionally — it does not short-circuit on // !twoFARequired(), so dev environments can exercise the same step (the mint // is harmless there). @@ -587,8 +580,8 @@ func SendDisableCodeHandler(w http.ResponseWriter, r *http.Request) { // was reused (LOW 5). A 200 with a reused code must NOT be read as "a new code // was sent": the frontend should use the already-delivered code and show the // countdown. 409 when the user has not enabled 2FA; 429 on the mint cooldown; -// 503 when no delivery channel is configured (production without -// TWO_FACTOR_ALLOW_LOG_DELIVERY=true); 500 on DB failure. The route is +// 503 when no delivery channel is configured (production — no email/SMS and +// stdout-log delivery is dev/test-only); 500 on DB failure. The route is // mounted with RequireAuth + RequireNonGuest + the shared per-user 2FA limiter // (plus the group's per-IP limiter), so an enabled user cannot hammer code // requests faster than the surface budget. @@ -739,9 +732,9 @@ func AdminSendVerificationCodeHandler(w http.ResponseWriter, r *http.Request) { // and delivered via the build-dependent delivery channel (see deliverTwoFACode) // when no valid pending code exists, and the submitted code is checked under the // shared 5-attempt lockout (wrong code → 400, lockout → 429); only a correct -// code clears the flag. When no delivery channel is configured (production -// without email/SMS and without the explicit TWO_FACTOR_ALLOW_LOG_DELIVERY -// opt-in), the mint fails loudly with the actionable setup error instead of a +// code clears the flag. When no delivery channel is configured (production — +// email/SMS unwired and stdout-log delivery is dev/test-only), the mint fails +// loudly with the actionable setup error instead of a // silent 500. Fresh-code mints are throttled per-user (twoFAMintCooldown) so // the loop above cannot reset the lockout faster than once per cooldown. In // unenforced (dev) environments the loose behavior is kept: no code required, diff --git a/backend/handlers/user/twofa_dev.go b/backend/handlers/user/twofa_dev.go index 2609dbc..f6ff400 100644 --- a/backend/handlers/user/twofa_dev.go +++ b/backend/handlers/user/twofa_dev.go @@ -2,12 +2,13 @@ package user -// Dev/test builds (the `dev` tag, or any build with the `test` tag) keep the -// documented loose-fake 2FA delivery: the plaintext code is written to the -// server log ([2FA] prefix) as the stand-in for the not-yet-wired email/SMS -// transport (P6), and a missing TWO_FACTOR_PEPPER still falls back to the -// legacy unsalted SHA-256 digest. Production builds (!dev && !test) instead -// never log the code and fail closed without the pepper — see twofa_prod.go. +// Dev/test builds (the `dev` tag, or any build with the `test` tag) deliver 2FA +// codes to the LOCAL DEV stdout log ([2FA] prefix) as the stand-in for the +// not-yet-wired email/SMS transport (P6), and a missing TWO_FACTOR_PEPPER still +// falls back to the legacy unsalted SHA-256 digest. Production builds +// (!dev && !test) instead NEVER log the code — log delivery is a dev/test-only +// local feature, never a production channel — and fail closed without the +// pepper — see twofa_prod.go. import ( "crussell/internal/twofa" @@ -45,10 +46,10 @@ func init() { func twoFAEnsureIssueAllowed() error { return nil } // twoFADeliverCode delivers a fresh verification code to the user. Dev/test: -// the [2FA] log line is the delivery channel — an operator relays the code to -// the user out-of-band until email/SMS lands. Production builds log it ONLY -// when the operator explicitly opts in via TWO_FACTOR_ALLOW_LOG_DELIVERY=true -// (see twofa_prod.go); otherwise they refuse issuance up front. +// the [2FA] log line is the LOCAL DEV delivery channel — an operator (or the +// developer) relays the code to the user out-of-band until email/SMS lands +// (P6). Production builds NEVER log it (see twofa_prod.go) and refuse issuance +// up front — stdout-log delivery is a dev/test-only feature. // // MEDIUM-3b: the user id and the plaintext code are written to SEPARATE log // lines so a log line cannot trivially pair a code with its owner. The two diff --git a/backend/handlers/user/twofa_failclosed_test.go b/backend/handlers/user/twofa_failclosed_test.go index 8bf0c4a..c50a43e 100644 --- a/backend/handlers/user/twofa_failclosed_test.go +++ b/backend/handlers/user/twofa_failclosed_test.go @@ -11,6 +11,11 @@ package user // those branches in the STANDARD test,dev run, so a regression in the prod // fail-closed behaviour is CI-visible even though the prod file itself is only // compiled in a genuine production build. +// +// DELIVERY POSTURE (current): stdout-log delivery of 2FA codes is a +// DEV/TEST-ONLY local feature. Production has no delivery channel of any kind +// (email/SMS unwired, P6; no production opt-in to log delivery), so the strict +// gate refuses issuance unconditionally after the pepper check. import ( "testing" @@ -22,39 +27,29 @@ import ( // issuance gate that twofa_prod.go's twoFAEnsureIssueAllowed delegates to: // (a) pepper unset → issuance refused (errTwoFAPepperRequired — an unsalted // digest in the 1M code space would be offline-brute-forceable); -// (b) delivery channel absent → issuance refused (errTwoFADeliveryUnavailable — -// the 503-style error the handlers surface as StatusServiceUnavailable); -// (c) both configured → issuance succeeds. +// (b) pepper set but no delivery channel → issuance STILL refused +// (errTwoFADeliveryUnavailable — production has no channel until email/SMS +// lands, P6; the 503-style error the handlers surface as +// StatusServiceUnavailable). Issuance can never succeed in a production build +// until a real transport exists. func TestTwoFAEnsureIssueAllowedStrict_FailClosed(t *testing.T) { t.Run("pepper_unset_refuses_issuance", func(t *testing.T) { t.Setenv(twoFAPepperEnv, "") - t.Setenv(twoFAAllowLogDeliveryEnv, "true") require.ErrorIs(t, twoFAEnsureIssueAllowedStrict(), errTwoFAPepperRequired) }) - t.Run("delivery_channel_absent_refuses_issuance", func(t *testing.T) { + t.Run("no_delivery_channel_refuses_issuance", func(t *testing.T) { t.Setenv(twoFAPepperEnv, "test-pepper") - t.Setenv(twoFAAllowLogDeliveryEnv, "") require.ErrorIs(t, twoFAEnsureIssueAllowedStrict(), errTwoFADeliveryUnavailable) }) - - t.Run("pepper_and_channel_present_allows_issuance", func(t *testing.T) { - t.Setenv(twoFAPepperEnv, "test-pepper") - t.Setenv(twoFAAllowLogDeliveryEnv, "true") - require.NoError(t, twoFAEnsureIssueAllowedStrict()) - }) } -// TestTwoFADeliveryChannelConfigured pins the pure delivery-channel predicate -// behind the 503 refusal: only the exact value "true" opens the channel. +// TestTwoFADeliveryChannelConfigured pins the pure delivery-channel predicate: +// production has NO delivery channel — stdout-log delivery is a dev/test-only +// local feature and there is no production opt-in — so it is always false. func TestTwoFADeliveryChannelConfigured(t *testing.T) { t.Setenv(twoFAPepperEnv, "test-pepper") - for _, v := range []string{"", "1", "yes", "on", "True", "TRUE", "false"} { - t.Setenv(twoFAAllowLogDeliveryEnv, v) - require.False(t, twoFADeliveryChannelConfigured(), "value %q must NOT open the delivery channel (exact 'true' only)", v) - } - t.Setenv(twoFAAllowLogDeliveryEnv, "true") - require.True(t, twoFADeliveryChannelConfigured()) + require.False(t, twoFADeliveryChannelConfigured()) } // TestTwoFAPepperConfigured pins the pure pepper predicate behind the diff --git a/backend/handlers/user/twofa_prod.go b/backend/handlers/user/twofa_prod.go index ce572a8..01f06e1 100644 --- a/backend/handlers/user/twofa_prod.go +++ b/backend/handlers/user/twofa_prod.go @@ -3,37 +3,33 @@ package user // Production builds (neither the `dev` nor the `test` tag) must never persist -// an unsalted digest and must never write a 2FA code in plaintext by default: -// the plaintext [2FA] log delivery and the TWO_FACTOR_PEPPER fallback exist -// only in dev/test builds (twofa_dev.go). Here code issuance fails closed on -// BOTH missing configuration pieces: +// an unsalted digest and must never write a 2FA code in plaintext: the +// plaintext [2FA] log delivery exists ONLY in dev/test builds (twofa_dev.go) +// as a LOCAL-DEV stand-in until the email/SMS transport is wired (P6). In a +// production build there is NO delivery channel of any kind, so code issuance +// fails closed unconditionally: // // - a missing TWO_FACTOR_PEPPER (an unsalted digest in the 1M code space // would be offline-brute-forceable from a log/DB leak), mirroring how // main.go refuses to start without a strong JWT_SECRET_KEY; and -// - a missing delivery channel. The email/SMS transport is not wired yet -// (P6), so the ONLY production channel is the operator's explicit opt-in -// to the insecure log-delivery mode (TWO_FACTOR_ALLOW_LOG_DELIVERY=true). -// Without it, issuing a code would silently dead-end setup — the user -// could never receive the code and the enforced saved-card-payments gate -// would lock them out with no way forward. Issuance is refused and the -// handlers surface errTwoFADeliveryUnavailable ("2FA requires an email or -// SMS delivery channel; contact the salon"). +// - no delivery channel by definition — email/SMS is not wired yet (P6) and +// stdout-log delivery is a dev/test-only convenience, never a production +// channel. There is deliberately NO production opt-in to log delivery: +// writing plaintext codes to a server log anyone with backend access can +// read would defeat the account-verification 2FA gate, and issuing a code +// that can never reach the user would silently dead-end setup. Issuance is +// refused and the handlers surface errTwoFADeliveryUnavailable ("2FA +// requires an email or SMS delivery channel; contact the salon") until a +// real transport lands. // -// The plaintext code is therefore never written to the server log unless the -// operator explicitly opted into log delivery and accepted its risk. +// The plaintext code is therefore NEVER written to the server log in a +// production build, under any configuration. import ( "crussell/internal/twofa" - "log" "os" ) -// twoFAAllowLogDeliveryEnv (the TWO_FACTOR_ALLOW_LOG_DELIVERY opt-in) and -// errTwoFAPepperRequired are defined in twofa.go — shared by the pure issuance -// gate (twoFAEnsureIssueAllowedStrict), which the test,dev suite exercises -// directly, and this production build. - // init registers the production pepper reader into the shared verification // core (crussell/internal/twofa): raw env read, no fallback — code issuance // fails closed via twoFAEnsureIssueAllowed, so no pending code is ever @@ -43,33 +39,28 @@ func init() { } // twoFADeliveryAvailable reports whether a 2FA code delivery channel exists in -// this build. Production: true only when the operator explicitly opted into the -// insecure log-delivery mode (TWO_FACTOR_ALLOW_LOG_DELIVERY=true) or a real -// email/SMS transport is wired (not yet — P6). Default false: no channel, so -// code issuance is refused and setup surfaces errTwoFADeliveryUnavailable -// instead of a silent dead-end. Delegates to the pure build-agnostic -// twoFADeliveryChannelConfigured (twofa.go); dev/test builds always return true -// (twofa_dev.go). -func twoFADeliveryAvailable() bool { - return twoFADeliveryChannelConfigured() -} +// this build. Production: always false — email/SMS is not wired (P6) and +// stdout-log delivery is a dev/test-only local feature (twofa_dev.go), never a +// production channel. Default false: no channel, so code issuance is refused +// and setup surfaces errTwoFADeliveryUnavailable instead of a silent dead-end. +func twoFADeliveryAvailable() bool { return false } // twoFAEnsureIssueAllowed reports whether a 2FA code may be issued in this -// deployment. Production requires BOTH a delivery channel and TWO_FACTOR_PEPPER: -// without a channel (no email/SMS, no TWO_FACTOR_ALLOW_LOG_DELIVERY=true) the -// code could never reach the user — issuing one would silently lock the user -// out of the enforced saved-card-payments gate; and without the pepper every -// stored code would be an offline-brute-forceable unsalted digest. Either way -// issuance is refused (fail-closed). Delegates to the pure build-agnostic gate -// twoFAEnsureIssueAllowedStrict (twofa.go), which the test,dev suite also -// exercises directly; dev/test builds always allow issuance (twofa_dev.go). +// deployment. Production requires TWO_FACTOR_PEPPER and, after that, a real +// delivery channel — which does not exist until email/SMS lands (P6), so +// issuance is ALWAYS refused (fail-closed): without a channel a code could +// never reach the user and would silently lock them out of the enforced +// saved-card-payments gate, and without the pepper every stored code would be +// an offline-brute-forceable unsalted digest. Delegates to the pure +// build-agnostic gate twoFAEnsureIssueAllowedStrict (twofa.go), which the +// test,dev suite also exercises directly; dev/test builds always allow +// issuance (twofa_dev.go). // -// The pepper check is the ONLY hard gate here (plus the delivery channel), and -// it is also the ONLY hard gate on the payments re-issue path -// (payments.twoFAReissueIssueAllowed). PEPPER-CHANGE HAZARD (Loop B finding 2): -// the pepper keys the HMAC-SHA256 of every stored pending-code hash, so -// CHANGING TWO_FACTOR_PEPPER invalidates ALL pending codes — every stored hash -// was computed with the old pepper and can never match a code minted under the +// The pepper check is the ONLY hard gate here (plus the always-absent +// delivery channel). PEPPER-CHANGE HAZARD (Loop B finding 2): the pepper keys +// the HMAC-SHA256 of every stored pending-code hash, so CHANGING +// TWO_FACTOR_PEPPER invalidates ALL pending codes — every stored hash was +// computed with the old pepper and can never match a code minted under the // new one. An operator who changes the pepper must re-mint every user's code // (or have each user re-run 2FA setup), or enforced saved-card charges will // strand customers with 400 ErrMissingOrExpired forever. @@ -77,21 +68,12 @@ func twoFAEnsureIssueAllowed() error { return twoFAEnsureIssueAllowedStrict() } -// twoFADeliverCode delivers a fresh verification code to the user. Production -// has no wired email/SMS transport (P6), so the ONLY channel is the operator's -// explicit, insecure opt-in to log delivery (TWO_FACTOR_ALLOW_LOG_DELIVERY=true -// — anyone with backend log access could defeat the 2FA gate on saved-card -// charges). WITHOUT that flag the plaintext code is NEVER written to the log; -// twoFAEnsureIssueAllowed already refused issuance, so this no-op is -// unreachable. With the flag set, the code is written to the [2FA] log line -// and an operator relays it to the user out-of-band, exactly like the -// documented dev flow — the operator has accepted the risk of log-based -// delivery. MEDIUM-3b: the user id and the plaintext code go to SEPARATE log -// lines so a single record cannot trivially pair a code with its owner. +// twoFADeliverCode delivers a fresh verification code to the user. Production: +// a deliberate no-op — there is no delivery channel (email/SMS unwired, P6) +// and the plaintext code is NEVER written to the server log, so this is +// unreachable (twoFAEnsureIssueAllowed already refused issuance). The +// dev/test build (twofa_dev.go) writes the [2FA] log line instead. func twoFADeliverCode(userID, label, code string) { - if os.Getenv(twoFAAllowLogDeliveryEnv) == "true" { - log.Printf("[2FA] code delivery requested (user=%s, purpose=%s)", userID, label) - log.Printf("[2FA] code: %s", code) - } - // Otherwise: deliberate no-op — never log the plaintext code by default. + // Deliberate no-op: production never logs plaintext codes, under any + // configuration. Delivery is dev/test-only until email/SMS lands (P6). } diff --git a/backend/handlers/user/twofa_prod_test.go b/backend/handlers/user/twofa_prod_test.go index a21855b..3c00eba 100644 --- a/backend/handlers/user/twofa_prod_test.go +++ b/backend/handlers/user/twofa_prod_test.go @@ -13,67 +13,65 @@ package user // their assertions ONLY when the prod variant marker reports the real prod // functions are live; under the test tag they skip with the same documented // rationale the payments package uses (twofa_delivery_prod_test.go). +// +// CLOSING THE GAP: run-prod-tag-tests.sh (backend/) runs `go test -tags +// "!dev,!test" ./handlers/user/` — the ONLY build configuration where +// twofa_prod.go compiles AND twofaProdVariant is true, so the assertions below +// actually execute there. The `if !twofaProdVariant { t.Skip(...) }` guards +// MUST stay: under the CI "test,!dev" matrix the dev/test variants are still +// the compiled functions (the `test` tag matches `dev || test`), so without +// the guards those runs would FAIL rather than skip. +// +// DELIVERY POSTURE (current): stdout-log delivery of 2FA codes is a +// DEV/TEST-ONLY local feature. A production build has NO delivery channel of +// any kind — email/SMS is not wired yet (P6) and there is deliberately no +// production opt-in to log delivery — so code issuance fails closed +// unconditionally (after the pepper check) and twoFADeliveryAvailable is +// always false. import ( "os" "testing" ) -// twoFAAllowLogDeliveryEnv is only defined in twofa_prod.go (!dev && !test); -// use the literal env name so this test also compiles under `test,!dev`. -const allowLogDeliveryEnv = "TWO_FACTOR_ALLOW_LOG_DELIVERY" - // TestTwoFAEnsureIssueAllowed_ProdPredicate pins the production issuance gate: // it fails closed without TWO_FACTOR_PEPPER (an unsalted digest in the 1M code -// space would be offline-brute-forceable) or without a delivery channel, and -// allows issuance only when both are configured. +// space would be offline-brute-forceable) and, with the pepper set, STILL fails +// closed because a production build has no delivery channel (email/SMS unwired, +// stdout-log delivery is dev/test-only) — issuance can never succeed until a +// real transport lands. func TestTwoFAEnsureIssueAllowed_ProdPredicate(t *testing.T) { if !twofaProdVariant { t.Skip("twoFAEnsureIssueAllowed() is the dev/test build's always-allowed variant (twofa_dev.go, `dev || test`); the prod fail-closed branches are unreachable under the test tag — see the file header for the documented limitation") } os.Unsetenv(twoFAPepperEnv) - os.Unsetenv(allowLogDeliveryEnv) if err := twoFAEnsureIssueAllowed(); err == nil { t.Error("expected issuance refused without TWO_FACTOR_PEPPER in a production build") } else if err.Error() != "TWO_FACTOR_PEPPER is not set; refusing to issue a 2FA code (an unsalted digest would be offline-brute-forceable)" { t.Errorf("expected the pepper-required error without the pepper, got %v", err) } + // With the pepper set, a production build STILL refuses: there is no + // delivery channel (email/SMS unwired, P6; stdout-log delivery is a + // dev/test-only local feature and there is no production opt-in). os.Setenv(twoFAPepperEnv, "test-pepper") - os.Unsetenv(allowLogDeliveryEnv) if err := twoFAEnsureIssueAllowed(); err == nil { - t.Error("expected issuance refused without a delivery channel in a production build") + t.Error("expected issuance refused in a production build with no delivery channel (email/SMS unwired, log delivery dev/test-only)") } else if err != errTwoFADeliveryUnavailable { - t.Errorf("expected errTwoFADeliveryUnavailable without a channel, got %v", err) - } - - os.Setenv(allowLogDeliveryEnv, "true") - if err := twoFAEnsureIssueAllowed(); err != nil { - t.Errorf("expected issuance allowed with both the pepper and a delivery channel, got %v", err) + t.Errorf("expected errTwoFADeliveryUnavailable with no channel, got %v", err) } } // TestTwoFADeliveryAvailable_ProdPredicate pins the production delivery -// predicate: TWO_FACTOR_ALLOW_LOG_DELIVERY unset → no channel (false), exactly -// "true" → channel (true), any other value → no channel. +// predicate: a production build ALWAYS reports no delivery channel — stdout-log +// delivery is a dev/test-only local feature, never a production channel. func TestTwoFADeliveryAvailable_ProdPredicate(t *testing.T) { if !twofaProdVariant { - t.Skip("twoFADeliveryAvailable() is the dev/test build's trivially-true variant (twofa_dev.go, `dev || test`); the 503 delivery-unavailable branch is unreachable under the test tag — see the file header for the documented limitation") + t.Skip("twoFADeliveryAvailable() is the dev/test build's trivially-true variant (twofa_dev.go, `dev || test`); the always-false production predicate is unreachable under the test tag — see the file header for the documented limitation") } - os.Unsetenv(allowLogDeliveryEnv) if twoFADeliveryAvailable() { - t.Error("production without the explicit opt-in must have NO 2FA delivery channel") - } - for _, v := range []string{"", "1", "yes", "on", "True", "TRUE", "false"} { - os.Setenv(allowLogDeliveryEnv, v) - if twoFADeliveryAvailable() { - t.Errorf("value %q must NOT open the delivery channel (exact 'true' only)", v) - } - } - os.Setenv(allowLogDeliveryEnv, "true") - if !twoFADeliveryAvailable() { - t.Error("the explicit insecure log-delivery opt-in must open the channel") + t.Error("a production build must ALWAYS report NO 2FA delivery channel (email/SMS unwired; stdout-log delivery is dev/test-only)") } } diff --git a/backend/internal/twofa/twofa.go b/backend/internal/twofa/twofa.go index c79aeb7..f9e1e6e 100644 --- a/backend/internal/twofa/twofa.go +++ b/backend/internal/twofa/twofa.go @@ -4,47 +4,33 @@ // // Why this package exists (B11c coordination contract): handlers/user imports // handlers/payments (TwoFactorEnforced, SquareClient), so handlers/payments -// CANNOT import handlers/user — Go would reject the cycle. The saved-card -// charge gate (B6/B10, owned by the payments agent) needs to verify a real 2FA -// challenge with the same brute-force lockout as the interactive endpoints, so -// the verification core lives here, importing neither. +// CANNOT import handlers/user — Go would reject the cycle. The verification +// core therefore lives here, importing neither, so both sides of the import +// boundary can reach it. // -// Contract for the payments gate: +// The verification consumers are the INTERACTIVE ACCOUNT FLOWS ONLY. The +// saved-card payments gates are now exclusively PSD2 SCA (Square buyer +// verification) and no longer call Check/VerifyForUser — the "charge gate" +// contract described in earlier revisions is obsolete. Today the callers are: // -// err := twofa.VerifyForUser(ctx, userID, code, twofa.ConsumeOnVerify) -// if err != nil { -// switch { -// case errors.Is(err, twofa.ErrIncorrect): -// // 400 -// case errors.Is(err, twofa.ErrLockedOut): -// // 429 -// case errors.Is(err, twofa.ErrMissingOrExpired): -// // 400 — user must request a fresh code -// default: -// // 500 (DB failure) -// } -// } +// - handlers/user: 2FA setup verify (VerifyTwoFAHandler) and 2FA disable +// re-verification (DisableTwoFAHandler), both via checkTwoFACode with +// DeferredConsume (they clear the pending fields themselves on success); +// - handlers/user/account.go: delete-account re-authentication +// (DeleteAccountHandler) via twofa.VerifyForUser with ConsumeOnVerify, so +// one code authorizes exactly one account erasure. // -// Consume mode (MEDIUM-2 remediation, finding 1): a successful verify with -// consume=true NULLs the pending code ATOMICALLY in the same critical section -// as the check, so one code authorizes exactly ONE operation — two concurrent -// charges can never both pass the gate with the same code (the per-user mutex -// serializes Check, and the second verify reads a NULLed digest and returns -// ErrMissingOrExpired). The payments saved-card CHARGE gates should therefore -// pass twofa.ConsumeOnVerify for FRESH charges: the code is burned at the gate, -// and a failed/ambiguous Square charge re-mints a fresh code (via the user -// package's exported EnsurePendingTwoFACode — reached through the HTTP mint -// endpoints, since handlers/payments cannot import handlers/user) instead of -// re-verifying the same code. This replaces the earlier MEDIUM-2 deferred -// consume (verify-with-consume=false at the gate + ConsumePendingCode at -// terminal success), which under concurrency let two gates both verify the same -// code before either charge consumed it. +// The payments package still touches this package on the TERMINAL-SUCCESS path +// only: ConsumePendingCode (after an SCA-approved saved-card charge or gift +// card issuance, where the pending code left over from the interactive mint +// must be retired) and StateFor/Hash for its code re-issue bookkeeping. // -// The interactive setup/disable flows pass DeferredConsume (false) — they clear -// the pending fields themselves on success (enableTwoFA / disableTwoFA), so the -// code must stay valid through their whole handshake. The save-card SAVE gate -// (handlers/payments) passes ConsumeOnVerify (true), since saving a card is a -// terminal operation with no downstream charge to attach consumption to. +// Consume mode: a successful verify with consume=true NULLs the pending code +// ATOMICALLY in the same critical section as the check, so one code authorizes +// exactly ONE operation — two concurrent consumers can never both pass the gate +// with the same code (the per-user mutex serializes Check, and the second +// verify reads a NULLed digest and returns ErrMissingOrExpired). DeleteAccount +// is the only current ConsumeOnVerify caller. // // The failed-attempt counter is keyed per user and resets ONLY on a successful // verify (or after the 10-minute attempt window elapses) — never on a fresh @@ -78,9 +64,10 @@ const MaxAttempts = 5 const ( // ConsumeOnVerify makes a successful verify SINGLE-USE immediately: the // pending-code digest and expiry are NULLed in the same critical section as - // the successful check (see Check). Use this for FRESH terminal operations — - // the saved-card CHARGE gates (finding 1) and the SAVE gate — where one - // code must authorize exactly one operation. + // the successful check (see Check). Use this for a TERMINAL operation that + // must be authorized by exactly one code — today that is the delete-account + // re-authentication flow (DeleteAccountHandler); the saved-card charge and + // SAVE gates no longer call Check (SCA-only since the PSD2 rework). ConsumeOnVerify = true // DeferredConsume verifies WITHOUT consuming; the caller NULLs the code // itself when its operation reaches terminal success (ConsumePendingCode) or @@ -194,7 +181,11 @@ func newSaturatedLockedState() *AttemptState { st.Count.Store(MaxAttempts) // Pinned so far in the future that now.Sub(LastActive) is always // <= AttemptWindow (LockedOut true) and never > AttemptWindow (no reset). - st.SetLastActive(time.Now().Add(24 * 365 * 24 * time.Hour)) + // clock.Now(), not time.Now(): the rest of the package reads time through + // crussell/clock (UTC-normalised, test-controllable) so the pinned stamp + // must be expressed in the same clock or a frozen test clock would leave + // now.Sub(LastActive) inconsistent with the saturation invariant. + st.SetLastActive(clock.Now().Add(24 * 365 * 24 * time.Hour)) return st } @@ -373,11 +364,12 @@ const ( // the pending code (lockout). A missing or expired pending code returns // MissingOrExpired. consume makes a correct code single-use IMMEDIATELY: the // stored digest and its expiry are NULLed right here, so one code cannot -// authorize a second operation within its lifetime. The interactive -// setup/disable flows pass DeferredConsume (false) and clear the pending fields -// themselves on success. The payments saved-card charge gates pass -// ConsumeOnVerify (true) for FRESH charges (finding 1): the code is burned at -// the gate, and a failed Square charge re-mints a fresh one. The returned +// authorize a second operation within its lifetime. The interactive account +// flows are the only consumers: the 2FA setup/disable handshakes pass +// DeferredConsume (false) and clear the pending fields themselves on success, +// while delete-account re-authentication passes ConsumeOnVerify (true) so one +// code authorizes exactly one erasure. The payments gates are SCA-only and no +// longer call Check. The returned // error is non-nil only for DB failures (callers return 500); a lockout's // pending-code invalidation failure is logged here and still reported as a // lockout. @@ -443,16 +435,16 @@ func Check(ctx context.Context, userID string, st *AttemptState, reqCode string, } // Success: clear the attempt counter before the caller performs its // action. The mint-cooldown stamp (LastMintAt) is deliberately NOT cleared - // here (Round 2 Loop A finding 2): a code verified at the saved-card gate - // may still be followed by a FAILED Square charge that re-issues a fresh - // code (payments.reissueTwoFACodeAfterFailedCharge), and that re-issue path + // here (Round 2 Loop A finding 2): a code verified at an interactive gate + // may still be followed by a FAILED money action whose retry mints a fresh + // code, and that fresh-code mint path (twoFAMintThrottled in handlers/user) // enforces the per-user mint cooldown against this stamp. Clearing it on a - // gate-verify let a charge-failure loop mint a fresh code on every - // iteration with no 60s cooldown (code churn + dev log flooding). The stamp - // is cleared only at a TERMINAL SUCCESS — the completed-charge consumption - // path (ConsumePendingCode, called by the money agent inside the - // transaction that records the completed charge) — so a customer who just - // completed a charge can immediately request a fresh code. + // gate-verify let a failure loop mint a fresh code on every iteration with + // no 60s cooldown (code churn + dev log flooding). The stamp is cleared + // only at a TERMINAL SUCCESS — the completed-operation consumption path + // (ConsumePendingCode, called inside the transaction that records the + // completed operation) — so a user who just completed a flow can + // immediately request a fresh code. st.Count.Store(0) st.SetLastActive(clock.Now()) ResetAttempts(userID) @@ -508,23 +500,23 @@ func Check(ctx context.Context, userID string, st *AttemptState, reqCode string, // ConsumePendingCode NULLs the user's pending 2FA code digest and expiry, and // clears the per-user mint-cooldown stamp (AttemptState.LastMintAt). -// Since finding 1 the saved-card CHARGE gates consume a FRESH charge's code at -// verify time (consume=true — single-use), so this is no longer the gate's -// consumption path: it is used by the PENDING-REUSE retry path, whose gate -// verified WITHOUT consuming (consume=false) so a retry that fails again keeps -// its code for one more attempt — the handlers call this when the retry reaches -// a TERMINAL SUCCESS state, inside the transaction that records the completed -// charge. Idempotent: consuming an already-NULL pending code is a no-op, so a -// code still authorizes exactly one completed charge and can never authorize a -// second after success. Accepts a db.Querier so the write can ride the caller's -// transaction (pgx.Tx) or the pool proxy. +// The saved-card charge gates are SCA-only and no longer consume codes at a +// gate verify (there is no homegrown gate verify to consume at); this is used +// on the TERMINAL-SUCCESS paths of the payments package — after an +// SCA-approved saved-card charge, a gift card issuance, or a till sale that +// used the customer's pending code — inside the transaction that records the +// completed operation, so a pending code minted for a flow can never authorize +// a second one. Idempotent: consuming an already-NULL pending code is a no-op, +// so a code still authorizes exactly one completed charge and can never +// authorize a second after success. Accepts a db.Querier so the write can ride +// the caller's transaction (pgx.Tx) or the pool proxy. // // Round 2 Loop A finding 2: this is the ONLY place the mint-cooldown stamp is // cleared on the charge path. A successful gate VERIFY (twofa.Check) must NOT -// clear it — the charge may still fail and the re-issue path -// (payments.reissueTwoFACodeAfterFailedCharge) enforces its cooldown against -// the stamp. Reaching terminal SUCCESS is what re-arms immediate re-minting, -// so consumption (which runs only at that terminal state) clears it. +// clear it — the flow may still fail and the retry's fresh-code mint +// (twoFAMintThrottled in handlers/user) enforces its cooldown against the +// stamp. Reaching terminal SUCCESS is what re-arms immediate re-minting, so +// consumption (which runs only at that terminal state) clears it. func ConsumePendingCode(ctx context.Context, q db.Querier, userID string) error { if userID == "" { return nil @@ -586,16 +578,13 @@ var ( // VerifyForUser verifies a 2FA code for a user outside the HTTP handler layer, // under the same per-user brute-force lockout as the interactive endpoints. // It returns nil on a correct code, or one of ErrIncorrect / ErrLockedOut / -// ErrMissingOrExpired (or a DB error, wrapped). This is the entry point for -// the payments card-access gate (B6/B10): a saved-card charge must present a -// real, freshly-verified challenge. consume makes a correct code single-use -// IMMEDIATELY (the pending-code digest and expiry are NULLed in the same -// critical section as the successful check — see Check). The payments saved- -// card CHARGE gate passes ConsumeOnVerify for FRESH charges (finding 1: a code -// authorizes exactly one charge, and a failed charge re-mints); the save-card -// SAVE gate passes ConsumeOnVerify too; the interactive setup/disable flows -// pass DeferredConsume and clear the pending fields themselves on success -// (enableTwoFA / disableTwoFA). +// ErrMissingOrExpired (or a DB error, wrapped). This is the non-HTTTP entry +// point for the interactive account flows — today only delete-account +// re-authentication (handlers/user/account.go), which passes ConsumeOnVerify so +// a code authorizes exactly one erasure. The payments saved-card gates no +// longer verify codes (SCA-only); the interactive setup/disable flows reach +// Check through handlers/user's checkTwoFACode with DeferredConsume and clear +// the pending fields themselves on success. func VerifyForUser(ctx context.Context, userID, code string, consume bool) error { st := StateFor(userID) st.Mu.Lock() diff --git a/backend/main.go b/backend/main.go index e681446..6b94886 100644 --- a/backend/main.go +++ b/backend/main.go @@ -200,16 +200,15 @@ func initSquare() { if enforced { // Delivery is build-dependent (handlers/user/twofa_dev.go / // twofa_prod.go): dev/test builds ALWAYS write the plaintext code to - // the [2FA] log line; production builds write it ONLY when the operator - // explicitly opts in with TWO_FACTOR_ALLOW_LOG_DELIVERY=true and refuse - // issuance otherwise. Warn accurately per case so the operator is never + // the local-dev [2FA] stdout log; production builds NEVER log it — + // stdout-log delivery is a dev/test-only feature, not a production + // channel. With no email/SMS transport wired yet (P6), a production + // build has NO delivery channel at all, so 2FA code issuance FAILS + // CLOSED and no user can complete 2FA setup or disable until the + // email/SMS transport lands. Warn loudly so the operator is never // misled into thinking codes are reaching users when issuance is // actually failing closed. - if os.Getenv("TWO_FACTOR_ALLOW_LOG_DELIVERY") == "true" { - log.Printf("WARNING: 2FA codes are delivered in PLAINTEXT via the server log ([2FA] prefix) — anyone with backend log access can defeat the account 2FA gate. Restrict log access and relay codes out-of-band; replace this loose-fake delivery with email/SMS (P6) before launch.") - } else { - log.Printf("WARNING: 2FA enforcement is ON but TWO_FACTOR_ALLOW_LOG_DELIVERY is unset: in a production build there is NO code-delivery channel (email/SMS is not wired — P6), so 2FA code issuance FAILS CLOSED and no user can complete 2FA setup or disable. Set TWO_FACTOR_ALLOW_LOG_DELIVERY=true to opt into the insecure [2FA] log-delivery channel (plaintext codes in the server log — restrict log access), or wire email/SMS (P6).") - } + log.Printf("WARNING: 2FA enforcement is ON, but this is not a dev/test build: stdout-log delivery is a LOCAL DEV ONLY feature and email/SMS is not wired yet (P6), so there is NO code-delivery channel and 2FA code issuance FAILS CLOSED — no user can complete 2FA setup or disable until email/SMS delivery is implemented. Card charges are unaffected (SCA-only). Run a dev/test build for local log-delivery testing.") } if !enforced && !payments.IsExplicitDevOrMockEnv() { log.Printf("WARNING: 2FA enforcement is OFF (REQUIRE_2FA=%q) with SQUARE_ENVIRONMENT=%q (not an explicit mock/dev value). Online saved-card payments will NOT require 2FA.", os.Getenv("REQUIRE_2FA"), env) @@ -788,6 +787,13 @@ func main() { r.Get("/{id}/edit-request", bookings.AdminGetBookingEditRequestHandler) r.Post("/{id}/edit-requests/{request_id}/approve", bookings.AdminApproveEditRequestHandler) r.Post("/{id}/edit-requests/{request_id}/deny", bookings.AdminRejectEditRequestHandler) + // Admin-side loyalty redemption: the admin "Take Payment" + // PaymentModal applies the customer's pending 10% loyalty + // redemption on the booking's behalf (the customer-facing + // /bookings/{id}/apply-redemption route below stays + // RequireNonGuest). ApplyLoyaltyRedemption resolves the acting + // role from context and lets an admin redeem on any booking. + r.Post("/{id}/apply-redemption", payments.ApplyLoyaltyRedemption) }) r.Route("/admin/users", func(r chi.Router) { diff --git a/backend/run-prod-tag-tests.sh b/backend/run-prod-tag-tests.sh new file mode 100755 index 0000000..e2cead2 --- /dev/null +++ b/backend/run-prod-tag-tests.sh @@ -0,0 +1,52 @@ +#!/usr/bin/env bash +# run-prod-tag-tests.sh — compiles and runs the PRODUCTION-ONLY 2FA tests. +# +# WHY: the fail-closed 2FA issuance/delivery branches live in +# handlers/user/twofa_prod.go, which is `//go:build !dev && !test`. The two +# documented CI test matrices — "test,dev" and "test,!dev" — BOTH set the +# `test` tag, so twofa_prod.go never compiles under either, and the dev/test +# variant (twofa_dev.go, `dev || test`) is always the compiled function. The +# production security default — no TWO_FACTOR_PEPPER → refuse issuance, no +# delivery channel → 503 — is therefore unreachable under both matrices. +# +# This script runs `go test` with NEITHER the `dev` NOR the `test` tag +# (`-tags "!dev,!test"`), the ONLY build configuration where twofa_prod.go +# compiles AND twofaProdVariant reports the real prod functions are live, so +# the assertions in twofa_prod_test.go actually execute instead of skipping. +# +# Note on `-tags "test,!dev"`: CI's "prod" matrix uses that tag set for +# vet/staticcheck/gosec (source-level checks), but for TESTS it still excludes +# twofa_prod.go (`!test`) and still compiles twofa_dev.go (`test` matches +# `dev || test`) — so it cannot exercise the prod fail-closed branches. The +# genuinely-prod test build is `!dev,!test` only. +# +# Env: GO_TESTING/DAV_SKIP_INIT are set so internal/dav's prod-service init +# (service_prod.go) does not attempt a real Postgres connect at package-load +# time — the CI test jobs set the same vars. +# +# USAGE: +# ./run-prod-tag-tests.sh # run the prod-shape 2FA tests +# ./run-prod-tag-tests.sh -v # pass through extra go test args +# +# Agents and humans: run backend tests through this script (or take the +# lockfile yourself: `flock /tmp/crussell-tests.lock -c ''`). The lock +# protects the shared test-DB namespace even though these packages do not +# currently touch the DB — if that ever changes the DB stays safe. + +set -u + +LOCKFILE="${LOCKFILE:-/tmp/crussell-tests.lock}" +LOCK_TIMEOUT="${LOCK_TIMEOUT:-300}" + +cd "$(dirname "$0")" + +# GO_TESTING=1: internal/dav prod init skips the real connect (service_prod.go). +# DAV_SKIP_INIT=1: belt-and-braces for the same skip gate. +export GO_TESTING=1 +export DAV_SKIP_INIT=1 + +# handlers/user holds the only prod-only test code (twofa_prod_test.go under +# `!dev`). handlers/payments is included to prove the payments package still +# compiles in a genuinely-prod build (it has no `test && dev` test files, so +# `go test` reports "[no test files]" and exercises the package build only). +exec flock -w "$LOCK_TIMEOUT" "$LOCKFILE" go test -tags "!dev,!test" -count=1 "$@" ./handlers/user/ ./handlers/payments/ diff --git a/backend/startup_checks_test.go b/backend/startup_checks_test.go new file mode 100644 index 0000000..fa6fd48 --- /dev/null +++ b/backend/startup_checks_test.go @@ -0,0 +1,144 @@ +//go:build test + +package main + +import ( + "bytes" + "encoding/base64" + "log" + "os" + "os/exec" + "strings" + "testing" + + "github.com/stretchr/testify/require" +) + +// captureLog redirects the process-wide logger into a buffer for the duration +// of fn and returns what was logged. These startup checks log through the +// global logger, so the tests that use this helper must stay sequential (no +// t.Parallel). +func captureLog(t *testing.T, fn func()) string { + t.Helper() + var buf bytes.Buffer + orig := log.Writer() + log.SetOutput(&buf) + defer log.SetOutput(orig) + fn() + return buf.String() +} + +// TestCheckSnapshotEncKey_FailLoudBranches pins the non-mock startup check for +// SNAPSHOT_ENC_KEY: a missing / invalid / wrong-length key must log a CRITICAL +// line (the at-rest PII warning), a valid base64 32-byte key logs nothing, and +// a dev/mock SQUARE_ENVIRONMENT skips the check entirely. +func TestCheckSnapshotEncKey_FailLoudBranches(t *testing.T) { + t.Setenv("SQUARE_ENVIRONMENT", "production") + + t.Run("missing_key_logs_critical", func(t *testing.T) { + t.Setenv("SNAPSHOT_ENC_KEY", "") + got := captureLog(t, checkSnapshotEncKey) + require.Contains(t, got, "SNAPSHOT_ENC_KEY is not set", "missing key must fail loud: %s", got) + }) + + t.Run("invalid_base64_logs_critical", func(t *testing.T) { + t.Setenv("SNAPSHOT_ENC_KEY", "!!!not-base64!!!") + got := captureLog(t, checkSnapshotEncKey) + require.Contains(t, got, "not valid base64", "invalid base64 must fail loud: %s", got) + }) + + t.Run("wrong_length_logs_critical", func(t *testing.T) { + // 16 bytes base64 → not 32 bytes → not AES-256. Built at runtime so no + // secret-shaped literal exists in source. + shortKey := base64.StdEncoding.EncodeToString([]byte("1234567890123456")) + t.Setenv("SNAPSHOT_ENC_KEY", shortKey) + got := captureLog(t, checkSnapshotEncKey) + require.Contains(t, got, "exactly 32 bytes", "wrong key length must fail loud: %s", got) + }) + + t.Run("valid_key_logs_nothing", func(t *testing.T) { + t.Setenv("SNAPSHOT_ENC_KEY", base64.StdEncoding.EncodeToString([]byte("12345678901234567890123456789012"))) // 32 bytes + got := captureLog(t, checkSnapshotEncKey) + require.Empty(t, got, "a valid key must not log: %s", got) + }) + + t.Run("mock_env_skips_check", func(t *testing.T) { + t.Setenv("SQUARE_ENVIRONMENT", "mock") + t.Setenv("SNAPSHOT_ENC_KEY", "") + got := captureLog(t, checkSnapshotEncKey) + require.Empty(t, got, "a dev/mock env must skip the check: %s", got) + }) +} + +// TestCheckProxyRateLimitConfig_WarnsWithoutTrustedProxy pins the fail-loud +// warning: a non-mock deployment without TRUST_PROXY_HEADERS collapses every +// per-IP limiter onto the proxy's address, so startup must warn. A dev/mock env +// skips the check. +func TestCheckProxyRateLimitConfig_WarnsWithoutTrustedProxy(t *testing.T) { + t.Setenv("SQUARE_ENVIRONMENT", "production") + got := captureLog(t, checkProxyRateLimitConfig) + require.Contains(t, got, "TRUST_PROXY_HEADERS", "a non-mock deployment without TRUST_PROXY_HEADERS must warn: %s", got) + + t.Setenv("SQUARE_ENVIRONMENT", "mock") + got = captureLog(t, checkProxyRateLimitConfig) + require.Empty(t, got, "a dev/mock env must skip the check: %s", got) +} + +// TestCheckWebhookSignatureKey_Branches pins the non-fatal branches of the +// webhook signing-key startup check: both configured → silent; key without URL +// → WARNING (fail-closed availability note); neither → CRITICAL. The fatal +// key-less-with-URL branch is covered by the subprocess test below (log.Fatalf +// exits the process). +func TestCheckWebhookSignatureKey_Branches(t *testing.T) { + t.Setenv("SQUARE_ENVIRONMENT", "production") + + t.Run("both_configured_silent", func(t *testing.T) { + t.Setenv("SQUARE_WEBHOOK_SIGNATURE_KEY", "k") + t.Setenv("SQUARE_WEBHOOK_NOTIFICATION_URL", "https://example.com/webhooks") + got := captureLog(t, checkWebhookSignatureKey) + require.Empty(t, got, "both configured must be silent: %s", got) + }) + + t.Run("key_without_url_warns", func(t *testing.T) { + t.Setenv("SQUARE_WEBHOOK_SIGNATURE_KEY", "k") + t.Setenv("SQUARE_WEBHOOK_NOTIFICATION_URL", "") + got := captureLog(t, checkWebhookSignatureKey) + require.Contains(t, got, "SQUARE_WEBHOOK_NOTIFICATION_URL is unset", "key without URL must warn: %s", got) + }) + + t.Run("neither_configured_critical", func(t *testing.T) { + t.Setenv("SQUARE_WEBHOOK_SIGNATURE_KEY", "") + t.Setenv("SQUARE_WEBHOOK_NOTIFICATION_URL", "") + got := captureLog(t, checkWebhookSignatureKey) + require.Contains(t, got, "SQUARE_WEBHOOK_SIGNATURE_KEY is not set", "neither configured must log CRITICAL: %s", got) + }) + + t.Run("mock_env_skips_check", func(t *testing.T) { + t.Setenv("SQUARE_ENVIRONMENT", "mock") + t.Setenv("SQUARE_WEBHOOK_SIGNATURE_KEY", "") + t.Setenv("SQUARE_WEBHOOK_NOTIFICATION_URL", "https://example.com/webhooks") + got := captureLog(t, checkWebhookSignatureKey) + require.Empty(t, got, "a dev/mock env must skip the check: %s", got) + }) +} + +// TestCheckWebhookSignatureKey_FatalBranch_Exits covers the fail-fast branch — +// a signing key is REQUIRED when a notification URL is configured, and the +// startup check calls log.Fatalf (os.Exit). That cannot run in-process, so the +// test re-executes the test binary with a marker env var and asserts the +// subprocess exits non-zero with the FATAL message naming the key. +func TestCheckWebhookSignatureKey_FatalBranch_Exits(t *testing.T) { + if os.Getenv("GO_WANT_HELPER_PROCESS") == "1" { + t.Setenv("SQUARE_ENVIRONMENT", "production") + t.Setenv("SQUARE_WEBHOOK_SIGNATURE_KEY", "") + t.Setenv("SQUARE_WEBHOOK_NOTIFICATION_URL", "https://example.com/webhooks") + checkWebhookSignatureKey() + return + } + cmd := exec.Command(os.Args[0], "-test.run=TestCheckWebhookSignatureKey_FatalBranch_Exits") + cmd.Env = append(os.Environ(), "GO_WANT_HELPER_PROCESS=1") + out, err := cmd.CombinedOutput() + require.Error(t, err, "expected the key-less-with-URL branch to exit (log.Fatalf); output: %s", out) + require.Contains(t, string(out), "SQUARE_WEBHOOK_SIGNATURE_KEY", "the fatal log must name the missing key: %s", out) + require.False(t, strings.Contains(string(out), "unexpected argument"), "the helper must not fail on argument parsing: %s", out) +} diff --git a/sabredav/server.php b/sabredav/server.php index 78684ff..3911a94 100644 --- a/sabredav/server.php +++ b/sabredav/server.php @@ -149,14 +149,55 @@ if ($stmt->fetchColumn() == 0) { // PII (vCards), so a default or publicly-known admin credential is never // acceptable — fail fast instead of starting with one. $davPassword = getenv('DAV_ADMIN_PASSWORD'); -$weakDavPasswords = ['admin', 'password', 'changeme', 'change-me', 'changethis', 'secret', 'sabredav', 'test']; -if ($davPassword === false || $davPassword === '' || in_array(strtolower(trim($davPassword)), $weakDavPasswords, true)) { - error_log("FATAL: DAV_ADMIN_PASSWORD is not set or is a known weak/default value. Refusing to start: set a strong random DAV_ADMIN_PASSWORD (e.g. `openssl rand -hex 32`) in the environment and restart."); +// Known weak/default values — INCLUDING compound placeholders like the one +// shipped in .env.example (changeme-admin-password). The old list only had the +// bare words, so `cp .env.example .env` booted CardDAV with a +// publicly-documented admin credential exposing every customer vCard +// (name/email/phone/DOB). Exact-match entries catch the documented +// placeholders; the entropy/length gate below catches everything else a +// copy-paste deployment could ship. +$weakDavPasswords = [ + 'admin', 'password', 'changeme', 'change-me', 'changethis', 'secret', + 'sabredav', 'test', + // Compound placeholders (documented in .env.example / READMEs / attack tooling). + 'changeme-admin-password', 'changeme-admin', 'change-me-admin', + 'change-me-admin-password', 'changethis-admin-password', 'admin-password', + 'sabredav-admin', 'sabredav-password', 'test-admin', 'test-password', + 'password123', 'admin123', 'adminadmin', 'passwordpassword', +]; +// davPasswordIsWeak reports whether a DAV_ADMIN_PASSWORD is a known +// placeholder, shorter than the minimum safe length, or lacks sufficient +// entropy. Mirrors isWeakJWTSecret in backend/main.go: length alone is not +// enough ("aaaaaaaaaaaaaaaa" passes a length check but has trivial key space). +// The bars here (>= 16 chars, >= 8 distinct characters) are deliberate: the +// goal is to refuse copy-paste defaults, not to gate a genuinely random secret +// (even pure hex, which can use at most 16 distinct chars, clears the 8-char bar). +function davPasswordIsWeak($password) { + $trimmed = strtolower(trim($password)); + if (strlen($trimmed) < 16) { + return true; + } + global $weakDavPasswords; + if (in_array($trimmed, $weakDavPasswords, true)) { + return true; + } + $distinct = count_chars($trimmed, 3); // returns only chars present + return strlen($distinct) < 8; +} +if ($davPassword === false || $davPassword === '' || davPasswordIsWeak($davPassword)) { + error_log("FATAL: DAV_ADMIN_PASSWORD is not set or is a known weak/default value (it must be at least 16 characters, use at least 8 distinct characters, and not be a documented placeholder). Refusing to start: set a strong random DAV_ADMIN_PASSWORD (e.g. `openssl rand -hex 32`) in the environment and restart."); http_response_code(500); die("DAV_ADMIN_PASSWORD is not configured"); } $stmt = $pdo->query("SELECT COUNT(*) FROM dav_users"); if ($stmt->fetchColumn() == 0) { + // digesta1 = md5(username:realm:password). SabreDAV 4.7.0's HTTP digest + // auth supports ONLY the RFC 2617 MD5 A1 form (see vendor sabre/http + // Auth/Digest.php validateA1) — a SHA-256 upgrade would require patching + // the vendored library, so it is intentionally NOT attempted here. The MD5 + // residual is bounded by the strong-password gate above: the stored value + // is a salted-by-realm MD5 of a high-entropy admin password, so a DB leak + // cannot yield the credential via a dictionary attack. $digest = md5('admin:SabreDAV:' . $davPassword); $pdo->exec(" INSERT INTO dav_users (username, digesta1) diff --git a/scripts/check-env-docs.py b/scripts/check-env-docs.py index e5017c6..c852f30 100644 --- a/scripts/check-env-docs.py +++ b/scripts/check-env-docs.py @@ -95,6 +95,12 @@ def find_env_vars_in_code(): # itself and cannot be defined in .env.example — never treat them as user env vars. env_vars -= {'DEV', 'PROD', 'SSR', 'MODE', 'BASE_URL', 'BUILD'} + # GO_WANT_HELPER_PROCESS is the conventional Go test-internal sentinel for + # the "re-exec self as helper process" pattern (startup_checks_test.go sets + # it via cmd.Env on the re-exec'd binary). It is NOT a user-configurable + # variable — it never belongs in .env.example, so never flag it. + env_vars -= {'GO_WANT_HELPER_PROCESS'} + return sorted(env_vars)