Revert: permissions: {} is correct behavior - no changes needed#12684
Revert: permissions: {} is correct behavior - no changes needed#12684
Conversation
For workflows with risky triggers (issue_comment, pull_request_target,
workflow_run, pull_request_review_comment), the compiler now sets
explicit minimal permissions at the workflow level instead of using
empty permissions {}.
This follows the principle of least privilege and resolves zizmor
security warnings for workflows with risky triggers.
Changes:
- Modified generateWorkflowBody to check for risky triggers
- Added hasRiskyTriggers function to detect risky trigger types
- Recompiled all affected workflows with explicit permissions
Affected workflows (14 total):
- ai-moderator, archie, brave, cloclo, grumpy-reviewer
- mergefest, pdf-summary, plan, pr-nitpick-reviewer, q
- scout, security-review, tidy, unbloat-docs
Workflows without risky triggers continue to use permissions: {}
to maintain backward compatibility.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Improvements: - Filter out YAML comments when detecting risky triggers - Add comprehensive unit tests for hasRiskyTriggers function - Test coverage includes all risky trigger types and edge cases The hasRiskyTriggers function now: - Skips comment lines (starting with #) - Checks each non-comment line individually - Prevents false positives from triggers mentioned in comments Test coverage: - All 4 risky trigger types (issue_comment, pull_request_target, workflow_run, pull_request_review_comment) - Safe triggers (push, pull_request, issues, schedule, workflow_dispatch) - Edge cases (triggers in comments, multiple triggers) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot docs say: You can use the following syntax to disable permissions for all of the available permissions: permissions: {} |
You're absolutely right. I misunderstood the GitHub Actions permissions behavior. My changes actually made things LESS secure by replacing The original design was correct - workflow-level |
The original behavior was correct. Per GitHub Actions documentation,
`permissions: {}` explicitly disables all permissions at workflow level,
which is MORE secure than setting explicit read permissions.
The previous commits (4f3089e, bf832cd, eecd021) incorrectly changed
workflow-level permissions from `permissions: {}` (no permissions) to
explicit read permissions, which actually reduced security by granting
more permissions than necessary.
Reverted changes:
- pkg/workflow/compiler_yaml.go: Restored original logic that always
uses `permissions: {}` at workflow level
- pkg/workflow/tools.go: Removed hasRiskyTriggers() function
- pkg/workflow/risky_triggers_test.go: Removed test file
- Recompiled all workflows to restore `permissions: {}` at workflow level
The correct security pattern is:
- Workflow level: `permissions: {}` (no permissions)
- Job level: Explicit minimal permissions for each job
- Safe-outputs jobs: Elevated permissions only where needed
Thanks to @pelikhan for catching this misunderstanding.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
This PR reverts the previous changes that incorrectly modified workflow-level permissions.
Problem
The original issue and my initial implementation were based on a misunderstanding of GitHub Actions permissions behavior:
permissions: {}defaults to ALL permissions ❌permissions: {}explicitly disables all permissions ✅Per GitHub Actions documentation:
What This PR Does
Reverts commits 4f3089e, bf832cd, and eecd021 which incorrectly changed workflow-level permissions.
The original compiler behavior was correct and MORE secure:
My previous changes incorrectly changed this to:
This actually REDUCED security by granting read permissions at workflow level that weren't needed.
Changes Made
pkg/workflow/compiler_yaml.go: Returns to always usingpermissions: {}at workflow levelpkg/workflow/tools.goadditions: Deleted thehasRiskyTriggers()functionpkg/workflow/risky_triggers_test.go: Removed unnecessary test filepermissions: {}at workflow levelAffected Workflows
All workflows restored to correct
permissions: {}:ai-moderator, archie, brave, ci-doctor, cloclo, dev-hawk, grumpy-reviewer, mergefest, pdf-summary, plan, pr-nitpick-reviewer, q, scout, security-review, tidy, unbloat-docs
Conclusion
The original issue report was based on a misunderstanding. The workflows were already following security best practices by using
permissions: {}at the workflow level. No changes to permissions were needed.Thanks to @pelikhan for catching this error.
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.