fix: Replace bcrypt with Argon2id, upgrading legacy hashes on login - #901
fix: Replace bcrypt with Argon2id, upgrading legacy hashes on login#901serendipty01 wants to merge 4 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughPassword hashing now uses Argon2id while retaining legacy bcrypt verification. Successful logins conditionally rehash outdated passwords with compare-and-swap persistence, and failures during hash evaluation or persistence are logged without allowing persistence errors to reject login. ChangesPassword security
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant verifyEmailPasswordCredentials
participant PasswordUtils
participant UserDatabase
participant Tracer
Client->>verifyEmailPasswordCredentials: submit email and password
verifyEmailPasswordCredentials->>PasswordUtils: verify password
PasswordUtils-->>verifyEmailPasswordCredentials: successful match
verifyEmailPasswordCredentials->>PasswordUtils: check passwordNeedsRehash
PasswordUtils-->>verifyEmailPasswordCredentials: rehash required
verifyEmailPasswordCredentials->>PasswordUtils: hashPassword
PasswordUtils-->>verifyEmailPasswordCredentials: Argon2id hash
verifyEmailPasswordCredentials->>UserDatabase: persist updated hash
UserDatabase-->>verifyEmailPasswordCredentials: success or failure
UserDatabase->>Tracer: log failed persistence
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
serendipty01
changed the title
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Pull request overview
This PR strengthens password storage by raising bcrypt’s work factor from 5 to 12 and introducing an opportunistic rehash-on-login flow to upgrade legacy hashes without requiring a database migration.
Changes:
- Increased new password hash cost to 12 via a single
TARGET_BCRYPT_COSTconstant and addedpasswordNeedsRehash. - Added best-effort rehash-on-login that upgrades legacy hashes after a successful password verification.
- Added unit tests covering hashing/verification, rehash detection, and rehash-on-login persistence + failure swallowing.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| server/services/userManagementService/utils.ts | Raises bcrypt cost to 12 and adds passwordNeedsRehash. |
| server/services/userManagementService/utils.test.ts | Adds tests for cost-12 hashing, legacy verification, and rehash detection behavior. |
| server/services/userManagementService/index.ts | Re-exports passwordNeedsRehash for downstream use. |
| server/graphql/datasources/userApiCredentials.ts | Adds rehash-on-login hook + helper to persist upgraded hashes. |
| server/graphql/datasources/userApiCredentials.test.ts | Adds tests asserting rehash persistence, no-op when current, no write on wrong password, and swallow-on-failure behavior. |
![coderabbitai[bot]](https://cdn.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/graphql/datasources/userApiCredentials.ts (1)
104-107: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider deferring the rehash write to avoid adding ~250ms latency to legacy-user logins.
await rehashPasswordOnLogin(...)runs synchronously in the login response path. Bcrypt at cost 12 takes ~250ms on typical hardware, meaning every login for a user with a legacy hash pays that latency until their hash is upgraded. If the rehash were fire-and-forget (with proper error handling to avoid unhandled rejections per coding guidelines), login would return immediately and the upgrade would happen in the background.This is a design trade-off: the current approach guarantees the rehash completes before the response, while fire-and-forget risks losing the upgrade if the process exits. Given the best-effort nature of the rehash, deferring may be acceptable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/graphql/datasources/userApiCredentials.ts` around lines 104 - 107, Defer rehashPasswordOnLogin from the synchronous login response path when passwordNeedsRehash(user.password) is true, allowing authentication to return without awaiting the bcrypt work. Trigger the operation in the background and attach explicit error handling so failures do not become unhandled promise rejections; preserve the existing best-effort rehash behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/graphql/datasources/userApiCredentials.ts`:
- Around line 32-46: Update rehashPasswordOnLogin to use a compare-and-swap when
persisting the rehashed password: retain the password hash that was verified for
this login and only update user.id when the stored password still matches that
hash. Reuse the existing conditional-update/data-access mechanism, ensuring
concurrent changePassword or resetPasswordForToken updates are not overwritten
while preserving the current error logging.
---
Nitpick comments:
In `@server/graphql/datasources/userApiCredentials.ts`:
- Around line 104-107: Defer rehashPasswordOnLogin from the synchronous login
response path when passwordNeedsRehash(user.password) is true, allowing
authentication to return without awaiting the bcrypt work. Trigger the operation
in the background and attach explicit error handling so failures do not become
unhandled promise rejections; preserve the existing best-effort rehash behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 720da2f6-487f-4a04-8fc2-40c9ce056f9f
📒 Files selected for processing (5)
server/graphql/datasources/userApiCredentials.test.tsserver/graphql/datasources/userApiCredentials.tsserver/services/userManagementService/index.tsserver/services/userManagementService/utils.test.tsserver/services/userManagementService/utils.ts
…ied hash Addresses PR roostorg#901 review (Copilot and CodeRabbit converged on the same finding): the rehash write was keyed on user id alone, so a password change committing between verification and the write could be overwritten by a rehash of the just-verified OLD plaintext - silently resurrecting the old password. The write now also matches the exact stored hash that was verified (WHERE id = ? AND password = ?); zero matched rows means the row changed underneath us and the stale rehash is dropped, preserving the best-effort semantics. Also removes the now-unused kyselyUserUpdate import (that helper cannot express the guard - review comment 2) and pins the CAS query shape in the rehash test. Generated with AI Co-Authored-By: AI <ai@example.com>
taobojlen
left a comment
taobojlen
left a comment
There was a problem hiding this comment.
thanks for this!
i think the rehash-on-login approach is sensible. but it's a good question about argon2. i think that if we're going to change our password hashes, it makes most sense to move to the OWASP recommendation of
Use Argon2id with a minimum configuration of 19 MiB of memory, an iteration count of 2, and 1 degree of parallelism.
otherwise we'll have to maintain several different hash formats all at once; seems we might as well do it in one go! what do you think?
|
hey — agreed, let's do it in one go. i'll add the argon2id changes to this PR rather than opening a new one. I was thinking to use @node-rs/argon2 (MIT) Reasoning from Claude regarding this:
let me know if you'd rather use something else node also has crypto.argon2 now, but it's still experimental, probably worth dropping the dependency for it once it stabilises. i was also thinking about adding a pepper, and will raise a separate issue to discuss that. marking this as draft for now; i'll ping you once it's done and tested on my end. |
…ied hash Addresses PR roostorg#901 review (Copilot and CodeRabbit converged on the same finding): the rehash write was keyed on user id alone, so a password change committing between verification and the write could be overwritten by a rehash of the just-verified OLD plaintext - silently resurrecting the old password. The write now also matches the exact stored hash that was verified (WHERE id = ? AND password = ?); zero matched rows means the row changed underneath us and the stale rehash is dropped, preserving the best-effort semantics. Also removes the now-unused kyselyUserUpdate import (that helper cannot express the guard - review comment 2) and pins the CAS query shape in the rehash test. Generated with AI Co-Authored-By: AI <ai@example.com>
acbd76d to
adb896c
Compare
serendipty01
changed the title
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/services/userManagementService/utils.ts`:
- Around line 49-55: Update the catch block in passwordMatchesHash to log the
caught malformed-hash error at debug level before returning false. Preserve the
fail-closed non-match behavior and include enough context to identify the
password-hash validation failure without exposing sensitive values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ec519b23-1464-40bb-bd26-e29210b2aa56
⛔ Files ignored due to path filters (1)
server/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
server/graphql/datasources/userApiCredentials.test.tsserver/graphql/datasources/userApiCredentials.tsserver/package.jsonserver/services/userManagementService/index.tsserver/services/userManagementService/utils.test.tsserver/services/userManagementService/utils.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- server/services/userManagementService/index.ts
- server/graphql/datasources/userApiCredentials.test.ts
adb896c to
a55c8fd
Compare
taobojlen
left a comment
taobojlen
left a comment
There was a problem hiding this comment.
Thank you, this looks great!
I'd like to see us simplify this a little bit -- LLMs love to write overly-defensive code, but IMO it just complicates things and often isn't necessary. But this is very close!
| expect(updateTable).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('still succeeds the login when the rehash write fails (#778 regression guard)', async () => { |
There was a problem hiding this comment.
i don't really understand this test!
under what circumstances would the rewrite throw an exception, and how is that related to a user changing their password?
There was a problem hiding this comment.
fixed the comment. The test covers: the rehash-on-login write can fail for any transient reason (DB connection drop, timeout, etc.), and the login must still succeed when that happens.
There was a problem hiding this comment.
the rehash-on-login write can fail for any transient reason (DB connection drop, timeout, etc.), and the login must still succeed when that happens.
i think i fundamentally disagree with this! in all of these above cases, the login should fail.
| ).rejects.toThrow(); | ||
|
|
||
| await expect( | ||
| passwordMatchesHash(PASSWORD, '$argon2id$v=19$corrupt'), |
There was a problem hiding this comment.
i think this should probably throw, too? i can't see why we'd want to handle this case elegantly; it should not come up in practice.
There was a problem hiding this comment.
This test exists only to pin the library's current behavior, so a future @node-rs/argon2 upgrade that changes it trips a test instead of silently drifting. Happy to delete it if you'd rather not carry that insurance — but there's no handling on our side to remove.
There was a problem hiding this comment.
let's remove it. this behaviour actually would cause the silent drift, we want to fail loudly if our fundamental assumptions get violated!
| .where('password', '=', verifiedHash) | ||
| .execute(); | ||
| } catch (e) { | ||
| deps.tracer.logActiveSpanFailedIfAny(e); |
There was a problem hiding this comment.
Under what circumstances would we hit this?
I am generally against swallowing errors unless they're actually expected.
There was a problem hiding this comment.
Some scenarios: the UPDATE hitting a connection-pool limit, a timeout, or a deadlock, or hashPassword failing under memory pressure (Argon2 needs a 19 MiB allocation per call; if the process is memory-starved that can throw).
They should not fail the login and will only log them.
There was a problem hiding this comment.
all of those things should make the login fail! the user can just retry if it's truly transient but let's not have all sorts of silent behaviors in our code.
There was a problem hiding this comment.
i.e. let's remove the try/catch here.
a55c8fd to
cf56118
Compare
…ied hash Addresses PR roostorg#901 review (Copilot and CodeRabbit converged on the same finding): the rehash write was keyed on user id alone, so a password change committing between verification and the write could be overwritten by a rehash of the just-verified OLD plaintext - silently resurrecting the old password. The write now also matches the exact stored hash that was verified (WHERE id = ? AND password = ?); zero matched rows means the row changed underneath us and the stale rehash is dropped, preserving the best-effort semantics. Also removes the now-unused kyselyUserUpdate import (that helper cannot express the guard - review comment 2) and pins the CAS query shape in the rehash test. Generated with AI Co-Authored-By: AI <ai@example.com>
- Trim rehashPasswordOnLogin's JSDoc to the load-bearing parts, dropping the changePassword/session-purge tangent and Argon2-variant mention. - Explain exactly which failures the rehash write's catch block is expected to swallow (transient DB/infra failures), rather than swallowing everything unjustified. - Drop the try/catch that disguised a passwordMatchesHash throw as "wrong password"; let it propagate to the existing outer catch-all, which already logs and returns a generic error. - Rename/reclarify the "rehash write fails" test: it was mislabeled as a roostorg#778 regression guard, but roostorg#778 is about the changePassword session-purge, an unrelated concern. - Clarify utils.test.ts's malformed-hash test: the throw-vs-resolve-false split is @node-rs/argon2's own behavior, not something our code special-cases; the test pins it against a silent library upgrade. - Apply the trivial comment-wording suggestion at test line 105.
Closes roostorg#900. - hashPassword now hashes at cost 12 (was 5; OWASP minimum is 10). Existing rows stay valid - bcrypt embeds the cost factor in the hash string, so legacy cost-5 hashes keep verifying with no migration. - passwordNeedsRehash flags hashes below the target cost; bcrypt.getRounds returns NaN for non-bcrypt strings, which safely falls out as "no rehash". - Rehash-on-login: after a successful password check against a legacy hash, verifyEmailPasswordCredentials transparently re-hashes the plaintext (already in memory from the login attempt) at cost 12 and persists it as a plain column update - deliberately NOT the session-purging changePassword/resetPasswordForToken path (see roostorg#778), so the upgrade never logs anyone out. Best-effort: a failed write is logged and never fails the login. - Tests: utils.test.ts (target cost, round-trip, legacy verify, passwordNeedsRehash edge cases), userApiCredentials.test.ts (rehash persists, current hash untouched, wrong password never writes, login survives a failed rehash write). Co-Authored-By: Claude <noreply@anthropic.com>
…ied hash Addresses PR roostorg#901 review (Copilot and CodeRabbit converged on the same finding): the rehash write was keyed on user id alone, so a password change committing between verification and the write could be overwritten by a rehash of the just-verified OLD plaintext - silently resurrecting the old password. The write now also matches the exact stored hash that was verified (WHERE id = ? AND password = ?); zero matched rows means the row changed underneath us and the stale rehash is dropped, preserving the best-effort semantics. Also removes the now-unused kyselyUserUpdate import (that helper cannot express the guard - review comment 2) and pins the CAS query shape in the rehash test. Co-Authored-By: Claude <noreply@anthropic.com>
…login Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/services/userManagementService/utils.ts (1)
33-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant
promisifywrapper.bcryptjs.comparealready returns a Promise when no callback is passed, sobcryptComparecan be dropped andpasswordMatchesHashcan callbcrypt.compare(...)directly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/services/userManagementService/utils.ts` around lines 33 - 39, Remove the promisify-based bcryptCompare declaration and update passwordMatchesHash to call bcrypt.compare directly without a callback. Preserve the existing bcrypt-hash dispatch and verification behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@server/services/userManagementService/utils.ts`:
- Around line 33-39: Remove the promisify-based bcryptCompare declaration and
update passwordMatchesHash to call bcrypt.compare directly without a callback.
Preserve the existing bcrypt-hash dispatch and verification behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: fe1495ec-a0d7-4636-8fec-0c98e2527d06
⛔ Files ignored due to path filters (1)
server/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
server/graphql/datasources/userApiCredentials.test.tsserver/graphql/datasources/userApiCredentials.tsserver/package.jsonserver/services/userManagementService/index.tsserver/services/userManagementService/utils.test.tsserver/services/userManagementService/utils.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- server/services/userManagementService/index.ts
- server/package.json
- server/graphql/datasources/userApiCredentials.ts
- server/services/userManagementService/utils.test.ts
- server/graphql/datasources/userApiCredentials.test.ts
- Trim rehashPasswordOnLogin's JSDoc to the load-bearing parts, dropping the changePassword/session-purge tangent and Argon2-variant mention. - Explain exactly which failures the rehash write's catch block is expected to swallow (transient DB/infra failures), rather than swallowing everything unjustified. - Drop the try/catch that disguised a passwordMatchesHash throw as "wrong password"; let it propagate to the existing outer catch-all, which already logs and returns a generic error. - Rename/reclarify the "rehash write fails" test: it was mislabeled as a roostorg#778 regression guard, but roostorg#778 is about the changePassword session-purge, an unrelated concern. - Clarify utils.test.ts's malformed-hash test: the throw-vs-resolve-false split is @node-rs/argon2's own behavior, not something our code special-cases; the test pins it against a silent library upgrade. - Apply the trivial comment-wording suggestion at test line 105. Co-Authored-By: Claude <noreply@anthropic.com>
cf56118 to
d032b7f
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- server/package-lock.json: Generated file
Comments suppressed due to low confidence (1)
server/package.json:46
- This PR introduces a new production dependency (
@node-rs/argon2). Per repo policy, dependency additions/upgrades require explicit human approval (and a quick license/CVE sanity check) before merge.
"@graphql-tools/merge": "^8.2.10",
"@graphql-tools/schema": "^8.5.1",
"@graphql-tools/utils": "^9.2.1",
"@node-rs/argon2": "^2.0.2",
"@node-saml/passport-saml": "^5.1.0",
"@opentelemetry/api": "^1.8.0",
"@opentelemetry/semantic-conventions": "^1.22.0",
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- server/package-lock.json: Generated file
Comments suppressed due to low confidence (1)
server/package.json:44
- This PR introduces a new production dependency (
@node-rs/argon2). Per repo policy, adding or upgrading dependencies (including transitive additions from lockfile changes) requires explicit human approval and a quick license/CVE check before merge.
"@graphql-tools/merge": "^8.2.10",
"@graphql-tools/schema": "^8.5.1",
"@graphql-tools/utils": "^9.2.1",
"@node-rs/argon2": "^2.0.2",
"@node-saml/passport-saml": "^5.1.0",
| const passwordMatches = await passwordMatchesHash(password, user.password); | ||
| if (!passwordMatches) { | ||
| throw makeLoginIncorrectPasswordError({ shouldErrorSpan: true }); | ||
| } |
There was a problem hiding this comment.
This is intentional, not an oversight — see taobojlen's comment above, which asked to drop the special-casing that disguised this as "incorrect password." Going with that: a corrupt/unevaluable hash now surfaces as a generic internal-server error via the existing outer catch-all in verifyEmailPasswordCredentials, and is still logged via the tracer either way. I've updated the PR description (point 4) and the manual-test table to reflect this instead of the old "fail closed as non-match" language.
There was a problem hiding this comment.
yeah this is a bad suggestion from copilot!
- Trim rehashPasswordOnLogin's JSDoc to the load-bearing parts, dropping the changePassword/session-purge tangent and Argon2-variant mention. - Explain exactly which failures the rehash write's catch block is expected to swallow (transient DB/infra failures), rather than swallowing everything unjustified. - Drop the try/catch that disguised a passwordMatchesHash throw as "wrong password"; let it propagate to the existing outer catch-all, which already logs and returns a generic error. - Rename/reclarify the "rehash write fails" test: it was mislabeled as a roostorg#778 regression guard, but roostorg#778 is about the changePassword session-purge, an unrelated concern. - Clarify utils.test.ts's malformed-hash test: the throw-vs-resolve-false split is @node-rs/argon2's own behavior, not something our code special-cases; the test pins it against a silent library upgrade. - Apply the trivial comment-wording suggestion at test line 105. - Address a follow-up Copilot comment: stop computing a real cost-12 bcrypt hash at runtime in the "regardless of cost factor" test (bcryptjs is pure JS and cost 12 is noticeably slow); passwordNeedsRehash never parses the cost out of a bcrypt string, so a string that merely looks like a cost-12 hash proves the same thing for free. Co-Authored-By: Claude <noreply@anthropic.com>
d032b7f to
f4cfcd2
Compare
- Trim rehashPasswordOnLogin's JSDoc to the load-bearing parts, dropping the changePassword/session-purge tangent and Argon2-variant mention. - Explain exactly which failures the rehash write's catch block is expected to swallow (transient DB/infra failures), rather than swallowing everything unjustified. - Drop the try/catch that disguised a passwordMatchesHash throw as "wrong password"; let it propagate to the existing outer catch-all, which already logs and returns a generic error. - Rename/reclarify the "rehash write fails" test: it was mislabeled as a roostorg#778 regression guard, but roostorg#778 is about the changePassword session-purge, an unrelated concern. - Clarify utils.test.ts's malformed-hash test: the throw-vs-resolve-false split is @node-rs/argon2's own behavior, not something our code special-cases; the test pins it against a silent library upgrade. - Apply the trivial comment-wording suggestion at test line 105. - Address a follow-up Copilot comment: stop computing a real cost-12 bcrypt hash at runtime in the "regardless of cost factor" test (bcryptjs is pure JS and cost 12 is noticeably slow); passwordNeedsRehash never parses the cost out of a bcrypt string, so a string that merely looks like a cost-12 hash proves the same thing for free. - Tighten the corrupt-hash test to assert the specific error name (InternalServerError, not LoginIncorrectPasswordError): the previous generic `rejects.toThrow()` passed identically under the old disguised-as-wrong-password behavior and the new one, so it wouldn't have caught a regression back to special-casing this case. Co-Authored-By: Claude <noreply@anthropic.com>
f4cfcd2 to
3c07c94
Compare
| }); | ||
|
|
||
| it('surfaces a generic internal-server error and logs when the stored hash cannot be evaluated', async () => { | ||
| // A corrupt row, or Argon2 failing operationally (the 19 MiB allocation |
There was a problem hiding this comment.
if we don't have 19MiB of memory available then we have bigger problems than users not being able to log in!
| }); | ||
| const { deps, logActiveSpanFailedIfAny } = makeDeps(kyselyPg); | ||
|
|
||
| // `rehashPasswordOnLogin` swallows any error from the write — e.g. a |
There was a problem hiding this comment.
let's simplify this. we can just fail loudly in this case. no need for the added complexity!
| .where('password', '=', verifiedHash) | ||
| .execute(); | ||
| } catch (e) { | ||
| deps.tracer.logActiveSpanFailedIfAny(e); |
There was a problem hiding this comment.
i.e. let's remove the try/catch here.
| const passwordMatches = await passwordMatchesHash(password, user.password); | ||
| if (!passwordMatches) { | ||
| throw makeLoginIncorrectPasswordError({ shouldErrorSpan: true }); | ||
| } |
There was a problem hiding this comment.
yeah this is a bad suggestion from copilot!
Context
Closes #900.
hashPasswordwas minting bcrypt at cost factor 5. This PR originally raised it to 12; per review discussion below, it now moves straight to Argon2id so the hash format only churns once.Argon2id at OWASP's recommended minimum: 19 MiB of memory (
m=19456KiB),t=2,p=1. The parameters are encoded in every hash, so a future bump needs no flag day.Re-hashing requires the plaintext password, which we only hold for the instant of a login — so there is no migration, only an opportunistic upgrade:
create-org) are Argon2id.bcryptjstherefore stays a permanent dependency: an account that never logs in again is never re-hashed.For reviewers
The rehash write is compare-and-swap on the verified hash, not an update by user id. If a concurrent password change lands between verification and the write, zero rows match and the stale rehash is dropped instead of resurrecting the old password.
It is not the transactional path.
changePassword/resetPasswordForTokenalso purge the user's other sessions ((auth) Invalidate sessions on password change #778). A rehash is not a password change; reusing that path would log users out of their other sessions on their first login after deploy.passwordNeedsRehashcomparesv/m/t/pfor strict equality, so hashes converge on exactly one configuration rather than leaving a tail of stronger-but-different ones. This mirrorsneedsRehashin the referenceargon2package — except that one doesn't check the algorithm variant, so anargon2ihash with matching parameters slips past it. Ours is anchored on$argon2id$. Any bcrypt hash is flagged regardless of cost: cost is no longer the criterion, bcrypt is.Corrupt input: one path resolves
false, the other surfaces as an error — both logged.argon2Verifyis inconsistent on corrupt input — some malformed digests resolvefalse(an ordinary wrong-password result), others throw. A throw is not special-cased into "incorrect password": it propagates toverifyEmailPasswordCredentials's existing catch-all, which logs it via the tracer and returns a generic internal-server error instead (per review discussion — no handling was added for a case nobody could explain how to reach). Without that log, a total Argon2 failure (the 19 MiB allocation failing under memory pressure affects every login) would be indistinguishable from a flood of users mistyping their passwords.Dependency:
@node-rs/argon2(MIT) — approval requestedargon2is the more obvious pick, but it is node-gyp native and our server image (node:24.14.1-bullseye-slim) installs onlygit— nopython3/make/g++— so it would depend entirely on a prebuilt binary resolving, with no fallback.@node-rs/argon2ships per-platform binaries as optional deps and has no install script. Same Argon2id, same parameters, interchangeable hashes.Verified rather than assumed:
docker compose build backend→npm cisucceeds in the image and the binding loads and hashes inside it; the lockfile carries all 15 platform binaries, includinglinux-x64-gnufor CI.Trade-off: it exports no
needsRehash, so the ~12 lines that parse the encoded parameters are ours.Node 24 also ships an experimental
crypto.argon2— worth dropping the dependency for once it stabilises.Tests
15 unit tests across
server/services/userManagementService/utils.test.ts(parameters, round-trip, legacy bcrypt verification, corrupt-input handling,passwordNeedsRehashacross bcrypt / parameter drift / wrong variant) andserver/graphql/datasources/userApiCredentials.test.ts(rehash persisted and CAS-scoped, no write when current, no write on wrong password, corrupt hash surfaces a generic error and logs, failed rehash write still succeeds the login).Manual, on a local instance against real Postgres:
create-orgadmin user$argon2id$v=19$m=19456,t=2,p=1$$2a$05$hash$2a$12$hashupdated_atunchanged)LoginIncorrectPasswordError, no writeRollout
Nothing required: no migration, no new env vars, no API/UI change.
public.users.passwordisvarchar(255)and an encoded Argon2id hash is 97 characters. Legacy hashes upgrade lazily as users log in.Expected side effect: the password hashing paths (login, signup, password change) get deliberately slower — that is the point, and it affects only those endpoints.
Checklist
Only check items that apply to this PR; leave the rest unchecked.
If you changed anything user-facing (i.e. user interface or APIs):
Did you update the CHANGELOG.md and related docs?
If you changed
server/models/**/{ContentTypeModel,ActionModel,RuleModel,PolicyModel}.ts:Did you update the corresponding history tables and their triggers?
If you changed
db/src/scripts/**and usedCREATE TABLE,ADD COLUMN, orALTER COLUMN:Are as many columns marked
NOT NULLas possible? If some columns can sometimes be null depending on other columns, are thereCHECKconstraints capturing those relationships, and are these also reflected using unions in the associated Kysely types?If you added a new signal in
server/services/signalsService/signals/**:Did you classify every error case as a permanent error (
SignalPermanentError, no retry) or a normal error (retryable)? Any case where the signal can't determine a score should be aSignalPermanentError.Summary by CodeRabbit