Fix/password policy hash 330#331
Fix/password policy hash 330#331minhpham1810 wants to merge 4 commits intocontainer-registry:mainfrom
Conversation
Signed-off-by: Minh Pham <kp025@bucknell.edu>
github-actions
bot
added
documentation
📝 WalkthroughWalkthroughCentralizes password policy validation inside Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Codacy's Analysis Summary0 new issue (≤ 0 issue) Review Pull Request in Codacy →
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
ground-control/internal/auth/password.go (1)
20-25:LoadPolicyFromEnv()is called on every hash invocation.This re-reads ~6 environment variables per call. Given that Argon2 hashing dominates the cost, the overhead is negligible. However, if this concern grows (e.g., policy becomes more complex), consider accepting the policy as a parameter or caching it.
One design trade-off worth noting: this makes
HashPasswordimplicitly dependent on the process environment, which can make unit testing less deterministic unless env vars are explicitly set/unset in each test. Currently the tests rely on default policy, which works — just be aware if the defaults ever change, tests may break silently.ground-control/internal/auth/password_test.go (2)
77-83: Fragile test: pass/fail depends on an implicit default policy value.The comment on Line 82 ("flip to true if you change this to exceed policy MaxLength") shifts the burden to future maintainers. If
DefaultPolicy().MaxLengthis ever lowered below the length of this string (~90 chars), the test breaks with a non-obvious error.Consider either:
- Explicitly generating a password that exceeds
DefaultPolicy().MaxLengthas a separatewantErr: truecase, or- Asserting the length against the loaded policy in the test itself.
94-97: Consider asserting the error type is*PasswordPolicyError.Callers (
bootstrap.go,user_handlers.go) rely onerrors.As(err, &pe)to distinguish policy violations from hashing failures. The tests currently only checkrequire.Error(t, err)without verifying the error type, so a regression that returns a plain error instead ofPasswordPolicyErrorwould go undetected.💡 Suggested addition
if tt.wantErr { require.Error(t, err) + var pe *PasswordPolicyError + require.True(t, errors.As(err, &pe), "expected PasswordPolicyError, got %T", err) return }(Requires adding
"errors"to the import block.)ground-control/internal/server/user_handlers.go (2)
206-230: New password is hashed before verifying the current password.
HashPassword(req.NewPassword)(Line 206) runs the expensive Argon2 computation beforeVerifyPassword(req.CurrentPassword, ...)(Line 226). If the current password is wrong, the hash work is wasted. More importantly from a UX perspective, the user could receive a "new password doesn't meet policy" error when their real problem is an incorrect current password.Consider swapping the order: verify the current password first, then hash the new one.
♻️ Suggested reorder
- hash, err := auth.HashPassword(req.NewPassword) - if err != nil { - var pe *auth.PasswordPolicyError - if errors.As(err, &pe) { - WriteJSONError(w, pe.Error(), http.StatusBadRequest) - return - } - - WriteJSONError(w, "Internal server error", http.StatusInternalServerError) - return - } - - // Verify current password user, err := s.dbQueries.GetUserByUsername(r.Context(), currentUser.Username) if err != nil { WriteJSONError(w, "Internal server error", http.StatusInternalServerError) return } valid := auth.VerifyPassword(req.CurrentPassword, user.PasswordHash) if !valid { WriteJSONError(w, "Current password is incorrect", http.StatusUnauthorized) return } + hash, err := auth.HashPassword(req.NewPassword) + if err != nil { + var pe *auth.PasswordPolicyError + if errors.As(err, &pe) { + WriteJSONError(w, pe.Error(), http.StatusBadRequest) + return + } + WriteJSONError(w, "Internal server error", http.StatusInternalServerError) + return + } if err := s.dbQueries.UpdateUserPassword(r.Context(), database.UpdateUserPasswordParams{
60-70: Consider extracting a helper for the repeatedPasswordPolicyErrorhandling pattern.The same
errors.As→ 400 / fallback → 500 block is repeated increateUserHandler,changeOwnPasswordHandler,changeUserPasswordHandler, andbootstrap.go. A small helper would reduce duplication:💡 Example helper
// handleHashError writes the appropriate error response for a HashPassword failure. // Returns true if the error was handled (response written). func handleHashError(w http.ResponseWriter, err error) { var pe *auth.PasswordPolicyError if errors.As(err, &pe) { WriteJSONError(w, pe.Error(), http.StatusBadRequest) return } WriteJSONError(w, "Internal server error", http.StatusInternalServerError) }Then each handler simplifies to:
hash, err := auth.HashPassword(req.Password) if err != nil { handleHashError(w, err) return }
Signed-off-by: Minh Pham <kp025@bucknell.edu>
minhpham1810
force-pushed
the
fix/password-policy-hash-330
branch
from
5403956 to
b0b9c81
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ground-control/internal/server/user_handlers.go (1)
206-230:⚠️ Potential issue | 🟠 MajorVerify current password before hashing the new one.
The new password is hashed (an expensive Argon2 operation) at Line 206 before the current password is verified at Line 226. If the current password is wrong, the hashing work is wasted. More importantly, the logical flow should authenticate the user's identity before processing the password change.
Swap the order: verify the current password first, then hash the new password.
Suggested reorder
- hash, err := auth.HashPassword(req.NewPassword) - if err != nil { - var pe *auth.PasswordPolicyError - if errors.As(err, &pe) { - WriteJSONError(w, pe.Error(), http.StatusBadRequest) - return - } - - WriteJSONError(w, "Internal server error", http.StatusInternalServerError) - return - } - - // Verify current password user, err := s.dbQueries.GetUserByUsername(r.Context(), currentUser.Username) if err != nil { WriteJSONError(w, "Internal server error", http.StatusInternalServerError) return } valid := auth.VerifyPassword(req.CurrentPassword, user.PasswordHash) if !valid { WriteJSONError(w, "Current password is incorrect", http.StatusUnauthorized) return } + hash, err := auth.HashPassword(req.NewPassword) + if err != nil { + var pe *auth.PasswordPolicyError + if errors.As(err, &pe) { + WriteJSONError(w, pe.Error(), http.StatusBadRequest) + return + } + + WriteJSONError(w, "Internal server error", http.StatusInternalServerError) + return + } if err := s.dbQueries.UpdateUserPassword(r.Context(), database.UpdateUserPasswordParams{
🧹 Nitpick comments (4)
ground-control/internal/auth/password.go (1)
20-25:LoadPolicyFromEnv()is called on every hash invocation.This re-reads ~6 environment variables and parses them on every
HashPasswordcall. Since the policy doesn't change at runtime, consider loading it once (e.g., at startup or viasync.Once) and passing it in or caching it. Not a correctness issue, but unnecessary repeated I/O on a hot path.ground-control/internal/auth/password_test.go (2)
94-97: Consider asserting the error type, not just its existence.When
wantErris true, the test only checks that an error occurred. Asserting that the error is a*PasswordPolicyErrorviaerrors.Aswould strengthen the test and verify the contract introduced by this PR.💡 Suggested improvement
if tt.wantErr { require.Error(t, err) + var pe *auth.PasswordPolicyError + require.True(t, errors.As(err, &pe), "expected PasswordPolicyError, got %T", err) return }(Would also need to add
"errors"and theauthimport if the test is in the same package — since it'spackage auth, just"errors"is needed.)
77-82: Fragile test: behavior depends on unset environment variables.This test (and others) implicitly depend on
PASSWORD_*env vars not being set so thatDefaultPolicy()is used. If CI or a developer's shell exports any of these vars, test results change silently. Consider explicitly setting the relevant env vars in tests usingt.Setenv()to make expectations deterministic.ground-control/internal/server/user_handlers.go (1)
62-66: Optional: Extract duplicatedPasswordPolicyErrorhandling into a helper.The same
errors.As→ 400 / fallback → 500 pattern is repeated in three handlers. A small helper likewriteHashError(w, err)would reduce duplication and make future changes (e.g., logging) easier to apply consistently.Also applies to: 208-212, 264-268
There was a problem hiding this comment.
Review
The goal of centralizing password policy validation inside `HashPassword()` is sound, but there are a few issues worth addressing.
Critical: Logic ordering bug in `changeOwnPasswordHandler`
The PR reorders operations in a way that runs Argon2id hashing before verifying the current password:
Before:
validate policy -> get user -> verify current password -> hash new password -> update
After:
hash new password (argon2id!) -> get user -> verify current password -> update
Argon2id is intentionally expensive (19 MiB, 2 iterations per ADR-0004). Any authenticated session can now trigger that computation with a wrong `current_password` repeatedly. Fail-fast is violated - the old order was correct.
Hidden dependency in `HashPassword()`
func HashPassword(password string) (string, error) {
policy := LoadPolicyFromEnv() // reads os.Getenv on every call
...
}Previously s.passwordPolicy was injected at startup (explicit dependency). Now HashPassword() reads env vars as a hidden side effect on every invocation. This makes testing env-dependent (the test comment even acknowledges this: "flip to true if you change this to exceed policy MaxLength"). ADR-0004 describes configurable policy via env vars, but the intent was explicit injection, not re-reading env on every hash.
Policy coupling limits HashPassword()
HashPassword() is now semantically "validate + hash" rather than just a crypto primitive. Any future call site that needs to hash a pre-validated password (migrations, seeding, admin override) cannot bypass the policy check. Policy enforcement is an application-layer concern; it should not be baked into the crypto primitive.
Style
- Double blank lines introduced in several places in
user_handlers.go password.gouses spaces for the new code block while the rest of the file uses tabs
Suggestion
The centralization goal is worth pursuing. A cleaner approach would be to keep HashPassword() as a pure crypto function and introduce a ValidateAndHashPassword(policy PasswordPolicy, password string) wrapper at the application layer - or at minimum fix the operation ordering in changeOwnPasswordHandler so current password verification happens before the expensive hash.
The OWASP parameters (Argon2id, 19 MiB, 2 iterations) and the configurable policy env vars are unchanged and remain compliant with ADR-0004.
|
@minhpham1810 #331 (review) - update the PR according to this comment Thanks |
|
@bupd thank you for the feedback. I will review and make changes. |
2 participants
Description
This PR implements Issue #330 by enforcing the configured password policy directly inside
auth.HashPassword().Additional context
Changes
Added password policy validation inside auth.HashPassword()
Introduced
auth.PasswordPolicyErrorso callers can treat policy violations as client errors (400) while keeping unexpected hashing failures as server errors (500)Updated password hashing tests to reflect the new behavior (invalid passwords can no longer be hashed)
Why
Previously, password validation was performed inconsistently at call sites, which risked missing validation in new code paths. Centralizing validation in
HashPassword()ensures all password hashes in the system enforce the same policy.Testing
go test ./... in ground-control/Fixes #330.
Summary by cubic
Centralized password policy enforcement in auth.HashPassword with PasswordPolicyError so policy violations return 400 and hashing failures return 500. Handlers and bootstrap now rely on this; tests updated; added a Go Report Card badge; fixes #330.
Written for commit 1788a1f. Summary will update on new commits.
Summary by CodeRabbit
Documentation
Bug Fixes
Tests