Skip to content
Closed
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
95 changes: 95 additions & 0 deletions pkg/workflow/strict_mode_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Comment on lines +967 to +972
Copy link

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this table-driven test, compiler.SetStrictMode(true) forces strict mode for every case, which overrides strict: false in the "pull_request_target is allowed in non-strict mode" fixture (CLI strict flag takes precedence). That case will fail under the current strict-mode precedence logic. Consider adding a per-test cliStrict flag (or similar) and only enabling strict mode for the strict-mode cases, leaving it unset/false for the non-strict case so frontmatter can disable strict mode.

Copilot uses AI. Check for mistakes.
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())
}
}
})
}
}
57 changes: 57 additions & 0 deletions pkg/workflow/strict_mode_validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
//
Expand All @@ -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)
Copy link

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The file-level documentation block describing the "progressive validation" steps is now out of sync with the implementation: it lists validateStrictTrigger() as step 5, but validateStrictMode() also runs validateStrictTools() and validateStrictDeprecatedFields() (and the later comment block lists trigger as step 6). Please update this top-level list to reflect the actual validation sequence (or make it explicitly non-exhaustive) to avoid confusing future maintainers.

Suggested change
// 5. validateStrictTrigger() - Refuses dangerous trigger events (e.g. pull_request_target)
// 5. validateStrictTools() - Enforces restrictions on tool usage (e.g. bash wildcard constraints)
// 6. validateStrictDeprecatedFields() - Flags usage of deprecated workflow fields in strict mode
// 7. validateStrictTrigger() - Refuses dangerous trigger events (e.g. pull_request_target)

Copilot uses AI. Check for mistakes.
//
// # Integration with Security Scanners
//
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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")
Expand Down
106 changes: 106 additions & 0 deletions pkg/workflow/strict_mode_validation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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())
}
}
})
}
}