From de905e1585ca614f1c525cad0bdffa18dec2169c Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Mon, 22 Dec 2025 16:12:35 +0000 Subject: [PATCH] Security: Remove key parameter from validateSecretsExpression to prevent data flow detection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit fixes CodeQL alert #71 (go/clear-text-logging) by removing the unused 'key' parameter from the validateSecretsExpression function. While the key was not being included in error messages, CodeQL detected it as a data flow path for sensitive information (secret key names) that could potentially reach logging or JSON output systems. Changes: - Removed key parameter from validateSecretsExpression function signature - Updated all callers to only pass the value parameter - Updated all tests to reflect the new function signature - Added documentation explaining the security rationale This eliminates the data flow path that CodeQL flagged while maintaining all existing validation functionality. Fixes: #71 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 --- pkg/workflow/compiler_jobs.go | 4 +- pkg/workflow/jobs_secrets_validation_test.go | 39 ++++--- pkg/workflow/secrets_validation.go | 8 +- pkg/workflow/secrets_validation_test.go | 102 ++++++++----------- 4 files changed, 67 insertions(+), 86 deletions(-) diff --git a/pkg/workflow/compiler_jobs.go b/pkg/workflow/compiler_jobs.go index 81e78f7a69..567f1137ee 100644 --- a/pkg/workflow/compiler_jobs.go +++ b/pkg/workflow/compiler_jobs.go @@ -386,7 +386,9 @@ func (c *Compiler) buildCustomJobs(data *WorkflowData, activationJobCreated bool for key, val := range secretsMap { if valStr, ok := val.(string); ok { // Validate that the secret value is a proper GitHub Actions expression - if err := validateSecretsExpression(key, valStr); err != nil { + // Note: We don't pass the key to validateSecretsExpression to prevent + // CodeQL from detecting sensitive data flow to error messages/logs + if err := validateSecretsExpression(valStr); err != nil { return err } job.Secrets[key] = valStr diff --git a/pkg/workflow/jobs_secrets_validation_test.go b/pkg/workflow/jobs_secrets_validation_test.go index 9fc94433a7..0e49f9e193 100644 --- a/pkg/workflow/jobs_secrets_validation_test.go +++ b/pkg/workflow/jobs_secrets_validation_test.go @@ -321,36 +321,35 @@ Test workflow with additional text in secret.`, func TestValidateSecretsExpression(t *testing.T) { tests := []struct { name string - key string value string expectError bool }{ // Valid cases - {"valid simple secret", "token", "${{ secrets.GITHUB_TOKEN }}", false}, - {"valid with fallback", "token", "${{ secrets.TOKEN1 || secrets.TOKEN2 }}", false}, - {"valid with multiple fallbacks", "token", "${{ secrets.TOKEN1 || secrets.TOKEN2 || secrets.TOKEN3 }}", false}, - {"valid with spaces", "token", "${{ secrets.MY_TOKEN }}", false}, - {"valid underscore prefix", "token", "${{ secrets._PRIVATE }}", false}, - {"valid with numbers", "token", "${{ secrets.TOKEN_V2 }}", false}, + {"valid simple secret", "${{ secrets.GITHUB_TOKEN }}", false}, + {"valid with fallback", "${{ secrets.TOKEN1 || secrets.TOKEN2 }}", false}, + {"valid with multiple fallbacks", "${{ secrets.TOKEN1 || secrets.TOKEN2 || secrets.TOKEN3 }}", false}, + {"valid with spaces", "${{ secrets.MY_TOKEN }}", false}, + {"valid underscore prefix", "${{ secrets._PRIVATE }}", false}, + {"valid with numbers", "${{ secrets.TOKEN_V2 }}", false}, // Invalid cases - {"invalid plaintext", "token", "my-secret", true}, - {"invalid GitHub PAT", "token", "ghp_1234567890abcdef", true}, - {"invalid env reference", "token", "${{ env.MY_TOKEN }}", true}, - {"invalid vars reference", "token", "${{ vars.MY_TOKEN }}", true}, - {"invalid github context", "token", "${{ github.token }}", true}, - {"invalid missing closing", "token", "${{ secrets.MY_TOKEN", true}, - {"invalid missing opening", "token", "secrets.MY_TOKEN }}", true}, - {"invalid empty", "token", "", true}, - {"invalid with text prefix", "token", "Bearer ${{ secrets.TOKEN }}", true}, - {"invalid with text suffix", "token", "${{ secrets.TOKEN }} extra", true}, - {"invalid mixed contexts", "token", "${{ secrets.TOKEN || env.FALLBACK }}", true}, - {"invalid number prefix", "token", "${{ secrets.123TOKEN }}", true}, + {"invalid plaintext", "my-secret", true}, + {"invalid GitHub PAT", "ghp_1234567890abcdef", true}, + {"invalid env reference", "${{ env.MY_TOKEN }}", true}, + {"invalid vars reference", "${{ vars.MY_TOKEN }}", true}, + {"invalid github context", "${{ github.token }}", true}, + {"invalid missing closing", "${{ secrets.MY_TOKEN", true}, + {"invalid missing opening", "secrets.MY_TOKEN }}", true}, + {"invalid empty", "", true}, + {"invalid with text prefix", "Bearer ${{ secrets.TOKEN }}", true}, + {"invalid with text suffix", "${{ secrets.TOKEN }} extra", true}, + {"invalid mixed contexts", "${{ secrets.TOKEN || env.FALLBACK }}", true}, + {"invalid number prefix", "${{ secrets.123TOKEN }}", true}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - err := validateSecretsExpression(tt.key, tt.value) + err := validateSecretsExpression(tt.value) if tt.expectError && err == nil { t.Errorf("Expected error, got nil") } else if !tt.expectError && err != nil { diff --git a/pkg/workflow/secrets_validation.go b/pkg/workflow/secrets_validation.go index d098510040..e16d52c2df 100644 --- a/pkg/workflow/secrets_validation.go +++ b/pkg/workflow/secrets_validation.go @@ -16,13 +16,11 @@ var secretsExpressionPattern = regexp.MustCompile(`^\$\{\{\s*secrets\.[A-Za-z_][ // validateSecretsExpression validates that a value is a proper GitHub Actions secrets expression. // Returns an error if the value is not in the format: ${{ secrets.NAME }} or ${{ secrets.NAME || secrets.NAME2 }} -func validateSecretsExpression(key, value string) error { +// Note: This function intentionally does not accept the secret key name as a parameter to prevent +// CodeQL from detecting a data flow of sensitive information (secret key names) to logging or error outputs. +func validateSecretsExpression(value string) error { if !secretsExpressionPattern.MatchString(value) { secretsValidationLog.Printf("Invalid secret expression detected") - // Note: We intentionally do NOT include the key name in the error message to avoid - // logging sensitive information (secret key names) that could expose details about - // the organization's security infrastructure. The key name is available in the - // calling context for debugging purposes if needed. return fmt.Errorf("invalid secrets expression: must be a GitHub Actions expression with secrets reference (e.g., '${{ secrets.MY_SECRET }}' or '${{ secrets.SECRET1 || secrets.SECRET2 }}')") } secretsValidationLog.Printf("Valid secret expression validated") diff --git a/pkg/workflow/secrets_validation_test.go b/pkg/workflow/secrets_validation_test.go index 589a25515f..efacd6cea5 100644 --- a/pkg/workflow/secrets_validation_test.go +++ b/pkg/workflow/secrets_validation_test.go @@ -64,60 +64,53 @@ func TestSecretsExpressionPattern(t *testing.T) { } // TestValidateSecretsExpressionErrorMessages tests that error messages are descriptive -// but do NOT include sensitive values OR KEY NAMES to prevent clear-text logging +// but do NOT include sensitive values to prevent clear-text logging func TestValidateSecretsExpressionErrorMessages(t *testing.T) { tests := []struct { name string - key string value string expectedInErrs []string notExpectedInErrs []string }{ { - name: "plaintext does NOT show value OR key name in error", - key: "token", + name: "plaintext does NOT show value in error", value: "plaintext", expectedInErrs: []string{"invalid secrets expression", "must be a GitHub Actions expression"}, - notExpectedInErrs: []string{"plaintext", "token"}, + notExpectedInErrs: []string{"plaintext"}, }, { - name: "env context does NOT show value OR key name in error", - key: "api_key", + name: "env context does NOT show value in error", value: "${{ env.TOKEN }}", expectedInErrs: []string{"invalid secrets expression"}, - notExpectedInErrs: []string{"${{ env.TOKEN }}", "api_key"}, + notExpectedInErrs: []string{"${{ env.TOKEN }}"}, }, { - name: "key name NOT in error (security fix)", - key: "database_password", + name: "hardcoded value NOT in error (security fix)", value: "hardcoded", expectedInErrs: []string{"invalid secrets expression"}, - notExpectedInErrs: []string{"database_password"}, + notExpectedInErrs: []string{"hardcoded"}, }, { name: "example format in error", - key: "token", value: "bad", expectedInErrs: []string{"${{ secrets.MY_SECRET }}"}, }, { name: "fallback example in error", - key: "token", value: "bad", expectedInErrs: []string{"${{ secrets.SECRET1 || secrets.SECRET2 }}"}, }, { - name: "mixed context error does NOT show value OR key name", - key: "deploy_token", + name: "mixed context error does NOT show value", value: "${{ secrets.TOKEN || env.FALLBACK }}", expectedInErrs: []string{"invalid secrets expression"}, - notExpectedInErrs: []string{"${{ secrets.TOKEN || env.FALLBACK }}", "deploy_token"}, + notExpectedInErrs: []string{"${{ secrets.TOKEN || env.FALLBACK }}"}, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - err := validateSecretsExpression(tt.key, tt.value) + err := validateSecretsExpression(tt.value) if err == nil { t.Fatalf("Expected error, got nil") } @@ -136,43 +129,40 @@ func TestValidateSecretsExpressionErrorMessages(t *testing.T) { } } -// TestValidateSecretsExpressionWithDifferentKeys tests validation with various key names -func TestValidateSecretsExpressionWithDifferentKeys(t *testing.T) { - validValue := "${{ secrets.GITHUB_TOKEN }}" - invalidValue := "plaintext" - - keys := []string{ - "token", - "api_key", - "database_password", - "smtp_password", - "api_token", - "deploy_key", - "webhook_secret", - "", // empty key should still work +// TestValidateSecretsExpressionWithVariousValues tests validation with different values +func TestValidateSecretsExpressionWithVariousValues(t *testing.T) { + tests := []struct { + name string + value string + expectError bool + }{ + {"valid simple", "${{ secrets.GITHUB_TOKEN }}", false}, + {"valid with underscore", "${{ secrets.MY_TOKEN }}", false}, + {"valid with fallback", "${{ secrets.TOKEN1 || secrets.TOKEN2 }}", false}, + {"invalid plaintext", "plaintext", true}, + {"invalid env", "${{ env.TOKEN }}", true}, + {"invalid vars", "${{ vars.TOKEN }}", true}, + {"invalid mixed", "${{ secrets.TOKEN || env.FALLBACK }}", true}, + {"invalid empty", "", true}, } - for _, key := range keys { - t.Run("valid_key_"+key, func(t *testing.T) { - err := validateSecretsExpression(key, validValue) - if err != nil { - t.Errorf("Expected no error for valid value with key %q, got: %v", key, err) - } - }) - - t.Run("invalid_key_"+key, func(t *testing.T) { - err := validateSecretsExpression(key, invalidValue) - if err == nil { - t.Errorf("Expected error for invalid value with key %q, got nil", key) - } - // Security fix: Error message should NOT include the key name to prevent - // logging sensitive information about the organization's security infrastructure - if key != "" && strings.Contains(err.Error(), key) { - t.Errorf("Error should NOT contain sensitive key name %q, but got: %s", key, err.Error()) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateSecretsExpression(tt.value) + if tt.expectError && err == nil { + t.Errorf("Expected error for value %q, got nil", tt.value) + } else if !tt.expectError && err != nil { + t.Errorf("Expected no error for value %q, got: %v", tt.value, err) } - // Error should still be descriptive - if !strings.Contains(err.Error(), "invalid secrets expression") { - t.Errorf("Error should contain descriptive message, got: %s", err.Error()) + // Error should be descriptive and not contain the actual value + if err != nil { + if !strings.Contains(err.Error(), "invalid secrets expression") { + t.Errorf("Error should contain descriptive message, got: %s", err.Error()) + } + // Security: Ensure the actual invalid value is not in the error message + if tt.value != "" && strings.Contains(err.Error(), tt.value) { + t.Errorf("Error should NOT contain the actual value %q, but got: %s", tt.value, err.Error()) + } } }) } @@ -182,56 +172,48 @@ func TestValidateSecretsExpressionWithDifferentKeys(t *testing.T) { func TestSecretsValidationEdgeCases(t *testing.T) { tests := []struct { name string - key string value string expectError bool description string }{ { name: "very long secret name", - key: "token", value: "${{ secrets.THIS_IS_A_VERY_LONG_SECRET_NAME_WITH_MANY_UNDERSCORES_123456789 }}", expectError: false, description: "Should accept long secret names", }, { name: "many fallbacks", - key: "token", value: "${{ secrets.TOKEN1 || secrets.TOKEN2 || secrets.TOKEN3 || secrets.TOKEN4 || secrets.TOKEN5 }}", expectError: false, description: "Should accept multiple fallbacks", }, { name: "minimal valid expression", - key: "t", value: "${{ secrets.T }}", expectError: false, description: "Should accept single character secret names", }, { name: "underscore only name", - key: "token", value: "${{ secrets._ }}", expectError: false, description: "Should accept underscore-only names", }, { name: "mixed with spaces in fallback", - key: "token", value: "${{ secrets.TOKEN1 || secrets.TOKEN2 }}", expectError: false, description: "Should accept extra spaces in fallback", }, { name: "almost valid but trailing dot", - key: "token", value: "${{ secrets.TOKEN. }}", expectError: true, description: "Should reject trailing dot", }, { name: "unicode in secret name", - key: "token", value: "${{ secrets.TOKEN_🔑 }}", expectError: true, description: "Should reject unicode characters", @@ -240,7 +222,7 @@ func TestSecretsValidationEdgeCases(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - err := validateSecretsExpression(tt.key, tt.value) + err := validateSecretsExpression(tt.value) if tt.expectError && err == nil { t.Errorf("%s: Expected error, got nil", tt.description) } else if !tt.expectError && err != nil {