diff --git a/.github/workflows/smoke-test-tools.lock.yml b/.github/workflows/smoke-test-tools.lock.yml index 42eb07c0928..b3686f4e6dd 100644 --- a/.github/workflows/smoke-test-tools.lock.yml +++ b/.github/workflows/smoke-test-tools.lock.yml @@ -671,7 +671,7 @@ jobs: timeout-minutes: 5 run: | set -o pipefail - sudo -E awf --env-all --container-workdir "${GITHUB_WORKSPACE}" --allow-domains '*.githubusercontent.com,*.jsr.io,*.pythonhosted.org,adoptium.net,anaconda.org,api.adoptium.net,api.business.githubcopilot.com,api.enterprise.githubcopilot.com,api.foojay.io,api.github.com,api.githubcopilot.com,api.individual.githubcopilot.com,api.npms.io,api.nuget.org,api.snapcraft.io,archive.apache.org,archive.ubuntu.com,azure.archive.ubuntu.com,azuresearch-usnc.nuget.org,azuresearch-ussc.nuget.org,binstar.org,bootstrap.pypa.io,builds.dotnet.microsoft.com,bun.sh,cdn.azul.com,cdn.jsdelivr.net,central.sonatype.com,ci.dot.net,codeload.github.com,conda.anaconda.org,conda.binstar.org,crates.io,crl.geotrust.com,crl.globalsign.com,crl.identrust.com,crl.sectigo.com,crl.thawte.com,crl.usertrust.com,crl.verisign.com,crl3.digicert.com,crl4.digicert.com,crls.ssl.com,dc.services.visualstudio.com,deb.nodesource.com,deno.land,dist.nuget.org,dl.google.com,dlcdn.apache.org,dot.net,dotnet.microsoft.com,dotnetcli.blob.core.windows.net,download.eclipse.org,download.java.net,download.oracle.com,esm.sh,files.pythonhosted.org,get.pnpm.io,github-cloud.githubusercontent.com,github-cloud.s3.amazonaws.com,github.com,github.githubassets.com,go.dev,golang.org,googleapis.deno.dev,googlechromelabs.github.io,goproxy.io,gradle.org,host.docker.internal,index.crates.io,jcenter.bintray.com,jdk.java.net,json-schema.org,json.schemastore.org,jsr.io,keyserver.ubuntu.com,lfs.github.com,maven.apache.org,maven.google.com,maven.oracle.com,maven.pkg.github.com,nodejs.org,npm.pkg.github.com,npmjs.com,npmjs.org,nuget.org,nuget.pkg.github.com,nugetregistryv2prod.blob.core.windows.net,objects.githubusercontent.com,ocsp.digicert.com,ocsp.geotrust.com,ocsp.globalsign.com,ocsp.identrust.com,ocsp.sectigo.com,ocsp.ssl.com,ocsp.thawte.com,ocsp.usertrust.com,ocsp.verisign.com,oneocsp.microsoft.com,packagecloud.io,packages.cloud.google.com,packages.microsoft.com,pip.pypa.io,pkg.go.dev,pkgs.dev.azure.com,plugins-artifacts.gradle.org,plugins.gradle.org,ppa.launchpad.net,proxy.golang.org,pypi.org,pypi.python.org,raw.githubusercontent.com,registry.bower.io,registry.npmjs.com,registry.npmjs.org,registry.yarnpkg.com,repo.anaconda.com,repo.continuum.io,repo.gradle.org,repo.grails.org,repo.maven.apache.org,repo.spring.io,repo.yarnpkg.com,repo1.maven.org,s.symcb.com,s.symcd.com,security.ubuntu.com,services.gradle.org,skimdb.npmjs.com,static.crates.io,storage.googleapis.com,sum.golang.org,telemetry.enterprise.githubcopilot.com,ts-crl.ws.symantec.com,ts-ocsp.ws.symantec.com,www.java.com,www.microsoft.com,www.npmjs.com,www.npmjs.org,yarnpkg.com' --log-level info --proxy-logs-dir /tmp/gh-aw/sandbox/firewall/logs --enable-host-access --image-tag 0.20.2 --skip-pull --enable-api-proxy \ + sudo -E awf --env-all --container-workdir "${GITHUB_WORKSPACE}" --allow-domains '*.githubusercontent.com,*.jsr.io,*.pythonhosted.org,adoptium.net,anaconda.org,api.adoptium.net,api.business.githubcopilot.com,api.enterprise.githubcopilot.com,api.foojay.io,api.github.com,api.githubcopilot.com,api.individual.githubcopilot.com,api.npms.io,api.nuget.org,api.snapcraft.io,archive.apache.org,archive.ubuntu.com,azure.archive.ubuntu.com,azuresearch-usnc.nuget.org,azuresearch-ussc.nuget.org,binstar.org,bootstrap.pypa.io,builds.dotnet.microsoft.com,bun.sh,cdn.azul.com,cdn.jsdelivr.net,central.sonatype.com,ci.dot.net,codeload.github.com,conda.anaconda.org,conda.binstar.org,crates.io,crl.geotrust.com,crl.globalsign.com,crl.identrust.com,crl.sectigo.com,crl.thawte.com,crl.usertrust.com,crl.verisign.com,crl3.digicert.com,crl4.digicert.com,crls.ssl.com,dc.services.visualstudio.com,deb.nodesource.com,deno.land,dist.nuget.org,dl.google.com,dlcdn.apache.org,dot.net,dotnet.microsoft.com,dotnetcli.blob.core.windows.net,download.eclipse.org,download.java.net,download.oracle.com,downloads.gradle-dn.com,esm.sh,files.pythonhosted.org,get.pnpm.io,github-cloud.githubusercontent.com,github-cloud.s3.amazonaws.com,github.com,github.githubassets.com,go.dev,golang.org,googleapis.deno.dev,googlechromelabs.github.io,goproxy.io,gradle.org,host.docker.internal,index.crates.io,jcenter.bintray.com,jdk.java.net,json-schema.org,json.schemastore.org,jsr.io,keyserver.ubuntu.com,lfs.github.com,maven.apache.org,maven.google.com,maven.oracle.com,maven.pkg.github.com,nodejs.org,npm.pkg.github.com,npmjs.com,npmjs.org,nuget.org,nuget.pkg.github.com,nugetregistryv2prod.blob.core.windows.net,objects.githubusercontent.com,ocsp.digicert.com,ocsp.geotrust.com,ocsp.globalsign.com,ocsp.identrust.com,ocsp.sectigo.com,ocsp.ssl.com,ocsp.thawte.com,ocsp.usertrust.com,ocsp.verisign.com,oneocsp.microsoft.com,packagecloud.io,packages.cloud.google.com,packages.microsoft.com,pip.pypa.io,pkg.go.dev,pkgs.dev.azure.com,plugins-artifacts.gradle.org,plugins.gradle.org,ppa.launchpad.net,proxy.golang.org,pypi.org,pypi.python.org,raw.githubusercontent.com,registry.bower.io,registry.npmjs.com,registry.npmjs.org,registry.yarnpkg.com,repo.anaconda.com,repo.continuum.io,repo.gradle.org,repo.grails.org,repo.maven.apache.org,repo.spring.io,repo.yarnpkg.com,repo1.maven.org,s.symcb.com,s.symcd.com,security.ubuntu.com,services.gradle.org,skimdb.npmjs.com,static.crates.io,storage.googleapis.com,sum.golang.org,telemetry.enterprise.githubcopilot.com,ts-crl.ws.symantec.com,ts-ocsp.ws.symantec.com,www.java.com,www.microsoft.com,www.npmjs.com,www.npmjs.org,yarnpkg.com' --log-level info --proxy-logs-dir /tmp/gh-aw/sandbox/firewall/logs --enable-host-access --image-tag 0.20.2 --skip-pull --enable-api-proxy \ -- /bin/bash -c '/usr/local/bin/copilot --add-dir /tmp/gh-aw/ --log-level all --log-dir /tmp/gh-aw/sandbox/agent/logs/ --add-dir "${GITHUB_WORKSPACE}" --disable-builtin-mcps --allow-all-tools --allow-all-paths --share /tmp/gh-aw/sandbox/agent/logs/conversation.md --prompt "$(cat /tmp/gh-aw/aw-prompts/prompt.txt)"${GH_AW_MODEL_AGENT_COPILOT:+ --model "$GH_AW_MODEL_AGENT_COPILOT"}' 2>&1 | tee -a /tmp/gh-aw/agent-stdio.log env: COPILOT_AGENT_RUNNER_TYPE: STANDALONE diff --git a/pkg/workflow/checkout_runtime_order_test.go b/pkg/workflow/checkout_runtime_order_test.go index 011bcfcf077..8960f38ff82 100644 --- a/pkg/workflow/checkout_runtime_order_test.go +++ b/pkg/workflow/checkout_runtime_order_test.go @@ -27,6 +27,8 @@ engine: copilot steps: - name: Checkout code uses: actions/checkout@v5 + with: + persist-credentials: false - name: Use Node run: node --version --- diff --git a/pkg/workflow/compiler_orchestrator_engine.go b/pkg/workflow/compiler_orchestrator_engine.go index 2386e086bf4..5021bdb11ad 100644 --- a/pkg/workflow/compiler_orchestrator_engine.go +++ b/pkg/workflow/compiler_orchestrator_engine.go @@ -277,6 +277,15 @@ func (c *Compiler) setupEngineAndImports(result *parser.FrontmatterResult, clean return nil, err } + // Validate that actions/checkout steps in the agent job include persist-credentials: false + orchestratorEngineLog.Printf("Validating checkout persist-credentials (strict=%v)", c.strictMode) + if err := c.validateCheckoutPersistCredentials(result.Frontmatter, importsResult.MergedSteps); err != nil { + orchestratorEngineLog.Printf("Checkout persist-credentials validation failed: %v", err) + // Restore strict mode before returning error + c.strictMode = initialStrictModeForFirewall + return nil, err + } + // Restore the strict mode state after network check c.strictMode = initialStrictModeForFirewall diff --git a/pkg/workflow/compiler_orchestrator_workflow_test.go b/pkg/workflow/compiler_orchestrator_workflow_test.go index 26d57d057e9..df53d735ca9 100644 --- a/pkg/workflow/compiler_orchestrator_workflow_test.go +++ b/pkg/workflow/compiler_orchestrator_workflow_test.go @@ -1251,6 +1251,8 @@ engine: copilot steps: - uses: actions/checkout@v3 name: Checkout + with: + persist-credentials: false --- # Test Workflow diff --git a/pkg/workflow/github_folder_checkout_optimization_test.go b/pkg/workflow/github_folder_checkout_optimization_test.go index e102bfb7fbc..b128720ecd8 100644 --- a/pkg/workflow/github_folder_checkout_optimization_test.go +++ b/pkg/workflow/github_folder_checkout_optimization_test.go @@ -50,6 +50,8 @@ engine: copilot steps: - name: Custom checkout uses: actions/checkout@v5 + with: + persist-credentials: false --- # Test workflow with custom checkout diff --git a/pkg/workflow/imported_steps_validation.go b/pkg/workflow/imported_steps_validation.go index 3df2c567e3f..1677a94c46d 100644 --- a/pkg/workflow/imported_steps_validation.go +++ b/pkg/workflow/imported_steps_validation.go @@ -2,9 +2,20 @@ // // # Imported Steps Validation // -// This file previously validated that custom engine steps did not use agentic engine -// secrets. With the removal of custom engine support, this validation is now a no-op. -// The file is kept for backwards compatibility with the compiler orchestrator. +// This file validates steps used in the agent job, including: +// - Main frontmatter steps (under the 'steps' key) +// - Imported steps merged from shared workflows (MergedSteps) +// +// # Checkout Credential Validation +// +// When actions/checkout is used without 'persist-credentials: false', the GitHub +// token is stored in .git/config and accessible to the agent, which is a security +// concern. This file detects this pattern and: +// - In strict mode: raises a compilation error +// - In non-strict mode: emits a warning +// +// Only the agent job is checked (frontmatter 'steps' and imported MergedSteps). +// Custom jobs defined under 'jobs:' are not affected by this validation. // // For general validation, see validation.go. // For strict mode validation, see strict_mode_validation.go. @@ -13,6 +24,13 @@ package workflow import ( + "fmt" + "os" + "strings" + + "github.com/goccy/go-yaml" + + "github.com/github/gh-aw/pkg/console" "github.com/github/gh-aw/pkg/logger" ) @@ -26,3 +44,136 @@ func (c *Compiler) validateImportedStepsNoAgenticSecrets(engineConfig *EngineCon importedStepsValidationLog.Print("Skipping validation: custom engine support removed") return nil } + +// validateCheckoutPersistCredentials checks that actions/checkout steps in the agent job +// include 'persist-credentials: false'. Without this setting, the git token is stored in +// .git/config and accessible to the agent. +// +// This validates steps from: +// - The main frontmatter 'steps' section (agent job steps) +// - Imported steps merged from shared workflows (MergedSteps) +// +// In strict mode this returns an error; in non-strict mode it emits a warning. +// Only applies to the agent job. Custom jobs under 'jobs:' are not checked. +func (c *Compiler) validateCheckoutPersistCredentials(frontmatter map[string]any, mergedSteps string) error { + importedStepsValidationLog.Printf("Validating checkout persist-credentials in agent job steps (strict=%v)", c.strictMode) + + var offendingStepNames []string + + // Check main frontmatter steps (agent job steps defined in the main workflow) + if stepsValue, exists := frontmatter["steps"]; exists { + if steps, ok := stepsValue.([]any); ok { + for _, step := range steps { + if stepMap, ok := step.(map[string]any); ok { + if checkoutMissingPersistCredentialsFalse(stepMap) { + offendingStepNames = append(offendingStepNames, stepDisplayName(stepMap)) + } + } + } + } + } + + // Check imported steps merged from shared workflow files + if mergedSteps != "" { + var importedSteps []any + if err := yaml.Unmarshal([]byte(mergedSteps), &importedSteps); err == nil { + for _, step := range importedSteps { + if stepMap, ok := step.(map[string]any); ok { + if checkoutMissingPersistCredentialsFalse(stepMap) { + offendingStepNames = append(offendingStepNames, stepDisplayName(stepMap)) + } + } + } + } + } + + if len(offendingStepNames) == 0 { + importedStepsValidationLog.Print("Checkout persist-credentials validation passed") + return nil + } + + msg := fmt.Sprintf( + "actions/checkout step(s) without 'persist-credentials: false' detected in the agent job: %s. "+ + "Without this setting the git token is stored in .git/config and leaked to the agent. "+ + "Add 'persist-credentials: false' to the 'with:' block of each checkout step. "+ + "See: https://github.github.com/gh-aw/reference/steps/", + strings.Join(offendingStepNames, ", "), + ) + + importedStepsValidationLog.Printf("Checkout validation failed: %s", msg) + + if c.strictMode { + return fmt.Errorf("strict mode: %s", msg) + } + + // Non-strict mode: emit a warning + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(msg)) + c.IncrementWarningCount() + return nil +} + +// checkoutMissingPersistCredentialsFalse returns true when the step uses actions/checkout +// but does not have 'persist-credentials: false' in its 'with' block. +func checkoutMissingPersistCredentialsFalse(step map[string]any) bool { + usesValue, exists := step["uses"] + if !exists { + return false + } + + usesStr, ok := usesValue.(string) + if !ok { + return false + } + + // Strip action pin comments (e.g., "actions/checkout@abc123 # v4") before matching + usesStr = strings.TrimSpace(strings.SplitN(usesStr, " #", 2)[0]) + + // Match "actions/checkout" or "actions/checkout@" + if usesStr != "actions/checkout" && !strings.HasPrefix(usesStr, "actions/checkout@") { + return false + } + + // Check whether persist-credentials is explicitly set to false + withValue, hasWithBlock := step["with"] + if !hasWithBlock { + // No with block: persist-credentials defaults to true → insecure + return true + } + + withMap, ok := withValue.(map[string]any) + if !ok { + // Unexpected with format: treat conservatively as insecure + return true + } + + persistCredentials, hasPersistCredentials := withMap["persist-credentials"] + if !hasPersistCredentials { + // Not set: defaults to true → insecure + return true + } + + // Bool false is the safe value + if boolVal, ok := persistCredentials.(bool); ok { + return boolVal // true = insecure, false = safe + } + + // String "false" is also accepted by GitHub Actions + if strVal, ok := persistCredentials.(string); ok { + return strVal != "false" + } + + // Unknown type: treat conservatively as insecure + return true +} + +// stepDisplayName returns a human-readable identifier for a step. +// It uses the step name if available, otherwise the uses value. +func stepDisplayName(step map[string]any) string { + if name, ok := step["name"].(string); ok && name != "" { + return fmt.Sprintf("'%s'", name) + } + if uses, ok := step["uses"].(string); ok && uses != "" { + return fmt.Sprintf("'%s'", uses) + } + return "''" +} diff --git a/pkg/workflow/imported_steps_validation_test.go b/pkg/workflow/imported_steps_validation_test.go new file mode 100644 index 00000000000..cf64a6599e3 --- /dev/null +++ b/pkg/workflow/imported_steps_validation_test.go @@ -0,0 +1,494 @@ +//go:build !integration + +package workflow + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestCheckoutMissingPersistCredentialsFalse tests the checkoutMissingPersistCredentialsFalse helper +func TestCheckoutMissingPersistCredentialsFalse(t *testing.T) { + tests := []struct { + name string + step map[string]any + expected bool // true = insecure (missing persist-credentials: false) + }{ + { + name: "non-checkout step is safe", + step: map[string]any{ + "name": "Run tests", + "run": "go test ./...", + }, + expected: false, + }, + { + name: "checkout without with block is insecure", + step: map[string]any{ + "uses": "actions/checkout@v4", + }, + expected: true, + }, + { + name: "checkout with persist-credentials false is safe", + step: map[string]any{ + "uses": "actions/checkout@v4", + "with": map[string]any{ + "persist-credentials": false, + }, + }, + expected: false, + }, + { + name: "checkout with persist-credentials true is insecure", + step: map[string]any{ + "uses": "actions/checkout@v4", + "with": map[string]any{ + "persist-credentials": true, + }, + }, + expected: true, + }, + { + name: "checkout with persist-credentials string false is safe", + step: map[string]any{ + "uses": "actions/checkout@v4", + "with": map[string]any{ + "persist-credentials": "false", + }, + }, + expected: false, + }, + { + name: "checkout with persist-credentials string true is insecure", + step: map[string]any{ + "uses": "actions/checkout@v4", + "with": map[string]any{ + "persist-credentials": "true", + }, + }, + expected: true, + }, + { + name: "checkout without persist-credentials key in with block is insecure", + step: map[string]any{ + "uses": "actions/checkout@v4", + "with": map[string]any{ + "fetch-depth": 1, + }, + }, + expected: true, + }, + { + name: "checkout without version tag is insecure", + step: map[string]any{ + "uses": "actions/checkout", + "with": map[string]any{ + "fetch-depth": 0, + }, + }, + expected: true, + }, + { + name: "checkout without version tag with persist-credentials false is safe", + step: map[string]any{ + "uses": "actions/checkout", + "with": map[string]any{ + "persist-credentials": false, + }, + }, + expected: false, + }, + { + name: "step with no uses key is safe", + step: map[string]any{ + "name": "Print message", + "run": "echo hello", + }, + expected: false, + }, + { + name: "checkout with SHA pin and persist-credentials false is safe", + step: map[string]any{ + "uses": "actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683", + "with": map[string]any{ + "persist-credentials": false, + }, + }, + expected: false, + }, + { + name: "checkout with SHA pin without persist-credentials is insecure", + step: map[string]any{ + "uses": "actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683", + }, + expected: true, + }, + { + name: "different action starting with actions/checkout prefix but not checkout itself", + step: map[string]any{ + "uses": "actions/checkout-extra@v1", + }, + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := checkoutMissingPersistCredentialsFalse(tt.step) + assert.Equal(t, tt.expected, result, "checkoutMissingPersistCredentialsFalse returned unexpected result") + }) + } +} + +// TestValidateCheckoutPersistCredentials_FrontmatterSteps tests the main frontmatter steps validation +func TestValidateCheckoutPersistCredentials_FrontmatterSteps(t *testing.T) { + tests := []struct { + name string + frontmatter map[string]any + mergedSteps string + strictMode bool + expectError bool + errorMsg string + }{ + { + name: "no steps section - no error", + frontmatter: map[string]any{ + "on": "push", + }, + strictMode: true, + expectError: false, + }, + { + name: "empty steps - no error", + frontmatter: map[string]any{ + "on": "push", + "steps": []any{}, + }, + strictMode: true, + expectError: false, + }, + { + name: "checkout with persist-credentials false - no error", + frontmatter: map[string]any{ + "steps": []any{ + map[string]any{ + "name": "Checkout", + "uses": "actions/checkout@v4", + "with": map[string]any{ + "persist-credentials": false, + }, + }, + }, + }, + strictMode: true, + expectError: false, + }, + { + name: "checkout without persist-credentials false in strict mode - error", + frontmatter: map[string]any{ + "steps": []any{ + map[string]any{ + "name": "Checkout", + "uses": "actions/checkout@v4", + }, + }, + }, + strictMode: true, + expectError: true, + errorMsg: "strict mode: actions/checkout step(s) without 'persist-credentials: false'", + }, + { + name: "checkout without persist-credentials false in non-strict mode - warning only", + frontmatter: map[string]any{ + "steps": []any{ + map[string]any{ + "name": "Checkout", + "uses": "actions/checkout@v4", + }, + }, + }, + strictMode: false, + expectError: false, // warning only in non-strict mode + }, + { + name: "non-checkout step - no error", + frontmatter: map[string]any{ + "steps": []any{ + map[string]any{ + "name": "Run tests", + "run": "go test ./...", + }, + }, + }, + strictMode: true, + expectError: false, + }, + { + name: "error includes step name", + frontmatter: map[string]any{ + "steps": []any{ + map[string]any{ + "name": "My Checkout Step", + "uses": "actions/checkout@v4", + }, + }, + }, + strictMode: true, + expectError: true, + errorMsg: "'My Checkout Step'", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + compiler := NewCompiler() + compiler.strictMode = tt.strictMode + + err := compiler.validateCheckoutPersistCredentials(tt.frontmatter, tt.mergedSteps) + + if tt.expectError { + require.Error(t, err, "Expected an error but got none") + assert.Contains(t, err.Error(), tt.errorMsg, "Error message mismatch") + } else { + assert.NoError(t, err, "Expected no error but got: %v", err) + } + }) + } +} + +// TestValidateCheckoutPersistCredentials_MergedSteps tests imported (merged) steps validation +func TestValidateCheckoutPersistCredentials_MergedSteps(t *testing.T) { + tests := []struct { + name string + mergedSteps string + strictMode bool + expectError bool + errorMsg string + }{ + { + name: "empty merged steps - no error", + mergedSteps: "", + strictMode: true, + expectError: false, + }, + { + name: "imported checkout with persist-credentials false - no error", + mergedSteps: `- name: Checkout + uses: actions/checkout@v4 + with: + persist-credentials: false +`, + strictMode: true, + expectError: false, + }, + { + name: "imported checkout without persist-credentials in strict mode - error", + mergedSteps: `- name: Imported Checkout + uses: actions/checkout@v4 +`, + strictMode: true, + expectError: true, + errorMsg: "strict mode: actions/checkout step(s) without 'persist-credentials: false'", + }, + { + name: "imported checkout without persist-credentials in non-strict mode - warning only", + mergedSteps: `- name: Imported Checkout + uses: actions/checkout@v4 +`, + strictMode: false, + expectError: false, + }, + { + name: "imported non-checkout step - no error", + mergedSteps: `- name: Setup node + uses: actions/setup-node@v4 + with: + node-version: '20' +`, + strictMode: true, + expectError: false, + }, + { + name: "error message includes imported step name", + mergedSteps: `- name: My Imported Checkout + uses: actions/checkout@v4 +`, + strictMode: true, + expectError: true, + errorMsg: "'My Imported Checkout'", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + compiler := NewCompiler() + compiler.strictMode = tt.strictMode + + err := compiler.validateCheckoutPersistCredentials(map[string]any{}, tt.mergedSteps) + + if tt.expectError { + require.Error(t, err, "Expected an error but got none") + assert.Contains(t, err.Error(), tt.errorMsg, "Error message mismatch") + } else { + assert.NoError(t, err, "Expected no error but got: %v", err) + } + }) + } +} + +// TestValidateCheckoutPersistCredentials_WarningEmitted tests that a warning is emitted in non-strict mode +func TestValidateCheckoutPersistCredentials_WarningEmitted(t *testing.T) { + compiler := NewCompiler() + compiler.strictMode = false + + frontmatter := map[string]any{ + "steps": []any{ + map[string]any{ + "name": "Checkout", + "uses": "actions/checkout@v4", + }, + }, + } + + initialWarnings := compiler.GetWarningCount() + err := compiler.validateCheckoutPersistCredentials(frontmatter, "") + require.NoError(t, err, "Should not return an error in non-strict mode") + assert.Greater(t, compiler.GetWarningCount(), initialWarnings, "Warning count should be incremented") +} + +// TestValidateCheckoutPersistCredentials_BothSourcesChecked tests that both frontmatter and +// imported steps are checked together +func TestValidateCheckoutPersistCredentials_BothSourcesChecked(t *testing.T) { + compiler := NewCompiler() + compiler.strictMode = true + + // Main frontmatter has safe checkout + frontmatter := map[string]any{ + "steps": []any{ + map[string]any{ + "name": "Safe Checkout", + "uses": "actions/checkout@v4", + "with": map[string]any{ + "persist-credentials": false, + }, + }, + }, + } + + // Imported steps have insecure checkout + mergedSteps := `- name: Insecure Imported Checkout + uses: actions/checkout@v4 +` + + err := compiler.validateCheckoutPersistCredentials(frontmatter, mergedSteps) + require.Error(t, err, "Should error when imported step has insecure checkout") + assert.Contains(t, err.Error(), "'Insecure Imported Checkout'", "Error should reference the insecure imported step") + assert.NotContains(t, err.Error(), "'Safe Checkout'", "Error should not reference the safe step") +} + +// TestValidateCheckoutPersistCredentials_MultipleOffenders tests reporting multiple insecure steps +func TestValidateCheckoutPersistCredentials_MultipleOffenders(t *testing.T) { + compiler := NewCompiler() + compiler.strictMode = true + + frontmatter := map[string]any{ + "steps": []any{ + map[string]any{ + "name": "First Checkout", + "uses": "actions/checkout@v4", + }, + map[string]any{ + "name": "Second Checkout", + "uses": "actions/checkout@v4", + }, + }, + } + + err := compiler.validateCheckoutPersistCredentials(frontmatter, "") + require.Error(t, err, "Should error when multiple steps have insecure checkout") + assert.Contains(t, err.Error(), "'First Checkout'", "Error should mention first step") + assert.Contains(t, err.Error(), "'Second Checkout'", "Error should mention second step") +} + +// TestStepDisplayName tests the stepDisplayName helper function +func TestStepDisplayName(t *testing.T) { + tests := []struct { + name string + step map[string]any + expected string + }{ + { + name: "step with name", + step: map[string]any{"name": "Checkout", "uses": "actions/checkout@v4"}, + expected: "'Checkout'", + }, + { + name: "step without name but with uses", + step: map[string]any{"uses": "actions/checkout@v4"}, + expected: "'actions/checkout@v4'", + }, + { + name: "step without name or uses", + step: map[string]any{"run": "echo hello"}, + expected: "''", + }, + { + name: "step with empty name falls back to uses", + step: map[string]any{"name": "", "uses": "actions/checkout@v4"}, + expected: "'actions/checkout@v4'", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := stepDisplayName(tt.step) + assert.Equal(t, tt.expected, result, "stepDisplayName returned unexpected value") + }) + } +} + +// TestCheckoutMissingPersistCredentialsFalse_ActionPin tests that action pin comments are handled +func TestCheckoutMissingPersistCredentialsFalse_ActionPin(t *testing.T) { + // Action pin with SHA and comment should still be recognized as checkout + step := map[string]any{ + "uses": "actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2", + } + result := checkoutMissingPersistCredentialsFalse(step) + assert.True(t, result, "Checkout with action pin comment but no persist-credentials should be flagged") + + // Action pin with SHA, comment, and persist-credentials: false should be safe + stepSafe := map[string]any{ + "uses": "actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2", + "with": map[string]any{ + "persist-credentials": false, + }, + } + resultSafe := checkoutMissingPersistCredentialsFalse(stepSafe) + assert.False(t, resultSafe, "Checkout with action pin comment and persist-credentials: false should be safe") +} + +// TestValidateCheckoutPersistCredentials_GitLeakErrorMessage validates the error mentions token leak +func TestValidateCheckoutPersistCredentials_GitLeakErrorMessage(t *testing.T) { + compiler := NewCompiler() + compiler.strictMode = true + + frontmatter := map[string]any{ + "steps": []any{ + map[string]any{ + "uses": "actions/checkout@v4", + }, + }, + } + + err := compiler.validateCheckoutPersistCredentials(frontmatter, "") + require.Error(t, err) + assert.True(t, + strings.Contains(err.Error(), "git token") || strings.Contains(err.Error(), ".git/config"), + "Error should mention git token leak", + ) + assert.Contains(t, err.Error(), "persist-credentials: false", "Error should mention the fix") +} diff --git a/pkg/workflow/temp_dir_before_custom_steps_test.go b/pkg/workflow/temp_dir_before_custom_steps_test.go index ba54e7cbc10..16afb56d4b1 100644 --- a/pkg/workflow/temp_dir_before_custom_steps_test.go +++ b/pkg/workflow/temp_dir_before_custom_steps_test.go @@ -149,6 +149,8 @@ engine: copilot steps: - name: Checkout code uses: actions/checkout@v5 + with: + persist-credentials: false - name: Custom step after checkout run: echo "Using /tmp/gh-aw/" ---