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 index 609f23eac38..7f83f0dee0e 100644 --- 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 @@ -14,7 +14,7 @@ The workflow compiler enforces a "strict mode" that restricts what GitHub Action ### 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. +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, and `with:` inputs for `uses:` action steps, which are passed to external actions and masked by the runner) from "unsafe" inline interpolations (`run:` and all other step fields). In strict mode, only unsafe secret references will be treated as errors; secrets that appear exclusively in safe 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 @@ -33,19 +33,20 @@ A third option was to keep secrets blocked by default but allow authors to annot ### Consequences #### Positive -- Workflows that supply tool credentials via `env:` bindings no longer need to disable strict mode entirely, preserving all other strict-mode protections. +- Workflows that supply tool credentials via `env:` bindings or `with:` inputs (for `uses:` action steps) 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. +- The error message for blocked secrets now explicitly suggests `env:` bindings and `with:` inputs as alternatives, improving the developer experience. +- The behavior aligns with GitHub Actions' native masking guarantee: `env:` bindings and `with:` inputs are masked by the runner before command execution. +- Enterprise workflows that use centralized secret managers (e.g., Conjur, HashiCorp Vault) via dedicated GitHub Actions can now use strict mode, passing authentication credentials via `with:` inputs. #### 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 security policy is now more nuanced: reviewers must understand the `env:`/`with:` 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. +- Non-strict mode still emits a warning for all secrets (including safe-bound ones), which may be slightly misleading now that safe bindings are permitted 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. +- Existing workflows that used `strict: false` specifically to allow safe bindings can now remove that override and adopt strict mode, but this migration is voluntary. +- Unit and integration tests must now cover both the "safe-binding-only" allowed path and the "mixed safe + run" blocked path to maintain adequate coverage. --- @@ -56,18 +57,21 @@ A third option was to keep secrets blocked by default but allow authors to annot ### 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. +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 a *safe* 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). +4. A secret reference found inside a well-formed `with:` mapping in a step that also has a `uses:` field **MUST** be classified as a *safe* reference, because `with:` inputs are passed to the external action (not interpolated into shell scripts) and the runner masks values derived from secrets. +5. A secret reference found inside a `with:` mapping in a step that does NOT have a `uses:` field **MUST** be classified as an *unsafe* reference. +6. A secret reference found inside a malformed `with:` value (e.g., `with:` is a bare string or a YAML sequence) **MUST** be classified as an *unsafe* reference. +7. A secret reference found in any other step field (including but not limited to `run:`, `name:`, `if:`) **MUST** be classified as an *unsafe* reference. +8. A step value that is not a YAML map (e.g., a raw string) **MUST** treat all secret references within it as *unsafe* references. +9. When a step contains *safe* secret references AND any non-`env:`/non-`with:` field references `GITHUB_ENV`, implementations **MUST** reclassify all *safe* 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. +2. When strict mode is active, implementations **MUST NOT** return an error solely because *safe* 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 (for `run:` steps) or `with:` inputs (for `uses:` action steps) as alternatives to inline interpolation. +4. The built-in `GITHUB_TOKEN` **MUST** be filtered out from both *unsafe* and *safe* reference lists before strict-mode enforcement, as it is present in every runner environment by default. ### Non-Strict Mode Behavior @@ -77,7 +81,7 @@ A third option was to keep secrets blocked by default but allow authors to annot ### 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. +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 *safe* references in strict mode, are both non-conformant behaviors. --- diff --git a/pkg/workflow/strict_mode_steps_secrets_integration_test.go b/pkg/workflow/strict_mode_steps_secrets_integration_test.go index 3ee467c48a9..881d2dd3dfb 100644 --- a/pkg/workflow/strict_mode_steps_secrets_integration_test.go +++ b/pkg/workflow/strict_mode_steps_secrets_integration_test.go @@ -136,6 +136,50 @@ post-steps: # Post-Steps Env Secret Test Post-steps with secrets in env bindings. +`, + }, + { + name: "secrets in with for uses action step compile in strict mode", + content: `--- +on: workflow_dispatch +permissions: + contents: read + issues: read + pull-requests: read +engine: copilot +steps: + - uses: my-org/secrets-action@v2 + with: + username: ${{ secrets.VAULT_USERNAME }} + password: ${{ secrets.VAULT_PASSWORD }} + secret_map: static-value +--- + +# With Secrets in Uses Action Test + +Action steps with secrets in with: inputs should compile in strict mode. +`, + }, + { + name: "mixed env and with secrets for uses action step compile in strict mode", + content: `--- +on: workflow_dispatch +permissions: + contents: read + issues: read + pull-requests: read +engine: copilot +steps: + - uses: my-org/secrets-action@v2 + env: + EXTRA_TOKEN: ${{ secrets.EXTRA_TOKEN }} + with: + username: ${{ secrets.VAULT_USERNAME }} +--- + +# Mixed Env and With Secrets Test + +Both env: and with: secrets in a uses: action step should compile. `, }, } @@ -165,8 +209,8 @@ Post-steps with secrets in env bindings. } // TestStrictModeStepUnsafeSecretsBlocked tests that secrets in non-env step -// fields (run, with, etc.) are still blocked in strict mode during full -// compilation. +// fields (run, etc.) are still blocked in strict mode during full compilation. +// Note: secrets in with: for uses: action steps are now allowed (safe binding). func TestStrictModeStepUnsafeSecretsBlocked(t *testing.T) { tests := []struct { name string @@ -194,7 +238,7 @@ 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", + name: "secret in with field without uses is blocked in strict mode", content: `--- on: workflow_dispatch permissions: @@ -203,14 +247,15 @@ permissions: pull-requests: read engine: copilot steps: - - uses: some/action@v1 + - name: Step with with but no uses with: token: ${{ secrets.MY_API_TOKEN }} + run: echo hi --- -# Unsafe With Secret Test +# Unsafe With Secret Test (no uses) -This should fail because secrets are used in with field. +This should fail because with: without uses: is not a safe binding. `, errorMsg: "strict mode: secrets expressions detected in 'steps' section", }, @@ -295,6 +340,9 @@ Check that error suggests env bindings. if !strings.Contains(err.Error(), "env: bindings") { t.Errorf("Expected error to suggest env: bindings, got: %v", err) } + if !strings.Contains(err.Error(), "with: inputs") { + t.Errorf("Expected error to suggest with: inputs for action steps, got: %v", err) + } } // TestNonStrictModeStepSecretsAllowedWithWarning verifies that in non-strict diff --git a/pkg/workflow/strict_mode_steps_validation.go b/pkg/workflow/strict_mode_steps_validation.go index f579fcd43a8..062b32121e0 100644 --- a/pkg/workflow/strict_mode_steps_validation.go +++ b/pkg/workflow/strict_mode_steps_validation.go @@ -2,9 +2,9 @@ // // It validates that secrets expressions are not used in custom steps (steps and // 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. +// env: bindings and with: inputs for uses: action steps are allowed (controlled +// binding, masked by GitHub Actions), while secrets in other fields (run, 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 @@ -27,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, secrets in step-level env: bindings are allowed (controlled, -// masked binding), while secrets in other fields (run, with, etc.) are errors. +// In strict mode, secrets in step-level env: bindings and with: inputs for +// uses: action steps are allowed (controlled, masked binding), while secrets +// in other fields (run, 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"} { @@ -42,9 +43,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. +// In strict mode, secrets in step-level env: bindings and with: inputs for +// uses: action steps are allowed because they are controlled bindings that are +// automatically masked by GitHub Actions. Secrets in other step fields (run, +// etc.) are still treated as errors. func (c *Compiler) validateStepsSectionSecrets(frontmatter map[string]any, sectionName string) error { rawValue, exists := frontmatter[sectionName] if !exists { @@ -58,37 +60,37 @@ func (c *Compiler) validateStepsSectionSecrets(frontmatter map[string]any, secti return nil } - // Separate secrets found in step-level env: bindings (safe, controlled) - // from secrets found in other fields (unsafe, potential leak). + // Separate secrets found in safe bindings (env: maps, with: maps in uses: + // action steps) from secrets found in other fields (unsafe, potential leak). var unsafeSecretRefs []string - var envSecretRefs []string + var safeSecretRefs []string for _, step := range steps { - unsafe, envOnly := classifyStepSecrets(step) + unsafe, safe := classifyStepSecrets(step) unsafeSecretRefs = append(unsafeSecretRefs, unsafe...) - envSecretRefs = append(envSecretRefs, envOnly...) + safeSecretRefs = append(safeSecretRefs, safe...) } // 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. unsafeSecretRefs = filterBuiltinTokens(unsafeSecretRefs) - envSecretRefs = filterBuiltinTokens(envSecretRefs) + safeSecretRefs = filterBuiltinTokens(safeSecretRefs) - allSecretRefs := append(unsafeSecretRefs, envSecretRefs...) + allSecretRefs := append(unsafeSecretRefs, safeSecretRefs...) 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: %d unsafe, %d in env bindings", - len(allSecretRefs), sectionName, len(unsafeSecretRefs), len(envSecretRefs)) + strictModeValidationLog.Printf("Found %d secret expression(s) in %s section: %d unsafe, %d in safe bindings", + len(allSecretRefs), sectionName, len(unsafeSecretRefs), len(safeSecretRefs)) 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.). + // In strict mode, secrets in step-level env: bindings and with: inputs + // for uses: action steps are allowed (controlled binding, masked by + // GitHub Actions). Only block secrets found in other fields (run, etc.). if len(unsafeSecretRefs) == 0 { - strictModeValidationLog.Printf("All secrets in %s section are in env bindings (allowed in strict mode)", sectionName) + strictModeValidationLog.Printf("All secrets in %s section are in safe bindings (allowed in strict mode)", sectionName) return nil } @@ -97,7 +99,7 @@ func (c *Compiler) validateStepsSectionSecrets(frontmatter map[string]any, secti 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, "+ - "or use step-level env: bindings instead", + "or use step-level env: bindings (for run: steps) or with: inputs (for uses: action steps) instead", sectionName, strings.Join(unsafeSecretRefs, ", "), ) } @@ -122,48 +124,68 @@ func (c *Compiler) validateStepsSectionSecrets(frontmatter map[string]any, secti 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) +// - unsafeRefs: secrets found in fields other than "env" or "with" (for uses: +// action steps), or secrets in env:/with: bindings when the step also writes +// to $GITHUB_ENV +// - safeRefs: secrets found in step-level env: bindings (controlled, masked), +// or in with: inputs for uses: action steps (passed to external actions, +// masked by the runner) // -// 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 }}"`. +// Only secrets in well-formed mappings (map[string]any) are classified as safe. +// Malformed 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 +// safe-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) { +func classifyStepSecrets(step any) (unsafeRefs, safeRefs []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 + // Check if this is a uses: action step. For action steps, with: inputs are + // passed to the external action (not interpolated into shell scripts), and + // the GitHub Actions runner masks with: values derived from secrets. + // Only treat with: as safe when uses is a valid non-empty string reference. + usesVal, hasUses := stepMap["uses"] + if hasUses { + usesStr, isString := usesVal.(string) + hasUses = isString && strings.TrimSpace(usesStr) != "" + } + + var localUnsafe, localSafe []string for key, val := range stepMap { refs := extractSecretsFromStepValue(val) if key == "env" { if _, isMap := val.(map[string]any); isMap { - localEnv = append(localEnv, refs...) + localSafe = append(localSafe, refs...) } else { // Malformed env (string, slice, etc.): treat as unsafe. localUnsafe = append(localUnsafe, refs...) } + } else if key == "with" && hasUses { + if _, isMap := val.(map[string]any); isMap { + localSafe = append(localSafe, refs...) + } else { + // Malformed with (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 + // If the step has safe-bound secrets AND references $GITHUB_ENV in any + // non-env/non-with field, reclassify all safe 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 + if len(localSafe) > 0 && stepReferencesGitHubEnv(stepMap) { + localUnsafe = append(localUnsafe, localSafe...) + localSafe = nil } - return localUnsafe, localEnv + return localUnsafe, localSafe } // extractSecretsFromStepValue recursively walks a step value (which may be a map, @@ -215,11 +237,13 @@ func filterBuiltinTokens(refs []string) []string { 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). +// stepReferencesGitHubEnv returns true if any non-env, non-with field in the +// step map contains a reference to GITHUB_ENV (e.g. in a run: command that +// writes to it). Both env: and with: are safe binding surfaces, so their +// values are excluded from GITHUB_ENV leak detection. func stepReferencesGitHubEnv(stepMap map[string]any) bool { for key, val := range stepMap { - if key == "env" { + if key == "env" || key == "with" { continue } if valueReferencesGitHubEnv(val) { diff --git a/pkg/workflow/strict_mode_steps_validation_test.go b/pkg/workflow/strict_mode_steps_validation_test.go index a070a5231df..af329cbf4d8 100644 --- a/pkg/workflow/strict_mode_steps_validation_test.go +++ b/pkg/workflow/strict_mode_steps_validation_test.go @@ -99,7 +99,7 @@ func TestValidateStepsSecrets(t *testing.T) { expectError: false, }, { - name: "steps with secret in with field in strict mode fails", + name: "steps with secret in with field for uses action step in strict mode is allowed", frontmatter: map[string]any{ "steps": []any{ map[string]any{ @@ -111,9 +111,41 @@ func TestValidateStepsSecrets(t *testing.T) { }, }, strictMode: true, + expectError: false, + }, + { + name: "steps with secret in with field without uses in strict mode fails", + frontmatter: map[string]any{ + "steps": []any{ + map[string]any{ + "name": "Step without uses", + "with": map[string]any{ + "token": "${{ secrets.MY_API_TOKEN }}", + }, + }, + }, + }, + strictMode: true, expectError: true, errorMsg: "strict mode: secrets expressions detected in 'steps' section", }, + { + name: "vault-style action with multiple secrets in with is allowed in strict mode", + frontmatter: map[string]any{ + "steps": []any{ + map[string]any{ + "uses": "my-org/secrets-action@v2", + "with": map[string]any{ + "username": "${{ secrets.VAULT_USERNAME }}", + "password": "${{ secrets.VAULT_PASSWORD }}", + "secret_map": "${{ inputs.secret_map }}", + }, + }, + }, + }, + strictMode: true, + expectError: false, + }, { name: "post-steps with secret in strict mode fails", frontmatter: map[string]any{ @@ -300,6 +332,50 @@ func TestValidateStepsSecrets(t *testing.T) { expectError: true, errorMsg: "strict mode: secrets expressions detected in 'steps' section", }, + { + name: "pre-steps with secret in with for uses action step is allowed in strict mode", + frontmatter: map[string]any{ + "pre-steps": []any{ + map[string]any{ + "uses": "my-org/vault-action@v1", + "with": map[string]any{ + "token": "${{ secrets.VAULT_TOKEN }}", + }, + }, + }, + }, + strictMode: true, + expectError: false, + }, + { + name: "post-steps with secret in with for uses action step is allowed in strict mode", + frontmatter: map[string]any{ + "post-steps": []any{ + map[string]any{ + "uses": "my-org/notify-action@v1", + "with": map[string]any{ + "webhook": "${{ secrets.SLACK_WEBHOOK }}", + }, + }, + }, + }, + strictMode: true, + expectError: false, + }, + { + name: "error message suggests with: inputs for uses: action steps", + frontmatter: map[string]any{ + "steps": []any{ + map[string]any{ + "name": "Leaky step", + "run": "echo ${{ secrets.TOKEN }}", + }, + }, + }, + strictMode: true, + expectError: true, + errorMsg: "with: inputs (for uses: action steps)", + }, } for _, tt := range tests { @@ -322,16 +398,17 @@ func TestValidateStepsSecrets(t *testing.T) { func TestClassifyStepSecrets(t *testing.T) { tests := []struct { - name string - step any - expectedUnsafe []string - expectedEnv []string + name string + step any + expectedUnsafe []string + expectedSafe []string + unorderedSafeMatch bool // use ElementsMatch instead of Equal for safe refs }{ { name: "non-map step classifies all as unsafe", step: "echo ${{ secrets.TOKEN }}", expectedUnsafe: []string{"${{ secrets.TOKEN }}"}, - expectedEnv: nil, + expectedSafe: nil, }, { name: "secret in run field is unsafe", @@ -340,10 +417,10 @@ func TestClassifyStepSecrets(t *testing.T) { "run": "echo ${{ secrets.API_KEY }}", }, expectedUnsafe: []string{"${{ secrets.API_KEY }}"}, - expectedEnv: nil, + expectedSafe: nil, }, { - name: "secret in env field is classified as env", + name: "secret in env field is classified as safe", step: map[string]any{ "name": "Env step", "env": map[string]any{ @@ -352,7 +429,7 @@ func TestClassifyStepSecrets(t *testing.T) { "run": "echo hi", }, expectedUnsafe: nil, - expectedEnv: []string{"${{ secrets.TOKEN }}"}, + expectedSafe: []string{"${{ secrets.TOKEN }}"}, }, { name: "secrets in both env and run are classified separately", @@ -364,18 +441,122 @@ func TestClassifyStepSecrets(t *testing.T) { "run": "curl ${{ secrets.LEAKED }}", }, expectedUnsafe: []string{"${{ secrets.LEAKED }}"}, - expectedEnv: []string{"${{ secrets.SAFE }}"}, + expectedSafe: []string{"${{ secrets.SAFE }}"}, }, { - name: "secret in with field is unsafe", + name: "secret in with field for uses action step is classified as safe", step: map[string]any{ "uses": "some/action@v1", "with": map[string]any{ "token": "${{ secrets.MY_TOKEN }}", }, }, + expectedUnsafe: nil, + expectedSafe: []string{"${{ secrets.MY_TOKEN }}"}, + }, + { + name: "secret in with field without uses is unsafe", + step: map[string]any{ + "name": "Step without uses", + "with": map[string]any{ + "token": "${{ secrets.MY_TOKEN }}", + }, + }, + expectedUnsafe: []string{"${{ secrets.MY_TOKEN }}"}, + expectedSafe: nil, + }, + { + name: "secret in with field with nil uses is unsafe", + step: map[string]any{ + "uses": nil, + "with": map[string]any{ + "token": "${{ secrets.MY_TOKEN }}", + }, + }, + expectedUnsafe: []string{"${{ secrets.MY_TOKEN }}"}, + expectedSafe: nil, + }, + { + name: "secret in with field with empty uses is unsafe", + step: map[string]any{ + "uses": "", + "with": map[string]any{ + "token": "${{ secrets.MY_TOKEN }}", + }, + }, + expectedUnsafe: []string{"${{ secrets.MY_TOKEN }}"}, + expectedSafe: nil, + }, + { + name: "secret in with field with whitespace-only uses is unsafe", + step: map[string]any{ + "uses": " ", + "with": map[string]any{ + "token": "${{ secrets.MY_TOKEN }}", + }, + }, + expectedUnsafe: []string{"${{ secrets.MY_TOKEN }}"}, + expectedSafe: nil, + }, + { + name: "secret in with field with non-string uses is unsafe", + step: map[string]any{ + "uses": 42, + "with": map[string]any{ + "token": "${{ secrets.MY_TOKEN }}", + }, + }, expectedUnsafe: []string{"${{ secrets.MY_TOKEN }}"}, - expectedEnv: nil, + expectedSafe: nil, + }, + { + name: "malformed string with in uses action step is unsafe", + step: map[string]any{ + "uses": "some/action@v1", + "with": "${{ secrets.TOKEN }}", + }, + expectedUnsafe: []string{"${{ secrets.TOKEN }}"}, + expectedSafe: nil, + }, + { + name: "malformed slice with in uses action step is unsafe", + step: map[string]any{ + "uses": "some/action@v1", + "with": []any{ + "${{ secrets.ARRAY_TOKEN }}", + }, + }, + expectedUnsafe: []string{"${{ secrets.ARRAY_TOKEN }}"}, + expectedSafe: nil, + }, + { + name: "multiple secrets in with for uses action step are safe", + step: map[string]any{ + "uses": "my-org/secrets-action@v2", + "with": map[string]any{ + "username": "${{ secrets.VAULT_USERNAME }}", + "password": "${{ secrets.VAULT_PASSWORD }}", + "secret_map": "static-value", + }, + }, + expectedUnsafe: nil, + expectedSafe: []string{"${{ secrets.VAULT_USERNAME }}", "${{ secrets.VAULT_PASSWORD }}"}, + unorderedSafeMatch: true, + }, + { + name: "secrets in both env and with for uses action step are safe", + step: map[string]any{ + "uses": "some/action@v1", + "env": map[string]any{ + "SAFE_ENV": "${{ secrets.ENV_SECRET }}", + }, + "with": map[string]any{ + "token": "${{ secrets.WITH_SECRET }}", + }, + }, + expectedUnsafe: nil, + expectedSafe: []string{"${{ secrets.ENV_SECRET }}", "${{ secrets.WITH_SECRET }}"}, + unorderedSafeMatch: true, }, { name: "step with no secrets returns empty", @@ -384,7 +565,7 @@ func TestClassifyStepSecrets(t *testing.T) { "run": "echo hello", }, expectedUnsafe: nil, - expectedEnv: nil, + expectedSafe: nil, }, { name: "secret in malformed string env is unsafe", @@ -394,7 +575,7 @@ func TestClassifyStepSecrets(t *testing.T) { "run": "echo hi", }, expectedUnsafe: []string{"${{ secrets.TOKEN }}"}, - expectedEnv: nil, + expectedSafe: nil, }, { name: "secret in malformed slice env is unsafe", @@ -406,7 +587,7 @@ func TestClassifyStepSecrets(t *testing.T) { "run": "echo hi", }, expectedUnsafe: []string{"${{ secrets.ARRAY_TOKEN }}"}, - expectedEnv: nil, + expectedSafe: nil, }, { name: "env-bound secret with GITHUB_ENV in run is reclassified as unsafe", @@ -418,10 +599,10 @@ func TestClassifyStepSecrets(t *testing.T) { "run": `echo "TOKEN=${TOKEN}" >> "$GITHUB_ENV"`, }, expectedUnsafe: []string{"${{ secrets.TOKEN }}"}, - expectedEnv: nil, + expectedSafe: nil, }, { - name: "env-bound secret without GITHUB_ENV reference stays env-bound", + name: "env-bound secret without GITHUB_ENV reference stays safe", step: map[string]any{ "name": "Safe step", "env": map[string]any{ @@ -430,22 +611,36 @@ func TestClassifyStepSecrets(t *testing.T) { "run": "my-tool --authenticate", }, expectedUnsafe: nil, - expectedEnv: []string{"${{ secrets.TOKEN }}"}, + expectedSafe: []string{"${{ secrets.TOKEN }}"}, + }, + { + name: "GITHUB_ENV string in with field does not trigger reclassification", + step: map[string]any{ + "uses": "some/action@v1", + "with": map[string]any{ + "path": "GITHUB_ENV", + "token": "${{ secrets.TOKEN }}", + }, + }, + expectedUnsafe: nil, + expectedSafe: []string{"${{ secrets.TOKEN }}"}, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - unsafe, env := classifyStepSecrets(tt.step) + unsafe, safe := 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") + if len(tt.expectedSafe) == 0 { + assert.Empty(t, safe, "expected no safe secrets") + } else if tt.unorderedSafeMatch { + assert.ElementsMatch(t, tt.expectedSafe, safe, "unexpected safe secrets") } else { - assert.Equal(t, tt.expectedEnv, env, "unexpected env secrets") + assert.Equal(t, tt.expectedSafe, safe, "unexpected safe secrets") } }) } @@ -625,6 +820,14 @@ func TestStepReferencesGitHubEnv(t *testing.T) { }, expected: false, }, + { + name: "GITHUB_ENV in with field is ignored", + stepMap: map[string]any{ + "run": "my-tool scan", + "with": map[string]any{"path": "GITHUB_ENV"}, + }, + expected: false, + }, } for _, tt := range tests { diff --git a/scratchpad/dev.md b/scratchpad/dev.md index a607d1e8480..89afd5ee016 100644 --- a/scratchpad/dev.md +++ b/scratchpad/dev.md @@ -444,12 +444,16 @@ func ValidatePermissions(perms map[string]string) error { ### Secrets in Custom Steps Validation -The compiler validates that `secrets.*` expressions are not used in `steps` and `post-steps` frontmatter sections (introduced in PR #24450). +The compiler validates that `secrets.*` expressions are not used unsafely in `steps` and `post-steps` frontmatter sections (introduced in PR #24450). -**Purpose**: Minimize secrets exposed to the agent job. The only secrets that should appear in the agent job are those required to configure the agentic engine itself. Steps or post-steps that need secrets should be moved to a separate GitHub Actions job outside the agent job. +**Purpose**: Minimize secrets exposed to the agent job. The only secrets that should appear in the agent job are those required to configure the agentic engine itself. + +**Safe bindings** (allowed in strict mode): +- Step-level `env:` bindings (controlled, masked by GitHub Actions) +- `with:` inputs for `uses:` action steps (passed to external actions, masked by the runner) **Behavior**: -- **Strict mode** (`--strict`): compilation fails with an error listing the found expressions +- **Strict mode** (`--strict`): compilation fails with an error for secrets in unsafe fields (e.g., `run:`); secrets in `env:` and `with:` (for `uses:` action steps) are allowed - **Non-strict mode**: a warning is emitted and the warning counter is incremented - `${{ secrets.GITHUB_TOKEN }}` is exempt — it is the built-in runner token, automatically available in every runner environment, and not a user-defined secret @@ -459,18 +463,31 @@ The compiler validates that `secrets.*` expressions are not used in `steps` and ``` strict mode: secrets expressions detected in 'steps' section may be leaked to the agent job. Found: ${{ secrets.MY_SECRET }}. -Operations requiring secrets must be moved to a separate job outside the agent job +Operations requiring secrets must be moved to a separate job outside the agent job, +or use step-level env: bindings (for run: steps) or with: inputs (for uses: action steps) instead ``` -**Migration**: +**Examples**: ```yaml -# ❌ Avoid: secret in custom step leaks into agent job +# ❌ Avoid: secret in run: field leaks into agent job +steps: + - name: Deploy + run: curl -H "Authorization: ${{ secrets.DEPLOY_KEY }}" https://example.com + +# ✅ Correct: secret in env: binding (safe, masked) steps: - name: Deploy env: API_KEY: ${{ secrets.DEPLOY_KEY }} run: ./deploy.sh +# ✅ Correct: secret in with: for uses: action step (safe, masked) +steps: + - uses: my-org/secrets-action@v2 + with: + username: ${{ secrets.VAULT_USERNAME }} + password: ${{ secrets.VAULT_PASSWORD }} + # ✅ Correct: secrets in a separate job outside the agent job jobs: deploy: