From 83831acb66debcb19e11be015f56e77533bb2a34 Mon Sep 17 00:00:00 2001 From: xarmian Date: Fri, 28 Aug 2026 18:50:55 +0000 Subject: [PATCH] test(server): the three remaining round-13 gaps (BUG-2803) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 13's other three findings, all of the same shape: a test that would stay green with the production change reverted. 1. THE MANIFEST CHECK WAS UNTESTED. The bundle test built archives containing only pad-export.json, so disabling the INDEPENDENT attachment- manifest check left the suite green. The new test builds a bundle with both entries, differing only in the manifest, so a refusal cannot come from the export half. Verified by disabling each check separately: only the matching test fails, so the two are independently covered. 2. THE TEST-EMAIL CHANGE HAD NO HANDLER-LEVEL TEST. Every existing leg exercised decodeJSON, so reverting handlers_admin.go to default EVERY decode failure to the admin's own address passed them all. The new test drives the real endpoint with a wired mock sender and pins the distinction that used to collapse: an ABSENT body still means "send it to me" (control), an ordinary body still sends (control), and a body that is present and refused answers 400 rather than being reinterpreted as the default recipient. 3. THE MULTIPART LEG CHECKED ONE BYTE CLASS. A filter rejecting NULs while letting malformed UTF-8 through would have passed it. It now drives both, which matters because invalid UTF-8 is the class that reaches Postgres as 22021 on a UTF8 database. Round 13 was asked whether the new TESTS are sound — deterministic, order-independent, and failing on broken code. It reported the fixtures isolated and found five ways they were not discriminating. Two were fixed in the previous commit; these are the rest. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN --- internal/server/decode_json_nul_test.go | 47 +++++++++++++ internal/server/handlers_attachments_test.go | 11 +++ .../server/handlers_import_bundle_test.go | 68 +++++++++++++++++++ 3 files changed, 126 insertions(+) diff --git a/internal/server/decode_json_nul_test.go b/internal/server/decode_json_nul_test.go index 1d5cb04c1..18be3ba6f 100644 --- a/internal/server/decode_json_nul_test.go +++ b/internal/server/decode_json_nul_test.go @@ -850,3 +850,50 @@ func TestArtifactBindableTextAcceptsLiteralEscapeText(t *testing.T) { } } } + +// TestTestEmailRefusesUnusableBody is the handler-level leg codex round 13 +// found missing. Reverting handlers_admin.go to `err != nil || input.To == ""` +// — which defaulted EVERY decode failure to the admin's own address — passed +// every other test in this file, because they only exercise decodeJSON. +// +// The distinction being pinned is between two things that used to collapse +// into one: an ABSENT body legitimately means "send it to me", while a body +// that is present and REFUSED must not be reinterpreted as that default. +func TestTestEmailRefusesUnusableBody(t *testing.T) { + srv := testServer(t) + token := bootstrapFirstUser(t, srv, "admin-mail@example.com", "Admin") + mock := mockMailerooEndpoint(t, http.StatusOK, true) + configureEmailForTest(srv, mock.URL, "http://localhost:7777") + + post := func(t *testing.T, body string) *httptest.ResponseRecorder { + t.Helper() + var r io.Reader + if body != "" { + r = strings.NewReader(body) + } + req := httptest.NewRequest("POST", "/api/v1/admin/test-email", r) + if body != "" { + req.Header.Set("Content-Type", "application/json") + } + req.Header.Set("Authorization", "Bearer "+token) + req.RemoteAddr = "127.0.0.1:1234" + rr := httptest.NewRecorder() + srv.ServeHTTP(rr, req) + return rr + } + + // Control 1: no body at all still means "send it to me". + if rr := post(t, ""); rr.Code != http.StatusOK { + t.Fatalf("absent body must still send, got %d: %s", rr.Code, rr.Body.String()) + } + // Control 2: an ordinary body still works. + if rr := post(t, `{"to":"someone@example.com"}`); rr.Code != http.StatusOK { + t.Fatalf("ordinary body must send, got %d: %s", rr.Code, rr.Body.String()) + } + // The case: a body that is valid JSON but carries an unusable value. + rr := post(t, `{"to":"a`+escNULLiteral+`b@example.com"}`) + if rr.Code != http.StatusBadRequest { + t.Errorf("a refused body must answer 400, not be reinterpreted as the default recipient; got %d: %s", + rr.Code, rr.Body.String()) + } +} diff --git a/internal/server/handlers_attachments_test.go b/internal/server/handlers_attachments_test.go index c59aedfa3..1078f584a 100644 --- a/internal/server/handlers_attachments_test.go +++ b/internal/server/handlers_attachments_test.go @@ -992,6 +992,17 @@ func TestUpload_MultipartTextFieldsAreBindableText(t *testing.T) { if control.Code != http.StatusCreated && control.Code != http.StatusOK { t.Fatalf("control (no item_id) must succeed, got %d: %s", control.Code, control.Body.String()) } + // Both byte classes, not just the NUL: a filter that rejected NULs + // while letting malformed UTF-8 through would have passed the + // earlier version of this leg (codex round 13). Invalid UTF-8 is the + // class that reaches Postgres as 22021 on a UTF8 database. + for _, bad := range []string{"TASK-1\x00", "TASK-1\xff"} { + rr := upload(t, "clean.png", &bad) + if rr.Code != control.Code { + t.Errorf("an unusable item_id %q must be treated as no value (like the control, %d), got %d: %s", + bad, control.Code, rr.Code, rr.Body.String()) + } + } bad := "TASK-1\x00" rr := upload(t, "clean.png", &bad) if rr.Code != control.Code { diff --git a/internal/server/handlers_import_bundle_test.go b/internal/server/handlers_import_bundle_test.go index 6058f0d1d..b5944c10f 100644 --- a/internal/server/handlers_import_bundle_test.go +++ b/internal/server/handlers_import_bundle_test.go @@ -697,3 +697,71 @@ func TestImportBundle_RefusesNULInExport(t *testing.T) { t.Errorf("the 400 should name the cause, got body=%s", got.Body.String()) } } + +// TestImportBundle_RefusesNULInManifest is the leg codex round 13 found +// missing: TestImportBundle_RefusesNULInExport builds bundles containing only +// pad-export.json, so removing the INDEPENDENT manifest check left the suite +// green. The manifest is a second JSON document parsed the same way and needs +// its own coverage. +// +// Both bundles carry a valid export; they differ only in the manifest, so a +// refusal cannot come from the export half. +func TestImportBundle_RefusesNULInManifest(t *testing.T) { + src, srcSlug := testServerWithAttachments(t) + rr := doRequest(src, "GET", "/api/v1/workspaces/"+srcSlug+"/export", nil) + if rr.Code != http.StatusOK { + t.Fatalf("export src: %d %s", rr.Code, rr.Body.String()) + } + exportJSON := rr.Body.Bytes() + + esc := string([]byte{'\\', 'u', '0', '0', '0', '0'}) + cleanManifest := `{"version":1,"entries":[]}` + nulManifest := `{"version":1,"entries":[{"id":"00000000-0000-0000-0000-000000000001",` + + `"filename":"a` + esc + `b.png","mime":"image/png","size_bytes":1,"content_hash":"deadbeef"}]}` + + bundle := func(t *testing.T, manifest string) []byte { + t.Helper() + var buf bytes.Buffer + gzw := gzip.NewWriter(&buf) + tw := tar.NewWriter(gzw) + for _, e := range []struct { + name string + body []byte + }{ + {"pad-export.json", exportJSON}, + {"attachments/manifest.json", []byte(manifest)}, + } { + if err := tw.WriteHeader(&tar.Header{Name: e.name, Mode: 0o644, Size: int64(len(e.body))}); err != nil { + t.Fatalf("write header %s: %v", e.name, err) + } + if _, err := tw.Write(e.body); err != nil { + t.Fatalf("write %s: %v", e.name, err) + } + } + tw.Close() + gzw.Close() + return buf.Bytes() + } + + post := func(t *testing.T, name string, body []byte) *httptest.ResponseRecorder { + t.Helper() + dest, _ := testServerWithAttachments(t) + req := httptest.NewRequest("POST", "/api/v1/workspaces/import?name="+name, bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/gzip") + req.RemoteAddr = "127.0.0.1:1234" + rec := httptest.NewRecorder() + dest.ServeHTTP(rec, req) + return rec + } + + if got := post(t, "CleanManifest", bundle(t, cleanManifest)); got.Code != http.StatusOK && got.Code != http.StatusCreated { + t.Fatalf("control bundle must import, got %d: %s", got.Code, got.Body.String()) + } + got := post(t, "NULManifest", bundle(t, nulManifest)) + if got.Code < 400 { + t.Errorf("a manifest carrying a NUL escape must be refused, got %d: %s", got.Code, got.Body.String()) + } + if !strings.Contains(got.Body.String(), "NUL") { + t.Errorf("the refusal should name the cause, got body=%s", got.Body.String()) + } +}