Merge pull request 'test(perf): stop hashing test passwords at production cost (#242)' (#243) from fix/242-bcrypt-cost-in-tests into main

Reviewed-on: #243

backend/src/passwordHashing.ts

@@ -0,0 +1,49 @@
+/**
+ * How expensive a password hash is, and why that differs under test.
+ *
+ * bcrypt's cost is exponential: each step doubles the work. Twelve is the right
+ * number for real passwords and the wrong one for a test suite that registers
+ * around thirty-five customers and asserts nothing about any of their hashes.
+ * `bcryptjs` is a pure-JS implementation, so it pays that cost several times
+ * over compared with a native build, and the integration suite spent most of
+ * its wall clock there. On a loaded runner that pushed
+ * adminInventory.integration.test.ts past its twenty-second timeout, which read
+ * as a foreign key violation somewhere else entirely — see #242.
+ *
+ * Deliberately not configurable.
+ * ------------------------------
+ * An environment variable here would be a way to weaken password hashing in
+ * production by misconfiguration, and nothing needs to tune this. The only way
+ * to reach the cheap cost is NODE_ENV=test, which a deployed container would
+ * also announce loudly by refusing to serve the built frontend — app.ts gates
+ * static file serving on the same value. A setting that quietly degrades a
+ * security property should be unreachable rather than merely warned about,
+ * which is the same reasoning that made DEMO_MODE strict.
+ */
+
+/** What real passwords are hashed with, everywhere that is not a test run. */
+export const PRODUCTION_ROUNDS = 12;
+
+/**
+ * What tests hash with. 2^8 = 256 times less work than production.
+ *
+ * Four is bcrypt's own floor, so this is as cheap as the algorithm allows. It
+ * is a fine number for a suite whose passwords are fixtures; it would be a
+ * serious defect anywhere a real one is stored.
+ */
+export const TEST_ROUNDS = 4;
+
+/**
+ * The cost for an environment, from NODE_ENV.
+ *
+ * Pure and exported for its test: this is the whole of the policy, and the
+ * failure it guards against is silent. Only the exact string 'test' earns the
+ * cheap cost — an unset NODE_ENV, or anything else, gets the strong one, so the
+ * dangerous direction requires saying so explicitly.
+ */
+export function hashRoundsFor(nodeEnv: string | undefined): number {
+  return nodeEnv === 'test' ? TEST_ROUNDS : PRODUCTION_ROUNDS;
+}
+
+/** Resolved once at import: NODE_ENV does not change while the process runs. */
+export const PASSWORD_HASH_ROUNDS = hashRoundsFor(process.env.NODE_ENV);

backend/src/routes/customers.ts

@@ -1,5 +1,6 @@
 import { Router, Request, Response } from 'express';
 import bcrypt from 'bcryptjs';
+import { PASSWORD_HASH_ROUNDS } from '../passwordHashing';
 import crypto from 'node:crypto';
 import { pool, requireRow } from '../db';
 import { requireCustomer } from '../middleware/customerAuth';
@@ -207,7 +208,7 @@ router.post('/register', asyncRoute(async (req: Request, res: Response) => {
   const { rows: existing } = await pool.query<IdRow>(`SELECT id FROM customers WHERE email = $1`, [normalizedEmail]);
   if (existing.length) return res.status(409).json({ error: 'an account with this email already exists' });
 
-  const passwordHash = await bcrypt.hash(password, 12);
+  const passwordHash = await bcrypt.hash(password, PASSWORD_HASH_ROUNDS);
   const unsubscribeToken = crypto.randomBytes(16).toString('hex');
   const consent = !!marketingConsent;
 
@@ -336,7 +337,7 @@ router.post('/reset-password', asyncRoute(async (req: Request, res: Response) =>
     return res.status(403).json({ error: 'this account has been disabled' });
   }
 
-  const passwordHash = await bcrypt.hash(String(password), 12);
+  const passwordHash = await bcrypt.hash(String(password), PASSWORD_HASH_ROUNDS);
 
   const client = await pool.connect();
   try {
@@ -475,7 +476,7 @@ router.post('/change-password', requireCustomer, asyncRoute(async (req: Request,
   if (!(await bcrypt.compare(currentPassword || '', customer.password_hash))) {
     return res.status(401).json({ error: 'current password is incorrect' });
   }
-  const newHash = await bcrypt.hash(newPassword, 12);
+  const newHash = await bcrypt.hash(newPassword, PASSWORD_HASH_ROUNDS);
   await pool.query(`UPDATE customers SET password_hash = $1 WHERE id = $2`, [newHash, req.customerId]);
 
   // Password reset already ends every session, on the reasoning that a password

backend/tests/unit/passwordHashing.test.ts

@@ -0,0 +1,39 @@
+import { hashRoundsFor, PRODUCTION_ROUNDS, TEST_ROUNDS } from '../../src/passwordHashing';
+
+// The cost factor is a security property, and the whole point of lowering it
+// under test is that it must not be lowered anywhere else. Asserted rather than
+// read, so a later edit that widens the test branch fails here first.
+describe('hashRoundsFor', () => {
+  it('uses the cheap cost under test', () => {
+    expect(hashRoundsFor('test')).toBe(TEST_ROUNDS);
+  });
+
+  it('uses the real cost in production', () => {
+    expect(hashRoundsFor('production')).toBe(PRODUCTION_ROUNDS);
+  });
+
+  // The container sets NODE_ENV=production, but a bare `node dist/server.js`
+  // does not set it at all. Absent must mean the strong cost, never the cheap
+  // one — defaulting the other way is how a weak hash reaches real passwords.
+  it('uses the real cost when NODE_ENV is unset', () => {
+    expect(hashRoundsFor(undefined)).toBe(PRODUCTION_ROUNDS);
+  });
+
+  it('uses the real cost for any value that is not exactly "test"', () => {
+    for (const value of ['development', 'staging', 'Test', 'TEST', 'testing', '']) {
+      expect(hashRoundsFor(value)).toBe(PRODUCTION_ROUNDS);
+    }
+  });
+
+  // Pinning the numbers themselves. Without this the tests above pass while
+  // both constants drift to something useless.
+  it('keeps the production cost at 12', () => {
+    expect(PRODUCTION_ROUNDS).toBe(12);
+  });
+
+  it('keeps the test cost cheap enough to be worth doing', () => {
+    expect(TEST_ROUNDS).toBeLessThanOrEqual(6);
+    // bcrypt refuses a cost below 4.
+    expect(TEST_ROUNDS).toBeGreaterThanOrEqual(4);
+  });
+});