diff --git a/cmd/linters/main.go b/cmd/linters/main.go index fd3486fe000..5b568945da9 100644 --- a/cmd/linters/main.go +++ b/cmd/linters/main.go @@ -28,6 +28,7 @@ import ( "github.com/github/gh-aw/pkg/linters/fprintlnsprintf" "github.com/github/gh-aw/pkg/linters/hardcodedfilepath" "github.com/github/gh-aw/pkg/linters/httpnoctx" + "github.com/github/gh-aw/pkg/linters/httpresponsebodyclose" "github.com/github/gh-aw/pkg/linters/jsonmarshalignoredeerror" "github.com/github/gh-aw/pkg/linters/largefunc" "github.com/github/gh-aw/pkg/linters/lenstringzero" @@ -61,6 +62,7 @@ func main() { fmterrorfnoverbs.Analyzer, hardcodedfilepath.Analyzer, httpnoctx.Analyzer, + httpresponsebodyclose.Analyzer, largefunc.Analyzer, manualmutexunlock.Analyzer, osexitinlibrary.Analyzer, diff --git a/docs/adr/40130-custom-linter-for-non-deferred-http-body-close.md b/docs/adr/40130-custom-linter-for-non-deferred-http-body-close.md new file mode 100644 index 00000000000..7087f7e18cd --- /dev/null +++ b/docs/adr/40130-custom-linter-for-non-deferred-http-body-close.md @@ -0,0 +1,42 @@ +# ADR-40130: Ship a Custom `httpresponsebodyclose` Linter for Non-Deferred HTTP Response Body Close + +**Date**: 2026-06-18 +**Status**: Draft + +## Context + +Calling `resp.Body.Close()` directly — rather than `defer resp.Body.Close()` — risks leaking the underlying connection: if the function returns early on a later error, panics, or gains a new `return` during maintenance, the close never runs. A scan of this repository (run #42) found a real occurrence at `pkg/cli/mcp_inspect_mcp_scripts_server.go:67`, where the body is closed inside an `if`-init assignment without a defer. No commonly enabled linter flags this: `go vet` does not track response-body lifetime, and the standard `bodyclose` linter is not part of this project's enabled set. The project maintains an in-repo `go/analysis` linter suite (`pkg/linters/*`, registered in `cmd/linters/main.go`) that is the established home for such repository-specific correctness guards. + +## Decision + +We will add a bespoke `go/analysis` analyzer, `httpresponsebodyclose`, to the project's custom linter suite rather than adopting an external linter. The analyzer walks each non-test `*ast.FuncDecl`, tracks variables whose type is `*net/http.Response` (verified via `types.Object` identity against `net/http`, not a syntactic name match), and records for each whether its `Body.Close()` is deferred versus called manually (either as a bare expression statement or captured in an assignment such as `closeErr := resp.Body.Close()`). When a response variable has a manual close but no deferred close, it reports a diagnostic at the response-acquisition site so the warning highlights where `defer resp.Body.Close()` should be added. It does not descend into function literals, so closures are analyzed independently to avoid false positives, and it is registered in the multichecker in `cmd/linters/main.go`. + +## Alternatives Considered + +### Alternative 1: Adopt the external `bodyclose` linter +The community `bodyclose` analyzer (via `golangci-lint`) detects unclosed response bodies. Rejected because this project deliberately curates a small, self-contained in-repo linter suite with shared `internal` helpers (`astutil`, `filecheck`); pulling in an external analyzer with broader, differently-scoped semantics would not compose with that suite and would target "never closed" rather than this project's specific "closed but not deferred" concern. + +### Alternative 2: Document the convention and rely on code review +A contributor guideline plus manual review requires no new code. Rejected because the early-return/panic leak is subtle and easy to miss — an instance already exists in the codebase — and manual review provides no repeatable, enforceable guard in CI. + +### Alternative 3: Syntactic detection by identifier name (`resp.Body.Close()`) +A simpler analyzer could flag any `Body.Close()` whose receiver is named `resp`. Rejected because it would miss response variables under other names and false-positive on unrelated types that happen to expose a `Body.Close()`; type-based `*net/http.Response` identity checking is more precise at modest extra complexity. + +## Consequences + +### Positive +- Catches a real, otherwise-undetected resource-leak pattern automatically in CI. +- Follows the established in-repo linter convention, mirroring `fileclosenotdeferred` in structure, so it composes with the existing `cmd/linters` multichecker and shared `internal` helpers. +- Precise: type-identity checking against `net/http.Response` targets genuine response bodies while ignoring same-named methods on unrelated types. + +### Negative +- Adds a custom analyzer the team must maintain, including AST handling for both expression-statement and assignment-captured close calls. +- Intra-procedural and pattern-bound: it only recognizes the `resp.Body.Close()` receiver shape and does not follow a body passed across function boundaries or aliased, so some manual-close leaks remain uncaught — which may create a false sense of full coverage. + +### Neutral +- Test files are excluded from analysis, matching the suite's existing conventions. +- The analyzer reports at the response-acquisition site, not at the later manual `Close()` call site. + +--- + +*This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/27789122704) workflow. The PR author must review, complete, and finalize this document before the PR can merge.* diff --git a/pkg/linters/README.md b/pkg/linters/README.md index ef563841a15..448d8a68f79 100644 --- a/pkg/linters/README.md +++ b/pkg/linters/README.md @@ -18,6 +18,7 @@ This package currently provides custom Go analyzers in the following subpackages - `fprintlnsprintf` — reports `fmt.Fprintln(..., fmt.Sprintf(...))` patterns and recommends direct formatting calls. - `hardcodedfilepath` — reports hard-coded file path string literals that match known path constants or should be extracted into named constants; also annotates paths that appear in log/print calls. - `httpnoctx` — reports HTTP client and package-level HTTP calls that do not accept a `context.Context`. +- `httpresponsebodyclose` — reports HTTP response `Body.Close()` calls that are not deferred immediately after a successful request. - `jsonmarshalignoredeerror` — reports `json.Marshal` and `json.Unmarshal` calls where the error return is discarded. - `largefunc` — reports function bodies that exceed a configurable line-count threshold. - `lenstringzero` — reports `len(s) == 0` / `len(s) != 0` comparisons on string values that should use `s == ""` / `s != ""`. @@ -55,6 +56,7 @@ This package currently provides custom Go analyzers in the following subpackages | `fprintlnsprintf` | Custom `go/analysis` analyzer that flags `fmt.Fprintln(..., fmt.Sprintf(...))` patterns | | `hardcodedfilepath` | Custom `go/analysis` analyzer that flags hard-coded file path string literals that match known path constants or should be extracted as named constants; annotates paths in log/print calls | | `httpnoctx` | Custom `go/analysis` analyzer that flags HTTP calls that do not accept a `context.Context` | +| `httpresponsebodyclose` | Custom `go/analysis` analyzer that flags HTTP response `Body.Close()` calls that are not deferred immediately | | `jsonmarshalignoredeerror` | Custom `go/analysis` analyzer that flags `json.Marshal`/`json.Unmarshal` calls where the error return is discarded | | `largefunc` | Custom `go/analysis` analyzer that flags large functions with actionable diagnostics | | `lenstringzero` | Custom `go/analysis` analyzer that flags `len(s) == 0` / `len(s) != 0` on string values that should use `s == ""` / `s != ""` | @@ -91,6 +93,7 @@ import ( "github.com/github/gh-aw/pkg/linters/execcommandwithoutcontext" "github.com/github/gh-aw/pkg/linters/fileclosenotdeferred" "github.com/github/gh-aw/pkg/linters/hardcodedfilepath" + "github.com/github/gh-aw/pkg/linters/httpresponsebodyclose" "github.com/github/gh-aw/pkg/linters/largefunc" "github.com/github/gh-aw/pkg/linters/lenstringzero" "github.com/github/gh-aw/pkg/linters/manualmutexunlock" @@ -110,6 +113,7 @@ _ = errstringmatch.Analyzer _ = execcommandwithoutcontext.Analyzer _ = fileclosenotdeferred.Analyzer _ = hardcodedfilepath.Analyzer +_ = httpresponsebodyclose.Analyzer _ = largefunc.Analyzer _ = lenstringzero.Analyzer _ = manualmutexunlock.Analyzer @@ -134,6 +138,7 @@ _ = ssljson.Analyzer - `github.com/github/gh-aw/pkg/linters/fmterrorfnoverbs` — fmt-errorf-no-verbs analyzer subpackage - `github.com/github/gh-aw/pkg/linters/fprintlnsprintf` — fprintln-sprintf analyzer subpackage - `github.com/github/gh-aw/pkg/linters/hardcodedfilepath` — hard-coded-file-path analyzer subpackage +- `github.com/github/gh-aw/pkg/linters/httpresponsebodyclose` — HTTP-response-body-close analyzer subpackage - `github.com/github/gh-aw/pkg/linters/jsonmarshalignoredeerror` — json-marshal-ignored-error analyzer subpackage - `github.com/github/gh-aw/pkg/linters/largefunc` — large-func analyzer subpackage - `github.com/github/gh-aw/pkg/linters/lenstringzero` — len-string-zero analyzer subpackage diff --git a/pkg/linters/doc.go b/pkg/linters/doc.go index 0bab5358155..8803302e20b 100644 --- a/pkg/linters/doc.go +++ b/pkg/linters/doc.go @@ -1,6 +1,6 @@ // Package linters is a namespace for gh-aw's custom Go analysis linters. // -// The actual analyzers are implemented in subpackages. All 29 active analyzers: +// The actual analyzers are implemented in subpackages. All 30 active analyzers: // // - contextcancelnotdeferred — flags context cancel functions called directly instead of deferred // - ctxbackground — flags context.Background() inside functions that already receive a context @@ -13,6 +13,7 @@ // - fmterrorfnoverbs — flags fmt.Errorf calls with no format verbs, recommending errors.New // - fprintlnsprintf — flags fmt.Fprintln(..., fmt.Sprintf(...)) patterns // - httpnoctx — flags HTTP calls that do not accept a context.Context +// - httpresponsebodyclose — flags HTTP response Body.Close() calls that are not deferred // - jsonmarshalignoredeerror — flags json.Marshal/Unmarshal calls where the error is discarded with _ // - largefunc — flags function bodies that exceed a configurable line-count threshold // - lenstringzero — flags len(s) == 0 / len(s) != 0 on string values that should use s == "" / s != "" diff --git a/pkg/linters/httpresponsebodyclose/httpresponsebodyclose.go b/pkg/linters/httpresponsebodyclose/httpresponsebodyclose.go new file mode 100644 index 00000000000..95d451bb35f --- /dev/null +++ b/pkg/linters/httpresponsebodyclose/httpresponsebodyclose.go @@ -0,0 +1,202 @@ +// Package httpresponsebodyclose implements a Go analysis linter that flags +// HTTP response bodies that are closed manually instead of via defer. +package httpresponsebodyclose + +import ( + "go/ast" + "go/token" + "go/types" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/inspect" + + "github.com/github/gh-aw/pkg/linters/internal/astutil" + "github.com/github/gh-aw/pkg/linters/internal/filecheck" +) + +// Analyzer is the http-response-body-close analysis pass. +var Analyzer = &analysis.Analyzer{ + Name: "httpresponsebodyclose", + Doc: "reports HTTP response Body.Close() calls that are not deferred, which can cause resource leaks on early return or panic", + URL: "https://github.com/github/gh-aw/tree/main/pkg/linters/httpresponsebodyclose", + Requires: []*analysis.Analyzer{inspect.Analyzer}, + Run: run, +} + +func run(pass *analysis.Pass) (any, error) { + insp, err := astutil.Inspector(pass) + if err != nil { + return nil, err + } + + nodeFilter := []ast.Node{ + (*ast.FuncDecl)(nil), + } + + insp.Preorder(nodeFilter, func(n ast.Node) { + inspectHTTPResponseFuncDecl(pass, n) + }) + + return nil, nil +} + +type responseVarState struct { + assignPos token.Pos + hasDefer bool + hasManualClose bool +} + +func inspectHTTPResponseFuncDecl(pass *analysis.Pass, n ast.Node) { + fn, ok := n.(*ast.FuncDecl) + if !ok || fn.Body == nil { + return + } + + pos := pass.Fset.PositionFor(fn.Pos(), false) + if filecheck.IsTestFile(pos.Filename) { + return + } + + responseVars := make(map[types.Object]*responseVarState) + + ast.Inspect(fn.Body, func(node ast.Node) bool { + return analyzeResponseNode(pass, responseVars, node) + }) + + for _, state := range responseVars { + if state.hasManualClose && !state.hasDefer { + pass.Report(analysis.Diagnostic{ + Pos: state.assignPos, + Message: "HTTP response Body.Close() should be deferred immediately after error check to prevent resource leaks", + }) + } + } +} + +func analyzeResponseNode(pass *analysis.Pass, responseVars map[types.Object]*responseVarState, node ast.Node) bool { + if node == nil { + return false + } + + // Do not descend into function literals — closures are independent execution + // contexts and should be analyzed separately to avoid false positives. + if _, ok := node.(*ast.FuncLit); ok { + return false + } + + if assign, ok := node.(*ast.AssignStmt); ok { + trackResponseAssignment(pass, responseVars, assign) + } + + if deferStmt, ok := node.(*ast.DeferStmt); ok { + if obj := getHTTPBodyCloseObject(pass, deferStmt.Call); obj != nil { + if state, found := responseVars[obj]; found { + state.hasDefer = true + } + } + } + + // Non-deferred resp.Body.Close() in expression statements. + if exprStmt, ok := node.(*ast.ExprStmt); ok { + if call, ok := exprStmt.X.(*ast.CallExpr); ok { + markManualBodyClose(pass, responseVars, call) + } + } + + // Non-deferred resp.Body.Close() captured in an assignment (e.g. closeErr := resp.Body.Close()). + if assign, ok := node.(*ast.AssignStmt); ok { + for _, rhs := range assign.Rhs { + if call, ok := rhs.(*ast.CallExpr); ok { + markManualBodyClose(pass, responseVars, call) + } + } + } + + return true +} + +func trackResponseAssignment(pass *analysis.Pass, responseVars map[types.Object]*responseVarState, assign *ast.AssignStmt) { + // Track any binding whose LHS has type *net/http.Response. Unlike + // fileclosenotdeferred (which tracks explicit os.Open/os.Create call sites), + // this intentionally keys on the assigned variable's type so responses + // returned by helper functions are also covered. + for _, lhs := range assign.Lhs { + ident, ok := lhs.(*ast.Ident) + if !ok || ident.Name == "_" { + continue + } + obj := pass.TypesInfo.ObjectOf(ident) + if obj == nil { + continue + } + if !isHTTPResponseType(obj.Type()) { + continue + } + if prev, exists := responseVars[obj]; exists && prev.hasManualClose && !prev.hasDefer { + pass.Report(analysis.Diagnostic{ + Pos: prev.assignPos, + Message: "HTTP response Body.Close() should be deferred immediately after error check to prevent resource leaks", + }) + } + assignPos := ident.Pos() + if len(assign.Rhs) == 1 { + if call, ok := assign.Rhs[0].(*ast.CallExpr); ok { + assignPos = call.Pos() + } + } + responseVars[obj] = &responseVarState{assignPos: assignPos} + } +} + +func markManualBodyClose(pass *analysis.Pass, responseVars map[types.Object]*responseVarState, call *ast.CallExpr) { + obj := getHTTPBodyCloseObject(pass, call) + if obj == nil { + return + } + if state, found := responseVars[obj]; found { + state.hasManualClose = true + } +} + +// getHTTPBodyCloseObject returns the types.Object for the *http.Response variable +// in a resp.Body.Close() call, or nil if the call does not match that pattern. +// Known limitation: body aliasing (body := resp.Body; body.Close()) is not +// detected because the selector chain no longer starts from the *http.Response +// variable directly. +func getHTTPBodyCloseObject(pass *analysis.Pass, call *ast.CallExpr) types.Object { + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok || sel.Sel.Name != "Close" { + return nil + } + // The receiver must be an expression of the form resp.Body. + bodyExpr, ok := sel.X.(*ast.SelectorExpr) + if !ok || bodyExpr.Sel.Name != "Body" { + return nil + } + ident, ok := bodyExpr.X.(*ast.Ident) + if !ok { + return nil + } + obj := pass.TypesInfo.ObjectOf(ident) + if obj == nil { + return nil + } + if !isHTTPResponseType(obj.Type()) { + return nil + } + return obj +} + +// isHTTPResponseType reports whether t is *net/http.Response. +func isHTTPResponseType(t types.Type) bool { + ptr, ok := t.(*types.Pointer) + if !ok { + return false + } + named, ok := ptr.Elem().(*types.Named) + if !ok { + return false + } + obj := named.Obj() + return obj.Name() == "Response" && obj.Pkg() != nil && obj.Pkg().Path() == "net/http" +} diff --git a/pkg/linters/httpresponsebodyclose/httpresponsebodyclose_test.go b/pkg/linters/httpresponsebodyclose/httpresponsebodyclose_test.go new file mode 100644 index 00000000000..c18820b5915 --- /dev/null +++ b/pkg/linters/httpresponsebodyclose/httpresponsebodyclose_test.go @@ -0,0 +1,16 @@ +//go:build !integration + +package httpresponsebodyclose_test + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" + + "github.com/github/gh-aw/pkg/linters/httpresponsebodyclose" +) + +func TestHTTPResponseBodyClose(t *testing.T) { + testdata := analysistest.TestData() + analysistest.Run(t, testdata, httpresponsebodyclose.Analyzer, "httpresponsebodyclose") +} diff --git a/pkg/linters/httpresponsebodyclose/testdata/src/httpresponsebodyclose/httpresponsebodyclose.go b/pkg/linters/httpresponsebodyclose/testdata/src/httpresponsebodyclose/httpresponsebodyclose.go new file mode 100644 index 00000000000..4471e2385cf --- /dev/null +++ b/pkg/linters/httpresponsebodyclose/testdata/src/httpresponsebodyclose/httpresponsebodyclose.go @@ -0,0 +1,98 @@ +package httpresponsebodyclose + +import ( + "io" + "net/http" +) + +// flagged: Body.Close() called manually, not deferred. +func fetchManualClose(url string) error { + req, _ := http.NewRequest(http.MethodGet, url, nil) + resp, err := http.DefaultClient.Do(req) // want `HTTP response Body\.Close\(\) should be deferred immediately after error check to prevent resource leaks` + if err != nil { + return err + } + _, _ = io.ReadAll(resp.Body) + resp.Body.Close() + return nil +} + +// not flagged: Body.Close() is deferred correctly. +func fetchDeferred(url string) error { + req, _ := http.NewRequest(http.MethodGet, url, nil) + resp, err := http.DefaultClient.Do(req) + if err != nil { + return err + } + defer resp.Body.Close() + _, _ = io.ReadAll(resp.Body) + return nil +} + +// flagged: Body.Close() return value captured but not deferred. +func fetchManualCloseWithErr(url string) error { + req, _ := http.NewRequest(http.MethodGet, url, nil) + resp, err := http.DefaultClient.Do(req) // want `HTTP response Body\.Close\(\) should be deferred immediately after error check to prevent resource leaks` + if err != nil { + return err + } + _, _ = io.ReadAll(resp.Body) + closeErr := resp.Body.Close() + return closeErr +} + +// flagged: Body.Close() captured in if-init, not deferred. +func fetchIfInitClose(url string) error { + req, _ := http.NewRequest(http.MethodGet, url, nil) + resp, err := http.DefaultClient.Do(req) // want `HTTP response Body\.Close\(\) should be deferred immediately after error check to prevent resource leaks` + if err != nil { + return err + } + if closeErr := resp.Body.Close(); closeErr != nil { + return closeErr + } + return nil +} + +// not flagged: closure is analyzed independently and its defer must not be +// attributed to the outer function. +func fetchInClosure(url string) { + fn := func() error { + resp, err := http.DefaultClient.Get(url) + if err != nil { + return err + } + defer resp.Body.Close() + _, _ = io.ReadAll(resp.Body) + return nil + } + _ = fn() +} + +// flagged: same response variable reassigned after a manual close; the first +// response acquisition must still be reported. +func fetchReassignAfterManualClose(url string) error { + resp, err := http.DefaultClient.Get(url) // want `HTTP response Body\.Close\(\) should be deferred immediately after error check to prevent resource leaks` + if err != nil { + return err + } + resp.Body.Close() + resp, err = http.DefaultClient.Get(url + "/retry") + if err != nil { + return err + } + defer resp.Body.Close() + _, _ = io.ReadAll(resp.Body) + return nil +} + +// not flagged: no Body.Close() call at all (different concern, out of scope). +func fetchNoClose(url string) error { + req, _ := http.NewRequest(http.MethodGet, url, nil) + resp, err := http.DefaultClient.Do(req) + if err != nil { + return err + } + _, _ = io.ReadAll(resp.Body) + return nil +} diff --git a/pkg/linters/spec_test.go b/pkg/linters/spec_test.go index f53401eb72d..c485873d1fe 100644 --- a/pkg/linters/spec_test.go +++ b/pkg/linters/spec_test.go @@ -23,6 +23,7 @@ import ( "github.com/github/gh-aw/pkg/linters/fprintlnsprintf" "github.com/github/gh-aw/pkg/linters/hardcodedfilepath" "github.com/github/gh-aw/pkg/linters/httpnoctx" + "github.com/github/gh-aw/pkg/linters/httpresponsebodyclose" "github.com/github/gh-aw/pkg/linters/jsonmarshalignoredeerror" "github.com/github/gh-aw/pkg/linters/largefunc" "github.com/github/gh-aw/pkg/linters/lenstringzero" @@ -55,15 +56,16 @@ type docAnalyzer struct { } // documentedAnalyzers returns the analyzer subpackages documented in the README -// "Public API > Subpackages" table. The README documents 29 analyzer +// "Public API > Subpackages" table. The README documents 30 analyzer // subpackages (the non-analyzer `internal` helper subpackage is excluded because // it exposes no Analyzer). // // Spec (README "Public API > Subpackages"): // // contextcancelnotdeferred, ctxbackground, errorfwrapv, excessivefuncparams, errormessage, -// errstringmatch, execcommandwithoutcontext, fileclosenotdeferred, fmterrorfnoverbs, fprintlnsprintf, -// hardcodedfilepath, httpnoctx, jsonmarshalignoredeerror, largefunc, lenstringzero, +// errstringmatch, execcommandwithoutcontext, fileclosenotdeferred, fmterrorfnoverbs, +// fprintlnsprintf, hardcodedfilepath, httpnoctx, httpresponsebodyclose, +// jsonmarshalignoredeerror, largefunc, lenstringzero, // manualmutexunlock, osexitinlibrary, ossetenvlibrary, panic-in-library-code, rawloginlib, // regexpcompileinfunction, seenmapbool, sortslice, ssljson, strconvparseignorederror, // timeafterleak, timesleepnocontext, tolowerequalfold, uncheckedtypeassertion @@ -81,6 +83,7 @@ func documentedAnalyzers() []docAnalyzer { {"fprintlnsprintf", fprintlnsprintf.Analyzer}, {"hardcodedfilepath", hardcodedfilepath.Analyzer}, {"httpnoctx", httpnoctx.Analyzer}, + {"httpresponsebodyclose", httpresponsebodyclose.Analyzer}, {"jsonmarshalignoredeerror", jsonmarshalignoredeerror.Analyzer}, {"largefunc", largefunc.Analyzer}, {"lenstringzero", lenstringzero.Analyzer},