diff --git a/.github/actions/cibuildgem/action.yml b/.github/actions/cibuildgem/action.yml index ceaf5e7f7..29afc5d25 100644 --- a/.github/actions/cibuildgem/action.yml +++ b/.github/actions/cibuildgem/action.yml @@ -22,21 +22,28 @@ runs: steps: - name: Install cibuildgem shell: bash + env: + CIBUILDGEM_VERSION: "${{ inputs.version }}" run: |- ${{ case( github.repository == 'Shopify/cibuildgem', 'rake install', - inputs.version != null, format('gem install cibuildgem -v {0}', inputs.version), + inputs.version != null, 'gem install cibuildgem -v "$CIBUILDGEM_VERSION"', 'gem install cibuildgem' ) }} working-directory: "${{ github.action_path }}" - name: "Setup Rake Compiler" if: "${{ inputs.step == 'compile' }}" uses: actions/github-script@v8 + env: + # Passed through the environment rather than interpolated into the script + # below: a value containing a quote would otherwise close the string + # literal and the remainder would be evaluated as JavaScript. + WORKING_DIRECTORY: "${{ inputs.working-directory }}" with: script: | const url = new URL(`file:///${process.env.GITHUB_ACTION_PATH}/dist/index.js`); const { run } = await import(url.href) - await run('${{ inputs.working-directory }}') + await run(process.env.WORKING_DIRECTORY) - name: Package if: "${{ inputs.step == 'compile' }}" working-directory: ${{ inputs.working-directory }} diff --git a/CHANGELOG.md b/CHANGELOG.md index 667aae1a8..7a5b2069d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,21 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Security + +- `cibuildgem release` no longer runs `gem push` through a shell. The `.gem` filenames it iterates over come from + artifacts built in the unprivileged compile job, and interpolating them into a command string let shell syntax in a + basename execute in the release job, after RubyGems credentials are configured. The filename is now passed as a + single argv element. +- `cibuildgem release` now refuses a `.gem` whose basename holds anything other than letters, digits, dots, dashes + and underscores, rather than publishing it. RubyGems restricts a gem's name, version and platform to those + characters, so every filename `cibuildgem package` produces is accepted and one that is not was planted by + something else running in the compile job. +- The action's `version` input is now passed to `gem install` through the step environment instead of being + interpolated into the shell script. A workflow that wired the input to a value it did not control turned a version + selector into arbitrary commands running in the job; the value now reaches `gem install` as a single argument and + RubyGems rejects it if it is not a version. + ## [0.3.0] - 2026-03-27 ### Added diff --git a/lib/cibuildgem/cli.rb b/lib/cibuildgem/cli.rb index e76fb0b3e..a44d3a5d7 100644 --- a/lib/cibuildgem/cli.rb +++ b/lib/cibuildgem/cli.rb @@ -11,6 +11,10 @@ class CLI < Thor source_root(File.expand_path("templates", __dir__)) + # RubyGems already restricts a gem's name, version and platform to these characters, so every filename + # `cibuildgem package` can legitimately produce matches. Anything else was not produced by the package task. + RELEASABLE_GEM_FILENAME = /\A[A-Za-z0-9._-]+\.gem\z/ + class << self def exit_on_failure? true @@ -113,7 +117,9 @@ def release pathname = Pathname(file) next if pathname.directory? || pathname.extname != ".gem" - out, status = Open3.capture2e("gem push #{file}") + verify_releasable_filename!(pathname) + + out, status = Open3.capture2e("gem", "push", file) next if status.success? if out =~ /Repushing of gem versions is not allowed/ @@ -139,8 +145,20 @@ def print_normalized_platform private + def verify_releasable_filename!(pathname) + basename = pathname.basename.to_s + return if RELEASABLE_GEM_FILENAME.match?(basename) + + raise(<<~MSG) + Refusing to publish #{basename.inspect}. + + A gem filename may only contain letters, digits, dots, dashes and underscores, so this file was not + produced by `cibuildgem package`. Artifacts reach the release job from the compile job, which runs + without publishing credentials, so an unexpected filename is treated as tampering rather than a gem. + MSG + end + def run_rake_tasks!(*tasks) - all_tasks = tasks.join(" ") rakelibdir = [File.expand_path("tasks", __dir__), "rakelib"].join(File::PATH_SEPARATOR) rake_compiler_path = Gem.loaded_specs["rake-compiler"].full_require_paths rake_specs = Gem.loaded_specs["rake"] @@ -152,7 +170,14 @@ def run_rake_tasks!(*tasks) system( { "RUBYLIB" => load_paths }, - "bundle exec #{RbConfig.ruby} #{rake_executable} #{all_tasks} -R#{rakelibdir} -r #{patch}", + "bundle", + "exec", + RbConfig.ruby, + rake_executable, + *tasks.map(&:to_s), + "-R#{rakelibdir}", + "-r", + patch, exception: true, ) end diff --git a/test/action_test.rb b/test/action_test.rb new file mode 100644 index 000000000..287180840 --- /dev/null +++ b/test/action_test.rb @@ -0,0 +1,105 @@ +# frozen_string_literal: true + +require "test_helper" +require "fileutils" +require "tmpdir" +require "yaml" + +module Cibuildgem + class ActionTest < Minitest::Test + ACTION_PATH = File.expand_path("../.github/actions/cibuildgem/action.yml", __dir__) + + def test_github_script_steps_do_not_interpolate_workflow_expressions + github_script_steps.each do |step| + script = step.dig("with", "script") + + refute_match( + /\$\{\{/, + script, + "Step #{step["name"].inspect} interpolates a workflow expression into evaluated " \ + "JavaScript. Pass the value through `env:` and read it from `process.env` instead.", + ) + end + end + + def test_setup_rake_compiler_reads_the_working_directory_from_the_environment + step = github_script_steps.fetch(0) + + assert_equal("${{ inputs.working-directory }}", step.dig("env", "WORKING_DIRECTORY")) + assert_includes(step.dig("with", "script"), "run(process.env.WORKING_DIRECTORY)") + end + + def test_install_step_reads_the_cibuildgem_version_from_the_environment + assert_equal("${{ inputs.version }}", install_step.dig("env", "CIBUILDGEM_VERSION")) + assert_equal(%(gem install cibuildgem -v "$CIBUILDGEM_VERSION"), version_branch_script("1.2.3")) + end + + def test_the_version_input_reaches_gem_install_as_a_single_argument + hostile_version = nil + argv = nil + + Dir.mktmpdir do |dir| + marker = File.join(dir, "injected") + argv_log = File.join(dir, "argv") + hostile_version = "$(touch #{marker})" + + system( + { "PATH" => "#{fake_gem_bin(dir, argv_log)}:#{ENV["PATH"]}", "CIBUILDGEM_VERSION" => hostile_version }, + "bash", + "-c", + version_branch_script(hostile_version), + out: File::NULL, + err: File::NULL, + ) + refute_path_exists(marker, "the version input was evaluated as shell syntax instead of passed as data") + + argv = File.readlines(argv_log, chomp: true) + end + + assert_equal(["install", "cibuildgem", "-v", hostile_version], argv) + end + + private + + def fake_gem_bin(dir, argv_log) + bin = File.join(dir, "bin") + FileUtils.mkdir_p(bin) + File.write(File.join(bin, "gem"), <<~SH) + #!/usr/bin/env bash + printf '%s\\n' "$@" > #{argv_log} + SH + FileUtils.chmod(0o755, File.join(bin, "gem")) + + bin + end + + def steps + YAML.safe_load_file(ACTION_PATH).dig("runs", "steps") + end + + def install_step + steps.fetch(0).tap { |step| assert_equal("Install cibuildgem", step["name"]) } + end + + # The `version` branch of the install step's `case()` expression, rendered as the shell script the runner + # would produce for `version`. Understands a literal command, which is the safe shape, and a `format()` + # call interpolating the input, which is the shape these tests exist to reject. + def version_branch_script(version) + branch = install_step.fetch("run")[/inputs\.version != null,\s*(.+?),\s*'[^']*'\s*\)/m, 1] + refute_nil(branch, "no `version` branch found in the install step") + + case branch + when /\A'(.*)'\z/m then Regexp.last_match(1) + when /\Aformat\('(.*)',\s*inputs\.version\)\z/m then Regexp.last_match(1).sub("{0}", version) + else flunk("cannot tell what the install step runs when `version` is set: #{branch}") + end + end + + def github_script_steps + matching = steps.select { |step| step["uses"].to_s.start_with?("actions/github-script") } + + refute_empty(matching) + matching + end + end +end diff --git a/test/cli_test.rb b/test/cli_test.rb index 7a1feebbb..c2d9e690a 100644 --- a/test/cli_test.rb +++ b/test/cli_test.rb @@ -3,6 +3,7 @@ require "test_helper" require "English" require "open3" +require "tmpdir" module Cibuildgem class CLITest < Minitest::Test @@ -137,8 +138,8 @@ def test_release_succeeds status = Struct.new(:success?) gem_pushed = [] - callable = proc do |gem_name| - gem_pushed << gem_name + callable = proc do |*command| + gem_pushed << command ["", status.new(true)] end @@ -147,7 +148,7 @@ def test_release_succeeds CLI.start(["release", "--glob", "tmp/*"]) end - assert_equal(["gem push tmp/bar.gem", "gem push tmp/foo.gem"], gem_pushed.sort) + assert_equal([["gem", "push", "tmp/bar.gem"], ["gem", "push", "tmp/foo.gem"]], gem_pushed.sort) ensure FileUtils.rm_rf("tmp/foo.gem") FileUtils.rm_rf("tmp/bar.gem") @@ -161,10 +162,10 @@ def test_release_when_gem_was_already_pushed status = Struct.new(:success?) gem_pushed = [] - callable = proc do |gem_name| - gem_pushed << gem_name + callable = proc do |*command| + gem_pushed << command - if gem_name == "gem push tmp/bar.gem" + if command == ["gem", "push", "tmp/bar.gem"] ["Repushing of gem versions is not allowed", status.new(false)] else ["", status.new(true)] @@ -179,7 +180,7 @@ def test_release_when_gem_was_already_pushed assert_equal("Gem tmp/bar.gem already exists on RubyGems.org, skipping...\n", out) end - assert_equal(["gem push tmp/bar.gem", "gem push tmp/foo.gem"], gem_pushed.sort) + assert_equal([["gem", "push", "tmp/bar.gem"], ["gem", "push", "tmp/foo.gem"]], gem_pushed.sort) ensure FileUtils.rm_rf("tmp/foo.gem") FileUtils.rm_rf("tmp/bar.gem") @@ -207,6 +208,30 @@ def test_release_fails FileUtils.rm_rf("tmp/some_file") end + def test_release_refuses_a_filename_that_is_not_a_plain_gem_name + with_fake_gem_executable do |argv_log| + FileUtils.touch("pkg/hello$(touch injected).gem") + + error = assert_raises(RuntimeError) do + CLI.start(["release", "--glob", "pkg/*"]) + end + + assert_match("hello$(touch injected).gem", error.message) + refute(File.exist?("injected"), "the filename was evaluated by a shell") + refute(File.exist?(argv_log), "the file was handed to `gem push` instead of being refused") + end + end + + def test_release_pushes_a_plain_gem_name_without_a_shell + with_fake_gem_executable do |argv_log| + FileUtils.touch("pkg/hello_world-1.2.3-x86_64-linux.gem") + + CLI.start(["release", "--glob", "pkg/*"]) + + assert_equal(["push", "pkg/hello_world-1.2.3-x86_64-linux.gem"], File.readlines(argv_log, chomp: true)) + end + end + def test_print_ruby_cc_version out, _ = capture_subprocess_io do Dir.chdir("test/fixtures/dummy_gem") do @@ -315,8 +340,56 @@ def test_keep_the_extension_task_config_defined_by_the_gem end end + def test_run_rake_tasks_builds_an_argv_list_rather_than_a_command_string + cli = CLI.new + command = nil + recorder = ->(*args, **_kwargs) do + command = args + + true + end + + cli.stub(:system, recorder) do + cli.send(:run_rake_tasks!, "cibuildgem:setup", :compile) + end + + _env, *argv = command + + assert_equal(["bundle", "exec"], argv.first(2)) + assert_includes(argv, "cibuildgem:setup") + assert_includes(argv, "compile") + end + private + # Runs the block in a scratch directory holding an empty `pkg/`, with a `gem` on PATH that records its + # arguments in the file whose path is yielded. Nothing is stubbed, so the release path resolves and + # executes a real subprocess the way it does on a release runner. + def with_fake_gem_executable(&block) + original_path = ENV["PATH"] + + Dir.mktmpdir do |dir| + argv_log = File.join(dir, "argv") + fake_bin = File.join(dir, "bin") + + FileUtils.mkdir_p(fake_bin) + File.write(File.join(fake_bin, "gem"), <<~SH) + #!/bin/sh + printf '%s\\n' "$@" > #{argv_log} + SH + FileUtils.chmod(0o755, File.join(fake_bin, "gem")) + ENV["PATH"] = [fake_bin, original_path].join(File::PATH_SEPARATOR) + + Dir.chdir(dir) do + FileUtils.mkdir_p("pkg") + + block.call(argv_log) + end + end + ensure + ENV["PATH"] = original_path + end + def raise_instead_of_exit(&block) Kernel.stub(:exit, ->(_) { raise }) do assert_raises(StandardError) do