Add admin password change flow (closes #65) #83
Reference in New Issue
Block a user
Delete Branch "issue-65-password-change"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Adds an authenticated, CSRF-protected flow that lets a user change their own password from the profile page.
Route
POST /passwordunder the/user/{username}group insetupUserRoutes(internal/server/routes.go). That group already appliesCSRF,NoCache, andRequireAuth, so the new endpoint inherits all three.Handler (
internal/handlers/profile.go)HandlePasswordChangeenforces own-user access: the{username}path parameter must equal the session username (same 403 ruleHandleProfileuses). This check plus the session lookup is factored into a sharedprofileOwnerOrDenyhelper now used by both handlers.current_password,new_password, andconfirm_password(body size limited viahttp.MaxBytesReader).database.VerifyPasswordagainst the stored hash.database.HashPassword— the same Argon2id helper used to bootstrap the admin user — and persists it on the user row. No new crypto.Template (
templates/profile.html)csrf_token(matching the login form's CSRF embedding).alert-success/alert-errorstyles. No new CSS classes, so no Tailwind rebuild is required.Tests (
internal/handlers/profile_test.go)TestHandlePasswordChange_Success: seeds a user, posts a valid change, asserts success message and that the stored hash changed and verifies against the new password.TestHandlePasswordChange_WrongCurrentPassword: posts a wrong current password, asserts the rejection message and that the stored hash is unchanged.Validated with
docker build .(fmt-check, lint, test, build) — exit 0.Closes #65
File-by-file summary:
internal/server/routes.go: addedr.Post("/password", s.h.HandlePasswordChange())tosetupUserRoutes, so it inherits the group'sCSRF,NoCache, andRequireAuthmiddleware.internal/handlers/profile.go: addedHandlePasswordChangeand anapplyPasswordChangehelper (load user, verify withdatabase.VerifyPassword, require non-empty new password equal to confirmation, hash withdatabase.HashPassword, persist on the row). Extracted the shared session/own-user check intoprofileOwnerOrDenyand arenderProfilehelper, both reused byHandleProfile. Request body is bounded withhttp.MaxBytesReaderbeforeParseForm.templates/profile.html: added a "Change Password" card (current / new / confirm fields + hiddencsrf_token) and success/error alerts using the existingalert-success/alert-errorclasses.internal/handlers/profile_test.go: addedTestHandlePasswordChange_Success(hash changes, new password verifies) andTestHandlePasswordChange_WrongCurrentPassword(rejected, stored hash unchanged), plus apasswordChangeRequesthelper.Validation:
docker build .(fmt-check + lint + test + build) exited 0.Independent review — PASS (merge-ready)
Adversarial review against the issue spec and repo policies.
POST /passwordadded under/user/{username}insetupUserRoutes, inheritingCSRF+RequireAuth+NoCache.HandlePasswordChange: own-user enforced viaprofileOwnerOrDeny(same 403 rule asHandleProfile); body limited viaMaxBytesReader; verifies the current password withdatabase.VerifyPassword; requires the new password non-empty and equal to the confirmation; hashes withdatabase.HashPassword(the Argon2id helper that bootstraps the admin) and persists it. It loads the user by the session username, so there is no IDOR — a user can only change their own password.{{.CSRFToken}}is populated.renderProfilepasses a map, andrenderTemplateinjectsCSRFToken(middleware.CSRFToken(r)) into map data, so the form submits a valid token in production and the CSRF middleware accepts it.profileOwnerOrDenyandrenderProfileare extracted and reused byHandleProfile, whose behaviour (own-profile 200, other 403, unauthenticated handled by middleware) is preserved.make fmtclean;docker build .green (which also confirms the integratedmaincompiles); no AI/tooling references; commit closes the issue.Non-blocking notes: there is no password strength/min-length rule (only non-empty plus confirmation match), which is fine per the DoD; and a password change does not invalidate other sessions, reasonable to defer. The host-linter
goconstwarnings the author flagged are pre-existing in untouched files (CI pins an older linter and stays green).Verdict: meets the bar. Marking merge-ready and handing to @sneak for final review.
Independent adversarial review (second, independent pass)
Reviewed the full diff at head
3084ed5against issue #65 (body and implementation comment), REPO_POLICIES.md, and repo conventions. This review is independent of the earlier one on this PR.Verified correct:
main(base81413c5ismainHEAD); CI green on head3084ed5(check / checksuccess).POST /user/{username}/passwordsits insetupUserRoutes, inheritingCSRF,NoCache, andRequireAuthexactly as the issue specifies.profileOwnerOrDenyrequires the path username to equal the session username (same 403 rule asHandleProfile), andapplyPasswordChangeloads the user row by the session username — no way to change another user's password.database.VerifyPassword; new password required non-empty and equal to confirmation; rejected paths leave the stored hash untouched; new hash produced bydatabase.HashPassword(the same Argon2id helper that bootstraps the admin). No new crypto.renderProfilepasses map data, andrenderTemplate(internal/handlers/handlers.go:202-206) injectsCSRFTokeninto map data, so the template's{{.CSRFToken}}renders a real token and production form posts pass the CSRF middleware.http.MaxBytesReader(1 MB) beforeParseForm; internal failures go throughserverError(generic 500, details only in logs — no leak).(closes #65), no AI/tooling references or trailers anywhere in commit, diff, or PR body.HandleProfilerefactor (profileOwnerOrDeny,renderProfile) preserves prior behaviour.Required change (policy):
POST /passwordaccepts and verifies a password. As shipped, an attacker holding a hijacked session cookie can brute-force the current password at unbounded rate (each guess is a cheap 200 response with "Current password is incorrect."), escalating session theft into full account takeover. The login endpoint already demonstrates the required pattern (LoginRateLimitininternal/middleware/ratelimit.go); the same per-IP POST limit must cover this route.Non-blocking notes (fine to leave):
Verdict: needs-rework for the rate-limiting gap. Everything else passes. I will apply the fix on this branch myself; per process the PR then goes back to needs-review for a fresh independent pass rather than merge-ready.
The password change POST verifies the current password, making it a password-based authentication endpoint that REPO_POLICIES.md requires rate limiting on. Extract the login limiter's POST-only per-IP pattern into a shared postRateLimit helper and apply the same 5-per-minute limit to POST /user/{username}/password.Rework applied
Pushed
8362ce9addressing the required change from the review above:internal/middleware/ratelimit.go: extracted the login limiter's POST-only, per-IP pattern into a sharedpostRateLimithelper (limit, interval, log message, and response message as parameters) and addedPasswordChangeRateLimit()using the same 5-per-minute per-IP limit as login.LoginRateLimit()now delegates to the helper; its behaviour (POST-only,KeyByRealIP, 429 with a clear message, warn log) is unchanged.internal/server/routes.go:POST /user/{username}/passwordis now wrapped withs.mw.PasswordChangeRateLimit()viar.With(...); the profile GET is unaffected.internal/middleware/ratelimit_test.go: addedTestPasswordChangeRateLimit_LimitsPOSTand factored the shared exercise logic into arunPostLimitTesthelper reused by the login test (avoids duplicated test bodies).internal/middleware/export_test.go: exposed the new limit constant for the test.Validation:
make fmtapplied;make testandmake fmt-checkpass locally. The host linter reports only the 17 pre-existinggoconstfindings in files this PR does not touch (host golangci-lint is newer than the pinned CI image); no findings in any file changed here. CI on the new head is the authoritative gate.Setting needs-review: this rework was authored by me, so a fresh independent review must gate merge-ready.
Independent adversarial review (third pass, post-rework)
Verdict: PASS
Reviewed the full diff at head
8362ce9(both commits,3084ed5+8362ce9) against issue #65, all prior review comments on this PR, and REPO_POLICIES.md. Special focus: whether the rework actually fixes the one required change from the previous review (missing rate limiting on the password-change endpoint).Rework verification (the prior blocking finding)
Confirmed fixed, in code, not just claimed:
internal/server/routes.go:POST /user/{username}/passwordis wrapped withs.mw.PasswordChangeRateLimit()viar.With(...). Middleware order in the group is CSRF -> NoCache -> RequireAuth -> rate limit -> handler, so the limiter counts exactly the requests that could actually probe a password. The profile GET is not limited.internal/middleware/ratelimit.go:PasswordChangeRateLimit()uses the same 5-per-minute per-IP POST-only pattern as login, via a new sharedpostRateLimithelper. I diffedLoginRateLimit's behaviour before/after the refactor: POST-only gate,KeyByRealIPkeying, 429 body text, and warn log are all preserved. The two endpoints get separate limiter instances (separate buckets), so login attempts do not consume the password-change budget or vice versa.internal/middleware/ratelimit_test.go:TestPasswordChangeRateLimit_LimitsPOSTdrives the new limiter to the limit, asserts the next POST is 429 and never reaches the handler; the sharedrunPostLimitTesthelper keeps the login test's assertions intact.8362ce9) touches only the four rate-limit-related files — no scope creep, no behaviour drift in the password handler itself.Full-PR verification (independent of prior reviews)
profileOwnerOrDeny: path username must equal session username, 403 otherwise; the user row is loaded by the session username, so there is no cross-user write path); current password verified viadatabase.VerifyPassword; new password required non-empty and equal to confirmation; new hash viadatabase.HashPassword(the same Argon2id helper that bootstraps the admin); clear success/error messages; rejected paths leave the stored hash untouched. All present.renderProfilepasses map data andrenderTemplateinjectsCSRFTokeninto map data, so the template's{{.CSRFToken}}renders a real token and production posts pass the gorilla/csrf middleware.serverError(generic 500, details only in logs). Error messages to the user are specific but leak nothing.8362ce9(check / check, 2m40s). I independently ranscript/cibuild(docker build .) on the checked-out head: fmt-check, lint, tests, and build all passed, exit 0. PR is mergeable; base81413c5is currentmainHEAD.(closes #65); the rework commit references#65without a duplicatecloses, which is correct. No AI/tooling references or attribution trailers anywhere in commits, diff, or PR body. Naming is consistent with the surrounding middleware (PasswordChangeRateLimitmirrorsLoginRateLimit; no stutter). No config parsing is touched, so the fail-loud-config rule is not implicated.Non-blocking observations (follow-up material, not defects of this PR)
httprate.KeyByRealIPhonoursTrue-Client-IP,X-Real-IP, and the firstX-Forwarded-Forentry unconditionally. Unless the reverse proxy strips/overwrites all three, an attacker with a stolen session can rotate these headers to get a fresh bucket per request, which substantially weakens both this limiter and the pre-existing login limiter against the exact brute-force threat they target. REPO_POLICIES.md requires forwarded headers to be trusted only from configured trusted proxies. This is the established pattern already onmain(login) and is precisely what the prior review prescribed for this PR, so it is not a defect of this change — but it deserves its own issue covering both endpoints (trusted-proxy allowlist, falling back toRemoteAddr).http.MaxBytesReader(1 MB) is installed after gorilla/csrf has already consumed the POST body (r.PostFormValuein the CSRF token check), so the effective bound on this route is net/http's 10 MB urlencoded-form cap, not 1 MB. The login handler onmainhas the identical latent idiom, and every CSRF-protected group mountsMaxBodySizeafterCSRFanyway, so this is a pre-existing repo-wide ordering question, not a regression here. A follow-up could move body limiting ahead of CSRF.Verdict: PASS. The prior blocking finding is genuinely fixed and everything else holds up.
Manager note: independent re-review above (comment 43912) verified the rate-limiting rework in
8362ce9in code and passed the PR with no blocking findings;script/cibuildwas independently run by the reviewer (exit 0) and CI is green on the head. Settingmerge-readyand assigning to sneak for merge (protectedmain).Non-blocking observations from the review are now tracked: forwarded-header trust in the rate limiters as #88, and the
MaxBytesReader-after-CSRF ordering as #90. Neither blocks this PR.@clawbot please file an issue with the security caveats here so we can fix them later.