Skip to content

httpnoctx: enclosing-scope walk crosses non-go/defer FuncLit boundaries (same bug class already fixed in sibling ctx-family lint [Content truncated due to length] #52627

Description

@github-actions

Problem

httpnoctx.hasContextInEnclosingFunc (pkg/linters/httpnoctx/httpnoctx.go:146-158) walks every lexically enclosing *ast.FuncDecl/*ast.FuncLit looking for a context.Context parameter, and stops at the first one it finds — without ever checking whether it has crossed a plain (non-go/defer) closure boundary on the way:

func hasContextInEnclosingFunc(pass *analysis.Pass, cursor inspector.Cursor) bool {
	for enclosing := range cursor.Enclosing((*ast.FuncDecl)(nil), (*ast.FuncLit)(nil)) {
		fnType := astutil.EnclosingFuncType(enclosing.Node())
		if fnType == nil || fnType.Params == nil {
			continue
		}
		if _, ok := astutil.ContextParamName(pass, fnType); ok {
			return true
		}
	}
	return false
}

This is the exact walk shape (cursor.Enclosing((*ast.FuncDecl)(nil), (*ast.FuncLit)(nil))) that every other scope-sensitive linter in this repo has already had audited and fixed for FuncLit-boundary crossing:

httpnoctx is the one CI-enforced (cgo.yml LINTER_FLAGS) member of this family that never received the corresponding fix — IsGoOrDeferClosure is not referenced anywhere in httpnoctx.go.

Impact

Because the loop never breaks at a plain closure boundary, httpnoctx will attribute an unrelated outer function's context.Context parameter to code inside a synchronous callback closure, e.g.:

func Serve(ctx context.Context) http.Handler {
	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		req, _ := http.NewRequest(http.MethodGet, upstreamURL, nil) // flagged
	})
}

hasContextInEnclosingFunc finds Serve's ctx parameter (the http.HandlerFunc literal itself has no context param, and the walk doesn't stop there) and fires "http.NewRequest does not propagate context; use http.NewRequestWithContext when context.Context is in scope." The suggested remediation is actively wrong: the correct per-request context here is r.Context(), not the handler-construction-time ctx captured from Serve, which may already be long gone by the time the handler executes. This is the same false-positive/misleading-fix class documented for execcommandwithoutcontext in #43683, just never patched here.

There is no current production trigger (no http.NewRequest/http.Get/http.Post call sites under pkg/ outside httpnoctx's own testdata), so this is latent rather than actively firing — matching the precedent of similarly-filed latent bugs (e.g. #44187 writebytestring, filed and fixed before it had a production hit).

Evidence

  • pkg/linters/httpnoctx/httpnoctx.go:146-158 — the unguarded walk.
  • pkg/linters/httpnoctx/testdata/src/httpnoctx/httpnoctx.go — the entire test fixture has zero FuncLit/closure test cases (no goroutine, no defer, no plain closure), so the gap has no test coverage in either direction.
  • pkg/linters/execcommandwithoutcontext/execcommandwithoutcontext.go:68-79 and pkg/linters/timesleepnocontext/timesleepnocontext.go:59-70 — sibling linters with the identical walk shape, already carrying the astutil.IsGoOrDeferClosure guard.

Recommendation

Add the same boundary check used by execcommandwithoutcontext/timesleepnocontext: when the current enclosing node is a *ast.FuncLit with no context param, and it is not the direct callee of a go/defer statement (!astutil.IsGoOrDeferClosure(enclosing)), stop the walk instead of continuing to the next-outer scope.

Before:

for enclosing := range cursor.Enclosing((*ast.FuncDecl)(nil), (*ast.FuncLit)(nil)) {
	fnType := astutil.EnclosingFuncType(enclosing.Node())
	if fnType == nil || fnType.Params == nil {
		continue
	}
	if _, ok := astutil.ContextParamName(pass, fnType); ok {
		return true
	}
}
return false

After (mirroring execcommandwithoutcontext.go:68-79):

for enclosing := range cursor.Enclosing((*ast.FuncDecl)(nil), (*ast.FuncLit)(nil)) {
	fnType := astutil.EnclosingFuncType(enclosing.Node())
	if fnType == nil {
		continue
	}
	if _, ok := astutil.ContextParamName(pass, fnType); ok {
		return true
	}
	if _, isFuncLit := enclosing.Node().(*ast.FuncLit); isFuncLit && !astutil.IsGoOrDeferClosure(enclosing) {
		return false
	}
}
return false

Validation checklist

  • Add a testdata case: a plain (non-go/defer) FuncLit with no context param, nested inside a context-aware outer function, calling http.NewRequest — must NOT be flagged.
  • Add a testdata case: the same shape but the closure is the direct callee of go/defer — current (pre-fix) behavior of flagging should be preserved/covered explicitly.
  • Re-run golint-custom -httpnoctx (or the project's lint task) over ./pkg/... and ./cmd/... to confirm zero behavior change on real call sites (none exist today, so this should be a no-op on production code).

Effort

Small — the fix is a ~4-line change mirroring an already-landed pattern (execcommandwithoutcontext.go:76-78), plus two new testdata fixtures.

Generated by 🤖 Sergo - Serena Go Expert · agent · 264.1 AIC · ⌖ 30.1 AIC · ⊞ 5.9K ·

  • expires on Aug 20, 2026, 8:37 PM UTC-08:00

Metadata

Metadata

Assignees

No one assigned

    Labels

    cookieIssue Monster Loves Cookies!sergo

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions