Skip to content

[security-fix] Fix clear-text logging vulnerability by removing secret key parameter from validation #7289

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 1 commit into from
Dec 22, 2025
Merged

[security-fix] Fix clear-text logging vulnerability by removing secret key parameter from validation #7289

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion pkg/workflow/compiler_jobs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
39 changes: 19 additions & 20 deletions pkg/workflow/jobs_secrets_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
8 changes: 3 additions & 5 deletions pkg/workflow/secrets_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down
102 changes: 42 additions & 60 deletions pkg/workflow/secrets_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
Expand All @@ -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())
}
}
})
}
Expand All @@ -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",
Expand All @@ -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 {
Expand Down