Potential fix for code scanning alert no. 226: Artifact poisoning#525
Merged
Conversation
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com>
|
Note Gemini is unable to generate a review for this pull request due to the file types involved not being currently supported. |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR hardens the npm publish workflow against artifact poisoning by forbidding artifact consumption outside trusted workflow_run invocations and removing any user-controlled run_id from the artifact download step. Sequence diagram for trusted artifact consumption in npm workflowsequenceDiagram
actor Maintainer
participant GitHubActions
participant NpmWorkflow
participant GuardStep
participant DownloadArtifactAction
participant ArtifactStore
Maintainer->>GitHubActions: trigger workflow_run or workflow_dispatch
GitHubActions->>NpmWorkflow: start npm workflow with event context
NpmWorkflow->>GuardStep: evaluate github.event_name
alt event is workflow_run
GuardStep-->>NpmWorkflow: allow artifact consumption
NpmWorkflow->>DownloadArtifactAction: download-artifact with run-id github.event.workflow_run.id
DownloadArtifactAction->>ArtifactStore: fetch artifacts for run
ArtifactStore-->>DownloadArtifactAction: return artifacts
DownloadArtifactAction-->>NpmWorkflow: artifacts extracted to artifact_dir
else event is not workflow_run
GuardStep-->>NpmWorkflow: refuse artifact consumption
NpmWorkflow->>GitHubActions: exit 1 with error
end
Flow diagram for guarded artifact download in npm workflowflowchart TD
Start[Start npm workflow]
CheckEvent[Check github.event_name]
IsWorkflowRun{Is event_name workflow_run}
GuardStep[Guard step
Refuse artifacts outside workflow_run]
Download[Download Release Assets
run-id = github.event.workflow_run.id]
Fail[Fail job
exit 1]
NextSteps[Subsequent npm publish steps]
Start --> CheckEvent --> IsWorkflowRun
IsWorkflowRun -- Yes --> Download --> NextSteps
IsWorkflowRun -- No --> GuardStep --> Fail
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- If
workflow_dispatchis still intentionally supported, consider tightening the guard step condition to explicitly targetworkflow_dispatch(e.g.if: ${{ github.event_name == 'workflow_dispatch' }}) so other non-workflow_runevent types do not get inadvertently blocked. - Now that
run-idalways usesgithub.event.workflow_run.id, you may want to remove or repurpose theinputs.run_idinput from the workflow definition and any associated UI hints to avoid confusion about unused parameters.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- If `workflow_dispatch` is still intentionally supported, consider tightening the guard step condition to explicitly target `workflow_dispatch` (e.g. `if: ${{ github.event_name == 'workflow_dispatch' }}`) so other non-`workflow_run` event types do not get inadvertently blocked.
- Now that `run-id` always uses `github.event.workflow_run.id`, you may want to remove or repurpose the `inputs.run_id` input from the workflow definition and any associated UI hints to avoid confusion about unused parameters.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Potential fix for https://github.com/Dargon789/foundry/security/code-scanning/226
The best fix is to remove untrusted artifact source selection from
workflow_dispatchby refusing manual runs unless they are tied to a trustedworkflow_runcontext, or at minimum by not accepting user-suppliedrun_id.The least-disruptive, single best change here is:
workflow_dispatchif needed for operational reasons.workflow_dispatch.download-artifactalways usegithub.event.workflow_run.idonly.This preserves existing publish logic for trusted
workflow_runinvocations, avoids changing downstream scripts, and closes both alert variants by eliminating the tainted flow from dispatch input to artifact consumption.Edits are in
.github/workflows/npm.ymlaround:Download Release Assetsstep (new guard step),run-idinDownload Release Assetsstep.No new imports/dependencies are needed.
Suggested fixes powered by Copilot Autofix. Review carefully before merging.
Summary by Sourcery
Enforce trusted workflow_run context for consuming release artifacts in the npm GitHub Actions workflow to mitigate artifact poisoning risk.
New Features:
Bug Fixes: