|
| 1 | +// Copyright 2026 The Forgejo Authors. All rights reserved. |
| 2 | +// SPDX-License-Identifier: GPL-3.0-or-later |
| 3 | + |
| 4 | +package singleresponse |
| 5 | + |
| 6 | +import ( |
| 7 | + "fmt" |
| 8 | + "go/ast" |
| 9 | + "go/types" |
| 10 | + "strings" |
| 11 | + |
| 12 | + "golang.org/x/tools/go/analysis" |
| 13 | + "golang.org/x/tools/go/analysis/passes/ctrlflow" |
| 14 | + "golang.org/x/tools/go/analysis/passes/inspect" |
| 15 | + "golang.org/x/tools/go/ast/inspector" |
| 16 | + "golang.org/x/tools/go/cfg" |
| 17 | +) |
| 18 | + |
| 19 | +var Analyzer = &analysis.Analyzer{ |
| 20 | + Name: "singleresponse", |
| 21 | + Doc: "checks that Forgejo web response methods are only invoked once in a control flow", |
| 22 | + Requires: []*analysis.Analyzer{inspect.Analyzer, ctrlflow.Analyzer}, |
| 23 | + Run: run, |
| 24 | +} |
| 25 | + |
| 26 | +func run(pass *analysis.Pass) (any, error) { |
| 27 | + insp := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) |
| 28 | + cfgs := pass.ResultOf[ctrlflow.Analyzer].(*ctrlflow.CFGs) |
| 29 | + |
| 30 | + webFuncs := map[string]map[string]any{ |
| 31 | + "*forgejo.org/services/context.APIContext": { |
| 32 | + "Error": true, |
| 33 | + "InternalServerError": true, |
| 34 | + "NotFound": true, |
| 35 | + "NotFoundOrServerError": true, |
| 36 | + "ServerError": true, |
| 37 | + }, |
| 38 | + "*forgejo.org/services/context.Base": { |
| 39 | + "Error": true, |
| 40 | + "JSON": true, |
| 41 | + "JSONWithContentType": true, |
| 42 | + "PlainText": true, |
| 43 | + "PlainTextBytes": true, |
| 44 | + "Redirect": true, |
| 45 | + "ServeContent": true, |
| 46 | + }, |
| 47 | + "*forgejo.org/services/context.Context": { |
| 48 | + "HTML": true, |
| 49 | + "JSONError": true, |
| 50 | + "JSONOK": true, |
| 51 | + "JSONRedirect": true, |
| 52 | + "JSONTemplate": true, |
| 53 | + "NotFound": true, |
| 54 | + "NotFoundOrServerError": true, |
| 55 | + "RedirectToFirst": true, |
| 56 | + "RenderWithErr": true, |
| 57 | + "ServerError": true, |
| 58 | + }, |
| 59 | + // Future: RedirectToUser does not accept a ctx LHS, but rather a first parameter -- needs different |
| 60 | + // implementation of detection, or, refactoring: "RedirectToUser": true, |
| 61 | + } |
| 62 | + |
| 63 | + insp.Nodes([]ast.Node{ |
| 64 | + (*ast.FuncDecl)(nil), |
| 65 | + (*ast.FuncLit)(nil), |
| 66 | + }, func(n ast.Node, push bool) bool { |
| 67 | + switch fn := n.(type) { |
| 68 | + case *ast.FuncDecl: |
| 69 | + // Skip test methods which are assumed to know what they're doing. |
| 70 | + if strings.HasPrefix(fn.Name.Name, "Test") { |
| 71 | + return false |
| 72 | + } |
| 73 | + cfg := cfgs.FuncDecl(fn) |
| 74 | + if cfg == nil { |
| 75 | + return true |
| 76 | + } |
| 77 | + inspectFunction(cfg, pass, webFuncs) |
| 78 | + case *ast.FuncLit: |
| 79 | + cfg := cfgs.FuncLit(fn) |
| 80 | + if cfg == nil { |
| 81 | + return true |
| 82 | + } |
| 83 | + inspectFunction(cfg, pass, webFuncs) |
| 84 | + } |
| 85 | + return false |
| 86 | + }) |
| 87 | + |
| 88 | + return nil, nil //nolint:nilnil |
| 89 | +} |
| 90 | + |
| 91 | +func inspectFunction(cfg *cfg.CFG, pass *analysis.Pass, webFuncs map[string]map[string]any) { |
| 92 | + for _, block := range cfg.Blocks { |
| 93 | + for nodeIdx, node := range block.Nodes { |
| 94 | + ast.Inspect(node, func(n ast.Node) bool { |
| 95 | + // Don't recurse inside of a function literal inside of a function declaration, as this isn't |
| 96 | + // related to the control flow that we're currently iterating through. |
| 97 | + _, isFuncLit := n.(*ast.FuncLit) |
| 98 | + if isFuncLit { |
| 99 | + return false |
| 100 | + } |
| 101 | + |
| 102 | + call, isCall := n.(*ast.CallExpr) |
| 103 | + if !isCall { |
| 104 | + return true |
| 105 | + } |
| 106 | + |
| 107 | + // SelectorExpr: "an expression followed by a selector", like "ctx.Error". All the functions |
| 108 | + // we're interested in match this pattern. |
| 109 | + selector, isSelector := call.Fun.(*ast.SelectorExpr) |
| 110 | + if !isSelector { |
| 111 | + return false |
| 112 | + } |
| 113 | + |
| 114 | + // We almost get the right information easily from the selector by using |
| 115 | + // pass.TypesInfo.Uses[selector.X] -- but that will be the type of the variable that we're |
| 116 | + // invoking a method on, and not the type of the method receiver. eg. on `ctx |
| 117 | + // *context.Context`, `ctx.ServerError(...)` will always be `*context.Context`, even if |
| 118 | + // `ServerError` is actually implemented on `*context.Base`. |
| 119 | + // |
| 120 | + // We need to dig a little deeper here to get the function type, then its signature, and then |
| 121 | + // it's receiver type, and we'll really have the method that will be invoked rather than just |
| 122 | + // the variable that it is called upon. |
| 123 | + selection, hasSelection := pass.TypesInfo.Selections[selector] |
| 124 | + if !hasSelection { |
| 125 | + return false |
| 126 | + } |
| 127 | + objFn, ok := selection.Obj().(*types.Func) |
| 128 | + if !ok { |
| 129 | + return false |
| 130 | + } |
| 131 | + fnSig, ok := objFn.Type().(*types.Signature) |
| 132 | + if !ok { |
| 133 | + return false |
| 134 | + } |
| 135 | + callType := fnSig.Recv().Type().String() |
| 136 | + |
| 137 | + typeMap, inTypeMap := webFuncs[callType] |
| 138 | + if inTypeMap { |
| 139 | + callName := selector.Sel.Name |
| 140 | + _, inFuncMap := typeMap[callName] |
| 141 | + if inFuncMap { |
| 142 | + // OK... we've found a call to a terminating function at |
| 143 | + // cfg.Blocks[blockIdx].Nodes[nodeIdx]. |
| 144 | + trace := false |
| 145 | + // For code-time debugging/analysis, set trace=true when digging into why something isn't |
| 146 | + // working: |
| 147 | + // if callName == "InternalServerError" { |
| 148 | + // trace = true |
| 149 | + // } |
| 150 | + sketchy := inspectCallSite(block, nodeIdx, trace) |
| 151 | + if sketchy != nil { |
| 152 | + pass.Reportf(node.Pos(), "Invocation of %s / %s, and control flow continues afterwards.", callType, callName) |
| 153 | + } |
| 154 | + } |
| 155 | + } |
| 156 | + |
| 157 | + return false |
| 158 | + }) |
| 159 | + } |
| 160 | + } |
| 161 | +} |
| 162 | + |
| 163 | +type sketchyCall struct{} |
| 164 | + |
| 165 | +func inspectCallSite(callingBlock *cfg.Block, callingNodeIndex int, trace bool) *sketchyCall { |
| 166 | + // Inspect the remainder of the block passed in, after callingNodeIndex, for "bad" statements |
| 167 | + if trace { |
| 168 | + println("remainder of block...") |
| 169 | + } |
| 170 | + for _, nextStmt := range callingBlock.Nodes[callingNodeIndex+1:] { |
| 171 | + if trace { |
| 172 | + println(fmt.Sprintf("\tnextStmt = %#v", nextStmt)) |
| 173 | + } |
| 174 | + // Only `return` is permitted after one of the web return functions; maybe this needs to expand in the future |
| 175 | + // but haven't identified any cases in Forgejo yet. |
| 176 | + _, stmtOk := nextStmt.(*ast.ReturnStmt) |
| 177 | + if !stmtOk { |
| 178 | + if trace { |
| 179 | + println(fmt.Sprintf("\tfound sketchy statement = %#v", nextStmt)) |
| 180 | + } |
| 181 | + // Future: add information about what was following the call, so that the diagnostic can be more specific |
| 182 | + // about the problematic next statement identified... but so far it seems pretty easy to analyze and fix. |
| 183 | + return &sketchyCall{} |
| 184 | + } |
| 185 | + } |
| 186 | + if trace { |
| 187 | + println("nothing found in remainder of block") |
| 188 | + println(fmt.Sprintf("%d Succs blocks will be investigated", len(callingBlock.Succs))) |
| 189 | + } |
| 190 | + |
| 191 | + // Now, assuming that there was nothing problematic found in the remainder of the block, use the control-flow graph |
| 192 | + // to identify where code execution would continue and see if there's anything inappropriate in it. |
| 193 | + // |
| 194 | + // 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 |
| 195 | + // block or a block that calls a function such as panic that never returns; one for a normal (jump) block; and two |
| 196 | + // for a conditional (if) block. |
| 197 | + // |
| 198 | + // It's possible for the next block to have either no nodes, or, no nodes that continue to do work and trigger |
| 199 | + // detection... but then to proceed into *another* block that does. So this investigation has to be done |
| 200 | + // recursively. Control-flow graph should prevent us from needing to stop this recursive detection; we'll hit a |
| 201 | + // 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 |
| 202 | + // looking, so no additional exit logic should be needed. |
| 203 | + for i, succ := range callingBlock.Succs { |
| 204 | + if trace { |
| 205 | + println(fmt.Sprintf("Succs[%d], block index %d, recursing:", i, succ.Index)) |
| 206 | + } |
| 207 | + // `-1` is used to start at index 0 in the nodes. |
| 208 | + sketchy := inspectCallSite(succ, -1, trace) |
| 209 | + if trace { |
| 210 | + println(fmt.Sprintf("Succs[%d], block index %d, had sketchy = %#v", i, succ.Index, sketchy)) |
| 211 | + } |
| 212 | + if sketchy != nil { |
| 213 | + return sketchy |
| 214 | + } |
| 215 | + } |
| 216 | + |
| 217 | + return nil |
| 218 | +} |
0 commit comments