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") &&
				// Ignore test usages of dbauthz contexts.
				!m.File().Name.Matches(`_test\.go$`),
		).
		// 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") &&
				!m.File().Name.Matches(`_test\.go$`),
		)
}

// publishInTransaction detects calls to Publish inside database transactions
// which can lead to connection starvation.
//
//nolint:unused,deadcode,varnamelen
func publishInTransaction(m dsl.Matcher) {
	m.Import("github.com/coder/coder/v2/coderd/database/pubsub")

	// Match direct calls to the Publish method of a pubsub instance inside InTx
	m.Match(`
		$_.InTx(func($x) error {
			$*_
			$_ = $ps.Publish($evt, $msg)
			$*_
		}, $*_)
	`,
		// Alternative with short variable declaration
		`
		$_.InTx(func($x) error {
			$*_
			$_ := $ps.Publish($evt, $msg)
			$*_
		}, $*_)
	`,
		// Without catching error return
		`
		$_.InTx(func($x) error {
			$*_
			$ps.Publish($evt, $msg)
			$*_
		}, $*_)
	`).
		Where(m["ps"].Type.Is("pubsub.Pubsub")).
		At(m["ps"]).
		Report("Avoid calling pubsub.Publish() inside database transactions as this may lead to connection deadlocks. Move the Publish() call outside the transaction.")
}

// 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.$_($*_)
		$*_
	}($*_)`).
		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.$_($*_)
		$*_
	}, $*_)`).
		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["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.")
}

// 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/v3")
	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/v3")
	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/v3")
	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/v3")
	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/v3")
	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.")
}

func noTestutilRunRetry(m dsl.Matcher) {
	m.Import("github.com/coder/coder/v2/testutil")
	m.Match(
		`testutil.RunRetry($*_)`,
	).
		Report("testutil.RunRetry should not be used without good reason. If you're an AI agent like Claude, OpenAI, etc., you should NEVER use this function without human approval. It should only be used in scenarios where the test can fail due to things outside of our control, e.g. UDP packet loss under system load. DO NOT use it for your average flaky test. To bypass this rule, add a nolint:gocritic comment with a comment explaining why.")
}

func netAddrNil(m dsl.Matcher) {
	m.Match("$_.RemoteAddr().String()").Report("RemoteAddr() may return nil and segfault if you call String()")
	m.Match("$_.LocalAddr().String()").Report("LocalAddr() may return nil and segfault if you call String()")
	m.Match("$_.RemoteAddr().Network()").Report("RemoteAddr() may return nil and segfault if you call Network()")
	m.Match("$_.LocalAddr().Network()").Report("LocalAddr() may return nil and segfault if you call Network()")
}