From 9586a4dd1942b4747ff1f464c9d2778fe84f49ba Mon Sep 17 00:00:00 2001 From: Shane da Silva Date: Tue, 10 Nov 2026 21:55:50 -0800 Subject: [PATCH 1/2] Fix all open CodeQL security findings --- .github/workflows/tests.yml | 3 + lib/overcommit/configuration.rb | 2 +- lib/overcommit/git_repo.rb | 9 +-- lib/overcommit/hook/pre_commit/es_lint.rb | 2 +- lib/overcommit/hook/pre_commit/haml_lint.rb | 2 +- lib/overcommit/hook/pre_commit/js_hint.rb | 2 +- lib/overcommit/hook/pre_commit/json_syntax.rb | 2 +- lib/overcommit/hook/pre_commit/reek.rb | 2 +- lib/overcommit/hook/pre_commit/ruby_syntax.rb | 2 +- lib/overcommit/hook/pre_commit/slim_lint.rb | 2 +- lib/overcommit/hook/pre_commit/xml_syntax.rb | 2 +- lib/overcommit/hook_context/commit_msg.rb | 2 +- lib/overcommit/utils.rb | 2 +- spec/overcommit/configuration_spec.rb | 31 ++++++++++ spec/overcommit/git_repo_spec.rb | 30 ++++++++++ .../hook/pre_commit/json_syntax_spec.rb | 20 ++++++- .../message_parsing_security_spec.rb | 57 +++++++++++++++++++ .../hook/pre_commit/xml_syntax_spec.rb | 23 +++++++- .../hook_context/commit_msg_spec.rb | 16 ++++++ spec/overcommit/utils_spec.rb | 15 +++++ 20 files changed, 205 insertions(+), 21 deletions(-) create mode 100644 spec/overcommit/hook/pre_commit/message_parsing_security_spec.rb diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 1b78237c..98edd39f 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -5,6 +5,9 @@ on: pull_request: branches: [main] +permissions: + contents: read + jobs: rspec: name: rspec (${{ matrix.ruby-version }}, ${{ matrix.target.os }}, shard ${{ matrix.target.shard }}/${{ matrix.target.shards }}) diff --git a/lib/overcommit/configuration.rb b/lib/overcommit/configuration.rb index 76da489c..ab686df5 100644 --- a/lib/overcommit/configuration.rb +++ b/lib/overcommit/configuration.rb @@ -47,7 +47,7 @@ def concurrency if content.is_a?(String) concurrency_expr = content % { processors: cores } - a, op, b = concurrency_expr.scan(%r{(\d+)\s*([+\-*\/])\s*(\d+)})[0] + a, op, b = concurrency_expr.scan(%r{(?] list of branches containing the given commit def branches_containing_commit(commit_ref) - `git branch --column=dense --contains #{commit_ref}`. - sub(/\((HEAD )?detached (from|at) .*?\)/, ''). # ignore detached HEAD - split(/\s+/). - reject { |s| s.empty? || s == '*' } + result = Overcommit::Utils.execute( + ['git', 'for-each-ref', '--format=%(refname)', "--contains=#{commit_ref}", + 'refs/heads/'] + ) + result.stdout.split("\n").map { |ref| ref.delete_prefix('refs/heads/') } end # Returns the name of the currently checked out branch. diff --git a/lib/overcommit/hook/pre_commit/es_lint.rb b/lib/overcommit/hook/pre_commit/es_lint.rb index 240d749d..d0453705 100644 --- a/lib/overcommit/hook/pre_commit/es_lint.rb +++ b/lib/overcommit/hook/pre_commit/es_lint.rb @@ -19,7 +19,7 @@ module Overcommit::Hook::PreCommit # @see http://eslint.org/ class EsLint < Base def run - eslint_regex = /^(?[^\s](?:\w:)?[^:]+):[^\d]+(?\d+).*?(?Error|Warning)/ + eslint_regex = /^(?[^\s](?:\w:)?[^:]+):[^\d]+(?\d++).*?(?Error|Warning)/ result = execute(command, args: applicable_files) output = result.stdout.chomp messages = output.split("\n").grep(eslint_regex) diff --git a/lib/overcommit/hook/pre_commit/haml_lint.rb b/lib/overcommit/hook/pre_commit/haml_lint.rb index af8d801a..724a41ff 100644 --- a/lib/overcommit/hook/pre_commit/haml_lint.rb +++ b/lib/overcommit/hook/pre_commit/haml_lint.rb @@ -15,7 +15,7 @@ def run extract_messages( result.stdout.split("\n"), - /^(?(?:\w:)?[^:]+):(?\d+)[^ ]* (?[^ ]+)/, + /^(?(?:\w:)?[^:]+):(?\d++)[^ ]* (?[^ ]+)/, MESSAGE_TYPE_CATEGORIZER, ) end diff --git a/lib/overcommit/hook/pre_commit/js_hint.rb b/lib/overcommit/hook/pre_commit/js_hint.rb index 6c22c538..ef420ef9 100644 --- a/lib/overcommit/hook/pre_commit/js_hint.rb +++ b/lib/overcommit/hook/pre_commit/js_hint.rb @@ -15,7 +15,7 @@ def run # path/to/file.js: line 1, col 0, Error message (E001) extract_messages( output.split("\n").grep(/E|W/), - /^(?(?:\w:)?[^:]+):[^\d]+(?\d+).+\((?E|W)\d+\)/, + /^(?(?:\w:)?[^:]+):[^\d]+(?\d++).+\((?E|W)\d+\)/, lambda { |type| type.include?('W') ? :warning : :error } ) end diff --git a/lib/overcommit/hook/pre_commit/json_syntax.rb b/lib/overcommit/hook/pre_commit/json_syntax.rb index bd162f7d..a5188831 100644 --- a/lib/overcommit/hook/pre_commit/json_syntax.rb +++ b/lib/overcommit/hook/pre_commit/json_syntax.rb @@ -7,7 +7,7 @@ def run messages = [] applicable_files.each do |file| - JSON.parse(IO.read(file)) + JSON.parse(File.read(file)) rescue JSON::ParserError => e error = "#{e.message} parsing #{file}" messages << Overcommit::Hook::Message.new(:error, file, nil, error) diff --git a/lib/overcommit/hook/pre_commit/reek.rb b/lib/overcommit/hook/pre_commit/reek.rb index 163d7206..55ad153f 100644 --- a/lib/overcommit/hook/pre_commit/reek.rb +++ b/lib/overcommit/hook/pre_commit/reek.rb @@ -13,7 +13,7 @@ def run extract_messages( output, - /^\s*(?(?:\w:)?[^:]+):(?\d+):/, + /^\s*+(?(?:\w:)?[^:]+):(?\d+):/, ) end diff --git a/lib/overcommit/hook/pre_commit/ruby_syntax.rb b/lib/overcommit/hook/pre_commit/ruby_syntax.rb index ac31e5ed..8bb014b1 100644 --- a/lib/overcommit/hook/pre_commit/ruby_syntax.rb +++ b/lib/overcommit/hook/pre_commit/ruby_syntax.rb @@ -19,7 +19,7 @@ def run # path/to/file.rb:1: syntax error, unexpected '^' extract_messages( result_lines, - /^(?[^:]+):(?\d+):\s*(?[^,]+),\s*(?.+)/, + /^(?[^:]+):(?\d+):\s*+(?[^,]+),\s*+(?.+)/, MESSAGE_TYPE_CATEGORIZER ) end diff --git a/lib/overcommit/hook/pre_commit/slim_lint.rb b/lib/overcommit/hook/pre_commit/slim_lint.rb index 8e1d1ce0..cab27b29 100644 --- a/lib/overcommit/hook/pre_commit/slim_lint.rb +++ b/lib/overcommit/hook/pre_commit/slim_lint.rb @@ -15,7 +15,7 @@ def run extract_messages( result.stdout.split("\n"), - /^(?(?:\w:)?[^:]+):(?\d+)[^ ]* (?[^ ]+)/, + /^(?(?:\w:)?[^:]+):(?\d++)[^ ]* (?[^ ]+)/, MESSAGE_TYPE_CATEGORIZER, ) end diff --git a/lib/overcommit/hook/pre_commit/xml_syntax.rb b/lib/overcommit/hook/pre_commit/xml_syntax.rb index 99465182..d194d68f 100644 --- a/lib/overcommit/hook/pre_commit/xml_syntax.rb +++ b/lib/overcommit/hook/pre_commit/xml_syntax.rb @@ -7,7 +7,7 @@ def run messages = [] applicable_files.each do |file| - REXML::Document.new(IO.read(file)) + REXML::Document.new(File.read(file)) rescue REXML::ParseException => e error = "Error parsing #{file}: #{e.message}" messages << Overcommit::Hook::Message.new(:error, file, nil, error) diff --git a/lib/overcommit/hook_context/commit_msg.rb b/lib/overcommit/hook_context/commit_msg.rb index efd934dc..addd32f8 100644 --- a/lib/overcommit/hook_context/commit_msg.rb +++ b/lib/overcommit/hook_context/commit_msg.rb @@ -55,7 +55,7 @@ def cleanup_mode end def raw_commit_message_lines - ::IO.readlines(commit_message_file) + ::File.readlines(commit_message_file) end end end diff --git a/lib/overcommit/utils.rb b/lib/overcommit/utils.rb index f26f7b23..bf13a1eb 100644 --- a/lib/overcommit/utils.rb +++ b/lib/overcommit/utils.rb @@ -113,7 +113,7 @@ def strip_color_codes(text) # stackoverflow.com/questions/1509915/converting-camel-case-to-underscore-case-in-ruby def snake_case(str) str.gsub(/::/, '/'). - gsub(/([A-Z]+)([A-Z][a-z])/, '\1_\2'). + gsub(/([A-Z])([A-Z][a-z])/, '\1_\2'). gsub(/([a-z\d])([A-Z])/, '\1_\2'). tr('-', '_'). downcase diff --git a/spec/overcommit/configuration_spec.rb b/spec/overcommit/configuration_spec.rb index d9860400..5e50913b 100644 --- a/spec/overcommit/configuration_spec.rb +++ b/spec/overcommit/configuration_spec.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require 'spec_helper' +require 'timeout' describe Overcommit::Configuration do let(:hash) { {} } @@ -48,6 +49,36 @@ it { should == File.expand_path('some-directory') } end + describe '#concurrency' do + before { allow(Overcommit::Utils).to receive(:processor_count).and_return(4) } + + { + '%d + 2' => 6, + '12 - 3' => 9, + '3 * 4' => 12, + '12 / 3' => 4, + 'prefix 12 + 3 suffix 4 * 5' => 15, + '7' => 7, + 8 => 8, + 'invalid' => 0, + }.each do |expression, expected| + it "evaluates #{expression.inspect}" do + config = described_class.new({ 'concurrency' => expression }, validate: false) + expect(config.concurrency).to eq(expected) + end + end + + it 'handles long digit runs without an arithmetic operator' do + config = described_class.new({ 'concurrency' => "#{'0' * 100_000}1" }, validate: false) + Timeout.timeout(2) { expect(config.concurrency).to eq(1) } + end + + it 'preserves division errors' do + config = described_class.new({ 'concurrency' => '4 / 0' }, validate: false) + expect { config.concurrency }.to raise_error(ZeroDivisionError) + end + end + describe '#enabled_builtin_hooks' do let(:hash) do { diff --git a/spec/overcommit/git_repo_spec.rb b/spec/overcommit/git_repo_spec.rb index 281ad6fa..f985071b 100644 --- a/spec/overcommit/git_repo_spec.rb +++ b/spec/overcommit/git_repo_spec.rb @@ -554,6 +554,36 @@ def stage_files(paths) end end + context 'when HEAD is detached at a commit contained in branches' do + let(:commit_ref) { 'master' } + + before do + `git checkout --detach > #{File::NULL} 2>&1` + `git config column.branch always` + `git config color.branch always` + end + + it 'returns only branch names regardless of display settings' do + expect(subject.sort).to eq(%w[master topic]) + end + end + + context 'when a tag has the same name as a branch' do + let(:commit_ref) { 'HEAD~1' } + + before { `git tag topic` } + + it 'returns the branch name without a disambiguating prefix' do + expect(subject.sort).to eq(%w[master topic]) + end + end + + context 'when the commit does not exist' do + let(:commit_ref) { 'nonexistent' } + + it { should be_empty } + end + context 'when no branches contain the commit' do let(:commit_ref) { 'HEAD' } diff --git a/spec/overcommit/hook/pre_commit/json_syntax_spec.rb b/spec/overcommit/hook/pre_commit/json_syntax_spec.rb index 0ee4a57e..ef1e9bd6 100644 --- a/spec/overcommit/hook/pre_commit/json_syntax_spec.rb +++ b/spec/overcommit/hook/pre_commit/json_syntax_spec.rb @@ -17,22 +17,36 @@ around do |example| repo do touch staged_file - `git add #{staged_file}` + Overcommit::Utils.execute(['git', 'add', '--', staged_file]) example.run end end context 'when JSON files have no errors' do before do - JSON.stub(:parse) + File.write(staged_file, '{"valid": true}') end it { should pass } end + it 'reads a pipe-prefixed filename literally' do + skip 'Windows filenames cannot contain pipes' if Overcommit::OS.windows? + + file = '|echo injected' + File.write(file, '{"literal": true}') + subject.stub(:applicable_files).and_return([file]) + expect(subject.run).to eq([]) + end + + it 'reports a missing file without executing a command' do + subject.stub(:applicable_files).and_return(['|echo injected']) + expect { subject.run }.to raise_error(SystemCallError) + end + context 'when JSON file has errors' do before do - JSON.stub(:parse).and_raise(JSON::ParserError) + File.write(staged_file, '{invalid JSON}') end it { should fail_hook } diff --git a/spec/overcommit/hook/pre_commit/message_parsing_security_spec.rb b/spec/overcommit/hook/pre_commit/message_parsing_security_spec.rb new file mode 100644 index 00000000..9acbd06e --- /dev/null +++ b/spec/overcommit/hook/pre_commit/message_parsing_security_spec.rb @@ -0,0 +1,57 @@ +# frozen_string_literal: true + +require 'spec_helper' +require 'timeout' + +describe 'Hook output parsing' do + let(:config) { Overcommit::ConfigurationLoader.default_configuration } + let(:context) { double('context') } + + { + EsLint: ['/file.js: line 123, col 0, Warning - message', '/file.js', :warning], + JsHint: ['C:/file.js: line 123, col 0, message (W033)', 'C:/file.js', :warning], + HamlLint: ['C:/file.haml:123:45 [W] message', 'C:/file.haml', :warning], + SlimLint: ['C:/file.slim:123:45 [E] message', 'C:/file.slim', :error], + Reek: [' C:/file.rb:123: message', 'C:/file.rb', :error], + RubySyntax: ['file.rb:123: syntax error, unexpected token', 'file.rb', :error], + }.each do |name, (output, file, type)| + it "preserves file, line, type, and output for #{name}" do + hook = Overcommit::Hook::PreCommit.const_get(name).new(config, context) + allow(hook).to receive(:applicable_files).and_return([]) + stdout, stderr = name == :RubySyntax ? ['', output] : [output, ''] + allow(hook).to receive(:execute). + and_return(Overcommit::Subprocess::Result.new(1, stdout, stderr)) + + message = hook.run.fetch(0) + expect(message.file).to eq(file) + expect(message.line).to eq(123) + expect(message.type).to eq(type) + expect(message.content).to eq(output) + end + end + + { + EsLint: "!9:/9#{'9' * 100_000}", + JsHint: "9:/9#{'9' * 100_000}W", + HamlLint: "9:9#{'0' * 100_000}", + SlimLint: "9:9#{'0' * 100_000}", + Reek: ' ' * 100_000, + RubySyntax: "9:9:#{' ' * 100_000}", + }.each do |name, output| + it "rejects long malformed #{name} output without excessive backtracking" do + hook = Overcommit::Hook::PreCommit.const_get(name).new(config, context) + allow(hook).to receive(:applicable_files).and_return([]) + stdout, stderr = name == :RubySyntax ? ['', output] : [output, ''] + allow(hook).to receive(:execute). + and_return(Overcommit::Subprocess::Result.new(1, stdout, stderr)) + + Timeout.timeout(2) do + if name == :EsLint + expect(hook.run).to eq([:fail, '']) + else + expect { hook.run }.to raise_error(Overcommit::Exceptions::MessageProcessingError) + end + end + end + end +end diff --git a/spec/overcommit/hook/pre_commit/xml_syntax_spec.rb b/spec/overcommit/hook/pre_commit/xml_syntax_spec.rb index d2a31c05..79341e2d 100644 --- a/spec/overcommit/hook/pre_commit/xml_syntax_spec.rb +++ b/spec/overcommit/hook/pre_commit/xml_syntax_spec.rb @@ -10,13 +10,30 @@ let(:staged_file) { 'file1.xml' } before do - IO.stub(:read).with(staged_file) subject.stub(:applicable_files).and_return([staged_file]) end + around do |example| + repo { example.run } + end + + it 'reads a pipe-prefixed filename literally' do + skip 'Windows filenames cannot contain pipes' if Overcommit::OS.windows? + + file = '|echo injected' + File.write(file, '') + subject.stub(:applicable_files).and_return([file]) + expect(subject.run).to eq([]) + end + + it 'reports a missing file without executing a command' do + subject.stub(:applicable_files).and_return(['|echo injected']) + expect { subject.run }.to raise_error(SystemCallError) + end + context 'when XML files have no errors' do before do - REXML::Document.stub(:new) + File.write(staged_file, '') end it { should pass } @@ -24,7 +41,7 @@ context 'when XML file has errors' do before do - REXML::Document.stub(:new).and_raise(REXML::ParseException.new('')) + File.write(staged_file, '') end it { should fail_hook } diff --git a/spec/overcommit/hook_context/commit_msg_spec.rb b/spec/overcommit/hook_context/commit_msg_spec.rb index 7d8b2252..5ad40e3e 100644 --- a/spec/overcommit/hook_context/commit_msg_spec.rb +++ b/spec/overcommit/hook_context/commit_msg_spec.rb @@ -40,6 +40,22 @@ describe '#commit_message' do subject { context.commit_message } + it 'reads a pipe-prefixed filename literally' do + skip 'Windows filenames cannot contain pipes' if Overcommit::OS.windows? + + repo do + file = '|echo injected' + File.write(file, "Literal message\n") + context = described_class.new(config, [file], input) + expect(context.commit_message).to eq("Literal message\n") + end + end + + it 'reports a missing file without executing a command' do + context = described_class.new(config, ['|echo injected'], input) + expect { context.commit_message }.to raise_error(SystemCallError) + end + it 'strips comments and trailing diff' do subject.should == "Some commit message\n" end diff --git a/spec/overcommit/utils_spec.rb b/spec/overcommit/utils_spec.rb index 89e2686a..4e10ac57 100644 --- a/spec/overcommit/utils_spec.rb +++ b/spec/overcommit/utils_spec.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require 'spec_helper' +require 'timeout' require 'securerandom' describe Overcommit::Utils do @@ -92,6 +93,20 @@ described_class.snake_case('HelloWorld').should == 'hello_world' end + it 'preserves acronym boundaries and namespaces' do + expect(described_class.snake_case('HTTPServer::XMLParser-API')). + to eq('http_server/xml_parser_api') + expect(described_class.snake_case('ABcDEFg')).to eq('a_bc_de_fg') + end + + it 'handles long uppercase names without backtracking' do + Timeout.timeout(2) do + expect(described_class.snake_case('A' * 100_000)).to eq('a' * 100_000) + expect(described_class.snake_case("#{'A' * 100_000}Word")). + to eq("#{'a' * 100_000}_word") + end + end + it 'leaves underscored strings as is' do described_class.snake_case('hello_world').should == 'hello_world' end From 6ad65d2a570d8e2a7e7d035406059e01caac7d17 Mon Sep 17 00:00:00 2001 From: Shane da Silva Date: Tue, 10 Nov 2026 21:57:19 -0800 Subject: [PATCH 2/2] Pin CI actions and avoid signing test tags --- .github/workflows/tests.yml | 6 +++--- spec/overcommit/git_repo_spec.rb | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 98edd39f..625c918f 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -51,7 +51,7 @@ jobs: key: dependency-lock-v1-${{ runner.os }}-ruby${{ matrix.ruby-version }}-${{ steps.dependency-cache-date.outputs.date }}-${{ hashFiles('Gemfile', '*.gemspec', 'lib/overcommit/version.rb') }} - name: Set up Ruby ${{ matrix.ruby-version }} - uses: ruby/setup-ruby@v1 + uses: ruby/setup-ruby@b3aaceaef7e0ee231b499e7b5f49c03fc1d32199 # v1 with: ruby-version: ${{ matrix.ruby-version }} bundler-cache: true @@ -89,7 +89,7 @@ jobs: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - name: Set up Ruby - uses: ruby/setup-ruby@v1 + uses: ruby/setup-ruby@b3aaceaef7e0ee231b499e7b5f49c03fc1d32199 # v1 with: ruby-version: 3.3 bundler-cache: true @@ -131,7 +131,7 @@ jobs: # Shard assignments shift when examples are added. Compare the complete # suite's coverage rather than unrelated fractions from individual shards. - name: Report combined code coverage - uses: coverallsapp/github-action@v2 + uses: coverallsapp/github-action@8d6379e14d29928660c4ba802d8e85393440b329 # v2 with: github-token: ${{ secrets.github_token }} files: coverage/*/lcov.info diff --git a/spec/overcommit/git_repo_spec.rb b/spec/overcommit/git_repo_spec.rb index f985071b..90aab79f 100644 --- a/spec/overcommit/git_repo_spec.rb +++ b/spec/overcommit/git_repo_spec.rb @@ -571,7 +571,7 @@ def stage_files(paths) context 'when a tag has the same name as a branch' do let(:commit_ref) { 'HEAD~1' } - before { `git tag topic` } + before { `git -c tag.gpgSign=false tag topic` } it 'returns the branch name without a disambiguating prefix' do expect(subject.sort).to eq(%w[master topic])