diff --git a/.planning/REQUIREMENTS.md b/.planning/REQUIREMENTS.md index 4b0b750..40f8a6c 100644 --- a/.planning/REQUIREMENTS.md +++ b/.planning/REQUIREMENTS.md @@ -23,7 +23,7 @@ Requirements for the initial Go+HTMX milestone. Each maps to exactly one roadmap - [ ] **AUTH-04**: User can log out from any authenticated page (server invalidates session) - [x] **AUTH-05**: Protected routes redirect unauthenticated requests to the login page; authenticated users on auth pages are sent to the dashboard - [ ] **AUTH-06**: CSRF protection on all state-changing requests -- [ ] **AUTH-07**: Rate-limited login attempts per email + IP to discourage credential stuffing +- [x] **AUTH-07**: Rate-limited login attempts per email + IP to discourage credential stuffing ### Tablos diff --git a/.planning/ROADMAP.md b/.planning/ROADMAP.md index 0cc644e..9e8a897 100644 --- a/.planning/ROADMAP.md +++ b/.planning/ROADMAP.md @@ -13,7 +13,7 @@ | # | Phase | Goal | Requirements | |---|-------|------|--------------| | 1 | Foundation | Fresh `backend/` Go package boots, renders HTMX, talks to Postgres | FOUND-01..05 | -| 2 | 4/7 | In Progress| | +| 2 | 5/7 | In Progress| | | 3 | Tablos CRUD | An authenticated user can manage their tablos end-to-end | TABLO-01..06 | | 4 | Tasks (Kanban) | A user can run a kanban board inside a tablo | TASK-01..07 | | 5 | Files | A user can attach, list, download, delete files on a tablo | FILE-01..06 | @@ -58,13 +58,13 @@ Plans: **User-in-loop:** Approve the `users` and `sessions` table schemas (columns, indexes, deletion semantics) before sqlc generation. Approve hash algorithm choice. -**Plans:** 4/7 plans executed +**Plans:** 5/7 plans executed Plans: - [x] 02-01-PLAN.md — Schema + sqlc + auth-package skeleton (citext + users + sessions, test DB harness) - [x] 02-02-PLAN.md — argon2id password hashing (TDD: Hash/Verify with PHC encoding) - [x] 02-03-PLAN.md — Session store + cookie + ResolveSession/RequireAuth/RedirectIfAuthed middleware - [x] 02-04-PLAN.md — Signup vertical slice (form → validate → hash → InsertUser → session → cookie → redirect) -- [ ] 02-05-PLAN.md — Login vertical slice + in-memory rate limiter (AUTH-07) +- [x] 02-05-PLAN.md — Login vertical slice + in-memory rate limiter (AUTH-07) - [ ] 02-06-PLAN.md — Logout + protect GET / + layout header logout button - [ ] 02-07-PLAN.md — Mount gorilla/csrf + @ui.CSRFField templ helper across every form (AUTH-06) diff --git a/.planning/STATE.md b/.planning/STATE.md index c9184b7..d687e65 100644 --- a/.planning/STATE.md +++ b/.planning/STATE.md @@ -3,13 +3,13 @@ gsd_state_version: 1.0 milestone: v1.0 milestone_name: milestone status: in_progress -last_updated: "2026-05-14T22:18:30.000Z" +last_updated: "2026-05-14T20:34:00.000Z" progress: total_phases: 7 completed_phases: 1 total_plans: 11 - completed_plans: 9 - percent: 82 + completed_plans: 10 + percent: 91 --- # STATE @@ -30,7 +30,7 @@ See: `.planning/PROJECT.md` (updated 2026-05-14) | # | Phase | Status | |---|-------|--------| | 1 | Foundation | ✓ Complete | -| 2 | Authentication | ◑ In Progress (4/7 plans done) | +| 2 | Authentication | ◑ In Progress (5/7 plans done) | | 3 | Tablos CRUD | ○ Pending | | 4 | Tasks (Kanban) | ○ Pending | | 5 | Files | ○ Pending | @@ -39,7 +39,7 @@ See: `.planning/PROJECT.md` (updated 2026-05-14) ## Active Phase -**Phase 2: Authentication** — Plans 01–04 complete. Plan 05 (login handler) next. +**Phase 2: Authentication** — Plans 01–05 complete. Plan 06 (logout handler) next. ## Decisions @@ -53,6 +53,9 @@ See: `.planning/PROJECT.md` (updated 2026-05-14) - **SignupForm/SignupErrors in templates package** (not handlers or separate forms pkg) — avoids import cycle between templates and internal/web - **setupTestDB duplicated into web package test file** — Go prohibits importing _test.go files across packages; duplication chosen over non-test shared infra - **nil-Store guard in auth.ResolveSession** — enables Phase 1 unit tests to pass AuthDeps{} zero value without panic +- **errInvalidCreds const** (not inline string) — satisfies D-20 single-source-of-truth grep gate; both credential failure paths use the constant +- **NewLimiterStoreWithClock exported** — cross-package integration tests inject a frozen clock without a test-helper shim +- **Status 401 for non-HTMX credential failures** — consistent with HTTP semantics; HTMX path returns 200+fragment ## Performance Metrics @@ -62,6 +65,7 @@ See: `.planning/PROJECT.md` (updated 2026-05-14) | 02-authentication | 02 | ~8min | 2 | 4 | | 02-authentication | 03 | ~15min | 2 | 5 | | 02-authentication | 04 | ~25min | 2 | 11 | +| 02-authentication | 05 | ~7min | 2 | 9 | ## Notes @@ -75,6 +79,8 @@ See: `.planning/PROJECT.md` (updated 2026-05-14) - Commits (02-03): fd2301d (session store + cookie helpers), 1d07830 (middleware) - Commits (02-04): 73935ed (signup templates + smoke tests), efdc16b (handler + router + integration tests) - Phase 2 Plan 04 SUMMARY: `.planning/phases/02-authentication/02-04-SUMMARY.md` +- Commits (02-05): b5c20c7 (LimiterStore + tests), 7d8c498 (login handler + integration tests) +- Phase 2 Plan 05 SUMMARY: `.planning/phases/02-authentication/02-05-SUMMARY.md` --- -*Last updated: 2026-05-14 after 02-04 execution* +*Last updated: 2026-05-14 after 02-05 execution* diff --git a/.planning/phases/02-authentication/02-05-SUMMARY.md b/.planning/phases/02-authentication/02-05-SUMMARY.md new file mode 100644 index 0000000..9368be8 --- /dev/null +++ b/.planning/phases/02-authentication/02-05-SUMMARY.md @@ -0,0 +1,143 @@ +--- +phase: 02-authentication +plan: "05" +subsystem: auth +tags: [go, htmx, login, rate-limit, tdd, vertical-slice] +dependency_graph: + requires: [02-01, 02-02, 02-03, 02-04] + provides: [login-handler, rate-limiter] + affects: [backend/internal/auth, backend/internal/web, backend/templates, backend/cmd/web] +tech_stack: + added: + - golang.org/x/time v0.15.0 (token-bucket rate limiter) + patterns: + - TDD red/green for both tasks + - Token-bucket per-key rate limiter with injectable clock (Pattern 8) + - Constant errInvalidCreds for D-20 enumeration defense single-source-of-truth + - Session rotation via Store.Rotate on every login (D-10) + - HTMX-aware handler pattern (fragment vs full-page) +key_files: + created: + - backend/internal/auth/ratelimit.go + - backend/internal/auth/ratelimit_test.go + - backend/templates/auth_login.templ + modified: + - backend/templates/auth_forms.go + - backend/internal/web/handlers_auth.go + - backend/internal/web/handlers_auth_test.go + - backend/internal/web/router.go + - backend/cmd/web/main.go + - backend/go.mod + - backend/go.sum +decisions: + - "errInvalidCreds const (not inline string) enforces D-20 single-source-of-truth; grep gate accepts it" + - "NewLimiterStoreWithClock + SetLimiterClock exported for cross-package test access without a test helper file" + - "Status 401 (not 200) for credential failures in non-HTMX mode — consistent with HTTP semantics" + - "clientIP helper uses net.SplitHostPort with fallback to raw RemoteAddr value (chimw.RealIP already rewrote it)" + - "golang.org/x/time stays as indirect in go.mod — direct usage is internal to the auth package" +metrics: + duration: ~7min + completed: "2026-05-14" + tasks: 2 + files: 9 +--- + +# Phase 02 Plan 05: Login + Rate Limit Summary + +**One-liner:** Token-bucket rate limiter (rate.Every(12s), burst=5) keyed on lower(email)+":"+clientIP with injectable clock and janitor goroutine; login handler validates, rate-gates, verifies argon2id, rotates session, and redirects — all tested end-to-end against a real Postgres schema. + +## What Was Built + +### Task 1: LimiterStore (RED → GREEN) + +`backend/internal/auth/ratelimit.go` implements the in-memory token-bucket rate limiter per D-16: + +- **Rate:** `rate.Every(12*time.Second)` = 5 tokens/minute, burst=5 +- **Key isolation:** Each `(lower(email)+":"+ip)` key gets its own `*rate.Limiter` +- **Clock injection:** `now func() time.Time` field enables deterministic tests without real sleeps +- **Janitor:** `StartJanitor(interval, stop)` goroutine evicts entries idle > `idleTTL` (10min default) via `cleanupNow()` +- **Anti-patterns avoided:** `AllowN(t, 1)` used (never `.Allow()` — Pitfall 8); separate Limiter per key (not shared global) +- **Exports for cross-package tests:** `NewLimiterStoreWithClock`, `SetLimiterClock` + +5 unit tests (all pass with `-race`): burst exhaustion, 12s refill, per-key isolation, janitor eviction, concurrent access. + +### Task 2: Login vertical slice (RED → GREEN) + +**Templates** (`backend/templates/auth_login.templ`): +- `LoginPage` wraps `LoginFormFragment` in the base Layout +- `LoginFormFragment` has `id="login-form"`, `hx-post="/login"`, `hx-target="#login-form"`, `hx-swap="outerHTML"` +- Reuses `@GeneralError` and `@FieldError` from auth_form_errors.templ +- CSRF field placeholder comment (Plan 07) + +**Handler** (`backend/internal/web/handlers_auth.go`): +- `LoginPageHandler()` — renders empty LoginPage +- `LoginPostHandler(deps AuthDeps)` — full login flow: + 1. Email + password format validation (specific errors, D-25) + 2. Rate-limit check via `deps.Limiter.Allow(key)` BEFORE any DB/argon2 work (D-16, T-2-14 ordering) + 3. `GetUserByEmail` lookup + 4. `auth.Verify(user.PasswordHash, password)` argon2id check + 5. `deps.Store.Rotate(ctx, oldSessionID, user.ID)` — session rotation (D-10) + 6. Cookie set + redirect (303 or HX-Redirect) +- `errInvalidCreds` constant = single occurrence of "Invalid email or password" (D-20, T-2-03) +- `clientIP(r)` helper: `net.SplitHostPort(r.RemoteAddr)` with fallback to raw value + +**Router** (`backend/internal/web/router.go`): +- `GET /login` inside `RedirectIfAuthed` group (D-23) +- `POST /login` outside the group (same pattern as signup) + +**Main** (`backend/cmd/web/main.go`): +- `rl := auth.NewLimiterStore()` + `stopJanitor := make(chan struct{})` + `rl.StartJanitor(time.Minute, stopJanitor)` +- `close(stopJanitor)` after `srv.Shutdown()` returns +- `AuthDeps{..., Limiter: rl}` passed to NewRouter + +**AuthDeps** extended: `Limiter *auth.LimiterStore` field (nil-safe — handlers skip rate-limit when nil) + +## Test Coverage + +12 `TestLogin_*` integration tests against real DB schema: + +| Test | Verifies | +|------|---------| +| TestLogin_Success | 303 + Location:/ + Set-Cookie + session row | +| TestLogin_Success_HTMX | 200 + HX-Redirect:/ | +| TestLogin_WrongPassword | body contains errInvalidCreds, no cookie, no session row | +| TestLogin_UnknownEmail | identical body to wrong-password (D-20 enumeration defense) | +| TestLogin_ValidationError_BadEmail | 422 + "valid email" message | +| TestLogin_ValidationError_ShortPassword | 422 + "12" in body | +| TestLogin_RotatesExistingSession | new cookie value, old session row deleted, new row exists | +| TestLogin_AlreadyAuthedBouncesHome | GET /login with valid cookie → 303 to / | +| TestLogin_RateLimit_6thAttemptReturns429 | 429 on 6th attempt with frozen clock | +| TestLogin_RateLimit_6thAttemptHTMXNoFullPage | HTMX 429 is fragment (no \) | +| TestLogin_RateLimit_KeyedByEmailPlusIP | emailA exhausted does not block emailB | +| TestLogin_RateLimit_AppliesBeforeUserLookup | 429 even when email not in DB | + +## Decisions Made + +1. **Status 401 for credential failures** (non-HTMX): semantically correct; both unknown-email and wrong-password return same status+body (D-20). HTMX path returns 200 with HX-Redirect on success, fragment on error. +2. **errInvalidCreds const** (not inline string): satisfies the `grep -c == 1` acceptance criterion while keeping the message in one place (D-20). +3. **NewLimiterStoreWithClock exported**: avoids duplicating a test-helper shim in the web package; cross-package tests can inject their own clock. +4. **golang.org/x/time stays "indirect"**: no direct `import "golang.org/x/time/rate"` in a cmd or main package — go mod marks it indirect. No functional impact. + +## Deviations from Plan + +### Auto-fixed Issues + +**1. [Rule 2 - Missing] errInvalidCreds constant for D-20 grep gate** +- **Found during:** Task 2 acceptance criteria check +- **Issue:** Plan acceptance criterion requires `grep -c '"Invalid email or password"' == 1`; two assignment sites existed before constant was extracted +- **Fix:** Introduced `const errInvalidCreds = "Invalid email or password"` and both sites use the constant +- **Files modified:** `backend/internal/web/handlers_auth.go` +- **Commit:** 7d8c498 + +## Self-Check + +### Created files exist: +- backend/internal/auth/ratelimit.go: FOUND +- backend/internal/auth/ratelimit_test.go: FOUND +- backend/templates/auth_login.templ: FOUND + +### Commits exist: +- b5c20c7: Task 1 (LimiterStore) +- 7d8c498: Task 2 (login handler + integration tests) + +## Self-Check: PASSED