Conversation
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR implements a security mitigation for the demo workflow by splitting it into two separate workflows following the workflow_run pattern to prevent "pwn request" attacks. The original single workflow that executed untrusted PR code with write permissions has been replaced with a secure two-workflow architecture: a low-privilege workflow that executes untrusted code from PRs, and a high-privilege workflow that only handles validated artifacts.
Changes:
- Split the single
demo.ymlworkflow intodemo-generate.yml(low-privilege, executes untrusted code) anddemo-publish.yml(high-privilege, handles artifacts only) - Added comprehensive documentation explaining the security challenges and mitigation strategies for workflows that write to pull requests
- Implemented artifact validation in the publish workflow to prevent malicious artifact injection
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
.github/workflows/demo.yml |
Removed original monolithic workflow that had security vulnerabilities |
.github/workflows/demo-generate.yml |
New low-privilege workflow that executes untrusted PR code with restricted permissions and persist-credentials: false |
.github/workflows/demo-publish.yml |
New high-privilege workflow triggered by workflow_run that validates artifacts and commits results |
docs/readme.md |
Added extensive documentation explaining the security pitfalls and mitigation strategies for PR-writing workflows |
Comments suppressed due to low confidence (2)
.github/workflows/demo-publish.yml:47
- The unzip commands do not validate the integrity or structure of the extracted files. An attacker could potentially craft malicious zip files with path traversal attacks (e.g., files with
../../in their names). Consider usingunzip -qwith additional safeguards, or verify that extracted files only contain expected filenames before proceeding. You could add validation like checking thatdemo-gif/demo.gifexists andpr-metadata/numberandpr-metadata/head_refexist with no other unexpected files.
run: |
mkdir -p demo-gif pr-metadata
unzip -d demo-gif/ demo-gif.zip
unzip -d pr-metadata/ pr-metadata.zip
.github/workflows/demo-publish.yml:81
- After extracting the artifacts, the workflow immediately copies the demo.gif file without verifying it exists or is a valid file. Consider adding validation to check that
demo-gif/demo.gifexists and has a reasonable size before copying it. This would catch potential issues with corrupted artifacts or unexpected archive structures.
- name: Copy demo GIF and commit
run: |
cp demo-gif/demo.gif docs/demo.gif
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
docs/readme.md:67
- Inconsistent terminology: This section refers to demo-dispatch.yml as "the trigger shim", but the document header calls it "Zero-privilege dispatcher". Consider using consistent terminology throughout the document, preferably "dispatcher" since that matches the filename and is more descriptive.
- Triggered by `workflow_dispatch` with a `pr_number` input — can only be invoked by users with write access or by the trigger shim
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| This is a common attack pattern known as a "pwn request"; GitHub's default PR security has mitigations to prevent this for `pull_request` workflows by disabling write permissions. But for PRs that need additional write permissions (e.g., to commit the generated GIF), it's important to implement additional protections. | ||
|
|
||
| The demo workflows use `workflow_run`, which was introduced to enable scenarios that require building untrusted code and also writing to update the PR (e.g., committing the generated GIF, or code coverage reports). |
There was a problem hiding this comment.
The statement "The demo workflows use workflow_run" is slightly misleading. The solution uses both workflow_dispatch (for controlled dispatching from demo-dispatch to demo-generate) and workflow_run (for triggering demo-publish after demo-generate completes). Consider clarifying this to say "The demo workflows use a combination of workflow_dispatch and workflow_run" to better reflect the two-stage dispatch mechanism.
This issue also appears on line 67 of the same file.
| The demo workflows use `workflow_run`, which was introduced to enable scenarios that require building untrusted code and also writing to update the PR (e.g., committing the generated GIF, or code coverage reports). | |
| The demo workflows use a combination of `workflow_dispatch` and `workflow_run`, which were introduced to enable scenarios that require building untrusted code and also writing to update the PR (e.g., committing the generated GIF, or code coverage reports). |
| @@ -0,0 +1,24 @@ | |||
| name: demo-trigger | |||
There was a problem hiding this comment.
The workflow name is "demo-trigger" but the filename is "demo-dispatch.yml". This inconsistency could cause confusion. Consider renaming the workflow name to "demo-dispatch" to match the filename, or vice versa. The documentation in docs/readme.md refers to this as "demo-dispatch.yml".
| name: demo-trigger | |
| name: demo-dispatch |
| exit 1 | ||
| fi | ||
|
|
||
| if ! echo "$head_ref" | grep -qE '^[a-zA-Z0-9._/-]+$'; then |
There was a problem hiding this comment.
The head_ref validation regex could be more restrictive to prevent edge cases. Consider disallowing branch names that start with dots, slashes, or contain consecutive slashes, as these could potentially cause issues in git operations or URL construction. For example: ^[a-zA-Z0-9][a-zA-Z0-9._-]*(/[a-zA-Z0-9._-]+)*$. This ensures branch names start with an alphanumeric character and don't have leading/trailing slashes or consecutive slashes.
| if ! echo "$head_ref" | grep -qE '^[a-zA-Z0-9._/-]+$'; then | |
| if ! echo "$head_ref" | grep -qE '^[a-zA-Z0-9][a-zA-Z0-9._-]*(/[a-zA-Z0-9._-]+)*$'; then |
| - name: Trigger demo generation | ||
| env: | ||
| GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| run: | | ||
| gh workflow run demo-generate.yml \ | ||
| --repo ${{ github.repository }} \ | ||
| -f pr_number=${{ github.event.issue.number }} |
There was a problem hiding this comment.
Consider adding a reaction to the comment to provide immediate feedback to the user that their request was received. The previous demo.yml workflow had an "eyes" reaction step that gave users confirmation the workflow was triggered. Without this, users may be unsure if their /demo command was processed.
Summary
Replaces the single
demo.ymlworkflow with a three-workflow architecture to mitigate a pwn request vulnerability (code scanning alerts #4, #6). The/democomment UX is preserved.Vulnerability
The original
demo.ymlworkflow combined anissue_commenttrigger with an explicit checkout of the PR head ref. This is a pwn request — the workflow ran in a privileged context (contents: write, access toCI_BOT_APP_ID/CI_BOT_APP_PRIVATE_KEYsecrets) while executing attacker-controlled code (docs/demo-setup.sh,docs/demo.tape). Any GitHub user who could comment on a PR could trigger it with/demo.Risk factors:
GITHUB_TOKENactions/checkoutstores credentials on disk (persist-credentials: true), making the token available to any process in subsequent stepsSolution
Split into three workflows, each with a single responsibility and minimal privilege:
sequenceDiagram participant U as Collaborator participant D as demo-dispatch.yml<br/>issue_comment<br/>🔒 actions: write participant G as demo-generate.yml<br/>workflow_dispatch<br/>🔒 contents: read participant P as demo-publish.yml<br/>workflow_run<br/>🔓 contents: write + secrets U->>D: Comments "/demo" on PR Note over D: Validates author_association<br/>(OWNER, MEMBER, COLLABORATOR)<br/>No checkout, no code execution D->>G: gh workflow run -f pr_number=N Note over G: Checks out PR code<br/>(persist-credentials: false)<br/>Runs demo-setup.sh + VHS<br/>⚠️ Untrusted code runs here G-->>G: Uploads GIF + PR metadata artifacts G->>P: workflow_run (completed) Note over P: Downloads & validates artifacts<br/>Creates GitHub App token<br/>Commits GIF, posts comment<br/>✅ No untrusted code P-->>U: Sticky PR comment with demo GIFdemo-dispatch.ymlissue_commentactions: writedemo-generateviaworkflow_dispatch. No checkout, no code execution, no secrets.demo-generate.ymlworkflow_dispatchcontents: read,pull-requests: readpersist-credentials: false, runsdemo-setup.shand VHS, uploads GIF + PR metadata as artifacts. No access to app secrets. Can only be invoked by users with write access (or by the dispatch shim).demo-publish.ymlworkflow_runcontents: write,pull-requests: write^[a-zA-Z0-9._/-]+$), creates GitHub App token, commits GIF to PR branch, posts sticky comment. Never executes untrusted code.Why three workflows instead of two?
The previous iteration used
issue_commentdirectly ondemo-generate.ymlwith anauthor_associationguard. While this prevented untrusted users from triggering the workflow,issue_commentstill runs in a privileged context — theGITHUB_TOKENhas whatever permissions are declared, and those permissions are available to untrusted code after checkout.By adding a zero-privilege dispatch shim:
issue_commenthandler has zero attack surface — it never checks out code, never runs scripts, and its only permission isactions: write(dispatch other workflows)demo-generate.ymlusesworkflow_dispatch, which GitHub restricts to users with write access — the trust boundary is enforced by GitHub's permission model, not by anifconditiondemo-generate.ymlwhich has no secretsSecurity properties
demo.yml)workflow_dispatch)GITHUB_TOKENavailable and persisted on diskGITHUB_TOKENis read-only andpersist-credentials: falseprevents disk persistencetrue) — stored on diskpersist-credentials: false— not stored on diskFiles changed
.github/workflows/demo.yml.github/workflows/demo-dispatch.ymlissue_commentshim.github/workflows/demo-generate.ymlworkflow_dispatchGIF generator.github/workflows/demo-publish.ymlworkflow_runpublisherdocs/readme.mdReferences
Testing
/demoon a test PR as a collaborator → verify GIF is generated, committed, and comment is posted/demoas a non-collaborator → verify workflow does not triggerdemo-generate.ymldirectly via Actions tab orgh workflow run→ verify it works independently of the comment shimdemo-publish.ymlrejects artifacts with non-numeric PR numbers or invalid head refs