diff --git a/backend/InTheKnow.Tests/ApiTests.cs b/backend/InTheKnow.Tests/ApiTests.cs index f7caee5..a28120f 100644 --- a/backend/InTheKnow.Tests/ApiTests.cs +++ b/backend/InTheKnow.Tests/ApiTests.cs @@ -158,7 +158,7 @@ public async Task EveryApiRouteNeedsASession() internal static readonly HashSet PublicRoutes = [ "/api/auth/bot-check", "/api/auth/sign-up", "/api/auth/confirmation", "/api/auth/confirm", "/api/auth/sign-in", - "/api/auth/password-reset", "/api/auth/password-reset/complete", "/api/auth/email-change/confirm", "/api/auth/invite/accept", + "/api/auth/password-reset", "/api/auth/password-reset/check", "/api/auth/password-reset/complete", "/api/auth/email-change/confirm", "/api/auth/invite/accept", ]; // A route pattern with its parameters filled with 1. diff --git a/backend/InTheKnow.Tests/SignInTests.cs b/backend/InTheKnow.Tests/SignInTests.cs index 29e6b7f..5785a93 100644 --- a/backend/InTheKnow.Tests/SignInTests.cs +++ b/backend/InTheKnow.Tests/SignInTests.cs @@ -317,4 +317,58 @@ public async Task ResetConfirmsAndAddsAPassword() Assert.Equal(1, await d.Db.ScalarAsync("SELECT count(*) FROM account_message WHERE kind = 'methodAdded' AND account_id = $1", keyOnly.AccountId)); await d.H.SignInViaAsync("password", keyOnly.Email, "chosen-password-34"); } + + // --- 009: the reset link is checked before a password is typed ------------------------------------------------- + + static Task CheckAsync(DeliveryHost d, string? token) => + SignUpTests.PostAsync(d, "/api/auth/password-reset/check", new { token }); + + // 009 US1, FR-002, FR-007 + [Fact] + public async Task ResetCheckRefusesDeadLinks() + { + await using var d = await SignUpTests.HostAsync(); + var used = await ResetTokenAsync(d, d.S1.Email); + await Expect(await CompleteAsync(d, used, "new-password-4567"), HttpStatusCode.NoContent); + await Expect(await CheckAsync(d, used), HttpStatusCode.BadRequest, "token_invalid"); + + var replaced = await ResetTokenAsync(d, d.S1.Email); + var current = await ResetTokenAsync(d, d.S1.Email); + await Expect(await CheckAsync(d, current), HttpStatusCode.NoContent); + + await SignUpTests.SignUpAsync(d, "pending@example.nl"); + await d.NotifyAsync(2); + var confirmation = SignUpTests.TokenOf(SignUpTests.Mails(d, "confirm").Last(m => m.Target == "pending@example.nl").Message); + + var disabled = await ResetTokenAsync(d, d.S2.Email); + await d.Db.ExecAsync("UPDATE account SET disabled_at = $2 WHERE id = $1", d.S2.AccountId, d.Now); + + foreach (var dead in new[] { "nonsense", "", confirmation, disabled }) + await Expect(await CheckAsync(d, dead), HttpStatusCode.BadRequest, "token_invalid"); + await Expect(await SignUpTests.PostAsync(d, "/api/auth/password-reset/check", new { }), HttpStatusCode.BadRequest, "token_invalid"); + + d.H.Time.Advance(TimeSpan.FromHours(1) + TimeSpan.FromSeconds(1)); + await Expect(await CheckAsync(d, current), HttpStatusCode.BadRequest, "token_invalid"); + Assert.NotEqual(replaced, current); + } + + // 009 US2, FR-005, SC-002, SC-004 + [Fact] + public async Task ResetCheckChangesNothing() + { + await using var d = await SignUpTests.HostAsync(); + var a = d.S1; + var token = await ResetTokenAsync(d, a.Email); + async Task SnapshotAsync() => string.Join("|", + (await d.Db.RowsAsync("SELECT id, used_at, token_expires_at FROM account_message WHERE account_id = $1 ORDER BY id", a.AccountId)).Select(r => string.Join(",", r)), + await d.Db.ScalarAsync("SELECT count(*) FROM session WHERE account_id = $1", a.AccountId), + await d.Db.ScalarAsync("SELECT count(*) FROM security_event WHERE account_id = $1", a.AccountId), + SignUpTests.Mails(d, "reset").Count); + var before = await SnapshotAsync(); + for (var i = 0; i < 3; i++) await Expect(await CheckAsync(d, token), HttpStatusCode.NoContent); + Assert.Equal(before, await SnapshotAsync()); + + await Expect(await CompleteAsync(d, token, "new-password-4567"), HttpStatusCode.NoContent); + await d.H.SignInViaAsync("password", a.Email, "new-password-4567"); + } } diff --git a/backend/InTheKnow/AccountApi.cs b/backend/InTheKnow/AccountApi.cs index c563639..7aa4c9b 100644 --- a/backend/InTheKnow/AccountApi.cs +++ b/backend/InTheKnow/AccountApi.cs @@ -84,6 +84,11 @@ await BotGateAsync(bot, body?.BotCheck, true, http.RequestAborted) ?? await sign await accounts.CompleteResetAsync(body?.Token, body?.Password, http) is { } error ? Refuse(error) : Results.NoContent()) .AllowAnonymous().RequireRateLimiting("public"); + // 009: side-effect free, so the reset page can show a dead link before a password is typed. + auth.MapPost("/password-reset/check", async (TokenPasswordRequest? body, HttpContext http, Accounts accounts) => + await accounts.ResetUsableAsync(body?.Token, http.RequestAborted) ? Results.NoContent() : Refuse("token_invalid")) + .AllowAnonymous().RequireRateLimiting("public"); + // The code step of a sign-in with 2FA on: only for an awaiting session (research R6). auth.MapPost("/sign-in/code", (ClaimsPrincipal user, CodeRequest? body, HttpContext http, SignIn signIn) => user.HasClaim(c => c.Type == Claims.Awaiting) ? signIn.CodeAsync(user.SessionId(), user.AccountId(), body?.Code, http) diff --git a/backend/InTheKnow/Accounts.cs b/backend/InTheKnow/Accounts.cs index c9ee06e..5548088 100644 --- a/backend/InTheKnow/Accounts.cs +++ b/backend/InTheKnow/Accounts.cs @@ -204,6 +204,26 @@ public async Task RequestResetAsync(string? email, CancellationToken ct) await tx.CommitAsync(ct); } + // The one rule for "this reset link can still be used", shared by the check and the submit (009 FR-002). + static async Task<(long MessageId, long AccountId, string Email, string Language, bool Confirmed)?> FindResetAsync( + NpgsqlConnection conn, string? token, DateTimeOffset now, CancellationToken ct) + { + await using var cmd = new NpgsqlCommand(""" + SELECT m.id, a.id, a.email, a.language, a.confirmed_at IS NOT NULL + FROM account_message m JOIN account a ON a.id = m.account_id + WHERE m.token_hash = $1 AND m.kind = 'reset' AND m.used_at IS NULL AND m.token_expires_at > $2 AND a.disabled_at IS NULL + """, conn) { Parameters = { new() { Value = SignIn.Hash(token ?? "") }, new() { Value = now } } }; + await using var r = await cmd.ExecuteReaderAsync(ct); + return await r.ReadAsync(ct) ? (r.GetInt64(0), r.GetInt64(1), r.GetString(2), r.GetString(3), r.GetBoolean(4)) : null; + } + + // 009: a pure read, so a link scanner or a page open never uses the link up. + public async Task ResetUsableAsync(string? token, CancellationToken ct) + { + await using var conn = await db.OpenConnectionAsync(ct); + return await FindResetAsync(conn, token, time.GetUtcNow(), ct) is not null; + } + // FR-009, FR-010: a new password (added when there was none), every session ends, the throttle is cleared, and an // unconfirmed account is confirmed. 2FA is untouched. public async Task CompleteResetAsync(string? token, string? password, HttpContext http) @@ -211,19 +231,7 @@ public async Task RequestResetAsync(string? email, CancellationToken ct) var ct = http.RequestAborted; var now = time.GetUtcNow(); await using var conn = await db.OpenConnectionAsync(ct); - long messageId, accountId; - string email, language; - bool confirmed; - await using (var cmd = new NpgsqlCommand(""" - SELECT m.id, a.id, a.email, a.language, a.confirmed_at IS NOT NULL - FROM account_message m JOIN account a ON a.id = m.account_id - WHERE m.token_hash = $1 AND m.kind = 'reset' AND m.used_at IS NULL AND m.token_expires_at > $2 AND a.disabled_at IS NULL - """, conn) { Parameters = { new() { Value = SignIn.Hash(token ?? "") }, new() { Value = now } } }) - await using (var r = await cmd.ExecuteReaderAsync(ct)) - { - if (!await r.ReadAsync(ct)) return "token_invalid"; - (messageId, accountId, email, language, confirmed) = (r.GetInt64(0), r.GetInt64(1), r.GetString(2), r.GetString(3), r.GetBoolean(4)); - } + if (await FindResetAsync(conn, token, now, ct) is not var (messageId, accountId, email, language, confirmed)) return "token_invalid"; if (Passwords.Check(password, email) is { } rule) return rule; await using var tx = await conn.BeginTransactionAsync(ct); diff --git a/frontend/src/routes/account/reset/+page.server.ts b/frontend/src/routes/account/reset/+page.server.ts index 3628c4b..df6d482 100644 --- a/frontend/src/routes/account/reset/+page.server.ts +++ b/frontend/src/routes/account/reset/+page.server.ts @@ -1,7 +1,17 @@ -import { api, clearSession } from '$lib/server/api'; +import { api, clearSession, must } from '$lib/server/api'; import { raw, refuse } from '$lib/server/actions'; -export const load = ({ url }) => ({ hasToken: Boolean(url.searchParams.get('token')) }); +// 009: a dead link says so before a password is typed. After a submit the action's result decides the page, and a re-check +// would find the token just used (or fail on a rate limit) and hide "done"; reset links are always a full page load. +export const load = async (event) => { + const token = event.url.searchParams.get('token'); + if (!token) return { usable: false }; + if (event.request.method !== 'GET' || event.isDataRequest) return { usable: true }; + const r = await api(event, 'POST', '/api/auth/password-reset/check', { token }); + if (r.ok) return { usable: true }; + if (r.status === 400 && r.reason === 'token_invalid') return { usable: false }; + return must(event, r) as never; +}; export const actions = { default: async (event) => { diff --git a/frontend/src/routes/account/reset/+page.svelte b/frontend/src/routes/account/reset/+page.svelte index 7977e4d..db481d6 100644 --- a/frontend/src/routes/account/reset/+page.svelte +++ b/frontend/src/routes/account/reset/+page.svelte @@ -22,7 +22,7 @@

{t.reset.done}

-{:else if !data.hasToken || r?.reason === 'token_invalid'} +{:else if !data.usable || r?.reason === 'token_invalid'}

{t.reset.newLink}

diff --git a/frontend/src/routes/account/reset/reset.test.ts b/frontend/src/routes/account/reset/reset.test.ts new file mode 100644 index 0000000..da744b7 --- /dev/null +++ b/frontend/src/routes/account/reset/reset.test.ts @@ -0,0 +1,44 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { fakeEvent } from '$lib/server/fake'; +import * as apiModule from '$lib/server/api'; +import { load } from './+page.server'; + +vi.mock('$lib/server/api', async (orig) => ({ ...(await orig()), api: vi.fn() })); +const api = vi.mocked(apiModule.api); + +beforeEach(() => vi.clearAllMocks()); + +const run = (o: Parameters[0]) => load(fakeEvent(o).event as never); +const attempt = (fn: () => T) => + Promise.resolve() + .then(fn) + .catch((e: unknown) => e); + +describe('reset page load', () => { + it('shows the dead-link panel without calling the API when there is no token', async () => { + expect(await run({ url: '/account/reset' })).toEqual({ usable: false }); + expect(api).not.toHaveBeenCalled(); + }); + + it('does not re-check after a submit (POST or data request)', async () => { + expect(await run({ url: '/account/reset?token=T', method: 'POST' })).toEqual({ usable: true }); + expect(await run({ url: '/account/reset?token=T', isDataRequest: true })).toEqual({ usable: true }); + expect(api).not.toHaveBeenCalled(); + }); + + it('shows the form when the check says the link is usable', async () => { + api.mockResolvedValue({ ok: true, status: 204, data: undefined }); + expect(await run({ url: '/account/reset?token=T' })).toEqual({ usable: true }); + expect(api.mock.calls[0].slice(1, 4)).toEqual(['POST', '/api/auth/password-reset/check', { token: 'T' }]); + }); + + it('shows the dead-link panel on token_invalid', async () => { + api.mockResolvedValue({ ok: false, status: 400, reason: 'token_invalid', extra: {} }); + expect(await run({ url: '/account/reset?token=T' })).toEqual({ usable: false }); + }); + + it.each([503, 429])('shows the error page when the check answers %i', async (status) => { + api.mockResolvedValue({ ok: false, status, reason: status === 503 ? 'unavailable' : 'rate_limited', extra: {} }); + expect(await attempt(() => run({ url: '/account/reset?token=T' }))).toMatchObject({ status }); + }); +}); diff --git a/specs/004-accounts-login/contracts/api.md b/specs/004-accounts-login/contracts/api.md index d832ee1..9d98411 100644 --- a/specs/004-accounts-login/contracts/api.md +++ b/specs/004-accounts-login/contracts/api.md @@ -150,6 +150,11 @@ fully signed in. A used recovery code queues `recoveryCodeUsed` with the number `{ "email": "…", "botCheck": "…?" }` → `202` always. A link is sent only if the address has a not-disabled account under its cap of 3 per 24 h. +### `POST /api/auth/password-reset/check` 🚦 (009) + +`{ "token": "…" }` → `204` when the link can still be used, `400 token_invalid` otherwise. Side-effect free; see +[009 contract](../../009-reset-link-precheck/contracts/reset-check.md). + ### `POST /api/auth/password-reset/complete` 🚦 (FR-009, FR-010, US3-2 to US3-5) `{ "token": "…", "password": "…" }` → `204`. The password is replaced or added, an unconfirmed account is confirmed, diff --git a/specs/005-sveltekit-frontend/contracts/pages.md b/specs/005-sveltekit-frontend/contracts/pages.md index d6649f9..b4733ff 100644 --- a/specs/005-sveltekit-frontend/contracts/pages.md +++ b/specs/005-sveltekit-frontend/contracts/pages.md @@ -83,6 +83,7 @@ second factor gets the "locked" page with a way forward (US8-1). Both come from | `POST /api/auth/sign-in` | `/sign-in` | ✓ | | `POST /api/auth/sign-in/code` | `/sign-in/code` | ✓ (code and recovery code) | | `POST /api/auth/password-reset` | `/account/forgot-password` | ✓ | +| `POST /api/auth/password-reset/check` | `/account/reset` (load) | ✓ | | `POST /api/auth/password-reset/complete` | `/account/reset` | ✓ | | `POST /api/auth/email-change/confirm` | `/account/email` | ✓ | | `POST /api/auth/invite/accept` | `/account/invite` | ✓ | diff --git a/specs/009-reset-link-precheck/checklists/requirements.md b/specs/009-reset-link-precheck/checklists/requirements.md new file mode 100644 index 0000000..a130855 --- /dev/null +++ b/specs/009-reset-link-precheck/checklists/requirements.md @@ -0,0 +1,35 @@ +# Specification Quality Checklist: Check the password-reset link before the new password is typed + +**Purpose**: Validate specification completeness and quality before proceeding to planning +**Created**: 2026-10-21 +**Feature**: [spec.md](../spec.md) + +## Content Quality + +- [x] No implementation details (languages, frameworks, APIs) +- [x] Focused on user value and business needs +- [x] Written for non-technical stakeholders +- [x] All mandatory sections completed + +## Requirement Completeness + +- [x] No [NEEDS CLARIFICATION] markers remain +- [x] Requirements are testable and unambiguous +- [x] Success criteria are measurable +- [x] Success criteria are technology-agnostic (no implementation details) +- [x] All acceptance scenarios are defined +- [x] Edge cases are identified +- [x] Scope is clearly bounded +- [x] Dependencies and assumptions identified + +## Feature Readiness + +- [x] All functional requirements have clear acceptance criteria +- [x] User scenarios cover primary flows +- [x] Feature meets measurable outcomes defined in Success Criteria +- [x] No implementation details leak into specification + +## Notes + +- All items pass on the first validation pass. No clarifications were needed: the issue is narrow and every open + choice has a safe default that matches current behaviour (see Assumptions). diff --git a/specs/009-reset-link-precheck/contracts/reset-check.md b/specs/009-reset-link-precheck/contracts/reset-check.md new file mode 100644 index 0000000..82e7198 --- /dev/null +++ b/specs/009-reset-link-precheck/contracts/reset-check.md @@ -0,0 +1,30 @@ +# Contract: reset-link check + +## `POST /api/auth/password-reset/check` 🚦 (FR-001 to FR-007) + +Public (no session), `"public"` per-source rate limit, no bot check. + +`{ "token": "…" }` → `204` when the token can still complete a reset under the same rules as +`POST /api/auth/password-reset/complete`. + +| Status | `reason` | When | +|---|---|---| +| `400` | `token_invalid` | missing, empty or unknown token; not a reset token; used; expired; account disabled | +| `429` | (rate limit) | too many requests from the source | + +- Side-effect free: the token is not used, changed or extended; no session ends, no email is queued, no security-log + entry is written. Repeated calls give the same answer until the token is used or expires. +- The answer never says which rule failed and carries no account detail. +- `/complete` keeps checking the token itself; a `204` here is not a promise. + +## Page `/account/reset?token=…` + +| Situation | Shown | +|---|---| +| no `token` | "link no longer works" panel + `/account/forgot-password` link (no API call) | +| check `204` | the new-password form, as before | +| check `400 token_invalid` | "link no longer works" panel + `/account/forgot-password` link, no password field | +| check fails otherwise (API down, 429, 5xx) | the standard error page (`must()`) | +| form sent (any result) | `load` skips the check after a submit (POST, or the `use:enhance` data request), so the action's result decides the page | +| form sent, reset done | the "done" panel | +| form sent, `token_invalid` | "link no longer works" panel, as before | diff --git a/specs/009-reset-link-precheck/data-model.md b/specs/009-reset-link-precheck/data-model.md new file mode 100644 index 0000000..6704e60 --- /dev/null +++ b/specs/009-reset-link-precheck/data-model.md @@ -0,0 +1,13 @@ +# Data model: reset-link pre-check + +No schema change and no new stored data. + +The check only reads what `CompleteResetAsync` already reads: + +| Table | Columns read | Rule (FR-002) | +|---|---|---| +| `account_message` | `token_hash`, `kind`, `used_at`, `token_expires_at`, `account_id` | `token_hash = hash(token)`, `kind = 'reset'`, `used_at IS NULL`, `token_expires_at > now` | +| `account` | `disabled_at` | `disabled_at IS NULL` | + +Nothing is written: `used_at` and `token_expires_at` stay as they are, and no `session`, `security_event` or queued +`account_message` row is added or removed (FR-005, SC-004). Token lifetime (1 h) and single use are unchanged. diff --git a/specs/009-reset-link-precheck/plan.md b/specs/009-reset-link-precheck/plan.md new file mode 100644 index 0000000..787614a --- /dev/null +++ b/specs/009-reset-link-precheck/plan.md @@ -0,0 +1,94 @@ +# Implementation Plan: Check the password-reset link before the new password is typed + +**Branch**: `009-reset-link-precheck` | **Date**: 2026-10-21 | **Spec**: [spec.md](./spec.md) + +**Input**: Feature specification from `/specs/009-reset-link-precheck/spec.md` + +## Summary + +The reset page's `load` asks a new, side-effect-free `POST /api/auth/password-reset/check { token }` whether the +link is still usable, and shows the existing "this link no longer works" panel instead of the form when it is not. +The backend answers with the same lookup `CompleteResetAsync` already uses (moved into one shared helper, so both +apply exactly the same rules, FR-002): `204` usable, `400 token_invalid` otherwise. Sending the form still checks +again (FR-006). No schema, text or dependency changes. + +## Technical Context + +**Language/Version**: C# on .NET 10 (backend); TypeScript, Svelte 5 / SvelteKit 2 (frontend) + +**Primary Dependencies**: existing only (ASP.NET Core minimal APIs, Npgsql; SvelteKit); no new package + +**Storage**: PostgreSQL, read-only for this feature: `account_message` joined to `account` (no schema change) + +**Testing**: xUnit v3 (`backend/InTheKnow.Tests`), vitest (`frontend`) + +**Target Platform**: Linux server (Docker stack from 006); any browser + +**Project Type**: web application (backend API + SvelteKit frontend) + +**Performance Goals**: one indexed lookup per page open (`token_hash` is already looked up this way on submit), SC-003 + +**Constraints**: the check changes nothing (FR-005, link scanners); token travels in the POST body, never in a URL +the API logs; the answer is only usable / not usable (FR-007) + +**Scale/Scope**: 1 new API route, 1 page load, 0 new texts + +## Constitution Check + +*GATE: Must pass before Phase 0 research. Re-check after Phase 1 design.* + +| Principle / rule | Status | +|---|---| +| I. Simplicity: smallest implementation, no speculative code | PASS: one route reusing the existing lookup; page reuses the existing `token_invalid` panel and texts; other emailed links stay out of scope | +| I. New dependency or layer justified | PASS: none added | +| II. No new personal data | PASS: nothing stored; the answer carries no account detail (FR-007) | +| II. No secrets or deployment details in repo | PASS: tests and quickstart use `localhost`/Mailpit and test accounts | +| II. Trust boundary validated server-side | PASS: the API decides; the frontend only renders its answer; submit re-checks (FR-006) | +| II. State change only on deliberate action | PASS: the check is a pure SELECT; tests assert no row in `account_message`, `security_event`, `session` or the mail queue changes (SC-004) | +| II. Output encoded by default | PASS: no new output; existing Svelte-escaped texts | +| II. Auth via established libraries | PASS: reuses `SignIn.Hash` and the existing token rows; no new crypto | +| Tech: GPL-compatible, lockfiles | PASS: no new package | +| Workflow: non-trivial logic has an automated check | PASS: backend test for usable/used/expired/unknown/disabled/empty + no side effects; frontend `load` test for no token / usable / invalid / unavailable | + +Post-design re-check: PASS (no dependency, layer, stored data or text added; the lookup is shared, not copied). + +## Project Structure + +### Documentation (this feature) + +```text +specs/009-reset-link-precheck/ +├── plan.md +├── research.md +├── data-model.md +├── quickstart.md +├── contracts/reset-check.md +└── tasks.md # /speckit-tasks +``` + +### Source Code (repository root) + +```text +backend/InTheKnow/ +├── Accounts.cs # reset-token lookup moved into one helper; new ResetUsableAsync(token, ct); CompleteResetAsync uses the helper +└── AccountApi.cs # + POST /api/auth/password-reset/check (AllowAnonymous, "public" rate limit) + +backend/InTheKnow.Tests/ +├── ApiTests.cs # PublicRoutes += "/api/auth/password-reset/check" +└── SignInTests.cs # US3 section: check answers, repeated checks keep the link usable, no side effects + +frontend/src/routes/account/reset/ +├── +page.server.ts # load: no token → usable false (no call); after a submit (POST or data request) → true (no call, the action decides); ok → true; token_invalid → false; else must() → error page +├── +page.svelte # `!data.hasToken` → `!data.usable`; the `'reset' in form` branch stays first +└── reset.test.ts # new: the four load outcomes + +specs/004-accounts-login/contracts/api.md # Public routes: the new route +specs/005-sveltekit-frontend/contracts/pages.md # API → page table: the new route on /account/reset +``` + +**Structure Decision**: the existing web-application layout (`backend/`, `frontend/`); no new projects or folders +beyond the one test file. + +## Complexity Tracking + +No violations. diff --git a/specs/009-reset-link-precheck/quickstart.md b/specs/009-reset-link-precheck/quickstart.md new file mode 100644 index 0000000..666f2d0 --- /dev/null +++ b/specs/009-reset-link-precheck/quickstart.md @@ -0,0 +1,26 @@ +# Quickstart: validating the reset-link pre-check + +## Automated + +```bash +dotnet test backend/InTheKnow.Tests --filter "FullyQualifiedName~SignInTests|FullyQualifiedName~ApiTests" +``` + +```bash +cd frontend && npm test && npm run check +``` + +Expected: all pass. The backend tests cover a usable token answering `204` repeatedly and still completing a reset, +`token_invalid` for used, replaced, expired, unknown, disabled-account and empty tokens, and unchanged `account_message`, +`session` and `security_event` rows after checks. The frontend `reset.test.ts` covers the `load` outcomes from +[contracts/reset-check.md](./contracts/reset-check.md). + +## Manual (dev stack + Mailpit) + +1. Open `/account/forgot-password`, request a reset for a test account, and open the link from Mailpit. + Expect the new-password form. +2. Reload the page a few times, then set a new password. Expect the "done" panel, and signing in with the new + password works (US2-1). +3. Open the same link again. Expect "This link no longer works." and the new-link link, with no password field (US1-2). +4. Open `/account/reset` with no token, and with `?token=nonsense`. Expect the same panel (US1-3, US1-5). +5. With a fresh link's page open, stop the backend and reload. Expect the standard error page, not the form (FR-008). diff --git a/specs/009-reset-link-precheck/research.md b/specs/009-reset-link-precheck/research.md new file mode 100644 index 0000000..6ecd81a --- /dev/null +++ b/specs/009-reset-link-precheck/research.md @@ -0,0 +1,44 @@ +# Research: reset-link pre-check + +No NEEDS CLARIFICATION remained in the Technical Context; these are the design choices. + +## R1. A POST route with the token in the body + +- **Decision**: `POST /api/auth/password-reset/check { "token": "…" }`. +- **Rationale**: a `GET …?token=` or `GET …/{token}` puts a live reset token into request paths, which ASP.NET and + the frontend's API logging record (008 FR-008, `LogHygieneTests`). The body keeps it out, like `/complete`. + POST does not imply a change here; the route is documented and tested as side-effect free. +- **Alternatives**: GET with the token in the path (logged); folding the check into the page's own `load` by reading + the database from SvelteKit (the frontend has no database access by design). + +## R2. One shared lookup, not a copy + +- **Decision**: move the reset-token `SELECT` (`kind = 'reset'`, unused, unexpired, account not disabled) out of + `CompleteResetAsync` into one helper that both the check and the submit call. +- **Rationale**: FR-002 requires exactly the same rules; two copies of the WHERE clause would drift. +- **Alternatives**: a second SQL string (drift risk); a database view (more than one query needs). + +## R3. Answer shape + +- **Decision**: `204` when usable, `400 { reason: "token_invalid" }` for every unusable reason, including a missing or + empty token. +- **Rationale**: reuses the existing reason, the existing frontend text and `refuse`/`must` handling; one reason for + every cause satisfies FR-007 and tells a link holder nothing new. +- **Alternatives**: `200 { usable: bool }` (a second way to say the same thing). + +## R4. Abuse limits + +- **Decision**: `AllowAnonymous().RequireRateLimiting("public")`, no bot check, like `/complete`. +- **Rationale**: the check reveals no more than `/complete` with an empty password already does, and tokens are 256-bit + random values; the same per-source limit applies. +- **Alternatives**: a bot check (would block link scanners and add a step before the page shows; not asked for). + +## R5. Frontend behaviour + +- **Decision**: the page `load` calls the check only when `token` is present; `token_invalid` → the existing "link + no longer works" panel; any other failure → `must()` → the standard error page (FR-008). +- **Rationale**: after a submit SvelteKit re-runs `load`. `load` skips the check after a submit (a POST, or the data + request `use:enhance` makes after a successful action), so a re-check that finds + the token just used, or fails with `429`/`503`, cannot replace the "done" panel with the error page. The + `'reset' in form` branch stays first in `+page.svelte`. +- **Alternatives**: a client-side fetch after render (shows the form first, the problem the issue describes). diff --git a/specs/009-reset-link-precheck/spec.md b/specs/009-reset-link-precheck/spec.md new file mode 100644 index 0000000..4d2f717 --- /dev/null +++ b/specs/009-reset-link-precheck/spec.md @@ -0,0 +1,118 @@ +# Feature Specification: Check the password-reset link before the new password is typed + +**Feature Branch**: `009-reset-link-precheck` + +**Created**: 2026-10-21 + +**Status**: Draft + +**Input**: User description: "Email: check password-reset link validity before the user types a new password" +(InTheKnow roadmap, [Wjouter/InTheKnow#17](https://github.com/Wjouter/InTheKnow/issues/17)): "Validate the reset +token when the page opens, so the user sees an expired/used link error up front instead of after typing a new +password and then getting the message that the link does not work." + +## User Scenarios & Testing *(mandatory)* + +### User Story 1 - A dead link says so straight away (Priority: P1) + +A student forgot their password, asked for a reset email last week, and only now clicks the link. The link has +expired (or was already used). Today the page asks for a new password, the student thinks one up, types it, presses +the button, and only then learns the link does not work. With this feature the page tells them at once that the +link no longer works and offers to send a new one, before they type anything. + +**Why this priority**: this is the whole issue. It saves the user a wasted step and the confusion of a password +that was "accepted" by the form but never saved. + +**Independent Test**: request a reset, use the link once (or let it expire), open the same link again, and check +that the page shows the "link does not work" message and the "request a new link" link, with no password field. + +**Acceptance Scenarios**: + +1. **Given** a reset link that has expired, **When** it is opened, **Then** the page shows the "this link does not + work" message and a link to request a new reset email, and no password field. +2. **Given** a reset link that was already used to set a password, **When** it is opened again, **Then** the same + message is shown. +3. **Given** a reset link that was replaced or made useless for any other reason (unknown link, account disabled), + **When** it is opened, **Then** the same message is shown. +4. **Given** a valid, unused reset link, **When** it is opened, **Then** the page shows the new-password form as + it does today. +5. **Given** the page reached without any link code at all, **When** it is opened, **Then** the same message is + shown, as today. + +--- + +### User Story 2 - A valid link still works after the check (Priority: P1) + +A student opens a fresh reset link. Opening the page checks it but does not use it up: they can type a new password +and set it, exactly as today. + +**Why this priority**: the check must never break the normal path. Mail systems of schools and companies open links +in emails automatically to scan them, so opening the page must not count as using the link. + +**Independent Test**: request a reset, open the link (several times, as a scanner would), then set a new password +with it and sign in with that password. + +**Acceptance Scenarios**: + +1. **Given** a valid reset link, **When** it is opened one or more times, **Then** it stays valid and the password + can still be set with it. +2. **Given** a valid link whose page is open, **When** the link expires or is used elsewhere before the form is sent, + **Then** sending the form shows the "this link does not work" message, as today, and the password is not + changed. + +--- + +### Edge Cases + +- **Link scanners**: opening the page (by a person or a scanner) only reads whether the link is still good. It never + uses the link, changes the password, ends sessions or sends an email. +- **Expiry while the page is open**: the check at opening is a convenience. The check when the form is sent stays + the final word (User Story 2, scenario 2). +- **No account details shown**: the page says only whether the link works. It does not show the email address or + any other account detail, for a good or a bad link, so a leaked link reveals no more than it does today. +- **Guessing links**: checking a link reveals no more than sending the form with it already does, and link codes + are too long to guess. The existing limits on repeated attempts keep applying. +- **Service down**: if the check cannot be done (temporary server problem), the page shows the normal "something + went wrong, try again" error rather than a form that may not work. +- **Language**: the message follows the visitor's current language (English or Dutch), as the rest of the page does. +- **Other emailed links** (invite, email confirmation, email change): out of scope. They can get the same treatment + later if wanted. + +## Requirements *(mandatory)* + +### Functional Requirements + +- **FR-001**: When the password-reset page is opened with a link code, the system MUST check whether that code can + still be used to set a new password, before showing the form. +- **FR-002**: A code counts as usable under exactly the same rules the system already uses when the form is sent: + it exists, belongs to a password reset, is not used, has not expired, and its account is not disabled. +- **FR-003**: If the code is not usable, the page MUST show the existing "this link does not work" message and the + link to request a new reset email, and MUST NOT show the password field. +- **FR-004**: If the code is usable, the page MUST show the new-password form as it does today. +- **FR-005**: The check MUST NOT use up, change or extend the code, and MUST have no other effect (no password + change, no ended sessions, no email, no security-log entry). +- **FR-006**: Sending the form MUST still check the code again and refuse an unusable one, exactly as today. +- **FR-007**: The check MUST answer only "usable" or "not usable"; it MUST NOT say why a code is not usable and MUST + NOT return any account detail. +- **FR-008**: If the check fails for a reason other than an unusable code, the page MUST show the general error + instead of the form. + +## Success Criteria *(mandatory)* + +### Measurable Outcomes + +- **SC-001**: In 100% of tests with an expired, used, unknown or missing link code, the user sees the "this link does + not work" message without being asked for a password. +- **SC-002**: In 100% of tests with a valid code, the password can be set after the page was opened any number of + times. +- **SC-003**: The reset page with a bad link appears as fast as it does today with a good one (no noticeable extra + wait). +- **SC-004**: Opening the page produces zero changes to the account, its sessions, its emails or the security log. + +## Assumptions + +- The existing "this link does not work" message and "request a new link" link are reused; no new wording is needed. +- The same message is used for every unusable reason (expired, used, unknown, disabled account), as today; telling + them apart is not asked for and would tell a link holder more about the account. +- Reset links keep their current lifetime and single use; this feature does not change how links are made or sent. +- Only the password-reset page is in scope (see Edge Cases for the other emailed links). diff --git a/specs/009-reset-link-precheck/tasks.md b/specs/009-reset-link-precheck/tasks.md new file mode 100644 index 0000000..0fabf8f --- /dev/null +++ b/specs/009-reset-link-precheck/tasks.md @@ -0,0 +1,185 @@ +--- + +description: "Task list for 009 reset-link pre-check" +--- + +# Tasks: Check the password-reset link before the new password is typed + +**Input**: Design documents from `/specs/009-reset-link-precheck/` + +**Prerequisites**: plan.md, spec.md, research.md, data-model.md, contracts/reset-check.md, quickstart.md + +**Tests**: included; the plan's Constitution Check requires a backend test (usable/used/expired/unknown/disabled/empty ++ no side effects) and a frontend `load` test (no token / usable / invalid / unavailable). + +**Organization**: US1 (dead link says so) and US2 (valid link still works) are both P1 and share one route and one +`load`; the route lives in Phase 2, each story owns its tests and page behaviour. + +## Format: `[ID] [P?] [Story] Description` + +--- + +## Phase 1: Setup + +None: no new project, package, schema or text (plan: "no schema, text or dependency changes"). + +--- + +## Phase 2: Foundational (Blocking Prerequisites) + +**Purpose**: the side-effect-free check route both stories call. + +- [X] T001 In `backend/InTheKnow/Accounts.cs`, move the reset-token `SELECT` out of `CompleteResetAsync` (lines ~217–226) + into one private helper (e.g. `FindResetAsync(conn, token, now, ct)`) returning the row (`m.id, a.id, a.email, + a.language, confirmed`) or null, keeping the WHERE clause verbatim: `m.token_hash = $1 AND m.kind = 'reset' AND + m.used_at IS NULL AND m.token_expires_at > $2 AND a.disabled_at IS NULL` with `SignIn.Hash(token ?? "")` + (research R2, FR-002). `CompleteResetAsync` calls the helper and still returns `"token_invalid"` on null, then + keeps its password check and `UPDATE … WHERE used_at IS NULL` re-check unchanged (FR-006). +- [X] T002 In `backend/InTheKnow/Accounts.cs`, add `public async Task ResetUsableAsync(string? token, + CancellationToken ct)`: opens a connection, calls the T001 helper with `time.GetUtcNow()`, returns `row != null`. + Pure read: no transaction, no UPDATE, no `SecurityLog`, no queue (FR-005). +- [X] T003 In `backend/InTheKnow/AccountApi.cs`, after the `/password-reset/complete` route (line ~83), add + `auth.MapPost("/password-reset/check", async (TokenPasswordRequest? body, HttpContext http, Accounts accounts) => await + accounts.ResetUsableAsync(body?.Token, http.RequestAborted) ? Results.NoContent() : Refuse("token_invalid")) + .AllowAnonymous().RequireRateLimiting("public");` (reuses `TokenPasswordRequest` from line 12; the password is + ignored); no bot check (research R4). Answer only `204` or + `400 token_invalid` (FR-007, contract). +- [X] T004 [P] In `backend/InTheKnow.Tests/ApiTests.cs`, add `"/api/auth/password-reset/check"` to `PublicRoutes` + (line ~158) so the auth-required sweep stays green. + +**Checkpoint**: `dotnet build backend/InTheKnow` succeeds; existing `CompleteReset` tests still pass. + +--- + +## Phase 3: User Story 1 - A dead link says so straight away (Priority: P1) 🎯 MVP + +**Goal**: an expired, used, unknown, disabled-account or missing token shows the "link no longer works" panel +before any password is typed. + +**Independent Test**: request a reset, use the link, open it again → panel + `/account/forgot-password` link, no +password field. + +### Tests for User Story 1 + +- [X] T005 [US1] In `backend/InTheKnow.Tests/SignInTests.cs`, after `ResetConfirmsAndAddsAPassword`, add a + `// 009 US1, FR-002, FR-007` test `ResetCheckRefusesDeadLinks`: using `ResetTokenAsync`/`CompleteAsync` and a + `CheckAsync(d, token)` helper (`SignUpTests.PostAsync(d, "/api/auth/password-reset/check", new { token })`), expect + `400 token_invalid` for: a used token (after a `204` complete), a replaced token (request two resets, check the + first), an expired token (advance the fake clock past 1 h, + as `CompleteReset` does for `late`), `"nonsense"`, `""`, a missing body field, a non-reset token (e.g. a confirmation + token if the fixtures expose one), and a token whose account has `disabled_at` set. +- [X] T006 [P] [US1] Create `frontend/src/routes/account/reset/reset.test.ts` in the style of + `frontend/src/routes/sign-in/code/code.test.ts` (`vi.mock('$lib/server/api', …)`, `fakeEvent`), covering `load`: + no `token` → `{ usable: false }` and `api` not called; a POST request, and a data request (`isDataRequest: true`), with a token → `{ usable: true }` + and `api` not called; `api` returns `400 { reason: 'token_invalid' }` → + `{ usable: false }`. Mock `must` with its real behaviour (`vi.importActual`) or keep it unmocked. + +### Implementation for User Story 1 + +- [X] T007 [US1] In `frontend/src/routes/account/reset/+page.server.ts`, make `load` async: no `token` → + `{ usable: false }` without an API call; after a submit → `{ usable: true }` without an API call, where "after a + submit" is `event.request.method !== 'GET' || event.isDataRequest` (no-JS POST, or the data request `use:enhance`'s + `update()` makes after a successful action; a failed action does not re-run `load`, and reset links are always a + full page load). The action already checked the token and its result picks the page ("done", the `token_invalid` + panel, or the form with a password error); a re-check here would find the token just used, or fail with + `429`/`503` and hide "done". Otherwise `api(event, 'POST', '/api/auth/password-reset/check', { token })`; + `r.ok` → `{ usable: true }`; `r.status === 400 && reason === 'token_invalid'` → `{ usable: false }`; anything + else → `must(event, r)` (from `$lib/server/api`, line ~80) so the standard error page shows (FR-008, research R5). Leave + `actions.default` unchanged. +- [X] T008 [US1] In `frontend/src/routes/account/reset/+page.svelte` line ~25, replace `!data.hasToken` with + `!data.usable`; keep the `{#if form && 'reset' in form}` branch first so the success panel still shows after + `load` re-runs on a used token (research R5). + +**Checkpoint**: T005, T006 pass; quickstart manual steps 3–4 show the panel. + +--- + +## Phase 4: User Story 2 - A valid link still works after the check (Priority: P1) + +**Goal**: opening the page (any number of times, by a person or a link scanner) never uses up the link. + +**Independent Test**: request a reset, check/open it several times, then set a new password and sign in with it. + +### Tests for User Story 2 + +- [X] T009 [US2] In `backend/InTheKnow.Tests/SignInTests.cs`, add `// 009 US2, FR-005, SC-002, SC-004` test + `ResetCheckChangesNothing`: get a fresh token, snapshot `account_message` (`used_at`, `token_expires_at` of the + row), `count(*)` of `session` and `security_event` (the table `CompleteReset` queries) for the account, and the queued-mail count; call `CheckAsync` + three times expecting `204` each; assert every snapshot value is unchanged; then `CompleteAsync` with a new + password expects `204` and sign-in with it succeeds. +- [X] T010 [P] [US2] In `frontend/src/routes/account/reset/reset.test.ts`, add `load` cases: `api` ok (`204`) → + `{ usable: true }` and the body sent is `{ token }` to `/api/auth/password-reset/check`; `api` returns `503` (or + `429`) → `load` throws (the `must()` error), not `{ usable: … }` (FR-008). + +### Implementation for User Story 2 + +No new code: T002/T003 are read-only and T001 keeps the submit re-check (FR-006). If T009 fails, fix it in +`backend/InTheKnow/Accounts.cs` (T002), not in the test. + +**Checkpoint**: quickstart manual steps 1–2 and 5 pass. + +--- + +## Phase 5: Polish & Cross-Cutting Concerns + +- [X] T011 [P] In `specs/004-accounts-login/contracts/api.md`, add `POST /api/auth/password-reset/check` to the public + routes with `204` / `400 token_invalid`, side-effect free (link to `specs/009-reset-link-precheck/contracts/reset-check.md`). +- [X] T012 [P] In `specs/005-sveltekit-frontend/contracts/pages.md`, add the check route to the API → page table row for + `/account/reset` (load). +- [X] T013 Run the quickstart automated commands: `dotnet test backend/InTheKnow.Tests --filter + "FullyQualifiedName~SignInTests|FullyQualifiedName~ApiTests|FullyQualifiedName~LogHygieneTests"` and + `cd frontend && npm test && npm run check`; all green. +- [X] T014 Walk `specs/009-reset-link-precheck/quickstart.md` manual steps 1–5 on the dev stack with Mailpit. + +--- + +## Dependencies & Execution Order + +- **Phase 2**: T001 → T002 → T003 (same two backend files, in order); T004 any time. +- **US1 (Phase 3)**: after Phase 2. T006 ‖ T005; T007 → T008. +- **US2 (Phase 4)**: after Phase 2; T010 extends the file from T006 (create it in T010 if US2 is done alone). +- **Polish**: T011 ‖ T012 any time; T013–T014 after both stories. + +### Within Each User Story + +- Tests first; T005/T009 fail until Phase 2 lands, T006/T010 until T007. +- T005 and T009 touch the same file (`SignInTests.cs`): run in order. + +### Parallel Opportunities + +- T004 (`ApiTests.cs`) ‖ T001–T003. +- Backend tests (T005, T009) ‖ frontend tests (T006, T010). +- T011 ‖ T012. + +--- + +## Parallel Example: User Story 1 + +```bash +Task: "Add ResetCheckRefusesDeadLinks in backend/InTheKnow.Tests/SignInTests.cs" +Task: "Create frontend/src/routes/account/reset/reset.test.ts with the no-token and token_invalid load cases" +``` + +--- + +## Implementation Strategy + +### MVP First + +US1 and US2 are both P1 and ship together: US1 without US2's guarantee would be unsafe (a check that used up links +would break every reset opened by a mail scanner). Order: Phase 2 → US1 → US2 → Polish. + +### Incremental Delivery + +1. Phase 2 → route exists, submit unchanged. +2. US1 → dead links show the panel up front. +3. US2 → proven side-effect free. +4. Polish → contracts agree, quickstart passes, commit. + +--- + +## Notes + +- No schema, text, dependency or new reason added (plan, data-model.md). +- Token only in the POST body, never a URL (research R1, `LogHygieneTests`). +- Other emailed links (invite, confirmation, email change) are out of scope (spec Edge Cases).