Improve security by enabling and enforcing more linting rules
Description
By enforcing stuff like error handling we can improve the security. Therefore, possible linting rules need to be gathered/discussed and then enabled. This might introduce flagging of some issues, which then also should be handled as part of this issue.
The following is a list of enabled and filtered rules with recommendations and other kinds of possible additional rules.
Please check if the enabled rules seem useful and check the additional options as well.
Current Status:
Enabled:
- errcheck (checks if returned errors are handled)
- ineffassign (Detects when assignments to existing variables are not used)
- varcheck (Finds unused global variables and constants)
- bidichk (checks for dangerous unicode character sequences)
- durationcheck (check for two durations multiplied together)
- errorlint (checks for issues when comparing errors, example: if err == io.EOF is bad)
- exportloopref (checks for pointers to enclosing loop variables, useful when using concurrency to be able to overwrite the var in each iteration)
- grouper (analyzer to analyze expression groups, for example for imports)
- makezero (Finds slice declarations with non-zero initial length, helps preventing extra 0 element)
- misspell
- nilnil (checks if there is a nil var and nil error in same return statement)
- predeclared (checks if func names are like ones already defined by go)
- godot (Check if comments end in a period, has auto-fix, definitely should use this, will cause an ugly first commit though 🥲)
Recommendations:
- govet (enable more/all rules of govet,
govet: # Enable all analyzers. # Default: false enable-all: true
) - errname (checks for mistakes in error naming conventions, would recommend this!)
- goconst (Finds repeated strings that could be replaced by a constant, would recommend this!)
- exhaustruct (Checks if all structure fields are initialized, hard to satisfy but might be worth it)
- gocognit (Computes and checks the cognitive complexity of functions, similar metric to cyclomatic complexity, might be worth a try)
- gomnd (checks for magic numbers, might be worth a try)
- gosec (Inspects source code for security problems, additional security checks, needs some configuration but might be worth a try)
- paralleltest (checks if there is t.Parallel missing in tests, might be worth but hard to satisfy)
- prealloc (checks if thigns could be preallocated, maybe worth using from time to time but might be hard to actually satisfy because flags are ‘consider changing this’)
- nestif (checks for nasty if constructs, might be worth but requires some initial work to satisfy)
- wrapcheck (checks if errors returned from other packages are wrapped e.g. we add our own information where/what happened, worth a try but requires lots of work initially)
- wsl (white space linter, worth a try but hard to satisfy initially)
- nolintlint (checks for unused //nolint comments, might cause false positives, for example there was one flag of a needed //nolint, this could be handled by adding //nolint:nolintlint
🙃 ) - gocritic (Provides diagnostics that check for bugs, performance and style issues. Extensible without recompilation through dynamic rules. Dynamic rules are written declaratively with AST patterns, filters, report message and optional suggestion., might need some work with configuration/inital enabling but then this would be a good addition I guess)
Not supported yet:
- structcheck (Finds unused struct fields, might be worth using but hard to satisfy, no support for go1.18 (yet))
- tparallel (checks wrong use of t.Parallel(), no support for go1.18 (yet))
- unparam (checks for unused params in functions, no support for go1.18 (yet))
- wastedassign (checks for unnecessary/unused var assignments no support for go1.18 (yet))
- noctx (checks if http requests are sent without context, no support for go1.18 (yet))
- nilerr (checks if errors get returned if there are non-nil checks and vice versa, no support for go1.18 (yet))
- nosnakecase (checks for for no snake case in naming, requires at least 1.47 of golangci (not used by us yet))
Maybe Candidates:
- gosimple (helps simplifying code, requires lots of fine tuning with settings to reduce false positives)
- containedctx (containedctx is a linter that detects struct contained context.Context field)
- contextcheck (check the function whether use a non-inherited context)
- depguard (checks for acceptable package imports, needs lots of fine tuning)
- errchkjson (helps checking for issues with json handling, also checks if there are errors that can be omitted)
- exhaustive (check exhaustiveness of enum switch statements, e.g. if every case of an enum is used, might cause lots of trouble to satisfy)
- forbidigo (could be used to prevent use of print statements, maybe only use temporarily for occasional cleanup)
- gochecknoglobals (see name, would not recommend)
- gochecknoinits (Checks that no init functions are present in Go code, hard to satisfy I guess)
- gofumpt (more strict code formatting than gofmt, Gofumpt checks whether code was gofumpt-ed, maybe?)
- importas (Enforces consistent import aliases)
- lll (reports long lines, could be okay to use)
- nlreturn (checks if there is a space before each return statement, no auto-fix though, so annoying initial use)
- testpackage (use _test for package names, maybe?)
Edited by Ghost User