Fix all open CodeQL security findings

.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 }})
@@ -48,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
@@ -86,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
@@ -128,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

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{(?<!\d)(\d++)\s*([+\-*\/])\s*(\d+)})[0]
             if a
               a.to_i.send(op, b.to_i)
             else

lib/overcommit/git_repo.rb

@@ -292,10 +292,11 @@ def submodules(options = {})
     # @param commit_ref [String] git tree ref that resolves to a commit
     # @return [Array<String>] 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.

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 = /^(?<file>[^\s](?:\w:)?[^:]+):[^\d]+(?<line>\d+).*?(?<type>Error|Warning)/
+      eslint_regex = /^(?<file>[^\s](?:\w:)?[^:]+):[^\d]+(?<line>\d++).*?(?<type>Error|Warning)/
       result = execute(command, args: applicable_files)
       output = result.stdout.chomp
       messages = output.split("\n").grep(eslint_regex)

lib/overcommit/hook/pre_commit/haml_lint.rb

@@ -15,7 +15,7 @@ def run
 
       extract_messages(
         result.stdout.split("\n"),
-        /^(?<file>(?:\w:)?[^:]+):(?<line>\d+)[^ ]* (?<type>[^ ]+)/,
+        /^(?<file>(?:\w:)?[^:]+):(?<line>\d++)[^ ]* (?<type>[^ ]+)/,
         MESSAGE_TYPE_CATEGORIZER,
       )
     end

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/),
-        /^(?<file>(?:\w:)?[^:]+):[^\d]+(?<line>\d+).+\((?<type>E|W)\d+\)/,
+        /^(?<file>(?:\w:)?[^:]+):[^\d]+(?<line>\d++).+\((?<type>E|W)\d+\)/,
         lambda { |type| type.include?('W') ? :warning : :error }
       )
     end

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)

lib/overcommit/hook/pre_commit/reek.rb

@@ -13,7 +13,7 @@ def run
 
       extract_messages(
         output,
-        /^\s*(?<file>(?:\w:)?[^:]+):(?<line>\d+):/,
+        /^\s*+(?<file>(?:\w:)?[^:]+):(?<line>\d+):/,
       )
     end
 

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,
-        /^(?<file>[^:]+):(?<line>\d+):\s*(?<type>[^,]+),\s*(?<message>.+)/,
+        /^(?<file>[^:]+):(?<line>\d+):\s*+(?<type>[^,]+),\s*+(?<message>.+)/,
         MESSAGE_TYPE_CATEGORIZER
       )
     end

lib/overcommit/hook/pre_commit/slim_lint.rb

@@ -15,7 +15,7 @@ def run
 
       extract_messages(
         result.stdout.split("\n"),
-        /^(?<file>(?:\w:)?[^:]+):(?<line>\d+)[^ ]* (?<type>[^ ]+)/,
+        /^(?<file>(?:\w:)?[^:]+):(?<line>\d++)[^ ]* (?<type>[^ ]+)/,
         MESSAGE_TYPE_CATEGORIZER,
       )
     end

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)

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

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

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) }
+
+    {
+      '%<processors>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
       {

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 -c tag.gpgSign=false 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' }
 

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 }

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

spec/overcommit/hook/pre_commit/xml_syntax_spec.rb

@@ -10,21 +10,38 @@
   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, '<root/>')
+    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, '<root/>')
     end
 
     it { should pass }
   end
 
   context 'when XML file has errors' do
     before do
-      REXML::Document.stub(:new).and_raise(REXML::ParseException.new(''))
+      File.write(staged_file, '<root></wrong>')
     end
 
     it { should fail_hook }

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