From e24516d66e6d0c11dc59b82cdc9602627e811f1d Mon Sep 17 00:00:00 2001 From: GrosQuildu Date: Wed, 6 May 2026 14:58:21 +0200 Subject: [PATCH 01/11] write 3 new rules --- go/http-error-missing-return.go | 222 +++++++++++++++++ go/http-error-missing-return.yaml | 159 +++++++++++++ go/pkg-errors-wrap-nil-err.go | 382 ++++++++++++++++++++++++++++++ go/pkg-errors-wrap-nil-err.yaml | 69 ++++++ go/shadowed-err-check.go | 134 +++++++++++ go/shadowed-err-check.yaml | 68 ++++++ 6 files changed, 1034 insertions(+) create mode 100644 go/http-error-missing-return.go create mode 100644 go/http-error-missing-return.yaml create mode 100644 go/pkg-errors-wrap-nil-err.go create mode 100644 go/pkg-errors-wrap-nil-err.yaml create mode 100644 go/shadowed-err-check.go create mode 100644 go/shadowed-err-check.yaml diff --git a/go/http-error-missing-return.go b/go/http-error-missing-return.go new file mode 100644 index 0000000..cb5b9e9 --- /dev/null +++ b/go/http-error-missing-return.go @@ -0,0 +1,222 @@ +package test + +import ( + "errors" + "fmt" + "log" + "net/http" + "os" +) + + +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")) +} + +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() {} + +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")) +} diff --git a/go/http-error-missing-return.yaml b/go/http-error-missing-return.yaml new file mode 100644 index 0000000..96cefa8 --- /dev/null +++ b/go/http-error-missing-return.yaml @@ -0,0 +1,159 @@ +rules: + - 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 + pattern-either: + # Case A — http.Error is the last statement of an `if`-body, and the + # if-block is followed by more work in the enclosing scope. + - patterns: + - pattern-either: + - pattern: | + if $COND { + ... + $ERR + } + $NEXT + - pattern: | + if $INIT; $COND { + ... + $ERR + } + $NEXT + - metavariable-regex: + metavariable: $ERR + regex: &http_error '^http\.Error\(' + - pattern-not: + patterns: + - pattern-either: + - pattern: | + if $COND { + ... + $TERM + ... + } + $NEXT + - pattern: | + if $INIT; $COND { + ... + $TERM + ... + } + $NEXT + - metavariable-regex: + metavariable: $TERM + regex: &terminator '^(return\b|panic\(|log\.Fatal(ln|f)?\(|os\.Exit\()' + - pattern-not: + patterns: + - pattern-either: + - pattern: | + if $COND { + ... + } + $NEXTTERM + - pattern: | + if $INIT; $COND { + ... + } + $NEXTTERM + - metavariable-regex: + metavariable: $NEXTTERM + regex: *terminator + - focus-metavariable: $ERR + + # Case A' — http.Error is the last statement of an `else`-body. + - patterns: + - pattern-either: + - pattern: | + if $COND { + ... + } else { + ... + $ERR + } + $NEXT + - pattern: | + if $INIT; $COND { + ... + } else { + ... + $ERR + } + $NEXT + - metavariable-regex: + metavariable: $ERR + regex: *http_error + - pattern-not: + patterns: + - pattern-either: + - pattern: | + if $COND { + ... + } else { + ... + $TERM + ... + } + $NEXT + - pattern: | + if $INIT; $COND { + ... + } else { + ... + $TERM + ... + } + $NEXT + - metavariable-regex: + metavariable: $TERM + regex: *terminator + - pattern-not: + patterns: + - pattern-either: + - pattern: | + if $COND { + ... + } + $NEXTTERM + - pattern: | + if $INIT; $COND { + ... + } + $NEXTTERM + - metavariable-regex: + metavariable: $NEXTTERM + regex: *terminator + - focus-metavariable: $ERR + + # Case B — http.Error is immediately followed by a non-terminator + # statement in the same block. + - patterns: + - pattern: | + http.Error(...) + $STMT + - pattern-not: + patterns: + - pattern: | + http.Error(...) + $TERM + - metavariable-regex: + metavariable: $TERM + regex: *terminator diff --git a/go/pkg-errors-wrap-nil-err.go b/go/pkg-errors-wrap-nil-err.go new file mode 100644 index 0000000..f49bf3f --- /dev/null +++ b/go/pkg-errors-wrap-nil-err.go @@ -0,0 +1,382 @@ +package testdata + +import ( + "fmt" + "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 +} + + +// 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 +} diff --git a/go/pkg-errors-wrap-nil-err.yaml b/go/pkg-errors-wrap-nil-err.yaml new file mode 100644 index 0000000..f016f40 --- /dev/null +++ b/go/pkg-errors-wrap-nil-err.yaml @@ -0,0 +1,69 @@ +rules: + - id: pkg-errors-wrap-nil-err + languages: [go] + severity: ERROR + message: >- + $FN is being called with an error variable that is provably `nil`. + This method returns `nil` when their 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: correctness + 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`/`Wrapf`/`WithMessage`/`WithMessagef`/`WithStack` called where the error argument is provably nil — wraps to nil, swallowing 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: + # Restrict $FN to the pkg/errors helpers that return nil when their + # error input is nil. + - metavariable-regex: + metavariable: $FN + regex: ^(Wrap|Wrapf|WithMessage|WithMessagef|WithStack)$ + # Restrict $ERR to identifiers that look like error variables. + - metavariable-regex: + metavariable: $ERR + regex: ^(e|err|.*[Ee]rr.*)$ + - pattern-either: + # Case A: wrap inside a non-err `if $COND { ... }` block, + # after a preceding `if $ERR != nil { return ... }` guard + - patterns: + - pattern: errors.$FN($ERR, ...) + - pattern-inside: | + if $ERR != nil { + return ... + } + ... + - pattern-inside: | + if $COND { + ... + } + - metavariable-pattern: + metavariable: $COND + patterns: + - pattern-not: $ERR != nil + - pattern-not: $ERR == nil + - pattern-not-inside: | + if $ERR != nil { + ... + } + # Case B: wrap inside the ELSE body of `if $ERR != nil { ... } else { ... }` + - patterns: + - pattern: | + if $ERR != nil { + ... + } else { + ... + $RET + ... + } + - metavariable-pattern: + metavariable: $RET + pattern: return errors.$FN($ERR, ...) + - focus-metavariable: $RET diff --git a/go/shadowed-err-check.go b/go/shadowed-err-check.go new file mode 100644 index 0000000..89ff979 --- /dev/null +++ b/go/shadowed-err-check.go @@ -0,0 +1,134 @@ +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 + } +} diff --git a/go/shadowed-err-check.yaml b/go/shadowed-err-check.yaml new file mode 100644 index 0000000..bdd213b --- /dev/null +++ b/go/shadowed-err-check.yaml @@ -0,0 +1,68 @@ +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`" + patterns: + # Match any `if := ; nil { ... }` where the + # declared variable and the variable in the check are both error-like. + - 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 the declared variable and the variable being checked must look + # like error variables. This focuses the rule on the real bug class + # (shadowed/typo'd error checks) and avoids false positives from + # legitimate uses such as `if v, ok := m[k]; ok { ... }`. + - metavariable-regex: + metavariable: $LOCAL + regex: ^(e|err|.*[Ee]rr.*)$ + - metavariable-regex: + metavariable: $CHECK + regex: ^(e|err|.*[Ee]rr.*)$ + # Subtract the correct case: when the declared variable IS the variable + # being checked. `pattern-not` enforces this via metavariable + # unification — `$SAME` must be identical on both sides. + - 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 { + ... + } From 91f8ffe32e1bed5d49f58ac706c9df2e390d5ec7 Mon Sep 17 00:00:00 2001 From: GrosQuildu Date: Wed, 6 May 2026 15:12:25 +0200 Subject: [PATCH 02/11] fix yaml validation --- go/http-error-missing-return.yaml | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/go/http-error-missing-return.yaml b/go/http-error-missing-return.yaml index 96cefa8..e966ee6 100644 --- a/go/http-error-missing-return.yaml +++ b/go/http-error-missing-return.yaml @@ -40,7 +40,7 @@ rules: $NEXT - metavariable-regex: metavariable: $ERR - regex: &http_error '^http\.Error\(' + regex: '^http\.Error\(' - pattern-not: patterns: - pattern-either: @@ -60,7 +60,7 @@ rules: $NEXT - metavariable-regex: metavariable: $TERM - regex: &terminator '^(return\b|panic\(|log\.Fatal(ln|f)?\(|os\.Exit\()' + regex: '^(return\b|panic\(|log\.Fatal(ln|f)?\(|os\.Exit\()' - pattern-not: patterns: - pattern-either: @@ -76,7 +76,7 @@ rules: $NEXTTERM - metavariable-regex: metavariable: $NEXTTERM - regex: *terminator + regex: '^(return\b|panic\(|log\.Fatal(ln|f)?\(|os\.Exit\()' - focus-metavariable: $ERR # Case A' — http.Error is the last statement of an `else`-body. @@ -100,7 +100,7 @@ rules: $NEXT - metavariable-regex: metavariable: $ERR - regex: *http_error + regex: '^http\.Error\(' - pattern-not: patterns: - pattern-either: @@ -124,7 +124,7 @@ rules: $NEXT - metavariable-regex: metavariable: $TERM - regex: *terminator + regex: '^(return\b|panic\(|log\.Fatal(ln|f)?\(|os\.Exit\()' - pattern-not: patterns: - pattern-either: @@ -140,7 +140,7 @@ rules: $NEXTTERM - metavariable-regex: metavariable: $NEXTTERM - regex: *terminator + regex: '^(return\b|panic\(|log\.Fatal(ln|f)?\(|os\.Exit\()' - focus-metavariable: $ERR # Case B — http.Error is immediately followed by a non-terminator @@ -156,4 +156,4 @@ rules: $TERM - metavariable-regex: metavariable: $TERM - regex: *terminator + regex: '^(return\b|panic\(|log\.Fatal(ln|f)?\(|os\.Exit\()' From 4ca7638f193d96e960fb92da5c29d1c11a0e62c7 Mon Sep 17 00:00:00 2001 From: GrosQuildu Date: Wed, 6 May 2026 15:12:31 +0200 Subject: [PATCH 03/11] update readme --- README.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/README.md b/README.md index dd571ca..1274d80 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`/`Wrapf`/`WithMessage`/`WithMessagef`/`WithStack` called where the error argument is provably nil — wraps to nil, swallowing 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) | 🟧 | 🌘 | | From 6fab03acbcc75df3b4e81e8ff430bd8130a59023 Mon Sep 17 00:00:00 2001 From: GrosQuildu Date: Wed, 6 May 2026 15:30:23 +0200 Subject: [PATCH 04/11] add reference to shadowed-err-check --- go/shadowed-err-check.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/go/shadowed-err-check.yaml b/go/shadowed-err-check.yaml index bdd213b..b0a1a9b 100644 --- a/go/shadowed-err-check.yaml +++ b/go/shadowed-err-check.yaml @@ -17,6 +17,8 @@ rules: 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: # Match any `if := ; nil { ... }` where the # declared variable and the variable in the check are both error-like. From 96b265b25e268a38daa8aa078fd650f14bc715be Mon Sep 17 00:00:00 2001 From: GrosQuildu Date: Wed, 6 May 2026 15:54:24 +0200 Subject: [PATCH 05/11] fix pkg-errors-wrap-nil-err fps --- go/pkg-errors-wrap-nil-err.go | 249 ++++++++++++++++++++++++++++++++ go/pkg-errors-wrap-nil-err.yaml | 67 ++++++++- 2 files changed, 309 insertions(+), 7 deletions(-) diff --git a/go/pkg-errors-wrap-nil-err.go b/go/pkg-errors-wrap-nil-err.go index f49bf3f..2f343d8 100644 --- a/go/pkg-errors-wrap-nil-err.go +++ b/go/pkg-errors-wrap-nil-err.go @@ -380,3 +380,252 @@ func tn10() error { } 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 +} + +// 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 +} diff --git a/go/pkg-errors-wrap-nil-err.yaml b/go/pkg-errors-wrap-nil-err.yaml index f016f40..8077444 100644 --- a/go/pkg-errors-wrap-nil-err.yaml +++ b/go/pkg-errors-wrap-nil-err.yaml @@ -31,8 +31,11 @@ rules: metavariable: $ERR regex: ^(e|err|.*[Ee]rr.*)$ - pattern-either: - # Case A: wrap inside a non-err `if $COND { ... }` block, - # after a preceding `if $ERR != nil { return ... }` guard + # Case A: wrap inside an `if $COND { ... }` block, after a + # preceding `if $ERR != nil { return ... }` guard. The wrap is + # excluded if the surrounding `if` is itself an err-check + # (compound `&&` allowed) or if $ERR is reassigned right + # before the wrap return. - patterns: - pattern: errors.$FN($ERR, ...) - pattern-inside: | @@ -44,15 +47,47 @@ rules: if $COND { ... } - - metavariable-pattern: - metavariable: $COND - patterns: - - pattern-not: $ERR != nil - - pattern-not: $ERR == nil + # The wrap is intentional when the surrounding `if` + # already constrains $ERR (directly or as one conjunct of + # an `&&` chain). - 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: an earlier `if $ERR == nil { return ... }` + # establishes that $ERR is NON-nil at the wrap site. + - pattern-not-inside: | + if $ERR == nil { + return ... + } + ... + # $ERR reassigned IMMEDIATELY before the wrap return — we + # can no longer prove $ERR is nil at the wrap. Tight + # adjacent shape only, so a reassignment followed by a + # fresh `if $ERR != nil { return ... }` guard (which DOES + # restore nil-ness at the wrap) still flags. + - 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 { ... }` - patterns: - pattern: | @@ -66,4 +101,22 @@ rules: - metavariable-pattern: metavariable: $RET pattern: return errors.$FN($ERR, ...) + # $ERR reassigned IMMEDIATELY before the wrap return inside + # the else body. + - pattern-not: | + if $ERR != nil { + ... + } else { + ... + $ERR = ... + return errors.$FN($ERR, ...) + } + - pattern-not: | + if $ERR != nil { + ... + } else { + ... + ..., $ERR = ... + return errors.$FN($ERR, ...) + } - focus-metavariable: $RET From f216cde79e84f5dff2242ee0899ae7ac2bd1b85f Mon Sep 17 00:00:00 2001 From: GrosQuildu Date: Wed, 6 May 2026 17:11:15 +0200 Subject: [PATCH 06/11] fix issues, improve false positive detection --- go/http-error-missing-return.go | 39 +++++++++++ go/http-error-missing-return.yaml | 103 +++++++++++++++++++++++++++++- go/pkg-errors-wrap-nil-err.go | 59 +++++++++++++++++ go/pkg-errors-wrap-nil-err.yaml | 41 ++++++++++-- go/shadowed-err-check.go | 9 +++ go/shadowed-err-check.yaml | 9 ++- 6 files changed, 249 insertions(+), 11 deletions(-) diff --git a/go/http-error-missing-return.go b/go/http-error-missing-return.go index cb5b9e9..7cdd0de 100644 --- a/go/http-error-missing-return.go +++ b/go/http-error-missing-return.go @@ -205,6 +205,45 @@ func handlerReturnPrefixIsNotTerminator(w http.ResponseWriter, r *http.Request) 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") +} + func handlerNestedNoInnerReturn(w http.ResponseWriter, r *http.Request) { if err := doSomething(r); err != nil { if errors.Is(err, errSentinel) { diff --git a/go/http-error-missing-return.yaml b/go/http-error-missing-return.yaml index e966ee6..7743819 100644 --- a/go/http-error-missing-return.yaml +++ b/go/http-error-missing-return.yaml @@ -143,17 +143,116 @@ rules: regex: '^(return\b|panic\(|log\.Fatal(ln|f)?\(|os\.Exit\()' - focus-metavariable: $ERR + # Case A'' — http.Error is the last stmt of an `else if` branch. + # `else if` parses as `else { if ... }`, so the inner if's + # surrounding-scope `$NEXT` must be at the level of the outer + # if-stmt (after the whole chain), not within the else block. + - patterns: + - pattern-either: + - pattern: | + if $COND1 { + ... + } else if $COND2 { + ... + $ERR + } + $NEXT + - pattern: | + if $INIT; $COND1 { + ... + } else if $COND2 { + ... + $ERR + } + $NEXT + - pattern: | + if $COND1 { + ... + } else if $INIT2; $COND2 { + ... + $ERR + } + $NEXT + - metavariable-regex: + metavariable: $ERR + regex: '^http\.Error\(' + - pattern-not: + patterns: + - pattern-either: + - pattern: | + if $COND1 { + ... + } else if $COND2 { + ... + $TERM + ... + } + $NEXT + - pattern: | + if $INIT; $COND1 { + ... + } else if $COND2 { + ... + $TERM + ... + } + $NEXT + - pattern: | + if $COND1 { + ... + } else if $INIT2; $COND2 { + ... + $TERM + ... + } + $NEXT + - metavariable-regex: + metavariable: $TERM + regex: '^(return\b|panic\(|log\.Fatal(ln|f)?\(|os\.Exit\()' + - pattern-not: + patterns: + - pattern-either: + - pattern: | + if $COND1 { + ... + } else if $COND2 { + ... + } + $NEXTTERM + - pattern: | + if $INIT; $COND1 { + ... + } else if $COND2 { + ... + } + $NEXTTERM + - pattern: | + if $COND1 { + ... + } else if $INIT2; $COND2 { + ... + } + $NEXTTERM + - metavariable-regex: + metavariable: $NEXTTERM + regex: '^(return\b|panic\(|log\.Fatal(ln|f)?\(|os\.Exit\()' + - focus-metavariable: $ERR + # Case B — http.Error is immediately followed by a non-terminator # statement in the same block. - patterns: - pattern: | - http.Error(...) + $ERR $STMT + - metavariable-regex: + metavariable: $ERR + regex: '^http\.Error\(' - pattern-not: patterns: - pattern: | - http.Error(...) + $ERR $TERM - metavariable-regex: metavariable: $TERM regex: '^(return\b|panic\(|log\.Fatal(ln|f)?\(|os\.Exit\()' + - focus-metavariable: $ERR diff --git a/go/pkg-errors-wrap-nil-err.go b/go/pkg-errors-wrap-nil-err.go index 2f343d8..8f05c61 100644 --- a/go/pkg-errors-wrap-nil-err.go +++ b/go/pkg-errors-wrap-nil-err.go @@ -2,6 +2,8 @@ package testdata import ( "fmt" + "log" + "github.com/pkg/errors" ) @@ -604,6 +606,63 @@ func tn22(cond bool) error { 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) { diff --git a/go/pkg-errors-wrap-nil-err.yaml b/go/pkg-errors-wrap-nil-err.yaml index 8077444..cadbb2c 100644 --- a/go/pkg-errors-wrap-nil-err.yaml +++ b/go/pkg-errors-wrap-nil-err.yaml @@ -8,7 +8,7 @@ rules: silently swallows the error path: callers see success when the surrounding condition was meant to surface a failure. metadata: - category: correctness + category: security cwe: "CWE-754: Improper Check for Unusual or Exceptional Conditions" subcategory: [audit] confidence: MEDIUM @@ -27,9 +27,13 @@ rules: metavariable: $FN regex: ^(Wrap|Wrapf|WithMessage|WithMessagef|WithStack)$ # Restrict $ERR to identifiers that look like error variables. + # Match `e`, `err`, `err\d*` (err1, err2), `errFoo` (err prefix + + # CamelCase), and `xErr` / `parseError` style names. Avoids + # accidental matches on words that merely contain the substring + # "err" (e.g. "terraform"). - metavariable-regex: metavariable: $ERR - regex: ^(e|err|.*[Ee]rr.*)$ + 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 { ... }` block, after a # preceding `if $ERR != nil { return ... }` guard. The wrap is @@ -40,6 +44,7 @@ rules: - pattern: errors.$FN($ERR, ...) - pattern-inside: | if $ERR != nil { + ... return ... } ... @@ -74,6 +79,7 @@ rules: # establishes that $ERR is NON-nil at the wrap site. - pattern-not-inside: | if $ERR == nil { + ... return ... } ... @@ -88,7 +94,10 @@ rules: - pattern-not-inside: | ..., $ERR = ... return errors.$FN($ERR, ...) - # Case B: wrap inside the ELSE body of `if $ERR != nil { ... } else { ... }` + # 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, ...)` — both swallow + # the failure path because Wrap returns nil for nil input. - patterns: - pattern: | if $ERR != nil { @@ -100,9 +109,12 @@ rules: } - metavariable-pattern: metavariable: $RET - pattern: return errors.$FN($ERR, ...) - # $ERR reassigned IMMEDIATELY before the wrap return inside - # the else body. + patterns: + - pattern-either: + - pattern: return errors.$FN($ERR, ...) + - pattern: $ERR = errors.$FN($ERR, ...) + # $ERR reassigned IMMEDIATELY before the wrap (return-form) + # inside the else body. - pattern-not: | if $ERR != nil { ... @@ -119,4 +131,21 @@ rules: ..., $ERR = ... return errors.$FN($ERR, ...) } + # Same FP 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, ...) + } - focus-metavariable: $RET diff --git a/go/shadowed-err-check.go b/go/shadowed-err-check.go index 89ff979..a1b42fb 100644 --- a/go/shadowed-err-check.go +++ b/go/shadowed-err-check.go @@ -132,3 +132,12 @@ func okConditionUsesOtherBoolean() { _ = 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 + } +} diff --git a/go/shadowed-err-check.yaml b/go/shadowed-err-check.yaml index b0a1a9b..62439a1 100644 --- a/go/shadowed-err-check.yaml +++ b/go/shadowed-err-check.yaml @@ -42,13 +42,16 @@ rules: # Both the declared variable and the variable being checked must look # like error variables. This focuses the rule on the real bug class # (shadowed/typo'd error checks) and avoids false positives from - # legitimate uses such as `if v, ok := m[k]; ok { ... }`. + # legitimate uses such as `if v, ok := m[k]; ok { ... }`. The regex + # matches `e`, `err`, `err\d*`, `errFoo` (err prefix + CamelCase), + # and `xErr` / `parseError` style names while rejecting accidental + # substring matches like `terraform`. - metavariable-regex: metavariable: $LOCAL - regex: ^(e|err|.*[Ee]rr.*)$ + 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|.*[Ee]rr.*)$ + 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: when the declared variable IS the variable # being checked. `pattern-not` enforces this via metavariable # unification — `$SAME` must be identical on both sides. From 13c7aaad41eff4a203ea5b2df293bd2eeb9d3652 Mon Sep 17 00:00:00 2001 From: GrosQuildu Date: Thu, 7 May 2026 10:23:54 +0200 Subject: [PATCH 07/11] less fps --- go/http-error-missing-return.go | 170 ++++++++++++- go/http-error-missing-return.yaml | 390 ++++++++++++------------------ go/pkg-errors-wrap-nil-err.go | 192 +++++++++++++++ go/pkg-errors-wrap-nil-err.yaml | 159 +++++++++++- 4 files changed, 669 insertions(+), 242 deletions(-) diff --git a/go/http-error-missing-return.go b/go/http-error-missing-return.go index 7cdd0de..6267070 100644 --- a/go/http-error-missing-return.go +++ b/go/http-error-missing-return.go @@ -6,6 +6,7 @@ import ( "log" "net/http" "os" + "runtime" ) @@ -48,7 +49,7 @@ func handlerLogThenContinue(w http.ResponseWriter, r *http.Request) { // 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 + // ruleid: http-error-missing-return-followed-by-stmt http.Error(w, "boom", http.StatusInternalServerError) w.Write([]byte("leaked body")) } @@ -195,7 +196,7 @@ func handlerCaseBOsExit(w http.ResponseWriter, r *http.Request) { // 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 + // ruleid: http-error-missing-return-followed-by-stmt http.Error(w, err.Error(), http.StatusInternalServerError) returnedSomething() } @@ -259,3 +260,168 @@ func handlerNestedNoInnerReturn(w http.ResponseWriter, r *http.Request) { 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` exits the loop entirely — handler then continues, but the call +// site after the break is the user's choice; the http.Error itself is +// followed by an explicit control-flow terminator. +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")) +} diff --git a/go/http-error-missing-return.yaml b/go/http-error-missing-return.yaml index 7743819..f78e425 100644 --- a/go/http-error-missing-return.yaml +++ b/go/http-error-missing-return.yaml @@ -1,4 +1,14 @@ rules: + # Case A — http.Error is the last statement of a containing block, + # and the surrounding if-chain or switch is followed by a non-terminator. + # Because Go parses `else if` as `else { if ... }`, the pattern-inside + # constraint naturally covers else-if chains of arbitrary depth. + # + # NOTE: this rule and `http-error-missing-return-followed-by-stmt` are + # split because Semgrep's `pattern-not-inside` does not subtract + # correctly when nested under `pattern-either > patterns`. Splitting + # keeps each rule's subtractions at the top level (where they work) + # while still emitting a single user-facing finding type. - id: http-error-missing-return languages: [go] severity: ERROR @@ -21,238 +31,152 @@ rules: description: "Missing `return` after `http.Error` lets the handler continue executing" references: - https://pkg.go.dev/net/http#Error - pattern-either: - # Case A — http.Error is the last statement of an `if`-body, and the - # if-block is followed by more work in the enclosing scope. - - patterns: - - pattern-either: - - pattern: | - if $COND { - ... - $ERR - } - $NEXT - - pattern: | - if $INIT; $COND { - ... - $ERR - } - $NEXT - - metavariable-regex: - metavariable: $ERR - regex: '^http\.Error\(' - - pattern-not: - patterns: - - pattern-either: - - pattern: | - if $COND { - ... - $TERM - ... - } - $NEXT - - pattern: | - if $INIT; $COND { - ... - $TERM - ... - } - $NEXT - - metavariable-regex: - metavariable: $TERM - regex: '^(return\b|panic\(|log\.Fatal(ln|f)?\(|os\.Exit\()' - - pattern-not: - patterns: - - pattern-either: - - pattern: | - if $COND { - ... - } - $NEXTTERM - - pattern: | - if $INIT; $COND { - ... - } - $NEXTTERM - - metavariable-regex: - metavariable: $NEXTTERM - regex: '^(return\b|panic\(|log\.Fatal(ln|f)?\(|os\.Exit\()' - - focus-metavariable: $ERR - - # Case A' — http.Error is the last statement of an `else`-body. - - patterns: - - pattern-either: - - pattern: | - if $COND { - ... - } else { - ... - $ERR - } - $NEXT - - pattern: | - if $INIT; $COND { - ... - } else { - ... - $ERR - } - $NEXT - - metavariable-regex: - metavariable: $ERR - regex: '^http\.Error\(' - - pattern-not: - patterns: - - pattern-either: - - pattern: | - if $COND { - ... - } else { - ... - $TERM - ... - } - $NEXT - - pattern: | - if $INIT; $COND { - ... - } else { - ... - $TERM - ... - } - $NEXT - - metavariable-regex: - metavariable: $TERM - regex: '^(return\b|panic\(|log\.Fatal(ln|f)?\(|os\.Exit\()' - - pattern-not: - patterns: - - pattern-either: - - pattern: | - if $COND { - ... - } - $NEXTTERM - - pattern: | - if $INIT; $COND { - ... - } - $NEXTTERM - - metavariable-regex: - metavariable: $NEXTTERM - regex: '^(return\b|panic\(|log\.Fatal(ln|f)?\(|os\.Exit\()' - - focus-metavariable: $ERR - - # Case A'' — http.Error is the last stmt of an `else if` branch. - # `else if` parses as `else { if ... }`, so the inner if's - # surrounding-scope `$NEXT` must be at the level of the outer - # if-stmt (after the whole chain), not within the else block. - - patterns: - - pattern-either: - - pattern: | - if $COND1 { - ... - } else if $COND2 { - ... - $ERR - } - $NEXT - - pattern: | - if $INIT; $COND1 { - ... - } else if $COND2 { - ... - $ERR - } - $NEXT - - pattern: | - if $COND1 { - ... - } else if $INIT2; $COND2 { - ... - $ERR - } - $NEXT - - metavariable-regex: - metavariable: $ERR - regex: '^http\.Error\(' - - pattern-not: - patterns: - - pattern-either: - - pattern: | - if $COND1 { - ... - } else if $COND2 { - ... - $TERM - ... - } - $NEXT - - pattern: | - if $INIT; $COND1 { - ... - } else if $COND2 { - ... - $TERM - ... - } - $NEXT - - pattern: | - if $COND1 { - ... - } else if $INIT2; $COND2 { - ... - $TERM - ... - } - $NEXT - - metavariable-regex: - metavariable: $TERM - regex: '^(return\b|panic\(|log\.Fatal(ln|f)?\(|os\.Exit\()' - - pattern-not: - patterns: - - pattern-either: - - pattern: | - if $COND1 { - ... - } else if $COND2 { - ... - } - $NEXTTERM - - pattern: | - if $INIT; $COND1 { - ... - } else if $COND2 { - ... - } - $NEXTTERM - - pattern: | - if $COND1 { - ... - } else if $INIT2; $COND2 { - ... - } - $NEXTTERM - - metavariable-regex: - metavariable: $NEXTTERM - regex: '^(return\b|panic\(|log\.Fatal(ln|f)?\(|os\.Exit\()' - - focus-metavariable: $ERR - - # Case B — http.Error is immediately followed by a non-terminator - # statement in the same block. - - patterns: - - pattern: | - $ERR - $STMT - - metavariable-regex: - metavariable: $ERR - regex: '^http\.Error\(' - - pattern-not: - patterns: + patterns: + # Anchor: the file imports `net/http`, so `http.Error` resolves to + # the stdlib symbol rather than a local identifier shadowing the name. + - pattern-inside: | + import "net/http" + ... + # Focus on the http.Error call. + - pattern: http.Error(...) + # http.Error is the tail of its containing block (no statement + # directly follows it inside the same block). + - pattern-not-inside: | + http.Error(...) + $ANY + # The focus is inside an if-chain or switch followed by $NEXT. + # With nested chains this naturally binds to the outermost + # containing structure with a $NEXT, so deep else-if chains work + # without case enumeration. + - 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 + # Subtract: $NEXT after some containing if-chain or switch is + # itself a terminator (return / panic / log.Fatal* / os.Exit / + # runtime.Goexit / continue / break / goto). With nesting, a + # terminator after ANY containing structure means control flow + # doesn't actually fall past http.Error. + - pattern-not-inside: + patterns: + - pattern-either: + - pattern: | + if $C2 { + ... + } + $TERM - pattern: | - $ERR + if $I2; $C2 { + ... + } $TERM - - metavariable-regex: - metavariable: $TERM - regex: '^(return\b|panic\(|log\.Fatal(ln|f)?\(|os\.Exit\()' - - focus-metavariable: $ERR + - 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)' + + # Case B — http.Error is immediately followed by a non-terminator + # statement in the same block (regardless of nesting context). + - id: http-error-missing-return-followed-by-stmt + 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: + - pattern-inside: | + import "net/http" + ... + - pattern: | + http.Error(...) + $STMT + - pattern-not: + 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)' diff --git a/go/pkg-errors-wrap-nil-err.go b/go/pkg-errors-wrap-nil-err.go index 8f05c61..76bfb69 100644 --- a/go/pkg-errors-wrap-nil-err.go +++ b/go/pkg-errors-wrap-nil-err.go @@ -3,6 +3,8 @@ package testdata import ( "fmt" "log" + "os" + "runtime" "github.com/pkg/errors" ) @@ -688,3 +690,193 @@ func tn23() (err error) { 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 +} diff --git a/go/pkg-errors-wrap-nil-err.yaml b/go/pkg-errors-wrap-nil-err.yaml index cadbb2c..d400447 100644 --- a/go/pkg-errors-wrap-nil-err.yaml +++ b/go/pkg-errors-wrap-nil-err.yaml @@ -42,12 +42,54 @@ rules: # before the wrap return. - patterns: - pattern: errors.$FN($ERR, ...) - - pattern-inside: | - if $ERR != nil { + # Accept any guard whose body ends in a control-flow + # terminator that prevents reaching the code below the + # `if`: `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() + } ... - return ... - } - ... - pattern-inside: | if $COND { ... @@ -75,14 +117,51 @@ rules: if $ERR == nil { ... } - # Reverse guard: an earlier `if $ERR == nil { return ... }` - # establishes that $ERR is NON-nil at the wrap site. + # Reverse guard: an earlier `if $ERR == nil { }` + # establishes that $ERR is NON-nil at the wrap site. Same + # terminator set as the forward guard above. - 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 IMMEDIATELY before the wrap return — we # can no longer prove $ERR is nil at the wrap. Tight # adjacent shape only, so a reassignment followed by a @@ -113,6 +192,14 @@ rules: - pattern-either: - pattern: return errors.$FN($ERR, ...) - pattern: $ERR = errors.$FN($ERR, ...) + # The `... $RET ...` shape lets Semgrep bind $RET to any + # statement-level node in the else-body — including an + # enclosing if-stmt or block whose body happens to contain + # the wrap. Anchor $RET's text to the actual return- or + # assignment-shape so we don't fire on the broader span. + - metavariable-regex: + metavariable: $RET + regex: '^(return |[a-zA-Z_][a-zA-Z0-9_]*\s*=[^=])' # $ERR reassigned IMMEDIATELY before the wrap (return-form) # inside the else body. - pattern-not: | @@ -149,3 +236,61 @@ rules: $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 { ... } }`, + # where the else-body is the inner if-stmt directly (not wrapped + # in an extra block). Semgrep's `... $RET ...` requires a + # multi-stmt block, so the basic Case B pattern doesn't bind + # $RET to the inner if-stmt — hence this separate case for the + # single-statement else-body shape. + # + # The wrap is reached only when `$ERR != nil` was FALSE (i.e. + # err is provably nil) AND the inner condition is TRUE — so the + # wrap swallows the error path the same way Case B does. No + # terminator in the if-body is needed: the else branch runs iff + # err is nil, regardless of what the if-body does. + - patterns: + - pattern: errors.$FN($ERR, ...) + - pattern-either: + # Plain `else if $COND` — the parser produces an + # else-body with a single if-stmt (no init). Semgrep + # binds $INNER to that 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 + # init form needs an explicit `else if $INIT; $COND` + # pattern. + - pattern-inside: | + if $ERR != nil { + ... + } else if $INIT; $COND { + ... + } + # FP: $ERR reassigned immediately before the wrap inside the + # else-if arm — wrap is on a freshly-assigned err, not the + # proven-nil one. + - 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, ...) From eb6f528ba34eaab6b5c6d216200224a33e7d7850 Mon Sep 17 00:00:00 2001 From: GrosQuildu Date: Thu, 7 May 2026 11:17:16 +0200 Subject: [PATCH 08/11] less fps - another pass --- go/pkg-errors-wrap-nil-err.go | 38 ++++++++++ go/pkg-errors-wrap-nil-err.yaml | 120 +++++++++++++++++++++++++------- go/shadowed-err-check.yaml | 11 ++- 3 files changed, 142 insertions(+), 27 deletions(-) diff --git a/go/pkg-errors-wrap-nil-err.go b/go/pkg-errors-wrap-nil-err.go index 76bfb69..47cd9fb 100644 --- a/go/pkg-errors-wrap-nil-err.go +++ b/go/pkg-errors-wrap-nil-err.go @@ -44,7 +44,27 @@ func tp2() error { 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 { @@ -880,3 +900,21 @@ func tn28(cond bool) error { } 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 +} + diff --git a/go/pkg-errors-wrap-nil-err.yaml b/go/pkg-errors-wrap-nil-err.yaml index d400447..96cd528 100644 --- a/go/pkg-errors-wrap-nil-err.yaml +++ b/go/pkg-errors-wrap-nil-err.yaml @@ -3,10 +3,11 @@ rules: languages: [go] severity: ERROR message: >- - $FN is being called with an error variable that is provably `nil`. - This method returns `nil` when their input is `nil`, so this - silently swallows the error path: callers see success when the - surrounding condition was meant to surface a failure. + `errors.$FN` is being called with an error variable that is likely + `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" @@ -15,12 +16,19 @@ rules: likelihood: MEDIUM impact: MEDIUM technology: [--no-technology--] - description: "`pkg/errors.Wrap`/`Wrapf`/`WithMessage`/`WithMessagef`/`WithStack` called where the error argument is provably nil — wraps to nil, swallowing the failure path" + description: "`pkg/errors` wrap-family call where the error argument is likely nil — wraps to nil, swallowing 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. Without this + # anchor the rule would also fire on calls to other packages + # aliased as `errors` (e.g. cockroachdb/errors, emperror.dev/errors) + # whose `Wrap` may have different nil-input semantics. + - pattern-inside: | + import "github.com/pkg/errors" + ... # Restrict $FN to the pkg/errors helpers that return nil when their # error input is nil. - metavariable-regex: @@ -35,13 +43,21 @@ rules: 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 { ... }` block, after a - # preceding `if $ERR != nil { return ... }` guard. The wrap is - # excluded if the surrounding `if` is itself an err-check - # (compound `&&` allowed) or if $ERR is reassigned right - # before the wrap return. + # Case A: wrap inside an `if $COND { ... }` block (structural + # constraint enforced via `pattern-inside: if $COND { ... }` + # below), after a preceding `if $ERR != nil { return ... }` + # guard. The wrap is excluded if the surrounding `if` is + # itself an err-check (compound `&&` allowed) or if $ERR is + # reassigned (or shadowed via `:=`) before the wrap return. - patterns: - pattern: errors.$FN($ERR, ...) + # Structural constraint: wrap must sit inside an + # `if $COND { ... }` block. Co-located with Case A's + # comment so the requirement is visible up-front. + - pattern-inside: | + if $COND { + ... + } # Accept any guard whose body ends in a control-flow # terminator that prevents reaching the code below the # `if`: `return`, `panic`, `log.Fatal*`, `os.Exit`, @@ -90,10 +106,6 @@ rules: runtime.Goexit() } ... - - pattern-inside: | - if $COND { - ... - } # The wrap is intentional when the surrounding `if` # already constrains $ERR (directly or as one conjunct of # an `&&` chain). @@ -162,17 +174,29 @@ rules: runtime.Goexit() } ... - # $ERR reassigned IMMEDIATELY before the wrap return — we - # can no longer prove $ERR is nil at the wrap. Tight - # adjacent shape only, so a reassignment followed by a - # fresh `if $ERR != nil { return ... }` guard (which DOES - # restore nil-ness at the wrap) still flags. + # $ERR reassigned (or shadowed via `:=`) IMMEDIATELY before + # the wrap return — we can no longer prove $ERR is nil + # at the wrap. Tight adjacent shape only: a multi-stmt + # `pattern-not-inside` with `...` between would traverse + # nested blocks and over-subtract any outer `err = ...` + # against an inner wrap. So a 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, ...) + # `:=` shadow: in a nested scope, `_, err := mayFail()` + # introduces a fresh `err` distinct from the proven-nil + # outer one. Same tight-adjacent constraint as above. + - 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, ...)` — both swallow @@ -200,8 +224,11 @@ rules: - metavariable-regex: metavariable: $RET regex: '^(return |[a-zA-Z_][a-zA-Z0-9_]*\s*=[^=])' - # $ERR reassigned IMMEDIATELY before the wrap (return-form) - # inside the else body. + # $ERR reassigned (or shadowed via `:=`) IMMEDIATELY before + # the wrap (return-form) inside the else body. Tight + # adjacent shape only — `...` between would traverse + # nested blocks (e.g. `if ... { err = X }` inside the + # else body) and over-subtract. - pattern-not: | if $ERR != nil { ... @@ -218,6 +245,22 @@ rules: ..., $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 FP shape for the assignment-form wrap. - pattern-not: | if $ERR != nil { @@ -235,6 +278,22 @@ rules: ..., $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`. @@ -279,18 +338,31 @@ rules: } else if $INIT; $COND { ... } - # FP: $ERR reassigned immediately before the wrap inside the - # else-if arm — wrap is on a freshly-assigned err, not the - # proven-nil one. + # FP: $ERR reassigned (or shadowed via `:=`) IMMEDIATELY + # before the wrap inside the else-if arm — wrap is on a + # fresh err, not the proven-nil one. Tight adjacent shape + # 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.yaml b/go/shadowed-err-check.yaml index 62439a1..2386806 100644 --- a/go/shadowed-err-check.yaml +++ b/go/shadowed-err-check.yaml @@ -52,9 +52,14 @@ rules: - 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: when the declared variable IS the variable - # being checked. `pattern-not` enforces this via metavariable - # unification — `$SAME` must be identical on both sides. + # Subtract the correct case: when the declared variable IS the + # variable being checked. `$SAME` is a fresh metavariable + # (intentionally distinct from `$LOCAL`/`$CHECK` above) so that + # within this single `pattern-not`, Semgrep unifies the two + # occurrences of `$SAME` and matches only when the same identifier + # appears on both sides. Reusing `$LOCAL`/`$CHECK` here would not + # work — Semgrep does not unify metavariables across the + # `pattern-not` boundary back to the outer patterns. - pattern-not: | if $SAME := $E; $SAME != nil { ... From bf38f790cb4bbf12efb5661cf7c1df5712a0a8c7 Mon Sep 17 00:00:00 2001 From: GrosQuildu Date: Thu, 7 May 2026 12:19:55 +0200 Subject: [PATCH 09/11] /pr-review pass --- README.md | 2 +- go/http-error-missing-return.go | 97 +++++++++++++++++++++++- go/pkg-errors-wrap-nil-err.go | 126 ++++++++++++++++++++++++++++++++ go/pkg-errors-wrap-nil-err.yaml | 6 +- go/shadowed-err-check.go | 82 +++++++++++++++++++++ 5 files changed, 306 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index 1274d80..3846e5c 100644 --- a/README.md +++ b/README.md @@ -60,7 +60,7 @@ $ semgrep --config /path/to/semgrep-rules/hanging-goroutine.yml -o leaks.txt' | [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`/`Wrapf`/`WithMessage`/`WithMessagef`/`WithStack` called where the error argument is provably nil — wraps to nil, swallowing the failure path | +| [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 | diff --git a/go/http-error-missing-return.go b/go/http-error-missing-return.go index 6267070..aa9b1d1 100644 --- a/go/http-error-missing-return.go +++ b/go/http-error-missing-return.go @@ -116,6 +116,9 @@ func handlerNestedReturn(w http.ResponseWriter, r *http.Request) { 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 @@ -245,6 +248,9 @@ func handlerElseIfNextTerm(w http.ResponseWriter, r *http.Request) { 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) { @@ -273,9 +279,11 @@ func handlerForLoopContinue(w http.ResponseWriter, r *http.Request, items []stri } } -// `break` exits the loop entirely — handler then continues, but the call -// site after the break is the user's choice; the http.Error itself is -// followed by an explicit control-flow terminator. +// `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" { @@ -425,3 +433,86 @@ func handlerSwitchDefault(w http.ResponseWriter, r *http.Request) { } 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/pkg-errors-wrap-nil-err.go b/go/pkg-errors-wrap-nil-err.go index 47cd9fb..8785824 100644 --- a/go/pkg-errors-wrap-nil-err.go +++ b/go/pkg-errors-wrap-nil-err.go @@ -918,3 +918,129 @@ func tn29(cond bool) error { 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 index 96cd528..585746f 100644 --- a/go/pkg-errors-wrap-nil-err.yaml +++ b/go/pkg-errors-wrap-nil-err.yaml @@ -3,7 +3,7 @@ rules: languages: [go] severity: ERROR message: >- - `errors.$FN` is being called with an error variable that is likely + `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 @@ -16,7 +16,7 @@ rules: likelihood: MEDIUM impact: MEDIUM technology: [--no-technology--] - description: "`pkg/errors` wrap-family call where the error argument is likely nil — wraps to nil, swallowing the failure path" + 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 @@ -223,7 +223,7 @@ rules: # assignment-shape so we don't fire on the broader span. - metavariable-regex: metavariable: $RET - regex: '^(return |[a-zA-Z_][a-zA-Z0-9_]*\s*=[^=])' + regex: '^(return\b|[a-zA-Z_][a-zA-Z0-9_]*\s*=(?!=))' # $ERR reassigned (or shadowed via `:=`) IMMEDIATELY before # the wrap (return-form) inside the else body. Tight # adjacent shape only — `...` between would traverse diff --git a/go/shadowed-err-check.go b/go/shadowed-err-check.go index a1b42fb..ddf4684 100644 --- a/go/shadowed-err-check.go +++ b/go/shadowed-err-check.go @@ -141,3 +141,85 @@ func okSubstringNotErrLike(cherry int) { _ = 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 + } +} From 0652c0830e18a71e99b044a08a531d7c3911bf50 Mon Sep 17 00:00:00 2001 From: GrosQuildu Date: Thu, 7 May 2026 12:54:46 +0200 Subject: [PATCH 10/11] merge http rules into one --- go/http-error-missing-return.go | 4 +- go/http-error-missing-return.yaml | 200 +++++++++++++++--------------- 2 files changed, 99 insertions(+), 105 deletions(-) diff --git a/go/http-error-missing-return.go b/go/http-error-missing-return.go index aa9b1d1..ce611e2 100644 --- a/go/http-error-missing-return.go +++ b/go/http-error-missing-return.go @@ -49,7 +49,7 @@ func handlerLogThenContinue(w http.ResponseWriter, r *http.Request) { // 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-followed-by-stmt + // ruleid: http-error-missing-return http.Error(w, "boom", http.StatusInternalServerError) w.Write([]byte("leaked body")) } @@ -199,7 +199,7 @@ func handlerCaseBOsExit(w http.ResponseWriter, r *http.Request) { // 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-followed-by-stmt + // ruleid: http-error-missing-return http.Error(w, err.Error(), http.StatusInternalServerError) returnedSomething() } diff --git a/go/http-error-missing-return.yaml b/go/http-error-missing-return.yaml index f78e425..1f4f538 100644 --- a/go/http-error-missing-return.yaml +++ b/go/http-error-missing-return.yaml @@ -1,14 +1,29 @@ rules: - # Case A — http.Error is the last statement of a containing block, - # and the surrounding if-chain or switch is followed by a non-terminator. - # Because Go parses `else if` as `else { if ... }`, the pattern-inside - # constraint naturally covers else-if chains of arbitrary depth. + # The rule has two cases combined under one user-facing id: # - # NOTE: this rule and `http-error-missing-return-followed-by-stmt` are - # split because Semgrep's `pattern-not-inside` does not subtract - # correctly when nested under `pattern-either > patterns`. Splitting - # keeps each rule's subtractions at the top level (where they work) - # while still emitting a single user-facing finding type. + # Case A — `http.Error` is the tail of its containing block, and the + # enclosing if-chain or switch is followed by another statement. + # Case B — `http.Error` is immediately followed by a statement in the + # same block (regardless of nesting context). + # + # The two cases are joined under a top-level `pattern-either`, then + # two top-level `pattern-not-inside` subtractions drop matches where + # the trailing statement is a control-flow terminator. The two + # subtractions are kept separate because they need different scoping: + # + # - The Case B subtraction (`http.Error(...)\n$TERM`) is shape-safe + # at the top level: the shape only matches Case B-style instances + # (where http.Error has a directly-following stmt), so it cannot + # wrongly subtract Case A matches. + # - The Case A subtraction (enclosing if/switch followed by $TERM) + # would over-subtract Case B matches whose enclosing structure + # also happens to be followed by a terminator. It is scoped to + # Case A by an extra `pattern-not http.Error(...)\n$ANY` + # conjunction (i.e., http.Error must be the tail of its block). + # + # Top-level placement of both subtractions is required because + # Semgrep silently ignores `pattern-not` / `pattern-not-inside` / + # `metavariable-regex` when nested under `pattern-either > patterns`. - id: http-error-missing-return languages: [go] severity: ERROR @@ -33,67 +48,83 @@ rules: - https://pkg.go.dev/net/http#Error patterns: # Anchor: the file imports `net/http`, so `http.Error` resolves to - # the stdlib symbol rather than a local identifier shadowing the name. + # the stdlib symbol rather than a local identifier shadowing it. - pattern-inside: | import "net/http" ... - # Focus on the http.Error call. - pattern: http.Error(...) - # http.Error is the tail of its containing block (no statement - # directly follows it inside the same block). - - pattern-not-inside: | - http.Error(...) - $ANY - # The focus is inside an if-chain or switch followed by $NEXT. - # With nested chains this naturally binds to the outermost - # containing structure with a $NEXT, so deep else-if chains work - # without case enumeration. - pattern-either: + # Case A — http.Error is the tail of its containing block, and + # the enclosing if-chain or switch is followed by $NEXT. With + # nested chains $NEXT naturally 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 — http.Error is directly followed by another stmt in + # the same block. - 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 - # Subtract: $NEXT after some containing if-chain or switch is - # itself a terminator (return / panic / log.Fatal* / os.Exit / - # runtime.Goexit / continue / break / goto). With nesting, a - # terminator after ANY containing structure means control flow - # doesn't actually fall past http.Error. + http.Error(...) + $STMT + # Subtract Case B: http.Error directly followed by a control-flow + # terminator (return / panic / log.Fatal* / os.Exit / + # runtime.Goexit / continue / break / goto). + - 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 + # control-flow terminator AND http.Error is the tail of its block. + # The tail-of-block conjunction prevents this subtraction from + # firing on Case B matches whose enclosing structure happens to + # also be followed by a terminator. - pattern-not-inside: patterns: - pattern-either: @@ -140,43 +171,6 @@ rules: - metavariable-regex: metavariable: $TERM regex: '^(return\b|panic\(|log\.Fatal(ln|f)?\(|os\.Exit\(|runtime\.Goexit\(|continue\b|break\b|goto\b)' - - # Case B — http.Error is immediately followed by a non-terminator - # statement in the same block (regardless of nesting context). - - id: http-error-missing-return-followed-by-stmt - 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: - - pattern-inside: | - import "net/http" - ... - - pattern: | - http.Error(...) - $STMT - - pattern-not: - patterns: - - pattern: | + - pattern-not: | 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)' + $ANY From ca7736c648ebe2374a9222dbd3a7a36b6345b7f0 Mon Sep 17 00:00:00 2001 From: GrosQuildu Date: Thu, 7 May 2026 13:48:23 +0200 Subject: [PATCH 11/11] simpler inline docs --- go/http-error-missing-return.yaml | 59 +++++---------- go/pkg-errors-wrap-nil-err.yaml | 115 ++++++++++-------------------- go/shadowed-err-check.yaml | 26 +++---- 3 files changed, 66 insertions(+), 134 deletions(-) diff --git a/go/http-error-missing-return.yaml b/go/http-error-missing-return.yaml index 1f4f538..bd0af7d 100644 --- a/go/http-error-missing-return.yaml +++ b/go/http-error-missing-return.yaml @@ -1,29 +1,13 @@ rules: - # The rule has two cases combined under one user-facing id: - # - # Case A — `http.Error` is the tail of its containing block, and the - # enclosing if-chain or switch is followed by another statement. - # Case B — `http.Error` is immediately followed by a statement in the - # same block (regardless of nesting context). - # - # The two cases are joined under a top-level `pattern-either`, then - # two top-level `pattern-not-inside` subtractions drop matches where - # the trailing statement is a control-flow terminator. The two - # subtractions are kept separate because they need different scoping: - # - # - The Case B subtraction (`http.Error(...)\n$TERM`) is shape-safe - # at the top level: the shape only matches Case B-style instances - # (where http.Error has a directly-following stmt), so it cannot - # wrongly subtract Case A matches. - # - The Case A subtraction (enclosing if/switch followed by $TERM) - # would over-subtract Case B matches whose enclosing structure - # also happens to be followed by a terminator. It is scoped to - # Case A by an extra `pattern-not http.Error(...)\n$ANY` - # conjunction (i.e., http.Error must be the tail of its block). - # - # Top-level placement of both subtractions is required because - # Semgrep silently ignores `pattern-not` / `pattern-not-inside` / - # `metavariable-regex` when nested under `pattern-either > patterns`. + # 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 @@ -47,18 +31,15 @@ rules: references: - https://pkg.go.dev/net/http#Error patterns: - # Anchor: the file imports `net/http`, so `http.Error` resolves to - # the stdlib symbol rather than a local identifier shadowing it. + # Anchor to files importing net/http so `http.Error` resolves to stdlib. - pattern-inside: | import "net/http" ... - pattern: http.Error(...) - pattern-either: - # Case A — http.Error is the tail of its containing block, and - # the enclosing if-chain or switch is followed by $NEXT. With - # nested chains $NEXT naturally binds to the trailing stmt of - # the outermost containing structure, so deep else-if chains - # work without explicit case enumeration. + # 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(...) @@ -104,14 +85,11 @@ rules: ... } $NEXT - # Case B — http.Error is directly followed by another stmt in - # the same block. + # Case B. - pattern-inside: | http.Error(...) $STMT - # Subtract Case B: http.Error directly followed by a control-flow - # terminator (return / panic / log.Fatal* / os.Exit / - # runtime.Goexit / continue / break / goto). + # Subtract Case B: http.Error directly followed by a terminator. - pattern-not-inside: patterns: - pattern: | @@ -121,10 +99,9 @@ rules: 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 - # control-flow terminator AND http.Error is the tail of its block. - # The tail-of-block conjunction prevents this subtraction from - # firing on Case B matches whose enclosing structure happens to - # also be followed by a terminator. + # 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: diff --git a/go/pkg-errors-wrap-nil-err.yaml b/go/pkg-errors-wrap-nil-err.yaml index 585746f..7f196d5 100644 --- a/go/pkg-errors-wrap-nil-err.yaml +++ b/go/pkg-errors-wrap-nil-err.yaml @@ -22,47 +22,36 @@ rules: - 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. Without this - # anchor the rule would also fire on calls to other packages - # aliased as `errors` (e.g. cockroachdb/errors, emperror.dev/errors) - # whose `Wrap` may have different nil-input semantics. + # 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" ... - # Restrict $FN to the pkg/errors helpers that return nil when their - # error input is nil. + # pkg/errors helpers that return nil for nil error input. - metavariable-regex: metavariable: $FN regex: ^(Wrap|Wrapf|WithMessage|WithMessagef|WithStack)$ - # Restrict $ERR to identifiers that look like error variables. - # Match `e`, `err`, `err\d*` (err1, err2), `errFoo` (err prefix + - # CamelCase), and `xErr` / `parseError` style names. Avoids - # accidental matches on words that merely contain the substring - # "err" (e.g. "terraform"). + # 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 { ... }` block (structural - # constraint enforced via `pattern-inside: if $COND { ... }` - # below), after a preceding `if $ERR != nil { return ... }` - # guard. The wrap is excluded if the surrounding `if` is - # itself an err-check (compound `&&` allowed) or if $ERR is - # reassigned (or shadowed via `:=`) before the wrap return. + # 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, ...) - # Structural constraint: wrap must sit inside an - # `if $COND { ... }` block. Co-located with Case A's - # comment so the requirement is visible up-front. + # Wrap must sit inside an `if $COND { ... }` block. - pattern-inside: | if $COND { ... } - # Accept any guard whose body ends in a control-flow - # terminator that prevents reaching the code below the - # `if`: `return`, `panic`, `log.Fatal*`, `os.Exit`, - # `runtime.Goexit`. After such a guard, $ERR is provably - # nil at the wrap site. + # 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 { @@ -106,9 +95,8 @@ rules: runtime.Goexit() } ... - # The wrap is intentional when the surrounding `if` - # already constrains $ERR (directly or as one conjunct of - # an `&&` chain). + # Surrounding `if` already constrains $ERR (direct or as a + # conjunct of `&&`). - pattern-not-inside: | if $ERR != nil { ... @@ -129,9 +117,8 @@ rules: if $ERR == nil { ... } - # Reverse guard: an earlier `if $ERR == nil { }` - # establishes that $ERR is NON-nil at the wrap site. Same - # terminator set as the forward guard above. + # Reverse guard: `if $ERR == nil { }` proves + # $ERR non-nil at the wrap site. Same terminator set. - pattern-not-inside: | if $ERR == nil { ... @@ -174,23 +161,17 @@ rules: runtime.Goexit() } ... - # $ERR reassigned (or shadowed via `:=`) IMMEDIATELY before - # the wrap return — we can no longer prove $ERR is nil - # at the wrap. Tight adjacent shape only: a multi-stmt - # `pattern-not-inside` with `...` between would traverse + # $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. So a reassignment separated - # from the wrap by other stmts in the same block remains - # a known FP. + # 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, ...) - # `:=` shadow: in a nested scope, `_, err := mayFail()` - # introduces a fresh `err` distinct from the proven-nil - # outer one. Same tight-adjacent constraint as above. - pattern-not-inside: | $ERR := ... return errors.$FN($ERR, ...) @@ -198,9 +179,8 @@ rules: ..., $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, ...)` — both swallow - # the failure path because Wrap returns nil for nil input. + # Matches both `return errors.Wrap(err, ...)` and the assignment + # form `err = errors.Wrap(err, ...)`. - patterns: - pattern: | if $ERR != nil { @@ -216,19 +196,14 @@ rules: - pattern-either: - pattern: return errors.$FN($ERR, ...) - pattern: $ERR = errors.$FN($ERR, ...) - # The `... $RET ...` shape lets Semgrep bind $RET to any - # statement-level node in the else-body — including an - # enclosing if-stmt or block whose body happens to contain - # the wrap. Anchor $RET's text to the actual return- or - # assignment-shape so we don't fire on the broader span. + # `... $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 via `:=`) IMMEDIATELY before - # the wrap (return-form) inside the else body. Tight - # adjacent shape only — `...` between would traverse - # nested blocks (e.g. `if ... { err = X }` inside the - # else body) and over-subtract. + # $ERR reassigned (or `:=`-shadowed) IMMEDIATELY before the + # wrap (return-form) inside the else body. Tight-adjacent only. - pattern-not: | if $ERR != nil { ... @@ -261,7 +236,7 @@ rules: ..., $ERR := ... return errors.$FN($ERR, ...) } - # Same FP shape for the assignment-form wrap. + # Same shape for the assignment-form wrap. - pattern-not: | if $ERR != nil { ... @@ -297,24 +272,15 @@ rules: - focus-metavariable: $RET # Case C: wrap inside an `else if` arm after `if $ERR != nil`. - # Go parses `else if $COND { ... }` as `else { if $COND { ... } }`, - # where the else-body is the inner if-stmt directly (not wrapped - # in an extra block). Semgrep's `... $RET ...` requires a - # multi-stmt block, so the basic Case B pattern doesn't bind - # $RET to the inner if-stmt — hence this separate case for the - # single-statement else-body shape. - # - # The wrap is reached only when `$ERR != nil` was FALSE (i.e. - # err is provably nil) AND the inner condition is TRUE — so the - # wrap swallows the error path the same way Case B does. No - # terminator in the if-body is needed: the else branch runs iff - # err is nil, regardless of what the if-body does. + # 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` — the parser produces an - # else-body with a single if-stmt (no init). Semgrep - # binds $INNER to that inner if-stmt via `else { $INNER }`. + # Plain `else if $COND` — Semgrep binds $INNER to the + # inner if-stmt via `else { $INNER }`. - patterns: - pattern-inside: | if $ERR != nil { @@ -330,18 +296,15 @@ rules: } # `else if $INIT; $COND` — Semgrep doesn't bind $INNER # via `else { $INNER }` for init-form else-ifs, so the - # init form needs an explicit `else if $INIT; $COND` - # pattern. + # shape needs an explicit pattern. - pattern-inside: | if $ERR != nil { ... } else if $INIT; $COND { ... } - # FP: $ERR reassigned (or shadowed via `:=`) IMMEDIATELY - # before the wrap inside the else-if arm — wrap is on a - # fresh err, not the proven-nil one. Tight adjacent shape - # only (same caveat as Case A). + # $ERR reassigned (or `:=`-shadowed) IMMEDIATELY before the + # wrap. Tight-adjacent only (same caveat as Case A). - pattern-not-inside: | $ERR = ... return errors.$FN($ERR, ...) diff --git a/go/shadowed-err-check.yaml b/go/shadowed-err-check.yaml index 2386806..b0df8eb 100644 --- a/go/shadowed-err-check.yaml +++ b/go/shadowed-err-check.yaml @@ -20,8 +20,6 @@ rules: references: - https://temporal.io/blog/go-shadowing-bad-choices#the-bug patterns: - # Match any `if := ; nil { ... }` where the - # declared variable and the variable in the check are both error-like. - pattern-either: - pattern: | if $LOCAL := $EXPR; $CHECK != nil { @@ -39,27 +37,21 @@ rules: if ..., $LOCAL := $EXPR; $CHECK == nil { ... } - # Both the declared variable and the variable being checked must look - # like error variables. This focuses the rule on the real bug class - # (shadowed/typo'd error checks) and avoids false positives from - # legitimate uses such as `if v, ok := m[k]; ok { ... }`. The regex - # matches `e`, `err`, `err\d*`, `errFoo` (err prefix + CamelCase), - # and `xErr` / `parseError` style names while rejecting accidental - # substring matches like `terraform`. + # 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: when the declared variable IS the - # variable being checked. `$SAME` is a fresh metavariable - # (intentionally distinct from `$LOCAL`/`$CHECK` above) so that - # within this single `pattern-not`, Semgrep unifies the two - # occurrences of `$SAME` and matches only when the same identifier - # appears on both sides. Reusing `$LOCAL`/`$CHECK` here would not - # work — Semgrep does not unify metavariables across the - # `pattern-not` boundary back to the outer patterns. + # 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 { ...