diff --git a/.planning/REQUIREMENTS.md b/.planning/REQUIREMENTS.md index 898f38a..f1df2a3 100644 --- a/.planning/REQUIREMENTS.md +++ b/.planning/REQUIREMENTS.md @@ -11,8 +11,8 @@ Requirements for this milestone. Each maps to roadmap phases. - [x] **DATA-01**: Webhook events use proper UPSERT (ON CONFLICT DO UPDATE) instead of INSERT OR REPLACE, preserving tag assignments when an image receives a new event - [x] **DATA-02**: SQLite foreign key enforcement is enabled (PRAGMA foreign_keys = ON) so tag deletion properly cascades to tag assignments -- [ ] **DATA-03**: Webhook and API endpoints enforce request body size limits (e.g., 1MB) to prevent OOM from oversized payloads -- [ ] **DATA-04**: Test error handling uses t.Fatal instead of silent returns, so test failures are never swallowed +- [x] **DATA-03**: Webhook and API endpoints enforce request body size limits (e.g., 1MB) to prevent OOM from oversized payloads +- [x] **DATA-04**: Test error handling uses t.Fatal instead of silent returns, so test failures are never swallowed ### Backend Refactor @@ -93,8 +93,8 @@ Which phases cover which requirements. Updated during roadmap creation. |-------------|-------|--------| | DATA-01 | Phase 1 | Complete | | DATA-02 | Phase 1 | Complete | -| DATA-03 | Phase 1 | Pending | -| DATA-04 | Phase 1 | Pending | +| DATA-03 | Phase 1 | Complete | +| DATA-04 | Phase 1 | Complete | | REFAC-01 | Phase 2 | Pending | | REFAC-02 | Phase 2 | Pending | | REFAC-03 | Phase 2 | Pending | diff --git a/.planning/ROADMAP.md b/.planning/ROADMAP.md index c96c32a..3dce3f3 100644 --- a/.planning/ROADMAP.md +++ b/.planning/ROADMAP.md @@ -32,7 +32,7 @@ Decimal phases appear between their surrounding integers in numeric order. Plans: - [x] 01-01-PLAN.md — Fix INSERT OR REPLACE → UPSERT in UpdateEvent(); enable PRAGMA foreign_keys = ON in InitDB(); add regression test -- [ ] 01-02-PLAN.md — Add http.MaxBytesReader body limits to 3 handlers (413 on oversized); replace 6 silent test returns with t.Fatalf +- [x] 01-02-PLAN.md — Add http.MaxBytesReader body limits to 3 handlers (413 on oversized); replace 6 silent test returns with t.Fatalf ### Phase 2: Backend Refactor **Goal**: The codebase has a clean Store interface and Server struct so the SQLite implementation can be swapped without touching HTTP handlers, enabling parallel test execution and PostgreSQL support diff --git a/.planning/STATE.md b/.planning/STATE.md index 0de3326..c950980 100644 --- a/.planning/STATE.md +++ b/.planning/STATE.md @@ -2,14 +2,14 @@ gsd_state_version: 1.0 milestone: v1.0 milestone_name: milestone -status: Ready to execute -stopped_at: Completed 01-01-PLAN.md (UPSERT + FK enforcement) -last_updated: "2026-03-23T20:16:27.362Z" +status: Phase complete — ready for verification +stopped_at: Completed 01-02-PLAN.md (body size limits + test hardening) +last_updated: "2026-03-23T20:25:50.296Z" progress: total_phases: 4 - completed_phases: 0 + completed_phases: 1 total_plans: 2 - completed_plans: 1 + completed_plans: 2 --- # Project State @@ -47,6 +47,7 @@ Plan: 2 of 2 *Updated after each plan completion* | Phase 01 P01 | 2 | 2 tasks | 2 files | +| Phase 01-data-integrity P02 | 7 | 2 tasks | 2 files | ## Accumulated Context @@ -61,6 +62,8 @@ Recent decisions affecting current work: - `DATABASE_URL` present activates PostgreSQL; absent falls back to SQLite with `DB_PATH` — no separate `DB_DRIVER` variable - [Phase 01]: Use named-column UPSERT (ON CONFLICT DO UPDATE) to preserve tag_assignments child rows on re-insert - [Phase 01]: Enable PRAGMA foreign_keys = ON in InitDB() before DDL to activate ON DELETE CASCADE for tag deletion +- [Phase 01-data-integrity]: Use MaxBytesReader + errors.As(*http.MaxBytesError) per-handler (not middleware) for request body size limiting — consistent with no-middleware architecture +- [Phase 01-data-integrity]: Oversized body tests need valid JSON prefix so decoder reads past 1MB limit; all-x bytes fail at byte 1 before MaxBytesReader triggers ### Pending Todos @@ -73,6 +76,6 @@ None yet. ## Session Continuity -Last session: 2026-03-23T20:16:27.360Z -Stopped at: Completed 01-01-PLAN.md (UPSERT + FK enforcement) +Last session: 2026-03-23T20:25:50.294Z +Stopped at: Completed 01-02-PLAN.md (body size limits + test hardening) Resume file: None diff --git a/.planning/phases/01-data-integrity/01-02-SUMMARY.md b/.planning/phases/01-data-integrity/01-02-SUMMARY.md new file mode 100644 index 0000000..49b8219 --- /dev/null +++ b/.planning/phases/01-data-integrity/01-02-SUMMARY.md @@ -0,0 +1,127 @@ +--- +phase: 01-data-integrity +plan: 02 +subsystem: api +tags: [go, http, security, testing, maxbytesreader, body-size-limit] + +# Dependency graph +requires: + - phase: 01-data-integrity plan 01 + provides: UPSERT fix and FK enforcement already applied; test file structure established + +provides: + - HTTP 413 response for oversized request bodies (>1MB) on WebhookHandler, TagsHandler POST, TagAssignmentHandler PUT/DELETE + - maxBodyBytes constant (1 << 20) and MaxBytesReader + errors.As pattern + - t.Fatalf at all 6 test setup call sites (no more silent test pass-on-setup-failure) + - 3 new oversized-body tests proving DATA-03 fixed + +affects: + - phase-02 (database refactor — handlers are now correct and hardened, test suite is reliable) + - any future handler additions that accept a body (pattern established) + +# Tech tracking +tech-stack: + added: [] + patterns: + - "MaxBytesReader + errors.As(*http.MaxBytesError) pattern for request body size limiting in handlers" + - "JSON-prefix oversized body test: use valid JSON opening so decoder reads past limit before MaxBytesReader triggers" + +key-files: + created: [] + modified: + - pkg/diunwebhook/diunwebhook.go + - pkg/diunwebhook/diunwebhook_test.go + +key-decisions: + - "Use MaxBytesReader wrapping r.Body before each JSON decode; distinguish 413 from 400 via errors.As on *http.MaxBytesError" + - "Oversized body test bodies must use valid JSON prefix (e.g. {\"image\":\") + padding — all-x bodies trigger JSON parse error before MaxBytesReader limit is reached" + +patterns-established: + - "MaxBytesReader body guard pattern: r.Body = http.MaxBytesReader(w, r.Body, maxBodyBytes) before decode, errors.As for 413 vs 400" + - "Test setup errors must use t.Fatalf, never silent return" + +requirements-completed: + - DATA-03 + - DATA-04 + +# Metrics +duration: 7min +completed: 2026-03-23 +--- + +# Phase 01 Plan 02: Body Size Limits and Test Setup Hardening Summary + +**Request body size limits (1MB cap, HTTP 413) added to four handler paths; six silent test-setup returns replaced with t.Fatalf to surface setup failures in CI** + +## Performance + +- **Duration:** 7 min +- **Started:** 2026-03-23T20:17:30Z +- **Completed:** 2026-03-23T20:24:37Z +- **Tasks:** 2 +- **Files modified:** 2 + +## Accomplishments + +- Added `maxBodyBytes` constant and `errors` import to `diunwebhook.go`; applied `http.MaxBytesReader` + `errors.As(*http.MaxBytesError)` guard before JSON decode in WebhookHandler, TagsHandler POST, TagAssignmentHandler PUT and DELETE — returns HTTP 413 on body > 1MB +- Replaced 6 silent `if err != nil { return }` test setup patterns with `t.Fatalf(...)` so failing setup always fails the test, not silently passes +- Added 3 new oversized-body tests (TestWebhookHandler_OversizedBody, TestTagsHandler_OversizedBody, TestTagAssignmentHandler_OversizedBody); all pass with 413 + +## Task Commits + +Each task was committed atomically: + +1. **RED: add failing 413 tests** - `311e91d` (test) +2. **Task 1: MaxBytesReader in handlers + GREEN test fix** - `98dfd76` (feat) +3. **Task 2: Replace silent returns with t.Fatalf** - `7bdfc5f` (fix) + +**Plan metadata:** (docs commit — see below) + +_Note: TDD task 1 has a RED commit followed by a combined feat commit covering the implementation and the test body correction._ + +## Files Created/Modified + +- `pkg/diunwebhook/diunwebhook.go` — added `errors` import, `maxBodyBytes` constant, MaxBytesReader guards in 4 handler paths +- `pkg/diunwebhook/diunwebhook_test.go` — 3 new oversized-body tests; 6 t.Fatalf replacements + +## Decisions Made + +- `http.MaxBytesReader` is applied per-handler (not via middleware) to match the existing no-middleware architecture +- Body limit set at 1MB (`1 << 20`) matching the plan spec +- Oversized body test bodies use a valid JSON prefix (`{"image":"` + padding) rather than all-`x` bytes — the JSON decoder reads only 1 byte of invalid content before failing, so all-`x` never triggers MaxBytesReader; a JSON string value causes the decoder to read the full field before the limit fires + +## Deviations from Plan + +### Auto-fixed Issues + +**1. [Rule 1 - Bug] Oversized body tests used all-x bytes; fixed to use valid JSON prefix** +- **Found during:** Task 1 GREEN phase +- **Issue:** Test body `make([]byte, 1<<20+1)` filled with `'x'` causes JSON decoder to fail at byte 1 with "invalid character" — MaxBytesReader never triggers because the read count never reaches the limit +- **Fix:** Changed test bodies to `{"image":"` (or `{"name":"`) + `bytes.Repeat([]byte("x"), 1<<20+1)` so the decoder reads past 1MB before encountering an unterminated string +- **Files modified:** pkg/diunwebhook/diunwebhook_test.go +- **Verification:** All 3 oversized-body tests now pass with HTTP 413 +- **Committed in:** 98dfd76 (Task 1 feat commit) + +--- + +**Total deviations:** 1 auto-fixed (Rule 1 - test bug) +**Impact on plan:** The fix is necessary for tests to validate what they claim. No scope creep; the handler implementation is exactly as specified. + +## Issues Encountered + +None beyond the test body deviation documented above. + +## User Setup Required + +None - no external service configuration required. + +## Next Phase Readiness + +- DATA-03 and DATA-04 fixed; all Phase 01 plans complete +- Full test suite passes with 0 failures +- Handler hardening pattern (MaxBytesReader + errors.As) established for future handlers +- Ready to transition to Phase 02 (database refactor / PostgreSQL support) + +--- +*Phase: 01-data-integrity* +*Completed: 2026-03-23*