mirror of https://github.com/coder/coder.git
470 lines
16 KiB
Go
470 lines
16 KiB
Go
// Package gorules defines custom lint rules for ruleguard.
|
|
//
|
|
// golangci-lint runs these rules via go-critic, which includes support
|
|
// for ruleguard. All Go files in this directory define lint rules
|
|
// in the Ruleguard DSL; see:
|
|
//
|
|
// - https://go-ruleguard.github.io/by-example/
|
|
// - https://pkg.go.dev/github.com/quasilyte/go-ruleguard/dsl
|
|
//
|
|
// You run one of the following commands to execute your go rules only:
|
|
//
|
|
// golangci-lint run
|
|
// golangci-lint run --disable-all --enable=gocritic
|
|
//
|
|
// Note: don't forget to run `golangci-lint cache clean`!
|
|
package gorules
|
|
|
|
import (
|
|
"github.com/quasilyte/go-ruleguard/dsl"
|
|
"github.com/quasilyte/go-ruleguard/dsl/types"
|
|
)
|
|
|
|
// dbauthzAuthorizationContext is a lint rule that protects the usage of
|
|
// system contexts. This is a dangerous pattern that can lead to
|
|
// leaking database information as a system context can be essentially
|
|
// "sudo".
|
|
//
|
|
// Anytime a function like "AsSystem" is used, it should be accompanied by a comment
|
|
// explaining why it's ok and a nolint.
|
|
func dbauthzAuthorizationContext(m dsl.Matcher) {
|
|
m.Import("context")
|
|
m.Import("github.com/coder/coder/v2/coderd/database/dbauthz")
|
|
|
|
m.Match(
|
|
`dbauthz.$f($c)`,
|
|
).
|
|
Where(
|
|
m["c"].Type.Implements("context.Context") &&
|
|
// Only report on functions that start with "As".
|
|
m["f"].Text.Matches("^As"),
|
|
).
|
|
// Instructions for fixing the lint error should be included on the dangerous function.
|
|
Report("Using '$f' is dangerous and should be accompanied by a comment explaining why it's ok and a nolint.")
|
|
}
|
|
|
|
// testingWithOwnerUser is a lint rule that detects potential permission bugs.
|
|
// Calling clitest.SetupConfig with a client authenticated as the Owner user
|
|
// can be a problem, since the CLI will be operating as that user and we may
|
|
// miss permission bugs.
|
|
//
|
|
//nolint:unused,deadcode,varnamelen
|
|
func testingWithOwnerUser(m dsl.Matcher) {
|
|
m.Import("testing")
|
|
m.Import("github.com/coder/coder/v2/cli/clitest")
|
|
m.Import("github.com/coder/coder/v2/enterprise/coderd/coderenttest")
|
|
|
|
// For both AGPL and enterprise code, we check for SetupConfig being called with a
|
|
// client authenticated as the Owner user.
|
|
m.Match(`
|
|
$_ := coderdtest.CreateFirstUser($t, $client)
|
|
$*_
|
|
clitest.$SetupConfig($t, $client, $_)
|
|
`).
|
|
Where(m["t"].Type.Implements("testing.TB") &&
|
|
m["SetupConfig"].Text.Matches("^SetupConfig$") &&
|
|
m.File().Name.Matches(`_test\.go$`)).
|
|
At(m["SetupConfig"]).
|
|
Report(`The CLI will be operating as the owner user, which has unrestricted permissions. Consider creating a different user.`)
|
|
|
|
m.Match(`
|
|
$client, $_ := coderdenttest.New($t, $*_)
|
|
$*_
|
|
clitest.$SetupConfig($t, $client, $_)
|
|
`).Where(m["t"].Type.Implements("testing.TB") &&
|
|
m["SetupConfig"].Text.Matches("^SetupConfig$") &&
|
|
m.File().Name.Matches(`_test\.go$`)).
|
|
At(m["SetupConfig"]).
|
|
Report(`The CLI will be operating as the owner user, which has unrestricted permissions. Consider creating a different user.`)
|
|
|
|
// For the enterprise code, we check for any method called on the client.
|
|
// While we want to be a bit stricter here, some methods are known to require
|
|
// the owner user, so we exclude them.
|
|
m.Match(`
|
|
$client, $_ := coderdenttest.New($t, $*_)
|
|
$*_
|
|
$_, $_ := $client.$Method($*_)
|
|
`).Where(m["t"].Type.Implements("testing.TB") &&
|
|
m.File().Name.Matches(`_test\.go$`) &&
|
|
!m["Method"].Text.Matches(`^(UpdateAppearance|Licenses|AddLicense|InsertLicense|DeleteLicense|CreateWorkspaceProxy|Replicas|Regions)$`)).
|
|
At(m["Method"]).
|
|
Report(`This client is operating as the owner user, which has unrestricted permissions. Consider creating a different user.`)
|
|
|
|
// Sadly, we need to match both one- and two-valued assignments separately.
|
|
m.Match(`
|
|
$client, $_ := coderdenttest.New($t, $*_)
|
|
$*_
|
|
$_ := $client.$Method($*_)
|
|
`).Where(m["t"].Type.Implements("testing.TB") &&
|
|
m.File().Name.Matches(`_test\.go$`) &&
|
|
!m["Method"].Text.Matches(`^(UpdateAppearance|Licenses|AddLicense|InsertLicense|DeleteLicense|CreateWorkspaceProxy|Replicas|Regions)$`)).
|
|
At(m["Method"]).
|
|
Report(`This client is operating as the owner user, which has unrestricted permissions. Consider creating a different user.`)
|
|
}
|
|
|
|
// Use xerrors everywhere! It provides additional stacktrace info!
|
|
//
|
|
//nolint:unused,deadcode,varnamelen
|
|
func xerrors(m dsl.Matcher) {
|
|
m.Import("errors")
|
|
m.Import("fmt")
|
|
m.Import("golang.org/x/xerrors")
|
|
|
|
m.Match("fmt.Errorf($arg)").
|
|
Suggest("xerrors.New($arg)").
|
|
Report("Use xerrors to provide additional stacktrace information!")
|
|
|
|
m.Match("fmt.Errorf($arg1, $*args)").
|
|
Suggest("xerrors.Errorf($arg1, $args)").
|
|
Report("Use xerrors to provide additional stacktrace information!")
|
|
|
|
m.Match("errors.$_($msg)").
|
|
Where(m["msg"].Type.Is("string")).
|
|
Suggest("xerrors.New($msg)").
|
|
Report("Use xerrors to provide additional stacktrace information!")
|
|
}
|
|
|
|
// databaseImport enforces not importing any database types into /codersdk.
|
|
//
|
|
//nolint:unused,deadcode,varnamelen
|
|
func databaseImport(m dsl.Matcher) {
|
|
m.Import("github.com/coder/coder/v2/coderd/database")
|
|
m.Match("database.$_").
|
|
Report("Do not import any database types into codersdk").
|
|
Where(m.File().PkgPath.Matches("github.com/coder/coder/v2/codersdk"))
|
|
}
|
|
|
|
// doNotCallTFailNowInsideGoroutine enforces not calling t.FailNow or
|
|
// functions that may themselves call t.FailNow in goroutines outside
|
|
// the main test goroutine. See testing.go:834 for why.
|
|
//
|
|
//nolint:unused,deadcode,varnamelen
|
|
func doNotCallTFailNowInsideGoroutine(m dsl.Matcher) {
|
|
m.Import("testing")
|
|
m.Match(`
|
|
go func($*_){
|
|
$*_
|
|
$require.$_($*_)
|
|
$*_
|
|
}($*_)`).
|
|
At(m["require"]).
|
|
Where(m["require"].Text == "require").
|
|
Report("Do not call functions that may call t.FailNow in a goroutine, as this can cause data races (see testing.go:834)")
|
|
|
|
// require.Eventually runs the function in a goroutine.
|
|
m.Match(`
|
|
require.Eventually(t, func() bool {
|
|
$*_
|
|
$require.$_($*_)
|
|
$*_
|
|
}, $*_)`).
|
|
At(m["require"]).
|
|
Where(m["require"].Text == "require").
|
|
Report("Do not call functions that may call t.FailNow in a goroutine, as this can cause data races (see testing.go:834)")
|
|
|
|
m.Match(`
|
|
go func($*_){
|
|
$*_
|
|
$t.$fail($*_)
|
|
$*_
|
|
}($*_)`).
|
|
At(m["fail"]).
|
|
Where(m["t"].Type.Implements("testing.TB") && m["fail"].Text.Matches("^(FailNow|Fatal|Fatalf)$")).
|
|
Report("Do not call functions that may call t.FailNow in a goroutine, as this can cause data races (see testing.go:834)")
|
|
}
|
|
|
|
// useStandardTimeoutsAndDelaysInTests ensures all tests use common
|
|
// constants for timeouts and delays in usual scenarios, this allows us
|
|
// to tweak them based on platform (important to avoid CI flakes).
|
|
//
|
|
//nolint:unused,deadcode,varnamelen
|
|
func useStandardTimeoutsAndDelaysInTests(m dsl.Matcher) {
|
|
m.Import("github.com/stretchr/testify/require")
|
|
m.Import("github.com/stretchr/testify/assert")
|
|
m.Import("github.com/coder/coder/v2/testutil")
|
|
|
|
m.Match(`context.WithTimeout($ctx, $duration)`).
|
|
Where(m.File().Imports("testing") && !m.File().PkgPath.Matches("testutil$") && !m["duration"].Text.Matches("^testutil\\.")).
|
|
At(m["duration"]).
|
|
Report("Do not use magic numbers in test timeouts and delays. Use the standard testutil.Wait* or testutil.Interval* constants instead.")
|
|
|
|
m.Match(`
|
|
$testify.$Eventually($t, func() bool {
|
|
$*_
|
|
}, $timeout, $interval, $*_)
|
|
`).
|
|
Where((m["testify"].Text == "require" || m["testify"].Text == "assert") &&
|
|
(m["Eventually"].Text == "Eventually" || m["Eventually"].Text == "Eventuallyf") &&
|
|
!m["timeout"].Text.Matches("^testutil\\.")).
|
|
At(m["timeout"]).
|
|
Report("Do not use magic numbers in test timeouts and delays. Use the standard testutil.Wait* or testutil.Interval* constants instead.")
|
|
|
|
m.Match(`
|
|
$testify.$Eventually($t, func() bool {
|
|
$*_
|
|
}, $timeout, $interval, $*_)
|
|
`).
|
|
Where((m["testify"].Text == "require" || m["testify"].Text == "assert") &&
|
|
(m["Eventually"].Text == "Eventually" || m["Eventually"].Text == "Eventuallyf") &&
|
|
!m["interval"].Text.Matches("^testutil\\.")).
|
|
At(m["interval"]).
|
|
Report("Do not use magic numbers in test timeouts and delays. Use the standard testutil.Wait* or testutil.Interval* constants instead.")
|
|
}
|
|
|
|
// InTx checks to ensure the database used inside the transaction closure is the transaction
|
|
// database, and not the original database that creates the tx.
|
|
func InTx(m dsl.Matcher) {
|
|
// ':=' and '=' are 2 different matches :(
|
|
m.Match(`
|
|
$x.InTx(func($y) error {
|
|
$*_
|
|
$*_ = $x.$f($*_)
|
|
$*_
|
|
})
|
|
`, `
|
|
$x.InTx(func($y) error {
|
|
$*_
|
|
$*_ := $x.$f($*_)
|
|
$*_
|
|
})
|
|
`).Where(m["x"].Text != m["y"].Text).
|
|
At(m["f"]).
|
|
Report("Do not use the database directly within the InTx closure. Use '$y' instead of '$x'.")
|
|
|
|
// When using a tx closure, ensure that if you pass the db to another
|
|
// function inside the closure, it is the tx.
|
|
// This will miss more complex cases such as passing the db as apart
|
|
// of another struct.
|
|
m.Match(`
|
|
$x.InTx(func($y database.Store) error {
|
|
$*_
|
|
$*_ = $f($*_, $x, $*_)
|
|
$*_
|
|
})
|
|
`, `
|
|
$x.InTx(func($y database.Store) error {
|
|
$*_
|
|
$*_ := $f($*_, $x, $*_)
|
|
$*_
|
|
})
|
|
`, `
|
|
$x.InTx(func($y database.Store) error {
|
|
$*_
|
|
$f($*_, $x, $*_)
|
|
$*_
|
|
})
|
|
`).Where(m["x"].Text != m["y"].Text).
|
|
At(m["f"]).Report("Pass the tx database into the '$f' function inside the closure. Use '$y' over $x'")
|
|
}
|
|
|
|
// HttpAPIErrorMessage intends to enforce constructing proper sentences as
|
|
// error messages for the api. A proper sentence includes proper capitalization
|
|
// and ends with punctuation.
|
|
// There are ways around the linter, but this should work in the common cases.
|
|
func HttpAPIErrorMessage(m dsl.Matcher) {
|
|
m.Import("github.com/coder/coder/v2/coderd/httpapi")
|
|
|
|
isNotProperError := func(v dsl.Var) bool {
|
|
return v.Type.Is("string") &&
|
|
// Either starts with a lowercase, or ends without punctuation.
|
|
// The reason I don't check for NOT ^[A-Z].*[.!?]$ is because there
|
|
// are some exceptions. Any string starting with a formatting
|
|
// directive (%s) for example is exempt.
|
|
(m["m"].Text.Matches(`^"[a-z].*`) ||
|
|
m["m"].Text.Matches(`.*[^.!?]"$`))
|
|
}
|
|
|
|
m.Match(`
|
|
httpapi.Write($_, $_, $s, httpapi.Response{
|
|
$*_,
|
|
Message: $m,
|
|
$*_,
|
|
})
|
|
`, `
|
|
httpapi.Write($_, $_, $s, httpapi.Response{
|
|
$*_,
|
|
Message: fmt.$f($m, $*_),
|
|
$*_,
|
|
})
|
|
`,
|
|
).Where(isNotProperError(m["m"])).
|
|
At(m["m"]).
|
|
Report("Field \"Message\" should be a proper sentence with a capitalized first letter and ending in punctuation. $m")
|
|
}
|
|
|
|
// HttpAPIReturn will report a linter violation if the http function is not
|
|
// returned after writing a response to the client.
|
|
func HttpAPIReturn(m dsl.Matcher) {
|
|
m.Import("github.com/coder/coder/v2/coderd/httpapi")
|
|
|
|
// Manually enumerate the httpapi function rather then a 'Where' condition
|
|
// as this is a bit more efficient.
|
|
m.Match(`
|
|
if $*_ {
|
|
httpapi.Write($*a)
|
|
}
|
|
`, `
|
|
if $*_ {
|
|
httpapi.Forbidden($*a)
|
|
}
|
|
`, `
|
|
if $*_ {
|
|
httpapi.ResourceNotFound($*a)
|
|
}
|
|
`).At(m["a"]).
|
|
Report("Forgot to return early after writing to the http response writer.")
|
|
}
|
|
|
|
// ProperRBACReturn ensures we always write to the response writer after a
|
|
// call to Authorize. If we just do a return, the client will get a status code
|
|
// 200, which is incorrect.
|
|
func ProperRBACReturn(m dsl.Matcher) {
|
|
m.Match(`
|
|
if !$_.Authorize($*_) {
|
|
return
|
|
}
|
|
`).Report("Must write to 'ResponseWriter' before returning'")
|
|
}
|
|
|
|
// FullResponseWriter ensures that any overridden response writer has full
|
|
// functionality. Mainly is hijackable and flushable.
|
|
func FullResponseWriter(m dsl.Matcher) {
|
|
m.Match(`
|
|
type $w struct {
|
|
$*_
|
|
http.ResponseWriter
|
|
$*_
|
|
}
|
|
`).
|
|
At(m["w"]).
|
|
Where(m["w"].Filter(notImplementsFullResponseWriter)).
|
|
Report("ResponseWriter \"$w\" must implement http.Flusher and http.Hijacker")
|
|
}
|
|
|
|
// notImplementsFullResponseWriter returns false if the type does not implement
|
|
// http.Flusher, http.Hijacker, and http.ResponseWriter.
|
|
func notImplementsFullResponseWriter(ctx *dsl.VarFilterContext) bool {
|
|
flusher := ctx.GetInterface(`net/http.Flusher`)
|
|
hijacker := ctx.GetInterface(`net/http.Hijacker`)
|
|
writer := ctx.GetInterface(`net/http.ResponseWriter`)
|
|
p := types.NewPointer(ctx.Type)
|
|
return !(types.Implements(p, writer) || types.Implements(ctx.Type, writer)) ||
|
|
!(types.Implements(p, flusher) || types.Implements(ctx.Type, flusher)) ||
|
|
!(types.Implements(p, hijacker) || types.Implements(ctx.Type, hijacker))
|
|
}
|
|
|
|
// slogFieldNameSnakeCase is a lint rule that ensures naming consistency
|
|
// of logged field names.
|
|
func slogFieldNameSnakeCase(m dsl.Matcher) {
|
|
m.Import("cdr.dev/slog")
|
|
m.Match(
|
|
`slog.F($name, $value)`,
|
|
).
|
|
Where(m["name"].Const && !m["name"].Text.Matches(`^"[a-z]+(_[a-z]+)*"$`)).
|
|
Report("Field name $name must be snake_case.")
|
|
}
|
|
|
|
// slogUUIDFieldNameHasIDSuffix ensures that "uuid.UUID" field has ID prefix
|
|
// in the field name.
|
|
func slogUUIDFieldNameHasIDSuffix(m dsl.Matcher) {
|
|
m.Import("cdr.dev/slog")
|
|
m.Import("github.com/google/uuid")
|
|
m.Match(
|
|
`slog.F($name, $value)`,
|
|
).
|
|
Where(m["value"].Type.Is("uuid.UUID") && !m["name"].Text.Matches(`_id"$`)).
|
|
Report(`uuid.UUID field $name must have "_id" suffix.`)
|
|
}
|
|
|
|
// slogMessageFormat ensures that the log message starts with lowercase, and does not
|
|
// end with special character.
|
|
func slogMessageFormat(m dsl.Matcher) {
|
|
m.Import("cdr.dev/slog")
|
|
m.Match(
|
|
`logger.Error($ctx, $message, $*args)`,
|
|
`logger.Warn($ctx, $message, $*args)`,
|
|
`logger.Info($ctx, $message, $*args)`,
|
|
`logger.Debug($ctx, $message, $*args)`,
|
|
|
|
`$foo.logger.Error($ctx, $message, $*args)`,
|
|
`$foo.logger.Warn($ctx, $message, $*args)`,
|
|
`$foo.logger.Info($ctx, $message, $*args)`,
|
|
`$foo.logger.Debug($ctx, $message, $*args)`,
|
|
|
|
`Logger.Error($ctx, $message, $*args)`,
|
|
`Logger.Warn($ctx, $message, $*args)`,
|
|
`Logger.Info($ctx, $message, $*args)`,
|
|
`Logger.Debug($ctx, $message, $*args)`,
|
|
|
|
`$foo.Logger.Error($ctx, $message, $*args)`,
|
|
`$foo.Logger.Warn($ctx, $message, $*args)`,
|
|
`$foo.Logger.Info($ctx, $message, $*args)`,
|
|
`$foo.Logger.Debug($ctx, $message, $*args)`,
|
|
).
|
|
Where(
|
|
(
|
|
// It doesn't end with a special character:
|
|
m["message"].Text.Matches(`[.!?]"$`) ||
|
|
// it starts with lowercase:
|
|
m["message"].Text.Matches(`^"[A-Z]{1}`) &&
|
|
// but there are exceptions:
|
|
!m["message"].Text.Matches(`^"Prometheus`) &&
|
|
!m["message"].Text.Matches(`^"X11`) &&
|
|
!m["message"].Text.Matches(`^"CSP`) &&
|
|
!m["message"].Text.Matches(`^"OIDC`))).
|
|
Report(`Message $message must start with lowercase, and does not end with a special characters.`)
|
|
}
|
|
|
|
// slogMessageLength ensures that important log messages are meaningful, and must be at least 16 characters long.
|
|
func slogMessageLength(m dsl.Matcher) {
|
|
m.Import("cdr.dev/slog")
|
|
m.Match(
|
|
`logger.Error($ctx, $message, $*args)`,
|
|
`logger.Warn($ctx, $message, $*args)`,
|
|
`logger.Info($ctx, $message, $*args)`,
|
|
|
|
`$foo.logger.Error($ctx, $message, $*args)`,
|
|
`$foo.logger.Warn($ctx, $message, $*args)`,
|
|
`$foo.logger.Info($ctx, $message, $*args)`,
|
|
|
|
`Logger.Error($ctx, $message, $*args)`,
|
|
`Logger.Warn($ctx, $message, $*args)`,
|
|
`Logger.Info($ctx, $message, $*args)`,
|
|
|
|
`$foo.Logger.Error($ctx, $message, $*args)`,
|
|
`$foo.Logger.Warn($ctx, $message, $*args)`,
|
|
`$foo.Logger.Info($ctx, $message, $*args)`,
|
|
|
|
// no debug
|
|
).
|
|
Where(
|
|
// It has at least 16 characters (+ ""):
|
|
m["message"].Text.Matches(`^".{0,15}"$`) &&
|
|
// but there are exceptions:
|
|
!m["message"].Text.Matches(`^"command exit"$`)).
|
|
Report(`Message $message is too short, it must be at least 16 characters long.`)
|
|
}
|
|
|
|
// slogErr ensures that errors are logged with "slog.Error" instead of "slog.F"
|
|
func slogError(m dsl.Matcher) {
|
|
m.Import("cdr.dev/slog")
|
|
m.Match(
|
|
`slog.F($name, $value)`,
|
|
).
|
|
Where(m["name"].Const && m["value"].Type.Is("error") && !m["name"].Text.Matches(`^"internal_error"$`)).
|
|
Report(`Error should be logged using "slog.Error" instead.`)
|
|
}
|
|
|
|
// withTimezoneUTC ensures that we don't just sprinkle dbtestutil.WithTimezone("UTC") about
|
|
// to work around real timezone bugs in our code.
|
|
//
|
|
//nolint:unused,deadcode,varnamelen
|
|
func withTimezoneUTC(m dsl.Matcher) {
|
|
m.Match(
|
|
`dbtestutil.WithTimezone($tz)`,
|
|
).Where(
|
|
m["tz"].Text.Matches(`[uU][tT][cC]"$`),
|
|
).Report(`Setting database timezone to UTC may mask timezone-related bugs.`).
|
|
At(m["tz"])
|
|
}
|