From 57cbf1eb887b994f43ca89129fc06187f8deb35a Mon Sep 17 00:00:00 2001 From: Distronode <254259913+distronode-com@users.noreply.github.com> Date: Wed, 16 Sep 2026 20:27:03 -0400 Subject: [PATCH 1/2] auth: a forgotten password can be reset by email, with a single-use hashed token The login page has linked to /admin/forgot-password since email and password sign-in shipped, but the page and the endpoint behind it were never built. The SPA fallback served the app shell, no route matched, the layout found no session and sent the visitor back to /admin/login: the "it merely refreshes the page" in #34. The link now leads to a form that emails a reset link, and the link opens a page to choose a new password. Three public routes share a per-IP bucket of their own, so someone who has just hit the login limit can still recover: /v1/auth/password-reset/request, /check and /confirm. The request cannot be used to learn whether an address has an account. It writes one fixed response for every well-formed body, whether the account exists, is archived, signs in with SSO only, or email is not configured, and it does no account-dependent work before answering: the lookup, the token and the send all run in a goroutine afterwards. Magic links already send that way; this moves the lookup off the request too. It is not a worker job because a job's payload is stored, so the raw token could not ride in it, and enqueueing for every typed address would keep attacker-chosen strings in a jobs table that nothing purges. Only an account that could sign in with a password is mailed: the condition LoginEmail uses (email_login set, a hash present), plus not archived. One SQL predicate carries it for all three routes, and it is applied again at the password write, so an account archived during the bcrypt cannot slip through. An SSO-only member still gets a password from an admin. The link is built from BASE_URL and never from the request, which is the classic way to have someone's reset token delivered to a host of the sender's choosing. The token rides in the URL fragment rather than the path (as invites do) or the query (as magic links do), so it never reaches the server or a proxy access log and never appears in a Referer. The page reads it and strips it from the address bar. Tokens are 32 random bytes and only their SHA-256 is stored. A link lasts 30 minutes, longer than a magic link's 15 only to survive greylisting. It is spent by a conditional UPDATE whose rows-affected count is the authority; a cheap lookup runs first so a bad token never costs a bcrypt, and bcrypt runs before the transaction so the single SQLite connection is never held across it. A new request deletes the account's unused tokens, and a per-account cooldown of one minute between emails bounds what any number of addresses can send to one inbox, and how fast a stranger can supersede the owner's newest link. Confirm sets the hash and deletes every session the account holds (as AdminSetPassword does) and its other reset tokens in one transaction, then signs the caller in, as claiming an invite and following a magic link do. API keys and MCP grants survive, as they do for the other two password-change paths. Tests cover enumeration (same status, Content-Type and body across five cases, with nothing minted or sent for four of them), hash-only storage by scanning every column of every table for the raw token, the link under a hostile Host, X-Forwarded-Host, Forwarded, Origin and Referer, single use both sequentially and under eight concurrent confirms, expiry, supersession, the cooldown, session revocation, an account that stops qualifying after its link was sent, a rejected password not spending the link, and, through the real mux from server.New, that the routes are public and share a per-IP limit separate from login's. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 19 + audit/claims.yaml | 33 + docs/ARCHITECTURE.md | 42 +- frontend/src/routes/+layout.svelte | 4 +- .../src/routes/forgot-password/+page.svelte | 118 ++++ frontend/src/routes/login/+page.svelte | 3 +- .../src/routes/reset-password/+page.svelte | 135 ++++ .../00058_password_reset_tokens.sql | 23 + internal/handler/export_test.go | 10 + internal/handler/handler.go | 1 + internal/handler/password_reset.go | 359 ++++++++++ internal/handler/password_reset_test.go | 615 ++++++++++++++++++ internal/server/password_reset_routes_test.go | 70 ++ internal/server/server.go | 11 + internal/worker/worker.go | 6 + 15 files changed, 1445 insertions(+), 4 deletions(-) create mode 100644 frontend/src/routes/forgot-password/+page.svelte create mode 100644 frontend/src/routes/reset-password/+page.svelte create mode 100644 internal/db/migrations/00058_password_reset_tokens.sql create mode 100644 internal/handler/export_test.go create mode 100644 internal/handler/password_reset.go create mode 100644 internal/handler/password_reset_test.go create mode 100644 internal/server/password_reset_routes_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index f45a54ce..239f75a5 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`. + ## [0.9.0] - 2026-09-10 ### Added 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 e91d5db6..e36195f7 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 @@ -672,7 +709,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..9c4f4ca1 --- /dev/null +++ b/frontend/src/routes/reset-password/+page.svelte @@ -0,0 +1,135 @@ + + +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 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 460b46f7..035f70b9 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) From 522dd182132f1018dc591f4838822e9e7a17d2ce Mon Sep 17 00:00:00 2001 From: Distronode <254259913+distronode-com@users.noreply.github.com> Date: Wed, 16 Sep 2026 21:32:36 -0400 Subject: [PATCH 2/2] auth: a failed link check keeps the reset link usable The reset page treated anything short of a 200 from the link check as a dead link, and then removed the #token= fragment from the address bar in a finally block. A network error or a 5xx on a check that is only there for display therefore ended at "invalid or expired" with the token gone, so neither a refresh nor the form could recover it, even though confirm never needed the check: the token is already in memory. A blip cost the user a new email. Only a definitive answer now ends the link. A 404 from check, or from confirm, shows the invalid state and strips the fragment; a 200 from check strips it too, as before. Anything else from check (a transport failure, a 5xx, a 429) is not a verdict on the link: the fragment stays so a refresh can try again, the form stays usable, and a notice offers a retry that reruns the check in place. Confirm's own 404 remains the authority on whether the link is valid. replaceState is still only reached after an await, on those definitive answers, because on a first page load SvelteKit's router is still starting when onMount runs and the call fails until it has. An empty token has nothing to check and nothing secret to remove, so it goes straight to the invalid state without touching the URL. Verified in a browser against a built binary: a 500 on check keeps the token through a reload and a retry then succeeds and strips it; a refused connection on check keeps the token and confirm still sets the password; a used link's 404, from check or from confirm, strips the fragment; and a clean first load still strips it with no page error. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/routes/reset-password/+page.svelte | 62 ++++++++++++++----- 1 file changed, 47 insertions(+), 15 deletions(-) diff --git a/frontend/src/routes/reset-password/+page.svelte b/frontend/src/routes/reset-password/+page.svelte index 9c4f4ca1..35a472b6 100644 --- a/frontend/src/routes/reset-password/+page.svelte +++ b/frontend/src/routes/reset-password/+page.svelte @@ -10,17 +10,26 @@ let token = ''; let phase = $state<'checking' | 'invalid' | 'ready'>('checking'); let accountEmail = $state(''); + // Set when the check could not give an answer (network error, 5xx, rate limit). That is + // not a verdict on the link, so the form stays usable and the fragment stays in the + // address bar for a refresh; confirm's own 404 is what decides the link is dead. + let checkError = $state(''); + let checking = $state(false); let password = $state(''); let confirmPassword = $state(''); let submitting = $state(false); let error = $state(''); - onMount(async () => { - token = new URLSearchParams(window.location.hash.slice(1)).get('token') ?? ''; - if (!token) { - phase = 'invalid'; - return; - } + // Drop the token from the address bar and the history entry once the server has given a + // definitive answer about it. Only ever called after an await: on the first page load + // SvelteKit's router is still starting when onMount runs, and replaceState throws until + // it has. + function dropTokenFromURL() { + if (window.location.hash) replaceState(window.location.pathname, {}); + } + + async function checkLink() { + checking = true; try { const res = await fetch('/v1/auth/password-reset/check', { method: 'POST', @@ -29,21 +38,35 @@ }); if (res.ok) { accountEmail = (await res.json()).email; + checkError = ''; phase = 'ready'; - } else if (res.status === 429) { - phase = 'ready'; - error = 'Too many attempts. Wait a minute and try again.'; - } else { + dropTokenFromURL(); + } else if (res.status === 404) { phase = 'invalid'; + dropTokenFromURL(); + } else { + checkError = + res.status === 429 + ? 'Too many attempts to check this link. Wait a minute, then try again.' + : "We couldn't check this link just now."; + phase = 'ready'; } } catch { - phase = 'invalid'; + checkError = "We couldn't reach the server to check this link."; + phase = 'ready'; } finally { - // Read once, then drop the token from the address bar and the history entry. Not - // before the first await: on the first page load SvelteKit's router is still - // starting when onMount runs, and replaceState throws until it has. - replaceState(window.location.pathname, {}); + checking = false; + } + } + + onMount(() => { + token = new URLSearchParams(window.location.hash.slice(1)).get('token') ?? ''; + if (!token) { + // Nothing to check and nothing secret in the address bar to remove. + phase = 'invalid'; + return; } + checkLink(); }); async function reset(e: SubmitEvent) { @@ -65,6 +88,7 @@ window.location.href = data.signed_in ? '/admin' : '/admin/login'; } else if (res.status === 404) { phase = 'invalid'; + dropTokenFromURL(); } else if (res.status === 429) { error = 'Too many attempts. Wait a minute and try again.'; } else { @@ -103,6 +127,14 @@ 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}