diff --git a/CHANGELOG.md b/CHANGELOG.md index 0f1ce7cc..b3fe368d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,25 @@ exact tag (`ghcr.io/calnode/calnode:0.1.0`) if you need stability between upgrad ## [Unreleased] +### Added +- **A forgotten password can be reset by email.** The login page's "Forgot password?" link + pointed at a page that was never built, so it quietly bounced back to sign-in. It now + leads to a form that emails a reset link, and the link opens a page to choose a new + password. Closes [#34](https://github.com/Calnode/calnode/issues/34). + + The request answers the same way whether or not the address has an account, and does no + account-dependent work before answering: the lookup, the token and the email happen after + the response has gone. Only an active account that signs in with a password is mailed; + an SSO-only member still gets a password from an admin. The link is built from + `BASE_URL`, never from the request's `Host`, and carries its token in the URL fragment, + so it never reaches a server or proxy log. Only the token's SHA-256 is stored. A link + works once, for 30 minutes, and only the newest one requested; one account gets at most + one email a minute however many addresses ask. Setting the new password signs out every + existing session, as an admin reset does, and signs the user in. + + The link is shown only when email is configured and password sign-in is in use. New + migration `00058_password_reset_tokens`. + ### Fixed - **The Zoom setup text no longer promises that an unpublished app works for "your own team".** Zoom only lets users inside the Zoom account that owns an unpublished app diff --git a/audit/claims.yaml b/audit/claims.yaml index 79aea672..0516c39c 100644 --- a/audit/claims.yaml +++ b/audit/claims.yaml @@ -340,3 +340,36 @@ claims: than add admin approval — the friction of gating a personal calendar connection behind admin sign-off was judged worse than the bounded, already-authenticated- user SSRF exposure it would close. + + - id: password-reset-no-enumeration-no-host-poisoning + claim: > + Self-service password reset (POST /v1/auth/password-reset/request) cannot be used + to learn whether an address has an account, and the emailed link always points at + the configured BASE_URL, never at a host taken from the request. Only the SHA-256 + of a reset token is stored; a token works once, for 30 minutes, and only while the + account is active and signs in with a password. Completing a reset revokes every + session the account held. + verify: + - "internal/handler/password_reset.go's RequestPasswordReset: writes one fixed + response and hands the email to a goroutine before any query runs; the user + lookup, token write and send are all in sendPasswordReset." + - "internal/handler/password_reset.go's sendPasswordReset: the link is + h.baseURL + \"/admin/reset-password#token=\"; grep the file for r.Host or + X-Forwarded and expect nothing." + - "internal/handler/password_reset_test.go: TestPasswordReset_requestIsIndistinguishable + (same status, Content-Type and body for existing, unknown, archived, SSO-only and + email-off), TestPasswordReset_linkUsesBaseURLNotHostHeader (hostile Host, + X-Forwarded-Host, Forwarded, Origin and Referer), TestPasswordReset_storesOnlyTheHash + (scans every column of every table for the raw token), + TestPasswordReset_concurrentConfirmsSpendTheTokenOnce and + TestPasswordReset_revokesSessions." + status: verified + caveat: > + Whoever controls an account's mailbox can take the account over. That was already + true before this endpoint existed, because magic-link sign-in grants a session to + the same proof. Response timing is flat on the request path, but a reset email's + arrival is of course visible to the mailbox owner, and an operator who reads logs + sees an info line when a request is suppressed by the per-account cooldown. API + keys and connected MCP apps survive a reset, as they survive the two other + password-change paths (POST /v1/users/me/password and POST /v1/users/{id}/password); + revoking them on any password change would be a separate decision for all three. diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 58defaab..e915ad8d 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -95,7 +95,8 @@ partial unique index covers) — no TOCTOU between concurrent bookings. `archived_by`, plus prefs `time_format`/`week_start`/`date_format`, seven `notify_*` toggles, and auth columns `email_login`/`password_hash`/`provider`/ `provider_id`), `sessions` (cookie auth), `api_keys` (SHA-256 hashed), - `invite_tokens` (hashed, single-use). **There is no `auth_providers` table** — + `invite_tokens` (hashed, single-use), `magic_link_tokens` and + `password_reset_tokens` (SHA-256 hashed, single-use, short-lived). **There is no `auth_providers` table** — OAuth identity is columns on `users` (migration 00012). - **Teams:** `teams`, `team_members` (with `routing_priority`). - **Event types:** `event_types` (+ `routing_mode` — CHECK has **four** values: @@ -177,6 +178,42 @@ the platform/recovery secret doesn't expose secrets. `POST /v1/auth/magic-link/request` + `GET …/verify`, `magic_link.go`, migration 00040 — alongside password auth.) The actual user-creation path for non-first users is **invites** (`invite_tokens`), not OAuth self-registration. +- **Password reset by email** (`password_reset.go`, migration 00058, issue #34). Three + public routes on their own per-IP bucket (10/min, deliberately not `loginRL`, so someone + locked out of login by wrong passwords can still recover): + `POST /v1/auth/password-reset/request` `{email}` → email → SPA + `/admin/reset-password#token=…` → `POST …/check` `{token}` (non-consuming, returns the + account email for display and password managers) → `POST …/confirm` `{token, password}`. + The rules that matter: + - **Eligible = LoginEmail's own condition** (`email_login = 1` and a stored hash) **and not + archived**, one SQL predicate (`resetEligibleUser`) read by all three routes and + re-applied at the password write. An SSO-only account is never given a password by + email; an admin reset or `calnode reset-admin` does that. Like a session, a link is + judged against the account as it is when used. + - **The request is not an oracle.** It answers with one fixed status and body for every + well-formed request (unknown, archived, SSO-only, inside cooldown, email not + configured) and does no account-dependent work first: lookup, mint and send run in a + goroutine after the response, one step further than magic links, which look the user up + inline. Not a worker job: a job payload is stored, so the raw token could not ride in + it, and enqueueing for every typed address would persist attacker-chosen strings in a + `jobs` table that is never purged. + - **Token:** 32 random bytes, hex; only SHA-256 stored; **30 min** TTL (magic links are 15; + the extra time is for greylisting mail servers); single use via + `UPDATE … WHERE used_at IS NULL AND expires_at > ?` + RowsAffected, which is the + authority even though a cheap lookup runs first (so a bad token never costs a bcrypt). + A new request deletes the account's unused tokens. **Per-account cooldown of 1 min** + between emails, which the per-IP limit cannot give and which also stops a third party + from superseding the owner's newest link faster than it can be clicked. + - **The link is built from `BASE_URL`, never from the request** (host-header poisoning), + and the token rides in the **fragment**, unlike invites (path) and magic links (query), so + it never reaches a proxy access log or a Referer. The page strips it from the address bar. + - **Confirm** hashes before opening the transaction (never hold the single connection + across bcrypt), then in one transaction consumes the token, sets the hash, deletes + **every** session (as `AdminSetPassword` does) and the account's other reset tokens, + then signs the caller in (as invite claim and magic links do). API keys and MCP OAuth + grants survive, as they do for the other two password-change paths. + - Emails are English-only, like invites and magic links. The login page shows the link + only when email is configured **and** some account uses password login. - **CSRF:** cookie sessions are `SameSite=Lax` (blocks the classic cross-site write), plus a `SameOriginCheck` middleware (`internal/server`) that rejects a state-changing request whose Origin/Referer host ≠ the request Host — but only when the @@ -676,7 +713,8 @@ as the desired state: - `internal/worker`: polls the `jobs` table **every 5s** (batch ≤10). Job types: `webhook.deliver` and `reminder.send`. Also purges expired manage tokens + - sessions + magic-link tokens + idempotency keys (>24h old) + OAuth auth codes + + sessions + magic-link and password-reset tokens (expired or used) + idempotency keys + (>24h old) + OAuth auth codes + **finished webhook deliveries (>30d, `webhookDeliveryRetention`)** each cycle, and reaps jobs whose 30s lock expired (crash recovery — reset to pending +1 min). Retry **backoff is a fixed two-step: 60s then 5 min** diff --git a/frontend/src/routes/+layout.svelte b/frontend/src/routes/+layout.svelte index 7a34f787..f51c2cb6 100644 --- a/frontend/src/routes/+layout.svelte +++ b/frontend/src/routes/+layout.svelte @@ -27,7 +27,9 @@ const isPublicRoute = $derived( $page.route.id === '/login' || $page.route.id === '/claim' || - $page.route.id === '/invite/[token]' + $page.route.id === '/invite/[token]' || + $page.route.id === '/forgot-password' || + $page.route.id === '/reset-password' ); const navItems = [ diff --git a/frontend/src/routes/forgot-password/+page.svelte b/frontend/src/routes/forgot-password/+page.svelte new file mode 100644 index 00000000..5330545b --- /dev/null +++ b/frontend/src/routes/forgot-password/+page.svelte @@ -0,0 +1,118 @@ + + +Reset your password — Calnode + +
+
+
+
+ + + + +
+

Reset your password

+
+ + {#if !loaded} +

Loading…

+ {:else if !available} +
+ Password reset by email isn't available on this server. Ask a workspace admin to set a new + password for you from the Members page. If you run this server, + calnode reset-admin resets any account from the command line. +
+ {:else if sentMessage} +
+
{sentMessage}
+

+ The link works once and expires in 30 minutes. If nothing arrives, check your spam folder, + or try again in a minute. +

+
+ {:else} + {#if error} +
{error}
+ {/if} +
+

+ Enter the email you sign in with and we'll send you a link to choose a new password. +

+
+ + +
+ +
+ {/if} + +

+ Back to sign in +

+
+
diff --git a/frontend/src/routes/login/+page.svelte b/frontend/src/routes/login/+page.svelte index 6c961f3c..0128fa6e 100644 --- a/frontend/src/routes/login/+page.svelte +++ b/frontend/src/routes/login/+page.svelte @@ -103,7 +103,8 @@ const showGoogle = $derived(status?.providers?.includes('google') ?? false); const showMicrosoft = $derived(status?.providers?.includes('microsoft') ?? false); const showEmail = $derived(status?.email_login ?? false); - const showForgot = $derived(status?.smtp_configured ?? false); + // Reset needs a way to send the link and an account that signs in with a password. + const showForgot = $derived((status?.smtp_configured && status?.email_login) ?? false); const showMagic = $derived(status?.smtp_configured ?? false); const showDivider = $derived((showGoogle || showMicrosoft) && showEmail); diff --git a/frontend/src/routes/reset-password/+page.svelte b/frontend/src/routes/reset-password/+page.svelte new file mode 100644 index 00000000..35a472b6 --- /dev/null +++ b/frontend/src/routes/reset-password/+page.svelte @@ -0,0 +1,167 @@ + + +Choose a new password — Calnode + +
+
+
+
+ + + + +
+

Choose a new password

+
+ + {#if phase === 'checking'} +

Checking your link…

+ {:else if phase === 'invalid'} +
+ This reset link is invalid or has expired. Links work once, for 30 minutes, and only the + newest one you requested. +
+

+ Request a new link +

+ {:else} + {#if checkError} +
+

{checkError} You can still set your password below, or check the link again.

+ +
+ {/if} + {#if error} +
{error}
+ {/if} +
+ {#if accountEmail} +
+ + + +
+ {/if} +
+ + +

8 to 72 characters

+
+
+ + +
+ +

+ This signs you out everywhere else and signs you in here. +

+
+ {/if} +
+
diff --git a/internal/db/migrations/00058_password_reset_tokens.sql b/internal/db/migrations/00058_password_reset_tokens.sql new file mode 100644 index 00000000..b2d92a9d --- /dev/null +++ b/internal/db/migrations/00058_password_reset_tokens.sql @@ -0,0 +1,23 @@ +-- +goose Up +-- Self-service password reset links (issue #34). Same shape as magic_link_tokens +-- (00040): only the SHA-256 of the token is stored, never the raw value, so a copy of +-- the database (or of its Litestream replica) cannot be turned into a working link. +-- Single use is enforced by a conditional UPDATE on used_at, not by this schema. +-- +-- created_at is written by the application in RFC 3339 rather than defaulted by +-- SQLite, because it is compared against RFC 3339 cutoffs: the per-account cooldown +-- between reset emails reads it, and datetime('now') produces a different text format +-- that would not order correctly against them. +CREATE TABLE password_reset_tokens ( + token_hash TEXT PRIMARY KEY, + user_id TEXT NOT NULL REFERENCES users(id) ON DELETE CASCADE, + expires_at TEXT NOT NULL, + used_at TEXT, + created_at TEXT NOT NULL +); + +CREATE INDEX idx_password_reset_tokens_user ON password_reset_tokens(user_id, created_at); + +-- +goose Down +DROP INDEX IF EXISTS idx_password_reset_tokens_user; +DROP TABLE IF EXISTS password_reset_tokens; diff --git a/internal/handler/export_test.go b/internal/handler/export_test.go new file mode 100644 index 00000000..67241ea0 --- /dev/null +++ b/internal/handler/export_test.go @@ -0,0 +1,10 @@ +package handler + +// WaitPasswordResetSends blocks until every password-reset send started so far has +// finished. RequestPasswordReset answers before it knows whether the account exists, so +// a test asserting that nothing was minted for an unknown or ineligible address has to +// wait for the background work to end rather than poll for a row that should never +// appear. +func (h *Handler) WaitPasswordResetSends() { + h.resetSends.Wait() +} diff --git a/internal/handler/handler.go b/internal/handler/handler.go index 6adbb088..59f2b882 100644 --- a/internal/handler/handler.go +++ b/internal/handler/handler.go @@ -50,6 +50,7 @@ type Handler struct { demoResetInterval time.Duration demoMu sync.RWMutex demoNextResetAt time.Time + resetSends sync.WaitGroup // password-reset emails running after their request was answered } // SetLiveKit swaps the active LiveKit client (nil disables built-in video rooms). diff --git a/internal/handler/password_reset.go b/internal/handler/password_reset.go new file mode 100644 index 00000000..26f90027 --- /dev/null +++ b/internal/handler/password_reset.go @@ -0,0 +1,359 @@ +package handler + +import ( + "context" + "crypto/rand" + "crypto/sha256" + "database/sql" + "encoding/hex" + "encoding/json" + "errors" + "fmt" + "html" + "net/http" + "strings" + "time" + + "golang.org/x/crypto/bcrypt" + + "github.com/calnode/calnode/internal/mailer" +) + +// passwordResetTTL is how long an emailed reset link stays usable. It is longer than the +// magic link's 15 minutes only to survive slow delivery: greylisting mail servers commonly +// hold a first message from a new sender for 5 to 15 minutes, and a link that is dead on +// arrival sends the user straight back to the form. It stays at the short end of the usual +// range otherwise, because a reset link is exactly as powerful as a login link. +const passwordResetTTL = 30 * time.Minute + +// passwordResetCooldown is the minimum gap between two reset emails for one account. +// The per-IP limit on the route cannot provide this, because a sender with many +// addresses gets many buckets. It matters for more than inbox flooding: every new +// request invalidates the account's outstanding link, so without a cooldown anyone who +// knows the address could keep the owner's links dying faster than they can click them. +// With it, the newest link always survives at least this long. +const passwordResetCooldown = time.Minute + +// passwordResetAccepted is the one response the request endpoint ever gives to a +// well-formed body. It must not vary with whether the account exists, is archived, signs +// in with SSO only, is inside its cooldown, or whether email is configured at all. +const passwordResetAccepted = "If that email belongs to an account that signs in with a password, a reset link is on its way." + +// passwordResetInvalid is the single answer for every token that cannot be used: unknown, +// expired, already used, superseded by a newer request, or belonging to an account that +// has since been archived or no longer signs in with a password. +const passwordResetInvalid = "This reset link is invalid or has expired. Request a new one." + +// resetEligibleUser is the SQL predicate, over users aliased u, for an account that may +// reset its password by email. It is LoginEmail's own condition for a password sign-in to +// be possible (email_login set and a hash present) plus not archived, so the reset can +// never hand a password to an account that could not have used one: an SSO-only member +// gets a password from an admin, not from their inbox. Request, check and confirm all +// read this one definition so the three cannot drift apart. +const resetEligibleUser = `u.archived_at IS NULL AND u.email_login = 1 AND COALESCE(u.password_hash, '') != ''` + +func hashPasswordResetToken(raw string) string { + sum := sha256.Sum256([]byte(raw)) + return hex.EncodeToString(sum[:]) +} + +// RequestPasswordReset handles POST /v1/auth/password-reset/request (public). It answers +// immediately with the same status and body for every well-formed request, and does no +// work that depends on the account before answering: the user lookup, the cooldown check, +// the token write and the email all run in the background. Magic links (magic_link.go) +// look the user up inline and move only the send off the request; this goes one step +// further so there is not even a found-versus-missing query to time. +// +// It is a goroutine rather than a worker job for two reasons. A job's payload is stored, +// so the raw token cannot travel in it, and minting inside the job would mean enqueueing +// for every address typed, known or not, which writes attacker-chosen strings into a +// jobs table that nothing purges (and that Litestream, where configured, replicates +// offsite). Enqueueing only for known accounts would put an account-dependent write back +// on the request path. +func (h *Handler) RequestPasswordReset(w http.ResponseWriter, r *http.Request) { + r.Body = http.MaxBytesReader(w, r.Body, 4<<10) + var req struct { + Email string `json:"email"` + } + if err := json.NewDecoder(r.Body).Decode(&req); err != nil { + h.writeError(w, http.StatusBadRequest, "invalid JSON") + return + } + email := strings.TrimSpace(strings.ToLower(req.Email)) + // isEmailEnabled is instance configuration, not a property of the account, so + // branching on it here tells a caller nothing they could not read from + // GET /v1/auth/status. + if email != "" && h.isEmailEnabled() { + ctx := context.WithoutCancel(r.Context()) + h.resetSends.Go(func() { h.sendPasswordReset(ctx, email) }) + } + h.writeJSON(w, http.StatusOK, map[string]any{ + "ok": true, + "message": passwordResetAccepted, + }) +} + +// sendPasswordReset mints a reset token for email's account and mails the link, when the +// account is eligible and outside its cooldown. Every other outcome is silent by design: +// the requester has already been answered, and logging an unknown address would copy +// attacker-supplied strings into the logs. +func (h *Handler) sendPasswordReset(ctx context.Context, email string) { + var userID string + err := h.db.QueryRowContext(ctx, + `SELECT u.id FROM users u WHERE u.email = ? AND `+resetEligibleUser, email).Scan(&userID) + if errors.Is(err, sql.ErrNoRows) { + return + } + if err != nil { + h.logger.ErrorContext(ctx, "password reset: look up user", "error", err) + return + } + + rawBytes := make([]byte, 32) + if _, err := rand.Read(rawBytes); err != nil { + h.logger.ErrorContext(ctx, "password reset: rand", "error", err) + return + } + raw := hex.EncodeToString(rawBytes) + now := time.Now().UTC() + + // Cooldown check, invalidation and insert in one transaction. With the single SQLite + // connection that serialises two concurrent requests for the same account, so both + // cannot pass the cooldown and mint. + tx, err := h.db.BeginTx(ctx, nil) + if err != nil { + h.logger.ErrorContext(ctx, "password reset: begin tx", "error", err) + return + } + defer tx.Rollback() //nolint:errcheck + + var recent int + if err := tx.QueryRowContext(ctx, + `SELECT COUNT(*) FROM password_reset_tokens WHERE user_id = ? AND created_at > ?`, + userID, now.Add(-passwordResetCooldown).Format(time.RFC3339)).Scan(&recent); err != nil { + h.logger.ErrorContext(ctx, "password reset: cooldown check", "error", err) + return + } + if recent > 0 { + h.logger.InfoContext(ctx, "password reset: skipped, a link was sent within the cooldown", + "user_id", userID, "cooldown", passwordResetCooldown.String()) + return + } + // Only one live link per account: a new request supersedes the old one, the same + // rule issueInvite applies to invites. + if _, err := tx.ExecContext(ctx, + `DELETE FROM password_reset_tokens WHERE user_id = ? AND used_at IS NULL`, userID); err != nil { + h.logger.ErrorContext(ctx, "password reset: invalidate older links", "error", err) + return + } + if _, err := tx.ExecContext(ctx, + `INSERT INTO password_reset_tokens (token_hash, user_id, expires_at, created_at) VALUES (?, ?, ?, ?)`, + hashPasswordResetToken(raw), userID, + now.Add(passwordResetTTL).Format(time.RFC3339), now.Format(time.RFC3339)); err != nil { + h.logger.ErrorContext(ctx, "password reset: store token", "error", err) + return + } + if err := tx.Commit(); err != nil { + h.logger.ErrorContext(ctx, "password reset: commit", "error", err) + return + } + + // The link is built from the configured BASE_URL and never from anything in the + // request. A reset link assembled from the Host header lets whoever sends the request + // choose the domain the victim's token is delivered to. + // + // The token travels in the fragment, unlike invites (path) and magic links (query). + // A fragment is never sent to the server, so it stays out of reverse-proxy access + // logs, and it is never included in a Referer, including the one on the admin app's + // font request. The reset page reads it client-side and strips it from the address bar. + link := h.baseURL + "/admin/reset-password#token=" + raw + if err := h.mailer.Send(ctx, passwordResetMessage(email, link)); err != nil { + h.logger.ErrorContext(ctx, "password reset: send email", "error", err, "user_id", userID) + } +} + +// passwordResetTokenRequest is the body of the check and confirm endpoints. The token is +// posted rather than put in a URL for the same reason the email link carries it in a +// fragment: nothing that logs request lines ever sees it. +type passwordResetTokenRequest struct { + Token string `json:"token"` + Password string `json:"password"` +} + +// CheckPasswordReset handles POST /v1/auth/password-reset/check (public). It reports +// whether a token can still be used, without consuming it, and returns the account's +// email so the reset page can say whose password is being changed and give password +// managers a username to save the new password against. The email reveals nothing the +// token holder cannot already take: the token alone is enough to set the password. +func (h *Handler) CheckPasswordReset(w http.ResponseWriter, r *http.Request) { + r.Body = http.MaxBytesReader(w, r.Body, 4<<10) + var req passwordResetTokenRequest + if err := json.NewDecoder(r.Body).Decode(&req); err != nil { + h.writeError(w, http.StatusBadRequest, "invalid JSON") + return + } + email, _, err := h.lookupPasswordResetToken(r.Context(), strings.TrimSpace(req.Token)) + if err != nil { + if !errors.Is(err, sql.ErrNoRows) { + h.logger.ErrorContext(r.Context(), "password reset check: lookup", "error", err) + } + h.writeError(w, http.StatusNotFound, passwordResetInvalid) + return + } + h.writeJSON(w, http.StatusOK, map[string]any{"email": email}) +} + +// lookupPasswordResetToken returns the email and user id behind a usable token, or +// sql.ErrNoRows when there is none (unknown, expired, used, or the account no longer +// qualifies). +func (h *Handler) lookupPasswordResetToken(ctx context.Context, raw string) (email, userID string, err error) { + if raw == "" { + return "", "", sql.ErrNoRows + } + err = h.db.QueryRowContext(ctx, ` + SELECT u.email, u.id FROM password_reset_tokens t + JOIN users u ON u.id = t.user_id + WHERE t.token_hash = ? AND t.used_at IS NULL AND t.expires_at > ? AND `+resetEligibleUser, + hashPasswordResetToken(raw), time.Now().UTC().Format(time.RFC3339)).Scan(&email, &userID) + return email, userID, err +} + +// ConfirmPasswordReset handles POST /v1/auth/password-reset/confirm (public). It consumes +// the token, sets the new password, revokes every session the account holds, and then +// signs the caller in, as claiming an invite and following a magic link both do. +// +// Order matters in three places: +// - The password policy is checked before anything else, so a rejected password does +// not spend the link. +// - bcrypt runs before the transaction. At DefaultCost it takes long enough that +// holding the single SQLite connection across it would stall every other request. +// A cheap non-consuming lookup comes first so an unusable token never costs a hash. +// - The token is consumed by a conditional UPDATE inside the transaction, and that +// UPDATE is the authority: two concurrent confirms can both pass the lookup, and +// exactly one of them will see a row affected. +func (h *Handler) ConfirmPasswordReset(w http.ResponseWriter, r *http.Request) { + r.Body = http.MaxBytesReader(w, r.Body, 4<<10) + var req passwordResetTokenRequest + if err := json.NewDecoder(r.Body).Decode(&req); err != nil { + h.writeError(w, http.StatusBadRequest, "invalid JSON") + return + } + token := strings.TrimSpace(req.Token) + if msg := validatePassword(req.Password); msg != "" { + h.writeError(w, http.StatusBadRequest, msg) + return + } + if _, _, err := h.lookupPasswordResetToken(r.Context(), token); err != nil { + if !errors.Is(err, sql.ErrNoRows) { + h.logger.ErrorContext(r.Context(), "password reset confirm: lookup", "error", err) + } + h.writeError(w, http.StatusNotFound, passwordResetInvalid) + return + } + + newHash, err := bcrypt.GenerateFromPassword([]byte(req.Password), bcrypt.DefaultCost) + if err != nil { + h.logger.ErrorContext(r.Context(), "password reset confirm: bcrypt", "error", err) + h.writeError(w, http.StatusInternalServerError, "internal error") + return + } + + tokenHash := hashPasswordResetToken(token) + now := time.Now().UTC().Format(time.RFC3339) + + tx, err := h.db.BeginTx(r.Context(), nil) + if err != nil { + h.logger.ErrorContext(r.Context(), "password reset confirm: begin tx", "error", err) + h.writeError(w, http.StatusInternalServerError, "internal error") + return + } + defer tx.Rollback() //nolint:errcheck + + res, err := tx.ExecContext(r.Context(), + `UPDATE password_reset_tokens SET used_at = ? WHERE token_hash = ? AND used_at IS NULL AND expires_at > ?`, + now, tokenHash, now) + if err != nil { + h.logger.ErrorContext(r.Context(), "password reset confirm: consume token", "error", err) + h.writeError(w, http.StatusInternalServerError, "internal error") + return + } + if n, _ := res.RowsAffected(); n == 0 { + h.writeError(w, http.StatusNotFound, passwordResetInvalid) // lost a race, or expired since the lookup + return + } + + var userID string + if err := tx.QueryRowContext(r.Context(), + `SELECT user_id FROM password_reset_tokens WHERE token_hash = ?`, tokenHash).Scan(&userID); err != nil { + h.logger.ErrorContext(r.Context(), "password reset confirm: load user", "error", err) + h.writeError(w, http.StatusInternalServerError, "internal error") + return + } + + // The eligibility predicate is re-applied at the write, not only at the lookup above, + // so an account archived (or moved off password sign-in) during the bcrypt cannot slip + // through. Refusing rolls the consume back: like a session, a link is judged against + // the account as it is when used, which is how archiving works everywhere else. + res, err = tx.ExecContext(r.Context(), + `UPDATE users AS u SET password_hash = ? WHERE u.id = ? AND `+resetEligibleUser, + string(newHash), userID) + if err != nil { + h.logger.ErrorContext(r.Context(), "password reset confirm: update password", "error", err) + h.writeError(w, http.StatusInternalServerError, "internal error") + return + } + if n, _ := res.RowsAffected(); n == 0 { + h.writeError(w, http.StatusNotFound, passwordResetInvalid) + return + } + + // Revoke every session, as AdminSetPassword does. A reset is what someone does when + // they believe their password is known to someone else, and a stolen session cookie + // must not outlive that. Any other outstanding link for the account dies too. + if _, err := tx.ExecContext(r.Context(), `DELETE FROM sessions WHERE user_id = ?`, userID); err != nil { + h.logger.ErrorContext(r.Context(), "password reset confirm: revoke sessions", "error", err) + h.writeError(w, http.StatusInternalServerError, "internal error") + return + } + if _, err := tx.ExecContext(r.Context(), + `DELETE FROM password_reset_tokens WHERE user_id = ? AND token_hash != ?`, userID, tokenHash); err != nil { + h.logger.ErrorContext(r.Context(), "password reset confirm: invalidate other links", "error", err) + h.writeError(w, http.StatusInternalServerError, "internal error") + return + } + if err := tx.Commit(); err != nil { + h.logger.ErrorContext(r.Context(), "password reset confirm: commit", "error", err) + h.writeError(w, http.StatusInternalServerError, "internal error") + return + } + + if err := h.createSession(r.Context(), w, userID); err != nil { + // The password is already changed; the user can sign in with it normally. + h.logger.ErrorContext(r.Context(), "password reset confirm: create session", "error", err) + h.writeJSON(w, http.StatusOK, map[string]any{"ok": true, "signed_in": false}) + return + } + h.writeJSON(w, http.StatusOK, map[string]any{"ok": true, "signed_in": true}) +} + +// passwordResetMessage builds the reset email (plain text plus minimal HTML), in the same +// form as magicLinkMessage. English only, like every other message addressed to a member +// rather than a booker: the admin app is not translated (ARCHITECTURE §23). +func passwordResetMessage(to, link string) mailer.Message { + minutes := int(passwordResetTTL.Minutes()) + escaped := html.EscapeString(link) + return mailer.Message{ + To: []string{to}, + Subject: "Reset your Calnode password", + Text: fmt.Sprintf("Someone asked to reset the password for the Calnode account %s. "+ + "Click the link below to choose a new one. It expires in %d minutes and can be used once.\n\n"+ + "%s\n\nIf you didn't ask for this, you can ignore this email. Your password has not been changed.", + to, minutes, link), + HTML: fmt.Sprintf(`
`+ + `

Someone asked to reset the password for the Calnode account %s. Click the button below to choose a new one. It expires in %d minutes and can be used once.

`+ + `

Reset password

`+ + `

Or paste this link into your browser:
%s

`+ + `

If you didn't ask for this, you can ignore this email. Your password has not been changed.

`, + html.EscapeString(to), minutes, escaped, escaped, escaped), + } +} diff --git a/internal/handler/password_reset_test.go b/internal/handler/password_reset_test.go new file mode 100644 index 00000000..bc783b64 --- /dev/null +++ b/internal/handler/password_reset_test.go @@ -0,0 +1,615 @@ +package handler_test + +import ( + "context" + "database/sql" + "fmt" + "net/http" + "net/http/httptest" + "regexp" + "strings" + "sync" + "testing" + "time" + + "golang.org/x/crypto/bcrypt" + + "github.com/calnode/calnode/internal/handler" + "github.com/calnode/calnode/internal/mailer" +) + +// resetBaseURL is the configured BASE_URL in these tests. Every assertion about the link +// is made against it, so a link built from anything else fails. +const resetBaseURL = "https://calnode.example" + +// resetMailer records every message sent. It is not a *mailer.Noop, so the handler treats +// email as configured. +type resetMailer struct { + mu sync.Mutex + sent []mailer.Message +} + +func (m *resetMailer) Send(_ context.Context, msg mailer.Message) error { + m.mu.Lock() + defer m.mu.Unlock() + m.sent = append(m.sent, msg) + return nil +} + +func (m *resetMailer) messages() []mailer.Message { + m.mu.Lock() + defer m.mu.Unlock() + return append([]mailer.Message(nil), m.sent...) +} + +func newResetHandler(t *testing.T) (*handler.Handler, *sql.DB, *resetMailer) { + t.Helper() + h, database := newTestHandlerDB(t) + m := &resetMailer{} + h.SetMailer(m, resetBaseURL) + return h, database, m +} + +// seedResetUsers inserts one account of every kind the request endpoint must treat +// identically from the outside. Only active@ is eligible for a reset. +func seedResetUsers(t *testing.T, database *sql.DB) { + t.Helper() + hash, err := bcrypt.GenerateFromPassword([]byte("oldpassword"), bcrypt.MinCost) + if err != nil { + t.Fatalf("bcrypt: %v", err) + } + for _, q := range []struct { + sql string + args []any + }{ + {`INSERT INTO users (id,email,name,iana_timezone,is_admin,email_login,password_hash) VALUES ('u-active','active@example.com','Active','UTC',0,1,?)`, []any{string(hash)}}, + {`INSERT INTO users (id,email,name,iana_timezone,is_admin,email_login,password_hash,archived_at) VALUES ('u-archived','archived@example.com','Archived','UTC',0,1,?,'2026-01-01T00:00:00Z')`, []any{string(hash)}}, + {`INSERT INTO users (id,email,name,iana_timezone,is_admin,email_login,provider,provider_id) VALUES ('u-sso','sso@example.com','SSO','UTC',0,0,'google','g-1')`, nil}, + {`INSERT INTO users (id,email,name,iana_timezone,is_admin,email_login,password_hash) VALUES ('u-bystander','bystander@example.com','Bystander','UTC',0,1,?)`, []any{string(hash)}}, + } { + if _, err := database.Exec(q.sql, q.args...); err != nil { + t.Fatalf("seed user: %v", err) + } + } +} + +func postReset(fn http.HandlerFunc, path, body string, mutate ...func(*http.Request)) *httptest.ResponseRecorder { + req := httptest.NewRequest(http.MethodPost, path, strings.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + for _, m := range mutate { + m(req) + } + rec := httptest.NewRecorder() + fn(rec, req) + return rec +} + +// requestReset posts to the request endpoint and waits for the background send, so the +// caller can assert on what was (or was not) minted and mailed. +func requestReset(h *handler.Handler, email string, mutate ...func(*http.Request)) *httptest.ResponseRecorder { + rec := postReset(h.RequestPasswordReset, "/v1/auth/password-reset/request", `{"email":"`+email+`"}`, mutate...) + h.WaitPasswordResetSends() + return rec +} + +func checkReset(h *handler.Handler, token string) *httptest.ResponseRecorder { + return postReset(h.CheckPasswordReset, "/v1/auth/password-reset/check", `{"token":"`+token+`"}`) +} + +func confirmReset(h *handler.Handler, token, password string) *httptest.ResponseRecorder { + return postReset(h.ConfirmPasswordReset, "/v1/auth/password-reset/confirm", + `{"token":"`+token+`","password":"`+password+`"}`) +} + +var resetLinkRE = regexp.MustCompile(`(\S+)/admin/reset-password#token=([0-9a-f]+)`) + +// lastResetToken returns the base and raw token of the newest reset link mailed to to. +func lastResetToken(t *testing.T, m *resetMailer, to string) (base, token string) { + t.Helper() + msgs := m.messages() + for i := len(msgs) - 1; i >= 0; i-- { + if len(msgs[i].To) == 1 && msgs[i].To[0] == to { + match := resetLinkRE.FindStringSubmatch(msgs[i].Text) + if match == nil { + t.Fatalf("reset email to %s has no reset link in its text body:\n%s", to, msgs[i].Text) + } + return match[1], match[2] + } + } + t.Fatalf("no reset email was sent to %s (sent: %d messages)", to, len(msgs)) + return "", "" +} + +func canLogIn(h *handler.Handler, email, password string) bool { + rec := httptest.NewRecorder() + h.LoginEmail(rec, loginEmailReq(email, password)) + return rec.Code == http.StatusOK +} + +func countRows(t *testing.T, database *sql.DB, query string, args ...any) int { + t.Helper() + var n int + if err := database.QueryRow(query, args...).Scan(&n); err != nil { + t.Fatalf("count %q: %v", query, err) + } + return n +} + +// TestPasswordReset_requestIsIndistinguishable pins the enumeration guarantee: an existing +// account, an unknown address, an archived account, an SSO-only account and an instance +// with email switched off all get the same status, headers and body. Only the eligible +// account is mailed or gets a token row. +func TestPasswordReset_requestIsIndistinguishable(t *testing.T) { + type outcome struct { + code int + contentType string + body string + } + cases := []struct { + name string + email string + smtpOff bool + wantMail bool + }{ + {name: "existing password account", email: "active@example.com", wantMail: true}, + {name: "unknown address", email: "nobody@example.com"}, + {name: "archived account", email: "archived@example.com"}, + {name: "SSO-only account", email: "sso@example.com"}, + {name: "email not configured", email: "active@example.com", smtpOff: true}, + } + + var first *outcome + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + var h *handler.Handler + var database *sql.DB + m := &resetMailer{} + if c.smtpOff { + h, database = newTestHandlerDB(t) // Noop mailer: email is not configured + h.SetBaseURL(resetBaseURL) + } else { + h, database, m = newResetHandler(t) + } + seedResetUsers(t, database) + + rec := requestReset(h, c.email) + got := outcome{rec.Code, rec.Header().Get("Content-Type"), rec.Body.String()} + if got.code != http.StatusOK { + t.Fatalf("status = %d; want 200: %s", got.code, got.body) + } + if first == nil { + first = &got + } else if got != *first { + t.Errorf("response differs from the existing-account case:\n got %+v\n want %+v", got, *first) + } + + tokens := countRows(t, database, `SELECT COUNT(*) FROM password_reset_tokens`) + sent := len(m.messages()) + if c.wantMail { + if tokens != 1 || sent != 1 { + t.Fatalf("tokens = %d, emails = %d; want 1 and 1", tokens, sent) + } + if to := m.messages()[0].To; len(to) != 1 || to[0] != c.email { + t.Errorf("email went to %v; want [%s]", to, c.email) + } + } else if tokens != 0 || sent != 0 { + t.Errorf("tokens = %d, emails = %d; want 0 and 0", tokens, sent) + } + }) + } +} + +// TestPasswordReset_requestNormalisesEmail mirrors LoginEmail, which lower-cases and trims. +func TestPasswordReset_requestNormalisesEmail(t *testing.T) { + h, database, m := newResetHandler(t) + seedResetUsers(t, database) + requestReset(h, " ACTIVE@Example.com ") + lastResetToken(t, m, "active@example.com") +} + +// TestPasswordReset_storesOnlyTheHash scans every column of every table for the raw +// token after a request. The scanner is proven able to find things by locating the hash +// the same way, so a scanner that silently reads nothing cannot pass this test. +func TestPasswordReset_storesOnlyTheHash(t *testing.T) { + h, database, m := newResetHandler(t) + seedResetUsers(t, database) + requestReset(h, "active@example.com") + _, raw := lastResetToken(t, m, "active@example.com") + + if len(raw) < 64 { + t.Errorf("token is %d hex characters; want at least 64 (32 random bytes)", len(raw)) + } + if hits := columnsContaining(t, database, raw); len(hits) != 0 { + t.Errorf("raw reset token found in the database at %v; only its hash may be stored", hits) + } + hash := sha256HexForTest(raw) + if hits := columnsContaining(t, database, hash); len(hits) != 1 || hits[0] != "password_reset_tokens.token_hash" { + t.Errorf("token hash found at %v; want exactly password_reset_tokens.token_hash", hits) + } +} + +// columnsContaining returns table.column for every stored text value containing needle, +// across every table in the database. Each table's cursor is drained and closed before +// the next query, per the single-connection rule. +func columnsContaining(t *testing.T, database *sql.DB, needle string) []string { + t.Helper() + rows, err := database.Query(`SELECT name FROM sqlite_master WHERE type = 'table'`) + if err != nil { + t.Fatalf("list tables: %v", err) + } + var tables []string + for rows.Next() { + var name string + if err := rows.Scan(&name); err != nil { + t.Fatalf("scan table name: %v", err) + } + tables = append(tables, name) + } + rows.Close() + + var hits []string + for _, table := range tables { + rows, err := database.Query(fmt.Sprintf(`SELECT * FROM %q`, table)) + if err != nil { + t.Fatalf("read %s: %v", table, err) + } + cols, _ := rows.Columns() + for rows.Next() { + vals := make([]any, len(cols)) + ptrs := make([]any, len(cols)) + for i := range vals { + ptrs[i] = &vals[i] + } + if err := rows.Scan(ptrs...); err != nil { + t.Fatalf("scan %s: %v", table, err) + } + for i, v := range vals { + var s string + switch x := v.(type) { + case string: + s = x + case []byte: + s = string(x) + } + if s != "" && strings.Contains(s, needle) { + hits = append(hits, table+"."+cols[i]) + } + } + } + rows.Close() + } + return hits +} + +// TestPasswordReset_linkUsesBaseURLNotHostHeader is the host-header poisoning guard. The +// request arrives with every header an attacker could use to name a host, and the mailed +// link must still point at the configured BASE_URL. +func TestPasswordReset_linkUsesBaseURLNotHostHeader(t *testing.T) { + h, database, m := newResetHandler(t) + seedResetUsers(t, database) + + requestReset(h, "active@example.com", func(r *http.Request) { + r.Host = "evil.example" + r.Header.Set("X-Forwarded-Host", "evil.example") + r.Header.Set("X-Forwarded-Proto", "http") + r.Header.Set("Forwarded", "host=evil.example;proto=http") + r.Header.Set("Origin", "https://evil.example") + r.Header.Set("Referer", "https://evil.example/admin/forgot-password") + }) + + base, token := lastResetToken(t, m, "active@example.com") + if base != resetBaseURL { + t.Errorf("reset link base = %q; want the configured BASE_URL %q", base, resetBaseURL) + } + msg := m.messages()[0] + for part, body := range map[string]string{"text": msg.Text, "html": msg.HTML} { + if strings.Contains(body, "evil.example") { + t.Errorf("%s body names the request's host; the link must come from BASE_URL:\n%s", part, body) + } + want := resetBaseURL + "/admin/reset-password#token=" + token + if !strings.Contains(body, want) { + t.Errorf("%s body lacks %q", part, want) + } + } + // The token rides in the fragment, never a query string or path the server would log. + if strings.Contains(msg.Text, "?token=") || strings.Contains(msg.Text, "reset-password/"+token) { + t.Errorf("token must be carried in the URL fragment:\n%s", msg.Text) + } +} + +// TestPasswordReset_confirmSetsPasswordAndIsSingleUse walks the whole flow and then +// replays the same token. +func TestPasswordReset_confirmSetsPasswordAndIsSingleUse(t *testing.T) { + h, database, m := newResetHandler(t) + seedResetUsers(t, database) + requestReset(h, "active@example.com") + _, token := lastResetToken(t, m, "active@example.com") + + check := checkReset(h, token) + if check.Code != http.StatusOK || !strings.Contains(check.Body.String(), `"email":"active@example.com"`) { + t.Fatalf("check before use: %d %s; want 200 with the account email", check.Code, check.Body.String()) + } + + rec := confirmReset(h, token, "brandnewpass1") + if rec.Code != http.StatusOK { + t.Fatalf("first confirm: %d %s; want 200", rec.Code, rec.Body.String()) + } + if !canLogIn(h, "active@example.com", "brandnewpass1") { + t.Error("new password does not work after reset") + } + if canLogIn(h, "active@example.com", "oldpassword") { + t.Error("old password still works after reset") + } + + replay := confirmReset(h, token, "attackerpass1") + if replay.Code != http.StatusNotFound { + t.Errorf("second confirm with the same token: %d %s; want 404", replay.Code, replay.Body.String()) + } + if canLogIn(h, "active@example.com", "attackerpass1") { + t.Fatal("a replayed token changed the password again") + } + if !canLogIn(h, "active@example.com", "brandnewpass1") { + t.Error("the password set by the first confirm was lost") + } + if c := checkReset(h, token); c.Code != http.StatusNotFound { + t.Errorf("check after use: %d; want 404", c.Code) + } +} + +// TestPasswordReset_concurrentConfirmsSpendTheTokenOnce pins that the conditional UPDATE, +// not the lookup in front of it, is what makes a token single-use. Every goroutine passes +// the lookup before any of them finishes hashing, so only the UPDATE can stop the rest. +func TestPasswordReset_concurrentConfirmsSpendTheTokenOnce(t *testing.T) { + h, database, m := newResetHandler(t) + seedResetUsers(t, database) + requestReset(h, "active@example.com") + _, token := lastResetToken(t, m, "active@example.com") + + const n = 8 + start := make(chan struct{}) + codes := make([]int, n) + var wg sync.WaitGroup + for i := range n { + wg.Go(func() { + <-start + codes[i] = confirmReset(h, token, fmt.Sprintf("concurrent-%d", i)).Code + }) + } + close(start) + wg.Wait() + + ok := 0 + for _, c := range codes { + switch c { + case http.StatusOK: + ok++ + case http.StatusNotFound: + default: + t.Errorf("unexpected status %d", c) + } + } + if ok != 1 { + t.Errorf("%d of %d concurrent confirms succeeded with one token; want exactly 1 (codes %v)", ok, n, codes) + } +} + +func TestPasswordReset_expiredTokenIsRejected(t *testing.T) { + h, database, m := newResetHandler(t) + seedResetUsers(t, database) + requestReset(h, "active@example.com") + _, token := lastResetToken(t, m, "active@example.com") + + past := time.Now().UTC().Add(-time.Second).Format(time.RFC3339) + if _, err := database.Exec(`UPDATE password_reset_tokens SET expires_at = ?`, past); err != nil { + t.Fatalf("expire token: %v", err) + } + if c := checkReset(h, token); c.Code != http.StatusNotFound { + t.Errorf("check with expired token: %d; want 404", c.Code) + } + if c := confirmReset(h, token, "brandnewpass1"); c.Code != http.StatusNotFound { + t.Errorf("confirm with expired token: %d %s; want 404", c.Code, c.Body.String()) + } + if !canLogIn(h, "active@example.com", "oldpassword") { + t.Error("an expired token changed the password") + } +} + +func TestPasswordReset_tokenLivesThirtyMinutes(t *testing.T) { + h, database, _ := newResetHandler(t) + seedResetUsers(t, database) + before := time.Now().UTC() + requestReset(h, "active@example.com") + + var expiresAt string + if err := database.QueryRow(`SELECT expires_at FROM password_reset_tokens`).Scan(&expiresAt); err != nil { + t.Fatalf("read expiry: %v", err) + } + exp, err := time.Parse(time.RFC3339, expiresAt) + if err != nil { + t.Fatalf("parse expiry %q: %v", expiresAt, err) + } + if ttl := exp.Sub(before); ttl < 29*time.Minute || ttl > 31*time.Minute { + t.Errorf("token lifetime = %s; want 30 minutes", ttl) + } +} + +// TestPasswordReset_newerRequestInvalidatesOlderToken: only the most recent link works. +func TestPasswordReset_newerRequestInvalidatesOlderToken(t *testing.T) { + h, database, m := newResetHandler(t) + seedResetUsers(t, database) + requestReset(h, "active@example.com") + _, older := lastResetToken(t, m, "active@example.com") + + // Step past the per-account cooldown without sleeping through it. + aged := time.Now().UTC().Add(-2 * time.Minute).Format(time.RFC3339) + if _, err := database.Exec(`UPDATE password_reset_tokens SET created_at = ?`, aged); err != nil { + t.Fatalf("age token: %v", err) + } + requestReset(h, "active@example.com") + _, newer := lastResetToken(t, m, "active@example.com") + if newer == older { + t.Fatal("second request mailed the same token") + } + + if c := confirmReset(h, older, "fromolderlink1"); c.Code != http.StatusNotFound { + t.Errorf("confirm with superseded token: %d %s; want 404", c.Code, c.Body.String()) + } + if c := confirmReset(h, newer, "fromnewerlink1"); c.Code != http.StatusOK { + t.Errorf("confirm with newest token: %d %s; want 200", c.Code, c.Body.String()) + } + if !canLogIn(h, "active@example.com", "fromnewerlink1") { + t.Error("newest link did not set the password") + } +} + +// TestPasswordReset_cooldownLimitsSendsPerAccount: a burst of requests for one account, +// from however many addresses, mails one link and leaves that link working. +func TestPasswordReset_cooldownLimitsSendsPerAccount(t *testing.T) { + h, database, m := newResetHandler(t) + seedResetUsers(t, database) + for i := range 5 { + requestReset(h, "active@example.com", func(r *http.Request) { + r.RemoteAddr = fmt.Sprintf("198.51.100.%d:4000", i+1) + }) + } + if got := len(m.messages()); got != 1 { + t.Fatalf("emails sent for 5 rapid requests = %d; want 1", got) + } + _, token := lastResetToken(t, m, "active@example.com") + if c := checkReset(h, token); c.Code != http.StatusOK { + t.Errorf("the one link sent was invalidated by requests inside the cooldown: %d", c.Code) + } + + // Another account is not held back by this one's cooldown. + requestReset(h, "bystander@example.com") + lastResetToken(t, m, "bystander@example.com") +} + +// TestPasswordReset_revokesSessions: every session the account held is gone, other +// accounts' sessions are untouched, and the caller is signed in with a fresh one. +func TestPasswordReset_revokesSessions(t *testing.T) { + h, database, m := newResetHandler(t) + seedResetUsers(t, database) + for _, s := range []struct{ id, user string }{ + {"stolen-sess", "u-active"}, {"laptop-sess", "u-active"}, {"other-user-sess", "u-bystander"}, + } { + if _, err := database.Exec(`INSERT INTO sessions (id,user_id,expires_at) VALUES (?,?,'2099-01-01T00:00:00Z')`, s.id, s.user); err != nil { + t.Fatalf("seed session: %v", err) + } + } + requestReset(h, "active@example.com") + _, token := lastResetToken(t, m, "active@example.com") + + rec := confirmReset(h, token, "brandnewpass1") + if rec.Code != http.StatusOK { + t.Fatalf("confirm: %d %s", rec.Code, rec.Body.String()) + } + if n := countRows(t, database, `SELECT COUNT(*) FROM sessions WHERE id IN ('stolen-sess','laptop-sess')`); n != 0 { + t.Errorf("%d pre-reset sessions survived; want all revoked", n) + } + if n := countRows(t, database, `SELECT COUNT(*) FROM sessions WHERE id = 'other-user-sess'`); n != 1 { + t.Error("another account's session was revoked by this reset") + } + + var fresh string + for _, c := range rec.Result().Cookies() { + if c.Name == "calnode_session" { + fresh = c.Value + } + } + if fresh == "" { + t.Fatal("confirm did not sign the user in") + } + if n := countRows(t, database, `SELECT COUNT(*) FROM sessions WHERE id = ? AND user_id = 'u-active'`, fresh); n != 1 { + t.Error("the new session cookie has no matching session row") + } +} + +// TestPasswordReset_accountThatStopsQualifyingCannotComplete covers a link minted while +// the account was eligible and used after it stopped being so: archived by an admin, or +// no longer signing in with a password. +func TestPasswordReset_accountThatStopsQualifyingCannotComplete(t *testing.T) { + cases := []struct { + name, change string + }{ + {"archived", `UPDATE users SET archived_at = '2026-09-01T00:00:00Z' WHERE id = 'u-active'`}, + {"password sign-in turned off", `UPDATE users SET email_login = 0 WHERE id = 'u-active'`}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + h, database, m := newResetHandler(t) + seedResetUsers(t, database) + if _, err := database.Exec(`INSERT INTO sessions (id,user_id,expires_at) VALUES ('existing-sess','u-active','2099-01-01T00:00:00Z')`); err != nil { + t.Fatalf("seed session: %v", err) + } + requestReset(h, "active@example.com") + _, token := lastResetToken(t, m, "active@example.com") + + if _, err := database.Exec(c.change); err != nil { + t.Fatalf("change account: %v", err) + } + if r := checkReset(h, token); r.Code != http.StatusNotFound { + t.Errorf("check: %d; want 404", r.Code) + } + rec := confirmReset(h, token, "brandnewpass1") + if rec.Code != http.StatusNotFound { + t.Errorf("confirm: %d %s; want 404", rec.Code, rec.Body.String()) + } + for _, ck := range rec.Result().Cookies() { + if ck.Name == "calnode_session" { + t.Error("a refused reset set a session cookie") + } + } + var hash string + if err := database.QueryRow(`SELECT password_hash FROM users WHERE id = 'u-active'`).Scan(&hash); err != nil { + t.Fatalf("read hash: %v", err) + } + if bcrypt.CompareHashAndPassword([]byte(hash), []byte("oldpassword")) != nil { + t.Error("a refused reset changed the stored password") + } + if n := countRows(t, database, `SELECT COUNT(*) FROM sessions WHERE id = 'existing-sess'`); n != 1 { + t.Error("a refused reset revoked the account's sessions") + } + }) + } +} + +// TestPasswordReset_confirmEnforcesPasswordPolicyWithoutSpendingTheLink: a rejected +// password is reported with the same rule text as everywhere else and the link survives. +func TestPasswordReset_confirmEnforcesPasswordPolicyWithoutSpendingTheLink(t *testing.T) { + h, database, m := newResetHandler(t) + seedResetUsers(t, database) + requestReset(h, "active@example.com") + _, token := lastResetToken(t, m, "active@example.com") + + for _, bad := range []string{"short", strings.Repeat("x", 73)} { + rec := confirmReset(h, token, bad) + if rec.Code != http.StatusBadRequest || !strings.Contains(rec.Body.String(), "password must be") { + t.Errorf("password of %d chars: %d %s; want 400 with the password rule", len(bad), rec.Code, rec.Body.String()) + } + } + if rec := confirmReset(h, token, "brandnewpass1"); rec.Code != http.StatusOK { + t.Errorf("valid confirm after rejected passwords: %d %s; want 200", rec.Code, rec.Body.String()) + } +} + +func TestPasswordReset_unknownTokensAreRejectedAlike(t *testing.T) { + h, database, _ := newResetHandler(t) + seedResetUsers(t, database) + var first string + for _, tok := range []string{"", "not-a-token", strings.Repeat("ab", 32)} { + for name, rec := range map[string]*httptest.ResponseRecorder{ + "check": checkReset(h, tok), + "confirm": confirmReset(h, tok, "brandnewpass1"), + } { + if rec.Code != http.StatusNotFound { + t.Errorf("%s with token %q: %d; want 404", name, tok, rec.Code) + } + if first == "" { + first = rec.Body.String() + } else if rec.Body.String() != first { + t.Errorf("%s with token %q: body %s differs from %s", name, tok, rec.Body.String(), first) + } + } + } +} diff --git a/internal/server/password_reset_routes_test.go b/internal/server/password_reset_routes_test.go new file mode 100644 index 00000000..71fd7c50 --- /dev/null +++ b/internal/server/password_reset_routes_test.go @@ -0,0 +1,70 @@ +package server + +import ( + "context" + "log/slog" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/calnode/calnode/internal/config" + "github.com/calnode/calnode/internal/db" +) + +// TestPasswordResetRoutes_publicAndRateLimitedPerIP drives the real mux from New, so it +// proves the routes are registered, reachable without a session, and behind a per-IP +// limit whose bucket the three reset routes share and the login route does not. +func TestPasswordResetRoutes_publicAndRateLimitedPerIP(t *testing.T) { + database, err := db.Open("sqlite://:memory:") + if err != nil { + t.Fatalf("db.Open: %v", err) + } + t.Cleanup(func() { database.Close() }) + if err := db.Migrate(database); err != nil { + t.Fatalf("db.Migrate: %v", err) + } + + ctx, cancel := context.WithCancel(context.Background()) + srv, drain := New(ctx, &config.Config{BaseURL: "http://app.example.com", DataDir: t.TempDir()}, + database, slog.New(slog.DiscardHandler)) + t.Cleanup(func() { cancel(); drain() }) + + post := func(path, ip, body string) int { + req := httptest.NewRequest(http.MethodPost, "http://app.example.com"+path, strings.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + req.RemoteAddr = ip + ":40000" + rec := httptest.NewRecorder() + srv.ServeHTTP(rec, req) + return rec.Code + } + const ( + request = "/v1/auth/password-reset/request" + check = "/v1/auth/password-reset/check" + confirm = "/v1/auth/password-reset/confirm" + ip = "192.0.2.50" + ) + emailBody := `{"email":"nobody@example.com"}` + + if got := post(check, ip, `{"token":"nope"}`); got != http.StatusNotFound { + t.Fatalf("check without a session: %d; want 404 from the handler (public route)", got) + } + for i := 2; i <= 10; i++ { + if got := post(request, ip, emailBody); got != http.StatusOK { + t.Fatalf("request %d of 10: %d; want 200", i, got) + } + } + for _, path := range []string{request, check, confirm} { + if got := post(path, ip, emailBody); got != http.StatusTooManyRequests { + t.Errorf("%s after the shared budget is spent: %d; want 429", path, got) + } + } + if got := post(request, "192.0.2.51", emailBody); got != http.StatusOK { + t.Errorf("another IP: %d; want 200 (buckets are per IP)", got) + } + // A separate bucket from login: exhausting reset must not lock anyone out of signing + // in, and the reverse is the case the separate bucket exists for. + if got := post("/v1/auth/login/email", ip, `{"email":"nobody@example.com","password":"wrongpassword"}`); got != http.StatusUnauthorized { + t.Errorf("login from the same IP: %d; want 401 (not rate limited by reset)", got) + } +} diff --git a/internal/server/server.go b/internal/server/server.go index a0825e45..dc461936 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -280,6 +280,17 @@ func New(ctx context.Context, cfg *config.Config, db *sql.DB, logger *slog.Logge mux.HandleFunc("POST /v1/auth/magic-link/request", loginRL(h.RequestMagicLink)) mux.HandleFunc("GET /v1/auth/magic-link/verify", loginRL(h.VerifyMagicLink)) + // Self-service password reset by email (public). A bucket of its own rather than + // loginRL: someone who has just locked themselves out of the login limit with wrong + // passwords is exactly the person who needs this next. The three routes share it, so + // a request, the reset page's check and the confirm are one person's budget. The + // per-IP limit is not the only one: sends are also capped per account + // (passwordResetCooldown), which no number of addresses gets around. + resetRL := RateLimit(10, time.Minute) + mux.HandleFunc("POST /v1/auth/password-reset/request", resetRL(h.RequestPasswordReset)) + mux.HandleFunc("POST /v1/auth/password-reset/check", resetRL(h.CheckPasswordReset)) + mux.HandleFunc("POST /v1/auth/password-reset/confirm", resetRL(h.ConfirmPasswordReset)) + // OAuth login (browser sessions for admin UI). authRL := RateLimit(10, time.Minute) mux.HandleFunc("GET /v1/auth/login", authRL(h.LoginGoogle)) diff --git a/internal/worker/worker.go b/internal/worker/worker.go index b44ed1bf..ee27913c 100644 --- a/internal/worker/worker.go +++ b/internal/worker/worker.go @@ -114,6 +114,12 @@ func (w *Worker) Poll(ctx context.Context) { `DELETE FROM magic_link_tokens WHERE expires_at < ? OR used_at IS NOT NULL`, now); err != nil { w.logger.Error("worker: purge magic link tokens", "error", err) } + // Password-reset tokens likewise. A used row is deleted rather than kept: the + // conditional UPDATE already refused a second use, and a missing row refuses it too. + if _, err := w.db.ExecContext(ctx, + `DELETE FROM password_reset_tokens WHERE expires_at < ? OR used_at IS NOT NULL`, now); err != nil { + w.logger.Error("worker: purge password reset tokens", "error", err) + } // Idempotency keys are only useful for the retry window of the original // request; purge them 24h after creation so the table stays small. idemCutoff := time.Now().UTC().Add(-24 * time.Hour).Format(time.RFC3339)