From 52f1fa3db03394a26e67f94b9e17e517c2c6220e Mon Sep 17 00:00:00 2001 From: saqib mir Date: Mon, 7 Sep 2026 17:26:49 +0530 Subject: [PATCH] docs(013-auth-hardening): plan, research, data model, contract, quickstart Reset tokens live only in Redis as a paired key shape (mirrors 010's own revocation-denylist pattern) - never in Postgres, never storing the raw token. Password-strength policy is one shared validator called from both the new reset-consume endpoint and 010's existing POST /admin/users. Login rate-limiting reuses the existing checkRateLimit helper from 002's own inbound trust boundary, keyed by submitted email, checked before any credential verification. Co-Authored-By: Claude Sonnet 5 --- .../contracts/auth-hardening-contract.md | 48 +++++++ specs/013-auth-hardening/data-model.md | 50 +++++++ specs/013-auth-hardening/plan.md | 126 ++++++++++++++++++ specs/013-auth-hardening/quickstart.md | 37 +++++ specs/013-auth-hardening/research.md | 83 ++++++++++++ 5 files changed, 344 insertions(+) create mode 100644 specs/013-auth-hardening/contracts/auth-hardening-contract.md create mode 100644 specs/013-auth-hardening/data-model.md create mode 100644 specs/013-auth-hardening/plan.md create mode 100644 specs/013-auth-hardening/quickstart.md create mode 100644 specs/013-auth-hardening/research.md diff --git a/specs/013-auth-hardening/contracts/auth-hardening-contract.md b/specs/013-auth-hardening/contracts/auth-hardening-contract.md new file mode 100644 index 0000000..abdffe7 --- /dev/null +++ b/specs/013-auth-hardening/contracts/auth-hardening-contract.md @@ -0,0 +1,48 @@ +# Contract: Authentication Hardening + +## `POST /auth/password-reset/request` + +**Auth**: None (like login itself — the caller has no session yet). + +**Request body**: `{ "email": "string" }` + +**Response `200`** (always, regardless of whether the account exists): + +```json +{ "success": true, "data": { "message": "If that account exists, a reset link has been sent." }, "meta": null } +``` + +No token, ever, appears in this response — it's only visible via the stub's own server-side log +line (`{ "event": "password_reset_requested", "userId": "...", "resetUrl": "..." }`). + +## `POST /auth/password-reset/consume` + +**Auth**: None (the token itself is the credential). + +**Request body**: `{ "token": "string", "newPassword": "string" }` + +**Responses**: +- `200` — `{ "success": true, "data": { "message": "Password updated." }, "meta": null }` +- `400 VALIDATION_ERROR` — `newPassword` doesn't meet `validatePasswordStrength`. +- `400 INVALID_RESET_TOKEN` (or equivalent) — token missing, expired, or already used. The + response never distinguishes which of the three — matching data-model.md's own note that a + consumer can't otherwise tell "expired" from "already used" from "never existed." + +## `PATCH /admin/users` — unchanged route, tightened validation + +`POST /admin/users` (010-identity-auth) now also rejects a `password` shorter than +`PASSWORD_MIN_LENGTH` with the same `validatePasswordStrength` message the reset-consume +endpoint uses — no new route, no schema field change, just a stricter check on the existing +`password` field. + +## `POST /auth/login` — unchanged route, new pre-check + +Before this feature: any number of attempts, any speed. After: attempts for the same submitted +`email` beyond `LOGIN_RATE_LIMIT_MAX_ATTEMPTS` within `LOGIN_RATE_LIMIT_WINDOW_SECONDS` receive: + +```json +{ "success": false, "error": { "code": "RATE_LIMIT_EXCEEDED", "message": "Too many login attempts. Try again later." } } +``` + +with HTTP `429`, distinct from the existing `401` identical-failure-response 010 already +returns for wrong credentials. diff --git a/specs/013-auth-hardening/data-model.md b/specs/013-auth-hardening/data-model.md new file mode 100644 index 0000000..106c039 --- /dev/null +++ b/specs/013-auth-hardening/data-model.md @@ -0,0 +1,50 @@ +# Data Model: Authentication Hardening + +No Postgres schema changes. `User.passwordHash` (010-identity-auth) is updated in place by a +successful reset; no other model changes. + +## Redis-only: Password Reset Token + +Not a Prisma model — exists only as two paired Redis keys, both expiring together. + +| Key | Value | TTL | +|---|---|---| +| `password-reset:token:` | `userId` | `PASSWORD_RESET_TOKEN_LIFETIME_MINUTES` | +| `password-reset:user:` | `sha256(token)` | same | + +**Issuing** (`requestPasswordReset`): if `password-reset:user:` already has a value, +delete `password-reset:token:` first (invalidating the prior token — FR-002), then +set both new keys. + +**Consuming** (`resetPassword`): `GET password-reset:token:` → if +absent, reject (FR-004: invalid/expired/already-used, indistinguishably — the key not existing +covers all three cases identically, which is itself desirable: a consumer can't tell "expired" +from "already used" from "never existed," matching the same non-leaking spirit as 010's own +login-failure parity). If present, resolve `userId`, delete both keys (single-use), update the +password. + +## Configuration (new) + +| Env var | Purpose | Default | +|---|---|---| +| `PASSWORD_MIN_LENGTH` | Minimum password length, enforced everywhere a password is set | `10` | +| `PASSWORD_RESET_TOKEN_LIFETIME_MINUTES` | How long a reset token stays valid | `30` | +| `LOGIN_RATE_LIMIT_MAX_ATTEMPTS` | Max login attempts per email per window | `5` | +| `LOGIN_RATE_LIMIT_WINDOW_SECONDS` | The window `LOGIN_RATE_LIMIT_MAX_ATTEMPTS` applies over | `300` | + +## Validation / Business Rules + +- `requestPasswordReset(email)`: always returns the same shape regardless of whether `email` + resolves to a real, active account (FR-001) — internally, only issues a real token when it + does; the caller-visible response is identical either way. +- `resetPassword(token, newPassword)`: `validatePasswordStrength` runs first (fail fast on the + cheap, stateless check), then the token is looked up. Unlike login/reset-request, + account-existence secrecy doesn't apply here — FR-004 and User Story 2 both call for their + *own*, specific rejection reasons ("password too short" vs. "invalid or expired token"); only + FR-001's account-existence question needs the identical-response treatment, not this + endpoint's two legitimately-different failure modes. +- `login(email, password)`: the rate-limit check (`login:`) runs first, before + `repo.findByEmail`/`verifyPassword` (FR-007) — a rate-limited request never reaches the + identical-failure-response logic 010 already built; it gets its own distinct rate-limit + rejection instead (Acceptance Scenario 1's own point: a rate limit is an honestly-different + condition from a credentials failure, not disguised as one). diff --git a/specs/013-auth-hardening/plan.md b/specs/013-auth-hardening/plan.md new file mode 100644 index 0000000..f52b39a --- /dev/null +++ b/specs/013-auth-hardening/plan.md @@ -0,0 +1,126 @@ +# Implementation Plan: Authentication Hardening + +**Branch**: `013-auth-hardening` | **Date**: 2026-09-07 | **Spec**: [spec.md](./spec.md) + +**Input**: Feature specification from `specs/013-auth-hardening/spec.md` + +## Summary + +Adds `POST /auth/password-reset/request` and `POST /auth/password-reset/consume` to +`identity/auth` (the module that already owns login/logout/self-identity mechanics), backed by +a Redis-stored, single-use reset token — the "delivery" step logs the token server-side rather +than emailing it. Adds a shared password-strength validator used by both the reset-consume +endpoint and 010's own `POST /admin/users`. Adds a pre-credential-check rate limit to +`POST /auth/login`, reusing the existing `checkRateLimit` helper 002's own inbound trust +boundary already established. + +## Technical Context + +**Language/Version**: TypeScript 5.4 / Node.js 20+ (unchanged). + +**Primary Dependencies**: None new — reuses `crypto` (Node built-in, for token generation and +hashing), the existing `ioredis` client, and `zod`. + +**Storage**: No schema change. Reset tokens live entirely in Redis (never in Postgres) — two +keys per active token, mirroring the existing revocation-denylist's own Redis-key-with-TTL shape: +`password-reset:token:` → `userId`, and `password-reset:user:` → +`sha256(token)`, both with the same TTL (the reset token's own lifetime). The second key is what +lets issuing a new token invalidate the previous one (FR-002) without a database table. + +**Testing**: Vitest — unit tests for the password-strength validator and the rate-limit's own +pre-credential-check ordering; integration tests against real Postgres/Redis for the full +request → (read the token from the stub's log output) → consume → login-with-new-password flow, +the identical-response-regardless-of-existing-account behavior, and the login rate limit +actually rejecting the N+1th attempt while a different account's login proceeds normally. + +**Target Platform**: Same Fastify modular monolith. Modifies `identity/auth` (new routes, +service methods, the shared password-strength validator) and `identity/agents` (existing +`POST /admin/users` now calls the shared validator instead of accepting any password +unchecked). + +**Project Type**: Backend service — single project. + +**Performance Goals**: The login rate-limit check is one Redis `INCR` (already how +`checkRateLimit` works) — no added database round trip on the login hot path, consistent with +010's own performance goal for `fastify.authenticate`. + +**Constraints**: FR-001/SC-001 — reset-request must respond identically regardless of account +existence, including timing-shape (the same pattern 010's login already established: do the +same amount of work either way). FR-007 — the rate-limit check MUST run before +`bcrypt.compare`, not after i.e. before any password-verification cost is paid, both for +FR-007's own ordering requirement and so a rate-limited attacker gains no timing signal from a +skipped bcrypt call. + +**Scale/Scope**: Two new routes, one new shared validator, one new env-configured rate-limit +policy, one modified existing endpoint (`POST /admin/users`). No new module, no schema +migration, no new module dependencies. Explicitly excludes: MFA, real email delivery, IP-based +rate limiting, password complexity rules beyond minimum length (spec.md Assumptions). + +## Constitution Check + +*GATE: Must pass before Phase 0 research. Re-check after Phase 1 design.* + +| Principle / Section | Check | Result | +|---|---|---| +| I. SaaS Is the Sole Identity & Access Authority | Same carve-out as 010 — this hardens SupportHub's own staff authentication, never touching SaaS-delegated customer identity. | PASS | +| II. Configuration Over Hardcoding | Password minimum length and the login rate-limit's max-attempts/window are both new env-configured values (`PASSWORD_MIN_LENGTH`, `LOGIN_RATE_LIMIT_MAX_ATTEMPTS`, `LOGIN_RATE_LIMIT_WINDOW_SECONDS`), never hardcoded magic numbers — matches spec.md's own Assumptions and the roadmap's "never hardcode a placeholder value and ship it as final." | PASS | +| III. Layered Architecture With Enforced Module Boundaries | Reset endpoints live in `identity/auth` (owns auth mechanics); the shared password-strength validator is exported from `identity/auth`'s own public `index.ts` for `identity/agents` to consume, the same precedent `hashPassword`/`verifyPassword` themselves already set. | PASS | +| IV. AI Recommends, Deterministic Policy Decides | Not applicable. | PASS — N/A | +| V. Evidence-Based Verification | Not applicable. | PASS — N/A | +| VI. Durable Audit & History | Not applicable — no new audit-relevant mutable domain state (a password hash change isn't itself an audited business event in this codebase's existing model). | PASS — N/A | +| VII. Concurrency-Safe, Durable Job Handling | Reset-token issuance/consumption is a single Redis operation per step, no shared in-memory state; two concurrent consume attempts for the same token race safely (Redis `GET`+`DEL` — the loser sees the key already gone and is rejected, not a partial/double-apply). | PASS | +| VIII. Problem and Ticket Are Separate, Related Entities | Not applicable. | PASS — N/A | +| Technology & Platform Constraints | No new dependencies or infrastructure — email delivery is explicitly stubbed (spec.md Assumptions, user decision), not a real provider integration. | PASS | + +No violations requiring Complexity Tracking justification. + +## Project Structure + +### Documentation (this feature) + +```text +specs/013-auth-hardening/ +├── plan.md +├── research.md +├── data-model.md +├── quickstart.md +├── contracts/ +└── tasks.md +``` + +### Source Code (repository root) + +```text +supporthub-api/ +├── src/ +│ ├── config/ +│ │ └── auth.ts # MODIFIED — passwordMinLength, loginRateLimit config +│ └── modules/ +│ └── identity/ +│ ├── auth/ # MODIFIED +│ │ ├── mapper/ +│ │ │ └── password-policy.ts # NEW — shared validatePasswordStrength +│ │ ├── mapper/ +│ │ │ └── reset-token.ts # NEW — generate/hash reset tokens +│ │ ├── repository/ +│ │ │ └── reset-token.repository.ts # NEW — the two-Redis-key shape +│ │ ├── service/ # MODIFIED — requestPasswordReset, resetPassword, +│ │ │ login's new pre-check rate-limit call +│ │ ├── controller/ routes/ # MODIFIED — the two new routes +│ │ └── schema/ # MODIFIED — request/consume body schemas +│ └── agents/ +│ └── service/ +│ └── users.service.ts # MODIFIED — calls the shared validator +└── tests/ + ├── unit/identity/ # password-policy validator, rate-limit ordering + └── integration/ # full reset flow, identical-response check, + login rate-limit behavior +``` + +**Structure Decision**: Single project, no new module. Everything lives in `identity/auth` +(already owns login/logout/self-identity) except the one-line call site change in +`identity/agents/service/users.service.ts`. + +## Complexity Tracking + +*No constitution violations — table intentionally omitted.* diff --git a/specs/013-auth-hardening/quickstart.md b/specs/013-auth-hardening/quickstart.md new file mode 100644 index 0000000..cd58a71 --- /dev/null +++ b/specs/013-auth-hardening/quickstart.md @@ -0,0 +1,37 @@ +# Quickstart: Validating Authentication Hardening + +## Scenario 1 — password reset, end to end + +1. `POST /auth/password-reset/request` with a real seeded account's email. **Expected**: `200`, + generic message; the server log shows a `password_reset_requested` line with a `resetUrl` + containing the real token. +2. Repeat with an email that doesn't exist. **Expected**: identical `200` response body to + step 1 — diff them to confirm. +3. `POST /auth/password-reset/consume` with the token from step 1's log and a policy-meeting new + password. **Expected**: `200`. +4. Repeat step 3 with the same token. **Expected**: rejected — the token is single-use. +5. `POST /auth/login` with the account's email and the new password from step 3. **Expected**: + `200`. Repeat with the account's old password. **Expected**: `401`. + +## Scenario 2 — password strength enforced everywhere + +1. `POST /admin/users` (as admin) with a password shorter than `PASSWORD_MIN_LENGTH`. + **Expected**: `400`, naming the actual minimum length. +2. `POST /auth/password-reset/consume` with a valid token and a too-short new password. + **Expected**: the same `400` rejection reason as step 1. + +## Scenario 3 — login rate limiting + +1. Submit `LOGIN_RATE_LIMIT_MAX_ATTEMPTS` failed login attempts for the same email within + `LOGIN_RATE_LIMIT_WINDOW_SECONDS`. **Expected**: each returns `401` (the existing + identical-failure-response). +2. Submit one more attempt for that same email, still within the window — this time with the + *correct* password. **Expected**: `429`, not `200` — the rate limit is checked before + credentials (FR-007). +3. Submit an attempt for a *different* email within the same window. **Expected**: proceeds + normally (evaluated on its own credentials, not rate-limited). + +## What "done" looks like + +All three scenarios pass against a real Postgres/Redis, and `POST /admin/users`'s own existing +tests (010-identity-auth) still pass with the added password-strength check in place. diff --git a/specs/013-auth-hardening/research.md b/specs/013-auth-hardening/research.md new file mode 100644 index 0000000..27deb7f --- /dev/null +++ b/specs/013-auth-hardening/research.md @@ -0,0 +1,83 @@ +# Research: Authentication Hardening + +## Decision: reset tokens live only in Redis, as a paired key shape, never in Postgres + +- **Decision**: A random 32-byte token (`crypto.randomBytes(32).toString('hex')`) is generated + per request; only its SHA-256 hash is ever stored (the raw token is returned to the caller of + `requestPasswordReset` for the stub-delivery step to log, then discarded). Two Redis keys per + active token, both with the same TTL (the reset lifetime): + - `password-reset:token:` → `userId` (resolves a presented token at consume time) + - `password-reset:user:` → `hash` (lets issuing a new token find and delete the prior + one's `token:` key, invalidating it — FR-002) +- **Rationale**: Storing only the hash (never the raw token) mirrors this codebase's own + password-hashing discipline (010's `hashPassword`) and 002's encrypted-credential-at-rest + precedent — a Redis compromise alone shouldn't hand over usable reset tokens. The paired-key + shape gets "only one active token per account" (FR-002) without a database table or a list + scan; it's the same Redis-key-with-TTL pattern 010's own revocation denylist and 002's jti + replay-guard already established, not a new pattern for this codebase. +- **Alternatives considered**: A signed JWT with a `purpose: 'password-reset'` claim — rejected; + a JWT can't be "invalidated by issuing a new one" without also tracking issued tokens + somewhere (defeating the point of using a stateless token), so it would need the same Redis + bookkeeping anyway while adding JWT-parsing overhead for no benefit. A Postgres table — works, + but adds a migration and a cleanup/expiry job for data Redis's own TTL already expires for + free; rejected as unnecessary durability for a short-lived, non-audit-relevant credential. + +## Decision: the "delivery" stub is a structured log line, not a fake email object + +- **Decision**: `requestPasswordReset` logs `{ event: 'password_reset_requested', userId, + resetUrl }` at `info` level via the existing Pino logger — no new "mock email" abstraction, + no `EmailService` interface to later swap out. +- **Rationale**: Per the user's own explicit choice (stub delivery, not real email), the + simplest honest stub is exactly what a developer needs during this phase: the token, visible + in the same place every other structured log already goes. Building a fake `EmailService` + interface now, before any real provider is chosen, would be speculative abstraction for a + contract nobody has decided yet (which provider, which template). +- **Alternatives considered**: A dedicated `EmailService`/`NotificationService` interface with a + console/log implementation, swapped for a real one later — rejected as premature + infrastructure for a single call site; revisit when a real provider is actually chosen (a + separate, later decision per spec.md Assumptions). + +## Decision: one shared `validatePasswordStrength`, minimum length only, `PASSWORD_MIN_LENGTH`-configured + +- **Decision**: `identity/auth/mapper/password-policy.ts` exports + `validatePasswordStrength(password: string): void`, throwing `ValidationError` naming the + actual requirement (e.g. "Password must be at least N characters.") if `password.length < + env.PASSWORD_MIN_LENGTH`. Called from both `AuthService`'s new `resetPassword` and + `identity/agents`'s existing `UsersService.create`. +- **Rationale**: FR-005 requires one policy enforced identically everywhere a password is set — + a shared function is the only way to guarantee that rather than trusting two call sites to + stay in sync by convention. Minimum length only (no character-class rules) matches current + NIST guidance (length matters far more than forced complexity) and spec.md's own explicit + scope boundary. +- **Alternatives considered**: A zod `.refine()` embedded separately in each schema — rejected; + duplicates the rule text and the minimum-length constant at two call sites, exactly the drift + FR-005 exists to prevent. + +## Decision: login rate-limit reuses the existing `checkRateLimit` helper, keyed by email + +- **Decision**: `AuthService.login` calls + `checkRateLimit(`login:${email}`, env.LOGIN_RATE_LIMIT_MAX_ATTEMPTS, + env.LOGIN_RATE_LIMIT_WINDOW_SECONDS)` as its very first step, before `repo.findByEmail` or + `verifyPassword` — throwing `RateLimitError` (already a distinct error/status from + `AuthenticationError`, per the existing `common/errors`) if exceeded. +- **Rationale**: `checkRateLimit` (`src/infrastructure/cache/rate-limiter.ts`) already exists, + already used by 002's own inbound-request rate limiting, and is exactly the fixed-window + Redis-`INCR` shape this feature needs — reusing it is the literal instruction 010's own + Assumptions gave ("beyond what 002's existing generic rate-limit infrastructure might already + cover"). Keying by the *submitted* email (not a resolved user id) means the limiter runs + identically whether or not the account exists, so it can't itself become a second + account-existence oracle. +- **Alternatives considered**: `@fastify/rate-limit`'s own global plugin (already registered, + 1000 req/min) — insufficient on its own; that's a blunt per-IP-or-global HTTP-level limit, not + a per-account brute-force defense, and 010's own Assumptions already anticipated needing + something more targeted for login specifically. + +## Decision: `POST /admin/users` gets the shared validator via a one-line call-site change + +- **Decision**: `UsersService.create` calls `validatePasswordStrength(body.password)` before + hashing, right alongside its existing duplicate-email check — no schema change, no new route. +- **Rationale**: FR-005's "identically everywhere" requirement includes this pre-existing + 010 endpoint, which today accepts any non-empty string as a password. Minimal, surgical fix + at the one call site that needed it. +- **Alternatives considered**: None — this is the only other password-setting call site in the + codebase (confirmed by searching for every `hashPassword(` call).