diff --git a/pkg/workflow/secrets_validation.go b/pkg/workflow/secrets_validation.go index 826ad95929..7aaacba9fa 100644 --- a/pkg/workflow/secrets_validation.go +++ b/pkg/workflow/secrets_validation.go @@ -18,8 +18,8 @@ var secretsExpressionPattern = regexp.MustCompile(`^\$\{\{\s*secrets\.[A-Za-z_][ // Returns an error if the value is not in the format: ${{ secrets.NAME }} or ${{ secrets.NAME || secrets.NAME2 }} func validateSecretsExpression(key, value string) error { if !secretsExpressionPattern.MatchString(value) { - secretsValidationLog.Printf("Invalid secret expression for key %s: %s", key, value) - return fmt.Errorf("jobs.secrets.%s must be a GitHub Actions expression with secrets reference (e.g., '${{ secrets.MY_SECRET }}' or '${{ secrets.SECRET1 || secrets.SECRET2 }}'), got: %q", key, value) + secretsValidationLog.Printf("Invalid secret expression for key %s", key) + return fmt.Errorf("jobs.secrets.%s must be a GitHub Actions expression with secrets reference (e.g., '${{ secrets.MY_SECRET }}' or '${{ secrets.SECRET1 || secrets.SECRET2 }}')", key) } secretsValidationLog.Printf("Valid secret expression for key %s", key) return nil diff --git a/pkg/workflow/secrets_validation_test.go b/pkg/workflow/secrets_validation_test.go index 13c77c1159..b273fa6198 100644 --- a/pkg/workflow/secrets_validation_test.go +++ b/pkg/workflow/secrets_validation_test.go @@ -64,24 +64,28 @@ func TestSecretsExpressionPattern(t *testing.T) { } // TestValidateSecretsExpressionErrorMessages tests that error messages are descriptive +// 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 + name string + key string + value string + expectedInErrs []string + notExpectedInErrs []string }{ { - name: "plaintext shows value in error", - key: "token", - value: "plaintext", - expectedInErrs: []string{"plaintext", "jobs.secrets.token"}, + name: "plaintext does NOT show value in error", + key: "token", + value: "plaintext", + expectedInErrs: []string{"jobs.secrets.token"}, + notExpectedInErrs: []string{"plaintext"}, }, { - name: "env context shows value in error", - key: "api_key", - value: "${{ env.TOKEN }}", - expectedInErrs: []string{"${{ env.TOKEN }}", "jobs.secrets.api_key"}, + name: "env context does NOT show value in error", + key: "api_key", + value: "${{ env.TOKEN }}", + expectedInErrs: []string{"jobs.secrets.api_key"}, + notExpectedInErrs: []string{"${{ env.TOKEN }}"}, }, { name: "key name in error", @@ -102,10 +106,11 @@ func TestValidateSecretsExpressionErrorMessages(t *testing.T) { expectedInErrs: []string{"${{ secrets.SECRET1 || secrets.SECRET2 }}"}, }, { - name: "mixed context error", - key: "token", - value: "${{ secrets.TOKEN || env.FALLBACK }}", - expectedInErrs: []string{"${{ secrets.TOKEN || env.FALLBACK }}"}, + name: "mixed context error does NOT show value", + key: "token", + value: "${{ secrets.TOKEN || env.FALLBACK }}", + expectedInErrs: []string{"jobs.secrets.token"}, + notExpectedInErrs: []string{"${{ secrets.TOKEN || env.FALLBACK }}"}, }, } @@ -121,6 +126,11 @@ func TestValidateSecretsExpressionErrorMessages(t *testing.T) { t.Errorf("Expected error to contain %q, got: %s", expected, errMsg) } } + for _, notExpected := range tt.notExpectedInErrs { + if strings.Contains(errMsg, notExpected) { + t.Errorf("Expected error NOT to contain sensitive value %q, but it does. Got: %s", notExpected, errMsg) + } + } }) } }