From 8ee68e9b4e73dcfbc020b1b9332bc4e5dd2f0e2d Mon Sep 17 00:00:00 2001 From: xarmian Date: Fri, 25 Sep 2026 14:22:55 -0400 Subject: [PATCH] test(server): the body-reader accounting asks the type checker, so a request reached through a struct field, context value, alias or embedding is counted (BUG-2820) (#1546) TestEveryRequestBodyReaderIsAccountedFor scanned internal/server with a parser that matched the spelling `http.Request` and names bound to it. A request reached through a struct field, a context value, a type alias, an embedded field, an import alias, a dot import, a var declaration, a call-derived alias, a named result or a range binding was invisible, and accounting was per file with a reader count as a workaround. - collectRequestBodyReads loads the package with go/packages and counts every selector whose resolved object is the SAME object that looking the member up on net/http.Request yields (Body, FormValue, PostFormValue, ParseForm, ParseMultipartForm, MultipartForm, PostForm, MultipartReader, FormFile). Accounting is per call site: file::function::member -> count, with the per-file reasons kept. - A fixture package (testdata/bodyreaders) pins the 12 forms the parser could not see, the 9 its own controls held it to, and 6 non-readers, including the two shadowing cases the parser over-flagged on purpose, which are now exact. - New: TestEveryRequestHandoffIsClassified. A request passed to code outside the module must be classified. fosite's five New*Request calls read the OAuth form body (BUG-2811) and are pinned per site; interface dispatch (http.Handler.ServeHTTP) is named as statically unresolvable. - Measured on this tree: the type checker finds exactly the readers the parser found, file for file. The blind spots were real, but nothing used them. - The parser (scanFileForRequestBodyReads) and its controls (TestBodyReaderScanDiscriminates) are retired; their cases live in the fixture. Claude-Session: https://claude.ai/code/session_01A3rXV7GDLAhBe9mL4V6z2m --- internal/server/body_reader_census_test.go | 508 ++++++++++++++++++ internal/server/decode_json_nul_test.go | 435 --------------- .../server/testdata/bodyreaders/fixture.go | 146 +++++ 3 files changed, 654 insertions(+), 435 deletions(-) create mode 100644 internal/server/body_reader_census_test.go create mode 100644 internal/server/testdata/bodyreaders/fixture.go diff --git a/internal/server/body_reader_census_test.go b/internal/server/body_reader_census_test.go new file mode 100644 index 000000000..6eada51f6 --- /dev/null +++ b/internal/server/body_reader_census_test.go @@ -0,0 +1,508 @@ +package server + +import ( + "go/ast" + "go/types" + "path/filepath" + "sort" + "strings" + "sync" + "testing" + + "golang.org/x/tools/go/packages" +) + +// bodyReaderSelectors are the members of net/http.Request that read the +// request BODY, or expose what parsing it produced. FormValue, PostFormValue, +// ParseForm, ParseMultipartForm and FormFile parse it; MultipartReader streams +// it; PostForm and MultipartForm hold the parsed result. +var bodyReaderSelectors = map[string]bool{ + "Body": true, "FormValue": true, "PostFormValue": true, "ParseForm": true, + "ParseMultipartForm": true, "MultipartForm": true, "PostForm": true, + "MultipartReader": true, "FormFile": true, +} + +var ( + censusLoadMu sync.Mutex + censusLoadCache = map[string]*packages.Package{} +) + +// loadCensusPackage type-checks the one package in dir, once per test binary: +// the reader census and the handoff census ask different questions of the same +// load, and a go/packages load of this package is several seconds. +func loadCensusPackage(t *testing.T, dir string) *packages.Package { + t.Helper() + censusLoadMu.Lock() + defer censusLoadMu.Unlock() + if p, ok := censusLoadCache[dir]; ok { + return p + } + cfg := &packages.Config{ + Mode: packages.NeedName | packages.NeedFiles | packages.NeedSyntax | + packages.NeedTypes | packages.NeedTypesInfo | packages.NeedImports | + packages.NeedDeps | packages.NeedModule, + Dir: dir, + Tests: false, + } + pkgs, err := packages.Load(cfg, ".") + if err != nil { + t.Fatalf("load %s: %v", dir, err) + } + if len(pkgs) != 1 { + t.Fatalf("load %s: want 1 package, got %d", dir, len(pkgs)) + } + if len(pkgs[0].Errors) > 0 { + t.Fatalf("load %s: %v", dir, pkgs[0].Errors) + } + censusLoadCache[dir] = pkgs[0] + return pkgs[0] +} + +// bodyReadSite is one selector expression that reaches a request body. +type bodyReadSite struct { + file string // base name + fn string // enclosing top-level function, "Recv.Method" for methods + sel string // the member selected + line int +} + +// key is the unit the accounting table is written in: one function's use of +// one member. Line numbers are not part of it, so an unrelated edit above a +// reader does not churn the table; a SECOND use of the same member in the +// same function changes that key's count instead. +func (s bodyReadSite) key() string { return s.file + "::" + s.fn + "::" + s.sel } + +var ( + bodyCensusMu sync.Mutex + bodyCensusCache = map[string][]bodyReadSite{} +) + +// collectRequestBodyReads type-checks the package in dir and returns every +// selector whose RESOLVED object is one of net/http.Request's body readers +// (BUG-2820). +// +// This asks the type checker, not the spelling. The parser it replaces matched +// the literal `http.Request` in a signature and then names bound to it, so a +// request reached through a struct field, a context value, a type alias, an +// embedded field, an import alias, a dot import, a `var` declaration, a +// call-derived alias, a named result or a range binding was invisible. Here +// a selector counts when go/types resolves it to the SAME object that +// looking the member up on net/http.Request yields, which is what every one of +// those forms has in common. Promotion through embedding resolves to the same +// field or method, and an alias is the same type, so none needs a special +// case. A same-named member of another type (a Response's Body, a struct's +// own Body field) resolves to a different object and is not counted. +func collectRequestBodyReads(t *testing.T, dir string) []bodyReadSite { + t.Helper() + bodyCensusMu.Lock() + defer bodyCensusMu.Unlock() + if cached, ok := bodyCensusCache[dir]; ok { + return cached + } + + pkg := loadCensusPackage(t, dir) + + httpPkg := findImportedPackage(pkg, "net/http") + if httpPkg == nil { + t.Fatalf("%s does not import net/http, so there is no request type to look for", dir) + } + reqObj := httpPkg.Types.Scope().Lookup("Request") + if reqObj == nil { + t.Fatal("net/http.Request not found") + } + reqType := reqObj.Type() + want := map[types.Object]string{} + for name := range bodyReaderSelectors { + obj, _, _ := types.LookupFieldOrMethod(reqType, true, nil, name) + if obj == nil { + t.Fatalf("net/http.Request has no member %q; the selector list is stale", name) + } + want[obj] = name + } + + var sites []bodyReadSite + for _, f := range pkg.Syntax { + file := filepath.Base(pkg.Fset.File(f.Pos()).Name()) + for _, decl := range f.Decls { + fn := declName(decl) + ast.Inspect(decl, func(n ast.Node) bool { + sel, ok := n.(*ast.SelectorExpr) + if !ok { + return true + } + selection, ok := pkg.TypesInfo.Selections[sel] + if !ok { + return true + } + if name, hit := want[selection.Obj()]; hit { + sites = append(sites, bodyReadSite{ + file: file, fn: fn, sel: name, + line: pkg.Fset.Position(sel.Pos()).Line, + }) + } + return true + }) + } + } + sort.Slice(sites, func(i, j int) bool { + if sites[i].file != sites[j].file { + return sites[i].file < sites[j].file + } + return sites[i].line < sites[j].line + }) + bodyCensusCache[dir] = sites + return sites +} + +func findImportedPackage(pkg *packages.Package, path string) *packages.Package { + seen := map[*packages.Package]bool{} + var walk func(*packages.Package) *packages.Package + walk = func(p *packages.Package) *packages.Package { + if seen[p] { + return nil + } + seen[p] = true + if p.PkgPath == path { + return p + } + for _, imp := range p.Imports { + if found := walk(imp); found != nil { + return found + } + } + return nil + } + return walk(pkg) +} + +// declName names a top-level declaration: "Func", "Recv.Method", or "var"/ +// "const"/"type" for a GenDecl (a reader inside a package-level initializer +// is still a reader, and still has to be accounted for). +func declName(d ast.Decl) string { + switch x := d.(type) { + case *ast.FuncDecl: + if x.Recv != nil && len(x.Recv.List) > 0 { + recv := x.Recv.List[0].Type + if star, ok := recv.(*ast.StarExpr); ok { + recv = star.X + } + if idx, ok := recv.(*ast.IndexExpr); ok { + recv = idx.X + } + if id, ok := recv.(*ast.Ident); ok { + return id.Name + "." + x.Name.Name + } + } + return x.Name.Name + case *ast.GenDecl: + return x.Tok.String() + } + return "?" +} + +// TestBodyReaderCensusSeesTheFormsTheParserMissed is the census's control: a +// fixture package with one function per form the old parser could not see, and +// three that must NOT count. A census that went blind to any form fails here, +// by name, rather than by a count drifting in the production table. +func TestBodyReaderCensusSeesTheFormsTheParserMissed(t *testing.T) { + sites := collectRequestBodyReads(t, filepath.Join("testdata", "bodyreaders")) + found := map[string]int{} + for _, s := range sites { + found[s.fn]++ + } + for _, fn := range []string{ + // The forms the parser could not see (BUG-2820). + "StructField", "ContextValue", "TypeAlias", "EmbeddedField", "EmbeddedMethod", + "ImportAlias", "DotImport", "VarDecl", "CallDerived", "NamedResult", + "RangeBinding", "MethodValue", + // The forms the parser's own controls pinned. + "DirectParam", "ClosureParam", "LocalAlias", "ValueCopy", "ClosureValueParam", + "ReassignedWithContext", "MixedShortDecl", "InsideSwitch", "AfterIfInitShadow", + } { + if found[fn] != 1 { + t.Errorf("%s: census counted %d body readers, want 1", fn, found[fn]) + } + } + for _, fn := range []string{"NotARequest", "HeaderOnly", "ResponseBody", + "ShadowedByClosureParam", "ReboundInNestedBlock", "CommentOnly"} { + if found[fn] != 0 { + t.Errorf("%s is not a request body reader, but the census counted %d", fn, found[fn]) + } + } +} + +func sitesByKey(sites []bodyReadSite) map[string]int { + out := map[string]int{} + for _, s := range sites { + out[s.key()]++ + } + return out +} + +// TestEveryRequestBodyReaderIsAccountedFor is the completeness claim for the +// request-body rule (BUG-2803): a JSON body reaches the store through +// decodeJSON / decodeJSONWithLimit, which refuse a decoded NUL and apply the +// size cap, so every OTHER reader of a request body has to be a deliberate, +// written-down decision. +// +// Since BUG-2820 the readers come from the TYPE CHECKER +// (collectRequestBodyReads), not a parser. The parser matched the spelling +// `http.Request` and names bound to it, so a request reached through a struct +// field, a context value, a type alias or an embedded field was invisible, and +// its accounting was per FILE with a reader count to stop an entry absorbing a +// new reader. Both workarounds are gone: a reader is found wherever its type +// says it is one, and the table below is per CALL SITE, keyed by +// file::function::member, with the number of uses of that member in that +// function. A new reader anywhere, including a second one in an accounted +// function, changes the table and fails here until someone writes it down. +// +// Run against the tree when this was built, the type checker found exactly the +// readers the parser had found, file for file (measured, not assumed): the +// blind spots were real but nothing in the package was using them yet. The +// fixture control, TestBodyReaderCensusSeesTheFormsTheParserMissed, is what +// shows the census would see one. +// +// It asserts BOTH directions: an unaccounted site fails, and an accounted site +// that no longer exists fails, so the table cannot rot into stale excuses. +func TestEveryRequestBodyReaderIsAccountedFor(t *testing.T) { + // WHY each file reads a body. Every site below must name a file here. + bodyReaderFileWhy := map[string]string{ + "middleware_request_text.go": "the chokepoint itself: readBodyForDecode reads the body under the caller's cap so bodyDecodesNUL can scan it", + "handlers_import_bundle.go": "tar.gz bundle import — streams the body through gzip, and its pad-export.json is checked with bodyDecodesNUL before ImportWorkspace", + "handlers_attachments.go": "multipart upload — the body is binary blob content, not text, and must NOT be scanned for text validity", + "artifact_import.go": "raw artifact TEXT (not JSON) — checked with bindableText, the same predicate ValidatePath and ValidateQuery apply", + "handlers_cloud.go": "bodyHasCloudSecret PEEKS at the body and restores the first 64 KiB of it — a larger body loses its tail, a bound that file documents and accepts; the real decode still happens through decodeJSON downstream", + "middleware_mcp_audit.go": "audit capture — parses the body ITSELF and binds the decoded method / params.name to mcp_audit_log.tool_name, so it is a second READER, not a pass-through. That the MCP dispatcher decodes the body again is true and says nothing about what this middleware persists — the earlier rationale here made exactly that mistake and certified it safe (codex round 20). parseMCPRequestBody now runs both caller-derived returns through sanitiseStoredText", + "handlers_tokens.go": "guards on r.Body != nil && r.ContentLength != 0, then decodes THROUGH decodeJSON — so the body is read by the chokepoint, which applies the cap and the NUL rule. The earlier reason here said it never reads the body, which was simply false (codex round 29): a wrong reason in this list is the same defect as a missing entry, since both let a reader pass as reviewed", + "handlers_oauth.go": "KNOWN GAP, tracked as BUG-2811: the OAuth handlers read FORM-encoded bodies (r.Form/FormValue), which no rule in this family covers — the transport rules see the query half of r.Form and not the body half. Listed so this test states the gap instead of being blind to it; measuring it needs a fosite-backed fixture.", + "handlers_watches.go": "guards on r.Body != nil && r.ContentLength != 0, then decodes THROUGH decodeJSON — the closing-round-4 fix for the chunked-body drop; the one reader expression is the nil check itself, and the body bytes flow through the chokepoint", + "handlers_item_lease.go": "guards on r.Body != nil && r.ContentLength != 0, then decodes THROUGH decodeJSON — same shape as handlers_watches.go; the one reader expression is the nil check, and the body (optional holder/ttl_seconds) flows through the chokepoint, so the caller-text holder gets BUG-2803's NUL refusal before it can reach items.lease_holder", + "import_read_deadline.go": "a PASS-THROUGH, not a reader: it replaces r.Body with a wrapper whose Read arms the connection deadline and delegates, byte for byte, to the original body (BUG-3184). Nothing in the file inspects the bytes; the import route's two decoders (decodeJSONWithLimit, and the bundle path's bodyDecodesNUL on pad-export.json) read them afterwards, unchanged", + } + + // Every call site, counted. MEASURED from the census, not written from + // expectation. + bodyReaderSites := map[string]int{ + "artifact_import.go::parseArtifactRequest::Body": 1, + "handlers_attachments.go::Server.handleUploadAttachment::Body": 2, + "handlers_attachments.go::Server.handleUploadAttachment::MultipartForm": 2, + "handlers_attachments.go::Server.handleUploadAttachment::ParseMultipartForm": 1, + "handlers_attachments.go::Server.handleUploadAttachment::FormFile": 1, + "handlers_attachments.go::multipartValues::MultipartForm": 2, + "handlers_cloud.go::hasCloudSecretMarker::Body": 1, + "handlers_cloud.go::bodyHasCloudSecret::Body": 2, + "handlers_import_bundle.go::Server.handleImportWorkspaceBundle::Body": 3, + "handlers_item_lease.go::Server.resolveLeaseRequest::Body": 1, + "handlers_oauth.go::Server.handleOAuthAuthorize::ParseForm": 1, + "handlers_oauth.go::Server.handleOAuthAuthorizeDecide::ParseForm": 1, + "handlers_oauth.go::Server.handleOAuthAuthorizeDecide::FormValue": 1, + "handlers_oauth.go::Server.parseConsentPayload::FormValue": 4, + "handlers_oauth.go::Server.parseConsentPayload::PostForm": 2, + "handlers_oauth.go::Server.handleOAuthToken::ParseForm": 1, + "handlers_oauth.go::Server.handleOAuthRevoke::ParseForm": 1, + "handlers_oauth.go::Server.handleOAuthRevoke::PostForm": 1, + "handlers_oauth.go::Server.validateConsentCSRFToken::FormValue": 1, + "handlers_tokens.go::Server.handleRotateUserToken::Body": 1, + "handlers_watches.go::Server.handleCreateWatch::Body": 1, + "import_read_deadline.go::Server.withImportReadDeadline::Body": 2, + "middleware_mcp_audit.go::Server.MCPAuditLog::Body": 5, + "middleware_mcp_audit.go::Server.emitMCPAuditDenied::Body": 2, + "middleware_request_text.go::readBodyForDecode::Body": 4, + } + + sites := collectRequestBodyReads(t, ".") + // The census must have found something: a load that silently resolved no + // request type would pass the comparisons below with an empty table. + if len(sites) < 20 { + t.Fatalf("census found only %d body readers in the package; the load or the type lookup is broken", len(sites)) + } + got := sitesByKey(sites) + for key, n := range got { + want, ok := bodyReaderSites[key] + switch { + case !ok: + t.Errorf("%s reads the request body %d time(s) and is not accounted for. A body carrying JSON must go "+ + "through decodeJSON/decodeJSONWithLimit, which apply BUG-2803's NUL refusal and the size cap. If this "+ + "reader is legitimate, add it to bodyReaderSites (and its file to bodyReaderFileWhy) WITH the reason.", key, n) + case n != want: + t.Errorf("%s has %d body reader(s) but was reviewed with %d. A reader was added or removed in that "+ + "function since it was justified: re-read it and update the count.", key, n, want) + } + file := strings.SplitN(key, "::", 2)[0] + if _, ok := bodyReaderFileWhy[file]; !ok { + t.Errorf("%s has no reason in bodyReaderFileWhy", file) + } + } + for key := range bodyReaderSites { + if _, ok := got[key]; !ok { + t.Errorf("%s is accounted for but no longer reads a request body; remove the entry so it cannot "+ + "cover a future reader", key) + } + } + for file := range bodyReaderFileWhy { + live := false + for key := range got { + if strings.HasPrefix(key, file+"::") { + live = true + } + } + if !live { + t.Errorf("bodyReaderFileWhy names %s, which no longer reads a request body; remove it", file) + } + } +} + +// requestHandoff is a call that passes a request to a function OUTSIDE this +// module, which may read the body where no selector here does. +type requestHandoff struct { + file, fn, callee string + line int +} + +func (h requestHandoff) key() string { return h.file + "::" + h.fn + "::" + h.callee } + +// collectRequestHandoffs returns every call in the package in dir that passes a +// value of type *net/http.Request or net/http.Request to a function or method +// declared outside this module. The body-reader census sees selectors; a +// library that reads the body on our behalf (fosite parsing an OAuth form) has +// no selector here, so it is counted as a HANDOFF and classified by callee. +// +// What it cannot see, stated: a call through an INTERFACE (http.Handler's +// ServeHTTP, which is how the router and every middleware chain dispatch) +// resolves to the interface method, not to whatever implements it. Those are +// classified as dispatch below, not as readers. +func collectRequestHandoffs(t *testing.T, dir string) []requestHandoff { + t.Helper() + pkg := loadCensusPackage(t, dir) + module := pkg.Module + if module == nil { + t.Fatalf("%s has no module information", dir) + } + reqType := findImportedPackage(pkg, "net/http").Types.Scope().Lookup("Request").Type() + + var out []requestHandoff + for _, f := range pkg.Syntax { + file := filepath.Base(pkg.Fset.File(f.Pos()).Name()) + for _, decl := range f.Decls { + fn := declName(decl) + ast.Inspect(decl, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + var id *ast.Ident + switch x := call.Fun.(type) { + case *ast.SelectorExpr: + id = x.Sel + case *ast.Ident: + id = x + } + if id == nil { + return true + } + callee := pkg.TypesInfo.ObjectOf(id) + if callee == nil || callee.Pkg() == nil { + return true + } + path := callee.Pkg().Path() + if path == module.Path || strings.HasPrefix(path, module.Path+"/") { + return true + } + for _, a := range call.Args { + at := pkg.TypesInfo.TypeOf(a) + if ptr, ok := at.(*types.Pointer); ok { + at = ptr.Elem() + } + if at != nil && types.Identical(at, reqType) { + out = append(out, requestHandoff{file: file, fn: fn, + callee: qualifiedCallee(callee), line: pkg.Fset.Position(call.Pos()).Line}) + break + } + } + return true + }) + } + } + return out +} + +// TestEveryRequestHandoffIsClassified extends the accounting past this +// package's own selectors (BUG-2820): a request handed to third-party code +// must be classified by what that code does with the body. A new callee fails +// here until someone decides. Body-READING callees are pinned per call site, +// like the readers above; the rest are pinned by callee. +func TestEveryRequestHandoffIsClassified(t *testing.T) { + notReaders := map[string]string{ + "github.com/go-chi/chi/v5.URLParam": "reads the route context, not the body", + "net/http.NotFound": "writes a 404; reads nothing of the request body", + "net/http.Redirect": "reads the URL to build the Location header", + "net/http.ServeContent": "reads range and conditional HEADERS", + "github.com/gorilla/websocket.IsWebSocketUpgrade": "reads headers", + "github.com/gorilla/websocket.Upgrader.Upgrade": "hijacks the connection after checking headers; a GET upgrade has no body", + "net/http.Handler.ServeHTTP": "interface DISPATCH: the router and middleware chains passing the request on to the next handler, whose own reads are counted where they are written. Which handler runs is not resolvable statically", + "github.com/go-chi/chi/v5.Mux.ServeHTTP": "dispatch into the router, as above", + } + // Body readers by delegation: fosite parses the OAuth form body itself. + // These are the handlers BUG-2811 tracks (form bodies are not covered by + // the BUG-2803 rule), now counted per call site instead of hidden behind + // the handler file's own ParseForm calls. + readers := map[string]int{ + "handlers_oauth.go::Server.handleOAuthAuthorize::github.com/ory/fosite.OAuth2Provider.NewAuthorizeRequest": 1, + "handlers_oauth.go::Server.handleOAuthAuthorizeDecide::github.com/ory/fosite.OAuth2Provider.NewAuthorizeRequest": 1, + "handlers_oauth.go::Server.handleOAuthToken::github.com/ory/fosite.OAuth2Provider.NewAccessRequest": 1, + "handlers_oauth.go::Server.handleOAuthRevoke::github.com/ory/fosite.OAuth2Provider.NewRevocationRequest": 1, + "handlers_oauth.go::Server.handleOAuthIntrospect::github.com/ory/fosite.OAuth2Provider.NewIntrospectionRequest": 1, + } + + handoffs := collectRequestHandoffs(t, ".") + if len(handoffs) < 10 { + t.Fatalf("found only %d handoffs; the census is broken", len(handoffs)) + } + got := map[string]int{} + seenCallee := map[string]bool{} + for _, h := range handoffs { + seenCallee[h.callee] = true + if _, ok := notReaders[h.callee]; ok { + continue + } + got[h.key()]++ + } + for key, n := range got { + if want, ok := readers[key]; !ok { + t.Errorf("%s hands a request to third-party code %d time(s) and is not classified. Decide whether that "+ + "callee reads the body: if not, add the callee to notReaders with the reason; if it does, add this site "+ + "to readers and make sure the body it reads is covered", key, n) + } else if n != want { + t.Errorf("%s: %d handoff(s), classified with %d", key, n, want) + } + } + for key := range readers { + if _, ok := got[key]; !ok { + t.Errorf("%s is classified as a reader but no longer exists; remove it", key) + } + } + for callee := range notReaders { + if !seenCallee[callee] { + t.Errorf("notReaders names %s, which nothing calls with a request any more; remove it", callee) + } + } +} + +// qualifiedCallee names a function "pkg/path.Name" and a method +// "pkg/path.Recv.Name", so an interface method (http.Handler's ServeHTTP) and a +// concrete one (chi's Mux) are told apart. +func qualifiedCallee(obj types.Object) string { + name := obj.Pkg().Path() + "." + if fn, ok := obj.(*types.Func); ok { + if sig, ok := fn.Type().(*types.Signature); ok && sig.Recv() != nil { + rt := sig.Recv().Type() + if ptr, ok := rt.(*types.Pointer); ok { + rt = ptr.Elem() + } + if named, ok := rt.(*types.Named); ok { + name += named.Obj().Name() + "." + } + } + } + return name + obj.Name() +} diff --git a/internal/server/decode_json_nul_test.go b/internal/server/decode_json_nul_test.go index 377709d91..7c9a064eb 100644 --- a/internal/server/decode_json_nul_test.go +++ b/internal/server/decode_json_nul_test.go @@ -5,9 +5,6 @@ import ( "database/sql" "encoding/json" "errors" - "go/ast" - "go/parser" - "go/token" "io" "net/http" "net/http/httptest" @@ -214,172 +211,6 @@ func TestDecodeJSONKeepsEmptyBodyEOF(t *testing.T) { } } -// TestEveryRequestBodyReaderIsAccountedFor is the completeness claim, made -// ENFORCEABLE rather than asserted — and made honest after codex round 3 -// showed the first version could not see past two exact call shapes. -// -// The fix works because a JSON request body reaches the store through -// decodeJSON/decodeJSONWithLimit, which refuse a decoded NUL and apply a size -// cap. The first version of this test scanned for `json.NewDecoder(r.Body)` -// and `io.ReadAll(r.Body)` only, so it was blind to -// `io.ReadAll(io.LimitReader(r.Body, n))` — a shape ALREADY PRESENT in the -// package — and to any alias, helper or future spelling. A completeness test -// that misses a live example is worse than none: it reads as coverage. -// -// So it now scans for something much harder to spell around — a NAME-visible -// reference to the request body — and requires every FILE that touches one to -// be accounted for here with a reason. Not impossible to spell around: a -// request reached through a struct field, a context value, or a type alias is -// invisible to it, exactly as the KNOWN LIMITS block at the bottom states — -// an earlier version of this sentence claimed more than the scanner delivers -// (codex closing round). A new body reader fails this test until -// someone writes down what it does, which is the point: the decision becomes -// deliberate instead of invisible. -// -// It asserts BOTH directions. An unaccounted file fails, because a door may -// have opened. An accounted file that no longer touches a body ALSO fails, so -// the list cannot rot into a set of stale excuses that quietly permits a -// future reader added to the same file. -func TestEveryRequestBodyReaderIsAccountedFor(t *testing.T) { - // Each entry records HOW MANY reader expressions the file had when it was - // reviewed, not just that it was reviewed. - // - // Per-FILE accounting alone is not a safety property: once a file is - // listed, a NEW reader added to it is covered by the existing entry and - // this test stays green (codex round 29 made the point that this is what - // makes over-flagging expensive rather than cheap — a false positive does - // not cost one review, it permanently blinds the list for that file). - // - // The count closes that. Adding a reader to an accounted file changes the - // number and fails here, so the entry has to be re-read and its reason - // re-justified. It churns exactly when a body reader is added or removed, - // which is precisely when a human should look. - type entry struct { - readers int - why string - } - accountedWhy := map[string]string{ - "middleware_request_text.go": "the chokepoint itself: readBodyForDecode reads the body under the caller's cap so bodyDecodesNUL can scan it", - "handlers_import_bundle.go": "tar.gz bundle import — streams the body through gzip, and its pad-export.json is checked with bodyDecodesNUL before ImportWorkspace", - "handlers_attachments.go": "multipart upload — the body is binary blob content, not text, and must NOT be scanned for text validity", - "artifact_import.go": "raw artifact TEXT (not JSON) — checked with bindableText, the same predicate ValidatePath and ValidateQuery apply", - "handlers_cloud.go": "bodyHasCloudSecret PEEKS at the body and restores the first 64 KiB of it — a larger body loses its tail, a bound that file documents and accepts; the real decode still happens through decodeJSON downstream", - "middleware_mcp_audit.go": "audit capture — parses the body ITSELF and binds the decoded method / params.name to mcp_audit_log.tool_name, so it is a second READER, not a pass-through. That the MCP dispatcher decodes the body again is true and says nothing about what this middleware persists — the earlier rationale here made exactly that mistake and certified it safe (codex round 20). parseMCPRequestBody now runs both caller-derived returns through sanitiseStoredText", - "handlers_tokens.go": "guards on r.Body != nil && r.ContentLength != 0, then decodes THROUGH decodeJSON — so the body is read by the chokepoint, which applies the cap and the NUL rule. The earlier reason here said it never reads the body, which was simply false (codex round 29): a wrong reason in this list is the same defect as a missing entry, since both let a reader pass as reviewed", - "handlers_oauth.go": "KNOWN GAP, tracked as BUG-2811: the OAuth handlers read FORM-encoded bodies (r.Form/FormValue), which no rule in this family covers — the transport rules see the query half of r.Form and not the body half. Listed so this test states the gap instead of being blind to it; measuring it needs a fosite-backed fixture.", - "handlers_watches.go": "guards on r.Body != nil && r.ContentLength != 0, then decodes THROUGH decodeJSON — the closing-round-4 fix for the chunked-body drop; the one reader expression is the nil check itself, and the body bytes flow through the chokepoint", - "handlers_item_lease.go": "guards on r.Body != nil && r.ContentLength != 0, then decodes THROUGH decodeJSON — same shape as handlers_watches.go; the one reader expression is the nil check, and the body (optional holder/ttl_seconds) flows through the chokepoint, so the caller-text holder gets BUG-2803's NUL refusal before it can reach items.lease_holder", - "import_read_deadline.go": "a PASS-THROUGH, not a reader: it replaces r.Body with a wrapper whose Read arms the connection deadline and delegates, byte for byte, to the original body (BUG-3184). Nothing in the file inspects the bytes; the import route's two decoders (decodeJSONWithLimit, and the bundle path's bodyDecodesNUL on pad-export.json) read them afterwards, unchanged", - } - - // Reader counts as reviewed. A mismatch means this file gained or lost a - // body reader since someone last justified it. - // MEASURED, not estimated. I first wrote plausible-looking numbers here - // and every one of them was wrong; the test reported the real counts on - // the first run. Recording that because it is the same habit this branch - // keeps catching — a figure written from expectation reads exactly like a - // figure that was counted. - accountedCount := map[string]int{ - "middleware_request_text.go": 4, - "handlers_import_bundle.go": 3, - "handlers_attachments.go": 8, - "artifact_import.go": 1, - "handlers_cloud.go": 3, - "middleware_mcp_audit.go": 7, - "handlers_tokens.go": 1, - "handlers_oauth.go": 13, - "handlers_watches.go": 1, - "handlers_item_lease.go": 1, - "import_read_deadline.go": 2, - } - accounted := map[string]entry{} - for name, why := range accountedWhy { - accounted[name] = entry{readers: accountedCount[name], why: why} - } - - // FormValue/PostFormValue/ParseForm/ParseMultipartForm read the request - // BODY too, and the first version of this scan looked only for .Body — so - // it reported full coverage while the OAuth form-body handlers, which - // BUG-2811 tracks, were entirely invisible to it (codex round 13). A - // completeness test with a blind spot is worse than none, because it - // reads as coverage. - entries, err := os.ReadDir(".") - if err != nil { - t.Fatalf("read package dir: %v", err) - } - touches := map[string]int{} - scanned := 0 - for _, e := range entries { - name := e.Name() - if e.IsDir() || !strings.HasSuffix(name, ".go") || strings.HasSuffix(name, "_test.go") { - continue - } - scanned++ - if n := scanFileForRequestBodyReads(t, filepath.Join(".", name)); n > 0 { - touches[name] = n - } - } - - // The scan must have looked at something, and must have FOUND something. - // A pattern that silently matched nothing would pass forever. - if scanned < 20 { - t.Fatalf("scan looked at only %d non-test .go files; the package is much larger, so the scan is broken", scanned) - } - if len(touches) < 3 { - t.Fatalf("scan found only %d files touching a request body; the pattern is broken", len(touches)) - } - - for name := range touches { - if _, ok := accounted[name]; !ok { - t.Errorf("%s reads the request body but is not accounted for in this test. "+ - "A body carrying JSON must go through decodeJSON/decodeJSONWithLimit, which apply "+ - "BUG-2803's NUL refusal and the size cap. If this reader is legitimate, add it here WITH "+ - "the reason it is safe.", name) - } - } - for name, ent := range accounted { - if got := touches[name]; got != 0 && got != ent.readers { - t.Errorf("%s has %d request-body readers but was reviewed with %d (%q). A reader was "+ - "added or removed since someone justified this entry — re-read it and update the "+ - "count, or the entry silently covers code nobody reviewed.", name, got, ent.readers, ent.why) - } - why := ent.why - if touches[name] == 0 { - t.Errorf("%s is accounted for here (%q) but no longer reads a request body — remove the entry "+ - "so it cannot silently cover a future reader added to that file", name, why) - } - } - - // KNOWN LIMITS, stated because this test's whole value is a completeness - // claim, and an unqualified one is how the MCP audit reader came to be - // certified safe while persisting a decoded NUL (codex round 20). - // - // 1. Accounting is per FILE, not per call site. Once a file is listed, a - // NEW reader added to it is covered by the existing entry and this - // test stays green. The reason strings are what a reviewer checks - // against; they are not permanent exemptions. - // 2. Only requests reachable by NAME are seen. Closure parameters, local - // aliases (`req := r`), and dereferenced copies (`c := *r`) are - // covered; a variable that SHADOWS a request name is conservatively - // over-flagged, not ignored — an earlier sentence here claimed the - // opposite while the controls asserted the flagging (codex closing - // round 2). All are pinned by controls run against this scanner. - // Still invisible: a request stored in a STRUCT FIELD, one obtained - // from a context, one whose type reaches http.Request through an - // alias or an embedded field, and the ordinary alias forms the - // fixed-point does not walk — a `var req = r` declaration, a - // call-derived alias (`req := r.WithContext(ctx)`), named results, - // and range bindings — since this matches literal spellings rather - // than resolving types (codex rounds 26/29, closing round 3). - // Closing the class means running the type checker, not another - // parser patch — tracked as BUG-2820, which enumerates these forms. - // - // Both want per-call-site accounting, which is a different instrument — - // BUG-2820's go/types rewrite is filed to provide exactly that (each - // reader expression individually justified), retiring the per-file - // count workaround along with the name-resolution blind spots. -} - // jsonEncode returns s as a JSON string literal — the wire form of a field // that crosses as a JSON-ENCODED STRING (an item's fields, a collection's // schema, a workspace's settings). Using encoding/json to build it, rather @@ -1541,269 +1372,3 @@ func TestBodyDecodesNULAgainstAnIndependentOracle(t *testing.T) { sawTrue, sawFalse) } } - -// probeFileHeader is the preamble every synthetic probe file shares. -var probeFileHeader = "package probe\n\nimport (\n\t\"context\"\n\t\"io\"\n\t\"net/http\"\n)\n\nvar _ = io.ReadAll\nvar _ = context.Background\n\n" - -// TestBodyReaderScanDiscriminates feeds the accounting scan SYNTHETIC source -// covering the cases that have broken it, instead of only pointing it at the -// package and trusting the result. -// -// Round 27 was right to flag this: the earlier controls were run as throwaway -// probes and deleted, so nothing in the suite held the scanner to them. A scan -// whose whole value is a completeness claim needs its own discrimination -// pinned, or the next rewrite silently loses a case. -// -// Both directions matter, and for a reason specific to this instrument. A -// false NEGATIVE hides a reader. A false POSITIVE is arguably worse: the only -// way to green a false flag is to add the file to the accounted list, and an -// accounting entry then hides every FUTURE reader in that file. -func TestBodyReaderScanDiscriminates(t *testing.T) { - for _, tc := range []struct { - name string - want bool - src string - }{ - {"plain handler", true, ` -func h(w http.ResponseWriter, r *http.Request) { io.ReadAll(r.Body) }`}, - {"unconventional parameter name", true, ` -func h(w http.ResponseWriter, httpReq *http.Request) { io.ReadAll(httpReq.Body) }`}, - {"closure parameter", true, ` -func h() func(*http.Request) { return func(hr *http.Request) { io.ReadAll(hr.Body) } }`}, - {"local alias", true, ` -func h(orig *http.Request) { req := orig; io.ReadAll(req.Body) }`}, - {"dereferenced local copy still shares the body", true, ` -func h(r *http.Request) { c := *r; io.ReadAll(c.Body) }`}, - {"value copy still shares the body", true, ` -func h(r *http.Request) { f := func(c http.Request) { io.ReadAll(c.Body) }; f(*r) }`}, - {"form reader", true, ` -func h(r *http.Request) { _ = r.FormValue("x") }`}, - // MultipartReader and FormFile are in the selector list but had no - // control here, so their removal from that list would have gone - // undetected (codex closing round 7). - {"multipart streaming reader", true, ` -func h(r *http.Request) { _, _ = r.MultipartReader() }`}, - {"form file reader", true, ` -func h(r *http.Request) { _, _, _ = r.FormFile("f") }`}, - {"reassigned through WithContext", true, ` -func h(r *http.Request) { - r = r.WithContext(context.Background()) - io.ReadAll(r.Body) -}`}, - {"reader inside an if body", true, ` -func h(r *http.Request) { - if r.Method == "POST" { - io.ReadAll(r.Body) - } -}`}, - {"reader inside a for body", true, ` -func h(r *http.Request) { - for i := 0; i < 1; i++ { - io.ReadAll(r.Body) - } -}`}, - - {"no request at all", false, ` -type payload struct{ Body string } - -func h(p payload) int { return len(p.Body) }`}, - // These two are OVER-FLAGGED on purpose, and asserted as such so the - // bias is a decision on the record rather than a surprise. The file - // binds `r` to a request, and the scanner does not model the scope - // that rebinds it. Erring this way is what makes the false-negative - // classes from rounds 24-28 structurally impossible. - {"shadowed by a closure parameter (conservatively flagged)", true, ` -type other struct{ Body string } - -func h(r *http.Request) int { f := func(r other) int { return len(r.Body) }; return f(other{}) }`}, - {"rebound in a nested block (conservatively flagged)", true, ` -type resp struct{ Body string } - -func h(r *http.Request) int { - n := 0 - { - r := resp{} - n = len(r.Body) - } - return n -}`}, - {"mixed short declaration reusing the request", true, ` -func getReq() (*http.Request, bool) { return nil, false } - -func h(r *http.Request) { - r, ok := getReq() - _ = ok - io.ReadAll(r.Body) -}`}, - {"reader in a switch case", true, ` -func h(r *http.Request) { - switch r.Method { - case "POST": - io.ReadAll(r.Body) - } -}`}, - {"reader after an if-initialiser shadow", true, ` -func h(r *http.Request) { - if r := 1; r > 0 { - _ = r - } - io.ReadAll(r.Body) -}`}, - {"mentioned only in a comment", false, ` -// This function does not read r.Body, it only talks about it. -func h(r *http.Request) int { return 1 }`}, - } { - t.Run(tc.name, func(t *testing.T) { - dir := t.TempDir() - path := filepath.Join(dir, "probe.go") - src := probeFileHeader + tc.src + "\n" - if err := os.WriteFile(path, []byte(src), 0o600); err != nil { - t.Fatalf("write probe: %v", err) - } - if got := scanFileForRequestBodyReads(t, path) > 0; got != tc.want { - t.Errorf("scan detected a reader = %v, want %v\n--- source ---\n%s", got, tc.want, src) - } - }) - } -} - -// scanFileForRequestBodyReads reports whether path contains code that reads the -// HTTP request body. -// -// DELIBERATELY CONSERVATIVE: it over-approximates. Any identifier bound to an -// http.Request anywhere in the file is treated as a request for the whole file, -// and a reader selector on such a name counts. Names are never un-bound. -// -// That is a decision, not an oversight, and it was made after five review -// rounds found successive FALSE NEGATIVES in a scope-modelling version of this -// scanner: a value-copied request, a plain `r = r.WithContext(ctx)` -// reassignment, a mixed `r, ok := ...` short declaration that reuses an -// existing variable, and if/for/switch initialisers and case clauses whose -// scopes it did not model. Each fix closed one case and left another. -// -// The two error directions are not symmetric for this test. A false NEGATIVE -// hides a body reader, which is the entire thing this test exists to prevent. -// A false POSITIVE costs one human review and an accounting entry with a -// reason attached. So the scanner is biased to over-flag, and the residual -// imprecision is stated below rather than chased. -// -// Getting this EXACT means resolving types, not parsing them — go/types with -// a real package load. That is a bigger instrument than this test warrants -// today; if the accounted list ever grows entries whose reason is "the scan -// over-flagged", that is the signal to build it. -// -// Known imprecision, all in the safe direction: a name shadowed by a -// non-request of the same spelling, or rebound in a nested scope, still counts -// for the whole file. Still invisible, and NOT in the safe direction: a request -// reached only through a struct field, a context value, or a type alias, since -// this matches the literal http.Request spelling. -func scanFileForRequestBodyReads(t *testing.T, path string) int { - t.Helper() - - readerSelectors := map[string]bool{ - "Body": true, "FormValue": true, "PostFormValue": true, "ParseForm": true, - "ParseMultipartForm": true, "MultipartForm": true, "PostForm": true, - // MultipartReader STREAMS the body, and FormFile triggers multipart - // parsing of it. Both were missing (codex round 29) — and FormFile is - // used in production, in handlers_attachments.go, so the list was - // incomplete against code that exists rather than hypothetically. - "MultipartReader": true, "FormFile": true, - } - - // Both *http.Request and http.Request count. A VALUE copy still shares the - // Body — it is an interface holding the same reader. - isRequestType := func(e ast.Expr) bool { - if star, ok := e.(*ast.StarExpr); ok { - e = star.X - } - sel, ok := e.(*ast.SelectorExpr) - if !ok || sel.Sel.Name != "Request" { - return false - } - pkg, ok := sel.X.(*ast.Ident) - return ok && pkg.Name == "http" - } - - fset := token.NewFileSet() - f, err := parser.ParseFile(fset, path, nil, 0) - if err != nil { - t.Fatalf("parse %s: %v", path, err) - } - - // Pass 1: every name ever bound to a request, from any signature in the - // file — declarations, literals, receivers — plus aliases of those. - reqNames := map[string]bool{} - collectParams := func(fl *ast.FieldList) { - if fl == nil { - return - } - for _, fld := range fl.List { - if !isRequestType(fld.Type) { - continue - } - for _, nm := range fld.Names { - reqNames[nm.Name] = true - } - } - } - ast.Inspect(f, func(n ast.Node) bool { - switch node := n.(type) { - case *ast.FuncDecl: - collectParams(node.Type.Params) - collectParams(node.Recv) - case *ast.FuncLit: - collectParams(node.Type.Params) - } - return true - }) - // Aliases, to a fixed point: `req := r`, `r2 = req`, and chains of them. - for changed := true; changed; { - changed = false - ast.Inspect(f, func(n ast.Node) bool { - as, ok := n.(*ast.AssignStmt) - if !ok { - return true - } - for i, rhs := range as.Rhs { - if i >= len(as.Lhs) { - break - } - // `req := r` aliases the pointer; `c := *r` copies the VALUE, - // and the copy still shares the Body (an interface holding the - // same reader) — the second form was invisible until the - // closing rounds caught it (codex closing round 2). - src := rhs - if star, ok := rhs.(*ast.StarExpr); ok { - src = star.X - } - id, isIdent := src.(*ast.Ident) - lhs, isLhsIdent := as.Lhs[i].(*ast.Ident) - if !isIdent || !isLhsIdent || !reqNames[id.Name] || reqNames[lhs.Name] { - continue - } - reqNames[lhs.Name] = true - changed = true - } - return true - }) - } - if len(reqNames) == 0 { - return 0 - } - - // Pass 2: COUNT reader selectors on those names. A count rather than a - // yes/no, so an accounting entry cannot silently absorb a reader added to - // a file that was already reviewed. - readers := 0 - ast.Inspect(f, func(n ast.Node) bool { - sel, ok := n.(*ast.SelectorExpr) - if !ok || !readerSelectors[sel.Sel.Name] { - return true - } - if id, ok := sel.X.(*ast.Ident); ok && reqNames[id.Name] { - readers++ - } - return true - }) - return readers -} diff --git a/internal/server/testdata/bodyreaders/fixture.go b/internal/server/testdata/bodyreaders/fixture.go new file mode 100644 index 000000000..57f2ca15d --- /dev/null +++ b/internal/server/testdata/bodyreaders/fixture.go @@ -0,0 +1,146 @@ +// Package bodyreaders is the fixture for the type-checked body-reader census +// (BUG-2820). Each function reaches a request body through one of the forms a +// NAME-matching parser could not see. The census must find every one; the +// function names say which form, and the test asserts on them by name. +package bodyreaders + +import ( + "context" + "io" + . "net/http" + nh "net/http" +) + +type holder struct{ req *nh.Request } + +type embeds struct{ *nh.Request } + +type aliasReq = nh.Request + +type ctxKey struct{} + +func StructField(h holder) { _, _ = io.ReadAll(h.req.Body) } + +func ContextValue(ctx context.Context) { + r := ctx.Value(ctxKey{}).(*nh.Request) + _, _ = io.ReadAll(r.Body) +} + +func TypeAlias(r *aliasReq) { _, _ = io.ReadAll(r.Body) } + +func EmbeddedField(e embeds) { _, _ = io.ReadAll(e.Body) } + +func EmbeddedMethod(e embeds) { _ = e.FormValue("x") } + +func ImportAlias(r *nh.Request) { _ = r.ParseForm() } + +func DotImport(r *Request) { _, _ = r.MultipartReader() } + +func VarDecl(r *nh.Request) { + var req = r + _, _ = io.ReadAll(req.Body) +} + +func CallDerived(ctx context.Context, r *nh.Request) { + req := r.WithContext(ctx) + _, _, _ = req.FormFile("f") +} + +func NamedResult() (r *nh.Request) { + _ = r.PostFormValue("x") + return r +} + +func RangeBinding(rs []*nh.Request) { + for _, r := range rs { + _ = r.PostForm + } +} + +func MethodValue(r *nh.Request) { f := r.ParseMultipartForm; _ = f(1) } + +// The cases the retired parser's own controls (TestBodyReaderScanDiscriminates) +// held it to, kept so the census answers them too. + +func DirectParam(w nh.ResponseWriter, r *nh.Request) { _, _ = io.ReadAll(r.Body) } + +func ClosureParam() func(*nh.Request) { + return func(hr *nh.Request) { _, _ = io.ReadAll(hr.Body) } +} + +func LocalAlias(orig *nh.Request) { + req := orig + _, _ = io.ReadAll(req.Body) +} + +// A VALUE copy still shares the Body: it is an interface holding the same +// reader. +func ValueCopy(r *nh.Request) { + c := *r + _, _ = io.ReadAll(c.Body) +} + +func ClosureValueParam(r *nh.Request) { + f := func(c nh.Request) { _, _ = io.ReadAll(c.Body) } + f(*r) +} + +func ReassignedWithContext(r *nh.Request) { + r = r.WithContext(context.Background()) + _, _ = io.ReadAll(r.Body) +} + +func MixedShortDecl(r *nh.Request) { + r, ok := getReq() + _ = ok + _, _ = io.ReadAll(r.Body) +} + +func getReq() (*nh.Request, bool) { return nil, false } + +func InsideSwitch(r *nh.Request) { + switch r.Method { + case "POST": + _, _ = io.ReadAll(r.Body) + } +} + +func AfterIfInitShadow(r *nh.Request) { + if r := 1; r > 0 { + _ = r + } + _, _ = io.ReadAll(r.Body) +} + +// NOT readers. The census must count none of these: a same-named field on +// another type, a request method that does not touch the body, and a Body on a +// Response. +type payload struct{ Body []byte } + +func NotARequest(p payload) int { return len(p.Body) } + +func HeaderOnly(r *nh.Request) string { return r.Header.Get("x") } + +func ResponseBody(resp *nh.Response) { _, _ = io.ReadAll(resp.Body) } + +// The parser OVER-FLAGGED these two on purpose (it could not model a scope that +// rebinds the name). The type checker resolves them exactly: neither `r` is a +// request here. +type other struct{ Body string } + +func ShadowedByClosureParam(r *nh.Request) int { + f := func(r other) int { return len(r.Body) } + return f(other{}) +} + +func ReboundInNestedBlock(r *nh.Request) int { + n := 0 + { + r := other{} + n = len(r.Body) + } + return n +} + +// This function does not read r.Body, it only talks about it. +func CommentOnly(r *nh.Request) int { return 1 }