diff --git a/docs/adr/0001-allow-secrets-in-step-env-bindings-under-strict-mode.md b/docs/adr/0001-allow-secrets-in-step-env-bindings-under-strict-mode.md new file mode 100644 index 0000000000..4ce83c9f62 --- /dev/null +++ b/docs/adr/0001-allow-secrets-in-step-env-bindings-under-strict-mode.md @@ -0,0 +1,83 @@ +# ADR-0001: 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 the `env:` field of a step **MUST** be classified as an *env-bound* reference. +3. 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. +4. A step value that is not a YAML map (e.g., a raw string) **MUST** treat all secret references within it as *unsafe* references. +5. Implementations **MUST NOT** treat an *env-bound* reference as *unsafe*, nor vice versa. + +### 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 0000000000..6791c2a57d --- /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: Export secret + env: + SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }} + run: echo "SONAR_TOKEN=${SONAR_TOKEN}" >> "$GITHUB_ENV" +--- + +# 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: Export scan credentials + env: + SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }} + CORONA_TOKEN: ${{ secrets.CORONA_TOKEN }} + run: | + echo "SONAR_TOKEN=${SONAR_TOKEN}" >> "$GITHUB_ENV" + echo "CORONA_TOKEN=${CORONA_TOKEN}" >> "$GITHUB_ENV" + - name: Export auth credentials + env: + CIAM_CLIENT_ID: ${{ secrets.CIAM_CLIENT_ID }} + CIAM_CLIENT_SECRET: ${{ secrets.CIAM_CLIENT_SECRET }} + SI_TOKEN: ${{ secrets.SI_TOKEN }} + run: | + echo "CIAM_CLIENT_ID=${CIAM_CLIENT_ID}" >> "$GITHUB_ENV" + echo "SI_TOKEN=${SI_TOKEN}" >> "$GITHUB_ENV" +--- + +# 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: Setup tokens + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + API_KEY: ${{ secrets.API_KEY }} + run: echo "API_KEY=${API_KEY}" >> "$GITHUB_ENV" +--- + +# 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: Export tool credentials + env: + SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }} + run: echo "SONAR_TOKEN=${SONAR_TOKEN}" >> "$GITHUB_ENV" +--- + +# 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 d3fb14c7f6..1c63fcafd2 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 @@ -22,8 +24,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 +38,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 +55,55 @@ 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) + + allSecretRefs := append(unsafeSecretRefs, envSecretRefs...) - if len(secretRefs) == 0 { + 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) 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) 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 +111,35 @@ func (c *Compiler) validateStepsSectionSecrets(frontmatter map[string]any, secti return nil } +// classifyStepSecrets separates secrets found in a step into two categories: +// - unsafeRefs: secrets found in fields other than "env" (e.g. run, with) +// - 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 }}"`. +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 + } + for key, val := range stepMap { + refs := extractSecretsFromStepValue(val) + if key == "env" { + if _, isMap := val.(map[string]any); isMap { + envRefs = append(envRefs, refs...) + } else { + // Malformed env (string, slice, etc.): treat as unsafe. + unsafeRefs = append(unsafeRefs, refs...) + } + } else { + unsafeRefs = append(unsafeRefs, refs...) + } + } + return unsafeRefs, envRefs +} + // 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 { diff --git a/pkg/workflow/strict_mode_steps_validation_test.go b/pkg/workflow/strict_mode_steps_validation_test.go index ba13d4b6ad..5359c6ca83 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,110 @@ 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": "Export creds", + "env": map[string]any{ + "CIAM_CLIENT_ID": "${{ secrets.CIAM_CLIENT_ID }}", + }, + "run": "echo CIAM_CLIENT_ID=${CIAM_CLIENT_ID} >> $GITHUB_ENV", + }, + }, + }, + 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", }, @@ -200,6 +303,113 @@ 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, + }, + } + + 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