Add owner-only permission check to manage-gist workflow#42
Add owner-only permission check to manage-gist workflow#42caibingcheng merged 3 commits intomasterfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…flow Co-authored-by: caibingcheng <[email protected]>
Co-authored-by: caibingcheng <[email protected]>
There was a problem hiding this comment.
Pull request overview
This PR adds a critical security fix to prevent unauthorized gist modifications by restricting workflow execution to the repository owner only. Previously, any GitHub user commenting on issues could trigger gist modifications through ADD/DELETE commands.
Key changes:
- Added owner-only permission check (
github.event.comment.user.login == github.repository_owner) to workflow trigger condition - Implemented comprehensive test suite with 8 test cases validating permission logic
- Updated documentation in README.md and USAGE_EXAMPLES.md to reflect the security enhancement
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
.github/workflows/manage-gist.yml |
Added permission check to workflow condition, restricting execution to repository owner |
.github/workflows/test_workflow_logic.py |
Added should_trigger_workflow() function with test cases for owner/non-owner scenarios |
.github/workflows/README.md |
Documented the new owner-only security requirement |
.github/workflows/USAGE_EXAMPLES.md |
Updated workflow trigger conditions and troubleshooting sections with owner verification details |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 1. A comment is created on an issue | ||
| 2. The comment contains "ADD " or "DELETE " (with space after the keyword) | ||
| 2. **The comment author is the repository owner (caibingcheng)** | ||
| 3. The comment contains "ADD " or "DELETE " (with space after the keyword) |
There was a problem hiding this comment.
The documentation states "with space after the keyword" but this is misleading. The workflow condition uses contains(github.event.comment.body, 'ADD') which is a simple substring check - it will match "ADD", "ADDON", "ADDED", "BADDER", etc. The actual filtering happens later via regex pattern ADD\s+(\S+)\s+(\S+) in the Python script (lines 149-150 in manage-gist.yml), which requires whitespace. The documentation should clarify that the workflow trigger is broad (substring match) but only properly formatted commands with spaces will be processed successfully.
|
|
||
| - The workflow triggers on issue comments (when a comment is created) | ||
| - **Security**: It only runs if the comment author is the repository owner (caibingcheng) | ||
| - It only runs if the comment contains "ADD " or "DELETE " commands |
There was a problem hiding this comment.
The documentation claims commands require "ADD " or "DELETE " with a space, but this contradicts the actual workflow behavior. The workflow's contains() function performs a simple substring check and will trigger on words like "ADDON", "BADDER", "DELETES", etc. While the Python script's regex later filters to proper format, the workflow will still run (and waste CI resources) on false matches. Consider either updating the documentation to accurately describe this two-stage filtering, or improving the workflow condition to be more precise (e.g., using regex in the workflow condition itself).
| - It only runs if the comment contains "ADD " or "DELETE " commands | |
| - It triggers when the comment text contains the substrings "ADD" or "DELETE" (simple substring match), and the Python script then processes only comments that contain properly formatted `ADD <section> <URL>` or `DELETE <section> <URL>` commands |
| test_cases = [ | ||
| # (comment_author, comment_body, should_trigger, description) | ||
| ("caibingcheng", "ADD friends https://example.com/feed.xml", True, "Owner with ADD command"), | ||
| ("caibingcheng", "DELETE friends https://example.com/feed.xml", True, "Owner with DELETE command"), | ||
| ("caibingcheng", "Just a regular comment", False, "Owner without command"), | ||
| ("other-user", "ADD friends https://example.com/feed.xml", False, "Non-owner with ADD command"), | ||
| ("other-user", "DELETE friends https://example.com/feed.xml", False, "Non-owner with DELETE command"), | ||
| ("other-user", "Just a regular comment", False, "Non-owner without command"), | ||
| ("caibingcheng", "Please ADD tech https://tech.example.com/rss", True, "Owner with ADD in sentence"), | ||
| ("random-user", "Can you DELETE blog https://blog.example.com/atom.xml", False, "Non-owner with DELETE in sentence"), | ||
| ] |
There was a problem hiding this comment.
The test cases don't verify the actual behavior of the workflow condition. The workflow uses contains() which is a substring match, meaning comments like "ADDON friends url" or "BADDER text" would trigger the workflow (though they'd fail during parsing). Consider adding test cases for edge cases like "ADDON", "ADDED", "BADDER", "DELETES", etc. to demonstrate this behavior and align with the actual workflow logic. This would help document the two-stage filtering: workflow trigger (broad substring match) vs. command parsing (strict regex).
| github.event.comment.user.login == github.repository_owner && | ||
| (contains(github.event.comment.body, 'ADD') || | ||
| contains(github.event.comment.body, 'DELETE')) |
There was a problem hiding this comment.
GitHub Actions' contains() function is case-sensitive, meaning lowercase commands like "add friends url" or "delete friends url" will not trigger the workflow. Consider documenting this behavior in the troubleshooting section, or add a case-insensitive check in the workflow condition. For example: contains(github.event.comment.body, 'ADD') || contains(github.event.comment.body, 'add') or convert to uppercase before checking. This would improve user experience and prevent confusion when commands don't trigger.
The workflow triggered on any user's issue comments containing ADD/DELETE commands, allowing unauthorized gist modifications.
Changes
github.event.comment.user.login == github.repository_ownercheck to restrict workflow execution to repository owner onlyDocumentation: Updated README.md and USAGE_EXAMPLES.md to reflect owner-only access requirement
Tests: Added
should_trigger_workflow()function with 8 test cases validating permission logic (owner vs non-owner, with/without commands)Impact
Previously any GitHub user could modify the gist by commenting on issues. Now restricted to repository owner (caibingcheng) only.
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.