linters: add httpstatus linter

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.

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
+}

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")
+}

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
+}

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)
+}

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)
+}

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)
+}