Adds an authenticated, CSRF-protected flow that lets a user change their own password from the profile page.
Route
New POST /password under the /user/{username} group in setupUserRoutes (internal/server/routes.go). That group already applies CSRF, NoCache, and RequireAuth, so the new endpoint inherits all three.
Handler (internal/handlers/profile.go)
HandlePasswordChange enforces own-user access: the {username} path parameter must equal the session username (same 403 rule HandleProfile uses). This check plus the session lookup is factored into a shared profileOwnerOrDeny helper now used by both handlers.
Parses current_password, new_password, and confirm_password (body size limited via http.MaxBytesReader).
Verifies the current password with database.VerifyPassword against the stored hash.
Requires the new password to be non-empty and equal to the confirmation.
Hashes the new password with database.HashPassword — the same Argon2id helper used to bootstrap the admin user — and persists it on the user row. No new crypto.
Re-renders the profile page with a clear success or error message. Wrong current password, empty new password, and mismatched confirmation are each rejected with their own message and leave the stored hash unchanged.
Template (templates/profile.html)
Adds a "Change Password" card with current / new / confirm password fields plus the hidden csrf_token (matching the login form's CSRF embedding).
Renders success/error alerts using the existing alert-success / alert-error styles. 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.
Adds an authenticated, CSRF-protected flow that lets a user change their own password from the profile page.
## Route
- New `POST /password` under the `/user/{username}` group in `setupUserRoutes` (`internal/server/routes.go`). That group already applies `CSRF`, `NoCache`, and `RequireAuth`, so the new endpoint inherits all three.
## Handler (`internal/handlers/profile.go`)
- `HandlePasswordChange` enforces own-user access: the `{username}` path parameter must equal the session username (same 403 rule `HandleProfile` uses). This check plus the session lookup is factored into a shared `profileOwnerOrDeny` helper now used by both handlers.
- Parses `current_password`, `new_password`, and `confirm_password` (body size limited via `http.MaxBytesReader`).
- Verifies the current password with `database.VerifyPassword` against the stored hash.
- Requires the new password to be non-empty and equal to the confirmation.
- Hashes the new password with `database.HashPassword` — the same Argon2id helper used to bootstrap the admin user — and persists it on the user row. No new crypto.
- Re-renders the profile page with a clear success or error message. Wrong current password, empty new password, and mismatched confirmation are each rejected with their own message and leave the stored hash unchanged.
## Template (`templates/profile.html`)
- Adds a "Change Password" card with current / new / confirm password fields plus the hidden `csrf_token` (matching the login form's CSRF embedding).
- Renders success/error alerts using the existing `alert-success` / `alert-error` styles. 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
internal/server/routes.go: added r.Post("/password", s.h.HandlePasswordChange()) to setupUserRoutes, so it inherits the group's CSRF, NoCache, and RequireAuth middleware.
internal/handlers/profile.go: added HandlePasswordChange and an applyPasswordChange helper (load user, verify with database.VerifyPassword, require non-empty new password equal to confirmation, hash with database.HashPassword, persist on the row). Extracted the shared session/own-user check into profileOwnerOrDeny and a renderProfile helper, both reused by HandleProfile. Request body is bounded with http.MaxBytesReader before ParseForm.
templates/profile.html: added a "Change Password" card (current / new / confirm fields + hidden csrf_token) and success/error alerts using the existing alert-success / alert-error classes.
internal/handlers/profile_test.go: added TestHandlePasswordChange_Success (hash changes, new password verifies) and TestHandlePasswordChange_WrongCurrentPassword (rejected, stored hash unchanged), plus a passwordChangeRequest helper.
File-by-file summary:
- `internal/server/routes.go`: added `r.Post("/password", s.h.HandlePasswordChange())` to `setupUserRoutes`, so it inherits the group's `CSRF`, `NoCache`, and `RequireAuth` middleware.
- `internal/handlers/profile.go`: added `HandlePasswordChange` and an `applyPasswordChange` helper (load user, verify with `database.VerifyPassword`, require non-empty new password equal to confirmation, hash with `database.HashPassword`, persist on the row). Extracted the shared session/own-user check into `profileOwnerOrDeny` and a `renderProfile` helper, both reused by `HandleProfile`. Request body is bounded with `http.MaxBytesReader` before `ParseForm`.
- `templates/profile.html`: added a "Change Password" card (current / new / confirm fields + hidden `csrf_token`) and success/error alerts using the existing `alert-success` / `alert-error` classes.
- `internal/handlers/profile_test.go`: added `TestHandlePasswordChange_Success` (hash changes, new password verifies) and `TestHandlePasswordChange_WrongCurrentPassword` (rejected, stored hash unchanged), plus a `passwordChangeRequest` helper.
Validation: `docker build .` (fmt-check + lint + test + build) exited 0.
Adversarial review against the issue spec and repo policies.
Route: POST /password added under /user/{username} in setupUserRoutes, inheriting CSRF + RequireAuth + NoCache.
HandlePasswordChange: own-user enforced via profileOwnerOrDeny (same 403 rule as HandleProfile); body limited via MaxBytesReader; verifies the current password with database.VerifyPassword; requires the new password non-empty and equal to the confirmation; hashes with database.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.
Verified the non-obvious point the tests cannot (they call the handler directly, bypassing CSRF): the form's {{.CSRFToken}} is populated. renderProfile passes a map, and renderTemplate injects CSRFToken (middleware.CSRFToken(r)) into map data, so the form submits a valid token in production and the CSRF middleware accepts it.
Refactor is clean: profileOwnerOrDeny and renderProfile are extracted and reused by HandleProfile, whose behaviour (own-profile 200, other 403, unauthenticated handled by middleware) is preserved.
Tests: the happy path asserts the stored hash changes and the new password verifies; the wrong-current-password path asserts rejection with the stored hash unchanged. make fmt clean; docker build . green (which also confirms the integrated main compiles); 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 goconst warnings 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 review — PASS (merge-ready)
Adversarial review against the issue spec and repo policies.
- Route: `POST /password` added under `/user/{username}` in `setupUserRoutes`, inheriting `CSRF` + `RequireAuth` + `NoCache`.
- `HandlePasswordChange`: own-user enforced via `profileOwnerOrDeny` (same 403 rule as `HandleProfile`); body limited via `MaxBytesReader`; verifies the current password with `database.VerifyPassword`; requires the new password non-empty and equal to the confirmation; hashes with `database.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.
- Verified the non-obvious point the tests cannot (they call the handler directly, bypassing CSRF): the form's `{{.CSRFToken}}` is populated. `renderProfile` passes a map, and `renderTemplate` injects `CSRFToken` (`middleware.CSRFToken(r)`) into map data, so the form submits a valid token in production and the CSRF middleware accepts it.
- Refactor is clean: `profileOwnerOrDeny` and `renderProfile` are extracted and reused by `HandleProfile`, whose behaviour (own-profile 200, other 403, unauthenticated handled by middleware) is preserved.
- Tests: the happy path asserts the stored hash changes and the new password verifies; the wrong-current-password path asserts rejection with the stored hash unchanged. `make fmt` clean; `docker build .` green (which also confirms the integrated `main` compiles); 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 `goconst` warnings 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.
sneak
was assigned by clawbot2026-08-07 17:56:51 +02:00
Reviewed the full diff at head 3084ed5 against 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:
Mergeable against current main (base 81413c5 is main HEAD); CI green on head 3084ed5 (check / check success).
Route: POST /user/{username}/password sits in setupUserRoutes, inheriting CSRF, NoCache, and RequireAuth exactly as the issue specifies.
Own-user enforcement: profileOwnerOrDeny requires the path username to equal the session username (same 403 rule as HandleProfile), and applyPasswordChange loads the user row by the session username — no way to change another user's password.
Current password verified via database.VerifyPassword; new password required non-empty and equal to confirmation; rejected paths leave the stored hash untouched; new hash produced by database.HashPassword (the same Argon2id helper that bootstraps the admin). No new crypto.
I verified the CSRF wiring the tests bypass: renderProfile passes map data, and renderTemplate (internal/handlers/handlers.go:202-206) injects CSRFToken into map data, so the template's {{.CSRFToken}} renders a real token and production form posts pass the CSRF middleware.
Body bounded with http.MaxBytesReader (1 MB) before ParseForm; internal failures go through serverError (generic 500, details only in logs — no leak).
Tests are present and meaningful: happy path asserts the stored hash changed AND the new password verifies; wrong-current-password asserts rejection AND unchanged hash. Matches the issue DoD.
Commit hygiene: single commit, subject ends with (closes #65), no AI/tooling references or trailers anywhere in commit, diff, or PR body.
No rate limiting on the new password-verification endpoint. REPO_POLICIES.md requires rate limiting on password-based authentication endpoints, and POST /password accepts 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 (LoginRateLimit in internal/middleware/ratelimit.go); the same per-IP POST limit must cover this route.
Non-blocking notes (fine to leave):
No PRG redirect after POST — a page refresh reposts the form. Cosmetic; consistent with the DoD wording ("re-render the profile page").
A successful password change does not invalidate other active sessions; reasonable to defer for a single-admin service, worth a follow-up issue if multi-user lands.
No minimum password length or strength rule (only non-empty + confirmation match), which is what the DoD asked for.
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.
## Independent adversarial review (second, independent pass)
Reviewed the full diff at head 3084ed5 against 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:
- Mergeable against current `main` (base 81413c5 is `main` HEAD); CI green on head 3084ed5 (`check / check` success).
- Route: `POST /user/{username}/password` sits in `setupUserRoutes`, inheriting `CSRF`, `NoCache`, and `RequireAuth` exactly as the issue specifies.
- Own-user enforcement: `profileOwnerOrDeny` requires the path username to equal the session username (same 403 rule as `HandleProfile`), and `applyPasswordChange` loads the user row by the session username — no way to change another user's password.
- Current password verified via `database.VerifyPassword`; new password required non-empty and equal to confirmation; rejected paths leave the stored hash untouched; new hash produced by `database.HashPassword` (the same Argon2id helper that bootstraps the admin). No new crypto.
- I verified the CSRF wiring the tests bypass: `renderProfile` passes map data, and `renderTemplate` (internal/handlers/handlers.go:202-206) injects `CSRFToken` into map data, so the template's `{{.CSRFToken}}` renders a real token and production form posts pass the CSRF middleware.
- Body bounded with `http.MaxBytesReader` (1 MB) before `ParseForm`; internal failures go through `serverError` (generic 500, details only in logs — no leak).
- Tests are present and meaningful: happy path asserts the stored hash changed AND the new password verifies; wrong-current-password asserts rejection AND unchanged hash. Matches the issue DoD.
- Commit hygiene: single commit, subject ends with `(closes #65)`, no AI/tooling references or trailers anywhere in commit, diff, or PR body.
- Formatting matches repo style; `HandleProfile` refactor (`profileOwnerOrDeny`, `renderProfile`) preserves prior behaviour.
Required change (policy):
1. No rate limiting on the new password-verification endpoint. REPO_POLICIES.md requires rate limiting on password-based authentication endpoints, and `POST /password` accepts 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 (`LoginRateLimit` in `internal/middleware/ratelimit.go`); the same per-IP POST limit must cover this route.
Non-blocking notes (fine to leave):
- No PRG redirect after POST — a page refresh reposts the form. Cosmetic; consistent with the DoD wording ("re-render the profile page").
- A successful password change does not invalidate other active sessions; reasonable to defer for a single-admin service, worth a follow-up issue if multi-user lands.
- No minimum password length or strength rule (only non-empty + confirmation match), which is what the DoD asked for.
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.
Pushed 8362ce9 addressing the required change from the review above:
internal/middleware/ratelimit.go: extracted the login limiter's POST-only, per-IP pattern into a shared postRateLimit helper (limit, interval, log message, and response message as parameters) and added PasswordChangeRateLimit() 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}/password is now wrapped with s.mw.PasswordChangeRateLimit() via r.With(...); the profile GET is unaffected.
internal/middleware/ratelimit_test.go: added TestPasswordChangeRateLimit_LimitsPOST and factored the shared exercise logic into a runPostLimitTest helper reused by the login test (avoids duplicated test bodies).
internal/middleware/export_test.go: exposed the new limit constant for the test.
Validation: make fmt applied; make test and make fmt-check pass locally. The host linter reports only the 17 pre-existing goconst findings 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.
## Rework applied
Pushed 8362ce9 addressing the required change from the review above:
- `internal/middleware/ratelimit.go`: extracted the login limiter's POST-only, per-IP pattern into a shared `postRateLimit` helper (limit, interval, log message, and response message as parameters) and added `PasswordChangeRateLimit()` 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}/password` is now wrapped with `s.mw.PasswordChangeRateLimit()` via `r.With(...)`; the profile GET is unaffected.
- `internal/middleware/ratelimit_test.go`: added `TestPasswordChangeRateLimit_LimitsPOST` and factored the shared exercise logic into a `runPostLimitTest` helper reused by the login test (avoids duplicated test bodies).
- `internal/middleware/export_test.go`: exposed the new limit constant for the test.
Validation: `make fmt` applied; `make test` and `make fmt-check` pass locally. The host linter reports only the 17 pre-existing `goconst` findings 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.
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}/password is wrapped with s.mw.PasswordChangeRateLimit() via r.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 shared postRateLimit helper. I diffed LoginRateLimit's behaviour before/after the refactor: POST-only gate, KeyByRealIP keying, 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_LimitsPOST drives the new limiter to the limit, asserts the next POST is 429 and never reaches the handler; the shared runPostLimitTest helper keeps the login test's assertions intact.
The rework commit (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)
Definition of done: authenticated + CSRF-protected form on the profile page; own-user enforced (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 via database.VerifyPassword; new password required non-empty and equal to confirmation; new hash via database.HashPassword (the same Argon2id helper that bootstraps the admin); clear success/error messages; rejected paths leave the stored hash untouched. All present.
CSRF wiring the handler tests bypass: renderProfile passes map data and renderTemplate injects CSRFToken into map data, so the template's {{.CSRFToken}} renders a real token and production posts pass the gorilla/csrf middleware.
Tests are meaningful: the happy path asserts the stored hash changed AND the new password verifies; the wrong-current-password path asserts the message AND the stored hash is byte-identical. The rate-limit test asserts both the 429 and that the handler was not invoked past the limit.
Error handling: internal failures go through serverError (generic 500, details only in logs). Error messages to the user are specific but leak nothing.
Gates: CI is green on head 8362ce9 (check / check, 2m40s). I independently ran script/cibuild (docker build .) on the checked-out head: fmt-check, lint, tests, and build all passed, exit 0. PR is mergeable; base 81413c5 is current main HEAD.
Hygiene: landing squash title ends with (closes #65); the rework commit references #65 without a duplicate closes, which is correct. No AI/tooling references or attribution trailers anywhere in commits, diff, or PR body. Naming is consistent with the surrounding middleware (PasswordChangeRateLimit mirrors LoginRateLimit; 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)
Rate-limit keying trusts client-suppliable headers. httprate.KeyByRealIP honours True-Client-IP, X-Real-IP, and the first X-Forwarded-For entry 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 on main (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 to RemoteAddr).
The handler's http.MaxBytesReader (1 MB) is installed after gorilla/csrf has already consumed the POST body (r.PostFormValue in 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 on main has the identical latent idiom, and every CSRF-protected group mounts MaxBodySize after CSRF anyway, so this is a pre-existing repo-wide ordering question, not a regression here. A follow-up could move body limiting ahead of CSRF.
Previously noted and still true, all within the DoD as written: no PRG redirect after POST, other active sessions are not invalidated on password change, and there is no minimum-length/strength rule beyond non-empty + confirmation match.
Verdict: PASS. The prior blocking finding is genuinely fixed and everything else holds up.
## 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}/password` is wrapped with `s.mw.PasswordChangeRateLimit()` via `r.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 shared `postRateLimit` helper. I diffed `LoginRateLimit`'s behaviour before/after the refactor: POST-only gate, `KeyByRealIP` keying, 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_LimitsPOST` drives the new limiter to the limit, asserts the next POST is 429 and never reaches the handler; the shared `runPostLimitTest` helper keeps the login test's assertions intact.
- The rework commit (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)
- Definition of done: authenticated + CSRF-protected form on the profile page; own-user enforced (`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 via `database.VerifyPassword`; new password required non-empty and equal to confirmation; new hash via `database.HashPassword` (the same Argon2id helper that bootstraps the admin); clear success/error messages; rejected paths leave the stored hash untouched. All present.
- CSRF wiring the handler tests bypass: `renderProfile` passes map data and `renderTemplate` injects `CSRFToken` into map data, so the template's `{{.CSRFToken}}` renders a real token and production posts pass the gorilla/csrf middleware.
- Tests are meaningful: the happy path asserts the stored hash changed AND the new password verifies; the wrong-current-password path asserts the message AND the stored hash is byte-identical. The rate-limit test asserts both the 429 and that the handler was not invoked past the limit.
- Error handling: internal failures go through `serverError` (generic 500, details only in logs). Error messages to the user are specific but leak nothing.
- Gates: CI is green on head 8362ce9 (`check / check`, 2m40s). I independently ran `script/cibuild` (`docker build .`) on the checked-out head: fmt-check, lint, tests, and build all passed, exit 0. PR is mergeable; base 81413c5 is current `main` HEAD.
- Hygiene: landing squash title ends with `(closes #65)`; the rework commit references `#65` without a duplicate `closes`, which is correct. No AI/tooling references or attribution trailers anywhere in commits, diff, or PR body. Naming is consistent with the surrounding middleware (`PasswordChangeRateLimit` mirrors `LoginRateLimit`; 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)
1. Rate-limit keying trusts client-suppliable headers. `httprate.KeyByRealIP` honours `True-Client-IP`, `X-Real-IP`, and the first `X-Forwarded-For` entry 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 on `main` (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 to `RemoteAddr`).
2. The handler's `http.MaxBytesReader` (1 MB) is installed after gorilla/csrf has already consumed the POST body (`r.PostFormValue` in 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 on `main` has the identical latent idiom, and every CSRF-protected group mounts `MaxBodySize` after `CSRF` anyway, so this is a pre-existing repo-wide ordering question, not a regression here. A follow-up could move body limiting ahead of CSRF.
3. Previously noted and still true, all within the DoD as written: no PRG redirect after POST, other active sessions are not invalidated on password change, and there is no minimum-length/strength rule beyond non-empty + confirmation match.
Verdict: PASS. The prior blocking finding is genuinely fixed and everything else holds up.
clawbot
removed their assignment 2026-08-07 19:12:07 +02:00
sneak
was assigned by clawbot2026-08-07 19:12:07 +02:00
Manager note: independent re-review above (comment 43912) verified the rate-limiting rework in 8362ce9 in code and passed the PR with no blocking findings; script/cibuild was independently run by the reviewer (exit 0) and CI is green on the head. Setting merge-ready and assigning to sneak for merge (protected main).
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.
Manager note: independent re-review above (comment 43912) verified the rate-limiting rework in 8362ce9 in code and passed the PR with no blocking findings; `script/cibuild` was independently run by the reviewer (exit 0) and CI is green on the head. Setting `merge-ready` and assigning to sneak for merge (protected `main`).
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.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
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.