Skip to content

Lower bcrypt cost in API tests - #326

Open
kaareal wants to merge 6 commits into
masterfrom
feature/password-test-bcrypt-cost-6f6c26
Open

kaareal wants to merge 6 commits into
masterfrom
feature/password-test-bcrypt-cost-6f6c26

Conversation

@kaareal

@kaareal kaareal commented Oct 5, 2026 •

Copy link
Copy Markdown
Collaborator

What

  • services/api/.env: adds BCRYPT_SALT_PASSES=12.
  • services/api/src/utils/auth/password.js: the bcrypt cost reads BCRYPT_SALT_PASSES via config.get instead of the literal 12.
  • services/api/vitest.config.js: sets BCRYPT_SALT_PASSES ||= '4' (the minimum; bcryptjs clamps anything lower up to 4, so 1 would behave the same) alongside the existing test-only env vars.

Why

setPassword hashed at a hard-coded cost of 12 with bcryptjs (pure JS). Every createUser({ password }) and every password login in tests paid for a cost-12 hash or compare, which dominated the auth test files.

Measurements (local, warm runs)

Run Before After
pnpm test src/routes/auth/password.test.js (20 tests) 8.0 - 10.2 s 1.5 - 2.3 s
pnpm test src/routes/auth src/utils/auth (90 tests) 11.4 s 4.8 - 6.9 s

Reviewer notes

  • No deployment manifest change: with the .env default, a generated hash still starts with $2b$12$.
  • The variable is only set in vitest.config.js, so nothing outside the test run gets a weaker cost.
  • Existing hashes are unaffected; bcrypt reads the cost from the stored hash on compare.

Password hashing used a hard-coded cost of 12 with bcryptjs (pure JS),
so every createUser({ password }) and login in tests paid ~200ms.
The cost now reads PASSWORD_BCRYPT_COST, defaulting to 12, and
vitest.config.js sets it to 4 for the test run.
@github-actions

github-actions Bot commented Oct 5, 2026

Copy link
Copy Markdown

API Changes

No changes.

@kaareal
kaareal requested a review from andrewplummer October 5, 2026 14:09

@andrewplummer andrewplummer left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is totally fine - approved, however I was thinking to myself maybe mocking bcrypt is the more "correct" way ... either way it's fine

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants