diff --git a/CHANGELOG.md b/CHANGELOG.md index c620806d..dc4ced45 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ## 0.75.0 +* Remember all locally approved configuration and hook signatures across rebases + * Gracefully handle hook loading on systems with non-UTF8 locale * Re-enable running test suite on Windows environments diff --git a/README.md b/README.md index 37abc7c7..59bc5ff2 100644 --- a/README.md +++ b/README.md @@ -721,22 +721,37 @@ that executes some malicious code. When you fetch and checkout this pull request, the `post-checkout` hook will be run on your machine, along with the malicious code that you just checked out. -Overcommit attempts to address this problem by storing a signature of your -configuration and all hook plugin code since the last time it ran. When the -signature changes, a warning is displayed alerting you to which plugins have -changed. It is then up to you to manually verify that the changes are not -malicious, and then continue running the hooks. - -The signature is derived from the contents of the plugin's source code itself -and any configuration for the plugin. Thus a change to the plugin's source -code or your local repo's `.overcommit.yml` file could result in a signature -change. - -Signatures are stored in your local repo's git configuration. If you use -multiple [worktrees](https://git-scm.com/docs/git-worktree) whose -configurations differ, enable `extensions.worktreeConfig` (`git config -extensions.worktreeConfig true`) so that Overcommit stores signatures per -worktree and signing in one worktree does not invalidate the others. +Overcommit stores signatures of configurations and hook plugin code you have +explicitly approved with `overcommit --sign` and `overcommit --sign `. +Unrecognized signatures trigger a warning so you can verify the changes before +running hooks. All previously approved signatures are remembered, so checking +out an older revision or rebasing through approved configurations does not +require signing them again. + +A plugin's signature covers its source code together with its configuration. +Approving two versions does not approve other combinations of their code and +configuration. + +Approvals are stored under `overcommit-signatures` in your repository's Git +directory, outside Git's object database. They cannot be committed, pushed, or +fetched, and Git garbage collection does not remove them. Each approval is a +small file at `/`, so lookups +do not scan the history. The file contains the original config key (such as +`overcommit.configuration.signature` or `overcommit.PreCommit.MyHook.signature`) +and the approved signature, one per line. There is no history size limit. + +To revoke one approval, delete its file. To revoke all approvals, delete the +`overcommit-signatures` directory. Run `overcommit --sign` and +`overcommit --sign ` to approve the current versions again. Existing +signatures in local Git config migrate automatically on first use; their old +keys are removed so deleting approval files also revokes migrated signatures. + +By default, linked [worktrees](https://git-scm.com/docs/git-worktree) share +history in their common Git directory. Enable `extensions.worktreeConfig` +(`git config extensions.worktreeConfig true`) to keep approvals and the +`verify_signatures` setting separate in each worktree's Git directory. Use +`git rev-parse --git-common-dir` to find shared storage, or +`git rev-parse --git-path overcommit-signatures` to find per-worktree storage. ### Disabling Signature Checking diff --git a/lib/overcommit.rb b/lib/overcommit.rb index 15fef94f..0dda5dab 100644 --- a/lib/overcommit.rb +++ b/lib/overcommit.rb @@ -24,3 +24,4 @@ require 'overcommit/installer' require 'overcommit/logger' require 'overcommit/version' +require 'overcommit/signature' diff --git a/lib/overcommit/configuration.rb b/lib/overcommit/configuration.rb index f5151031..8d955c86 100644 --- a/lib/overcommit/configuration.rb +++ b/lib/overcommit/configuration.rb @@ -190,12 +190,11 @@ def plugin_hook?(hook_context_or_type, hook_name) File.exist?(File.join(plugin_directory, hook_type_name, "#{hook_name}.rb")) end - # Return whether the signature for this configuration has changed since it - # was last calculated. + # Return whether this configuration has been explicitly trusted before. # # @return [true,false] - def signature_changed? - signature != stored_signature + def signature_verified? + signature_store.include?(signature) end # Return whether a previous signature has been recorded for this @@ -203,7 +202,7 @@ def signature_changed? # # @return [true,false] def previous_signature? - !stored_signature.empty? + signature_store.recorded? end # Returns whether this configuration should verify itself by checking the @@ -233,13 +232,10 @@ def verify_signatures? # Update the currently stored signature for this hook. def update_signature! + signature_store.add(signature) scope = Overcommit::GitConfig.signature_scope - result = Overcommit::Utils.execute( - %W[git config #{scope}] + [signature_config_key, signature] - ) - verify_signature_value = @hash['verify_signatures'] ? 1 : 0 - result &&= Overcommit::Utils.execute( + result = Overcommit::Utils.execute( %W[git config #{scope} #{verify_signature_config_key} #{verify_signature_value}] ) @@ -320,26 +316,8 @@ def signature Digest::SHA256.hexdigest(@hash.to_json) end - # Returns the stored signature of this repo's Overcommit configuration. - # - # This is intended to be compared against the current signature of this - # configuration object. - # - # @return [String] - def stored_signature - scope = Overcommit::GitConfig.signature_scope - result = Overcommit::Utils.execute( - %W[git config #{scope} --get] + [signature_config_key] - ) - - if result.status == 1 # Key doesn't exist - return '' - elsif result.status != 0 - raise Overcommit::Exceptions::GitConfigError, - "Unable to read from local repo git config: #{result.stderr}" - end - - result.stdout.chomp + def signature_store + @signature_store ||= Overcommit::Signature.new(signature_config_key) end def signature_config_key diff --git a/lib/overcommit/configuration_loader.rb b/lib/overcommit/configuration_loader.rb index 8fd68a01..5473e8ce 100644 --- a/lib/overcommit/configuration_loader.rb +++ b/lib/overcommit/configuration_loader.rb @@ -95,7 +95,7 @@ def verify_signatures(config) "No previously recorded signature for configuration file.\n" \ 'Run `overcommit --sign` if you trust the hooks in this repository.' - elsif config.signature_changed? + elsif !config.signature_verified? raise Overcommit::Exceptions::ConfigurationSignatureChanged, "Signature of configuration file has changed!\n" \ "Run `overcommit --sign` once you've verified the configuration changes." diff --git a/lib/overcommit/exceptions.rb b/lib/overcommit/exceptions.rb index 050982fb..87011023 100644 --- a/lib/overcommit/exceptions.rb +++ b/lib/overcommit/exceptions.rb @@ -13,6 +13,9 @@ class ConfigurationSignatureChanged < Error; end # Raised when trying to read/write to/from the local repo git config fails. class GitConfigError < Error; end + # Raised when local signature history cannot be read or written. + class SignatureError < Error; end + # Raised when there was a problem reading submodule information for a repo. class GitSubmoduleError < Error; end diff --git a/lib/overcommit/hook_loader/plugin_hook_loader.rb b/lib/overcommit/hook_loader/plugin_hook_loader.rb index d168a0de..12d4fe45 100644 --- a/lib/overcommit/hook_loader/plugin_hook_loader.rb +++ b/lib/overcommit/hook_loader/plugin_hook_loader.rb @@ -7,7 +7,7 @@ module Overcommit::HookLoader # is running in. class PluginHookLoader < Base def load_hooks - check_for_modified_plugins if @config.verify_signatures? + check_for_unverified_plugins if @config.verify_signatures? hooks = plugin_paths.map do |plugin_path| require plugin_path @@ -22,9 +22,9 @@ def load_hooks end def update_signatures - log.success('No plugin signatures have changed') if modified_plugins.empty? + log.success('All plugin signatures have been verified') if unverified_plugins.empty? - modified_plugins.each do |plugin| + unverified_plugins.each do |plugin| plugin.update_signature! log.warning "Updated signature of plugin #{plugin.hook_name}" end @@ -47,20 +47,20 @@ def ad_hoc_hook_names @config.enabled_ad_hoc_hooks(@context) end - def modified_plugins + def unverified_plugins (plugin_hook_names + ad_hoc_hook_names). map { |hook_name| Overcommit::HookSigner.new(hook_name, @config, @context) }. - select(&:signature_changed?) + reject(&:signature_verified?) end - def check_for_modified_plugins - return if modified_plugins.empty? + def check_for_unverified_plugins + return if unverified_plugins.empty? log.bold_warning "The following #{@context.hook_script_name} plugins " \ 'have been added, changed, or had their configuration modified:' log.newline - modified_plugins.each do |signer| + unverified_plugins.each do |signer| log.warning " * #{signer.hook_name} in #{signer.hook_path}" end diff --git a/lib/overcommit/hook_signer.rb b/lib/overcommit/hook_signer.rb index 67870488..3bbf3c6e 100644 --- a/lib/overcommit/hook_signer.rb +++ b/lib/overcommit/hook_signer.rb @@ -58,25 +58,17 @@ def signable_file?(file) file.start_with?(Overcommit::Utils.repo_root) end - # Return whether the signature for this hook has changed since it was last - # calculated. + # Return whether this exact combination of hook code and configuration + # has been explicitly trusted before. # # @return [true,false] - def signature_changed? - signature != stored_signature + def signature_verified? + signature_store.include?(signature) end - # Update the current stored signature for this hook. + # Remember the current signature without forgetting older approvals. def update_signature! - scope = Overcommit::GitConfig.signature_scope - result = Overcommit::Utils.execute( - %W[git config #{scope}] + [signature_config_key, signature] - ) - - unless result.success? - raise Overcommit::Exceptions::GitConfigError, - "Unable to write to local repo git config: #{result.stderr}" - end + signature_store.add(signature) end private @@ -103,20 +95,8 @@ def hook_contents File.read(hook_path) end - def stored_signature - scope = Overcommit::GitConfig.signature_scope - result = Overcommit::Utils.execute( - %W[git config #{scope} --get] + [signature_config_key] - ) - - if result.status == 1 # Key doesn't exist - return '' - elsif result.status != 0 - raise Overcommit::Exceptions::GitConfigError, - "Unable to read from local repo git config: #{result.stderr}" - end - - result.stdout.chomp + def signature_store + @signature_store ||= Overcommit::Signature.new(signature_config_key) end def signature_config_key diff --git a/lib/overcommit/signature.rb b/lib/overcommit/signature.rb new file mode 100644 index 00000000..a61af9ce --- /dev/null +++ b/lib/overcommit/signature.rb @@ -0,0 +1,71 @@ +# frozen_string_literal: true + +require 'digest' +require 'fileutils' + +module Overcommit + # Repository-local approvals, indexed by hook identity and SHA256 signature. + # These files are outside Git's object database so fetching cannot add trust. + class Signature + def initialize(config_key) + @config_key = config_key + scope = Overcommit::GitConfig.signature_scope + root = if scope == '--worktree' + Overcommit::Utils.git_path('overcommit-signatures') + else + File.join(Overcommit::Utils.git_dir, 'overcommit-signatures') + end + @directory = File.join(root, Digest::SHA256.hexdigest(config_key)) + migrate_legacy_signature(scope) + end + + def include?(signature) + File.read(path(signature)) == contents(signature) + rescue Errno::ENOENT + false + rescue SystemCallError => e + raise Overcommit::Exceptions::SignatureError, "Unable to read signature history: #{e}" + end + + def recorded? + File.directory?(@directory) + end + + def add(signature) + FileUtils.mkdir_p(@directory) + File.write(path(signature), contents(signature), mode: 'w', perm: 0o600) + rescue SystemCallError => e + raise Overcommit::Exceptions::SignatureError, "Unable to write signature history: #{e}" + end + + private + + def path(signature) + File.join(@directory, signature) + end + + def contents(signature) + "#{@config_key}\n#{signature}\n" + end + + # Preserve an existing approval before removing its legacy Git config key. + # Removing the key means deleting history files actually revokes trust. + def migrate_legacy_signature(scope) + result = Overcommit::Utils.execute(%W[git config #{scope} --get] + [@config_key]) + return if result.status == 1 + + check_config_result(result) + signature = result.stdout.chomp + add(signature) if signature.match?(/\A[0-9a-f]{64}\z/) + result = Overcommit::Utils.execute(%W[git config #{scope} --unset-all] + [@config_key]) + check_config_result(result) unless result.status == 5 # Already removed by another process. + end + + def check_config_result(result) + return if result.success? + + raise Overcommit::Exceptions::GitConfigError, + "Unable to migrate local repo git config signature: #{result.stderr}" + end + end +end diff --git a/spec/integration/signature_history_spec.rb b/spec/integration/signature_history_spec.rb new file mode 100644 index 00000000..0d4eca07 --- /dev/null +++ b/spec/integration/signature_history_spec.rb @@ -0,0 +1,154 @@ +# frozen_string_literal: true + +require 'spec_helper' +require 'yaml' + +describe 'remembering previously approved configurations and hooks' do + let(:enable_verification) { true } + let(:plugin_path) { '.git-hooks/pre_commit/history_hook.rb' } + let(:original_config) do + { + 'verify_signatures' => true, + 'CommitMsg' => { 'ALL' => { 'enabled' => false } }, + 'PreCommit' => { + 'ALL' => { 'enabled' => false }, + 'HistoryHook' => { 'enabled' => true, 'requires_files' => false }, + }, + } + end + let(:original_plugin) do + <<~RUBY_CODE + module Overcommit::Hook::PreCommit + class HistoryHook < Base + def run + :pass + end + end + end + RUBY_CODE + end + + def run_command(*command) + result = shell(command) + expect(result.status).to eq(0), "#{command.inspect}: #{result.stdout}\n#{result.stderr}" + result + end + + def sign + run_command('overcommit', '--sign') + run_command('overcommit', '--sign', 'pre-commit') + end + + def commit + shell(%w[git commit --allow-empty -m Test]) + end + + around do |example| + repo do + echo(original_config.to_yaml, '.overcommit.yml') + FileUtils.mkdir_p(File.dirname(plugin_path)) + echo(original_plugin, plugin_path) + install_overcommit_hooks + run_command('git', 'add', '.') + sign + run_command('git', 'commit', '-m', 'Original') + @original = get_sha1('HEAD') + + changed = original_config.merge('HistoryOption' => 'changed') + echo(changed.to_yaml, '.overcommit.yml') + echo("#{original_plugin}\n# Changed plugin", plugin_path) + run_command('git', 'add', '.') + sign + run_command('git', 'commit', '-m', 'Changed') + @changed = get_sha1('HEAD') + example.run + end + end + + it 'allows checkout between both signed configurations without signing again' do + [@original, @changed, @original].each do |revision| + run_command('git', 'checkout', '--detach', revision) + commit.status.should == 0 + end + end + + it 'rebases onto an older approved configuration and stays on the branch' do + run_command('git', 'checkout', '-b', 'feature') + echo('feature', 'feature.txt') + run_command('git', 'add', 'feature.txt') + run_command('git', 'commit', '-m', 'Feature') + + result = run_command('git', 'rebase', '--onto', @original, @changed) + result.stderr.should_not include('Signature of configuration file has changed') + run_command('git', 'symbolic-ref', '--short', 'HEAD').stdout.chomp.should == 'feature' + File.read('.overcommit.yml').should == original_config.to_yaml + File.read(plugin_path).should == original_plugin + File.read('feature.txt').should == "feature\n" + commit.status.should == 0 + end + + it 'still rejects a configuration that has never been signed' do + echo(original_config.merge('HistoryOption' => 'never approved').to_yaml, '.overcommit.yml') + result = commit + result.status.should_not == 0 + result.stderr.should include('Signature of configuration file has changed') + end + + it 'still rejects plugin code that has never been signed' do + echo("#{original_plugin}\n# Never approved", plugin_path) + run_command('git', 'add', plugin_path) + result = commit + result.status.should_not == 0 + result.stderr.should include('HistoryHook') + end + + it 'requires approval for a new combination of previously signed code and configuration' do + # The plugin configuration and its code are signed together. Approval for + # two combinations must not allow arbitrary recombinations of their parts. + changed = original_config + changed['PreCommit']['HistoryHook']['new_option'] = true + echo(changed.to_yaml, '.overcommit.yml') + sign + echo(original_plugin, plugin_path) + run_command('git', 'add', '.') + result = commit + result.status.should_not == 0 + result.stderr.should include('HistoryHook') + end + + it 'allows a signed old plugin with the same configuration after reverting its code' do + echo(original_plugin, plugin_path) + run_command('git', 'add', plugin_path) + commit.status.should == 0 + end + + it 'migrates configuration and plugin approvals created by older releases' do + logger = Overcommit::Logger.new(StringIO.new) + config = Overcommit::ConfigurationLoader.new(logger, verify: false).load_repo_config + # These are the pre-history signature formats, independent of the new store. + config_signature = Digest::SHA256.hexdigest(config.instance_variable_get(:@hash).to_json) + hook_config = config.for_hook('HistoryHook', 'PreCommit').dup + hook_config.delete('skip') + hook_signature = Digest::SHA256.hexdigest(File.read(plugin_path) + hook_config.to_s) + FileUtils.rm_rf(File.join(Overcommit::Utils.git_dir, 'overcommit-signatures')) + run_command('git', 'config', '--local', 'overcommit.configuration.signature', config_signature) + run_command('git', 'config', '--local', + 'overcommit.PreCommit.HistoryHook.signature', hook_signature) + + commit.status.should == 0 + %w[overcommit.configuration.signature overcommit.PreCommit.HistoryHook.signature].each do |key| + shell(%W[git config --local --get #{key}]).status.should == 1 + end + FileUtils.rm_rf(File.join(Overcommit::Utils.git_dir, 'overcommit-signatures')) + commit.status.should_not == 0 + end + + it 'requires signing again after all local history is deleted' do + FileUtils.rm_rf(File.join(Overcommit::Utils.git_dir, 'overcommit-signatures')) + result = commit + result.status.should_not == 0 + result.stderr.should include('No previously recorded signature') + sign + commit.status.should == 0 + end +end diff --git a/spec/integration/worktree_signing_spec.rb b/spec/integration/worktree_signing_spec.rb index c992829b..514652fb 100644 --- a/spec/integration/worktree_signing_spec.rb +++ b/spec/integration/worktree_signing_spec.rb @@ -62,10 +62,9 @@ def commit_in_linked_worktree context 'and extensions.worktreeConfig is not enabled' do let(:worktree_config) { false } - it 'reports a signature error in the worktree signed first' do - result = commit_in_main_worktree - result.status.should_not == 0 - result.stderr.should include 'Signature of configuration file has changed!' + it 'remembers both approved configurations in the shared history' do + commit_in_main_worktree.status.should == 0 + commit_in_linked_worktree.status.should == 0 end end @@ -101,4 +100,26 @@ def commit_in_linked_worktree end end end + + context 'when verification is enabled and only the main worktree is signed' do + let(:sign_linked_worktree) { false } + + context 'and extensions.worktreeConfig is not enabled' do + let(:worktree_config) { false } + + it 'shares approval for an identical configuration' do + commit_in_linked_worktree.status.should == 0 + end + end + + context 'and extensions.worktreeConfig is enabled' do + let(:worktree_config) { true } + + it 'does not share approval with the unsigned worktree' do + result = commit_in_linked_worktree + result.status.should_not == 0 + result.stderr.should include 'No previously recorded signature for configuration file.' + end + end + end end diff --git a/spec/overcommit/hook_signer_spec.rb b/spec/overcommit/hook_signer_spec.rb index 98862fd0..8bef1090 100644 --- a/spec/overcommit/hook_signer_spec.rb +++ b/spec/overcommit/hook_signer_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe Overcommit::HookSigner do - describe '#signature_changed?' do + describe '#signature_verified?' do let(:config) { double('config') } let(:context) { double('context') } @@ -11,10 +11,11 @@ described_class.new('.git-hooks/pre_commit/some_path.rb', config, context) end - let(:hook_config) { { 'enabled' => false } } + let(:original_hook_config) { { 'enabled' => false } } + let(:hook_config) { original_hook_config } let(:modified_hook_config) { hook_config } - let(:hook_contents) { <<-RUBY } + let(:original_hook_contents) { <<-RUBY } module Overcommit::Hook::PreCommit class SomeHook def run @@ -23,10 +24,10 @@ def run end end RUBY - + let(:hook_contents) { original_hook_contents } let(:modified_hook_contents) { hook_contents } - subject { signer.signature_changed? } + subject { signer.signature_verified? } around do |example| repo do @@ -49,17 +50,7 @@ def run signer.stub(:hook_contents).and_return(modified_hook_contents) end - context 'when the hook code and config are the same' do - it { should == false } - - context 'and the user has specified they wish to skip the hook' do - let(:modified_hook_config) { hook_config.merge('skip' => true) } - - it { should == false } - end - end - - context 'when the hook code has changed' do + shared_context 'hook code has changed' do let(:modified_hook_contents) { <<-RUBY } module Overcommit::Hook::PreCommit class SomeHook @@ -69,14 +60,70 @@ def run end end RUBY + end - it { should == true } + shared_context 'hook code has changed back' do + include_context 'hook code has changed' + before do + signer.update_signature! + config.stub(:for_hook).and_return(modified_hook_config) + signer.stub(:hook_contents).and_return(original_hook_contents) + end end - context 'when the hook config has changed' do + shared_context 'hook config has changed' do let(:modified_hook_config) { { 'enabled' => true } } + end + shared_context 'hook config has changed back' do + include_context 'hook config has changed' + before do + signer.update_signature! + config.stub(:for_hook).and_return(original_hook_config) + signer.stub(:hook_contents).and_return(modified_hook_contents) + end + end + + shared_examples 'signature has been verified' do it { should == true } end + + shared_examples 'signature has not been verified' do + it { should == false } + end + + context 'when the hook code and config are the same' do + include_examples 'signature has been verified' + + context 'and the user has specified they wish to skip the hook' do + let(:modified_hook_config) { hook_config.merge('skip' => true) } + + include_examples 'signature has been verified' + end + end + + context 'when the hook code has changed' do + include_context 'hook code has changed' + + include_examples 'signature has not been verified' + end + + context 'when the hook code has changed back to a previously verified signature' do + include_context 'hook code has changed back' + + include_examples 'signature has been verified' + end + + context 'when the hook config has changed' do + include_context 'hook config has changed' + + include_examples 'signature has not been verified' + end + + context 'when the hook config has changed back' do + include_context 'hook config has changed back' + + include_examples 'signature has been verified' + end end end diff --git a/spec/overcommit/signature_spec.rb b/spec/overcommit/signature_spec.rb new file mode 100644 index 00000000..d051fd2a --- /dev/null +++ b/spec/overcommit/signature_spec.rb @@ -0,0 +1,212 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Overcommit::Signature do + let(:key) { 'overcommit.configuration.signature' } + let(:signature) { Digest::SHA256.hexdigest('original configuration') } + let(:other_signature) { Digest::SHA256.hexdigest('changed configuration') } + let(:store) { described_class.new(key) } + let(:root) { File.join(Overcommit::Utils.git_dir, 'overcommit-signatures') } + let(:signature_file) { File.join(root, Digest::SHA256.hexdigest(key), signature) } + + around do |example| + repo { example.run } + end + + it 'does not approve a signature that was never signed' do + store.include?(signature).should == false + store.recorded?.should == false + File.exist?(root).should == false + end + + it 'remembers all approvals across instances without growing Git config' do + store.add(signature) + store.add(other_signature) + restored = described_class.new(key) + restored.include?(signature).should == true + restored.include?(other_signature).should == true + restored.recorded?.should == true + shell(%W[git config --local --get #{key}]).status.should == 1 + end + + it 'signs the same content idempotently' do + 2.times { store.add(signature) } + Dir[File.join(root, '*', '*')].size.should == 1 + store.include?(signature).should == true + end + + it 'indexes approvals independently of the number of historical signatures' do + 1000.times { |i| store.add(Digest::SHA256.hexdigest(i.to_s)) } + store.add(signature) + File.should_receive(:read).once.with(signature_file).and_call_original + Dir.should_not_receive(:glob) + store.include?(signature).should == true + end + + it 'keeps approvals separate for each hook and hook type' do + store.add(signature) + %w[overcommit.PreCommit.Custom.signature + overcommit.PostCheckout.Custom.signature].each do |name| + described_class.new(name).include?(signature).should == false + end + end + + it 'does not derive filesystem paths from hook names' do + described_class.new('overcommit.PreCommit.../../Custom.signature').add(signature) + Dir[File.join(root, '*', '*')].size.should == 1 + end + + it 'revokes one approval by removing its file while preserving others' do + store.add(signature) + store.add(other_signature) + File.delete(signature_file) + restored = described_class.new(key) + restored.include?(signature).should == false + restored.include?(other_signature).should == true + end + + it 'revokes all approvals by removing the history directory' do + store.add(signature) + FileUtils.rm_rf(root) + restored = described_class.new(key) + restored.include?(signature).should == false + restored.recorded?.should == false + end + + it 'rejects a corrupted or truncated approval file' do + store.add(signature) + File.write(signature_file, '') + store.include?(signature).should == false + end + + it 'rejects an approval file copied from another hook' do + store.add(signature) + File.write(signature_file, "overcommit.PreCommit.Custom.signature\n#{signature}\n") + store.include?(signature).should == false + end + + it 'does not lose approvals when Git garbage collects unreachable objects' do + store.add(signature) + shell(%w[git gc --prune=now]).status.should == 0 + described_class.new(key).include?(signature).should == true + end + + it 'does not trust signature blobs received through a remote tag' do + # Reproduce the original implementation's trust-injection vulnerability. + store.add(signature) + File.write('untrusted-configuration', 'changed configuration') + config_id = shell(%w[git hash-object untrusted-configuration]).stdout.chomp + # The old implementation would accept this blob's presence as approval. + File.write('injected-signature', "#{config_id} #{other_signature}") + object_id = shell(%w[git hash-object -w injected-signature]).stdout.chomp + shell(%W[git update-ref refs/tags/forged-approval #{object_id}]).status.should == 0 + destination = File.join(File.dirname(Dir.pwd), 'clone') + shell(['git', 'clone', '--no-local', Dir.pwd, destination]).status.should == 0 + Dir.chdir(destination) do + %w[git_dir repo_root].each { |name| Overcommit::Utils.instance_variable_set("@#{name}", nil) } + shell(%W[git cat-file -e #{object_id}]).status.should == 0 + cloned = described_class.new(key) + cloned.include?(signature).should == false + cloned.include?(other_signature).should == false + end + end + + context 'with a legacy approval in Git config' do + before do + shell(%W[git config --local #{key} #{signature}]).status.should == 0 + end + + it 'migrates the old approval and removes the old key' do + store.include?(signature).should == true + shell(%W[git config --local --get #{key}]).status.should == 1 + store.include?(other_signature).should == false + end + + it 'preserves the old approval when signing a new configuration first' do + store.add(other_signature) + store.include?(signature).should == true + store.include?(other_signature).should == true + end + + it 'does not resurrect a revoked legacy approval' do + store.include?(signature).should == true + FileUtils.rm_rf(root) + described_class.new(key).include?(signature).should == false + end + + it 'leaves the legacy approval intact if writing history fails' do + FileUtils.should_receive(:mkdir_p).and_raise(Errno::EACCES) + expect do + store + end.to raise_error(Overcommit::Exceptions::SignatureError, /write signature history/) + shell(%W[git config --local --get #{key}]).stdout.chomp.should == signature + end + + it 'does not accept an invalid legacy digest as a path' do + shell(%W[git config --local #{key} ../../untrusted]).status.should == 0 + store.include?(signature).should == false + File.exist?(root).should == false + end + end + + context 'with worktree configuration enabled' do + before { shell(%w[git config --local extensions.worktreeConfig true]) } + + it 'migrates only the approval in worktree config' do + shell(%W[git config --local #{key} #{other_signature}]).status.should == 0 + shell(%W[git config --worktree #{key} #{signature}]).status.should == 0 + store.include?(signature).should == true + store.include?(other_signature).should == false + shell(%W[git config --worktree --get #{key}]).status.should == 1 + shell(%W[git config --local --get #{key}]).stdout.chomp.should == other_signature + end + end + + it 'supports repositories whose Git directory is stored separately' do + destination = File.join(File.dirname(Dir.pwd), 'repository metadata') + shell(['git', 'init', '--separate-git-dir', destination]).status.should == 0 + Overcommit::Utils.instance_variable_set(:@git_dir, nil) + store.add(signature) + File.file?('.git').should == true + Dir[File.join(destination, 'overcommit-signatures', '*', '*')].size.should == 1 + described_class.new(key).include?(signature).should == true + end + + it 'handles concurrent approval writes without forgetting any signatures' do + # Separate instances have no shared Ruby state, as with concurrent hook runs. + stores = Array.new(4) { described_class.new(key) } + signatures = Array.new(40) { |i| Digest::SHA256.hexdigest(i.to_s) } + threads = stores.each_with_index.map do |history, index| + Thread.new { signatures.rotate(index * 10).each { |sig| history.add(sig) } } + end + threads.each(&:value) + restored = described_class.new(key) + signatures.each { |sig| restored.include?(sig).should == true } + end + + it 'fails closed if reading an approval fails' do + store.add(signature) + File.should_receive(:read).with(signature_file).and_raise(Errno::EACCES) + expect { store.include?(signature) }. + to raise_error(Overcommit::Exceptions::SignatureError, /read signature history/) + end + + it 'reports Git config read errors' do + Overcommit::Utils.stub(:execute).and_call_original + result = double(status: 2, success?: false, stderr: 'cannot read config') + Overcommit::Utils.should_receive(:execute). + with(%W[git config --local --get #{key}]).and_return(result) + expect { store }.to raise_error(Overcommit::Exceptions::GitConfigError, /cannot read config/) + end + + it 'reports Git config removal errors without losing the migrated approval' do + shell(%W[git config --local #{key} #{signature}]).status.should == 0 + Overcommit::Utils.stub(:execute).and_call_original + result = double(status: 2, success?: false, stderr: 'cannot lock config') + Overcommit::Utils.should_receive(:execute). + with(%W[git config --local --unset-all #{key}]).and_return(result) + expect { store }.to raise_error(Overcommit::Exceptions::GitConfigError, /cannot lock config/) + File.read(signature_file).should include(signature) + end +end