415 lines
16 KiB
Markdown
415 lines
16 KiB
Markdown
---
|
|
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>
|