diff --git a/README.md b/README.md index dd571ca..3846e5c 100644 --- a/README.md +++ b/README.md @@ -54,14 +54,17 @@ $ semgrep --config /path/to/semgrep-rules/hanging-goroutine.yml -o leaks.txt' | [eth-rpc-tracetransaction](go/eth-rpc-tracetransaction.yaml) | [🛝🔗](https://semgrep.dev/playground/r/trailofbits.go.eth-rpc-tracetransaction.eth-rpc-tracetransaction) | 🟥 | 🌕 | Detects attempts to extract trace information from an EVM transaction or block. In exchange or bridge applications, extra logic must be implemented encapsulating these endpoints to prevent the values transferred during reverted call frames from being counted. | | [eth-txreceipt-status](go/eth-txreceipt-status.yaml) | [🛝🔗](https://semgrep.dev/playground/r/trailofbits.go.eth-txreceipt-status.eth-txreceipt-status) | 🟥 | 🌕 | Detects when a transaction receipt's status is read | | [hanging-goroutine](go/hanging-goroutine.yaml) | [🛝🔗](https://semgrep.dev/playground/r/trailofbits.go.hanging-goroutine.hanging-goroutine) | 🟩 | 🌗 | Goroutine leaks | +| [http-error-missing-return](go/http-error-missing-return.yaml) | [🛝🔗](https://semgrep.dev/playground/r/trailofbits.go.http-error-missing-return.http-error-missing-return) | 🟧 | 🌗 | Missing `return` after `http.Error` lets the handler continue executing | | [invalid-usage-of-modified-variable](go/invalid-usage-of-modified-variable.yaml) | [🛝🔗](https://semgrep.dev/playground/r/trailofbits.go.invalid-usage-of-modified-variable.invalid-usage-of-modified-variable) | 🟧 | 🌘 | Possible unintentional assignment when an error occurs | | [iterate-over-empty-map](go/iterate-over-empty-map.yaml) | [🛝🔗](https://semgrep.dev/playground/r/trailofbits.go.iterate-over-empty-map.iterate-over-empty-map) | 🟩 | 🌗 | Probably redundant iteration over an empty map | | [missing-runlock-on-rwmutex](go/missing-runlock-on-rwmutex.yaml) | [🛝🔗](https://semgrep.dev/playground/r/trailofbits.go.missing-runlock-on-rwmutex.missing-runlock-on-rwmutex) | 🟧 | 🌗 | Missing `RUnlock` on an `RWMutex` lock before returning from a function | | [missing-unlock-before-return](go/missing-unlock-before-return.yaml) | [🛝🔗](https://semgrep.dev/playground/r/trailofbits.go.missing-unlock-before-return.missing-unlock-before-return) | 🟧 | 🌗 | Missing `mutex` unlock before returning from a function | | [nil-check-after-call](go/nil-check-after-call.yaml) | [🛝🔗](https://semgrep.dev/playground/r/trailofbits.go.nil-check-after-call.nil-check-after-call) | 🟧 | 🌗 | Possible nil dereferences | +| [pkg-errors-wrap-nil-err](go/pkg-errors-wrap-nil-err.yaml) | [🛝🔗](https://semgrep.dev/playground/r/trailofbits.go.pkg-errors-wrap-nil-err.pkg-errors-wrap-nil-err) | 🟧 | 🌗 | `pkg/errors` wrap-family call on a provably-nil error — silently swallows the failure path | | [racy-append-to-slice](go/racy-append-to-slice.yaml) | [🛝🔗](https://semgrep.dev/playground/r/trailofbits.go.racy-append-to-slice.racy-append-to-slice) | 🟧 | 🌗 | Concurrent calls to `append` from multiple goroutines | | [racy-write-to-map](go/racy-write-to-map.yaml) | [🛝🔗](https://semgrep.dev/playground/r/trailofbits.go.racy-write-to-map.racy-write-to-map) | 🟧 | 🌗 | Concurrent writes to the same map in multiple goroutines | | [servercodec-readrequestbody-unhandled-nil](go/servercodec-readrequestbody-unhandled-nil.yaml) | [🛝🔗](https://semgrep.dev/playground/r/trailofbits.go.servercodec-readrequestbody-unhandled-nil.servercodec-readrequestbody-unhandled-nil) | 🟩 | 🌘 | Possible incorrect `ServerCodec` interface implementation | +| [shadowed-err-check](go/shadowed-err-check.yaml) | [🛝🔗](https://semgrep.dev/playground/r/trailofbits.go.shadowed-err-check.shadowed-err-check) | 🟧 | 🌗 | Shadowed error variable: declared `err` is not the one checked against `nil` | | [string-to-int-signedness-cast](go/string-to-int-signedness-cast.yaml) | [🛝🔗](https://semgrep.dev/playground/r/trailofbits.go.string-to-int-signedness-cast.string-to-int-signedness-cast) | 🟧 | 🌘 | Integer underflows | | [sync-mutex-value-copied](go/sync-mutex-value-copied.yaml) | [🛝🔗](https://semgrep.dev/playground/r/trailofbits.go.sync-mutex-value-copied.sync-mutex-value-copied) | 🟩 | 🌘 | Copying of `sync.Mutex` via value receivers | | [unmarshal-tag-is-dash](go/unmarshal_tag_is_dash.yaml) | [🛝🔗](https://semgrep.dev/playground/r/trailofbits.go.unmarshal_tag_is_dash.unmarshal-tag-is-dash) | 🟧 | 🌘 | | diff --git a/go/http-error-missing-return.go b/go/http-error-missing-return.go new file mode 100644 index 0000000..ce611e2 --- /dev/null +++ b/go/http-error-missing-return.go @@ -0,0 +1,518 @@ +package test + +import ( + "errors" + "fmt" + "log" + "net/http" + "os" + "runtime" +) + + +var errSentinel = errors.New("sentinel") + +func checkAuth(r *http.Request) error { return errors.New("x") } +func getUser(r *http.Request) (string, error) { return "", errors.New("x") } +func doSomething(r *http.Request) error { return errors.New("x") } +func processRequest(w http.ResponseWriter, r *http.Request) {} + +func handlerAuthCheckBug(w http.ResponseWriter, r *http.Request) { + if err := checkAuth(r); err != nil { + // ruleid: http-error-missing-return + http.Error(w, fmt.Sprintf("auth check failed: %s", err.Error()), http.StatusBadRequest) + } + + w.WriteHeader(http.StatusOK) + w.Write([]byte("ok")) +} + +func handlerSimpleBug(w http.ResponseWriter, r *http.Request) { + user, err := getUser(r) + if err != nil { + // ruleid: http-error-missing-return + http.Error(w, "user not found", http.StatusNotFound) + } + + w.Write([]byte("user: " + user)) +} + +func handlerLogThenContinue(w http.ResponseWriter, r *http.Request) { + if r.Method != "POST" { + log.Print("bad method") + // ruleid: http-error-missing-return + http.Error(w, "method not allowed", http.StatusMethodNotAllowed) + } + + processRequest(w, r) +} + +// http.Error followed by another statement in the same block — Case B. +func handlerErrorFollowedByWrite(w http.ResponseWriter, r *http.Request) { + // ruleid: http-error-missing-return + http.Error(w, "boom", http.StatusInternalServerError) + w.Write([]byte("leaked body")) +} + +func handlerCorrectReturn(w http.ResponseWriter, r *http.Request) { + if err := checkAuth(r); err != nil { + // ok: http-error-missing-return + http.Error(w, err.Error(), http.StatusBadRequest) + return + } + + w.WriteHeader(http.StatusOK) +} + +func handlerCorrectLogThenReturn(w http.ResponseWriter, r *http.Request) { + if err := doSomething(r); err != nil { + log.Print("error: ", err) + // ok: http-error-missing-return + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } + + w.Write([]byte("ok")) +} + +func helperReturningError(w http.ResponseWriter, r *http.Request) error { + if err := doSomething(r); err != nil { + // ok: http-error-missing-return + http.Error(w, err.Error(), http.StatusInternalServerError) + return err + } + + return nil +} + +func handlerLastStatement(w http.ResponseWriter, r *http.Request) { + if err := doSomething(r); err != nil { + // ok: http-error-missing-return + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } +} + +func handlerIfElseBothReturn(w http.ResponseWriter, r *http.Request) { + if r.Method == "GET" { + // ok: http-error-missing-return + http.Error(w, "GET not allowed", http.StatusMethodNotAllowed) + return + } else { + w.Write([]byte("ok")) + return + } +} + +func handlerNestedReturn(w http.ResponseWriter, r *http.Request) { + if r.Method == "POST" { + if r.ContentLength == 0 { + // ok: http-error-missing-return + http.Error(w, "empty body", http.StatusBadRequest) + return + } + } + + w.Write([]byte("ok")) +} + +// The if-block is the LAST statement of the function — there is no +// `$NEXT` after it, so Case A's pattern-inside requirement (a trailing +// statement after the if-chain) is not met and the rule does not fire. +func handlerEndOfFunction(w http.ResponseWriter, r *http.Request) { + if err := doSomething(r); err != nil { + // ok: http-error-missing-return + http.Error(w, err.Error(), http.StatusInternalServerError) + } +} + +// log.Fatal terminates execution; treat as a control-flow terminator. +func handlerLogFatal(w http.ResponseWriter, r *http.Request) { + if err := doSomething(r); err != nil { + // ok: http-error-missing-return + http.Error(w, err.Error(), http.StatusInternalServerError) + log.Fatal("unrecoverable: ", err) + } + + w.Write([]byte("ok")) +} + +// log.Fatalln terminator. +func handlerLogFatalln(w http.ResponseWriter, r *http.Request) { + if err := doSomething(r); err != nil { + // ok: http-error-missing-return + http.Error(w, err.Error(), http.StatusInternalServerError) + log.Fatalln("unrecoverable") + } + + w.Write([]byte("ok")) +} + +// log.Fatalf terminator. +func handlerLogFatalf(w http.ResponseWriter, r *http.Request) { + if err := doSomething(r); err != nil { + // ok: http-error-missing-return + http.Error(w, err.Error(), http.StatusInternalServerError) + log.Fatalf("unrecoverable: %v", err) + } + + w.Write([]byte("ok")) +} + +// os.Exit terminator. +func handlerOsExit(w http.ResponseWriter, r *http.Request) { + if err := doSomething(r); err != nil { + // ok: http-error-missing-return + http.Error(w, err.Error(), http.StatusInternalServerError) + os.Exit(1) + } + + w.Write([]byte("ok")) +} + +// Terminator follows the if-block at the enclosing scope (Case A's +// "next-stmt is terminator" exclusion). +func handlerNextStmtLogFatal(w http.ResponseWriter, r *http.Request) { + if err := doSomething(r); err != nil { + // ok: http-error-missing-return + http.Error(w, err.Error(), http.StatusInternalServerError) + } + + log.Fatal("downstream did not run") +} + +// Case B: log.Fatal immediately after http.Error — not flagged. +func handlerCaseBLogFatal(w http.ResponseWriter, r *http.Request) { + // ok: http-error-missing-return + http.Error(w, "boom", http.StatusInternalServerError) + log.Fatalf("boom %d", 1) +} + +// Case B: os.Exit immediately after http.Error — not flagged. +func handlerCaseBOsExit(w http.ResponseWriter, r *http.Request) { + // ok: http-error-missing-return + http.Error(w, "boom", http.StatusInternalServerError) + os.Exit(2) +} + +// Sanity: an identifier whose name *starts with* "return" must not be +// mistaken for a return statement by the terminator regex. +func handlerReturnPrefixIsNotTerminator(w http.ResponseWriter, r *http.Request) { + if err := doSomething(r); err != nil { + // ruleid: http-error-missing-return + http.Error(w, err.Error(), http.StatusInternalServerError) + returnedSomething() + } + + w.Write([]byte("ok")) +} + +func returnedSomething() {} + +// http.Error in the middle branch of an `else if` chain — handler +// keeps running past the if-else and reaches the trailing write. +func handlerElseIfMissingReturn(w http.ResponseWriter, r *http.Request) { + if r.Method == "GET" { + w.Write([]byte("get")) + } else if r.Method == "POST" { + // ruleid: http-error-missing-return + http.Error(w, "no post", http.StatusMethodNotAllowed) + } + + w.Write([]byte("done")) +} + +// `else if` middle branch with a `return` is fine. +func handlerElseIfWithReturn(w http.ResponseWriter, r *http.Request) { + if r.Method == "GET" { + w.Write([]byte("get")) + } else if r.Method == "POST" { + // ok: http-error-missing-return + http.Error(w, "no post", http.StatusMethodNotAllowed) + return + } + + w.Write([]byte("done")) +} + +// `else if` middle branch where a terminator follows the entire +// if-else chain — no fall-through, so no bug. +func handlerElseIfNextTerm(w http.ResponseWriter, r *http.Request) { + if r.Method == "GET" { + w.Write([]byte("get")) + } else if r.Method == "POST" { + // ok: http-error-missing-return + http.Error(w, "no post", http.StatusMethodNotAllowed) + } + + log.Fatal("downstream did not run") +} + +// Trailing `return` after the inner if-else satisfies Case A's +// terminator subtraction at the outer scope, so neither inner +// `http.Error` is flagged. +func handlerNestedNoInnerReturn(w http.ResponseWriter, r *http.Request) { + if err := doSomething(r); err != nil { + if errors.Is(err, errSentinel) { + // ok: http-error-missing-return + http.Error(w, "not found", http.StatusNotFound) + } else { + // ok: http-error-missing-return + http.Error(w, err.Error(), http.StatusInternalServerError) + } + + return + } + + w.Write([]byte("ok")) +} + +// `continue` legitimately ends an iteration; the response was written for +// this request item, so the loop just moves on. +func handlerForLoopContinue(w http.ResponseWriter, r *http.Request, items []string) { + for range items { + if err := doSomething(r); err != nil { + // ok: http-error-missing-return + http.Error(w, err.Error(), http.StatusInternalServerError) + continue + } + } +} + +// `break` is treated as a terminator by the rule even though it exits +// the loop, not the handler. Conservative choice: legitimate retry-style +// loops commonly write a single error and `break`, and flagging them +// produces noise. Real fall-through bugs after `break` (loop is followed +// by code that double-writes) are accepted as a known FN. +func handlerForLoopBreak(w http.ResponseWriter, r *http.Request) { + for { + if r.Method != "POST" { + // ok: http-error-missing-return + http.Error(w, "no", http.StatusMethodNotAllowed) + break + } + } +} + +// `goto` to a cleanup label is also a control-flow terminator for the +// purpose of this rule. +func handlerGoto(w http.ResponseWriter, r *http.Request) { + if r.Method != "POST" { + // ok: http-error-missing-return + http.Error(w, "no", http.StatusMethodNotAllowed) + goto cleanup + } + w.Write([]byte("ok")) +cleanup: +} + +// `runtime.Goexit` terminates the current goroutine — also a terminator. +func handlerRuntimeGoexit(w http.ResponseWriter, r *http.Request) { + if r.Method != "POST" { + // ok: http-error-missing-return + http.Error(w, "no", http.StatusMethodNotAllowed) + runtime.Goexit() + } + w.Write([]byte("ok")) +} + +// 3-way else-if chain: http.Error in the deepest branch, no return, and +// code follows the entire chain. Should be flagged. (Previously a FN.) +func handlerThreeWayElseIf(w http.ResponseWriter, r *http.Request) { + if r.Method == "GET" { + w.Write([]byte("g")) + } else if r.Method == "POST" { + w.Write([]byte("p")) + } else if r.Method == "DELETE" { + // ruleid: http-error-missing-return + http.Error(w, "no del", http.StatusMethodNotAllowed) + } + w.Write([]byte("done")) +} + +// 4-way else-if: still a bug, exercises arbitrary depth. +func handlerFourWayElseIf(w http.ResponseWriter, r *http.Request) { + if r.Method == "GET" { + w.Write([]byte("g")) + } else if r.Method == "POST" { + w.Write([]byte("p")) + } else if r.Method == "PUT" { + w.Write([]byte("u")) + } else if r.Method == "DELETE" { + // ruleid: http-error-missing-return + http.Error(w, "no del", http.StatusMethodNotAllowed) + } + w.Write([]byte("done")) +} + +// 4-way else-if where the trailing chain is followed by a terminator — +// no fall-through, so no bug. +func handlerFourWayElseIfNextTerm(w http.ResponseWriter, r *http.Request) { + if r.Method == "GET" { + w.Write([]byte("g")) + } else if r.Method == "POST" { + w.Write([]byte("p")) + } else if r.Method == "PUT" { + w.Write([]byte("u")) + } else if r.Method == "DELETE" { + // ok: http-error-missing-return + http.Error(w, "no del", http.StatusMethodNotAllowed) + } + log.Fatal("downstream did not run") +} + +// 4-way else-if where the deepest branch terminates with `return` — no +// bug. Verifies the inner-block-tail check correctly excludes calls that +// have a terminator after them in their own block. +func handlerFourWayElseIfWithReturn(w http.ResponseWriter, r *http.Request) { + if r.Method == "GET" { + w.Write([]byte("g")) + } else if r.Method == "POST" { + w.Write([]byte("p")) + } else if r.Method == "PUT" { + w.Write([]byte("u")) + } else if r.Method == "DELETE" { + // ok: http-error-missing-return + http.Error(w, "no del", http.StatusMethodNotAllowed) + return + } + w.Write([]byte("done")) +} + +// Switch case where http.Error is the tail of a case body and the +// switch is followed by non-terminating code. Switch cases don't +// fall through in Go, but the function continues past the switch. +func handlerSwitchCase(w http.ResponseWriter, r *http.Request) { + switch r.Method { + case "POST": + // ruleid: http-error-missing-return + http.Error(w, "no posts", http.StatusMethodNotAllowed) + } + w.Write([]byte("leaked")) +} + +// Switch case with `return` after http.Error — fine. +func handlerSwitchCaseReturn(w http.ResponseWriter, r *http.Request) { + switch r.Method { + case "POST": + // ok: http-error-missing-return + http.Error(w, "no posts", http.StatusMethodNotAllowed) + return + } + w.Write([]byte("ok")) +} + +// Switch is the LAST stmt of the function — handler returns naturally, +// so missing terminator inside a case is harmless. +func handlerSwitchCaseEndOfFunc(w http.ResponseWriter, r *http.Request) { + switch r.Method { + case "POST": + // ok: http-error-missing-return + http.Error(w, "no posts", http.StatusMethodNotAllowed) + } +} + +// Switch is followed by a terminator — no fall-through past the switch. +func handlerSwitchCaseNextTerm(w http.ResponseWriter, r *http.Request) { + switch r.Method { + case "POST": + // ok: http-error-missing-return + http.Error(w, "no posts", http.StatusMethodNotAllowed) + } + log.Fatal("downstream did not run") +} + +// Switch with default clause — same fall-through bug. +func handlerSwitchDefault(w http.ResponseWriter, r *http.Request) { + switch r.Method { + case "GET": + w.Write([]byte("g")) + default: + // ruleid: http-error-missing-return + http.Error(w, "bad method", http.StatusMethodNotAllowed) + } + w.Write([]byte("leaked")) +} + +// Multi-case switch — fall-through bug in a middle case body, with +// other case bodies present alongside. +func handlerSwitchMultiCase(w http.ResponseWriter, r *http.Request) { + switch r.Method { + case "GET": + w.Write([]byte("g")) + case "POST": + // ruleid: http-error-missing-return + http.Error(w, "no posts", http.StatusMethodNotAllowed) + case "PUT": + w.Write([]byte("u")) + } + w.Write([]byte("done")) +} + +// Switch with init clause: `switch $INIT; $X { ... }` followed by +// trailing code — exercises the init-form `pattern-inside` branches. +func handlerSwitchWithInit(w http.ResponseWriter, r *http.Request) { + switch m := r.Method; m { + case "POST": + // ruleid: http-error-missing-return + http.Error(w, "no", http.StatusMethodNotAllowed) + } + w.Write([]byte("done")) +} + +type server struct{} + +// Method-receiver handler — same fall-through bug, different signature. +func (s *server) ServeHTTP(w http.ResponseWriter, r *http.Request) { + if err := doSomething(r); err != nil { + // ruleid: http-error-missing-return + http.Error(w, err.Error(), http.StatusInternalServerError) + } + w.Write([]byte("ok")) +} + +// Method-receiver handler with `return` — fine. +func (s *server) Get(w http.ResponseWriter, r *http.Request) { + if err := doSomething(r); err != nil { + // ok: http-error-missing-return + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } + w.Write([]byte("ok")) +} + +// Anonymous handler passed to http.HandleFunc — bug fires inside the closure. +func registerHandlerBug() { + http.HandleFunc("/x", func(w http.ResponseWriter, r *http.Request) { + if err := doSomething(r); err != nil { + // ruleid: http-error-missing-return + http.Error(w, err.Error(), http.StatusInternalServerError) + } + w.Write([]byte("ok")) + }) +} + +// Anonymous handler with `return` inside the closure — fine. +func registerHandlerOK() { + http.HandleFunc("/x", func(w http.ResponseWriter, r *http.Request) { + if err := doSomething(r); err != nil { + // ok: http-error-missing-return + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } + w.Write([]byte("ok")) + }) +} + +// `select` cases are caught by the existing switch-shaped patterns +// (Semgrep's Go AST treats `select` similarly to `switch`), so a +// fall-through bug inside a `select` case is flagged the same way. +func handlerSelectFallThrough(w http.ResponseWriter, r *http.Request) { + ctx := r.Context() + select { + case <-ctx.Done(): + // ruleid: http-error-missing-return + http.Error(w, "timeout", http.StatusGatewayTimeout) + } + w.Write([]byte("done")) +} diff --git a/go/http-error-missing-return.yaml b/go/http-error-missing-return.yaml new file mode 100644 index 0000000..bd0af7d --- /dev/null +++ b/go/http-error-missing-return.yaml @@ -0,0 +1,153 @@ +rules: + # Two cases under one id: + # Case A — http.Error is the tail of its block, and the enclosing + # if-chain or switch is followed by another statement. + # Case B — http.Error is directly followed by a statement. + # Subtractions for terminator-after-error are kept at top level because + # Semgrep silently ignores `pattern-not` / `metavariable-regex` when + # nested under `pattern-either > patterns`. The Case A subtraction also + # carries an extra `pattern-not http.Error(...)\n$ANY` so it doesn't + # over-subtract Case B matches. + - id: http-error-missing-return + languages: [go] + severity: ERROR + message: >- + `http.Error` writes a complete response but does not stop handler + execution. When the call is not followed by `return` (or another + terminating statement), the handler keeps running, potentially writing + additional response bytes, performing actions that should have been + gated by the response, or producing a malformed/duplicated response. + Add a `return` immediately after the call, or otherwise terminate the + control flow. + metadata: + category: security + cwe: "CWE-754: Improper Check for Unusual or Exceptional Conditions" + subcategory: [vuln] + confidence: MEDIUM + likelihood: MEDIUM + impact: MEDIUM + technology: [--no-technology--] + description: "Missing `return` after `http.Error` lets the handler continue executing" + references: + - https://pkg.go.dev/net/http#Error + patterns: + # Anchor to files importing net/http so `http.Error` resolves to stdlib. + - pattern-inside: | + import "net/http" + ... + - pattern: http.Error(...) + - pattern-either: + # Case A. With nested chains, $NEXT binds to the trailing stmt of + # the outermost containing structure, so deep else-if chains work + # without explicit case enumeration. + - patterns: + - pattern-not-inside: | + http.Error(...) + $ANY + - pattern-either: + - pattern-inside: | + if $C1 { + ... + } + $NEXT + - pattern-inside: | + if $I1; $C1 { + ... + } + $NEXT + - pattern-inside: | + switch $X1 { + case $L1: + ... + } + $NEXT + - pattern-inside: | + switch $I1; $X1 { + case $L1: + ... + } + $NEXT + - pattern-inside: | + switch { + case $L1: + ... + } + $NEXT + - pattern-inside: | + switch $X1 { + default: + ... + } + $NEXT + - pattern-inside: | + switch { + default: + ... + } + $NEXT + # Case B. + - pattern-inside: | + http.Error(...) + $STMT + # Subtract Case B: http.Error directly followed by a terminator. + - pattern-not-inside: + patterns: + - pattern: | + http.Error(...) + $TERM + - metavariable-regex: + metavariable: $TERM + regex: '^(return\b|panic\(|log\.Fatal(ln|f)?\(|os\.Exit\(|runtime\.Goexit\(|continue\b|break\b|goto\b)' + # Subtract Case A: enclosing if-chain or switch is followed by a + # terminator AND http.Error is the tail of its block. The tail-of-block + # conjunction prevents subtracting Case B matches whose enclosing + # structure also happens to be followed by a terminator. + - pattern-not-inside: + patterns: + - pattern-either: + - pattern: | + if $C2 { + ... + } + $TERM + - pattern: | + if $I2; $C2 { + ... + } + $TERM + - pattern: | + switch $X2 { + case $L2: + ... + } + $TERM + - pattern: | + switch $I2; $X2 { + case $L2: + ... + } + $TERM + - pattern: | + switch { + case $L2: + ... + } + $TERM + - pattern: | + switch $X2 { + default: + ... + } + $TERM + - pattern: | + switch { + default: + ... + } + $TERM + - metavariable-regex: + metavariable: $TERM + regex: '^(return\b|panic\(|log\.Fatal(ln|f)?\(|os\.Exit\(|runtime\.Goexit\(|continue\b|break\b|goto\b)' + - pattern-not: | + http.Error(...) + $ANY diff --git a/go/pkg-errors-wrap-nil-err.go b/go/pkg-errors-wrap-nil-err.go new file mode 100644 index 0000000..8785824 --- /dev/null +++ b/go/pkg-errors-wrap-nil-err.go @@ -0,0 +1,1046 @@ +package testdata + +import ( + "fmt" + "log" + "os" + "runtime" + + "github.com/pkg/errors" +) + +func someCall() (string, error) { return "", nil } +func anotherCall() error { return nil } +func mayFail() (int, error) { return 0, nil } +func someCondition() bool { return true } + +// TP1: simplest case — wrap inside non-err if-block after a guard. +func tp1() error { + err := anotherCall() + if err != nil { + // ok: pkg-errors-wrap-nil-err + return errors.Wrap(err, "first") + } + + if someCondition() { + // ruleid: pkg-errors-wrap-nil-err + return errors.Wrapf(err, "second %s", "x") + } + + return nil +} + +// TP2: errors.Wrap (not Wrapf) variant. +func tp2() error { + _, err := someCall() + if err != nil { + return err + } + + if someCondition() { + // ruleid: pkg-errors-wrap-nil-err + return errors.Wrap(err, "boom") + } + + return nil +} +// TP3: branchy reassignment — `err` is reassigned only inside an inner +// `if branch { ... return }`, so along the wrap path `err` is still +// proven-nil from the outer guard. The reassignment lives in a sibling +// block, not the wrap's enclosing block. +func tp3(branch bool) error { + err := anotherCall() + if err != nil { + return err + } + + if someCondition() { + if branch { + _, err = mayFail() + return err + } + + // ruleid: pkg-errors-wrap-nil-err + return errors.Wrap(err, "branchy reassign") + } + return nil +} + +// TP4: bug nested inside a for loop body. +func tp4(items []string) error { + for range items { + _, err := mayFail() + if err != nil { + // ok: pkg-errors-wrap-nil-err + return errors.Wrap(err, "loop") + } + + if someCondition() { + // ruleid: pkg-errors-wrap-nil-err + return errors.Wrapf(err, "post check") + } + } + return nil +} + +// TP5: wrap inside a complex non-err condition +func tp5(xs []int, xx int) error { + _, err := mayFail() + if err != nil { + // ok: pkg-errors-wrap-nil-err + return errors.Wrap(err, "init") + } + + if len(xs) < xx { + // ruleid: pkg-errors-wrap-nil-err + return errors.Wrapf(err, "too short, got %d want %d", len(xs), xx) + } + return nil +} + +// TP6: guard returns a non-wrap value but err is still provably nil after. +func tp6() error { + err := anotherCall() + if err != nil { + return fmt.Errorf("init: %w", err) + } + + if someCondition() { + // ruleid: pkg-errors-wrap-nil-err + return errors.Wrapf(err, "later") + } + return nil +} + +// TP7: many statements between guard and wrap. +func tp7() error { + _, err := someCall() + if err != nil { + return err + } + + a := 1 + b := 2 + c := a + b + _ = c + + if someCondition() { + // ruleid: pkg-errors-wrap-nil-err + return errors.Wrap(err, "noisy") + } + return nil +} + +// TP8: bug inside an `else` branch — still an `if $COND { ... }` body. +func tp8() error { + err := anotherCall() + if err != nil { + return err + } + + if someCondition() { + return nil + } else { + // ruleid: pkg-errors-wrap-nil-err + return errors.Wrap(err, "else branch") + } +} + +// TP9: bug nested inside an inner if (deeper than the guard). +func tp9(n int) error { + err := anotherCall() + if err != nil { + return err + } + + if n > 0 { + if n > 100 { + // ruleid: pkg-errors-wrap-nil-err + return errors.Wrapf(err, "way too big: %d", n) + } + } + return nil +} + +// TP10: errors.WithMessage on provably-nil err — same nil-passthrough bug. +func tp10() error { + err := anotherCall() + if err != nil { + return err + } + + if someCondition() { + // ruleid: pkg-errors-wrap-nil-err + return errors.WithMessage(err, "with-message") + } + return nil +} + +// TP11: errors.WithMessagef on provably-nil err. +func tp11() error { + err := anotherCall() + if err != nil { + return err + } + + if someCondition() { + // ruleid: pkg-errors-wrap-nil-err + return errors.WithMessagef(err, "with-message %d", 42) + } + return nil +} + +// TP12: errors.WithStack on provably-nil err — also returns nil for nil input. +func tp12() error { + err := anotherCall() + if err != nil { + return err + } + + if someCondition() { + // ruleid: pkg-errors-wrap-nil-err + return errors.WithStack(err) + } + return nil +} + +// TP13: alternate variable name `e` — same bug shape. +func tp13() error { + e := anotherCall() + if e != nil { + return e + } + + if someCondition() { + // ruleid: pkg-errors-wrap-nil-err + return errors.Wrap(e, "alt name e") + } + return nil +} + +// TP14: alternate variable name `readErr` — same bug shape. +func tp14() error { + readErr := anotherCall() + if readErr != nil { + return readErr + } + + if someCondition() { + // ruleid: pkg-errors-wrap-nil-err + return errors.Wrapf(readErr, "alt name readErr") + } + return nil +} + +// TP15: PascalCase suffix — `parseErr` (still err-like). +func tp15() error { + parseErr := anotherCall() + if parseErr != nil { + return parseErr + } + + if someCondition() { + // ruleid: pkg-errors-wrap-nil-err + return errors.WithMessage(parseErr, "alt name parseErr") + } + return nil +} + +// TP16: invalid else branch +func tp16() error { + parseErr := anotherCall() + if parseErr != nil { + return parseErr + } else { + // ruleid: pkg-errors-wrap-nil-err + return errors.WithMessage(parseErr, "alt name parseErr") + } +} + +// TN1: wrap inside the `if err != nil` block — canonical correct usage. +func tn1() error { + err := anotherCall() + if err != nil { + // ok: pkg-errors-wrap-nil-err + return errors.Wrap(err, "legit") + } + return nil +} + +// TN2: wrap inside `if err == nil` (intentional success-side check). +func tn2() error { + err := anotherCall() + if err != nil { + return err + } + + if err == nil { + // ok: pkg-errors-wrap-nil-err + return errors.Wrap(err, "intentional nil branch") + } + return nil +} + +// TN3: legitimate "wrap result of trailing call" idiom — wrap is at function +// level, NOT inside a non-err if-block. Wrapping nil returns nil, so this +// pattern is intentional even when err might be nil. +func tn3() error { + err := anotherCall() + if err != nil { + return err + } + + err = anotherCall() + // ok: pkg-errors-wrap-nil-err + return errors.Wrap(err, "trailing call") +} + +// TN4: a different error variable is being wrapped — the literal `err` token +// in the pattern doesn't bind to it. +func tn4() error { + err := anotherCall() + if err != nil { + return err + } + + otherErr := anotherCall() + if someCondition() { + // ok: pkg-errors-wrap-nil-err + return errors.Wrap(otherErr, "other variable") + } + return nil +} + +// TN5: no preceding `if err != nil { return }` guard at all — `err` may be +// non-nil at the wrap site. +func tn5() error { + err := anotherCall() + if someCondition() { + // ok: pkg-errors-wrap-nil-err + return errors.Wrap(err, "no guard") + } + return nil +} + +// TN6: wrap with a literal `nil` argument — not the `err` variable shape. +func tn6() error { + err := anotherCall() + if err != nil { + return err + } + if someCondition() { + // ok: pkg-errors-wrap-nil-err + return errors.Wrap(nil, "explicitly nil") + } + return nil +} + +// TN7: standard library `errors` package usage — irrelevant. Included to +// confirm we don't accidentally trip on it (no Wrap in std `errors`, but +// this stresses that the rule is scoped to Wrap/Wrapf calls). +func tn7() error { + err := anotherCall() + if err != nil { + return err + } + if someCondition() { + // ok: pkg-errors-wrap-nil-err + return fmt.Errorf("plain %w", err) + } + return nil +} + +// TN8: WithMessage / WithMessagef / WithStack inside `if err != nil` — +// canonical correct usage. +func tn8() error { + err := anotherCall() + if err != nil { + // ok: pkg-errors-wrap-nil-err + return errors.WithMessage(err, "ctx") + } + err = anotherCall() + if err != nil { + // ok: pkg-errors-wrap-nil-err + return errors.WithMessagef(err, "ctx %d", 1) + } + err = anotherCall() + if err != nil { + // ok: pkg-errors-wrap-nil-err + return errors.WithStack(err) + } + return nil +} + +// TN9: variable name `result` doesn't match the err-like regex even after a +// preceding `if err != nil { return }` — we don't flag wraps of +// non-error-shaped names. +func tn9() error { + err := anotherCall() + if err != nil { + return err + } + + var result error = anotherCall() + if someCondition() { + // ok: pkg-errors-wrap-nil-err + return errors.Wrap(result, "non-err-named") + } + return nil +} + +// TN10: nil-check on a DIFFERENT err variable than the one being wrapped — +// the metavariable unification means $ERR must be the same name on both +// sides, so this is a TN. +func tn10() error { + outerErr := anotherCall() + if outerErr != nil { + return outerErr + } + + innerErr := anotherCall() + if innerErr != nil { + // ok: pkg-errors-wrap-nil-err + return errors.Wrap(innerErr, "different err var than the guard") + } + return nil +} + +// TP17: chained calls — each call uses `:=` (re)binding `err` and is +// followed by its own `if err != nil { return ... }` guard. After both +// guards, `err` is provably nil, so the wrap inside the trailing +// non-err `if $COND { ... }` is the bug. +func tp17(threshold int) error { + id, err := someCall() + if err != nil { + // ok: pkg-errors-wrap-nil-err + return errors.Wrapf(err, "lookup %q", id) + } + + count, err := mayFail() + if err != nil { + // ok: pkg-errors-wrap-nil-err + return errors.Wrapf(err, "count %q", id) + } + + if count < threshold { + // ruleid: pkg-errors-wrap-nil-err + return errors.Wrapf(err, "below threshold %d", threshold) + } + return nil +} + +// TN11: classic FP shape — `err` is reassigned via tuple-assign between +// the guard and the wrap, so the wrap site is on a freshly-assigned +// (potentially non-nil) err, not the proven-nil one. +func tn11(cond bool) error { + res, err := mayFail() + if err != nil { + return errors.Wrap(err, "first call") + } + + if res > 0 { + return nil + } + + if cond { + _, err = mayFail() + // ok: pkg-errors-wrap-nil-err + return errors.Wrap(err, "second call") + } + return nil +} + +// TN12: same as TN11 but the reassignment lives in the surrounding +// function scope (not inside the `if cond` block). The wrap is the last +// statement of an enclosing `if cond { ... }` block. +func tn12(cond bool) error { + res, err := mayFail() + if err != nil { + return errors.Wrap(err, "first call") + } + + if cond { + if res > 0 { + return nil + } + + _, err = mayFail() + + // ok: pkg-errors-wrap-nil-err + return errors.Wrap(err, "second call") + } + return nil +} + +// TN13: single-LHS reassignment between guard and wrap. +func tn13(cond bool) error { + err := anotherCall() + if err != nil { + return err + } + + if cond { + err = anotherCall() + // ok: pkg-errors-wrap-nil-err + return errors.Wrapf(err, "single reassign") + } + return nil +} + +// TN14: reassignment happens inside an else branch before the wrap — +// Case B FP shape. +func tn14() error { + err := anotherCall() + if err != nil { + return err + } else { + _, err = mayFail() + // ok: pkg-errors-wrap-nil-err + return errors.Wrap(err, "reassigned in else") + } +} + +// TN15: alt-named err reassigned between guard and wrap. +func tn15(cond bool) error { + parseErr := anotherCall() + if parseErr != nil { + return parseErr + } + + if cond { + parseErr = anotherCall() + // ok: pkg-errors-wrap-nil-err + return errors.WithMessage(parseErr, "after reassign") + } + return nil +} + +// TN16: multiple reassignments before the wrap — still a TN because the +// last value of err is not provably nil. +func tn16(cond bool) error { + _, err := mayFail() + if err != nil { + return err + } + + if cond { + _, err = mayFail() + _, err = mayFail() + // ok: pkg-errors-wrap-nil-err + return errors.Wrap(err, "after multiple reassigns") + } + return nil +} + +func threeVals() (int, string, error) { return 0, "", nil } + +// TN17: 3-value tuple reassignment between guard and wrap (err is last, +// per Go convention). +func tn17(cond bool) error { + _, _, err := threeVals() + if err != nil { + return err + } + + if cond { + _, _, err = threeVals() + // ok: pkg-errors-wrap-nil-err + return errors.Wrap(err, "three-tuple reassign") + } + return nil +} + +// TN18: 3-value tuple reassignment inside an else branch. +func tn18() error { + _, _, err := threeVals() + if err != nil { + return err + } else { + _, _, err = threeVals() + // ok: pkg-errors-wrap-nil-err + return errors.Wrap(err, "three-tuple else reassign") + } +} + +var ErrSentinelA = errors.New("sentinel a") +var ErrSentinelB = errors.New("sentinel b") + +// TN19: compound `if $ERR != nil && ...` — `&&` guarantees err is +// non-nil inside the body, so the wrap is intentional. +func tn19() error { + _, err := mayFail() + if err != nil { + return err + } + + if err != nil && !errors.Is(err, ErrSentinelA) { + // ok: pkg-errors-wrap-nil-err + return errors.Wrap(err, "compound err first") + } + return nil +} + +// TN20: compound condition with `$ERR != nil` as the LAST conjunct. +func tn20(extra bool) error { + _, err := mayFail() + if err != nil { + return err + } + + if extra && err != nil { + // ok: pkg-errors-wrap-nil-err + return errors.Wrap(err, "compound err last") + } + return nil +} + +// TN21: compound condition with `$ERR != nil` in the MIDDLE position +// of a 3-conjunct `&&` chain. +func tn21() error { + _, err := mayFail() + if err != nil { + return err + } + + if err != nil && !errors.Is(err, ErrSentinelA) && !errors.Is(err, ErrSentinelB) { + // ok: pkg-errors-wrap-nil-err + return errors.Wrapf(err, "compound err middle") + } + return nil +} + +// TN22: reverse guard `if $ERR == nil { return ... }` followed by a +// wrap on the err-non-nil path. +func tn22(cond bool) error { + err := anotherCall() + if err != nil { + return err + } + + if err == nil { + return nil + } + + if cond { + // ok: pkg-errors-wrap-nil-err + return errors.Wrap(err, "after err==nil reverse guard") + } + return nil +} + +// TP18: guard body has a logging stmt before `return` — `err` is still +// provably nil after the guard, so the wrap inside the trailing +// non-err `if` is the bug. +func tp18() error { + err := anotherCall() + if err != nil { + log.Print("init failed: ", err) + return err + } + + if someCondition() { + // ruleid: pkg-errors-wrap-nil-err + return errors.Wrap(err, "after multi-stmt guard") + } + return nil +} + +// TP19: assignment-form wrap inside the else of `if err != nil { ... } else { ... }`. +// Wrap returns nil for nil input, so `err` stays nil. +func tp19() error { + err := anotherCall() + if err != nil { + return err + } else { + // ruleid: pkg-errors-wrap-nil-err + err = errors.Wrap(err, "assignment-form") + return err + } +} + +// TN24: assignment-form wrap in else preceded by a single-LHS reassign +// — `err` is no longer provably nil, so the wrap is intentional. +func tn24() error { + err := anotherCall() + if err != nil { + return err + } else { + err = anotherCall() + // ok: pkg-errors-wrap-nil-err + err = errors.Wrap(err, "single reassign in else") + return err + } +} + +// TN25: assignment-form wrap in else preceded by a tuple reassign. +func tn25() error { + _, err := mayFail() + if err != nil { + return err + } else { + _, err = mayFail() + // ok: pkg-errors-wrap-nil-err + err = errors.Wrap(err, "tuple reassign in else") + return err + } +} + +// TN23: reverse guard inside a deferred closure — the wrap runs only +// when the captured err is non-nil. +func tn23() (err error) { + err = anotherCall() + if err != nil { + return err + } + + defer func() { + if err == nil { + return + } + + if cleanupErr := anotherCall(); cleanupErr != nil { + if err == nil { + err = cleanupErr + return + } + // ok: pkg-errors-wrap-nil-err + err = errors.Wrap(err, "with cleanup failure") + } + }() + + return nil +} + +// TP20: panic-based guard. After `if err != nil { panic(err) }`, `err` +// is provably nil, so wrapping it inside the trailing non-err `if` +// swallows the failure path. +func tp20() error { + err := anotherCall() + if err != nil { + panic(err) + } + + if someCondition() { + // ruleid: pkg-errors-wrap-nil-err + return errors.Wrap(err, "after panic guard") + } + return nil +} + +// TP21: log.Fatal-based guard. +func tp21() error { + err := anotherCall() + if err != nil { + log.Fatal("init: ", err) + } + + if someCondition() { + // ruleid: pkg-errors-wrap-nil-err + return errors.Wrap(err, "after log.Fatal guard") + } + return nil +} + +// TP22: log.Fatalf-based guard. +func tp22() error { + err := anotherCall() + if err != nil { + log.Fatalf("init: %v", err) + } + + if someCondition() { + // ruleid: pkg-errors-wrap-nil-err + return errors.Wrapf(err, "after log.Fatalf guard") + } + return nil +} + +// TP23: log.Fatalln-based guard. +func tp23() error { + err := anotherCall() + if err != nil { + log.Fatalln("init failed", err) + } + + if someCondition() { + // ruleid: pkg-errors-wrap-nil-err + return errors.WithMessage(err, "after log.Fatalln guard") + } + return nil +} + +// TP24: os.Exit-based guard. +func tp24() error { + err := anotherCall() + if err != nil { + os.Exit(1) + } + + if someCondition() { + // ruleid: pkg-errors-wrap-nil-err + return errors.WithStack(err) + } + return nil +} + +// TP25: runtime.Goexit-based guard. +func tp25() error { + err := anotherCall() + if err != nil { + runtime.Goexit() + } + + if someCondition() { + // ruleid: pkg-errors-wrap-nil-err + return errors.WithMessagef(err, "after runtime.Goexit guard") + } + return nil +} + +// TN26: reverse guard with panic — `err` is non-nil after. +func tn26(cond bool) error { + err := anotherCall() + if err != nil { + return err + } + + if err == nil { + panic("unreachable: err already non-nil") + } + + if cond { + // ok: pkg-errors-wrap-nil-err + return errors.Wrap(err, "after err==nil panic guard") + } + return nil +} + +// TN27: reverse guard with log.Fatal — `err` is non-nil after. +func tn27(cond bool) error { + err := anotherCall() + if err != nil { + return err + } + + if err == nil { + log.Fatal("unreachable") + } + + if cond { + // ok: pkg-errors-wrap-nil-err + return errors.WithStack(err) + } + return nil +} + +// TP26: wrap inside `else if` arm — Go parses this as +// `if err != nil { return err } else { if cond { return errors.Wrap(...) } }`, +// and inside the inner if (which runs only when err is nil) the wrap +// swallows the error path. +func tp26(cond bool) error { + err := anotherCall() + if err != nil { + return err + } else if cond { + // ruleid: pkg-errors-wrap-nil-err + return errors.Wrap(err, "else-if arm") + } + return nil +} + +// TP27: wrap inside a nested `else if` (depth 2). +func tp27(cond1, cond2 bool) error { + err := anotherCall() + if err != nil { + return err + } else if cond1 { + return nil + } else if cond2 { + // ruleid: pkg-errors-wrap-nil-err + return errors.WithStack(err) + } + return nil +} + +// TP28: assignment-form wrap inside an `else if` arm. +func tp28(cond bool) error { + err := anotherCall() + if err != nil { + return err + } else if cond { + // ruleid: pkg-errors-wrap-nil-err + err = errors.Wrap(err, "assignment-form in else-if") + return err + } + return nil +} + +// TP29: `else if` arm with init clause. +func tp29() error { + err := anotherCall() + if err != nil { + return err + } else if x := 1; x > 0 { + // ruleid: pkg-errors-wrap-nil-err + return errors.Wrapf(err, "init else-if: %d", x) + } + return nil +} + +// TN28: `else if` arm where err is reassigned before the wrap — wrap +// is on a freshly-assigned (potentially non-nil) err. +func tn28(cond bool) error { + err := anotherCall() + if err != nil { + return err + } else if cond { + _, err = mayFail() + // ok: pkg-errors-wrap-nil-err + return errors.Wrap(err, "after reassign in else-if") + } + return nil +} + +// TN29: `:=` shadow inside the wrap branch. `_, err := mayFail()` in a +// nested scope declares a fresh `err` distinct from the proven-nil +// outer one, so the wrap is on the fresh, possibly non-nil shadow. +func tn29(cond bool) error { + err := anotherCall() + if err != nil { + return err + } + + if cond { + _, err := mayFail() + // ok: pkg-errors-wrap-nil-err + return errors.Wrap(err, "shadowed via :=") + } + return nil +} + +type repo struct{} + +func (r *repo) load() (string, error) { return "", nil } +func (r *repo) check() error { return nil } + +// TP30: receiver method call (single-LHS) returning error — proven-nil +// reasoning applies the same way as for package-level functions. +func tp30() error { + r := &repo{} + err := r.check() + if err != nil { + return err + } + + if someCondition() { + // ruleid: pkg-errors-wrap-nil-err + return errors.Wrap(err, "after method-call guard") + } + return nil +} + +// TP31: tuple-form receiver method call. +func tp31() error { + r := &repo{} + id, err := r.load() + if err != nil { + return err + } + + if someCondition() { + // ruleid: pkg-errors-wrap-nil-err + return errors.Wrapf(err, "tuple method call: %s", id) + } + return nil +} + +// TP32: named return value with naked `return` inside the guard. +// After `if err != nil { return }`, `err` is still provably nil at the +// trailing wrap. +func tp32() (err error) { + err = anotherCall() + if err != nil { + return + } + + if someCondition() { + // ruleid: pkg-errors-wrap-nil-err + return errors.Wrap(err, "naked return guard") + } + return nil +} + +// TP33: nested `else { else { wrap } }` chain (with explicit braces, +// not else-if). Wrap inside the inner else still runs on the +// proven-nil-err path. +func tp33(cond1, cond2 bool) error { + err := anotherCall() + if err != nil { + return err + } + + if cond1 { + return nil + } else { + if cond2 { + return nil + } else { + // ruleid: pkg-errors-wrap-nil-err + return errors.Wrap(err, "else of else") + } + } +} + +// TN30: single-LHS `:=` shadow (not tuple) inside a nested scope before +// the wrap. The fresh `err :=` declares a new variable distinct from +// the proven-nil outer one. +func tn30(cond bool) error { + err := anotherCall() + if err != nil { + return err + } + + if cond { + err := anotherCall() + // ok: pkg-errors-wrap-nil-err + return errors.Wrap(err, "single-LHS := shadow") + } + return nil +} + +// Wrap inside a `switch`-case after a guard is a known FN: Case A's +// `pattern-inside` enumerates `if`-blocks but not `case` bodies, so +// the wrap is not recognized as being inside a non-err conditional. +func tnSwitchCaseKnownFN(cond int) error { + err := anotherCall() + if err != nil { + return err + } + + switch cond { + case 1: + // ok: pkg-errors-wrap-nil-err + return errors.Wrap(err, "switch-case wrap (known FN)") + } + return nil +} + +// Wrap inside a goroutine launched after the guard. Captured `err` may +// be mutated by the outer function before the goroutine runs, so the +// proven-nil reasoning is unsound across goroutine boundaries — +// documented here for awareness. The rule fires syntactically. +func tpGoroutineWrap() error { + err := anotherCall() + if err != nil { + return err + } + + go func() { + if someCondition() { + // ruleid: pkg-errors-wrap-nil-err + _ = errors.Wrap(err, "goroutine wrap") + } + }() + return nil +} + diff --git a/go/pkg-errors-wrap-nil-err.yaml b/go/pkg-errors-wrap-nil-err.yaml new file mode 100644 index 0000000..7f196d5 --- /dev/null +++ b/go/pkg-errors-wrap-nil-err.yaml @@ -0,0 +1,331 @@ +rules: + - id: pkg-errors-wrap-nil-err + languages: [go] + severity: ERROR + message: >- + `errors.$FN` is being called with an error variable that is provably + `nil` at this point. These functions return `nil` when their error + input is `nil`, so this silently swallows the error path: callers + see success when the surrounding condition was meant to surface a + failure. + metadata: + category: security + cwe: "CWE-754: Improper Check for Unusual or Exceptional Conditions" + subcategory: [audit] + confidence: MEDIUM + likelihood: MEDIUM + impact: MEDIUM + technology: [--no-technology--] + description: "`pkg/errors` wrap-family call on a provably-nil error — silently swallows the failure path" + references: + - https://pkg.go.dev/github.com/pkg/errors#Wrap + - https://pkg.go.dev/github.com/pkg/errors#WithMessage + - https://pkg.go.dev/github.com/pkg/errors#WithStack + patterns: + # Anchor to files that import github.com/pkg/errors. Other packages + # aliased as `errors` (cockroachdb/errors, emperror.dev/errors) may + # have different nil-input semantics. + - pattern-inside: | + import "github.com/pkg/errors" + ... + # pkg/errors helpers that return nil for nil error input. + - metavariable-regex: + metavariable: $FN + regex: ^(Wrap|Wrapf|WithMessage|WithMessagef|WithStack)$ + # err-like identifiers: `e`, `err`, `err\d*`, `errFoo`, `xErr`, + # `parseError`. Excludes substring-only matches like `terraform`. + - metavariable-regex: + metavariable: $ERR + regex: ^(e|err\d*|error|err[A-Z][a-zA-Z0-9]*|[a-z][a-zA-Z0-9]*Err[a-zA-Z0-9]*|[a-z][a-zA-Z0-9]*Error[a-zA-Z0-9]*)$ + - pattern-either: + # Case A: wrap inside an `if $COND { ... }` after a preceding + # `if $ERR != nil { }` guard. Excluded when the + # surrounding `if` is itself an err-check (compound `&&` allowed) + # or when $ERR is reassigned/shadowed before the wrap. + - patterns: + - pattern: errors.$FN($ERR, ...) + # Wrap must sit inside an `if $COND { ... }` block. + - pattern-inside: | + if $COND { + ... + } + # Guards whose body ends in a terminator (return / panic / + # log.Fatal* / os.Exit / runtime.Goexit). After such a guard, + # $ERR is provably nil at the wrap site. + - pattern-either: + - pattern-inside: | + if $ERR != nil { + ... + return ... + } + ... + - pattern-inside: | + if $ERR != nil { + ... + panic(...) + } + ... + - pattern-inside: | + if $ERR != nil { + ... + log.Fatal(...) + } + ... + - pattern-inside: | + if $ERR != nil { + ... + log.Fatalf(...) + } + ... + - pattern-inside: | + if $ERR != nil { + ... + log.Fatalln(...) + } + ... + - pattern-inside: | + if $ERR != nil { + ... + os.Exit(...) + } + ... + - pattern-inside: | + if $ERR != nil { + ... + runtime.Goexit() + } + ... + # Surrounding `if` already constrains $ERR (direct or as a + # conjunct of `&&`). + - pattern-not-inside: | + if $ERR != nil { + ... + } + - pattern-not-inside: | + if $ERR != nil && ... { + ... + } + - pattern-not-inside: | + if ... && $ERR != nil { + ... + } + - pattern-not-inside: | + if ... && $ERR != nil && ... { + ... + } + - pattern-not-inside: | + if $ERR == nil { + ... + } + # Reverse guard: `if $ERR == nil { }` proves + # $ERR non-nil at the wrap site. Same terminator set. + - pattern-not-inside: | + if $ERR == nil { + ... + return ... + } + ... + - pattern-not-inside: | + if $ERR == nil { + ... + panic(...) + } + ... + - pattern-not-inside: | + if $ERR == nil { + ... + log.Fatal(...) + } + ... + - pattern-not-inside: | + if $ERR == nil { + ... + log.Fatalf(...) + } + ... + - pattern-not-inside: | + if $ERR == nil { + ... + log.Fatalln(...) + } + ... + - pattern-not-inside: | + if $ERR == nil { + ... + os.Exit(...) + } + ... + - pattern-not-inside: | + if $ERR == nil { + ... + runtime.Goexit() + } + ... + # $ERR reassigned (or `:=`-shadowed) IMMEDIATELY before the + # wrap. Tight-adjacent only: a `...` between would traverse + # nested blocks and over-subtract any outer `err = ...` + # against an inner wrap. Reassignment separated from the wrap + # by other stmts in the same block remains a known FP. + - pattern-not-inside: | + $ERR = ... + return errors.$FN($ERR, ...) + - pattern-not-inside: | + ..., $ERR = ... + return errors.$FN($ERR, ...) + - pattern-not-inside: | + $ERR := ... + return errors.$FN($ERR, ...) + - pattern-not-inside: | + ..., $ERR := ... + return errors.$FN($ERR, ...) + # Case B: wrap inside the ELSE body of `if $ERR != nil { ... } else { ... }`. + # Matches both `return errors.Wrap(err, ...)` and the assignment + # form `err = errors.Wrap(err, ...)`. + - patterns: + - pattern: | + if $ERR != nil { + ... + } else { + ... + $RET + ... + } + - metavariable-pattern: + metavariable: $RET + patterns: + - pattern-either: + - pattern: return errors.$FN($ERR, ...) + - pattern: $ERR = errors.$FN($ERR, ...) + # `... $RET ...` could bind $RET to any stmt-level node + # (including an enclosing if-stmt containing the wrap). + # Anchor $RET's text to the return/assignment shape. + - metavariable-regex: + metavariable: $RET + regex: '^(return\b|[a-zA-Z_][a-zA-Z0-9_]*\s*=(?!=))' + # $ERR reassigned (or `:=`-shadowed) IMMEDIATELY before the + # wrap (return-form) inside the else body. Tight-adjacent only. + - pattern-not: | + if $ERR != nil { + ... + } else { + ... + $ERR = ... + return errors.$FN($ERR, ...) + } + - pattern-not: | + if $ERR != nil { + ... + } else { + ... + ..., $ERR = ... + return errors.$FN($ERR, ...) + } + - pattern-not: | + if $ERR != nil { + ... + } else { + ... + $ERR := ... + return errors.$FN($ERR, ...) + } + - pattern-not: | + if $ERR != nil { + ... + } else { + ... + ..., $ERR := ... + return errors.$FN($ERR, ...) + } + # Same shape for the assignment-form wrap. + - pattern-not: | + if $ERR != nil { + ... + } else { + ... + $ERR = ... + $ERR = errors.$FN($ERR, ...) + } + - pattern-not: | + if $ERR != nil { + ... + } else { + ... + ..., $ERR = ... + $ERR = errors.$FN($ERR, ...) + } + - pattern-not: | + if $ERR != nil { + ... + } else { + ... + $ERR := ... + $ERR = errors.$FN($ERR, ...) + } + - pattern-not: | + if $ERR != nil { + ... + } else { + ... + ..., $ERR := ... + $ERR = errors.$FN($ERR, ...) + } + - focus-metavariable: $RET + + # Case C: wrap inside an `else if` arm after `if $ERR != nil`. + # Go parses `else if $COND { ... }` as `else { if $COND { ... } }` + # — the else-body is the inner if-stmt directly, not a multi-stmt + # block. Semgrep's `... $RET ...` needs a multi-stmt block, so + # Case B doesn't catch this shape. + - patterns: + - pattern: errors.$FN($ERR, ...) + - pattern-either: + # Plain `else if $COND` — Semgrep binds $INNER to the + # inner if-stmt via `else { $INNER }`. + - patterns: + - pattern-inside: | + if $ERR != nil { + ... + } else { + $INNER + } + - metavariable-pattern: + metavariable: $INNER + pattern: | + if $COND { + ... + } + # `else if $INIT; $COND` — Semgrep doesn't bind $INNER + # via `else { $INNER }` for init-form else-ifs, so the + # shape needs an explicit pattern. + - pattern-inside: | + if $ERR != nil { + ... + } else if $INIT; $COND { + ... + } + # $ERR reassigned (or `:=`-shadowed) IMMEDIATELY before the + # wrap. Tight-adjacent only (same caveat as Case A). + - pattern-not-inside: | + $ERR = ... + return errors.$FN($ERR, ...) + - pattern-not-inside: | + ..., $ERR = ... + return errors.$FN($ERR, ...) + - pattern-not-inside: | + $ERR := ... + return errors.$FN($ERR, ...) + - pattern-not-inside: | + ..., $ERR := ... + return errors.$FN($ERR, ...) + - pattern-not-inside: | + $ERR = ... + $ERR = errors.$FN($ERR, ...) + - pattern-not-inside: | + ..., $ERR = ... + $ERR = errors.$FN($ERR, ...) + - pattern-not-inside: | + $ERR := ... + $ERR = errors.$FN($ERR, ...) + - pattern-not-inside: | + ..., $ERR := ... + $ERR = errors.$FN($ERR, ...) diff --git a/go/shadowed-err-check.go b/go/shadowed-err-check.go new file mode 100644 index 0000000..ddf4684 --- /dev/null +++ b/go/shadowed-err-check.go @@ -0,0 +1,225 @@ +package fixtures + +import ( + "context" + "errors" + "fmt" + "net/http" +) + +func someFunc() error { return nil } +func multiReturn() (int, error) { return 0, nil } +func tripleReturn() (int, string, error) { return 0, "", nil } +func xFunc(_ context.Context) error { return nil } +func compute() int { return 0 } +func httpServer() *http.Server { return &http.Server{} } +func newCtx() context.Context { return context.Background() } +func errorsJoin(errs ...error) error { return errors.Join(errs...) } +func wrap(s string, e error) error { return fmt.Errorf(s+": %w", e) } + +func bugSimple(err error) error { + // ruleid: shadowed-err-check + if xErr := xFunc(newCtx()); err != nil { + return wrap("x", xErr) + } + return nil +} + +func bugListenAndServe() (err error) { + srv := httpServer() + // ruleid: shadowed-err-check + if svrErr := srv.ListenAndServe(); err != nil { + err = errorsJoin(err, wrap("listen", svrErr)) + } + return +} + +func bugMultiAssign() (err error) { + // ruleid: shadowed-err-check + if v, xErr := multiReturn(); err != nil { + _ = v + err = errorsJoin(err, xErr) + } + return +} + +func bugTripleAssign() (err error) { + // ruleid: shadowed-err-check + if a, b, innerErr := tripleReturn(); err != nil { + _ = a + _ = b + err = errorsJoin(err, innerErr) + } + return +} + +func bugEqualsNil() (err error) { + // ruleid: shadowed-err-check + if xErr := someFunc(); err == nil { + _ = xErr + } + return +} + + +func okSimple() error { + // ok: shadowed-err-check + if err := someFunc(); err != nil { + return err + } + return nil +} + +func okSimpleEqualsNil() { + // ok: shadowed-err-check + if err := someFunc(); err == nil { + fmt.Println("success") + } +} + +func okMultiAssign() error { + // ok: shadowed-err-check + if v, err := multiReturn(); err != nil { + _ = v + return err + } + return nil +} + +func okTripleAssign() error { + // ok: shadowed-err-check + if a, b, err := tripleReturn(); err != nil { + _ = a + _ = b + return err + } + return nil +} + +func okCustomNamedErr() error { + // ok: shadowed-err-check + if xErr := xFunc(newCtx()); xErr != nil { + return xErr + } + return nil +} + +func okCustomNamedErrEqualsNil() { + // ok: shadowed-err-check + if svrErr := someFunc(); svrErr == nil { + fmt.Println("ok") + } +} + +func okNonErrorVariable() { + // ok: shadowed-err-check + if err := compute(); err > 0 { + fmt.Println(err) + } +} + +func okNonErrorMultiAssign() { + // ok: shadowed-err-check + if v, ok := map[string]int{"a": 1}["a"]; ok { + _ = v + } +} + +func okConditionUsesOtherBoolean() { + flag := true + // ok: shadowed-err-check + if err := someFunc(); flag { + _ = err + } +} + +// Variable names that merely contain `err` as a substring (e.g. +// `terraform`, `cherry`) must not be treated as error variables. +func okSubstringNotErrLike(cherry int) { + // ok: shadowed-err-check + if terraform := compute(); cherry > 0 { + _ = terraform + } +} + +// Same substring-rejection rule applied to the CHECK side: LOCAL is +// err-like, but the variable being compared to nil is not. +func okCheckSideSubstringNotErrLike() { + var terraform error + // ok: shadowed-err-check + if xErr := someFunc(); terraform != nil { + _ = xErr + } +} + +// Single-letter `e` LOCAL — exercises the `e` branch of the regex. +func bugSingleLetterLocal(err error) error { + // ruleid: shadowed-err-check + if e := someFunc(); err != nil { + return e + } + return nil +} + +// Numeric-suffix `err1` LOCAL — exercises the `err\d*` branch. +func bugNumericSuffixLocal(err error) error { + // ruleid: shadowed-err-check + if err1 := someFunc(); err != nil { + return err1 + } + return nil +} + +// Err-prefix CamelCase `errFoo` LOCAL — exercises the +// `err[A-Z][a-zA-Z0-9]*` branch. +func bugErrPrefixLocal(err error) error { + // ruleid: shadowed-err-check + if errFoo := someFunc(); err != nil { + return errFoo + } + return nil +} + +// Error-suffix `parseError` LOCAL — exercises the +// `[a-z][a-zA-Z0-9]*Error[a-zA-Z0-9]*` branch. +func bugErrorSuffixLocal(err error) error { + // ruleid: shadowed-err-check + if parseError := someFunc(); err != nil { + return parseError + } + return nil +} + +// Cross-name typo: both LOCAL and CHECK are err-like, but distinct +// identifiers. Canonical shadowed-check shape — declared `xErr` is +// silently ignored while the outer `yErr` is checked. +func bugCrossNameTypo(yErr error) error { + // ruleid: shadowed-err-check + if xErr := someFunc(); yErr != nil { + return xErr + } + return nil +} + +// Err in non-final tuple position (`if err, ok := f(); ok`). The +// rule's `..., $LOCAL := $EXPR` pattern only binds $LOCAL to the +// LAST element, so $LOCAL here would be `ok` (not err-like) and the +// rule does not fire — no shadowed-err shape to flag. +func okErrInFirstTuplePos() { + tupleFn := func() (error, bool) { return nil, true } + // ok: shadowed-err-check + if err, ok := tupleFn(); ok { + _ = err + } +} + +// Compound `&&` check is a known FN: the rule's pattern matches +// `$CHECK != nil` directly, not `$CHECK != nil && ...`. Documented +// here so a future change extending the pattern can update this +// marker. +func okCompoundCheckKnownFN(err error, retries int) { + // ok: shadowed-err-check + if xErr := someFunc(); err != nil && retries < 3 { + _ = xErr + } +} diff --git a/go/shadowed-err-check.yaml b/go/shadowed-err-check.yaml new file mode 100644 index 0000000..b0df8eb --- /dev/null +++ b/go/shadowed-err-check.yaml @@ -0,0 +1,70 @@ +rules: + - id: shadowed-err-check + languages: [go] + severity: ERROR + message: >- + The `if` statement declares an error-like variable but then checks a + different variable against `nil`. This is almost always a copy-paste / + typo bug: the error returned by the function call in the init clause is + silently ignored, while a stale variable from an outer scope is checked + instead. + metadata: + category: security + cwe: "CWE-754: Improper Check for Unusual or Exceptional Conditions" + subcategory: [audit] + confidence: MEDIUM + likelihood: HIGH + impact: MEDIUM + technology: [--no-technology--] + description: "Shadowed error variable: declared `err` is not the one checked against `nil`" + references: + - https://temporal.io/blog/go-shadowing-bad-choices#the-bug + patterns: + - pattern-either: + - pattern: | + if $LOCAL := $EXPR; $CHECK != nil { + ... + } + - pattern: | + if $LOCAL := $EXPR; $CHECK == nil { + ... + } + - pattern: | + if ..., $LOCAL := $EXPR; $CHECK != nil { + ... + } + - pattern: | + if ..., $LOCAL := $EXPR; $CHECK == nil { + ... + } + # Both declared and checked variables must be err-like. Matches + # `e`, `err`, `err\d*`, `errFoo`, `xErr`, `parseError`. Rejects + # accidental substring matches like `terraform`. Also avoids + # legit non-err shapes like `if v, ok := m[k]; ok { ... }`. + - metavariable-regex: + metavariable: $LOCAL + regex: ^(e|err\d*|error|err[A-Z][a-zA-Z0-9]*|[a-z][a-zA-Z0-9]*Err[a-zA-Z0-9]*|[a-z][a-zA-Z0-9]*Error[a-zA-Z0-9]*)$ + - metavariable-regex: + metavariable: $CHECK + regex: ^(e|err\d*|error|err[A-Z][a-zA-Z0-9]*|[a-z][a-zA-Z0-9]*Err[a-zA-Z0-9]*|[a-z][a-zA-Z0-9]*Error[a-zA-Z0-9]*)$ + # Subtract the correct case: declared var IS the var being checked. + # `$SAME` is a fresh metavariable (distinct from $LOCAL/$CHECK) so + # Semgrep unifies its two occurrences within this `pattern-not`. + # Reusing $LOCAL/$CHECK wouldn't work — Semgrep doesn't unify + # metavariables across the `pattern-not` boundary. + - pattern-not: | + if $SAME := $E; $SAME != nil { + ... + } + - pattern-not: | + if $SAME := $E; $SAME == nil { + ... + } + - pattern-not: | + if ..., $SAME := $E; $SAME != nil { + ... + } + - pattern-not: | + if ..., $SAME := $E; $SAME == nil { + ... + }