From d7dacffa1128ed32e4e9873e5929500b54c83ebb Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Sun, 30 Aug 2026 12:29:19 -0500 Subject: [PATCH] test(perf): stop hashing test passwords at production cost (#242) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The integration suite registers around thirty-five customers and asserts nothing about any of their hashes, yet paid bcrypt cost 12 for every one. bcryptjs is a pure-JS implementation, so it pays that cost several times over compared with a native build, and hashing was most of the suite's wall clock. On a contended runner it pushed adminInventory.integration.test.ts past its twenty-second timeout, which then surfaced as a foreign key violation somewhere else entirely — the test timed out, jest moved on, beforeEach truncated, and the still-in-flight registration wrote a token for a customer that had just been deleted. Measured rather than asserted, warm run against warm run with only the constant changed: 34.5s at cost 12, 9.8s at cost 4. Three and a half times faster, about twenty-five seconds off every integration run, with all 263 tests passing either way. The first attempt at that measurement was wrong and worth recording. Comparing a cold run at cost 4 against a warm run at cost 12 made the change look like a 36% regression-shaped improvement of the wrong size; the difference was ts-jest and Postgres warming up, not the cost factor. Both numbers above are second runs, and the cost-12 figure was taken twice — 34.3s and 34.5s — before being believed. Deliberately not configurable. An environment variable here would be a way to weaken password hashing in production by misconfiguration, and nothing needs to tune it. The only route to the cheap cost is NODE_ENV=test, which a deployed container would announce anyway by refusing to serve the built frontend, since app.ts gates static serving on the same value. A setting that quietly degrades a security property should be unreachable rather than warned about, which is the reasoning that already made DEMO_MODE strict. `hashRoundsFor` is pure and separately tested because the failure it guards against is silent: only the exact string 'test' earns the cheap cost, and an unset NODE_ENV gets the strong one, so the dangerous direction has to be asked for explicitly. Both constants are pinned by assertions too — without that the branch tests pass while the numbers drift to something useless. Closes #242 --- backend/src/passwordHashing.ts | 49 ++++++++++++++++++++++ backend/src/routes/customers.ts | 7 ++-- backend/tests/unit/passwordHashing.test.ts | 39 +++++++++++++++++ 3 files changed, 92 insertions(+), 3 deletions(-) create mode 100644 backend/src/passwordHashing.ts create mode 100644 backend/tests/unit/passwordHashing.test.ts diff --git a/backend/src/passwordHashing.ts b/backend/src/passwordHashing.ts new file mode 100644 index 0000000..8d1a5b9 --- /dev/null +++ b/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); diff --git a/backend/src/routes/customers.ts b/backend/src/routes/customers.ts index 20e7129..f9b8477 100755 --- a/backend/src/routes/customers.ts +++ b/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(`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 diff --git a/backend/tests/unit/passwordHashing.test.ts b/backend/tests/unit/passwordHashing.test.ts new file mode 100644 index 0000000..b5f8043 --- /dev/null +++ b/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); + }); +});