feat/password hash#3902
Conversation
fiftin
commented
May 30, 2026
fiftin
commented
- docs: remove extra docs
- sec(passwords): use Argon2id hash algorithm
- fix(passwords): missed import
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aaa570a8f6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| derived := argon2.IDKey( | ||
| []byte(password), | ||
| salt, | ||
| time, | ||
| memory, | ||
| threads, | ||
| uint32(len(expected)), | ||
| ) |
There was a problem hiding this comment.
When an existing/imported Argon2id row is malformed with t=0, p=0, or an empty final hash field, these parsed values are passed directly into argon2.IDKey; x/crypto/argon2 requires CPU cost/parallelism (and effectively output length) to be greater than zero and panics otherwise. In that state a single password login for the affected user can panic the request instead of returning ErrMalformedHash as the verifier contract says, so reject zero/empty parameters before deriving.
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Security review — PR #3902 (feat/password hash)
Outcome: No medium, high, or critical vulnerabilities identified in this change.
This PR replaces bcrypt generation with a dedicated pkg/password_hash package (Argon2id for new hashes, bcrypt verification for legacy hashes, transparent rehash on login). Prior automation review threads on this PR were absent; nothing to re-validate.
What was reviewed
pkg/password_hash/password_hash.go—Hash/Verify/verifyArgon2idapi/login.go—loginByPasswordrehash pathdb/sql/user.go,db/bolt/user.go— password persistence call sites
Why no findings were raised
| Area | Result |
|---|---|
| Auth bypass | Verify uses subtle.ConstantTimeCompare for Argon2id; malformed/unknown hashes and failed matches are treated like wrong password (db.ErrNotFound). External users are still rejected before verify. |
| Secret handling | Password hashes are not exposed via JSON (json:"-" on User.Password). Rehash failure logs only user_id, not the password. |
| Injection | No new SQL/query construction; parameterized updates unchanged. |
| Hash planting | All API/CLI write paths (CreateUser, UpdateUser, SetUserPassword) call password_hash.Hash() with fixed parameters. ImportUser can persist arbitrary stored hashes, but that path predates this PR and project web restore does not import user password fields. |
Argon2 DoS via crafted m/t/p |
verifyArgon2id trusts parameters embedded in the stored hash, so a DB-level write (or migration from a compromised source) could make login CPU/memory expensive. That is defense-in-depth, not a new unauthenticated or low-privilege vector introduced here; bcrypt import had a similar property with high cost factors. |
Positive security changes
- New passwords use Argon2id with reasonable defaults (~19 MiB,
t=2,p=1). - Legacy bcrypt users remain compatible; successful login upgrades weak hashes automatically.
Slack summary
PR #3902 security review — clean. No medium+ issues found. Argon2id password hashing with bcrypt back-compat and login-time rehash; auth boundary and constant-time compare look sound. No action required from a security standpoint.
Sent by Cursor Automation: Find vulnerabilities
1 participant
