From 8540d4016c0da825684cf26403f6f7f222f86c8b Mon Sep 17 00:00:00 2001 From: Knut Ahlers Date: Wed, 4 Oct 2023 22:53:29 +0200 Subject: [PATCH] CI: Add code linting (#118) --- .github/workflows/test-and-build.yml | 3 + .golangci.yml | 174 +++++++++++++++++++++++++++ api.go | 11 +- customize.go | 8 +- main.go | 6 +- storage.go | 4 +- storage_redis.go | 6 +- tplFuncs.go | 2 +- 8 files changed, 200 insertions(+), 14 deletions(-) create mode 100644 .golangci.yml diff --git a/.github/workflows/test-and-build.yml b/.github/workflows/test-and-build.yml index a50fcdb..772b01c 100644 --- a/.github/workflows/test-and-build.yml +++ b/.github/workflows/test-and-build.yml @@ -53,16 +53,19 @@ jobs: - name: 'Lint and test code: API' run: | go test -cover -v ./... + golangci-lint run ./... - name: 'Lint and test code: Client' working-directory: ./pkg/client run: | go test -cover -v ./... + golangci-lint run ./... - name: 'Lint and test code: OTS-CLI' working-directory: ./cmd/ots-cli run: | go test -cover -v ./... + golangci-lint run ./... - name: Generate (and validate) translations run: make translate diff --git a/.golangci.yml b/.golangci.yml new file mode 100644 index 0000000..e4d0cf1 --- /dev/null +++ b/.golangci.yml @@ -0,0 +1,174 @@ +# Derived from https://github.com/golangci/golangci-lint/blob/master/.golangci.example.yml + +--- + +run: + # timeout for analysis, e.g. 30s, 5m, default is 1m + timeout: 5m + # Force readonly modules usage for checking + modules-download-mode: readonly + +output: + format: tab + +issues: + # This disables the included exclude-list in golangci-lint as that + # list for example fully hides G304 gosec rule, errcheck, exported + # rule of revive and other errors one really wants to see. + # Smme detail: https://github.com/golangci/golangci-lint/issues/456 + exclude-use-default: false + # Don't limit the number of shown issues: Report ALL of them + max-issues-per-linter: 0 + max-same-issues: 0 + +linters: + disable-all: true + enable: + - asciicheck # Simple linter to check that your code does not contain non-ASCII identifiers [fast: true, auto-fix: false] + - bidichk # Checks for dangerous unicode character sequences [fast: true, auto-fix: false] + - bodyclose # checks whether HTTP response body is closed successfully [fast: true, auto-fix: false] + - containedctx # containedctx is a linter that detects struct contained context.Context field [fast: true, auto-fix: false] + - contextcheck # check the function whether use a non-inherited context [fast: false, auto-fix: false] + - dogsled # Checks assignments with too many blank identifiers (e.g. x, _, _, _, := f()) [fast: true, auto-fix: false] + - durationcheck # check for two durations multiplied together [fast: false, auto-fix: false] + - errcheck # Errcheck is a program for checking for unchecked errors in go programs. These unchecked errors can be critical bugs in some cases [fast: false, auto-fix: false] + - errchkjson # Checks types passed to the json encoding functions. Reports unsupported types and optionally reports occations, where the check for the returned error can be omitted. [fast: false, auto-fix: false] + - exportloopref # checks for pointers to enclosing loop variables [fast: true, auto-fix: false] + - forbidigo # Forbids identifiers [fast: true, auto-fix: false] + - funlen # Tool for detection of long functions [fast: true, auto-fix: false] + - gocognit # Computes and checks the cognitive complexity of functions [fast: true, auto-fix: false] + - goconst # Finds repeated strings that could be replaced by a constant [fast: true, auto-fix: false] + - gocritic # The most opinionated Go source code linter [fast: true, auto-fix: false] + - gocyclo # Computes and checks the cyclomatic complexity of functions [fast: true, auto-fix: false] + - godox # Tool for detection of FIXME, TODO and other comment keywords [fast: true, auto-fix: false] + - gofmt # Gofmt checks whether code was gofmt-ed. By default this tool runs with -s option to check for code simplification [fast: true, auto-fix: true] + - gofumpt # Gofumpt checks whether code was gofumpt-ed. [fast: true, auto-fix: true] + - goimports # Goimports does everything that gofmt does. Additionally it checks unused imports [fast: true, auto-fix: true] + - gomnd # An analyzer to detect magic numbers. [fast: true, auto-fix: false] + - gosec # Inspects source code for security problems [fast: true, auto-fix: false] + - gosimple # Linter for Go source code that specializes in simplifying a code [fast: true, auto-fix: false] + - govet # Vet examines Go source code and reports suspicious constructs, such as Printf calls whose arguments do not align with the format string [fast: true, auto-fix: false] + - ineffassign # Detects when assignments to existing variables are not used [fast: true, auto-fix: false] + - misspell # Finds commonly misspelled English words in comments [fast: true, auto-fix: true] + - nakedret # Finds naked returns in functions greater than a specified function length [fast: true, auto-fix: false] + - nilerr # Finds the code that returns nil even if it checks that the error is not nil. [fast: false, auto-fix: false] + - nilnil # Checks that there is no simultaneous return of `nil` error and an invalid value. [fast: false, auto-fix: false] + - noctx # noctx finds sending http request without context.Context [fast: true, auto-fix: false] + - nolintlint # Reports ill-formed or insufficient nolint directives [fast: true, auto-fix: false] + - revive # Fast, configurable, extensible, flexible, and beautiful linter for Go. Drop-in replacement of golint. [fast: false, auto-fix: false] + - staticcheck # Staticcheck is a go vet on steroids, applying a ton of static analysis checks [fast: true, auto-fix: false] + - stylecheck # Stylecheck is a replacement for golint [fast: true, auto-fix: false] + - tenv # tenv is analyzer that detects using os.Setenv instead of t.Setenv since Go1.17 [fast: false, auto-fix: false] + - typecheck # Like the front-end of a Go compiler, parses and type-checks Go code [fast: true, auto-fix: false] + - unconvert # Remove unnecessary type conversions [fast: true, auto-fix: false] + - unused # Checks Go code for unused constants, variables, functions and types [fast: false, auto-fix: false] + - wastedassign # wastedassign finds wasted assignment statements. [fast: false, auto-fix: false] + - wrapcheck # Checks that errors returned from external packages are wrapped [fast: false, auto-fix: false] + +linters-settings: + funlen: + lines: 100 + statements: 60 + + gocyclo: + # minimal code complexity to report, 30 by default (but we recommend 10-20) + min-complexity: 15 + + gomnd: + settings: + mnd: + ignored-functions: 'strconv.(?:Format|Parse)\B+' + + revive: + rules: + #- name: add-constant # Suggests using constant for magic numbers and string literals + # Opinion: Makes sense for strings, not for numbers but checks numbers + #- name: argument-limit # Specifies the maximum number of arguments a function can receive | Opinion: Don't need this + - name: atomic # Check for common mistaken usages of the `sync/atomic` package + - name: banned-characters # Checks banned characters in identifiers + arguments: + - 'Íž' # Greek question mark + - name: bare-return # Warns on bare returns + - name: blank-imports # Disallows blank imports + - name: bool-literal-in-expr # Suggests removing Boolean literals from logic expressions + - name: call-to-gc # Warns on explicit call to the garbage collector + #- name: cognitive-complexity # Sets restriction for maximum Cognitive complexity. + # There is a dedicated linter for this + - name: confusing-naming # Warns on methods with names that differ only by capitalization + - name: confusing-results # Suggests to name potentially confusing function results + - name: constant-logical-expr # Warns on constant logical expressions + - name: context-as-argument # `context.Context` should be the first argument of a function. + - name: context-keys-type # Disallows the usage of basic types in `context.WithValue`. + #- name: cyclomatic # Sets restriction for maximum Cyclomatic complexity. + # There is a dedicated linter for this + #- name: datarace # Spots potential dataraces + # Is not (yet) available? + - name: deep-exit # Looks for program exits in funcs other than `main()` or `init()` + - name: defer # Warns on some [defer gotchas](https://blog.learngoprogramming.com/5-gotchas-of-defer-in-go-golang-part-iii-36a1ab3d6ef1) + - name: dot-imports # Forbids `.` imports. + - name: duplicated-imports # Looks for packages that are imported two or more times + - name: early-return # Spots if-then-else statements that can be refactored to simplify code reading + - name: empty-block # Warns on empty code blocks + - name: empty-lines # Warns when there are heading or trailing newlines in a block + - name: errorf # Should replace `errors.New(fmt.Sprintf())` with `fmt.Errorf()` + - name: error-naming # Naming of error variables. + - name: error-return # The error return parameter should be last. + - name: error-strings # Conventions around error strings. + - name: exported # Naming and commenting conventions on exported symbols. + arguments: ['sayRepetitiveInsteadOfStutters'] + #- name: file-header # Header which each file should have. + # Useless without config, have no config for it + - name: flag-parameter # Warns on boolean parameters that create a control coupling + #- name: function-length # Warns on functions exceeding the statements or lines max + # There is a dedicated linter for this + #- name: function-result-limit # Specifies the maximum number of results a function can return + # Opinion: Don't need this + - name: get-return # Warns on getters that do not yield any result + - name: identical-branches # Spots if-then-else statements with identical `then` and `else` branches + - name: if-return # Redundant if when returning an error. + #- name: imports-blacklist # Disallows importing the specified packages + # Useless without config, have no config for it + - name: import-shadowing # Spots identifiers that shadow an import + - name: increment-decrement # Use `i++` and `i--` instead of `i += 1` and `i -= 1`. + - name: indent-error-flow # Prevents redundant else statements. + #- name: line-length-limit # Specifies the maximum number of characters in a lined + # There is a dedicated linter for this + #- name: max-public-structs # The maximum number of public structs in a file. + # Opinion: Don't need this + - name: modifies-parameter # Warns on assignments to function parameters + - name: modifies-value-receiver # Warns on assignments to value-passed method receivers + #- name: nested-structs # Warns on structs within structs + # Opinion: Don't need this + - name: optimize-operands-order # Checks inefficient conditional expressions + #- name: package-comments # Package commenting conventions. + # Opinion: Don't need this + - name: range # Prevents redundant variables when iterating over a collection. + - name: range-val-address # Warns if address of range value is used dangerously + - name: range-val-in-closure # Warns if range value is used in a closure dispatched as goroutine + - name: receiver-naming # Conventions around the naming of receivers. + - name: redefines-builtin-id # Warns on redefinitions of builtin identifiers + #- name: string-format # Warns on specific string literals that fail one or more user-configured regular expressions + # Useless without config, have no config for it + - name: string-of-int # Warns on suspicious casts from int to string + - name: struct-tag # Checks common struct tags like `json`,`xml`,`yaml` + - name: superfluous-else # Prevents redundant else statements (extends indent-error-flow) + - name: time-equal # Suggests to use `time.Time.Equal` instead of `==` and `!=` for equality check time. + - name: time-naming # Conventions around the naming of time variables. + - name: unconditional-recursion # Warns on function calls that will lead to (direct) infinite recursion + - name: unexported-naming # Warns on wrongly named un-exported symbols + - name: unexported-return # Warns when a public return is from unexported type. + - name: unhandled-error # Warns on unhandled errors returned by funcion calls + arguments: + - "fmt.(Fp|P)rint(f|ln|)" + - name: unnecessary-stmt # Suggests removing or simplifying unnecessary statements + - name: unreachable-code # Warns on unreachable code + - name: unused-parameter # Suggests to rename or remove unused function parameters + - name: unused-receiver # Suggests to rename or remove unused method receivers + #- name: use-any # Proposes to replace `interface{}` with its alias `any` + # Is not (yet) available? + - name: useless-break # Warns on useless `break` statements in case clauses + - name: var-declaration # Reduces redundancies around variable declaration. + - name: var-naming # Naming rules. + - name: waitgroup-by-value # Warns on functions taking sync.WaitGroup as a by-value parameter + +... diff --git a/api.go b/api.go index d3d598a..2f76dce 100644 --- a/api.go +++ b/api.go @@ -22,7 +22,7 @@ type apiResponse struct { Error string `json:"error,omitempty"` ExpiresAt *time.Time `json:"expires_at,omitempty"` Secret string `json:"secret,omitempty"` - SecretId string `json:"secret_id,omitempty"` + SecretID string `json:"secret_id,omitempty"` } type apiRequest struct { @@ -83,7 +83,7 @@ func (a apiServer) handleCreate(res http.ResponseWriter, r *http.Request) { a.jsonResponse(res, http.StatusCreated, apiResponse{ ExpiresAt: expiresAt, Success: true, - SecretId: id, + SecretID: id, }) } @@ -124,10 +124,13 @@ func (a apiServer) errorResponse(res http.ResponseWriter, status int, err error, }) } -func (a apiServer) jsonResponse(res http.ResponseWriter, status int, response apiResponse) { +func (apiServer) jsonResponse(res http.ResponseWriter, status int, response apiResponse) { res.Header().Set("Content-Type", "application/json") res.Header().Set("Cache-Control", "no-store, max-age=0") res.WriteHeader(status) - json.NewEncoder(res).Encode(response) + if err := json.NewEncoder(res).Encode(response); err != nil { + logrus.WithError(err).Error("encoding JSON response") + http.Error(res, `{"error":"could not encode response"}`, http.StatusInternalServerError) + } } diff --git a/customize.go b/customize.go index bf808c5..1a6f50a 100644 --- a/customize.go +++ b/customize.go @@ -38,7 +38,7 @@ func loadCustomize(filename string) (cust customize, err error) { return cust, nil } - cf, err := os.Open(filename) + cf, err := os.Open(filename) //#nosec:G304 // Loading a custom file is the intention here if err != nil { if errors.Is(err, fs.ErrNotExist) { logrus.Warn("customize file given but not found") @@ -46,7 +46,11 @@ func loadCustomize(filename string) (cust customize, err error) { } return cust, errors.Wrap(err, "opening customize file") } - defer cf.Close() + defer func() { + if err := cf.Close(); err != nil { + logrus.WithError(err).Error("closing customize file (leaked fd)") + } + }() if err = yaml.NewDecoder(cf).Decode(&cust); err != nil { return cust, errors.Wrap(err, "decoding customize file") diff --git a/main.go b/main.go index 7e2b931..6ffc081 100644 --- a/main.go +++ b/main.go @@ -155,10 +155,12 @@ func assetDelivery(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", mime.TypeByExtension(ext)) w.Header().Set("X-Content-Type-Options", "nosniff") - w.Write(assetData) + if _, err = w.Write(assetData); err != nil { + logrus.WithError(err).Error("writing asset data") + } } -func handleIndex(w http.ResponseWriter, r *http.Request) { +func handleIndex(w http.ResponseWriter, _ *http.Request) { inlineContentNonce := make([]byte, scriptNonceSize) if _, err := rand.Read(inlineContentNonce); err != nil { logrus.WithError(err).Error("generating script nonce") diff --git a/storage.go b/storage.go index 0f70d5f..f8ccdfa 100644 --- a/storage.go +++ b/storage.go @@ -6,7 +6,7 @@ import ( "time" ) -var errSecretNotFound = errors.New("Secret not found") +var errSecretNotFound = errors.New("secret not found") type storage interface { Create(secret string, expireIn time.Duration) (string, error) @@ -20,6 +20,6 @@ func getStorageByType(t string) (storage, error) { case "redis": return newStorageRedis() default: - return nil, fmt.Errorf("Storage type %q not found", t) + return nil, fmt.Errorf("storage type %q not found", t) } } diff --git a/storage_redis.go b/storage_redis.go index fe841d6..f9dc056 100644 --- a/storage_redis.go +++ b/storage_redis.go @@ -53,14 +53,14 @@ func (s storageRedis) ReadAndDestroy(id string) (string, error) { if errors.Is(err, redis.Nil) { return "", errSecretNotFound } - return "", err + return "", errors.Wrap(err, "getting key") } err = s.conn.Del(context.Background(), s.redisKey(id)).Err() - return string(secret), errors.Wrap(err, "deleting key") + return secret, errors.Wrap(err, "deleting key") } -func (s storageRedis) redisKey(id string) string { +func (storageRedis) redisKey(id string) string { prefix := redisDefaultPrefix if prfx := os.Getenv("REDIS_KEY"); prfx != "" { prefix = prfx diff --git a/tplFuncs.go b/tplFuncs.go index d82e63e..24fc0bc 100644 --- a/tplFuncs.go +++ b/tplFuncs.go @@ -30,7 +30,7 @@ func assetSRIHash(assetName string) string { } h := sha512.New384() - h.Write(data) + _, _ = h.Write(data) sum := h.Sum(nil) sri := "sha384-" + base64.StdEncoding.EncodeToString(sum)