diff --git a/pkg/workflow/compiler_orchestrator_engine.go b/pkg/workflow/compiler_orchestrator_engine.go index e5cf478e0c..9e6e18f010 100644 --- a/pkg/workflow/compiler_orchestrator_engine.go +++ b/pkg/workflow/compiler_orchestrator_engine.go @@ -76,6 +76,14 @@ func (c *Compiler) setupEngineAndImports(result *parser.FrontmatterResult, clean return nil, err } + // Validate env secrets regardless of strict mode (error in strict, warning in non-strict) + if err := c.validateEnvSecrets(result.Frontmatter); err != nil { + orchestratorEngineLog.Printf("Env secrets validation failed: %v", err) + // Restore strict mode before returning error + c.strictMode = initialStrictMode + return nil, err + } + // Restore the initial strict mode state after validation // This ensures strict mode doesn't leak to other workflows being compiled c.strictMode = initialStrictMode diff --git a/pkg/workflow/env_secrets_validation_test.go b/pkg/workflow/env_secrets_validation_test.go new file mode 100644 index 0000000000..6060f1c635 --- /dev/null +++ b/pkg/workflow/env_secrets_validation_test.go @@ -0,0 +1,449 @@ +//go:build !integration + +package workflow + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestValidateEnvSecrets(t *testing.T) { + tests := []struct { + name string + frontmatter map[string]any + strictMode bool + expectError bool + errorMsg string + }{ + { + name: "no env section is allowed", + frontmatter: map[string]any{ + "on": "push", + }, + strictMode: true, + expectError: false, + }, + { + name: "env with no secrets is allowed", + frontmatter: map[string]any{ + "on": "push", + "env": map[string]any{ + "NODE_ENV": "production", + "API_URL": "https://api.example.com", + "ENABLE_LOGS": "true", + }, + }, + strictMode: true, + expectError: false, + }, + { + name: "env with plain values in non-strict mode is allowed", + frontmatter: map[string]any{ + "on": "push", + "env": map[string]any{ + "NODE_ENV": "production", + "API_URL": "https://api.example.com", + }, + }, + strictMode: false, + expectError: false, + }, + { + name: "env with single secret in strict mode fails", + frontmatter: map[string]any{ + "on": "push", + "env": map[string]any{ + "API_KEY": "${{ secrets.API_KEY }}", + }, + }, + strictMode: true, + expectError: true, + errorMsg: "strict mode: secrets detected in 'env' section will be leaked to the agent container. Found: ${{ secrets.API_KEY }}", + }, + { + name: "env with multiple secrets in strict mode fails", + frontmatter: map[string]any{ + "on": "push", + "env": map[string]any{ + "API_KEY": "${{ secrets.API_KEY }}", + "DB_PASS": "${{ secrets.DATABASE_PASSWORD }}", + "AUTH_TOKEN": "${{ secrets.AUTH_TOKEN }}", + }, + }, + strictMode: true, + expectError: true, + errorMsg: "strict mode: secrets detected in 'env' section will be leaked to the agent container", + }, + { + name: "env with secret with fallback in strict mode fails", + frontmatter: map[string]any{ + "on": "push", + "env": map[string]any{ + "API_KEY": "${{ secrets.API_KEY || 'default' }}", + }, + }, + strictMode: true, + expectError: true, + errorMsg: "strict mode: secrets detected in 'env' section will be leaked to the agent container", + }, + { + name: "env with secret embedded in string in strict mode fails", + frontmatter: map[string]any{ + "on": "push", + "env": map[string]any{ + "AUTH_HEADER": "Bearer ${{ secrets.TOKEN }}", + }, + }, + strictMode: true, + expectError: true, + errorMsg: "strict mode: secrets detected in 'env' section will be leaked to the agent container. Found: ${{ secrets.TOKEN }}", + }, + { + name: "env with mixed secrets and plain values in strict mode fails", + frontmatter: map[string]any{ + "on": "push", + "env": map[string]any{ + "NODE_ENV": "production", + "API_KEY": "${{ secrets.API_KEY }}", + "API_URL": "https://api.example.com", + }, + }, + strictMode: true, + expectError: true, + errorMsg: "strict mode: secrets detected in 'env' section will be leaked to the agent container. Found: ${{ secrets.API_KEY }}", + }, + { + name: "env with env variables (not secrets) is allowed", + frontmatter: map[string]any{ + "on": "push", + "env": map[string]any{ + "NODE_ENV": "production", + "WORKSPACE": "${{ github.workspace }}", + "RUNNER_TEMP": "${{ runner.temp }}", + "GITHUB_TOKEN": "${{ github.token }}", + }, + }, + strictMode: true, + expectError: false, + }, + { + name: "empty env section is allowed", + frontmatter: map[string]any{ + "on": "push", + "env": map[string]any{}, + }, + strictMode: true, + expectError: false, + }, + { + name: "env section with non-string values is allowed (edge case)", + frontmatter: map[string]any{ + "on": "push", + "env": map[string]any{ + "PORT": 8080, + "ENABLE_DEBUG": true, + }, + }, + strictMode: true, + expectError: false, + }, + { + name: "env with sub-expression: github.workflow && secrets.TOKEN fails in strict mode", + frontmatter: map[string]any{ + "on": "push", + "env": map[string]any{ + "TOKEN": "${{ github.workflow && secrets.TOKEN }}", + }, + }, + strictMode: true, + expectError: true, + errorMsg: "strict mode: secrets detected in 'env' section will be leaked to the agent container", + }, + { + name: "env with sub-expression: secrets in OR with env fails in strict mode", + frontmatter: map[string]any{ + "on": "push", + "env": map[string]any{ + "DB_PASS": "${{ secrets.DB_PASSWORD || env.DEFAULT_PASS }}", + }, + }, + strictMode: true, + expectError: true, + errorMsg: "strict mode: secrets detected in 'env' section will be leaked to the agent container", + }, + { + name: "env with sub-expression: secrets in parentheses fails in strict mode", + frontmatter: map[string]any{ + "on": "push", + "env": map[string]any{ + "AUTH": "${{ (github.actor || secrets.HIDDEN_KEY) }}", + }, + }, + strictMode: true, + expectError: true, + errorMsg: "strict mode: secrets detected in 'env' section will be leaked to the agent container", + }, + { + name: "env with sub-expression: NOT operator with secrets fails in strict mode", + frontmatter: map[string]any{ + "on": "push", + "env": map[string]any{ + "CHECK": "${{ !secrets.PRIVATE_KEY && github.workflow }}", + }, + }, + strictMode: true, + expectError: true, + errorMsg: "strict mode: secrets detected in 'env' section will be leaked to the agent container", + }, + { + name: "env with multiple sub-expressions fails in strict mode", + frontmatter: map[string]any{ + "on": "push", + "env": map[string]any{ + "SIMPLE": "${{ secrets.SIMPLE_KEY }}", + "SUB_EXPR1": "${{ github.workflow && secrets.TOKEN }}", + "SUB_EXPR2": "${{ (github.actor || secrets.HIDDEN) }}", + }, + }, + strictMode: true, + expectError: true, + errorMsg: "strict mode: secrets detected in 'env' section will be leaked to the agent container", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + compiler := NewCompiler() + compiler.strictMode = tt.strictMode + + err := compiler.validateEnvSecrets(tt.frontmatter) + + if tt.expectError { + require.Error(t, err, "Expected an error but got none") + if tt.errorMsg != "" { + assert.Contains(t, err.Error(), tt.errorMsg, "Error message should contain expected text") + } + } else { + assert.NoError(t, err, "Expected no error but got: %v", err) + } + }) + } +} + +func TestValidateEnvSecretsNonStrictMode(t *testing.T) { + // Note: This test cannot capture the warning output directly as it goes to stderr + // The warning behavior is tested by ensuring no error is returned in non-strict mode + tests := []struct { + name string + frontmatter map[string]any + }{ + { + name: "env with single secret in non-strict mode emits warning", + frontmatter: map[string]any{ + "on": "push", + "env": map[string]any{ + "API_KEY": "${{ secrets.API_KEY }}", + }, + }, + }, + { + name: "env with multiple secrets in non-strict mode emits warning", + frontmatter: map[string]any{ + "on": "push", + "env": map[string]any{ + "API_KEY": "${{ secrets.API_KEY }}", + "DB_PASS": "${{ secrets.DATABASE_PASSWORD }}", + }, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + compiler := NewCompiler() + compiler.strictMode = false + + // In non-strict mode, this should not return an error (only emit a warning) + err := compiler.validateEnvSecrets(tt.frontmatter) + assert.NoError(t, err, "Non-strict mode should not return an error, only emit a warning") + }) + } +} + +func TestValidateEnvSecretsIntegration(t *testing.T) { + // Test that validateEnvSecrets is properly integrated with the compiler + tests := []struct { + name string + frontmatter map[string]any + strictMode bool + expectError bool + }{ + { + name: "compilation with env secrets in strict mode should fail", + frontmatter: map[string]any{ + "on": "push", + "engine": "copilot", + "env": map[string]any{ + "API_KEY": "${{ secrets.API_KEY }}", + }, + }, + strictMode: true, + expectError: true, + }, + { + name: "compilation with env secrets in non-strict mode should succeed with warning", + frontmatter: map[string]any{ + "on": "push", + "engine": "copilot", + "strict": false, + "env": map[string]any{ + "API_KEY": "${{ secrets.API_KEY }}", + }, + }, + strictMode: false, + expectError: false, + }, + { + name: "compilation without env secrets in strict mode should succeed", + frontmatter: map[string]any{ + "on": "push", + "engine": "copilot", + "env": map[string]any{ + "NODE_ENV": "production", + }, + }, + strictMode: true, + expectError: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + compiler := NewCompiler() + compiler.strictMode = tt.strictMode + + err := compiler.validateEnvSecrets(tt.frontmatter) + + if tt.expectError { + assert.Error(t, err, "Expected an error but got none") + } else { + assert.NoError(t, err, "Expected no error but got: %v", err) + } + }) + } +} + +func TestValidateEnvSecretsErrorMessage(t *testing.T) { + compiler := NewCompiler() + compiler.strictMode = true + + frontmatter := map[string]any{ + "on": "push", + "env": map[string]any{ + "API_KEY": "${{ secrets.API_KEY }}", + }, + } + + err := compiler.validateEnvSecrets(frontmatter) + require.Error(t, err, "Expected an error") + + errorMsg := err.Error() + assert.Contains(t, errorMsg, "strict mode", "Error should mention strict mode") + assert.Contains(t, errorMsg, "secrets detected in 'env' section", "Error should explain the issue") + assert.Contains(t, errorMsg, "leaked to the agent container", "Error should explain the security risk") + assert.Contains(t, errorMsg, "${{ secrets.API_KEY }}", "Error should list the specific secret found") + assert.Contains(t, errorMsg, "Use engine-specific secret configuration", "Error should provide guidance") +} + +func TestExtractSecretsFromEnv(t *testing.T) { + // Test the underlying secret extraction logic + tests := []struct { + name string + envMap map[string]string + expectedCount int + expectedSecret string + }{ + { + name: "single secret", + envMap: map[string]string{ + "API_KEY": "${{ secrets.API_KEY }}", + }, + expectedCount: 1, + expectedSecret: "${{ secrets.API_KEY }}", + }, + { + name: "multiple secrets", + envMap: map[string]string{ + "API_KEY": "${{ secrets.API_KEY }}", + "DB_PASS": "${{ secrets.DB_PASSWORD }}", + }, + expectedCount: 2, + }, + { + name: "secret with fallback", + envMap: map[string]string{ + "API_KEY": "${{ secrets.API_KEY || 'default' }}", + }, + expectedCount: 1, + }, + { + name: "embedded secret", + envMap: map[string]string{ + "AUTH_HEADER": "Bearer ${{ secrets.TOKEN }}", + }, + expectedCount: 1, + expectedSecret: "${{ secrets.TOKEN }}", + }, + { + name: "no secrets", + envMap: map[string]string{ + "NODE_ENV": "production", + "API_URL": "https://api.example.com", + }, + expectedCount: 0, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + secrets := ExtractSecretsFromMap(tt.envMap) + assert.Len(t, secrets, tt.expectedCount, "Secret count mismatch") + + if tt.expectedSecret != "" { + found := false + for _, secret := range secrets { + if secret == tt.expectedSecret { + found = true + break + } + } + assert.True(t, found, "Expected secret %s not found in extracted secrets", tt.expectedSecret) + } + }) + } +} + +func TestValidateEnvSecretsMultipleSecretsErrorMessage(t *testing.T) { + compiler := NewCompiler() + compiler.strictMode = true + + frontmatter := map[string]any{ + "on": "push", + "env": map[string]any{ + "API_KEY": "${{ secrets.API_KEY }}", + "DB_PASS": "${{ secrets.DB_PASSWORD }}", + "AUTH_TOKEN": "${{ secrets.AUTH_TOKEN }}", + }, + } + + err := compiler.validateEnvSecrets(frontmatter) + require.Error(t, err, "Expected an error") + + errorMsg := err.Error() + // Should contain all three secrets or at least indicate multiple secrets + secretCount := strings.Count(errorMsg, "${{ secrets.") + assert.GreaterOrEqual(t, secretCount, 1, "Error should mention at least one secret") +} diff --git a/pkg/workflow/secret_extraction.go b/pkg/workflow/secret_extraction.go index f6d2618935..514d010d90 100644 --- a/pkg/workflow/secret_extraction.go +++ b/pkg/workflow/secret_extraction.go @@ -34,52 +34,35 @@ func ExtractSecretName(value string) string { // ExtractSecretsFromValue extracts all GitHub Actions secret expressions from a string value // Returns a map of environment variable names to their full secret expressions +// This function detects secrets in both simple expressions and sub-expressions: // Examples: // - "${{ secrets.DD_API_KEY }}" -> {"DD_API_KEY": "${{ secrets.DD_API_KEY }}"} // - "${{ secrets.DD_SITE || 'datadoghq.com' }}" -> {"DD_SITE": "${{ secrets.DD_SITE || 'datadoghq.com' }}"} // - "Bearer ${{ secrets.TOKEN }}" -> {"TOKEN": "${{ secrets.TOKEN }}"} +// - "${{ github.workflow && secrets.TOKEN }}" -> {"TOKEN": "${{ github.workflow && secrets.TOKEN }}"} +// - "${{ (github.actor || secrets.HIDDEN) }}" -> {"HIDDEN": "${{ (github.actor || secrets.HIDDEN) }}"} func ExtractSecretsFromValue(value string) map[string]string { secrets := make(map[string]string) - // Pattern to match ${{ secrets.VARIABLE_NAME }} or ${{ secrets.VARIABLE_NAME || 'default' }} - // We need to extract the variable name and the full expression - start := 0 - for { - // Find the start of an expression - startIdx := strings.Index(value[start:], "${{ secrets.") - if startIdx == -1 { - break - } - startIdx += start - - // Find the end of the expression - endIdx := strings.Index(value[startIdx:], "}}") - if endIdx == -1 { - break - } - endIdx += startIdx + 2 // Include the closing }} - - // Extract the full expression - fullExpr := value[startIdx:endIdx] - - // Extract the variable name from "secrets.VARIABLE_NAME" or "secrets.VARIABLE_NAME ||" - secretsPart := strings.TrimPrefix(fullExpr, "${{ secrets.") - secretsPart = strings.TrimSuffix(secretsPart, "}}") - secretsPart = strings.TrimSpace(secretsPart) - - // Find the variable name (everything before space, ||, or end) - varName := secretsPart - if spaceIdx := strings.IndexAny(varName, " |"); spaceIdx != -1 { - varName = varName[:spaceIdx] + // Find all ${{ ... }} expressions in the value + // Pattern matches from ${{ to }} allowing nested content + exprPattern := regexp.MustCompile(`\$\{\{[^}]+\}\}`) + expressions := exprPattern.FindAllString(value, -1) + + // For each expression, check if it contains secrets.VARIABLE_NAME + // This handles both simple cases like "${{ secrets.TOKEN }}" + // and complex sub-expressions like "${{ github.workflow && secrets.TOKEN }}" + secretPattern := regexp.MustCompile(`secrets\.([A-Z_][A-Z0-9_]*)`) + for _, expr := range expressions { + matches := secretPattern.FindAllStringSubmatch(expr, -1) + for _, match := range matches { + if len(match) >= 2 { + varName := match[1] + // Store the full expression that contains this secret + secrets[varName] = expr + secretLog.Printf("Extracted secret: %s from expression: %s", varName, expr) + } } - - // Store the variable name and full expression - if varName != "" { - secrets[varName] = fullExpr - secretLog.Printf("Extracted secret: %s", varName) - } - - start = endIdx } if len(secrets) > 0 { diff --git a/pkg/workflow/secret_extraction_test.go b/pkg/workflow/secret_extraction_test.go index d853a63044..2a5ab23f36 100644 --- a/pkg/workflow/secret_extraction_test.go +++ b/pkg/workflow/secret_extraction_test.go @@ -118,6 +118,49 @@ func TestSharedExtractSecretsFromValue(t *testing.T) { "CONFIG": "${{ secrets.CONFIG || 'default-config-value' }}", }, }, + { + name: "sub-expression: github.workflow && secrets.TOKEN", + value: "${{ github.workflow && secrets.TOKEN }}", + expected: map[string]string{ + "TOKEN": "${{ github.workflow && secrets.TOKEN }}", + }, + }, + { + name: "sub-expression: secrets in OR expression with env", + value: "${{ secrets.DB_PASS || env.FALLBACK }}", + expected: map[string]string{ + "DB_PASS": "${{ secrets.DB_PASS || env.FALLBACK }}", + }, + }, + { + name: "sub-expression: secrets in parentheses", + value: "${{ (github.actor || secrets.HIDDEN) }}", + expected: map[string]string{ + "HIDDEN": "${{ (github.actor || secrets.HIDDEN) }}", + }, + }, + { + name: "sub-expression: complex boolean with secrets", + value: "${{ (github.workflow || secrets.TOKEN) && github.repository }}", + expected: map[string]string{ + "TOKEN": "${{ (github.workflow || secrets.TOKEN) && github.repository }}", + }, + }, + { + name: "sub-expression: NOT operator with secrets", + value: "${{ !secrets.PRIVATE_KEY && github.workflow }}", + expected: map[string]string{ + "PRIVATE_KEY": "${{ !secrets.PRIVATE_KEY && github.workflow }}", + }, + }, + { + name: "sub-expression: multiple secrets in same expression", + value: "${{ secrets.KEY1 && secrets.KEY2 }}", + expected: map[string]string{ + "KEY1": "${{ secrets.KEY1 && secrets.KEY2 }}", + "KEY2": "${{ secrets.KEY1 && secrets.KEY2 }}", + }, + }, } for _, tt := range tests { diff --git a/pkg/workflow/strict_mode_validation.go b/pkg/workflow/strict_mode_validation.go index a519137b5a..db0f1ba1f5 100644 --- a/pkg/workflow/strict_mode_validation.go +++ b/pkg/workflow/strict_mode_validation.go @@ -41,8 +41,10 @@ package workflow import ( "fmt" + "os" "strings" + "github.com/github/gh-aw/pkg/console" "github.com/github/gh-aw/pkg/logger" "github.com/github/gh-aw/pkg/parser" ) @@ -246,6 +248,59 @@ func (c *Compiler) validateStrictDeprecatedFields(frontmatter map[string]any) er return nil } +// validateEnvSecrets detects secrets in the env section and raises an error in strict mode +// or a warning in non-strict mode. Secrets in env will be leaked to the agent container. +func (c *Compiler) validateEnvSecrets(frontmatter map[string]any) error { + // Check if env section exists + envValue, exists := frontmatter["env"] + if !exists { + strictModeValidationLog.Printf("No env section found, validation passed") + return nil + } + + // Parse env as map[string]string + envMap, ok := envValue.(map[string]any) + if !ok { + strictModeValidationLog.Printf("Env section is not a map, skipping validation") + return nil + } + + // Convert to map[string]string for secret extraction + envStrings := make(map[string]string) + for key, value := range envMap { + if strValue, ok := value.(string); ok { + envStrings[key] = strValue + } + } + + // Extract secrets from env values + secrets := ExtractSecretsFromMap(envStrings) + if len(secrets) == 0 { + strictModeValidationLog.Printf("No secrets found in env section") + return nil + } + + // Build list of secret references found + var secretRefs []string + for _, secretExpr := range secrets { + secretRefs = append(secretRefs, secretExpr) + } + + strictModeValidationLog.Printf("Found %d secret(s) in env section: %v", len(secrets), secretRefs) + + // In strict mode, this is an error + if c.strictMode { + return fmt.Errorf("strict mode: secrets detected in 'env' section will be leaked to the agent container. Found: %s. Use engine-specific secret configuration instead. See: https://github.github.com/gh-aw/reference/engines/", strings.Join(secretRefs, ", ")) + } + + // In non-strict mode, emit a warning + warningMsg := fmt.Sprintf("Warning: secrets detected in 'env' section will be leaked to the agent container. Found: %s. Consider using engine-specific secret configuration instead.", strings.Join(secretRefs, ", ")) + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(warningMsg)) + c.IncrementWarningCount() + + return nil +} + // validateStrictMode performs strict mode validations on the workflow // // This is the main orchestrator that calls individual validation functions. @@ -256,6 +311,9 @@ func (c *Compiler) validateStrictDeprecatedFields(frontmatter map[string]any) er // 4. validateStrictTools() - Validates tools configuration (e.g., serena local mode) // 5. validateStrictDeprecatedFields() - Refuses deprecated fields // +// Note: Env secrets validation (validateEnvSecrets) is called separately outside of strict mode +// to emit warnings in non-strict mode and errors in strict mode. +// // Note: Strict mode also affects zizmor security scanner behavior (see pkg/cli/zizmor.go) // When zizmor is enabled with --zizmor flag, strict mode will treat any security // findings as compilation errors rather than warnings.