makiolaj/DiunDashboard
1
0
Fork 0
Code Issues 1 Pull Requests Actions Packages Projects Releases 1 Wiki Activity

docs(01-data-integrity): create phase 1 plans

Browse Source
This commit is contained in:
Jean-Luc Makiola
2026-03-23 20:04:57 +01:00
parent 19d757d060
commit b89e607493
3 changed files with 684 additions and 2 deletions
Download Patch File Download Diff File Expand all files Collapse all files

414
.planning/phases/01-data-integrity/01-02-PLAN.md Normal file
View File

@@ -0,0 +1,414 @@
---
phase: 01-data-integrity
plan: 02
type: execute
wave: 2
depends_on:
- 01-01
files_modified:
- pkg/diunwebhook/diunwebhook.go
- pkg/diunwebhook/diunwebhook_test.go
autonomous: true
requirements:
- DATA-03
- DATA-04
must_haves:
truths:
- "An oversized webhook payload (>1MB) is rejected with HTTP 413, not processed"
- "A failing test setup call (UpdateEvent error, DB error) causes the test run to report FAIL, not pass silently"
- "The full test suite passes with no regressions from Plan 01"
artifacts:
- path: "pkg/diunwebhook/diunwebhook.go"
provides: "maxBodyBytes constant; MaxBytesReader + errors.As pattern in WebhookHandler, TagsHandler POST, TagAssignmentHandler PUT and DELETE"
contains: "maxBodyBytes"
- path: "pkg/diunwebhook/diunwebhook_test.go"
provides: "New tests TestWebhookHandler_OversizedBody, TestTagsHandler_OversizedBody, TestTagAssignmentHandler_OversizedBody; t.Fatalf replacements at 6 call sites"
contains: "TestWebhookHandler_OversizedBody"
key_links:
- from: "WebhookHandler"
to: "http.StatusRequestEntityTooLarge (413)"
via: "r.Body = http.MaxBytesReader(w, r.Body, maxBodyBytes) then errors.As(err, &maxBytesErr)"
pattern: "MaxBytesReader"
- from: "diunwebhook_test.go setup calls"
to: "t.Fatalf"
via: "replace `if err != nil { return }` with `t.Fatalf(...)`"
pattern: "t\\.Fatalf"
---
<objective>
Fix two remaining bugs: unbounded request body reads (DATA-03) and silently swallowed test failures (DATA-04).
Bug 3 (DATA-03): `WebhookHandler`, `TagsHandler` POST branch, and `TagAssignmentHandler` PUT/DELETE branches decode JSON directly from `r.Body` with no size limit. A malicious or buggy DIUN installation could POST a multi-GB payload causing OOM. The fix applies `http.MaxBytesReader` before each decode and returns HTTP 413 when the limit is exceeded.
Bug 4 (DATA-04): Six test call sites use `if err != nil { return }` instead of `t.Fatalf(...)`. When test setup fails (e.g., InitDB fails, UpdateEvent fails), the test silently exits with PASS, hiding the real failure from CI.
These two bugs are fixed in the same plan because they are independent of Plan 01's changes and both small enough to fit comfortably together.
Purpose: Webhook endpoint is safe from OOM attacks; test failures are always visible to the developer and CI.
Output: Updated `diunwebhook.go` with MaxBytesReader in three handlers; updated `diunwebhook_test.go` with t.Fatalf at 6 sites and 3 new 413 tests.
</objective>
<execution_context>
@$HOME/.claude/get-shit-done/workflows/execute-plan.md
@$HOME/.claude/get-shit-done/templates/summary.md
</execution_context>
<context>
@.planning/PROJECT.md
@.planning/ROADMAP.md
@.planning/STATE.md
@.planning/phases/01-data-integrity/01-RESEARCH.md
@.planning/phases/01-data-integrity/01-01-SUMMARY.md
</context>
<tasks>
<task type="auto" tdd="true">
<name>Task 1: Add request body size limits to WebhookHandler, TagsHandler, and TagAssignmentHandler</name>
<files>pkg/diunwebhook/diunwebhook.go</files>
<read_first>
- pkg/diunwebhook/diunwebhook.go — read the entire file before touching it; locate the exact lines for each handler's JSON decode call; the Plan 01 changes (UPSERT, PRAGMA) are already present — do not revert them
</read_first>
<behavior>
- Test (new): POST /webhook with a body of 1MB + 1 byte returns HTTP 413
- Test (new): POST /api/tags with a body of 1MB + 1 byte returns HTTP 413
- Test (new): PUT /api/tag-assignments with a body of 1MB + 1 byte returns HTTP 413
- Test (existing): POST /webhook with valid JSON still returns HTTP 200
- Test (existing): POST /api/tags with valid JSON still returns HTTP 201
</behavior>
<action>
Make the following changes to pkg/diunwebhook/diunwebhook.go:
CHANGE 1 — Add package-level constant after the import block, before the type declarations:
```go
const maxBodyBytes = 1 << 20 // 1 MB
```
CHANGE 2 — Add `"errors"` to the import block (it is not currently imported in diunwebhook.go; it is imported in the test file but not the production file).
The import block becomes:
```go
import (
"crypto/subtle"
"database/sql"
"encoding/json"
"errors"
"log"
"net/http"
"strconv"
"strings"
"sync"
"time"
_ "modernc.org/sqlite"
)
```
CHANGE 3 — In WebhookHandler, BEFORE the `var event DiunEvent` line (currently line ~177), add:
```go
r.Body = http.MaxBytesReader(w, r.Body, maxBodyBytes)
```
Then update the decode error handling block to distinguish 413 from 400:
```go
if err := json.NewDecoder(r.Body).Decode(&event); err != nil {
var maxBytesErr *http.MaxBytesError
if errors.As(err, &maxBytesErr) {
http.Error(w, "request body too large", http.StatusRequestEntityTooLarge)
return
}
log.Printf("WebhookHandler: failed to decode request: %v", err)
http.Error(w, "bad request", http.StatusBadRequest)
return
}
```
CHANGE 4 — In TagsHandler POST branch, BEFORE `var req struct { Name string }`, add:
```go
r.Body = http.MaxBytesReader(w, r.Body, maxBodyBytes)
```
Then update the decode error handling — the current code is:
```go
if err := json.NewDecoder(r.Body).Decode(&req); err != nil || req.Name == "" {
http.Error(w, "bad request: name required", http.StatusBadRequest)
return
}
```
Replace with:
```go
if err := json.NewDecoder(r.Body).Decode(&req); err != nil {
var maxBytesErr *http.MaxBytesError
if errors.As(err, &maxBytesErr) {
http.Error(w, "request body too large", http.StatusRequestEntityTooLarge)
return
}
http.Error(w, "bad request: name required", http.StatusBadRequest)
return
}
if req.Name == "" {
http.Error(w, "bad request: name required", http.StatusBadRequest)
return
}
```
(The `req.Name == ""` check must remain, now as a separate if-block after the decode succeeds.)
CHANGE 5 — In TagAssignmentHandler PUT branch, BEFORE `var req struct { Image string; TagID int }`, add:
```go
r.Body = http.MaxBytesReader(w, r.Body, maxBodyBytes)
```
Then update the decode error handling — the current code is:
```go
if err := json.NewDecoder(r.Body).Decode(&req); err != nil || req.Image == "" {
http.Error(w, "bad request", http.StatusBadRequest)
return
}
```
Replace with:
```go
if err := json.NewDecoder(r.Body).Decode(&req); err != nil {
var maxBytesErr *http.MaxBytesError
if errors.As(err, &maxBytesErr) {
http.Error(w, "request body too large", http.StatusRequestEntityTooLarge)
return
}
http.Error(w, "bad request", http.StatusBadRequest)
return
}
if req.Image == "" {
http.Error(w, "bad request", http.StatusBadRequest)
return
}
```
CHANGE 6 — In TagAssignmentHandler DELETE branch, BEFORE `var req struct { Image string }`, add:
```go
r.Body = http.MaxBytesReader(w, r.Body, maxBodyBytes)
```
Then update the decode error handling — the current code is:
```go
if err := json.NewDecoder(r.Body).Decode(&req); err != nil || req.Image == "" {
http.Error(w, "bad request", http.StatusBadRequest)
return
}
```
Replace with:
```go
if err := json.NewDecoder(r.Body).Decode(&req); err != nil {
var maxBytesErr *http.MaxBytesError
if errors.As(err, &maxBytesErr) {
http.Error(w, "request body too large", http.StatusRequestEntityTooLarge)
return
}
http.Error(w, "bad request", http.StatusBadRequest)
return
}
if req.Image == "" {
http.Error(w, "bad request", http.StatusBadRequest)
return
}
```
No other changes to diunwebhook.go in this task.
</action>
<verify>
<automated>cd /home/jean-luc-makiola/Development/projects/DiunDashboard && go build ./pkg/diunwebhook/ && go test -v -run "TestWebhookHandler_BadRequest|TestCreateTagHandler_EmptyName|TestTagAssignmentHandler_Assign" ./pkg/diunwebhook/</automated>
</verify>
<done>
- `grep -c "maxBodyBytes" pkg/diunwebhook/diunwebhook.go` outputs `5` (1 constant definition + 4 MaxBytesReader calls)
- `grep -c "MaxBytesReader" pkg/diunwebhook/diunwebhook.go` outputs `4`
- `grep -c "errors.As" pkg/diunwebhook/diunwebhook.go` outputs `4`
- `go build ./pkg/diunwebhook/` exits 0
- All pre-existing handler tests still pass
</done>
</task>
<task type="auto">
<name>Task 2: Replace silent returns with t.Fatalf at 6 test setup call sites; add 3 oversized-body tests</name>
<files>pkg/diunwebhook/diunwebhook_test.go</files>
<read_first>
- pkg/diunwebhook/diunwebhook_test.go — read the entire file; locate the exact 6 `if err != nil { return }` call sites at lines 38-40, 153-154, 228-231, 287-289, 329-331, 350-351 and verify they still exist after Plan 01 (which only appended to the file)
</read_first>
<action>
CHANGE 1 — Replace the 6 silent-return call sites with t.Fatalf. Each replacement follows this pattern:
OLD (line ~38-40, in TestUpdateEventAndGetUpdates):
```go
err := diun.UpdateEvent(event)
if err != nil {
return
}
```
NEW:
```go
if err := diun.UpdateEvent(event); err != nil {
t.Fatalf("test setup: UpdateEvent failed: %v", err)
}
```
OLD (line ~153-154, in TestUpdatesHandler):
```go
err := diun.UpdateEvent(event)
if err != nil {
return
}
```
NEW:
```go
if err := diun.UpdateEvent(event); err != nil {
t.Fatalf("test setup: UpdateEvent failed: %v", err)
}
```
OLD (line ~228-231, in TestConcurrentUpdateEvent goroutine):
```go
err := diun.UpdateEvent(diun.DiunEvent{Image: fmt.Sprintf("image:%d", i)})
if err != nil {
return
}
```
NEW (note: in a goroutine, t.Fatalf is safe — testing.T.Fatalf calls runtime.Goexit which unwinds the goroutine cleanly):
```go
if err := diun.UpdateEvent(diun.DiunEvent{Image: fmt.Sprintf("image:%d", i)}); err != nil {
t.Fatalf("test setup: UpdateEvent[%d] failed: %v", i, err)
}
```
OLD (line ~287-289, in TestDismissHandler_Success):
```go
err := diun.UpdateEvent(diun.DiunEvent{Image: "nginx:latest"})
if err != nil {
return
}
```
NEW:
```go
if err := diun.UpdateEvent(diun.DiunEvent{Image: "nginx:latest"}); err != nil {
t.Fatalf("test setup: UpdateEvent failed: %v", err)
}
```
OLD (line ~329-331, in TestDismissHandler_SlashInImageName):
```go
err := diun.UpdateEvent(diun.DiunEvent{Image: "ghcr.io/user/image:tag"})
if err != nil {
return
}
```
NEW:
```go
if err := diun.UpdateEvent(diun.DiunEvent{Image: "ghcr.io/user/image:tag"}); err != nil {
t.Fatalf("test setup: UpdateEvent failed: %v", err)
}
```
OLD (line ~350-351, in TestDismissHandler_ReappearsAfterNewWebhook — note: line 350 is `diun.UpdateEvent(diun.DiunEvent{Image: "nginx:latest"})` with no error check at all):
```go
diun.UpdateEvent(diun.DiunEvent{Image: "nginx:latest"})
```
NEW:
```go
if err := diun.UpdateEvent(diun.DiunEvent{Image: "nginx:latest"}); err != nil {
t.Fatalf("test setup: UpdateEvent failed: %v", err)
}
```
CHANGE 2 — Add three new test functions after all existing tests (at the end of the file, after TestUpdateEvent_PreservesTagOnUpsert which was added in Plan 01):
```go
func TestWebhookHandler_OversizedBody(t *testing.T) {
// Generate a body that exceeds 1 MB (maxBodyBytes = 1<<20 = 1,048,576 bytes)
oversized := make([]byte, 1<<20+1)
for i := range oversized {
oversized[i] = 'x'
}
req := httptest.NewRequest(http.MethodPost, "/webhook", bytes.NewReader(oversized))
rec := httptest.NewRecorder()
diun.WebhookHandler(rec, req)
if rec.Code != http.StatusRequestEntityTooLarge {
t.Errorf("expected 413 for oversized body, got %d", rec.Code)
}
}
func TestTagsHandler_OversizedBody(t *testing.T) {
oversized := make([]byte, 1<<20+1)
for i := range oversized {
oversized[i] = 'x'
}
req := httptest.NewRequest(http.MethodPost, "/api/tags", bytes.NewReader(oversized))
rec := httptest.NewRecorder()
diun.TagsHandler(rec, req)
if rec.Code != http.StatusRequestEntityTooLarge {
t.Errorf("expected 413 for oversized body, got %d", rec.Code)
}
}
func TestTagAssignmentHandler_OversizedBody(t *testing.T) {
oversized := make([]byte, 1<<20+1)
for i := range oversized {
oversized[i] = 'x'
}
req := httptest.NewRequest(http.MethodPut, "/api/tag-assignments", bytes.NewReader(oversized))
rec := httptest.NewRecorder()
diun.TagAssignmentHandler(rec, req)
if rec.Code != http.StatusRequestEntityTooLarge {
t.Errorf("expected 413 for oversized body, got %d", rec.Code)
}
}
```
No new imports are needed — `bytes`, `net/http`, `net/http/httptest`, and `testing` are already imported.
</action>
<verify>
<automated>cd /home/jean-luc-makiola/Development/projects/DiunDashboard && go test -v -run "TestWebhookHandler_OversizedBody|TestTagsHandler_OversizedBody|TestTagAssignmentHandler_OversizedBody" ./pkg/diunwebhook/</automated>
</verify>
<done>
- `grep -c "if err != nil {" pkg/diunwebhook/diunwebhook_test.go` is reduced by 6 compared to before this task (the 6 setup-path returns are gone; other `if err != nil {` blocks with t.Fatal/t.Fatalf remain)
- `grep -c "return$" pkg/diunwebhook/diunwebhook_test.go` no longer contains bare `return` in error-check positions (the 6 silent returns are gone)
- TestWebhookHandler_OversizedBody passes (413)
- TestTagsHandler_OversizedBody passes (413)
- TestTagAssignmentHandler_OversizedBody passes (413)
- Full test suite passes: `go test ./pkg/diunwebhook/` exits 0
</done>
</task>
</tasks>
<verification>
Run the full test suite after both tasks are complete:
```bash
cd /home/jean-luc-makiola/Development/projects/DiunDashboard && go test -v -coverprofile=coverage.out -coverpkg=./... ./...
```
Expected outcome:
- All tests pass (no regressions from Plan 01 or Plan 02)
- Three new 413 tests pass (proves DATA-03)
- Six `if err != nil { return }` patterns replaced with t.Fatalf (proves DATA-04)
Spot-check the fixes:
```bash
grep -n "maxBodyBytes\|MaxBytesReader\|errors.As" pkg/diunwebhook/diunwebhook.go
grep -n "t.Fatalf" pkg/diunwebhook/diunwebhook_test.go | wc -l # should be >= 6 more than before
```
</verification>
<success_criteria>
- `grep -c "MaxBytesReader" pkg/diunwebhook/diunwebhook.go` outputs `4`
- `grep -c "maxBodyBytes" pkg/diunwebhook/diunwebhook.go` outputs `5`
- `grep -c "StatusRequestEntityTooLarge" pkg/diunwebhook/diunwebhook.go` outputs `4`
- TestWebhookHandler_OversizedBody, TestTagsHandler_OversizedBody, TestTagAssignmentHandler_OversizedBody all exist and pass
- `grep -c "if err != nil {$" pkg/diunwebhook/diunwebhook_test.go` followed by `return` no longer appears at the 6 original sites
- `go test -coverprofile=coverage.out -coverpkg=./... ./...` exits 0
</success_criteria>
<output>
After completion, create `.planning/phases/01-data-integrity/01-02-SUMMARY.md` following the summary template at `@$HOME/.claude/get-shit-done/templates/summary.md`.
</output>