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
10 changes: 3 additions & 7 deletions pkg/workflow/playwright_validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,9 @@ import (

var playwrightValidationLog = logger.New("workflow:playwright_validation")

// validatePlaywrightMode warns (non-strict) or errors (strict) when the
// playwright tool is configured in MCP mode. MCP mode is deprecated; use
// mode: cli instead for token-efficient, container-free browser automation.
// validatePlaywrightMode warns when the playwright tool is configured in MCP
// mode. MCP mode is deprecated; use mode: cli instead for token-efficient,
// container-free browser automation.
func (c *Compiler) validatePlaywrightMode(workflowData *WorkflowData) error {
if workflowData == nil || workflowData.Tools == nil {
return nil
Expand All @@ -64,10 +64,6 @@ func (c *Compiler) validatePlaywrightMode(workflowData *WorkflowData) error {
"Update your prompts to run `playwright-cli <command>` in bash instead of using MCP browser tools. " +
"See: https://github.com/github/gh-aw/blob/main/docs/src/content/docs/reference/playwright.md"

if c.strictMode {
return fmt.Errorf("strict mode: %s", warningMsg)
}

fmt.Fprintln(os.Stderr, console.FormatWarningMessage(warningMsg))
c.IncrementWarningCount()
return nil
Expand Down
18 changes: 8 additions & 10 deletions pkg/workflow/playwright_validation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,18 +55,16 @@ func TestValidatePlaywrightMode(t *testing.T) {
expectWarn: false,
},
{
name: "playwright mcp mode in strict mode returns error",
tools: map[string]any{"playwright": map[string]any{"mode": "mcp"}},
strictMode: true,
expectError: true,
errorSubstr: "strict mode",
name: "playwright mcp mode in strict mode warns only",
tools: map[string]any{"playwright": map[string]any{"mode": "mcp"}},
strictMode: true,
expectWarn: true,
},
Comment on lines +58 to 62
{
name: "playwright nil (default MCP) in strict mode returns error",
tools: map[string]any{"playwright": nil},
strictMode: true,
expectError: true,
errorSubstr: "strict mode",
name: "playwright nil (default MCP) in strict mode warns only",
tools: map[string]any{"playwright": nil},
strictMode: true,
expectWarn: true,
},
}

Expand Down
14 changes: 12 additions & 2 deletions pkg/workflow/pull_request_target_validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
// 1. In strict mode: always emit a warning that pull_request_target is a very
// dangerous trigger, even when checkout: false is set, because the workflow
// still runs with full write permissions and secret access.
// Workflows can opt out by setting strict: false in frontmatter.
//
// 2. When checkout is NOT explicitly disabled (checkout: false not set):
// - In strict mode: return a hard error (extremely insecure).
Expand Down Expand Up @@ -51,6 +52,7 @@ var pullRequestTargetLog = newValidationLogger("pull_request_target")
//
// In strict mode, a warning is always emitted that pull_request_target is inherently dangerous
// even with checkout disabled, since the workflow still runs with elevated permissions.
// This strict behavior is disabled when the workflow frontmatter explicitly sets strict: false.
func (c *Compiler) validatePullRequestTargetTrigger(workflowData *WorkflowData, markdownPath string) error {
// Fast path: skip expensive YAML parsing when the On field cannot possibly contain
// a pull_request_target trigger. This avoids yaml.Unmarshal on every
Expand Down Expand Up @@ -87,10 +89,18 @@ func (c *Compiler) validatePullRequestTargetTrigger(workflowData *WorkflowData,
return nil
}

effectiveStrictMode := c.strictMode
if workflowData.RawFrontmatter != nil {
if strictBool, ok := workflowData.RawFrontmatter["strict"].(bool); ok && !strictBool {
pullRequestTargetLog.Print("Frontmatter strict: false detected, disabling strict mode error for pull_request_target validation")
effectiveStrictMode = false
}
}

// In strict mode, always emit a warning that pull_request_target is a very dangerous trigger,
// regardless of whether checkout is disabled. The workflow still runs with full write
// permissions and has access to all repository secrets.
if c.strictMode {
if effectiveStrictMode {
pullRequestTargetLog.Print("Emitting strict mode warning: pull_request_target is a very dangerous trigger")
warningMsg := "pull_request_target is a very dangerous trigger.\n" +
"This event runs with full write permissions and access to all repository secrets.\n" +
Expand Down Expand Up @@ -122,7 +132,7 @@ func (c *Compiler) validatePullRequestTargetTrigger(workflowData *WorkflowData,
"checkout: false\n\n" +
"See: https://securitylab.github.com/resources/github-actions-preventing-pwn-requests/"

if c.strictMode {
if effectiveStrictMode {
return formatCompilerError(markdownPath, "error", message, nil)
}

Expand Down
22 changes: 22 additions & 0 deletions pkg/workflow/pull_request_target_validation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,28 @@ Test workflow content.`,
errorContains: "pull_request_target trigger with checkout enabled is extremely insecure",
warningCount: 1, // dangerous-trigger warning
},
{
name: "pull_request_target with checkout enabled - strict CLI + frontmatter strict false - warning only",
frontmatter: `---
strict: false
on:
pull_request_target:
types: [opened]
tools:
github:
toolsets: [pull_requests]
permissions:
pull-requests: read
---

# PR Target Strict Opt-Out
Test workflow content.`,
filename: "prt-checkout-enabled-strict-frontmatter-opt-out.md",
strictMode: true,
expectError: false,
expectWarning: true,
warningCount: 1, // insecure-checkout warning only
},
{
name: "pull_request trigger (not target) - strict - no diagnostic",
frontmatter: `---
Expand Down