gitforge/build/lint-single-response/singleresponse.go
Mathieu Fenniak 7b5d623737 fix: introduce lint-single-response to prevent control flow continuing past a ctx.Error(...)-style method (#13087)
This PR adds a new linter to the codebase and addresses all the problems that it identified (including a small number of false positives).  The lint-single-response Go analyzer attempts to prevent a common problem in Forgejo where it is possible for a web handler to provide a response to a request, and then continue code execution unintentionally.  For example:

```go
err := json.Unmarshal(data, &claims)
if err != nil {
    ctx.Error(http.StatusInternalServerError, "Error in unmarshal", err)
    // Oops, I forgot to `return` here...
}
// ... more work occurs ...
ctx.JSON(http.StatusOK, resp)
```

In order to detect these cases, lint-single-response contains a list of functions that deliver a web response.  When any of those functions are used within a function, the control flow must not perform any work after the function is invoked -- it can only return and exit the function.

### Tests for Go changes

- I added test coverage for Go changes...
  - [x] in their respective `*_test.go` for unit tests.
  - [ ] in the `tests/integration` directory if it involves interactions with a live Forgejo server.
- I ran...
  - [x] `make pr-go` before pushing

### Documentation

- [x] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change.
    - Documentation on the new linter is included inline, in `build/lint-single-response/README.md`.
- [ ] I did not document these changes and I do not expect someone else to do it.

### Release notes

- [ ] This change will be noticed by a Forgejo user or admin (feature, bug fix, performance, etc.). I suggest to include a release note for this change.
- [x] This change is not visible to a Forgejo user or admin (refactor, dependency upgrade, etc.). I think there is no need to add a release note for this change.

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/13087
Reviewed-by: Andreas Ahlenstorf <aahlenst@noreply.codeberg.org>
2026-06-14 17:06:03 +02:00

218 lines
7.4 KiB
Go

// Copyright 2026 The Forgejo Authors. All rights reserved.
// SPDX-License-Identifier: GPL-3.0-or-later
package singleresponse
import (
"fmt"
"go/ast"
"go/types"
"strings"
"golang.org/x/tools/go/analysis"
"golang.org/x/tools/go/analysis/passes/ctrlflow"
"golang.org/x/tools/go/analysis/passes/inspect"
"golang.org/x/tools/go/ast/inspector"
"golang.org/x/tools/go/cfg"
)
var Analyzer = &analysis.Analyzer{
Name: "singleresponse",
Doc: "checks that Forgejo web response methods are only invoked once in a control flow",
Requires: []*analysis.Analyzer{inspect.Analyzer, ctrlflow.Analyzer},
Run: run,
}
func run(pass *analysis.Pass) (any, error) {
insp := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
cfgs := pass.ResultOf[ctrlflow.Analyzer].(*ctrlflow.CFGs)
webFuncs := map[string]map[string]any{
"*forgejo.org/services/context.APIContext": {
"Error": true,
"InternalServerError": true,
"NotFound": true,
"NotFoundOrServerError": true,
"ServerError": true,
},
"*forgejo.org/services/context.Base": {
"Error": true,
"JSON": true,
"JSONWithContentType": true,
"PlainText": true,
"PlainTextBytes": true,
"Redirect": true,
"ServeContent": true,
},
"*forgejo.org/services/context.Context": {
"HTML": true,
"JSONError": true,
"JSONOK": true,
"JSONRedirect": true,
"JSONTemplate": true,
"NotFound": true,
"NotFoundOrServerError": true,
"RedirectToFirst": true,
"RenderWithErr": true,
"ServerError": true,
},
// Future: RedirectToUser does not accept a ctx LHS, but rather a first parameter -- needs different
// implementation of detection, or, refactoring: "RedirectToUser": true,
}
insp.Nodes([]ast.Node{
(*ast.FuncDecl)(nil),
(*ast.FuncLit)(nil),
}, func(n ast.Node, push bool) bool {
switch fn := n.(type) {
case *ast.FuncDecl:
// Skip test methods which are assumed to know what they're doing.
if strings.HasPrefix(fn.Name.Name, "Test") {
return false
}
cfg := cfgs.FuncDecl(fn)
if cfg == nil {
return true
}
inspectFunction(cfg, pass, webFuncs)
case *ast.FuncLit:
cfg := cfgs.FuncLit(fn)
if cfg == nil {
return true
}
inspectFunction(cfg, pass, webFuncs)
}
return false
})
return nil, nil //nolint:nilnil
}
func inspectFunction(cfg *cfg.CFG, pass *analysis.Pass, webFuncs map[string]map[string]any) {
for _, block := range cfg.Blocks {
for nodeIdx, node := range block.Nodes {
ast.Inspect(node, func(n ast.Node) bool {
// Don't recurse inside of a function literal inside of a function declaration, as this isn't
// related to the control flow that we're currently iterating through.
_, isFuncLit := n.(*ast.FuncLit)
if isFuncLit {
return false
}
call, isCall := n.(*ast.CallExpr)
if !isCall {
return true
}
// SelectorExpr: "an expression followed by a selector", like "ctx.Error". All the functions
// we're interested in match this pattern.
selector, isSelector := call.Fun.(*ast.SelectorExpr)
if !isSelector {
return false
}
// We almost get the right information easily from the selector by using
// pass.TypesInfo.Uses[selector.X] -- but that will be the type of the variable that we're
// invoking a method on, and not the type of the method receiver. eg. on `ctx
// *context.Context`, `ctx.ServerError(...)` will always be `*context.Context`, even if
// `ServerError` is actually implemented on `*context.Base`.
//
// We need to dig a little deeper here to get the function type, then its signature, and then
// it's receiver type, and we'll really have the method that will be invoked rather than just
// the variable that it is called upon.
selection, hasSelection := pass.TypesInfo.Selections[selector]
if !hasSelection {
return false
}
objFn, ok := selection.Obj().(*types.Func)
if !ok {
return false
}
fnSig, ok := objFn.Type().(*types.Signature)
if !ok {
return false
}
callType := fnSig.Recv().Type().String()
typeMap, inTypeMap := webFuncs[callType]
if inTypeMap {
callName := selector.Sel.Name
_, inFuncMap := typeMap[callName]
if inFuncMap {
// OK... we've found a call to a terminating function at
// cfg.Blocks[blockIdx].Nodes[nodeIdx].
trace := false
// For code-time debugging/analysis, set trace=true when digging into why something isn't
// working:
// if callName == "InternalServerError" {
// trace = true
// }
sketchy := inspectCallSite(block, nodeIdx, trace)
if sketchy != nil {
pass.Reportf(node.Pos(), "Invocation of %s / %s, and control flow continues afterwards.", callType, callName)
}
}
}
return false
})
}
}
}
type sketchyCall struct{}
func inspectCallSite(callingBlock *cfg.Block, callingNodeIndex int, trace bool) *sketchyCall {
// Inspect the remainder of the block passed in, after callingNodeIndex, for "bad" statements
if trace {
println("remainder of block...")
}
for _, nextStmt := range callingBlock.Nodes[callingNodeIndex+1:] {
if trace {
println(fmt.Sprintf("\tnextStmt = %#v", nextStmt))
}
// Only `return` is permitted after one of the web return functions; maybe this needs to expand in the future
// but haven't identified any cases in Forgejo yet.
_, stmtOk := nextStmt.(*ast.ReturnStmt)
if !stmtOk {
if trace {
println(fmt.Sprintf("\tfound sketchy statement = %#v", nextStmt))
}
// Future: add information about what was following the call, so that the diagnostic can be more specific
// about the problematic next statement identified... but so far it seems pretty easy to analyze and fix.
return &sketchyCall{}
}
}
if trace {
println("nothing found in remainder of block")
println(fmt.Sprintf("%d Succs blocks will be investigated", len(callingBlock.Succs)))
}
// Now, assuming that there was nothing problematic found in the remainder of the block, use the control-flow graph
// to identify where code execution would continue and see if there's anything inappropriate in it.
//
// https://pkg.go.dev/golang.org/x/tools@v0.46.0/go/cfg#Block -> A block may have 0-2 successors: zero for a return
// block or a block that calls a function such as panic that never returns; one for a normal (jump) block; and two
// for a conditional (if) block.
//
// It's possible for the next block to have either no nodes, or, no nodes that continue to do work and trigger
// detection... but then to proceed into *another* block that does. So this investigation has to be done
// recursively. Control-flow graph should prevent us from needing to stop this recursive detection; we'll hit a
// return statement or end of function and that's the end of the CFG, and that's also the time we'd want to stop
// looking, so no additional exit logic should be needed.
for i, succ := range callingBlock.Succs {
if trace {
println(fmt.Sprintf("Succs[%d], block index %d, recursing:", i, succ.Index))
}
// `-1` is used to start at index 0 in the nodes.
sketchy := inspectCallSite(succ, -1, trace)
if trace {
println(fmt.Sprintf("Succs[%d], block index %d, had sketchy = %#v", i, succ.Index, sketchy))
}
if sketchy != nil {
return sketchy
}
}
return nil
}