Repository navigation
Hardening patches - #63
Merged
Merged
Conversation
Contributor
Author
|
I just signed the CLA 😅 |
Contributor
Author
|
I have signed the CLA! |
Contributor
Author
|
I definitely signed it lol 🥀 
|
Contributor
Author
|
I have signed the CLA! |
Edouard-chin
approved these changes
Sep 17, 2026
Edouard-chin
force-pushed
the
patches
branch
from
September 18, 2026 12:39
31d0e70 to
89b1661
Compare
`CLI#release` globbed `.gem` files and interpolated each one into a single command string, so `Open3.capture2e` handed the whole thing to a shell. Those filenames come from artifacts the unprivileged compile job uploads (`pkg/*.gem`, so any file the build produces, not just the ones `Gem::PackageTask` names), and the release job downloads them with their basenames intact, after RubyGems credentials are configured. A basename such as `hello$(touch injected).gem` therefore executed in the privileged publishing job. The `extname == ".gem"` check does not prevent that, it only skips non-gem files. Passing the filename as its own argv element means no shell is involved and the name can never be parsed as syntax. The regression test runs the real release path against a fake `gem` on PATH rather than stubbing the call, so it fails on the old code both ways: the planted command runs, and `gem` receives `pkg/hello.gem` instead of the literal basename. Reported as https://vault.shopify.io/issues/78390 (CWE-78). Co-authored-by: Aaron Patterson <aaron.patterson@shopify.com>
`run_rake_tasks!` joined the ruby binary, the rake executable, the task names, the rakelib dir and the patch path into one string and handed it to `system`. None of those are attacker-controlled the way the release job's artifact filenames were, so this is hardening rather than a fix: whether a shell gets involved currently depends on whether any of those interpolated paths happens to contain a shell metacharacter, which is a property of where the gem is installed rather than of this code. Passing an argv list removes the question. It also means an install path containing a space now works, which the string form silently broke: Ruby splits a metacharacter-free command string on whitespace itself. Co-authored-by: Aaron Patterson <aaron.patterson@shopify.com>
The argv change stops a crafted artifact filename reaching a shell, but it leaves the release job publishing whatever `.gem` files the compile job happened to produce. The compile job runs without publishing credentials precisely because its output is less trusted, and the upload pattern is `pkg/*.gem`, so the set is not limited to what `Gem::PackageTask` names. `release` now checks each basename against /\A[A-Za-z0-9._-]+\.gem\z/ and raises otherwise. RubyGems already restricts a gem's name, version and platform to those characters, so every filename `cibuildgem package` can produce is accepted, and this cannot start rejecting a legitimate release. This is deliberately not the other remediation the report suggested, narrowing the compile job's upload to the package task's expected outputs. Reconstructing `<name>-<version>[-<platform>].gem` in the CLI can drift from what rake-compiler actually writes, and with `if-no-files-found: error` any drift breaks the release; the README also invites consumers to enhance the packaging tasks, so a gem that legitimately produces extra packages would have them silently dropped. Checking the shape of the name has neither failure mode and covers any sink, not just this one. The end-to-end test from the previous commit becomes the refusal case, since a crafted name no longer reaches `gem push` at all. A second case covers a real platform gem name going through untouched, so the argv contract stays proven against an executed subprocess rather than only against the stubbed `Open3` calls. Co-authored-by: Aaron Patterson <aaron.patterson@shopify.com>
The "Setup Rake Compiler" step placed `inputs.working-directory` inside a single-quoted JavaScript string literal in the `actions/github-script` body. Workflow expression substitution does not escape quotes, backslashes or newlines, so a `working-directory` value containing a single quote closes the literal and everything after it is evaluated as JavaScript by the runner (CWE-94, code injection). Callers passing a fixed path such as `.` are unaffected; a caller deriving the path from event data or workflow inputs turns a path selector into arbitrary code execution inside the compile job. Pass the value through the step environment and read it from `process.env` instead, so the script source is static and the input stays data. Add a regression test asserting that no github-script step in the action interpolates a workflow expression into its script body. Co-authored-by: Aaron Patterson <aaron.patterson@shopify.com>
The composite action built its install command with
`format('gem install cibuildgem -v {0}', inputs.version)` and emitted the
result as a `shell: bash` run body. Workflow expression substitution only
concatenates text, so a workflow that wires `version` to a value it does not
control (an event payload field, a `workflow_call` input) turns a version
selector into arbitrary commands running in the job, with its token and
secrets (CWE-78). A hard-coded version is unaffected, so is omitting the
input, and invocations inside this repository take the `rake install` branch.
Pass the input through the step environment and reference it as a quoted
shell variable, so the script source is static and the value stays data. It
reaches `gem install` as a single argument, and RubyGems rejects it with
"Illformed requirement" when it is not a version, so nothing else has to
validate it.
Two regression tests cover it. The first asserts the step reads the version
from the environment and runs a literal command. The second renders the
`version` branch of the step's `case()` expression and runs it against a fake
`gem` on PATH with a hostile value, asserting that nothing was executed and
that `gem` received the value as a single argument. Both fail on the old
shape, the second by actually creating the file the planted command touches.
Reported as https://vault.shopify.io/issues/78388 (CWE-78).
Co-authored-by: Aaron Patterson <aaron.patterson@shopify.com>
Edouard-chin
force-pushed
the
patches
branch
from
September 18, 2026 12:50
89b1661 to
9af1b06
Compare
Split the multi-line system() call so each argument is on its own line (Layout/MultilineMethodArgumentLineBreaks) and keep the trailing comma consistent with each item on its own line (Style/TrailingCommaInArguments).
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
3 participants
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.
These are just a few patches that fix some automated security scans. They're mostly to do with command line argument processing (escaping the arguments). I don't think any of these cases can encounter "user controlled strings", so I don't think it's a real vulnerability, but it doesn't hurt to make these changes.