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 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Sonnet 5
parent
b016e77b70
commit
52f1fa3db0
@@ -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.
|
||||
@@ -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:<sha256(token)>` | `userId` | `PASSWORD_RESET_TOKEN_LIFETIME_MINUTES` |
|
||||
| `password-reset:user:<userId>` | `sha256(token)` | same |
|
||||
|
||||
**Issuing** (`requestPasswordReset`): if `password-reset:user:<userId>` already has a value,
|
||||
delete `password-reset:token:<that value>` first (invalidating the prior token — FR-002), then
|
||||
set both new keys.
|
||||
|
||||
**Consuming** (`resetPassword`): `GET password-reset:token:<sha256(presented 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:<email>`) 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).
|
||||
@@ -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:<sha256(token)>` → `userId`, and `password-reset:user:<userId>` →
|
||||
`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.*
|
||||
@@ -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.
|
||||
@@ -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:<hash>` → `userId` (resolves a presented token at consume time)
|
||||
- `password-reset:user:<userId>` → `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).
|
||||
Reference in New Issue
Block a user