From 9f10798be1051eece75274a0996c3c402a846c37 Mon Sep 17 00:00:00 2001 From: ~wispem-wantex Date: Sun, 18 Oct 2026 15:02:44 -0700 Subject: [PATCH] linters: add httpstatus linter --- .golangci.yaml | 4 + pkg/httpstatus/README.md | 37 +++++ pkg/httpstatus/analyzer.go | 153 ++++++++++++++++++ pkg/httpstatus/analyzer_test.go | 22 +++ pkg/httpstatus/plugin.go | 31 ++++ pkg/httpstatus/settings.go | 16 ++ .../testdata/src/configured/configured.go | 29 ++++ .../testdata/src/default/default.go | 22 +++ root.go | 1 + 9 files changed, 315 insertions(+) create mode 100644 pkg/httpstatus/README.md create mode 100644 pkg/httpstatus/analyzer.go create mode 100644 pkg/httpstatus/analyzer_test.go create mode 100644 pkg/httpstatus/plugin.go create mode 100644 pkg/httpstatus/settings.go create mode 100644 pkg/httpstatus/testdata/src/configured/configured.go create mode 100644 pkg/httpstatus/testdata/src/default/default.go diff --git a/.golangci.yaml b/.golangci.yaml index 4ea3416..e586a72 100644 --- a/.golangci.yaml +++ b/.golangci.yaml @@ -28,6 +28,7 @@ linters: # Custom linters - ifinit - boolnames + - httpstatus settings: custom: @@ -37,6 +38,9 @@ linters: boolnames: type: module description: boolean variables and functions should use "is", "has", "was" etc + httpstatus: + type: module + description: HTTP status codes should use the project's typical-code policy forbidigo: forbid: - pattern: ^sync\.Map$ diff --git a/pkg/httpstatus/README.md b/pkg/httpstatus/README.md new file mode 100644 index 0000000..2f3a638 --- /dev/null +++ b/pkg/httpstatus/README.md @@ -0,0 +1,37 @@ +# `httpstatus` linter + +This linter imposes constraints on use of http status codes, targeted mainly at JSON APIs. The goal is to discourage use of non-typical status codes. + +A "typical" status code is one which: +- is commonly used +- is significantly differentiated from all other "typical" status codes (i.e., there should never be more than one plausible choice of which code to return) +- programmers are expected to have it memorized. + +These constraints impose a natural limit on how many "typical" status codes you can have. + +A certain list of "typical" status codes is a config parameter. By default it includes: + - 200, 204 + - Notes: 201 is disallowed. Use 200 instead + - 302 + - Notes: all other 300-series are situationally valid, as they directly affect browser / cache behavior, but are less frequently used in JSON APIs. For this reason, `http.Redirect()` calls are exempt from the "restrict" rule. + - 400, 401, 403, 404, 429 + - 409, 410 and 422 are disallowed; use 400 instead. + - Notes: explicit 405 is disallowed; router should return one automatically + - 502, 504 + - Notes: explicit 500 is disallowed. Use panic instead + - All other status codes are considered "non-typical" + +Status codes in the "typical" list require use of "magic numbers" (the opposite of `staticcheck`'s ST1013 check, on purpose); using `http.StatusBlahBlah` is disallowed. Only non-typical status codes are permitted to use the named `net/http` constants. "Magic numbers" are shorter and-- for typical status codes-- more readable. + +If the optional "restrict" config value is set to "true", then setting non-typical status codes in HTTP handlers is also disallowed. As noted above, `http.Redirect()` and the 300-series is exempt from this. + +--- + +## Implementation clarifications + +- Check every reference to a `net/http` `Status*` constant, regardless of context. A typical constant is disallowed even in a declaration such as `const ok = http.StatusOK`. +- Inspect only status codes supplied to `http.Error` and the `WriteHeader` method declared by `http.ResponseWriter`. Do not inspect fixed-status helpers such as `http.NotFound()` or `ResponseWriter.Write()`. +- Do not diagnose a runtime value such as `w.WriteHeader(code)` when `code` is a variable. +- With `restrict: true`, reject statically known non-typical codes passed to `http.Error` or `http.ResponseWriter.WriteHeader`. +- `http.Redirect` is exempt from `restrict` for 300-series codes. Its use of a named typical `net/http` status constant remains disallowed by the general constant rule. +- A configured typical-status-code list replaces the default list. diff --git a/pkg/httpstatus/analyzer.go b/pkg/httpstatus/analyzer.go new file mode 100644 index 0000000..a4732de --- /dev/null +++ b/pkg/httpstatus/analyzer.go @@ -0,0 +1,153 @@ +// Package httpstatus reports discouraged HTTP status-code usage. +package httpstatus + +import ( + "go/ast" + "go/constant" + "go/types" + "strings" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/inspect" + "golang.org/x/tools/go/ast/inspector" +) + +const netHTTPPath = "net/http" + +var defaultTypicalStatusCodes = []int{ + 200, 204, + 302, + 400, 401, 403, 404, 429, + 502, 504, +} + +var Analyzer = NewAnalyzer(Settings{}) + +func NewAnalyzer(settings Settings) *analysis.Analyzer { + if settings.TypicalStatusCodes == nil { + settings.TypicalStatusCodes = defaultTypicalStatusCodes + } + + typicalStatusCodes := make(map[int]struct{}, len(settings.TypicalStatusCodes)) + for _, code := range settings.TypicalStatusCodes { + typicalStatusCodes[code] = struct{}{} + } + + return &analysis.Analyzer{ + Name: "httpstatus", + Doc: "checks HTTP status-code usage", + Requires: []*analysis.Analyzer{inspect.Analyzer}, + Run: func(pass *analysis.Pass) (any, error) { + insp, ok := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) + if !ok { + panic("inspect.Analyzer did not produce *inspector.Inspector") + } + + // --------------- + // Checks + // --------------- + + // Disallows `http.StatusBlahBlah` symbols for typical status codes + enforceIntegerLiterals := func(node *ast.SelectorExpr) { + status, ok := pass.TypesInfo.Uses[node.Sel].(*types.Const) + if !ok || status.Pkg() == nil || status.Pkg().Path() != netHTTPPath || !strings.HasPrefix(status.Name(), "Status") { + return + } + // Check if it corresponds to one of the codes in the "typical" list + code, ok := integerConstant(status.Val()) + if !ok { + return + } + if _, ok := typicalStatusCodes[code]; !ok { + return + } + + pass.Reportf(node.Pos(), "HTTP status code %d should use its integer literal", code) + } + + // Disallows sending non-typical response statuses + requireTypicalResponseStatus := func(node *ast.CallExpr) { + checkArgument := func(argIdx int) { + // Require a status-code argument. + if argIdx < 0 || len(node.Args) <= argIdx { + return + } + + // Report non-typical codes. + code, ok := integerConstant(pass.TypesInfo.Types[node.Args[argIdx]].Value) + if !ok { + return + } + if _, ok := typicalStatusCodes[code]; !ok { + pass.Reportf(node.Args[argIdx].Pos(), "non-typical HTTP status code %d is not allowed", code) + } + } + switch functionExpression := node.Fun.(type) { + case *ast.Ident: + // Handle dot-imported http.Error. + function, ok := pass.TypesInfo.Uses[functionExpression].(*types.Func) + if ok && function.Pkg() != nil && function.Pkg().Path() == netHTTPPath && function.Name() == "Error" { + checkArgument(2) + } + case *ast.SelectorExpr: + // Handle qualified http.Error. + function, _ := pass.TypesInfo.Uses[functionExpression.Sel].(*types.Func) + if function != nil && function.Pkg() != nil && function.Pkg().Path() == netHTTPPath && function.Name() == "Error" { + checkArgument(2) + return + } + + // Handle both forms of WriteHeader. + selection := pass.TypesInfo.Selections[functionExpression] + if selection == nil { + return + } + method, ok := selection.Obj().(*types.Func) + if ok && method.Pkg() != nil && method.Pkg().Path() == netHTTPPath && method.Name() == "WriteHeader" { + if selection.Kind() == types.MethodExpr { + checkArgument(1) // `http.ResponseWriter.WriteHeader` + } else { + checkArgument(0) // `w.WriteHeader` + } + } + default: + return + } + } + + // ------------------- + // Walk the tree + // ------------------- + + insp.Preorder([]ast.Node{(*ast.SelectorExpr)(nil), (*ast.CallExpr)(nil)}, func(node ast.Node) { + switch node := node.(type) { + case *ast.SelectorExpr: + enforceIntegerLiterals(node) + case *ast.CallExpr: + if !settings.IsRestricted { + return + } + requireTypicalResponseStatus(node) + default: + panic("inspector passed an unexpected node") + } + }) + + return nil, nil + }, + } +} + +// integerConstant attempts to extract the value of a constant.Value as an int. +func integerConstant(value constant.Value) (int, bool) { + if value == nil { + return 0, false + } + + integer, ok := constant.Int64Val(value) + if !ok || int64(int(integer)) != integer { + return 0, false + } + + return int(integer), true +} diff --git a/pkg/httpstatus/analyzer_test.go b/pkg/httpstatus/analyzer_test.go new file mode 100644 index 0000000..7ecde10 --- /dev/null +++ b/pkg/httpstatus/analyzer_test.go @@ -0,0 +1,22 @@ +package httpstatus_test + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" + + "git.offline-twitter.com/offline-labs/go-lint/pkg/httpstatus" +) + +func TestAnalyzer(t *testing.T) { + analysistest.Run(t, analysistest.TestData(), httpstatus.Analyzer, "default") +} + +func TestAnalyzerWithSettings(t *testing.T) { + analyzer := httpstatus.NewAnalyzer(httpstatus.Settings{ + TypicalStatusCodes: []int{201}, + IsRestricted: true, + }) + + analysistest.Run(t, analysistest.TestData(), analyzer, "configured") +} diff --git a/pkg/httpstatus/plugin.go b/pkg/httpstatus/plugin.go new file mode 100644 index 0000000..0e0254e --- /dev/null +++ b/pkg/httpstatus/plugin.go @@ -0,0 +1,31 @@ +package httpstatus + +import ( + "github.com/golangci/plugin-module-register/register" + "golang.org/x/tools/go/analysis" +) + +func init() { + register.Plugin("httpstatus", New) +} + +type plugin struct { + settings Settings +} + +func New(rawSettings any) (register.LinterPlugin, error) { + settings, err := decodeSettings(rawSettings) + if err != nil { + return nil, err + } + + return &plugin{settings: settings}, nil +} + +func (p *plugin) BuildAnalyzers() ([]*analysis.Analyzer, error) { + return []*analysis.Analyzer{NewAnalyzer(p.settings)}, nil +} + +func (p *plugin) GetLoadMode() string { + return register.LoadModeTypesInfo +} diff --git a/pkg/httpstatus/settings.go b/pkg/httpstatus/settings.go new file mode 100644 index 0000000..55d82e5 --- /dev/null +++ b/pkg/httpstatus/settings.go @@ -0,0 +1,16 @@ +package httpstatus + +import "github.com/golangci/plugin-module-register/register" + +type Settings struct { + TypicalStatusCodes []int `json:"typical_status_codes"` + IsRestricted bool `json:"restrict"` +} + +func decodeSettings(rawSettings any) (Settings, error) { + if rawSettings == nil { + return Settings{}, nil + } + + return register.DecodeSettings[Settings](rawSettings) +} diff --git a/pkg/httpstatus/testdata/src/configured/configured.go b/pkg/httpstatus/testdata/src/configured/configured.go new file mode 100644 index 0000000..20c146d --- /dev/null +++ b/pkg/httpstatus/testdata/src/configured/configured.go @@ -0,0 +1,29 @@ +package configured + +import "net/http" + +const created = http.StatusCreated // want `HTTP status code 201 should use its integer literal` +const ok = http.StatusOK + +func response(w http.ResponseWriter, r *http.Request, code int) { + http.Error(w, "teapot", 418) // want `non-typical HTTP status code 418 is not allowed` + http.Error(w, "created", 201) + http.Error(w, "variable", code) + + w.WriteHeader(200) // want `non-typical HTTP status code 200 is not allowed` + w.WriteHeader(code) + http.ResponseWriter.WriteHeader(w, 400) // want `non-typical HTTP status code 400 is not allowed` + + http.NotFound(w, r) + _, _ = w.Write([]byte("response")) + http.Redirect(w, r, "/moved", 301) + http.Redirect(w, r, "/created", http.StatusCreated) // want `HTTP status code 201 should use its integer literal` +} + +type unrelatedWriter struct{} + +func (unrelatedWriter) WriteHeader(int) {} + +func unrelated(writer unrelatedWriter) { + writer.WriteHeader(418) +} diff --git a/pkg/httpstatus/testdata/src/default/default.go b/pkg/httpstatus/testdata/src/default/default.go new file mode 100644 index 0000000..97b7b08 --- /dev/null +++ b/pkg/httpstatus/testdata/src/default/default.go @@ -0,0 +1,22 @@ +package cases + +import "net/http" + +const ok = http.StatusOK // want `HTTP status code 200 should use its integer literal` +const teapot = http.StatusTeapot + +func response(w http.ResponseWriter, r *http.Request, code int) { + _ = http.StatusNotFound // want `HTTP status code 404 should use its integer literal` + + http.Error(w, "bad request", http.StatusBadRequest) // want `HTTP status code 400 should use its integer literal` + http.Error(w, "teapot", http.StatusTeapot) + http.Error(w, "created", 201) + w.WriteHeader(http.StatusGatewayTimeout) // want `HTTP status code 504 should use its integer literal` + w.WriteHeader(code) + + http.NotFound(w, r) + _, _ = w.Write([]byte("response")) + + http.Redirect(w, r, "/login", http.StatusFound) // want `HTTP status code 302 should use its integer literal` + http.Redirect(w, r, "/other", http.StatusMovedPermanently) +} diff --git a/root.go b/root.go index 6571c65..8b3ddd2 100644 --- a/root.go +++ b/root.go @@ -2,5 +2,6 @@ package customlint import ( _ "git.offline-twitter.com/offline-labs/go-lint/pkg/boolnames" + _ "git.offline-twitter.com/offline-labs/go-lint/pkg/httpstatus" _ "git.offline-twitter.com/offline-labs/go-lint/pkg/ifinit" )