diff --git a/internal/api/issue68_validation_test.go b/internal/api/issue68_validation_test.go new file mode 100644 index 0000000..9f3e0e7 --- /dev/null +++ b/internal/api/issue68_validation_test.go @@ -0,0 +1,198 @@ +package api + +// Regression tests for #68 input-validation gaps: negative/oversized content +// lengths (413), limit=0 → default page size, unchecked query params +// (negative offset), and negative burn_after_reads. + +import ( + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +func createPasteRaw(t *testing.T, h http.Handler, body string) *httptest.ResponseRecorder { + t.Helper() + req := httptest.NewRequest("POST", "/api/pastes", strings.NewReader(body)) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + return rec +} + +// A request whose decoded content exceeds the admin-tunable cap is rejected +// with 413 and a clear message (content under the body cap, over the +// content cap). +func TestCreatePasteContentOverCap413(t *testing.T) { + s := testServer(t) + h := s.routes() + + // content over MaxContentBytes (5MiB) but under body cap (+4KiB): send + // just over the content cap so the per-field check fires first. + content := strings.Repeat("a", 5*1024*1024+10) + rec := createPasteRaw(t, h, fmt.Sprintf(`{"content":"%s"}`, content)) + if rec.Code != http.StatusRequestEntityTooLarge { + t.Fatalf("got %d want 413: %s", rec.Code, rec.Body.String()) + } + if !strings.Contains(rec.Body.String(), "content exceeds max") { + t.Fatalf("unclear error message: %s", rec.Body.String()) + } +} + +// A request whose entire body is larger than the server-side body cap is cut +// off by http.MaxBytesReader and answered with 413, not decoded into memory +// (#68: previously a giant body was fully buffered, then rejected only at +// the per-field check — actually the decode happened before any check). +func TestCreatePasteBodyOverCap413(t *testing.T) { + s := testServer(t) + // shrink the content cap so the body cap is small too + ss := s.settings.get() + ss.MaxContentBytes = 64 * 1024 + if err := s.settings.set(ss); err != nil { + t.Fatal(err) + } + h := s.routes() + + content := strings.Repeat("a", 200*1024) // 200KiB > 64KiB+4KiB body cap + rec := createPasteRaw(t, h, fmt.Sprintf(`{"content":"%s","title":"x"}`, content)) + if rec.Code != http.StatusRequestEntityTooLarge { + t.Fatalf("got %d want 413: %s", rec.Code, rec.Body.String()) + } +} + +// Negative content lengths cannot be expressed via JSON, but a negative +// expires-style numeric payload must not crash; more importantly the +// burn_after_reads field: negative values are rejected with a clear message. +func TestCreatePasteNegativeBurnAfterReads(t *testing.T) { + s := testServer(t) + h := s.routes() + + rec := createPasteRaw(t, h, `{"content":"hi","burn_after_reads":-5}`) + if rec.Code != http.StatusBadRequest { + t.Fatalf("got %d want 400: %s", rec.Code, rec.Body.String()) + } + if !strings.Contains(rec.Body.String(), "burn_after_reads") { + t.Fatalf("unclear error message: %s", rec.Body.String()) + } + + // 0 and positive values still work (0 = default single read, per #49) + rec = createPasteRaw(t, h, `{"content":"hi","burn_after_reads":0,"burn_after_read":true}`) + if rec.Code != http.StatusCreated { + t.Fatalf("zero burn_after_reads: got %d want 201: %s", rec.Code, rec.Body.String()) + } +} + +// limit=0 on list endpoints returns the default page size (existing clamp +// treats <=0 as default; #68 asks this be explicit and tested). +func TestListLimitZeroUsesDefault(t *testing.T) { + s := testServer(t) + h := s.routes() + + // seed 3 public pastes + for i := 0; i < 3; i++ { + rec := createPasteRaw(t, h, fmt.Sprintf(`{"content":"p%d"}`, i)) + if rec.Code != 201 { + t.Fatalf("seed: got %d: %s", rec.Code, rec.Body.String()) + } + } + + for _, q := range []string{"/api/public?limit=0", "/api/public"} { + req := httptest.NewRequest("GET", q, nil) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + if rec.Code != 200 { + t.Fatalf("%s: got %d", q, rec.Code) + } + var got struct { + Limit int `json:"limit"` + Total int `json:"total"` + Items []struct{ ID string } `json:"items"` + } + json.Unmarshal(rec.Body.Bytes(), &got) + if got.Limit != 25 || len(got.Items) != 3 { + t.Fatalf("%s: limit=%d items=%d, want limit 25 and all 3 items", q, got.Limit, len(got.Items)) + } + } +} + +// Huge limit values are clamped to the max page size (already the behavior; +// regression-tested here per #68 "validate unchecked params"). +func TestListLimitHugeClamped(t *testing.T) { + s := testServer(t) + h := s.routes() + + req := httptest.NewRequest("GET", "/api/public?limit=999999999", nil) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + var got struct { + Limit int `json:"limit"` + } + json.Unmarshal(rec.Body.Bytes(), &got) + if got.Limit != 25 { + t.Fatalf("limit=%d, want clamped to 25", got.Limit) + } +} + +// Negative offset previously passed through unchecked to SQL (harmless in +// SQLite, but invalid); it must be clamped to 0 (#68). +func TestListNegativeOffsetClamped(t *testing.T) { + s := testServer(t) + h := s.routes() + + for i := 0; i < 2; i++ { + rec := createPasteRaw(t, h, fmt.Sprintf(`{"content":"p%d"}`, i)) + if rec.Code != 201 { + t.Fatalf("seed: got %d", rec.Code) + } + } + + req := httptest.NewRequest("GET", "/api/public?offset=-999", nil) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + if rec.Code != 200 { + t.Fatalf("got %d", rec.Code) + } + var got struct { + Offset int `json:"offset"` + Total int `json:"total"` + } + json.Unmarshal(rec.Body.Bytes(), &got) + if got.Offset != 0 || got.Total != 2 { + t.Fatalf("offset=%d total=%d, want offset 0 and total 2", got.Offset, got.Total) + } + + // /api/mine too (needs the viewer cookie) + req = httptest.NewRequest("GET", "/api/mine?offset=-5", nil) + req.AddCookie(&http.Cookie{Name: "vwr", Value: "offclamp"}) + rec = httptest.NewRecorder() + h.ServeHTTP(rec, req) + var mine struct { + Offset int `json:"offset"` + } + json.Unmarshal(rec.Body.Bytes(), &mine) + if mine.Offset != 0 { + t.Fatalf("mine offset=%d, want 0", mine.Offset) + } +} + +// Non-numeric limit/offset fall back to defaults instead of 500s. +func TestListGarbageParams(t *testing.T) { + s := testServer(t) + h := s.routes() + + req := httptest.NewRequest("GET", "/api/public?limit=abc&offset=xyz", nil) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + if rec.Code != 200 { + t.Fatalf("got %d", rec.Code) + } + var got struct { + Limit int `json:"limit"` + Offset int `json:"offset"` + } + json.Unmarshal(rec.Body.Bytes(), &got) + if got.Limit != 25 || got.Offset != 0 { + t.Fatalf("limit=%d offset=%d, want 25/0", got.Limit, got.Offset) + } +} diff --git a/internal/api/server.go b/internal/api/server.go index d720e96..8a5c78b 100644 --- a/internal/api/server.go +++ b/internal/api/server.go @@ -59,6 +59,7 @@ func (a *apiServer) routes() http.Handler { r := chi.NewRouter() r.Use(middleware.Recoverer) r.Use(middleware.Timeout(30 * time.Second)) + r.Use(a.limitRequestBody) // #68: hard server-side body cap -> 413 r.Use(viewerCookieMiddleware) r.Use(web.SecurityHeaders) // #59: CSP + hardening headers on HTML pages @@ -153,16 +154,22 @@ func (a *apiServer) handleCreatePaste(w http.ResponseWriter, r *http.Request) { } var p store.Paste if err := json.NewDecoder(r.Body).Decode(&p); err != nil { + if isBodyTooLarge(err) { // #68: body cut off by MaxBytesReader + writeBodyTooLarge(w) + return + } writeErr(w, 400, "invalid json body") return } - if strings.TrimSpace(p.Content) == "" { - writeErr(w, 400, "content is required") + if status, msg := checkContent(p.Content, s.MaxContentBytes); status != 0 { + writeErr(w, status, msg) return } - if int64(len(p.Content)) > s.MaxContentBytes { // #40: admin-tunable - writeErr(w, 413, fmt.Sprintf("content exceeds max %d bytes", s.MaxContentBytes)) - return + if p.BurnAfterReads != nil { // #68: reject negative read budgets + if err := parseBurnAfterReads(*p.BurnAfterReads); err != nil { + writeErr(w, 400, err.Error()) + return + } } // #40: admin-configurable default expiry if (p.ExpiresIn == nil || *p.ExpiresIn == "") && s.DefaultExpiry != "" { @@ -291,11 +298,8 @@ func (a *apiServer) handleListMine(w http.ResponseWriter, r *http.Request) { writeJSON(w, 200, map[string]any{"total": 0, "items": []any{}}) return } - limit, _ := strconv.Atoi(r.URL.Query().Get("limit")) - if limit <= 0 || limit > 100 { - limit = 50 - } - offset, _ := strconv.Atoi(r.URL.Query().Get("offset")) + limit := parseLimit(r, 50, 100) + offset := parseOffset(r) rows, total, err := a.store.ListMine(vid, limit, offset) if err != nil { writeErr(w, 500, "db error") @@ -314,11 +318,8 @@ func (a *apiServer) handleListMine(w http.ResponseWriter, r *http.Request) { } func (a *apiServer) handleListPublic(w http.ResponseWriter, r *http.Request) { - limit, _ := strconv.Atoi(r.URL.Query().Get("limit")) - if limit <= 0 || limit > 100 { - limit = 25 - } - offset, _ := strconv.Atoi(r.URL.Query().Get("offset")) + limit := parseLimit(r, 25, 100) + offset := parseOffset(r) rows, total, err := a.store.ListPublic(limit, offset) if err != nil { writeErr(w, 500, "db error") diff --git a/internal/api/validate.go b/internal/api/validate.go new file mode 100644 index 0000000..fe2e22d --- /dev/null +++ b/internal/api/validate.go @@ -0,0 +1,92 @@ +package api + +import ( + "errors" + "fmt" + "net/http" + "strconv" + "strings" +) + +// #68 input-validation helpers. Paste/can payloads are size-capped and list +// endpoints get a single place where limit/offset are parsed and clamped. + +// maxRequestBody returns the HTTP body cap for JSON create endpoints: the +// admin-tunable content cap plus headroom for JSON field overhead, floored +// at 64KiB so a tiny admin-configured cap can't break small requests. +func (a *apiServer) maxRequestBody() int64 { + s := a.settings.get() + max := s.MaxContentBytes + 4096 + if max < 64*1024 { + max = 64 * 1024 + } + return max +} + +// limitRequestBody wraps the request body with http.MaxBytesReader so +// oversized payloads are cut off server-side instead of being fully decoded +// into memory before the per-field size check runs (#68). A read over the +// cap surfaces as *http.MaxBytesError, which handlers map to 413. +func (a *apiServer) limitRequestBody(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Body != nil { + r.Body = http.MaxBytesReader(w, r.Body, a.maxRequestBody()) + } + next.ServeHTTP(w, r) + }) +} + +// writeBodyTooLarge emits the 413 response for a body rejected by +// MaxBytesReader. +func writeBodyTooLarge(w http.ResponseWriter) { + writeErr(w, http.StatusRequestEntityTooLarge, "request body too large") +} + +// isBodyTooLarge reports whether err came from http.MaxBytesReader. +func isBodyTooLarge(err error) bool { + var mbe *http.MaxBytesError + return errors.As(err, &mbe) +} + +// checkContent validates paste content: rejects whitespace-only content +// (400) and content over the byte cap (413). Returns (0, "") when valid. +func checkContent(content string, maxBytes int64) (int, string) { + if strings.TrimSpace(content) == "" { + return http.StatusBadRequest, "content is required" + } + if int64(len(content)) > maxBytes { // #40/#68: admin-tunable cap + return http.StatusRequestEntityTooLarge, + fmt.Sprintf("content exceeds max %d bytes", maxBytes) + } + return 0, "" +} + +// parseLimit clamps the ?limit query param: missing/non-numeric/zero/negative +// or over-max values fall back to def. Zero intentionally maps to the default +// page size, matching the pre-existing `<= 0` clamp (#68). +func parseLimit(r *http.Request, def, max int) int { + n, err := strconv.Atoi(r.URL.Query().Get("limit")) + if err != nil || n <= 0 || n > max { + return def + } + return n +} + +// parseOffset clamps the ?offset query param: missing/non-numeric or negative +// values become 0 (#68: negative offsets previously passed through to SQL). +func parseOffset(r *http.Request) int { + n, err := strconv.Atoi(r.URL.Query().Get("offset")) + if err != nil || n < 0 { + return 0 + } + return n +} + +// parseBurnAfterReads validates the burn_after_reads field (#68): negative +// values are rejected; zero/absent mean the default single read. +func parseBurnAfterReads(n int) error { + if n < 0 { + return errors.New("burn_after_reads must be a positive number") + } + return nil +}