diff --git a/docs/adr/0002-allow-secrets-in-step-env-bindings-under-strict-mode.md b/docs/adr/0002-allow-secrets-in-step-env-bindings-under-strict-mode.md new file mode 100644 index 00000000000..609f23eac38 --- /dev/null +++ b/docs/adr/0002-allow-secrets-in-step-env-bindings-under-strict-mode.md @@ -0,0 +1,84 @@ +# ADR-0002: Allow Secrets in Step-Level env: Bindings Under Strict Mode + +**Date**: 2026-04-11 +**Status**: Draft +**Deciders**: pelikhan, Copilot + +--- + +## Part 1 — Narrative (Human-Friendly) + +### Context + +The workflow compiler enforces a "strict mode" that restricts what GitHub Actions expressions authors may use in user-defined step fields, with the goal of preventing secrets from leaking into the agent job environment. Before this change, strict mode applied an all-or-nothing rule: the presence of *any* `${{ secrets.* }}` expression in `pre-steps`, `steps`, or `post-steps` was an error, regardless of where inside the step the expression appeared. This forced authors who needed tool credentials (API tokens, OAuth keys, SonarQube tokens, etc.) to opt out of strict mode entirely via `strict: false`, surrendering all other protections the mode provides. The GitHub Actions platform already distinguishes between controlled secret surfaces: a secret bound to a step's `env:` map is automatically masked by the runner before it can appear in logs, whereas a secret interpolated directly into a `run:` script string can be echoed, logged, or passed to an external process before masking takes effect. + +### Decision + +We will introduce a per-field classification of secret references within a step, distinguishing "safe" bindings (`env:` fields, which are automatically masked by GitHub Actions) from "unsafe" inline interpolations (`run:`, `with:`, and all other step fields). In strict mode, only unsafe secret references will be treated as errors; secrets that appear exclusively in `env:` bindings will be permitted. We implement this via a new `classifyStepSecrets()` helper that partitions a step's secret references, and update `validateStepsSectionSecrets()` to only block the unsafe partition under strict mode. + +### Alternatives Considered + +#### Alternative 1: Keep the Existing All-or-Nothing Block + +Maintaining the current policy — all `secrets.*` references are errors in strict mode — is the simplest approach. It was rejected because it creates an unacceptable ergonomic cost: workflows that need to supply API tokens to scanning tools (a common real-world pattern) must disable strict mode entirely, removing protection against other classes of secret leak that strict mode prevents. The framework's own generated jobs already use `env:` bindings for secrets, making it inconsistent to block that same pattern in user-defined steps. + +#### Alternative 2: Allow All Secrets in Strict Mode + +Relaxing strict mode to permit secrets everywhere (matching non-strict mode, but without the warning) would maximally ease author burden. It was rejected because it removes the core protection that strict mode is designed to provide: preventing accidental inline interpolation of secrets into command strings where they can be observed before the runner's masking logic fires. + +#### Alternative 3: Introduce a Per-Step Annotation to Opt In + +A third option was to keep secrets blocked by default but allow authors to annotate individual steps (e.g., with a `allow-secrets: true` flag) to opt into secret access. This was rejected as unnecessarily complex: the `env:` binding pattern is already a well-established GitHub Actions idiom, so using the structural location of the reference (field name `env` vs. any other field) as the signal provides an equivalent security boundary without requiring new syntax. + +### Consequences + +#### Positive +- Workflows that supply tool credentials via `env:` bindings no longer need to disable strict mode entirely, preserving all other strict-mode protections. +- The enforcement policy now mirrors how the framework's own generated jobs handle secrets, making the security model internally consistent. +- The error message for blocked secrets now explicitly suggests `env:` bindings as an alternative, improving the developer experience. +- The behavior aligns with GitHub Actions' native masking guarantee: `env:` bindings are masked by the runner before command execution. + +#### Negative +- The security policy is now more nuanced: reviewers must understand the `env:` vs. inline distinction rather than a simple blanket rule, increasing the surface area to reason about. +- The `classifyStepSecrets()` function must be kept accurate as the step data model evolves; an incorrectly classified field could silently downgrade a secret from "unsafe" to "safe". +- Non-strict mode still emits a warning for all secrets (including `env:`-bound ones), which may be slightly misleading now that `env:` bindings are considered safe in strict mode. + +#### Neutral +- Existing workflows that used `strict: false` specifically to allow `env:` bindings can now remove that override and adopt strict mode, but this migration is voluntary. +- Unit and integration tests must now cover both the "env-only" allowed path and the "mixed env + run" blocked path to maintain adequate coverage. + +--- + +## Part 2 — Normative Specification (RFC 2119) + +> The key words **MUST**, **MUST NOT**, **REQUIRED**, **SHALL**, **SHALL NOT**, **SHOULD**, **SHOULD NOT**, **RECOMMENDED**, **MAY**, and **OPTIONAL** in this section are to be interpreted as described in [RFC 2119](https://www.rfc-editor.org/rfc/rfc2119). + +### Secret Classification + +1. Implementations **MUST** classify each `${{ secrets.* }}` reference within a step by the name of the YAML field in which it appears. +2. A secret reference found inside a well-formed `env:` mapping (i.e., the `env:` value is a YAML map of key-value pairs) **MUST** be classified as an *env-bound* reference. +3. A secret reference found inside a malformed `env:` value (e.g., `env:` is a bare string or a YAML sequence) **MUST** be classified as an *unsafe* reference, because the runner cannot apply per-variable masking to such values. +4. A secret reference found in any other step field (including but not limited to `run:`, `with:`, `name:`, `if:`) **MUST** be classified as an *unsafe* reference. +5. A step value that is not a YAML map (e.g., a raw string) **MUST** treat all secret references within it as *unsafe* references. +6. When a step contains *env-bound* secret references AND any non-`env:` field references `GITHUB_ENV`, implementations **MUST** reclassify all *env-bound* references in that step as *unsafe*, because writing to `GITHUB_ENV` persists secrets to subsequent steps (including the agent step). + +### Strict Mode Enforcement + +1. When strict mode is active, implementations **MUST** return an error if one or more *unsafe* secret references are found in a `pre-steps`, `steps`, or `post-steps` section. +2. When strict mode is active, implementations **MUST NOT** return an error solely because *env-bound* secret references are present in a section. +3. The error message for blocked secrets in strict mode **SHOULD** suggest the use of step-level `env:` bindings as an alternative to inline interpolation. +4. The built-in `GITHUB_TOKEN` **MUST** be filtered out from both *unsafe* and *env-bound* reference lists before strict-mode enforcement, as it is present in every runner environment by default. + +### Non-Strict Mode Behavior + +1. When strict mode is not active, implementations **MUST** emit a warning (to stderr) if any secret references — whether *env-bound* or *unsafe* — are found in a steps section. +2. Implementations **MUST NOT** return an error in non-strict mode for secret references in steps sections; the warning is advisory only. +3. Implementations **SHOULD** deduplicate secret reference identifiers before including them in warning or error messages. + +### Conformance + +An implementation is considered conformant with this ADR if it satisfies all **MUST** and **MUST NOT** requirements above. Failure to meet any **MUST** or **MUST NOT** requirement constitutes non-conformance. In particular: permitting *unsafe* secret references in strict mode, or blocking *env-bound* references in strict mode, are both non-conformant behaviors. + +--- + +*This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/24279167784) workflow. The PR author must review, complete, and finalize this document before the PR can merge.* diff --git a/pkg/workflow/strict_mode_steps_secrets_integration_test.go b/pkg/workflow/strict_mode_steps_secrets_integration_test.go new file mode 100644 index 00000000000..3ee467c48a9 --- /dev/null +++ b/pkg/workflow/strict_mode_steps_secrets_integration_test.go @@ -0,0 +1,334 @@ +//go:build integration + +package workflow + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/github/gh-aw/pkg/stringutil" + "github.com/github/gh-aw/pkg/testutil" +) + +// TestStrictModeStepEnvSecretsAllowed tests full compilation of workflows +// that use secrets in step-level env: bindings under strict mode. +// These should compile successfully because env: bindings are controlled, +// masked surfaces in GitHub Actions. +func TestStrictModeStepEnvSecretsAllowed(t *testing.T) { + tests := []struct { + name string + content string + }{ + { + name: "single secret in step env binding compiles in strict mode", + content: `--- +on: workflow_dispatch +permissions: + contents: read + issues: read + pull-requests: read +engine: copilot +steps: + - name: Run scan with secret + env: + SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }} + run: sonar-scanner +--- + +# Step Env Secret Test + +Run a tool with a secret from env. +`, + }, + { + name: "multiple secrets across multiple steps compile in strict mode", + content: `--- +on: workflow_dispatch +permissions: + contents: read + issues: read + pull-requests: read +engine: copilot +steps: + - name: Run scan with credentials + env: + SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }} + CORONA_TOKEN: ${{ secrets.CORONA_TOKEN }} + run: | + sonar-scanner + corona-lint check + - name: Run auth check + env: + CIAM_CLIENT_ID: ${{ secrets.CIAM_CLIENT_ID }} + CIAM_CLIENT_SECRET: ${{ secrets.CIAM_CLIENT_SECRET }} + SI_TOKEN: ${{ secrets.SI_TOKEN }} + run: | + ciam-auth verify + si-tool check +--- + +# Multi-Secret Step Env Test + +Agent workflow with multiple tool credentials in step env bindings. +`, + }, + { + name: "GITHUB_TOKEN in step env alongside user secrets compiles in strict mode", + content: `--- +on: workflow_dispatch +permissions: + contents: read + issues: read + pull-requests: read +engine: copilot +steps: + - name: Run tool with tokens + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + API_KEY: ${{ secrets.API_KEY }} + run: my-tool --authenticate +--- + +# GITHUB_TOKEN Mixed Test + +Step env with both GITHUB_TOKEN and user secrets. +`, + }, + { + name: "pre-steps with secrets in env compile in strict mode", + content: `--- +on: workflow_dispatch +permissions: + contents: read + issues: read + pull-requests: read +engine: copilot +pre-steps: + - name: Run pre-check with credentials + env: + SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }} + run: sonar-scanner --pre-check +--- + +# Pre-Steps Env Secret Test + +Pre-steps with secrets in env bindings. +`, + }, + { + name: "post-steps with secrets in env compile in strict mode", + content: `--- +on: workflow_dispatch +permissions: + contents: read + issues: read + pull-requests: read +engine: copilot +post-steps: + - name: Send notification + env: + SLACK_TOKEN: ${{ secrets.SLACK_TOKEN }} + run: send-notification +--- + +# Post-Steps Env Secret Test + +Post-steps with secrets in env bindings. +`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tmpDir := testutil.TempDir(t, "strict-step-env-secrets-test") + testFile := filepath.Join(tmpDir, "test-workflow.md") + if err := os.WriteFile(testFile, []byte(tt.content), 0644); err != nil { + t.Fatal(err) + } + + compiler := NewCompiler() + compiler.SetStrictMode(true) + err := compiler.CompileWorkflow(testFile) + if err != nil { + t.Errorf("Expected compilation to succeed with secrets in step env bindings, but got error: %v", err) + } + + // Verify lock file was generated + lockFile := stringutil.MarkdownToLockFile(testFile) + if _, err := os.Stat(lockFile); os.IsNotExist(err) { + t.Errorf("Expected lock file %s to be generated", lockFile) + } + }) + } +} + +// TestStrictModeStepUnsafeSecretsBlocked tests that secrets in non-env step +// fields (run, with, etc.) are still blocked in strict mode during full +// compilation. +func TestStrictModeStepUnsafeSecretsBlocked(t *testing.T) { + tests := []struct { + name string + content string + errorMsg string + }{ + { + name: "secret in run field is blocked in strict mode", + content: `--- +on: workflow_dispatch +permissions: + contents: read + issues: read + pull-requests: read +engine: copilot +steps: + - name: Inline secret leak + run: echo ${{ secrets.API_TOKEN }} +--- + +# Unsafe Run Secret Test + +This should fail because secrets are used inline in run. +`, + errorMsg: "strict mode: secrets expressions detected in 'steps' section", + }, + { + name: "secret in with field is blocked in strict mode", + content: `--- +on: workflow_dispatch +permissions: + contents: read + issues: read + pull-requests: read +engine: copilot +steps: + - uses: some/action@v1 + with: + token: ${{ secrets.MY_API_TOKEN }} +--- + +# Unsafe With Secret Test + +This should fail because secrets are used in with field. +`, + errorMsg: "strict mode: secrets expressions detected in 'steps' section", + }, + { + name: "mixed env and run secrets - run secret still blocked", + content: `--- +on: workflow_dispatch +permissions: + contents: read + issues: read + pull-requests: read +engine: copilot +steps: + - name: Mixed secrets + env: + SAFE_KEY: ${{ secrets.SAFE_KEY }} + run: echo ${{ secrets.LEAKED_KEY }} +--- + +# Mixed Secret Test + +Should fail because run field contains a secret even though env is safe. +`, + errorMsg: "strict mode: secrets expressions detected in 'steps' section", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tmpDir := testutil.TempDir(t, "strict-step-unsafe-secrets-test") + testFile := filepath.Join(tmpDir, "test-workflow.md") + if err := os.WriteFile(testFile, []byte(tt.content), 0644); err != nil { + t.Fatal(err) + } + + compiler := NewCompiler() + compiler.SetStrictMode(true) + err := compiler.CompileWorkflow(testFile) + + if err == nil { + t.Error("Expected compilation to fail with secrets in non-env step fields, but it succeeded") + } else if !strings.Contains(err.Error(), tt.errorMsg) { + t.Errorf("Expected error containing %q, got %q", tt.errorMsg, err.Error()) + } + }) + } +} + +// TestStrictModeStepEnvSecretsErrorSuggestsEnvBindings verifies that the error +// message for unsafe secrets suggests using step-level env: bindings as an +// alternative. +func TestStrictModeStepEnvSecretsErrorSuggestsEnvBindings(t *testing.T) { + content := `--- +on: workflow_dispatch +permissions: + contents: read + issues: read + pull-requests: read +engine: copilot +steps: + - name: Inline secret + run: echo ${{ secrets.MY_TOKEN }} +--- + +# Error Message Test + +Check that error suggests env bindings. +` + tmpDir := testutil.TempDir(t, "strict-error-msg-test") + testFile := filepath.Join(tmpDir, "test-workflow.md") + if err := os.WriteFile(testFile, []byte(content), 0644); err != nil { + t.Fatal(err) + } + + compiler := NewCompiler() + compiler.SetStrictMode(true) + err := compiler.CompileWorkflow(testFile) + + if err == nil { + t.Fatal("Expected compilation to fail, but it succeeded") + } + if !strings.Contains(err.Error(), "env: bindings") { + t.Errorf("Expected error to suggest env: bindings, got: %v", err) + } +} + +// TestNonStrictModeStepSecretsAllowedWithWarning verifies that in non-strict +// mode, secrets in any step field (including non-env) are allowed with a +// warning. +func TestNonStrictModeStepSecretsAllowedWithWarning(t *testing.T) { + content := `--- +on: workflow_dispatch +permissions: + contents: read + issues: read + pull-requests: read +engine: copilot +strict: false +steps: + - name: Use inline secret + run: echo ${{ secrets.API_KEY }} +--- + +# Non-Strict Step Secret Test + +Should compile with a warning in non-strict mode. +` + tmpDir := testutil.TempDir(t, "non-strict-step-secrets-test") + testFile := filepath.Join(tmpDir, "test-workflow.md") + if err := os.WriteFile(testFile, []byte(content), 0644); err != nil { + t.Fatal(err) + } + + compiler := NewCompiler() + // Do NOT set strict mode - let frontmatter control it + err := compiler.CompileWorkflow(testFile) + + if err != nil { + t.Errorf("Non-strict mode should allow secrets in step fields with a warning, but got error: %v", err) + } +} diff --git a/pkg/workflow/strict_mode_steps_validation.go b/pkg/workflow/strict_mode_steps_validation.go index d3fb14c7f6b..f579fcd43a8 100644 --- a/pkg/workflow/strict_mode_steps_validation.go +++ b/pkg/workflow/strict_mode_steps_validation.go @@ -1,8 +1,10 @@ // This file contains strict mode validation for secrets in custom steps. // // It validates that secrets expressions are not used in custom steps (steps and -// post-steps injected in the agent job). In strict mode this is an error; in -// non-strict mode a warning is emitted instead. +// post-steps injected in the agent job). In strict mode, secrets in step-level +// env: bindings are allowed (controlled binding, masked by GitHub Actions), +// while secrets in other fields (run, with, etc.) are treated as errors. +// In non-strict mode a warning is emitted instead. // // The goal is to minimise the number of secrets present in the agent job: the // only secrets that should appear there are those required to configure the @@ -13,6 +15,9 @@ package workflow import ( "fmt" "os" + "regexp" + "slices" + "sort" "strings" "github.com/github/gh-aw/pkg/console" @@ -22,8 +27,9 @@ import ( // validateStepsSecrets checks the "pre-steps", "steps", and "post-steps" frontmatter sections // for secrets expressions (e.g. ${{ secrets.MY_SECRET }}). // -// In strict mode the presence of any such expression is treated as an error. -// In non-strict mode a warning is emitted instead. +// In strict mode, secrets in step-level env: bindings are allowed (controlled, +// masked binding), while secrets in other fields (run, with, etc.) are errors. +// In non-strict mode a warning is emitted for all secrets. func (c *Compiler) validateStepsSecrets(frontmatter map[string]any) error { for _, sectionName := range []string{"pre-steps", "steps", "post-steps"} { if err := c.validateStepsSectionSecrets(frontmatter, sectionName); err != nil { @@ -35,6 +41,10 @@ func (c *Compiler) validateStepsSecrets(frontmatter map[string]any) error { // validateStepsSectionSecrets inspects a single steps section (named by sectionName) // inside frontmatter for any secrets.* expressions. +// +// In strict mode, secrets in step-level env: bindings are allowed because they are +// controlled bindings that are automatically masked by GitHub Actions. Secrets in +// other step fields (run, with, etc.) are still treated as errors. func (c *Compiler) validateStepsSectionSecrets(frontmatter map[string]any, sectionName string) error { rawValue, exists := frontmatter[sectionName] if !exists { @@ -48,39 +58,57 @@ func (c *Compiler) validateStepsSectionSecrets(frontmatter map[string]any, secti return nil } - var secretRefs []string + // Separate secrets found in step-level env: bindings (safe, controlled) + // from secrets found in other fields (unsafe, potential leak). + var unsafeSecretRefs []string + var envSecretRefs []string for _, step := range steps { - refs := extractSecretsFromStepValue(step) - secretRefs = append(secretRefs, refs...) + unsafe, envOnly := classifyStepSecrets(step) + unsafeSecretRefs = append(unsafeSecretRefs, unsafe...) + envSecretRefs = append(envSecretRefs, envOnly...) } // Filter out the built-in GITHUB_TOKEN: it is already present in every runner // environment and is not a user-defined secret that could be accidentally leaked. - secretRefs = filterBuiltinTokens(secretRefs) + unsafeSecretRefs = filterBuiltinTokens(unsafeSecretRefs) + envSecretRefs = filterBuiltinTokens(envSecretRefs) - if len(secretRefs) == 0 { + allSecretRefs := append(unsafeSecretRefs, envSecretRefs...) + + if len(allSecretRefs) == 0 { strictModeValidationLog.Printf("No secrets found in %s section", sectionName) return nil } - strictModeValidationLog.Printf("Found %d secret expression(s) in %s section: %v", len(secretRefs), sectionName, secretRefs) - - // Deduplicate for cleaner messages. - secretRefs = sliceutil.Deduplicate(secretRefs) + strictModeValidationLog.Printf("Found %d secret expression(s) in %s section: %d unsafe, %d in env bindings", + len(allSecretRefs), sectionName, len(unsafeSecretRefs), len(envSecretRefs)) if c.strictMode { + // In strict mode, secrets in step-level env: bindings are allowed + // (controlled binding, masked by GitHub Actions). Only block secrets + // found in other fields (run, with, etc.). + if len(unsafeSecretRefs) == 0 { + strictModeValidationLog.Printf("All secrets in %s section are in env bindings (allowed in strict mode)", sectionName) + return nil + } + + unsafeSecretRefs = sliceutil.Deduplicate(unsafeSecretRefs) + sort.Strings(unsafeSecretRefs) return fmt.Errorf( "strict mode: secrets expressions detected in '%s' section may be leaked to the agent job. Found: %s. "+ - "Operations requiring secrets must be moved to a separate job outside the agent job", - sectionName, strings.Join(secretRefs, ", "), + "Operations requiring secrets must be moved to a separate job outside the agent job, "+ + "or use step-level env: bindings instead", + sectionName, strings.Join(unsafeSecretRefs, ", "), ) } - // Non-strict mode: emit a warning. + // Non-strict mode: emit a warning for all secrets. + allSecretRefs = sliceutil.Deduplicate(allSecretRefs) + sort.Strings(allSecretRefs) warningMsg := fmt.Sprintf( "Warning: secrets expressions detected in '%s' section may be leaked to the agent job. Found: %s. "+ "Consider moving operations requiring secrets to a separate job outside the agent job.", - sectionName, strings.Join(secretRefs, ", "), + sectionName, strings.Join(allSecretRefs, ", "), ) fmt.Fprintln(os.Stderr, console.FormatWarningMessage(warningMsg)) c.IncrementWarningCount() @@ -88,6 +116,56 @@ func (c *Compiler) validateStepsSectionSecrets(frontmatter map[string]any, secti return nil } +// githubEnvWritePattern matches common patterns that write to $GITHUB_ENV, +// which would leak step-level env-bound secrets to subsequent steps. +// Covers: >> "$GITHUB_ENV", >> $GITHUB_ENV, >> ${GITHUB_ENV} +var githubEnvWritePattern = regexp.MustCompile(`(?i)GITHUB_ENV`) + +// classifyStepSecrets separates secrets found in a step into two categories: +// - unsafeRefs: secrets found in fields other than "env" (e.g. run, with), +// or secrets in env: bindings when the step also writes to $GITHUB_ENV +// - envRefs: secrets found in step-level env: bindings (controlled, masked) +// +// Only secrets in well-formed env: mappings (map[string]any) are classified as +// envRefs. Malformed env values (string, slice, etc.) are treated as unsafe to +// prevent strict-mode bypass via invalid YAML like `env: "${{ secrets.TOKEN }}"`. +// +// Steps that reference $GITHUB_ENV in their run: command while also using +// env-bound secrets are treated as entirely unsafe, because writing to +// $GITHUB_ENV would leak the secret to subsequent steps (including the agent). +func classifyStepSecrets(step any) (unsafeRefs, envRefs []string) { + stepMap, ok := step.(map[string]any) + if !ok { + // Non-map steps: all secrets are considered unsafe. + return extractSecretsFromStepValue(step), nil + } + + var localUnsafe, localEnv []string + for key, val := range stepMap { + refs := extractSecretsFromStepValue(val) + if key == "env" { + if _, isMap := val.(map[string]any); isMap { + localEnv = append(localEnv, refs...) + } else { + // Malformed env (string, slice, etc.): treat as unsafe. + localUnsafe = append(localUnsafe, refs...) + } + } else { + localUnsafe = append(localUnsafe, refs...) + } + } + + // If the step has env-bound secrets AND references $GITHUB_ENV in any + // non-env field, reclassify all env refs as unsafe. Writing to + // $GITHUB_ENV would persist the secret to subsequent steps. + if len(localEnv) > 0 && stepReferencesGitHubEnv(stepMap) { + localUnsafe = append(localUnsafe, localEnv...) + localEnv = nil + } + + return localUnsafe, localEnv +} + // extractSecretsFromStepValue recursively walks a step value (which may be a map, // slice, or primitive) and returns all secrets.* expressions found in string values. func extractSecretsFromStepValue(value any) []string { @@ -109,16 +187,62 @@ func extractSecretsFromStepValue(value any) []string { return refs } -// filterBuiltinTokens removes secret expressions that reference GitHub's built-in -// GITHUB_TOKEN from the list. GITHUB_TOKEN is automatically provided by the runner -// environment and is not a user-defined secret; it therefore does not represent an -// accidental leak into the agent job. +// filterBuiltinTokens removes secret expressions that reference *only* GitHub's +// built-in GITHUB_TOKEN from the list. GITHUB_TOKEN is automatically provided by +// the runner environment and is not a user-defined secret; it therefore does not +// represent an accidental leak into the agent job. +// +// Expressions that reference GITHUB_TOKEN alongside other secrets (e.g. +// "${{ secrets.GITHUB_TOKEN && secrets.OTHER }}") are NOT filtered, because the +// other secret still represents a potential leak. Expressions referencing secrets +// whose names merely start with GITHUB_TOKEN (e.g. secrets.GITHUB_TOKEN_SUFFIX) +// are also NOT filtered. func filterBuiltinTokens(refs []string) []string { out := refs[:0:0] for _, ref := range refs { - if !strings.Contains(ref, "secrets.GITHUB_TOKEN") { + names := secretsNamePattern.FindAllStringSubmatch(ref, -1) + allBuiltin := len(names) > 0 + for _, m := range names { + if len(m) >= 2 && m[1] != "GITHUB_TOKEN" { + allBuiltin = false + break + } + } + if !allBuiltin { out = append(out, ref) } } return out } + +// stepReferencesGitHubEnv returns true if any non-env field in the step map +// contains a reference to GITHUB_ENV (e.g. in a run: command that writes to it). +func stepReferencesGitHubEnv(stepMap map[string]any) bool { + for key, val := range stepMap { + if key == "env" { + continue + } + if valueReferencesGitHubEnv(val) { + return true + } + } + return false +} + +// valueReferencesGitHubEnv recursively checks whether a value contains a +// reference to GITHUB_ENV. +func valueReferencesGitHubEnv(value any) bool { + switch v := value.(type) { + case string: + return githubEnvWritePattern.MatchString(v) + case map[string]any: + for _, fieldValue := range v { + if valueReferencesGitHubEnv(fieldValue) { + return true + } + } + case []any: + return slices.ContainsFunc(v, valueReferencesGitHubEnv) + } + return false +} diff --git a/pkg/workflow/strict_mode_steps_validation_test.go b/pkg/workflow/strict_mode_steps_validation_test.go index ba13d4b6adc..a070a5231df 100644 --- a/pkg/workflow/strict_mode_steps_validation_test.go +++ b/pkg/workflow/strict_mode_steps_validation_test.go @@ -83,7 +83,7 @@ func TestValidateStepsSecrets(t *testing.T) { errorMsg: "strict mode: secrets expressions detected in 'steps' section", }, { - name: "steps with secret in env field in strict mode fails", + name: "steps with secret in env field only in strict mode is allowed", frontmatter: map[string]any{ "steps": []any{ map[string]any{ @@ -96,8 +96,7 @@ func TestValidateStepsSecrets(t *testing.T) { }, }, strictMode: true, - expectError: true, - errorMsg: "strict mode: secrets expressions detected in 'steps' section", + expectError: false, }, { name: "steps with secret in with field in strict mode fails", @@ -164,7 +163,7 @@ func TestValidateStepsSecrets(t *testing.T) { expectError: false, }, { - name: "multiple secrets in steps are all reported", + name: "multiple secrets in env bindings only are allowed in strict mode", frontmatter: map[string]any{ "steps": []any{ map[string]any{ @@ -177,6 +176,127 @@ func TestValidateStepsSecrets(t *testing.T) { }, }, strictMode: true, + expectError: false, + }, + { + name: "secrets in both env and run fields in strict mode fails for run only", + frontmatter: map[string]any{ + "steps": []any{ + map[string]any{ + "name": "Step with mixed secrets", + "env": map[string]any{ + "SAFE_KEY": "${{ secrets.SAFE_KEY }}", + }, + "run": "echo ${{ secrets.LEAKED }}", + }, + }, + }, + strictMode: true, + expectError: true, + errorMsg: "strict mode: secrets expressions detected in 'steps' section", + }, + { + name: "secrets in env only across multiple steps are allowed in strict mode", + frontmatter: map[string]any{ + "steps": []any{ + map[string]any{ + "name": "Step 1", + "run": "my-tool scan", + "env": map[string]any{ + "SONAR_TOKEN": "${{ secrets.SONAR_TOKEN }}", + }, + }, + map[string]any{ + "name": "Step 2", + "run": "other-tool check", + "env": map[string]any{ + "CORONA_TOKEN": "${{ secrets.CORONA_TOKEN }}", + "SI_TOKEN": "${{ secrets.SI_TOKEN }}", + }, + }, + }, + }, + strictMode: true, + expectError: false, + }, + { + name: "post-steps with secret in env only is allowed in strict mode", + frontmatter: map[string]any{ + "post-steps": []any{ + map[string]any{ + "name": "Notify", + "run": "send-notification", + "env": map[string]any{ + "SLACK_TOKEN": "${{ secrets.SLACK_TOKEN }}", + }, + }, + }, + }, + strictMode: true, + expectError: false, + }, + { + name: "pre-steps with secret in env only is allowed in strict mode", + frontmatter: map[string]any{ + "pre-steps": []any{ + map[string]any{ + "name": "Run pre-check with credentials", + "env": map[string]any{ + "CIAM_CLIENT_ID": "${{ secrets.CIAM_CLIENT_ID }}", + }, + "run": "ciam-auth verify", + }, + }, + }, + strictMode: true, + expectError: false, + }, + { + name: "malformed string env with secret is blocked in strict mode", + frontmatter: map[string]any{ + "steps": []any{ + map[string]any{ + "name": "Malformed env", + "env": "${{ secrets.TOKEN }}", + "run": "echo hi", + }, + }, + }, + strictMode: true, + expectError: true, + errorMsg: "strict mode: secrets expressions detected in 'steps' section", + }, + { + name: "malformed slice env with secret is blocked in strict mode", + frontmatter: map[string]any{ + "steps": []any{ + map[string]any{ + "name": "Malformed slice env", + "env": []any{ + "${{ secrets.ARRAY_TOKEN }}", + }, + "run": "echo hi", + }, + }, + }, + strictMode: true, + expectError: true, + errorMsg: "strict mode: secrets expressions detected in 'steps' section", + }, + { + name: "env-bound secret with GITHUB_ENV write in run is blocked in strict mode", + frontmatter: map[string]any{ + "steps": []any{ + map[string]any{ + "name": "Leaky step", + "env": map[string]any{ + "TOKEN": "${{ secrets.TOKEN }}", + }, + "run": `echo "TOKEN=${TOKEN}" >> "$GITHUB_ENV"`, + }, + }, + }, + strictMode: true, expectError: true, errorMsg: "strict mode: secrets expressions detected in 'steps' section", }, @@ -200,6 +320,137 @@ func TestValidateStepsSecrets(t *testing.T) { } } +func TestClassifyStepSecrets(t *testing.T) { + tests := []struct { + name string + step any + expectedUnsafe []string + expectedEnv []string + }{ + { + name: "non-map step classifies all as unsafe", + step: "echo ${{ secrets.TOKEN }}", + expectedUnsafe: []string{"${{ secrets.TOKEN }}"}, + expectedEnv: nil, + }, + { + name: "secret in run field is unsafe", + step: map[string]any{ + "name": "Run step", + "run": "echo ${{ secrets.API_KEY }}", + }, + expectedUnsafe: []string{"${{ secrets.API_KEY }}"}, + expectedEnv: nil, + }, + { + name: "secret in env field is classified as env", + step: map[string]any{ + "name": "Env step", + "env": map[string]any{ + "TOKEN": "${{ secrets.TOKEN }}", + }, + "run": "echo hi", + }, + expectedUnsafe: nil, + expectedEnv: []string{"${{ secrets.TOKEN }}"}, + }, + { + name: "secrets in both env and run are classified separately", + step: map[string]any{ + "name": "Mixed step", + "env": map[string]any{ + "SAFE": "${{ secrets.SAFE }}", + }, + "run": "curl ${{ secrets.LEAKED }}", + }, + expectedUnsafe: []string{"${{ secrets.LEAKED }}"}, + expectedEnv: []string{"${{ secrets.SAFE }}"}, + }, + { + name: "secret in with field is unsafe", + step: map[string]any{ + "uses": "some/action@v1", + "with": map[string]any{ + "token": "${{ secrets.MY_TOKEN }}", + }, + }, + expectedUnsafe: []string{"${{ secrets.MY_TOKEN }}"}, + expectedEnv: nil, + }, + { + name: "step with no secrets returns empty", + step: map[string]any{ + "name": "Plain step", + "run": "echo hello", + }, + expectedUnsafe: nil, + expectedEnv: nil, + }, + { + name: "secret in malformed string env is unsafe", + step: map[string]any{ + "name": "Malformed env step", + "env": "${{ secrets.TOKEN }}", + "run": "echo hi", + }, + expectedUnsafe: []string{"${{ secrets.TOKEN }}"}, + expectedEnv: nil, + }, + { + name: "secret in malformed slice env is unsafe", + step: map[string]any{ + "name": "Malformed env slice step", + "env": []any{ + "${{ secrets.ARRAY_TOKEN }}", + }, + "run": "echo hi", + }, + expectedUnsafe: []string{"${{ secrets.ARRAY_TOKEN }}"}, + expectedEnv: nil, + }, + { + name: "env-bound secret with GITHUB_ENV in run is reclassified as unsafe", + step: map[string]any{ + "name": "Leaky step", + "env": map[string]any{ + "TOKEN": "${{ secrets.TOKEN }}", + }, + "run": `echo "TOKEN=${TOKEN}" >> "$GITHUB_ENV"`, + }, + expectedUnsafe: []string{"${{ secrets.TOKEN }}"}, + expectedEnv: nil, + }, + { + name: "env-bound secret without GITHUB_ENV reference stays env-bound", + step: map[string]any{ + "name": "Safe step", + "env": map[string]any{ + "TOKEN": "${{ secrets.TOKEN }}", + }, + "run": "my-tool --authenticate", + }, + expectedUnsafe: nil, + expectedEnv: []string{"${{ secrets.TOKEN }}"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + unsafe, env := classifyStepSecrets(tt.step) + if len(tt.expectedUnsafe) == 0 { + assert.Empty(t, unsafe, "expected no unsafe secrets") + } else { + assert.Equal(t, tt.expectedUnsafe, unsafe, "unexpected unsafe secrets") + } + if len(tt.expectedEnv) == 0 { + assert.Empty(t, env, "expected no env secrets") + } else { + assert.Equal(t, tt.expectedEnv, env, "unexpected env secrets") + } + }) + } +} + func TestExtractSecretsFromStepValue(t *testing.T) { tests := []struct { name string @@ -298,3 +549,88 @@ func TestDeduplicateStringSlice(t *testing.T) { }) } } + +func TestFilterBuiltinTokens(t *testing.T) { + tests := []struct { + name string + input []string + expected []string + }{ + { + name: "GITHUB_TOKEN is filtered out", + input: []string{"${{ secrets.GITHUB_TOKEN }}"}, + expected: []string{}, + }, + { + name: "user secret is kept", + input: []string{"${{ secrets.API_KEY }}"}, + expected: []string{"${{ secrets.API_KEY }}"}, + }, + { + name: "GITHUB_TOKEN_SUFFIX is NOT filtered (precise match)", + input: []string{"${{ secrets.GITHUB_TOKEN_SUFFIX }}"}, + expected: []string{"${{ secrets.GITHUB_TOKEN_SUFFIX }}"}, + }, + { + name: "mixed expression with GITHUB_TOKEN and other secret is NOT filtered", + input: []string{"${{ secrets.GITHUB_TOKEN && secrets.OTHER }}"}, + expected: []string{"${{ secrets.GITHUB_TOKEN && secrets.OTHER }}"}, + }, + { + name: "expression with only GITHUB_TOKEN references is filtered", + input: []string{"${{ secrets.GITHUB_TOKEN || secrets.GITHUB_TOKEN }}"}, + expected: []string{}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := filterBuiltinTokens(tt.input) + if len(tt.expected) == 0 { + assert.Empty(t, result, "expected all to be filtered") + } else { + assert.Equal(t, tt.expected, result, "unexpected filter result") + } + }) + } +} + +func TestStepReferencesGitHubEnv(t *testing.T) { + tests := []struct { + name string + stepMap map[string]any + expected bool + }{ + { + name: "run with GITHUB_ENV reference", + stepMap: map[string]any{ + "run": `echo "KEY=val" >> "$GITHUB_ENV"`, + "env": map[string]any{"K": "v"}, + }, + expected: true, + }, + { + name: "run without GITHUB_ENV reference", + stepMap: map[string]any{ + "run": "my-tool scan", + "env": map[string]any{"K": "v"}, + }, + expected: false, + }, + { + name: "GITHUB_ENV in env field is ignored", + stepMap: map[string]any{ + "run": "my-tool scan", + "env": map[string]any{"GITHUB_ENV": "/tmp/env"}, + }, + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := stepReferencesGitHubEnv(tt.stepMap) + assert.Equal(t, tt.expected, result, "unexpected GITHUB_ENV detection result") + }) + } +}