From 9532ca1da0624aed5d83b9283506f2256b3745cb Mon Sep 17 00:00:00 2001 From: Brandon Wilde Date: Fri, 11 Sep 2026 10:43:07 -0700 Subject: [PATCH 1/3] Stop wrapping commands in cmd.exe on Windows `Subprocess.spawn` wrapped every command in `cmd.exe /c` and joined the arguments into a single string before handing them to childprocess. That collapsed the whole command into one argv element, and childprocess 5.x on Windows is a thin shim over `Process.spawn`, which sees the embedded spaces, wraps the element in quotes and backslash-escapes the quotes already inside it. cmd.exe strips the outer quotes and the child's C runtime then reads the remaining `\"` as a literal quote rather than a delimiter, so the argument gets re-split on whitespace. For the pre-commit stash this meant `git stash save` received the trailing UTC offset of the stash message as a switch of its own and aborted with `error: unknown switch '0'`, leaving hooks unable to run at all. No cmd.exe escaping scheme fixes this properly either: quoting makes `& | < > ( )` literal but does not prevent `%VAR%` expansion, and carets inside quotes survive as literal carets. The wrapper is unnecessary. `Process.spawn` performs the same command lookup cmd.exe was being used for, so `.bat`, `.cmd`, extensionless RubyGems shims and bare names resolved via PATHEXT all launch correctly when passed straight to `ChildProcess.build`, and arguments stay atomic because no shell ever re-parses them. Drop the wrapper and pass the argument vector through untouched on every platform. This requires the `Process.spawn`-based Windows backend that childprocess introduced in 5.0.0, so raise the dependency floor accordingly. Fixes #847 --- lib/overcommit/subprocess.rb | 19 ---- overcommit.gemspec | 2 +- spec/overcommit/subprocess_spec.rb | 166 +++++++++++++++++++++++++++++ 3 files changed, 167 insertions(+), 20 deletions(-) create mode 100644 spec/overcommit/subprocess_spec.rb diff --git a/lib/overcommit/subprocess.rb b/lib/overcommit/subprocess.rb index 41175fb9..88ef362e 100644 --- a/lib/overcommit/subprocess.rb +++ b/lib/overcommit/subprocess.rb @@ -2,7 +2,6 @@ require 'childprocess' require 'tempfile' -require 'overcommit/os' module Overcommit # Manages execution of a child process, collecting the exit status and @@ -28,8 +27,6 @@ class << self # @option options [String] input string to pass via standard input stream # @return [Result] def spawn(args, options = {}) - args = win32_prepare_args(args) if OS.windows? - process = ChildProcess.build(*args) out, err = assign_output_streams(process) @@ -58,8 +55,6 @@ def spawn(args, options = {}) # Spawns a new process in the background using the given array of # arguments (the first element is the command). def spawn_detached(args) - args = win32_prepare_args(args) if OS.windows? - process = ChildProcess.build(*args) process.detach = true @@ -70,20 +65,6 @@ def spawn_detached(args) private - # Necessary to run commands in the cmd.exe context. - # Args are joined to properly handle quotes and special characters. - def win32_prepare_args(args) - args = args.map do |arg| - # Quote args that contain whitespace - arg = "\"#{arg}\"" if arg =~ /\s/ - - # Escape cmd.exe metacharacters - arg.gsub(/[()%!^"<>&|]/, '^\0') - end - - %w[cmd.exe /c] + [args.join(' ')] - end - # @param process [ChildProcess] # @return [Array] def assign_output_streams(process) diff --git a/overcommit.gemspec b/overcommit.gemspec index 1bb8adaa..a753af6d 100644 --- a/overcommit.gemspec +++ b/overcommit.gemspec @@ -31,7 +31,7 @@ Gem::Specification.new do |s| s.required_ruby_version = '>= 2.6' - s.add_dependency 'childprocess', '>= 0.6.3', '< 6' + s.add_dependency 'childprocess', '>= 5.0.0', '< 6' s.add_dependency 'iniparse', '~> 1.4' s.add_dependency 'rexml', '>= 3.4.2' end diff --git a/spec/overcommit/subprocess_spec.rb b/spec/overcommit/subprocess_spec.rb new file mode 100644 index 00000000..8bf82c65 --- /dev/null +++ b/spec/overcommit/subprocess_spec.rb @@ -0,0 +1,166 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Overcommit::Subprocess do + # Absolute path to the Ruby currently running the specs. Using this instead of + # a bare `ruby` keeps these specs working regardless of what is on the PATH. + let(:ruby) { Gem.ruby } + + # Script which writes its own ARGV to standard output, using NUL as the + # separator so that arguments containing whitespace remain distinguishable. + let(:argv_dump) { 'STDOUT.print ARGV.join("\0")' } + + # Splits the output of `argv_dump` back into the array the child received. + def received_argv(result) + result.stdout.split("\0", -1) + end + + describe '.spawn' do + context 'when the command succeeds' do + subject do + described_class.spawn([ruby, '-e', 'STDOUT.print "hello"; STDERR.print "world"']) + end + + it 'returns a successful result containing the captured output' do + subject.should be_a described_class::Result + subject.should be_success + subject.status.should == 0 + subject.stdout.should == 'hello' + subject.stderr.should == 'world' + end + end + + context 'when the command fails' do + subject do + described_class.spawn([ruby, '-e', 'STDERR.print "boom"; exit 42']) + end + + it 'returns an unsuccessful result containing the exit status' do + subject.should_not be_success + subject.status.should == 42 + subject.stdout.should == '' + subject.stderr.should == 'boom' + end + end + + context 'when given input' do + subject do + described_class.spawn([ruby, '-e', 'STDOUT.print STDIN.read'], input: 'from-stdin') + end + + it 'passes the input to the standard input stream of the process' do + subject.should be_success + subject.stdout.chomp.should == 'from-stdin' + end + end + + context 'when an argument contains whitespace and a dash-prefixed token' do + # Regression test for https://github.com/sds/overcommit/issues/847, where + # arguments were joined into a single string before being handed to the + # shell, causing the child to re-split them on whitespace. This made + # `git stash save` interpret the tail of the stash message (the UTC offset) + # as a switch of its own, failing with `unknown switch '0'`. + let(:stash_message) do + 'Overcommit: Stash of repo state before hook run at 2024-04-10 12:34:56 -0700' + end + + subject do + described_class.spawn([ruby, '-e', argv_dump, 'save', stash_message]) + end + + it 'delivers the argument to the child as a single atomic argument' do + subject.should be_success + received_argv(subject).should == ['save', stash_message] + end + end + + context 'when arguments contain shell metacharacters' do + let(:tricky_args) do + [ + 'a&b', + 'a|b', + 'a>b', + 'a nil, :stderr= => nil) } + + let(:process) do + double( + 'process', + io: io, + :duplex= => nil, + :detach= => nil, + start: nil, + wait: nil, + exit_code: 0 + ) + end + + [true, false].each do |windows| + context "when Overcommit::OS.windows? is #{windows}" do + before do + Overcommit::OS.stub(:windows?).and_return(windows) + end + + it 'passes the arguments through verbatim from .spawn' do + expect(ChildProcess).to receive(:build).with(*args).and_return(process) + described_class.spawn(args) + end + + it 'passes the arguments through verbatim from .spawn_detached' do + expect(ChildProcess).to receive(:build).with(*args).and_return(process) + described_class.spawn_detached(args) + end + + it 'does not wrap the command in a cmd.exe invocation' do + received = nil + ChildProcess.stub(:build) do |*actual| + received = actual + process + end + + described_class.spawn(args) + + received.should == args + received.should_not include 'cmd.exe' + received.first.should == 'git' + end + end + end + end +end From f288bedd283820829ccd2523df02cefc9d14c914 Mon Sep 17 00:00:00 2001 From: Brandon Wilde Date: Fri, 11 Sep 2026 13:36:16 -0700 Subject: [PATCH 2/3] Restore cmd.exe wrapping for mklink and dir on Windows `mklink` and `dir` are cmd.exe built-ins with no standalone executable, so removing the cmd.exe wrapper for all Subprocess calls broke FileUtils.symlink (and left FileUtils.symlink?/readlink working only by accident, since `dir` happens to be on MRI's hardcoded list of legacy cmd.exe built-ins that Process.spawn falls back to). Unlike the removed general-purpose wrapper, this passes cmd.exe, /c, and the command as separate argv elements rather than joining them into one pre-escaped string, so it doesn't reintroduce the argument corruption from #847. --- lib/overcommit/utils/file_utils.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/overcommit/utils/file_utils.rb b/lib/overcommit/utils/file_utils.rb index cff2ce0b..1d8b7bb9 100644 --- a/lib/overcommit/utils/file_utils.rb +++ b/lib/overcommit/utils/file_utils.rb @@ -49,13 +49,13 @@ def readlink(link_name) def win32_dir_cmd(file_name) Overcommit::Subprocess.spawn( - %W[dir #{win32_fix_pathsep(file_name)}] + %W[cmd.exe /c dir #{win32_fix_pathsep(file_name)}] ) end def win32_mklink_cmd(old_name, new_name) Overcommit::Subprocess.spawn( - %W[mklink #{win32_fix_pathsep(new_name)} #{win32_fix_pathsep(old_name)}] + %W[cmd.exe /c mklink #{win32_fix_pathsep(new_name)} #{win32_fix_pathsep(old_name)}] ) end From 5d233fe4daac03c87c5d71172b69857463e6b7a0 Mon Sep 17 00:00:00 2001 From: Brandon Wilde Date: Fri, 11 Sep 2026 13:56:50 -0700 Subject: [PATCH 3/3] Simplify argv-verbatim examples in subprocess_spec Subprocess no longer branches on Overcommit::OS.windows?, so stubbing it to both true and false ran the same assertions twice and the comment claiming it exercised "the Windows code path" was no longer accurate. Collapse to one context, drop the example that duplicated an existing assertion, and match the surrounding suite's should_receive/stub syntax. --- spec/overcommit/subprocess_spec.rb | 42 +++++++----------------------- 1 file changed, 10 insertions(+), 32 deletions(-) diff --git a/spec/overcommit/subprocess_spec.rb b/spec/overcommit/subprocess_spec.rb index 8bf82c65..07503d25 100644 --- a/spec/overcommit/subprocess_spec.rb +++ b/spec/overcommit/subprocess_spec.rb @@ -108,8 +108,8 @@ def received_argv(result) end end - # These examples assert on the argument vector handed to ChildProcess rather - # than on observable behaviour, so that the Windows code path can be checked + # Subprocess no longer branches on platform, but this guards against + # reintroducing a shell wrapper -- if one came back, this would catch it # from CI (which only runs Linux -- see # https://github.com/sds/overcommit/issues/836). describe 'the argument vector handed to ChildProcess' do @@ -131,36 +131,14 @@ def received_argv(result) ) end - [true, false].each do |windows| - context "when Overcommit::OS.windows? is #{windows}" do - before do - Overcommit::OS.stub(:windows?).and_return(windows) - end - - it 'passes the arguments through verbatim from .spawn' do - expect(ChildProcess).to receive(:build).with(*args).and_return(process) - described_class.spawn(args) - end - - it 'passes the arguments through verbatim from .spawn_detached' do - expect(ChildProcess).to receive(:build).with(*args).and_return(process) - described_class.spawn_detached(args) - end - - it 'does not wrap the command in a cmd.exe invocation' do - received = nil - ChildProcess.stub(:build) do |*actual| - received = actual - process - end - - described_class.spawn(args) - - received.should == args - received.should_not include 'cmd.exe' - received.first.should == 'git' - end - end + it 'passes the arguments through verbatim from .spawn' do + ChildProcess.should_receive(:build).with(*args).and_return(process) + described_class.spawn(args) + end + + it 'passes the arguments through verbatim from .spawn_detached' do + ChildProcess.should_receive(:build).with(*args).and_return(process) + described_class.spawn_detached(args) end end end