Reach 100% line coverage and fix absolute-path command signing

CHANGELOG.md

@@ -2,6 +2,8 @@
 
 ## main (unreleased)
 
+* Include command file contents in hook signatures when an absolute path is configured.
+
 * Add a configurable `Vale` pre-commit hook to check spelling and style in text and source files (#758).
 
 * Honor Git commit-message cleanup settings when handling comment-prefixed titles (#736).

lib/overcommit/hook_signer.rb

@@ -45,7 +45,7 @@ def hook_path
                   'must be tracked by Git in order to be signed'
           end
 
-          File.join(Overcommit::Utils.repo_root, command.first.to_s)
+          File.expand_path(command.first.to_s, Overcommit::Utils.repo_root)
         end
       end
     end

spec/overcommit/command_splitter_spec.rb

@@ -3,6 +3,47 @@
 require 'spec_helper'
 
 describe Overcommit::CommandSplitter do
+  describe 'platform command limits' do
+    around do |example|
+      previous = described_class.instance_variable_get(:@max_command_length)
+      described_class.instance_variable_set(:@max_command_length, nil)
+      example.run
+    ensure
+      described_class.instance_variable_set(:@max_command_length, previous)
+    end
+
+    shared_examples 'a 2048-byte command limit' do
+      it 'splits commands at the platform limit' do
+        argument = 'a' * 2045
+        result = Overcommit::Subprocess::Result.new(0, '', '')
+        expect(Overcommit::Subprocess).to receive(:spawn).with(['cmd', argument], {}).
+          ordered.and_return(result)
+        expect(Overcommit::Subprocess).to receive(:spawn).with(%w[cmd b], {}).
+          ordered.and_return(result)
+
+        expect(described_class.execute(['cmd'], args: [argument, 'b'])).to be_success
+      end
+    end
+
+    context 'on Windows' do
+      before do
+        allow(Gem).to receive(:win_platform?).and_return(true)
+        expect(described_class).not_to receive(:`)
+      end
+
+      include_examples 'a 2048-byte command limit'
+    end
+
+    context 'on Unix' do
+      before do
+        allow(Gem).to receive(:win_platform?).and_return(false)
+        expect(described_class).to receive(:`).with('getconf ARG_MAX').once.and_return("4096\n")
+      end
+
+      include_examples 'a 2048-byte command limit'
+    end
+  end
+
   describe '.execute' do
     let(:args_prefix) { %w[cmd] }
     let(:max_command_length) { 10 }

spec/overcommit/configuration_validator_spec.rb

@@ -55,6 +55,15 @@
       end
     end
 
+    context 'and a variable name contains an equals sign' do
+      let(:env) { { 'INVALID=NAME' => 'value' } }
+
+      it 'rejects the name and explains which variable is invalid' do
+        expect { subject }.to raise_error(Overcommit::Exceptions::ConfigurationError)
+        expect(output.string).to include('variable name `INVALID=NAME` cannot contain `=`')
+      end
+    end
+
     context 'and it is a hash with integer values' do
       let(:env) { { 'OVERCOMMIT_ENV_VAR' => 1, 'OVERCOMMIT_ENV_VAR_2' => 2 } }
 

spec/overcommit/gem_plugin_spec.rb

@@ -229,6 +229,17 @@ def changed_config(hash)
                                                            /does not satisfy/)
     end
 
+    it 'rejects conflicting versions encountered through a dependency cycle' do
+      dependency.add_runtime_dependency('company-hooks', '>= 1.0')
+      conflicting = specification.dup
+      conflicting.version = '2.0.0'
+      allow(Gem::Specification).to receive(:find_by_name).
+        with('company-hooks', anything).and_return(specification, conflicting)
+
+      expect { plugin.update_signature! }.to raise_error(Overcommit::Exceptions::HookLoadError,
+                                                         /Conflicting versions of company-hooks/)
+    end
+
     it 'activates the resolved gems only when explicitly requested to load them' do
       expect(specification).to receive(:activate)
       expect(dependency).to receive(:activate)

spec/overcommit/hook/pre_push/protected_branches_spec.rb

@@ -117,4 +117,26 @@
 
     it { should pass }
   end
+
+  context 'with configured branch names and patterns' do
+    let(:hook_config) { { 'branches' => ['stable'], 'branch_patterns' => ['release/*'] } }
+    let(:pushed_ref_name) { 'stable' }
+
+    before do
+      allow(subject).to receive(:branch_configurations).and_call_original
+      allow(pushed_ref).to receive(:destructive?).and_return(true)
+    end
+
+    it 'protects explicitly named branches' do
+      expect(subject).to fail_hook
+    end
+
+    context 'when the branch matches a configured pattern' do
+      let(:pushed_ref_name) { 'release/1.0' }
+
+      it 'protects matching branches' do
+        expect(subject).to fail_hook
+      end
+    end
+  end
 end

spec/overcommit/hook/pre_rebase/merged_commits_spec.rb

@@ -55,6 +55,18 @@
       end
 
       it { should fail_hook }
+
+      it 'uses configured protected branches and names them in the failure' do
+        settings = { 'PreRebase' => { 'MergedCommits' => { 'branches' => ['release'] } } }
+        configured = config.merge(Overcommit::Configuration.new(settings))
+        allow(Overcommit::GitRepo).to receive(:branches_containing_commit).
+          with(commit_sha1).and_return(['release'])
+        hook = described_class.new(configured, context)
+
+        expect(hook.run).to eq([
+          :fail, 'Cannot rebase commits that have already been merged into one of release'
+        ])
+      end
     end
   end
 end

spec/overcommit/hook_loader/base_spec.rb

@@ -0,0 +1,10 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Overcommit::HookLoader::Base do
+  it 'requires subclasses to implement hook loading' do
+    loader = described_class.new(double('config'), double('context'), Overcommit::Logger.silent)
+    expect { loader.load_hooks }.to raise_error(NotImplementedError)
+  end
+end

spec/overcommit/hook_loader/plugin_hook_loader_spec.rb

@@ -260,4 +260,10 @@
     allow(reloaded).to receive(:execute).and_return(result)
     expect(reloaded.run.first.file).to eq('file.foo')
   end
+
+  it 'wraps missing ad hoc hook base classes in a hook loading error' do
+    hide_const('Overcommit::Hook::PreCommit::Base')
+    expect { loader.load_hooks }.to raise_error(Overcommit::Exceptions::HookLoadError,
+                                                /Unable to load hook 'CustomScript'/)
+  end
 end

spec/overcommit/hook_loader/repository_plugins_spec.rb

@@ -0,0 +1,51 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Overcommit::HookLoader::PluginHookLoader, 'with repository plugins' do
+  let(:enable_verification) { true }
+  let(:config) { Overcommit::ConfigurationLoader.default_configuration }
+  let(:context) { Overcommit::HookContext.create('pre-commit', config, [], StringIO.new) }
+  let(:loader) { described_class.new(config, context, Overcommit::Logger.silent) }
+  let(:plugin_path) { File.join(config.plugin_directory, 'pre_commit/coverage_fixture.rb') }
+
+  around do |example|
+    repo do
+      example.run
+    ensure
+      $LOADED_FEATURES.delete(plugin_path)
+      hook_module = Overcommit::Hook::PreCommit
+      if hook_module.const_defined?(:CoverageFixture, false)
+        hook_module.send(:remove_const, :CoverageFixture)
+      end
+    end
+  end
+
+  before do
+    FileUtils.mkdir_p(File.dirname(plugin_path))
+    File.write(plugin_path, <<~RUBY)
+      module Overcommit::Hook::PreCommit
+        class CoverageFixture < Base
+          def run
+            :pass
+          end
+        end
+      end
+    RUBY
+    Overcommit::Utils.execute(['git', 'add', plugin_path])
+  end
+
+  it 'loads and instantiates a signed repository hook' do
+    loader.update_signatures
+    hook = loader.load_hooks.fetch(0)
+    expect(hook.name).to eq('CoverageFixture')
+    expect(hook.run).to eq(:pass)
+  end
+
+  it 'rejects changed plugin contents before requiring them' do
+    loader.update_signatures
+    File.write(plugin_path, "raise 'unapproved code ran'")
+    expect { loader.load_hooks }.to raise_error(Overcommit::Exceptions::InvalidHookSignature)
+    expect(Overcommit::Hook::PreCommit.const_defined?(:CoverageFixture, false)).to eq(false)
+  end
+end

spec/overcommit/hook_runner_scheduling_spec.rb

@@ -0,0 +1,49 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+require 'timeout'
+
+describe Overcommit::HookRunner, 'processor reservations' do
+  let(:config) { double('config', concurrency: 2) }
+  let(:resource) { ConditionVariable.new }
+  let(:runner) do
+    condition = resource
+    allow(ConditionVariable).to receive(:new).and_return(condition)
+    described_class.new(config, Overcommit::Logger.silent, double('context'), double('printer'))
+  end
+  let(:waiting) { Queue.new }
+  let(:acquired) { Queue.new }
+  let(:hook) { double('hook', parallelize?: true, processors: 2) }
+
+  [1, 2].each do |reserved|
+    it "waits with #{reserved} processors occupied and resumes after they are released" do
+      occupied = double('running hook', parallelize?: true, processors: reserved)
+      scheduler = runner
+      scheduler.send(:wait_for_slot, occupied)
+      allow(resource).to receive(:wait).and_wrap_original do |method, *args|
+        waiting << true
+        method.call(*args)
+      end
+
+      worker = Thread.new do
+        scheduler.send(:wait_for_slot, hook)
+        acquired << true
+      end
+
+      Timeout.timeout(5) do
+        waiting.pop
+        expect(acquired).to be_empty
+        scheduler.send(:release_slot, occupied)
+        acquired.pop
+        worker.join
+      end
+
+      # A second reservation can consume every processor after the waiter releases them.
+      scheduler.send(:release_slot, hook)
+      Timeout.timeout(5) { scheduler.send(:wait_for_slot, hook) }
+    ensure
+      worker&.kill
+      worker&.join
+    end
+  end
+end

spec/overcommit/hook_signer_files_spec.rb

@@ -0,0 +1,52 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Overcommit::HookSigner, 'with repository commands' do
+  let(:enable_verification) { true }
+  let(:script_path) { File.expand_path('hook-script') }
+  let(:command) { script_path }
+  let(:config) do
+    Overcommit::Configuration.new(
+      'PreCommit' => { 'CustomScript' => { 'enabled' => true, 'command' => [command] } }
+    )
+  end
+  let(:context) { double('context', hook_type_name: 'pre_commit', hook_class_name: 'PreCommit') }
+  let(:signer) { described_class.new('CustomScript', config, context) }
+
+  around do |example|
+    repo { example.run }
+  end
+
+  before do
+    File.write(script_path, 'original command')
+  end
+
+  it 'rejects untracked repository commands when signing is enabled' do
+    expect { signer.update_signature! }.to raise_error(Overcommit::Exceptions::InvalidHookDefinition,
+                                                       /must be tracked by Git/)
+  end
+
+  shared_examples 'a signed repository command' do
+    it 'signs the actual file and requires fresh approval when its contents change' do
+      Overcommit::Utils.execute(['git', 'add', script_path])
+      expect(File.realpath(signer.hook_path)).to eq(File.realpath(script_path))
+      signer.update_signature!
+      expect(signer.signature_verified?).to eq(true)
+
+      File.write(script_path, 'modified command')
+      expect(signer.signature_verified?).to eq(false)
+
+      File.write(script_path, 'original command')
+      expect(signer.signature_verified?).to eq(true)
+    end
+  end
+
+  include_examples 'a signed repository command'
+
+  context 'with a relative command path' do
+    let(:command) { ".#{Overcommit::OS.windows? ? '\\' : '/'}hook-script" }
+
+    include_examples 'a signed repository command'
+  end
+end

spec/overcommit/utils_spec.rb

@@ -211,6 +211,18 @@
         it { should be_nil }
       end
 
+      context 'when the process ancestry contains only env.exe launchers' do
+        let(:processes) do
+          [double('launcher', Name: 'env.exe', ParentProcessId: Process.pid)]
+        end
+
+        it 'stops after four queries without launching a fallback' do
+          expect(service).to receive(:ExecQuery).exactly(4).times.and_return(processes)
+          expect(described_class).not_to receive(:execute)
+          expect(subject).to be_nil
+        end
+      end
+
       context 'when the command line is inaccessible' do
         let(:command) { nil }