Skip to content

Disallow pull_request_target trigger in strict mode#19951

Closed
Copilot wants to merge 3 commits intomainfrom
copilot/update-compiler-strict-mode
Closed

Disallow pull_request_target trigger in strict mode#19951
Copilot wants to merge 3 commits intomainfrom
copilot/update-compiler-strict-mode

Conversation

Copy link
Contributor

Copilot AI commented Mar 7, 2026

  • Add validateStrictTrigger function to strict_mode_validation.go that rejects pull_request_target in strict mode
  • Call validateStrictTrigger from the validateStrictMode orchestrator (step 6)
  • Update the validateStrictMode docblock to include the new validation step
  • Update file-level documentation comments
  • Add unit tests for validateStrictTrigger in strict_mode_validation_test.go (9 test cases covering string, map, and array on forms)
  • Add integration tests in strict_mode_test.go (4 test cases for full compilation scenarios)
  • Fix failing integration test: add setCLIStrictMode field to test struct so the non-strict case doesn't inadvertently apply CLI strict mode
Original prompt

Update the compiler to dissallow the use of pull_request_target in strict mode.


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot AI changed the title [WIP] Update compiler to disallow pull_request_target in strict mode Disallow pull_request_target trigger in strict mode Mar 7, 2026
@pelikhan pelikhan marked this pull request as ready for review March 7, 2026 13:10
Copilot AI review requested due to automatic review settings March 7, 2026 13:10
@pelikhan
Copy link
Contributor

pelikhan commented Mar 7, 2026

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds strict-mode validation to reject the GitHub Actions pull_request_target trigger (a known security risk because it runs in the base-repo context with access to secrets), while leaving non-strict workflows unaffected.

Changes:

  • Implemented validateStrictTrigger() to detect pull_request_target across on: string, map, and array forms.
  • Wired trigger validation into validateStrictMode() as an additional strict-mode validation step.
  • Added unit and integration test coverage for the new trigger restriction.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
pkg/workflow/strict_mode_validation.go Adds validateStrictTrigger() and invokes it from validateStrictMode() to block pull_request_target in strict mode.
pkg/workflow/strict_mode_validation_test.go Adds unit tests covering validateStrictTrigger() behavior for multiple on: encodings.
pkg/workflow/strict_mode_test.go Adds integration tests asserting strict-mode compilation fails for pull_request_target and succeeds otherwise.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

// 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.
Comment on lines +962 to +965
compiler := NewCompiler()
compiler.SetStrictMode(true)
err := compiler.CompileWorkflow(testFile)

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.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Mar 7, 2026

@copilot fix test in https://github.com/github/gh-aw/actions/runs/22799578318/job/66139401587

Fixed in 57a6a09. The non-strict test case was incorrectly calling compiler.SetStrictMode(true) for all cases — the CLI strict flag overrides frontmatter strict: false, so the non-strict case was still running with strict mode enabled. Added a setCLIStrictMode field to the test struct so the non-strict case doesn't set the CLI strict flag.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants