fix(backend): scrub ambient GH_TOKEN in internal/github tests (#2792)

Co-authored-by: ayattara <alassane.yattara@savoirfairelinux.com>
Co-authored-by: Carlos Florencio <carlosmflorencio@gmail.com>

.github/workflows/backend-tests.yml

@@ -159,17 +159,22 @@ jobs:
         working-directory: ${{ github.workspace }}
         env:
           EVENT_NAME: ${{ github.event_name }}
-          PR_BASE_SHA: ${{ github.event.pull_request.base.sha }}
+          BASE_REF: ${{ github.base_ref }}
           MERGE_GROUP_BASE_SHA: ${{ github.event.merge_group.base_sha }}
           PUSH_BEFORE: ${{ github.event.before }}
         run: |
           set -euo pipefail
 
-          # A merge_group run has no pull request behind it, so neither
-          # `pull_request.base.sha` nor `event.before` exists. The queue branch
+          # On `pull_request` the checkout is the merge ref, whose first parent
+          # is the current base branch tip. `pull_request.base.sha` is the base
+          # tip from when the PR event fired, so it can be stale after main
+          # advances. Use the merge ref's actual base tip to keep the diff and
+          # incremental lint scoped to this PR.
+          #
+          # A merge_group run has no pull request behind it, so the queue branch
           # is cut from `merge_group.base_sha`, which is the equivalent base.
           if [[ "${EVENT_NAME}" == "pull_request" ]]; then
-            BASE_SHA="${PR_BASE_SHA}"
+            BASE_SHA="$(git merge-base HEAD "origin/${BASE_REF}")"
           elif [[ "${EVENT_NAME}" == "merge_group" ]]; then
             BASE_SHA="${MERGE_GROUP_BASE_SHA}"
           else
@@ -337,9 +342,11 @@ jobs:
     # so it cannot catch a package that starts reading them again — the class
     # of bug that shipped alongside internal/gitlab's host-trust tests (they
     # only passed because CI never set KANDEV_GITLAB_HOST/GITLAB_HOST/
-    # GITLAB_TOKEN). This job poisons them with obviously-fake .invalid-TLD
-    # values so both GitLab-facing packages are forced to prove they stay
-    # hermetic instead of accidentally depending on an unset ambient env.
+    # GITLAB_TOKEN), and that internal/github hit for real with an ambient
+    # GH_TOKEN/GITHUB_TOKEN picking a PAT client over the "no auth configured"
+    # path. This job poisons them with obviously-fake values so the GitLab-
+    # and GitHub-facing packages are forced to prove they stay hermetic
+    # instead of accidentally depending on an unset ambient env.
     container: ghcr.io/kdlbs/kandev-ci:build-latest
     defaults:
       run:
@@ -375,6 +382,12 @@ jobs:
           GITLAB_TOKEN: glpat-ambient-ci-not-a-real-token
         run: go test -race ./internal/gitlab/... ./internal/agentctl/server/process/...
 
+      - name: Run GitHub-facing package under a poisoned ambient environment
+        env:
+          GH_TOKEN: ghp-ambient-ci-not-a-real-token
+          KANDEV_MOCK_GITHUB: "true"
+        run: GITHUB_TOKEN=ghp-ambient-ci-not-a-real-token-legacy go test -race ./internal/github/...
+
   # The required check. Runs on every event, including the ones where every job
   # above is correctly skipped, so the merge queue always has a conclusion to
   # read instead of a check that stays pending forever.

apps/backend/internal/github/env_hermetic_test.go

@@ -0,0 +1,219 @@
+package github
+
+import (
+	"go/ast"
+	"go/parser"
+	"go/token"
+	"os"
+	"path/filepath"
+	"strconv"
+	"strings"
+	"testing"
+)
+
+// ambientGitHubEnvVars lists every environment variable this package reads in
+// non-test code. TestMain clears all of them before any test runs.
+//
+// Developer and CI shells commonly export GH_TOKEN/GITHUB_TOKEN so the gh CLI
+// and other tooling work. Left in place they silently change auth-method
+// resolution under test: legacyEnvironmentToken picks up the ambient token
+// and newLegacyCredential returns a PAT client instead of the "no auth
+// configured" NoopClient, which fails TestNewClient_NoAuth_ReturnsNoop and
+// TestClearToken. KANDEV_MOCK_GITHUB is included for the same reason gitlab's
+// mock switch is: an ambient "true" would silently swap in the mock client.
+//
+// Clearing once here rather than per test also sidesteps t.Setenv, which
+// cannot be called from a test that uses t.Parallel().
+var ambientGitHubEnvVars = []string{
+	"GH_TOKEN",
+	"GITHUB_TOKEN",
+	"KANDEV_MOCK_GITHUB",
+}
+
+// clearAmbientGitHubEnv removes the inherited values so tests observe an
+// unconfigured environment. Individual tests that need one of these variables
+// still set it explicitly with t.Setenv.
+func clearAmbientGitHubEnv() {
+	for _, name := range ambientGitHubEnvVars {
+		if err := os.Unsetenv(name); err != nil {
+			panic("github tests: unset " + name + ": " + err.Error())
+		}
+	}
+}
+
+func TestAmbientGitHubEnvIsClearedForTests(t *testing.T) {
+	// Report only the name: one of these variables holds a real token on a
+	// developer machine and test output ends up in CI logs.
+	for _, name := range ambientGitHubEnvVars {
+		if _, ok := os.LookupEnv(name); ok {
+			t.Errorf("%s is set during tests; TestMain must clear it", name)
+		}
+	}
+}
+
+// TestAmbientGitHubEnvCoversEveryPackageEnvRead fails when non-test code grows
+// a new os.Getenv/os.LookupEnv call that ambientGitHubEnvVars does not cover,
+// so the scrub cannot silently fall behind the code it protects.
+func TestAmbientGitHubEnvCoversEveryPackageEnvRead(t *testing.T) {
+	fileSet := token.NewFileSet()
+	files := parseNonTestSources(t, fileSet)
+	constants := stringConstants(files)
+	covered := make(map[string]bool, len(ambientGitHubEnvVars))
+	for _, name := range ambientGitHubEnvVars {
+		covered[name] = true
+	}
+	for _, file := range files {
+		for _, call := range envReadCalls(file) {
+			name, resolved := resolveEnvName(call, constants)
+			if !resolved {
+				t.Errorf("%s: environment variable name is not a string literal or a constant of this "+
+					"package; a constant imported from elsewhere needs resolveEnvName taught about it",
+					fileSet.Position(call.Pos()))
+				continue
+			}
+			if !covered[name] {
+				t.Errorf("%s: %s is read in non-test code but missing from ambientGitHubEnvVars",
+					fileSet.Position(call.Pos()), name)
+			}
+		}
+	}
+}
+
+func TestEnvReadCallsResolvesOSImportAlias(t *testing.T) {
+	fileSet := token.NewFileSet()
+	file, err := parser.ParseFile(fileSet, "aliased.go", `package github
+
+import stdos "os"
+
+func readToken() string {
+	return stdos.Getenv("GH_TOKEN")
+}
+`, 0)
+	if err != nil {
+		t.Fatalf("parse aliased source: %v", err)
+	}
+
+	if got := len(envReadCalls(file)); got != 1 {
+		t.Fatalf("envReadCalls() found %d calls, want 1", got)
+	}
+}
+
+// parseNonTestSources parses every production file of the package, which is
+// the code whose environment reads the scrub has to keep up with.
+func parseNonTestSources(t *testing.T, fileSet *token.FileSet) []*ast.File {
+	t.Helper()
+	paths, err := filepath.Glob("*.go")
+	if err != nil {
+		t.Fatalf("list sources: %v", err)
+	}
+	var files []*ast.File
+	for _, path := range paths {
+		if strings.HasSuffix(path, "_test.go") {
+			continue
+		}
+		file, parseErr := parser.ParseFile(fileSet, path, nil, 0)
+		if parseErr != nil {
+			t.Fatalf("parse %s: %v", path, parseErr)
+		}
+		files = append(files, file)
+	}
+	if len(files) == 0 {
+		t.Fatal("no non-test sources found in the package directory")
+	}
+	return files
+}
+
+// stringConstants maps package-level string constant names to their values so
+// env reads written as os.Getenv(envSomeConst) resolve to the variable name.
+func stringConstants(files []*ast.File) map[string]string {
+	constants := make(map[string]string)
+	for _, file := range files {
+		for _, decl := range file.Decls {
+			genDecl, ok := decl.(*ast.GenDecl)
+			if !ok || genDecl.Tok != token.CONST {
+				continue
+			}
+			for _, spec := range genDecl.Specs {
+				valueSpec, ok := spec.(*ast.ValueSpec)
+				if !ok {
+					continue
+				}
+				for i, name := range valueSpec.Names {
+					if i >= len(valueSpec.Values) {
+						continue
+					}
+					if value, ok := literalString(valueSpec.Values[i]); ok {
+						constants[name.Name] = value
+					}
+				}
+			}
+		}
+	}
+	return constants
+}
+
+func osPackageName(file *ast.File) (string, bool) {
+	for _, spec := range file.Imports {
+		path, err := strconv.Unquote(spec.Path.Value)
+		if err != nil || path != "os" {
+			continue
+		}
+		if spec.Name != nil {
+			return spec.Name.Name, spec.Name.Name != "_" && spec.Name.Name != "."
+		}
+		return "os", true
+	}
+	return "", false
+}
+
+// envReadCalls collects every os.Getenv / os.LookupEnv call in a file.
+func envReadCalls(file *ast.File) []*ast.CallExpr {
+	osName, ok := osPackageName(file)
+	if !ok {
+		return nil
+	}
+	var calls []*ast.CallExpr
+	ast.Inspect(file, func(node ast.Node) bool {
+		call, ok := node.(*ast.CallExpr)
+		if !ok || len(call.Args) != 1 {
+			return true
+		}
+		selector, ok := call.Fun.(*ast.SelectorExpr)
+		if !ok {
+			return true
+		}
+		pkgIdent, ok := selector.X.(*ast.Ident)
+		if !ok || pkgIdent.Name != osName {
+			return true
+		}
+		if selector.Sel.Name == "Getenv" || selector.Sel.Name == "LookupEnv" {
+			calls = append(calls, call)
+		}
+		return true
+	})
+	return calls
+}
+
+func resolveEnvName(call *ast.CallExpr, constants map[string]string) (string, bool) {
+	if value, ok := literalString(call.Args[0]); ok {
+		return value, true
+	}
+	ident, ok := call.Args[0].(*ast.Ident)
+	if !ok {
+		return "", false
+	}
+	value, ok := constants[ident.Name]
+	return value, ok
+}
+
+func literalString(expr ast.Expr) (string, bool) {
+	lit, ok := expr.(*ast.BasicLit)
+	if !ok || lit.Kind != token.STRING {
+		return "", false
+	}
+	value, err := strconv.Unquote(lit.Value)
+	if err != nil {
+		return "", false
+	}
+	return value, true
+}

apps/backend/internal/github/goleak_test.go

@@ -21,6 +21,7 @@ import (
 // these readers before checking for leaks; doing it here keeps the assertion
 // strict (no broad IgnoreTopFunction suppression) without polluting prod code.
 func TestMain(m *testing.M) {
+	clearAmbientGitHubEnv()
 	exitCode := m.Run()
 	if tr, ok := http.DefaultTransport.(*http.Transport); ok {
 		tr.CloseIdleConnections()