Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
20 changes: 20 additions & 0 deletions pkg/constants/constants.go
Original file line number Diff line number Diff line change
Expand Up @@ -547,6 +547,26 @@ var AllowedExpressions = []string{
"github.workspace",
} // needs., steps. already allowed

// DangerousPropertyNames contains JavaScript built-in property names that are blocked
// in GitHub Actions expressions to prevent prototype pollution and traversal attacks.
// This list matches the DANGEROUS_PROPS list in actions/setup/js/runtime_import.cjs
// See PR #14826 for context on these security measures.
var DangerousPropertyNames = []string{
"constructor",
"__proto__",
"prototype",
"__defineGetter__",
"__defineSetter__",
"__lookupGetter__",
"__lookupSetter__",
"hasOwnProperty",
"isPrototypeOf",
"propertyIsEnumerable",
"toString",
"valueOf",
"toLocaleString",
}

const AgentJobName JobName = "agent"
const ActivationJobName JobName = "activation"
const PreActivationJobName JobName = "pre_activation"
Expand Down
314 changes: 314 additions & 0 deletions pkg/workflow/expression_safety_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -430,3 +430,317 @@ func TestUnauthorizedExpressionFuzzyMatchSuggestions(t *testing.T) {
})
}
}

// TestValidateExpressionForDangerousProps tests that dangerous JavaScript property names
// are blocked at compile time to prevent prototype pollution attacks (PR #14826)
func TestValidateExpressionForDangerousProps(t *testing.T) {
tests := []struct {
name string
expression string
expectError bool
errorProp string // The dangerous property that should be mentioned in error
}{
// Test constructor blocking
{
name: "block_constructor_simple",
expression: "github.constructor",
expectError: true,
errorProp: "constructor",
},
{
name: "block_constructor_in_chain",
expression: "github.event.constructor",
expectError: true,
errorProp: "constructor",
},
{
name: "block_constructor_with_inputs",
expression: "inputs.constructor",
expectError: true,
errorProp: "constructor",
},
{
name: "block_constructor_with_needs",
expression: "needs.job.constructor",
expectError: true,
errorProp: "constructor",
},

// Test __proto__ blocking
{
name: "block_proto_simple",
expression: "github.__proto__",
expectError: true,
errorProp: "__proto__",
},
{
name: "block_proto_in_chain",
expression: "github.event.__proto__",
expectError: true,
errorProp: "__proto__",
},
{
name: "block_proto_with_inputs",
expression: "inputs.__proto__",
expectError: true,
errorProp: "__proto__",
},

// Test prototype blocking
{
name: "block_prototype_simple",
expression: "github.prototype",
expectError: true,
errorProp: "prototype",
},
{
name: "block_prototype_in_chain",
expression: "github.event.prototype",
expectError: true,
errorProp: "prototype",
},
{
name: "block_prototype_with_inputs",
expression: "inputs.prototype",
expectError: true,
errorProp: "prototype",
},

// Test __defineGetter__ blocking
{
name: "block_defineGetter",
expression: "github.__defineGetter__",
expectError: true,
errorProp: "__defineGetter__",
},

// Test __defineSetter__ blocking
{
name: "block_defineSetter",
expression: "github.__defineSetter__",
expectError: true,
errorProp: "__defineSetter__",
},

// Test __lookupGetter__ blocking
{
name: "block_lookupGetter",
expression: "github.__lookupGetter__",
expectError: true,
errorProp: "__lookupGetter__",
},

// Test __lookupSetter__ blocking
{
name: "block_lookupSetter",
expression: "github.__lookupSetter__",
expectError: true,
errorProp: "__lookupSetter__",
},

// Test hasOwnProperty blocking
{
name: "block_hasOwnProperty",
expression: "github.hasOwnProperty",
expectError: true,
errorProp: "hasOwnProperty",
},

// Test isPrototypeOf blocking
{
name: "block_isPrototypeOf",
expression: "github.isPrototypeOf",
expectError: true,
errorProp: "isPrototypeOf",
},

// Test propertyIsEnumerable blocking
{
name: "block_propertyIsEnumerable",
expression: "github.propertyIsEnumerable",
expectError: true,
errorProp: "propertyIsEnumerable",
},

// Test toString blocking
{
name: "block_toString",
expression: "github.toString",
expectError: true,
errorProp: "toString",
},

// Test valueOf blocking
{
name: "block_valueOf",
expression: "github.valueOf",
expectError: true,
errorProp: "valueOf",
},

// Test toLocaleString blocking
{
name: "block_toLocaleString",
expression: "github.toLocaleString",
expectError: true,
errorProp: "toLocaleString",
},

// Test blocking in array access patterns (bracket notation)
{
name: "block_constructor_in_array_access",
expression: "github.event.release.assets[0].constructor",
expectError: true,
errorProp: "constructor",
},
{
name: "block_proto_in_array_access",
expression: "github.event.release.assets[0].__proto__",
expectError: true,
errorProp: "__proto__",
},

// Test that safe expressions are allowed
{
name: "allow_safe_github_actor",
expression: "github.actor",
expectError: false,
},
{
name: "allow_safe_github_repository",
expression: "github.repository",
expectError: false,
},
{
name: "allow_safe_github_event_issue_number",
expression: "github.event.issue.number",
expectError: false,
},
{
name: "allow_safe_needs_outputs",
expression: "needs.job-id.outputs.result",
expectError: false,
},
{
name: "allow_safe_steps_outputs",
expression: "steps.step-id.outputs.value",
expectError: false,
},
{
name: "allow_safe_inputs",
expression: "inputs.repository",
expectError: false,
},
{
name: "allow_safe_env",
expression: "env.MY_VAR",
expectError: false,
},
{
name: "allow_safe_array_access",
expression: "github.event.release.assets[0].id",
expectError: false,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
err := validateExpressionForDangerousProps(tt.expression)

if tt.expectError {
if err == nil {
t.Errorf("Expected error for expression '%s', but got nil", tt.expression)
return
}

// Verify error message contains the dangerous property name
errMsg := err.Error()
if !strings.Contains(errMsg, tt.errorProp) {
t.Errorf("Error message should mention dangerous property '%s', got: %s",
tt.errorProp, errMsg)
}

// Verify error message mentions the expression
if !strings.Contains(errMsg, tt.expression) {
t.Errorf("Error message should mention expression '%s', got: %s",
tt.expression, errMsg)
}
} else {
if err != nil {
t.Errorf("Expected no error for safe expression '%s', but got: %v",
tt.expression, err)
}
}
})
}
}

// TestValidateExpressionSafetyWithDangerousProps tests that dangerous properties
// are blocked in full workflow markdown content
func TestValidateExpressionSafetyWithDangerousProps(t *testing.T) {
tests := []struct {
name string
content string
expectError bool
errorProp string
}{
{
name: "block_constructor_in_markdown",
content: "The value is ${{ github.constructor }}",
expectError: true,
errorProp: "constructor",
},
{
name: "block_proto_in_markdown",
content: "The value is ${{ inputs.__proto__ }}",
expectError: true,
errorProp: "__proto__",
},
{
name: "block_prototype_in_markdown",
content: "The value is ${{ github.event.prototype }}",
expectError: true,
errorProp: "prototype",
},
{
name: "block_toString_in_markdown",
content: "The value is ${{ github.toString }}",
expectError: true,
errorProp: "toString",
},
{
name: "allow_safe_expressions_in_markdown",
content: "Issue number: ${{ github.event.issue.number }}, Actor: ${{ github.actor }}",
expectError: false,
},
{
name: "block_multiple_dangerous_props",
content: "First: ${{ github.constructor }}, Second: ${{ inputs.__proto__ }}",
expectError: true,
errorProp: "constructor", // Should catch at least one
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
err := validateExpressionSafety(tt.content)

if tt.expectError {
if err == nil {
t.Errorf("Expected error for content with dangerous property, but got nil")
return
}

errMsg := err.Error()
if !strings.Contains(errMsg, tt.errorProp) {
t.Errorf("Error message should mention dangerous property '%s', got: %s",
tt.errorProp, errMsg)
}
} else {
if err != nil {
t.Errorf("Expected no error for safe content, but got: %v", err)
}
}
})
}
}
40 changes: 40 additions & 0 deletions pkg/workflow/expression_validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -194,10 +194,50 @@ type ExpressionValidationOptions struct {
UnauthorizedExpressions *[]string
}

// validateExpressionForDangerousProps checks if an expression contains dangerous JavaScript
// property names that could be used for prototype pollution or traversal attacks.
// This matches the JavaScript runtime validation in actions/setup/js/runtime_import.cjs
// Returns an error if dangerous properties are found.
func validateExpressionForDangerousProps(expression string) error {
trimmed := strings.TrimSpace(expression)

// Split expression into parts handling both dot notation (e.g., "github.event.issue")
// and bracket notation (e.g., "release.assets[0].id")
// Filter out numeric indices (e.g., "0" in "assets[0]")
parts := regexp.MustCompile(`[.\[\]]+`).Split(trimmed, -1)

for _, part := range parts {
// Skip empty parts and numeric indices
if part == "" || regexp.MustCompile(`^\d+$`).MatchString(part) {
continue
}

// Check if this part is a dangerous property name
for _, dangerousProp := range constants.DangerousPropertyNames {
if part == dangerousProp {
return NewValidationError(
"expressions",
fmt.Sprintf("dangerous property name '%s' found in expression", dangerousProp),
fmt.Sprintf("expression '%s' contains the dangerous property name '%s'", expression, dangerousProp),
fmt.Sprintf("Remove the dangerous property '%s' from the expression. Property names like constructor, __proto__, prototype, and similar JavaScript built-ins are blocked to prevent prototype pollution attacks. See PR #14826 for more details.", dangerousProp),
)
}
}
}

return nil
}

// validateSingleExpression validates a single literal expression
func validateSingleExpression(expression string, opts ExpressionValidationOptions) error {
expression = strings.TrimSpace(expression)

// First, check for dangerous JavaScript property names that could be used for
// prototype pollution or traversal attacks (PR #14826)
if err := validateExpressionForDangerousProps(expression); err != nil {
return err
}

// Check if this expression is in the allowed list
allowed := false

Expand Down
Loading