Skip to content

auth: a forgotten password can be reset by email, with a single-use hashed token - #53

Closed
distronode-com wants to merge 3 commits into
Calnode:mainfrom
distronode-corporation:up/forgot-password
Closed

distronode-com wants to merge 3 commits into
Calnode:mainfrom
distronode-corporation:up/forgot-password

Conversation

@distronode-com

Copy link
Copy Markdown
Contributor

What was broken

The login page links to /admin/forgot-password whenever email is configured, but that page and any 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. That is the "it merely refreshes the page" in #34.

What it does now

"Forgot password?" opens a form that emails a reset link. The link opens a page that shows which account is being reset and asks for a new password. Saving it signs out every other session and signs the user in.

  • POST /v1/auth/password-reset/request {email}: always the same 200 and body.
  • POST /v1/auth/password-reset/check {token}: does not use up the token. Returns the account email so the page can show it and password managers have a username.
  • POST /v1/auth/password-reset/confirm {token, password}: sets the password.
  • The link on the login page now appears only when email is configured and password sign-in is in use. With email off, visiting the page directly explains that an admin can reset the password, or calnode reset-admin from the command line.
  • New migration 00058_password_reset_tokens. The worker cleans up expired and used tokens, just like magic-link tokens.

Design choices

  • No way to tell if an account exists. The request returns one fixed response whether the account exists, is archived, uses SSO only, is in its cooldown, or email is off. It does nothing that depends on the account before answering. The lookup, the token and the send all run in a goroutine afterwards. Magic links already send that way. This also takes the lookup off the request.
  • A goroutine, not a worker job. A job's payload is stored, so the raw token can't go in it. Queueing a job for every address someone types would leave attacker-chosen strings in jobs, which nothing cleans up.
  • Who can reset: the same test LoginEmail uses (email_login = 1 and a stored hash), plus not archived. One SQL predicate covers all three routes and is checked again when the password is written. An SSO-only member still gets a password from an admin. Like a session, a link is judged by the account's state when it is used.
  • The link comes from BASE_URL, never from the request. A hostile Host, X-Forwarded-Host, Forwarded, Origin or Referer has no effect on it.
  • The token is in the URL fragment (#token=), not the path (as invites do) or the query string (as magic links do). Browsers never send the fragment to a server, so it can't show up in a proxy access log or a Referer. That includes the admin app's Google Fonts request. The page reads the token, then removes it from the address bar.
  • Token: 32 random bytes, hex. Only the SHA-256 is stored.
    • Lifetime: 30 minutes. That is longer than a magic link's 15 only to allow for greylisting mail servers. A reset link is as powerful as a login link.
    • Single use. A conditional UPDATE ... WHERE used_at IS NULL AND expires_at > ? spends the token, and its rows-affected count decides the outcome. A cheap lookup runs first so a bad token never costs a bcrypt.
    • bcrypt runs before the transaction, so the single SQLite connection is never held while hashing.
  • A new request cancels older links, like issueInvite does. Each account gets at most one email a minute. The per-IP limit can't enforce that across many IPs. Without it, a stranger could also cancel the owner's newest link faster than the owner can click it.
  • Rate limit: 10 requests a minute per IP, in a bucket separate from loginRL. Someone who just hit the login limit can still recover. The three reset routes share the bucket.
  • Confirm matches the existing paths. In one transaction it deletes every session (as AdminSetPassword does) and the account's other reset tokens. Then it signs the user in, as claiming an invite and following a magic link do.
  • The email is English only, like invites and magic links (the admin side isn't translated). Its format is based on magicLinkMessage.

Tests

  • internal/handler/password_reset_test.go:
    • No enumeration: status, Content-Type and body are identical for an existing account, an unknown address, an archived account, an SSO-only account, and email off. Nothing is minted or sent in the last four cases.
    • Only the hash is stored: every column of every table is scanned for the raw token. The same scanner must find the hash, so a scanner that reads nothing can't pass.
    • Link built from BASE_URL under hostile Host, X-Forwarded-Host, Forwarded, Origin and Referer headers, with the token in the fragment.
    • Single use, both one after another and with eight simultaneous confirms.
    • Expiry and the 30-minute lifetime.
    • A newer request cancels the older link.
    • The per-account cooldown, including requests from five different IPs.
    • Sessions revoked, other accounts' sessions untouched, new session created.
    • An account archived, or taken off password sign-in, after its link was sent can't finish the reset.
    • A rejected password doesn't use up the link.
    • Unknown tokens all get the same response.
  • internal/server/password_reset_routes_test.go uses the real mux from server.New. It checks that the routes are public, share a per-IP limit, and don't eat into login's limit.
  • Mutation checks:
    • Removing used_at IS NULL from the spending UPDATE fails the concurrent test (8 of 8 succeed). The sequential test still passes because the lookup also guards, which is why both tests exist.
    • Ignoring rows-affected fails the concurrent test.
    • Removing the guard from both the lookup and the UPDATE fails both tests.
    • Building the link from r.Host fails the Host-header test.
  • Checked in a real browser against a local build and an SMTP sink:
    • The link on the login page, the same message for unknown and real addresses, and the received email.
    • The token removed from the address bar and absent from every request URL.
    • The new password works and the old one doesn't.
    • A second browser's session is revoked.
    • A used link shows the invalid state.
    • No horizontal overflow at 375px.
    • The email-off guidance.
  • gofmt -l ., go vet ./..., go build ./..., go test ./..., pnpm check and pnpm build all pass. Locally, gosec reports 0 issues in the touched packages.

What it deliberately does not do

  • It doesn't revoke API keys or connected MCP apps. Neither POST /v1/users/me/password nor the admin reset does either. Revoking them on any password change should be one decision covering all three paths.
  • It doesn't give a password to an account that never had one (SSO-only).
  • It doesn't translate the email or the pages. The admin side is English only by design (ARCHITECTURE §23).
  • It doesn't change magic links, invites, or calnode reset-admin.
  • A reset requested while the server is shutting down may not send, as with magic links. The goroutine isn't tied to the shutdown drain.

Fixes #34

🤖 Generated with Claude Code

…ashed 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) <noreply@anthropic.com>

@shockalotti shockalotti left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is excellent security engineering and I intend to approve once CI is green. Enumeration resistance (uniform response + background send so there is not even a timing oracle), token-in-fragment, single-use with the conditional UPDATE as the race authority, eligibility re-checked at write (the archive-during-bcrypt TOCTOU is closed), dedicated rate-limit bucket that correctly avoids the login-lockout interplay, BASE_URL-built links, and the claims.yaml entry with an honest caveat. The frontend uses shadcn Button/Input/Label per the repo's UI rule. Two non-blocking questions:

  1. Confirm revokes sessions but not MCP OAuth tokens or API keys — disclosed in the claims.yaml caveat as consistent with the other two password-change paths, which I accept as the right call for this PR (making reset stricter alone would be the inconsistent one). But it leaves a real gap in the compromise story: a reset is what someone does when they believe their password is known, and the attacker's agent bearer survives it. Proposal, as a separate cross-cutting decision covering all three password-change paths (not here):password change revokes sessions + MCP tokens, documented the same way. Worth tracking as an issue if you agree.
  2. Did pnpm test:visual run for the three new routes? The repo rule triggers on ui/**, app.css and theme changes, so new routes built from existing components may not strictly need it — just confirming.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No critical issues — one recovery-path edge on the reset page.

Reviewed changes

Self-service password reset for #34: public request/check/confirm routes, hashed single-use tokens, admin SPA pages, and tests for enumeration, host-header poisoning, and concurrent spend.

  • Public reset API — RequestPasswordReset answers one fixed 200 then mints and mails in a goroutine; CheckPasswordReset is non-consuming; ConfirmPasswordReset spends the token with a conditional UPDATE, revokes sessions, and signs in.
  • Token storage — migration 00058_password_reset_tokens stores SHA-256 only; worker purges expired and used rows like magic links.
  • Admin SPA — /forgot-password and /reset-password are public routes; the login "Forgot password?" link requires both SMTP and password login.
  • Limits and claims — dedicated 10/min/IP bucket (not loginRL), per-account send cooldown, audit/claims.yaml entry for no-enumeration and BASE_URL links.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using Grok | 𝕏

Comment thread frontend/src/routes/reset-password/+page.svelte Outdated
@shockalotti

Copy link
Copy Markdown
Contributor

Maintainer decision on my question 1 above, so it does not linger: password changes leave API keys and MCP bearers in place. A reset/compromise recovery that silently kills an operator's scripts and agents would be its own outage, and consistency across all three password paths matters more than the stricter single path. Sessions-only revocation stands as designed — no change requested.

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) <noreply@anthropic.com>

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

The reset page no longer treats a failed check as a dead link.

  • Keep the form on a failed check — network errors, 5xx, and 429 leave #token= in the address bar, show a retry notice, and still allow confirm. Only 404 (or a missing token) marks the link invalid and strips the fragment.

Pullfrog  | View workflow run | Using Grok | 𝕏

CHANGELOG conflict only: kept both [Unreleased] entries.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@distronode-com

Copy link
Copy Markdown
Contributor Author

On 2: not when this was opened, since it touches no ui/**, app.css or theme file. I've run it now on the merged head (62a6f68): pnpm test:visual passes 5/5 and svelte-check reports 0 errors. That suite checks Switch, Checkbox and Calendar state styling, none of which these pages use. They are built from Button, Input and Label, unchanged.

On 1: understood, sessions only.

I see #34 was closed against 659626d on fix/mcp-bearer-archived. Since that is what your production runs now, here is where it differs from this PR on the points your review singled out, so the choice is made with them in view:

  • The token is in the query string (/admin/reset-password?token=). Calnode's own request log records only the path, but a reverse proxy or load balancer in front logs the full URI by default, and the token works for an hour. This PR carries it in the fragment, which the browser never sends.
  • No per-account send limit, and every link stays live. Each request for a known address mints another token and sends another email, limited only per IP, so someone rotating addresses can fill a user's inbox with working links. This PR sends at most one email a minute per account, and a new link revokes the older ones.
  • It shares loginRL. Forgot and reset requests spend the same 10-a-minute per-IP budget as password and magic-link sign-in, so a burst of reset requests from one office address blocks sign-in from there for the rest of that minute. This PR has its own bucket.
  • Eligibility is checked before bcrypt, and the password write is unconditional. An account archived while the hash runs still gets its password changed. This PR re-applies the eligibility check in the UPDATE.

On the database, either order is safe. Both create password_reset_tokens as 00058 with the same five columns. On a database that has run yours, goose skips this PR's 00058, and this code works against your table (it only lacks the (user_id, created_at) index). The SPA paths are the same; the API paths differ.

So, two ways forward: merge this and drop 659626d, or keep 659626d, close this, and I'll send those four points as a small PR on top of it.

@distronode-com

Copy link
Copy Markdown
Contributor Author

Closing this now that 659626d is merged in #57. The four differences from my earlier comment still apply to it, in brief:

  • the reset token travels in the query string, where proxy logs record it;
  • there is no per-account send limit, and every link stays live;
  • reset requests share the sign-in rate-limit bucket;
  • eligibility is checked before bcrypt, so the password write can land on an account archived in between.

If you'd like any of them, say which and I'll send a small PR against main.

@github-actions github-actions Bot locked and limited conversation to collaborators Sep 17, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Forgot password button does not seem to do anything

2 participants