CI: drop setup-command input from reusable community-world workflows#1828
Open
CI: drop setup-command input from reusable community-world workflows#1828
Conversation
The community-world matrix is produced by running scripts/create-community-worlds-matrix.mjs in the fork PR's checkout, so any field on it is attacker-controlled. Forwarding matrix.world.setup-command into the reusable workflow and eval-ing it let a malicious fork PR execute arbitrary shell on the runner. Replace the pass-through with a hardcoded per-world-id case in the reusable workflows (only turso currently needs a setup step) and drop the setup field from the matrix generator.
🦋 Changeset detectedLatest commit: 9cf6b2b The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
Contributor
🧪 E2E Test Results✅ All tests passed Summary
Details by Category✅ 💻 Local Development
✅ 📦 Local Production
✅ 🐘 Local Postgres
✅ 🪟 Windows
✅ 📋 Other
❌ Some E2E test jobs failed:
Check the workflow run for details. |
Contributor
📊 Benchmark Results
workflow with no steps💻 Local Development
workflow with 1 step💻 Local Development
workflow with 10 sequential steps💻 Local Development
workflow with 25 sequential steps💻 Local Development
workflow with 50 sequential steps💻 Local Development
Promise.all with 10 concurrent steps💻 Local Development
Promise.all with 25 concurrent steps💻 Local Development
Promise.all with 50 concurrent steps💻 Local Development
Promise.race with 10 concurrent steps💻 Local Development
Promise.race with 25 concurrent steps💻 Local Development
Promise.race with 50 concurrent steps💻 Local Development
workflow with 10 sequential data payload steps (10KB)💻 Local Development
workflow with 25 sequential data payload steps (10KB)💻 Local Development
workflow with 50 sequential data payload steps (10KB)💻 Local Development
workflow with 10 concurrent data payload steps (10KB)💻 Local Development
workflow with 25 concurrent data payload steps (10KB)💻 Local Development
workflow with 50 concurrent data payload steps (10KB)💻 Local Development
Stream Benchmarks (includes TTFB metrics)workflow with stream💻 Local Development
stream pipeline with 5 transform steps (1MB)💻 Local Development
10 parallel streams (1MB each)💻 Local Development
fan-out fan-in 10 streams (1MB each)💻 Local Development
SummaryFastest Framework by WorldWinner determined by most benchmark wins
Fastest World by FrameworkWinner determined by most benchmark wins
Column Definitions
Worlds:
❌ Some benchmark jobs failed:
Check the workflow run for details. |
Contributor
There was a problem hiding this comment.
Pull request overview
Removes an attacker-controlled setup-command field from the community-world CI matrix/workflows to prevent command injection via eval in reusable workflows.
Changes:
- Drop
setup-commandfromcreate-community-worlds-matrix.mjsoutput and stop forwarding it fromtests.yml/benchmarks.yml. - Remove
setup-commandinput from reusable workflows and replaceevalwith a hardcoded per-world-idsetup case (currently onlyturso). - Add an (empty) changeset entry for the CI-only change.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/create-community-worlds-matrix.mjs | Removes setup-command from generated matrix JSON. |
| .github/workflows/tests.yml | Stops passing matrix.world.setup-command into the reusable E2E workflow. |
| .github/workflows/e2e-community-world.yml | Removes setup-command input and replaces eval with a world-id case statement. |
| .github/workflows/benchmarks.yml | Stops passing matrix.world.setup-command into the reusable benchmark workflow. |
| .github/workflows/benchmark-community-world.yml | Removes setup-command input and replaces eval with a world-id case statement. |
| .changeset/drop-setup-command-input.md | Adds an empty changeset file to document the CI change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+110
to
113
| # Per-world setup. Hardcoded (not taken from the matrix) so a malicious | ||
| # fork PR cannot smuggle arbitrary shell through matrix.world.setup-command. | ||
| - name: Run setup command | ||
| if: ${{ inputs.setup-command != '' }} | ||
| env: |
There was a problem hiding this comment.
The step name still implies an arbitrary command is executed, but the logic is now a fixed per-world switch. Consider renaming this step (e.g., "Per-world setup") and optionally adding an if so it only runs when the world actually needs setup, to reduce log noise for the common case.
Contributor
Author
There was a problem hiding this comment.
Renamed to 'Per-world setup' in 9cf6b2b. Skipping the optional if-guard since the default case just echoes and the per-world-id list already lives in one place (the case statement itself).
Comment on lines
+114
to
117
| # Per-world setup. Hardcoded (not taken from the matrix) so a malicious | ||
| # fork PR cannot smuggle arbitrary shell through matrix.world.setup-command. | ||
| - name: Run setup command | ||
| if: ${{ inputs.setup-command != '' }} | ||
| env: |
There was a problem hiding this comment.
The step name still implies an arbitrary command is executed, but the logic is now a fixed per-world switch. Consider renaming this step (e.g., "Per-world setup") and optionally adding an if so it only runs when the world actually needs setup, to reduce log noise for the common case.
Contributor
Author
There was a problem hiding this comment.
Renamed to 'Per-world setup' in 9cf6b2b. Skipping the optional if-guard since the default case just echoes and the per-world-id list already lives in one place (the case statement itself).
Addresses Copilot review feedback: the step no longer executes an arbitrary command, so the old name was misleading.
pranaygp
added
the
backport-stable
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.
Summary
setup-commandinput from the reusablee2e-community-world.ymlandbenchmark-community-world.ymlworkflows and stop forwardingmatrix.world.setup-commandfromtests.yml/benchmarks.yml.evalwith a hardcoded per-world-idcase statement (onlytursocurrently needs a setup step).setup-commandfield fromscripts/create-community-worlds-matrix.mjsoutput.The community-world matrix is generated by running that script in the fork PR's checkout, so every field on it is attacker-controlled. Forwarding the value into the reusable workflow and
eval-ing it inside allowed a malicious fork PR to execute arbitrary shell on the runner.Test plan
tursostill passes its setup step.