diff --git a/pkg/workflow/strict_mode_test.go b/pkg/workflow/strict_mode_test.go index 81ff8753064..ac3f7c6941c 100644 --- a/pkg/workflow/strict_mode_test.go +++ b/pkg/workflow/strict_mode_test.go @@ -887,3 +887,98 @@ engine: copilot }) } } + +func TestStrictModePullRequestTarget(t *testing.T) { + tests := []struct { + name string + content string + setCLIStrictMode bool + expectError bool + errorMsg string + }{ + { + name: "pull_request_target trigger is refused in strict mode", + content: `--- +on: pull_request_target +permissions: + contents: read +engine: copilot +--- + +# Test Workflow`, + setCLIStrictMode: true, + expectError: true, + errorMsg: "strict mode: 'pull_request_target' trigger is not allowed for security reasons", + }, + { + name: "pull_request_target with types is refused in strict mode", + content: `--- +on: + pull_request_target: + types: [opened, synchronize] +permissions: + contents: read +engine: copilot +--- + +# Test Workflow`, + setCLIStrictMode: true, + expectError: true, + errorMsg: "strict mode: 'pull_request_target' trigger is not allowed for security reasons", + }, + { + name: "pull_request trigger is allowed in strict mode", + content: `--- +on: pull_request +permissions: + contents: read +engine: copilot +--- + +# Test Workflow`, + setCLIStrictMode: true, + expectError: false, + }, + { + name: "pull_request_target is allowed in non-strict mode", + content: `--- +on: pull_request_target +strict: false +permissions: + contents: read +engine: copilot +--- + +# Test Workflow`, + setCLIStrictMode: false, + expectError: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tmpDir := testutil.TempDir(t, "strict-prt-test") + + testFile := filepath.Join(tmpDir, "test-workflow.md") + if err := os.WriteFile(testFile, []byte(tt.content), 0644); err != nil { + t.Fatal(err) + } + + compiler := NewCompiler() + if tt.setCLIStrictMode { + compiler.SetStrictMode(true) + } + err := compiler.CompileWorkflow(testFile) + + if tt.expectError && err == nil { + t.Error("Expected compilation to fail but it succeeded") + } else if !tt.expectError && err != nil { + t.Errorf("Expected compilation to succeed but it failed: %v", err) + } else if tt.expectError && err != nil && tt.errorMsg != "" { + if !strings.Contains(err.Error(), tt.errorMsg) { + t.Errorf("Expected error containing '%s', got '%s'", tt.errorMsg, err.Error()) + } + } + }) + } +} diff --git a/pkg/workflow/strict_mode_validation.go b/pkg/workflow/strict_mode_validation.go index cff339b5d41..99de0c243dc 100644 --- a/pkg/workflow/strict_mode_validation.go +++ b/pkg/workflow/strict_mode_validation.go @@ -11,6 +11,7 @@ // - Network access configuration // - Top-level network configuration required for container-based MCP servers // - Bash wildcard tool usage +// - Dangerous trigger events (e.g. pull_request_target) // // # Validation Functions // @@ -19,6 +20,7 @@ // 2. validateStrictPermissions() - Refuses write permissions on sensitive scopes // 3. validateStrictNetwork() - Requires explicit network configuration // 4. validateStrictMCPNetwork() - Requires top-level network config for container-based MCP servers +// 5. validateStrictTrigger() - Refuses dangerous trigger events (e.g. pull_request_target) // // # Integration with Security Scanners // @@ -198,6 +200,53 @@ func (c *Compiler) validateStrictTools(frontmatter map[string]any) error { return nil } +// validateStrictTrigger refuses the use of pull_request_target trigger in strict mode. +// +// pull_request_target runs in the context of the base repository and has access to +// repository secrets, even when triggered from a forked pull request. This makes it +// a common vector for "pwn request" attacks where untrusted code from a fork can be +// executed with elevated privileges. Strict mode disallows this trigger to prevent +// accidental exposure of secrets to untrusted code. +// +// See: https://securitylab.github.com/research/github-actions-preventing-pwn-requests/ +func (c *Compiler) validateStrictTrigger(frontmatter map[string]any) error { + onValue, exists := frontmatter["on"] + if !exists { + return nil + } + + hasPRT := false + + switch on := onValue.(type) { + case string: + // on: pull_request_target + hasPRT = on == "pull_request_target" + case map[string]any: + // on: + // pull_request_target: ... + _, hasPRT = on["pull_request_target"] + case []any: + // on: [push, pull_request_target] + for _, item := range on { + if s, ok := item.(string); ok && s == "pull_request_target" { + hasPRT = true + break + } + } + } + + if hasPRT { + strictModeValidationLog.Printf("Trigger validation failed: pull_request_target is not allowed in strict mode") + return errors.New("strict mode: 'pull_request_target' trigger is not allowed for security reasons. " + + "This trigger runs with the privileges of the base repository and has access to repository secrets, " + + "even when triggered from a forked pull request, making it a common vector for supply chain attacks. " + + "Use 'pull_request' instead. See: https://securitylab.github.com/research/github-actions-preventing-pwn-requests/") + } + + strictModeValidationLog.Printf("Trigger validation passed") + return nil +} + // validateStrictDeprecatedFields refuses deprecated fields in strict mode func (c *Compiler) validateStrictDeprecatedFields(frontmatter map[string]any) error { // Get the list of deprecated fields from the schema @@ -361,6 +410,7 @@ func (c *Compiler) validateEnvSecretsSection(config map[string]any, sectionName // 3. validateStrictMCPNetwork() - Requires top-level network config for container-based MCP servers // 4. validateStrictTools() - Validates tools configuration (e.g., serena local mode) // 5. validateStrictDeprecatedFields() - Refuses deprecated fields +// 6. validateStrictTrigger() - Refuses dangerous trigger events (e.g. pull_request_target) // // Note: Env secrets validation (validateEnvSecrets) is called separately outside of strict mode // to emit warnings in non-strict mode and errors in strict mode. @@ -414,6 +464,13 @@ func (c *Compiler) validateStrictMode(frontmatter map[string]any, networkPermiss } } + // 6. Refuse dangerous trigger events + if err := c.validateStrictTrigger(frontmatter); err != nil { + if returnErr := collector.Add(err); returnErr != nil { + return returnErr // Fail-fast mode + } + } + strictModeValidationLog.Printf("Strict mode validation completed: error_count=%d", collector.Count()) return collector.FormattedError("strict mode") diff --git a/pkg/workflow/strict_mode_validation_test.go b/pkg/workflow/strict_mode_validation_test.go index ebc28607b28..0bbeaf55675 100644 --- a/pkg/workflow/strict_mode_validation_test.go +++ b/pkg/workflow/strict_mode_validation_test.go @@ -662,3 +662,109 @@ func TestValidateStrictCacheMemoryScope(t *testing.T) { }) } } + +// TestValidateStrictTrigger tests the validateStrictTrigger function +func TestValidateStrictTrigger(t *testing.T) { + tests := []struct { + name string + frontmatter map[string]any + expectError bool + errorMsg string + }{ + { + name: "push trigger is allowed", + frontmatter: map[string]any{ + "on": "push", + }, + expectError: false, + }, + { + name: "pull_request trigger is allowed", + frontmatter: map[string]any{ + "on": "pull_request", + }, + expectError: false, + }, + { + name: "pull_request_target as string is refused", + frontmatter: map[string]any{ + "on": "pull_request_target", + }, + expectError: true, + errorMsg: "strict mode: 'pull_request_target' trigger is not allowed for security reasons", + }, + { + name: "pull_request_target as map key is refused", + frontmatter: map[string]any{ + "on": map[string]any{ + "pull_request_target": map[string]any{ + "types": []string{"opened"}, + }, + }, + }, + expectError: true, + errorMsg: "strict mode: 'pull_request_target' trigger is not allowed for security reasons", + }, + { + name: "pull_request_target in array is refused", + frontmatter: map[string]any{ + "on": []any{"push", "pull_request_target"}, + }, + expectError: true, + errorMsg: "strict mode: 'pull_request_target' trigger is not allowed for security reasons", + }, + { + name: "pull_request_target mixed with other triggers in map is refused", + frontmatter: map[string]any{ + "on": map[string]any{ + "push": map[string]any{}, + "pull_request_target": map[string]any{}, + }, + }, + expectError: true, + errorMsg: "strict mode: 'pull_request_target' trigger is not allowed for security reasons", + }, + { + name: "no on field is allowed", + frontmatter: map[string]any{ + "engine": "copilot", + }, + expectError: false, + }, + { + name: "pull_request trigger as map key is allowed", + frontmatter: map[string]any{ + "on": map[string]any{ + "pull_request": map[string]any{ + "types": []string{"opened"}, + }, + }, + }, + expectError: false, + }, + { + name: "multiple safe triggers in array are allowed", + frontmatter: map[string]any{ + "on": []any{"push", "pull_request"}, + }, + expectError: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + compiler := NewCompiler() + err := compiler.validateStrictTrigger(tt.frontmatter) + + if tt.expectError && err == nil { + t.Error("Expected validation to fail but it succeeded") + } else if !tt.expectError && err != nil { + t.Errorf("Expected validation to succeed but it failed: %v", err) + } else if tt.expectError && err != nil && tt.errorMsg != "" { + if !strings.Contains(err.Error(), tt.errorMsg) { + t.Errorf("Expected error containing '%s', got '%s'", tt.errorMsg, err.Error()) + } + } + }) + } +}