coder/scripts/rules.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"])
}

// workspaceActivity ensures that updating workspace activity is only done in the workspacestats package.
//
//nolint:unused,deadcode,varnamelen
func workspaceActivity(m dsl.Matcher) {
	m.Import("github.com/coder/coder/v2/coderd/database")
	m.Match(
		`$_.ActivityBumpWorkspace($_, $_)`,
		`$_.UpdateWorkspaceLastUsedAt($_, $_)`,
		`$_.BatchUpdateWorkspaceLastUsedAt($_, $_)`,
		`$_.UpdateTemplateWorkspacesLastUsedAt($_, $_)`,
		`$_.InsertWorkspaceAgentStats($_, $_)`,
		`$_.InsertWorkspaceAppStats($_, $_)`,
	).Where(
		!m.File().PkgPath.Matches(`workspacestats`) &&
			!m.File().PkgPath.Matches(`dbauthz$`) &&
			!m.File().PkgPath.Matches(`dbgen$`) &&
			!m.File().Name.Matches(`_test\.go$`),
	).Report("Updating workspace activity should always be done in the workspacestats package.")
}

// noExecInAgent ensures that packages under agent/ don't use exec.Command or
// exec.CommandContext directly.
//
//nolint:unused,deadcode,varnamelen
func noExecInAgent(m dsl.Matcher) {
	m.Import("os/exec")
	m.Match(
		`exec.Command($*_)`,
		`exec.CommandContext($*_)`,
	).
		Where(
			m.File().PkgPath.Matches("/agent/") &&
				!m.File().PkgPath.Matches("/agentexec") &&
				!m.File().Name.Matches(`_test\.go$`),
		).
		Report("The agent and its subpackages should not use exec.Command or exec.CommandContext directly. Consider using an agentexec.Execer instead.")
}

// noPTYInAgent ensures that packages under agent/ don't use pty.Command or
// pty.CommandContext directly.
//
//nolint:unused,deadcode,varnamelen
func noPTYInAgent(m dsl.Matcher) {
	m.Import("github.com/coder/coder/v2/pty")
	m.Match(
		`pty.Command($*_)`,
		`pty.CommandContext($*_)`,
	).
		Where(
			m.File().PkgPath.Matches(`/agent/`) &&
				!m.File().PkgPath.Matches(`/agentexec`) &&
				!m.File().Name.Matches(`_test\.go$`),
		).
		Report("The agent and its subpackages should not use pty.Command or pty.CommandContext directly. Consider using an agentexec.Execer instead.")
}