diff --git a/lib/cli/kit/config.rb b/lib/cli/kit/config.rb index e817adf..970555a 100644 --- a/lib/cli/kit/config.rb +++ b/lib/cli/kit/config.rb @@ -14,11 +14,9 @@ class Config # pointing into +/nix/store+). # # Inherits from SystemCallError so existing `rescue SystemCallError` - # handlers around +Config#set+ continue to match. The message contains - # only the keys that actually changed; unchanged keys (which may - # include sensitive values such as API tokens) are intentionally - # excluded so the failure message can never leak secrets through - # stderr or exception reports. + # handlers around +Config#set+ continue to match. Diagnostics contain + # only changed section/key names, never their values. Config contents + # are not retained on the exception, to keep them out of error reports. class ConfigWriteError < SystemCallError class << self # Ruby's +SystemCallError.===+ uses errno-based matching, @@ -41,10 +39,10 @@ def ===(other) # checkers and callers see the real signature, and allocate # the instance manually to bypass +SystemCallError+'s # factory behaviour. - #: (String config_path, String old_content, String new_content, SystemCallError cause) -> ConfigWriteError - def new(config_path, old_content, new_content, cause) + #: (String config_path, Hash[String, Hash[String, String]] old_config, Hash[String, Hash[String, String]] new_config, SystemCallError cause) -> ConfigWriteError + def new(config_path, old_config, new_config, cause) instance = allocate - instance.__send__(:initialize, config_path, old_content, new_content, cause) + instance.__send__(:initialize, config_path, old_config, new_config, cause) instance end end @@ -53,10 +51,7 @@ def new(config_path, old_content, new_content, cause) attr_reader :config_path #: String - attr_reader :old_content - - #: String - attr_reader :new_content + attr_reader :diff # rubocop:disable Lint/MissingSuper # +SystemCallError#initialize+ has a factory-style signature that @@ -65,16 +60,15 @@ def new(config_path, old_content, new_content, cause) # initialize via Exception so we just get a message-only # exception that +rescue SystemCallError+ still catches via # inheritance. +super+ would not work here. - #: (String config_path, String old_content, String new_content, SystemCallError cause) -> void - def initialize(config_path, old_content, new_content, cause) + #: (String config_path, Hash[String, Hash[String, String]] old_config, Hash[String, Hash[String, String]] new_config, SystemCallError cause) -> void + def initialize(config_path, old_config, new_config, cause) @config_path = config_path - @old_content = old_content - @new_content = new_content + @diff = build_diff(old_config, new_config) @wrapped_errno = cause.errno message = <<~MSG.rstrip Could not write to #{config_path}: #{cause.message} - Attempted changes (unchanged keys omitted): + Attempted changes (values and unchanged keys omitted): #{diff} MSG Exception.instance_method(:initialize).bind(self).call(message) @@ -88,19 +82,14 @@ def errno @wrapped_errno end - # A line-by-line diff of only the sections/keys that changed - # between +old_content+ and +new_content+. Unchanged keys are - # omitted so that sensitive values stored elsewhere in the config - # are never included in the failure message. - #: -> String - def diff - old_ini = CLI::Kit::Ini.new(config: @old_content).tap(&:parse).ini - new_ini = CLI::Kit::Ini.new(config: @new_content).tap(&:parse).ini + private + #: (Hash[String, Hash[String, String]] old_config, Hash[String, Hash[String, String]] new_config) -> String + def build_diff(old_config, new_config) lines = [] - (old_ini.keys | new_ini.keys).each do |section| - old_section = old_ini[section] || {} - new_section = new_ini[section] || {} + (old_config.keys | new_config.keys).each do |section| + old_section = old_config[section] || {} + new_section = new_config[section] || {} changes = [] (old_section.keys | new_section.keys).each do |key| @@ -108,8 +97,8 @@ def diff new_val = new_section[key] next if old_val == new_val - changes << "- #{key} = #{old_val}" if old_val - changes << "+ #{key} = #{new_val}" if new_val + changes << "- #{key}" if old_val + changes << "+ #{key}" if new_val end next if changes.empty? @@ -270,8 +259,8 @@ def write_config # for nix/home-manager configs that live in +/nix/store+. Wrap # it the same way as +EACCES+/+EPERM+ so callers always see # the diff and any +rescue ConfigWriteError+ handler matches. - old_content = read_config_for_diff(config_path) - raise(ConfigWriteError.new(config_path, old_content, new_content, e)) + old_config = read_config_for_diff(config_path) + raise(ConfigWriteError.new(config_path, old_config, all_configs, e)) end end @@ -306,11 +295,11 @@ def write_config_atomic(config_path, config_dir, new_content) end end - #: (String config_path) -> String + #: (String config_path) -> Hash[String, Hash[String, String]] def read_config_for_diff(config_path) - File.read(config_path) + CLI::Kit::Ini.new(config: File.read(config_path)).tap(&:parse).ini rescue SystemCallError - '' + {} end # Resolve +config_path+ through any symlinks so the atomic rename diff --git a/test/cli/kit/config_test.rb b/test/cli/kit/config_test.rb index bf1859f..b17a7ea 100644 --- a/test/cli/kit/config_test.rb +++ b/test/cli/kit/config_test.rb @@ -163,8 +163,8 @@ def test_write_to_readonly_dir_raises_config_write_error end assert_includes(error.message, @file) - assert_includes(error.message, '- key = original') - assert_includes(error.message, '+ key = updated') + assert_includes(error.message, '- key') + assert_includes(error.message, '+ key') ensure FileUtils.chmod(0o755, config_dir) if config_dir && File.directory?(config_dir) end @@ -180,7 +180,7 @@ def test_config_write_error_includes_new_sections end assert_includes(error.message, '+ [hooks]') - assert_includes(error.message, '+ path_check_enabled = false') + assert_includes(error.message, '+ path_check_enabled') ensure FileUtils.chmod(0o755, config_dir) if config_dir && File.directory?(config_dir) end @@ -216,12 +216,68 @@ def test_config_write_error_omits_unchanged_sensitive_values '[buildkite]', 'sections with no changes must not appear in the error message', ) - assert_includes(error.message, '- path_check_enabled = true') - assert_includes(error.message, '+ path_check_enabled = false') + refute_includes(Marshal.dump(error), 'super_secret_token_xyz') + assert_includes(error.message, '- path_check_enabled') + assert_includes(error.message, '+ path_check_enabled') ensure FileUtils.chmod(0o755, config_dir) if config_dir && File.directory?(config_dir) end + def test_config_write_error_omits_new_sensitive_values + Tempfile.stubs(:new).raises(Errno::EACCES.new(@file)) + + error = assert_raises(Config::ConfigWriteError) do + @config.set('buildkite', 'api_token', 'new_secret_token') + end + + assert_equal("+ [buildkite]\n+ api_token", error.diff) + assert_includes(error.message, error.diff) + refute_includes(error.message, 'new_secret_token') + refute_includes(Marshal.dump(error), 'new_secret_token') + end + + def test_config_write_error_omits_rotated_sensitive_values + @config.set('buildkite', 'api_token', 'old_secret_token') + Tempfile.stubs(:new).raises(Errno::EROFS.new(@file)) + + error = assert_raises(Config::ConfigWriteError) do + @config.set('buildkite', 'api_token', 'new_secret_token') + end + + assert_equal(" [buildkite]\n- api_token\n+ api_token", error.diff) + assert_includes(error.message, error.diff) + ['old_secret_token', 'new_secret_token'].each do |value| + refute_includes(error.message, value) + refute_includes(Marshal.dump(error), value) + end + end + + def test_config_write_error_omits_deleted_sensitive_values + @config.set('buildkite', 'api_token', 'old_secret_token') + Tempfile.stubs(:new).raises(Errno::EPERM.new(@file)) + + error = assert_raises(Config::ConfigWriteError) do + @config.unset('buildkite', 'api_token') + end + + assert_equal("- [buildkite]\n- api_token", error.diff) + assert_includes(error.message, error.diff) + refute_includes(error.message, 'old_secret_token') + refute_includes(Marshal.dump(error), 'old_secret_token') + end + + def test_config_write_error_does_not_parse_multiline_values_as_keys_or_sections + Tempfile.stubs(:new).raises(Errno::EACCES.new(@file)) + + error = assert_raises(Config::ConfigWriteError) do + @config.set('buildkite', 'api_token', "secret_line_one\n[secret_section]\nsecret_key = secret_value") + end + + assert_equal("+ [buildkite]\n+ api_token", error.diff) + refute_includes(error.message, 'secret_') + refute_includes(Marshal.dump(error), 'secret_') + end + def test_config_write_error_is_rescued_as_system_call_error config_dir = File.dirname(@file) FileUtils.mkdir_p(config_dir) @@ -260,8 +316,8 @@ def test_write_to_readonly_filesystem_raises_config_write_error end assert_includes(error.message, @file) - assert_includes(error.message, '- key = original') - assert_includes(error.message, '+ key = updated') + assert_includes(error.message, '- key') + assert_includes(error.message, '+ key') assert_equal(Errno::EROFS::Errno, error.errno) end @@ -316,8 +372,8 @@ def test_write_preserves_symlink_when_target_is_readonly 'symlink must NOT be replaced with a regular file when the target is read-only', ) assert_equal(target_path, File.readlink(link_path)) - assert_includes(error.message, '- key = original') - assert_includes(error.message, '+ key = updated') + assert_includes(error.message, '- key') + assert_includes(error.message, '+ key') ensure if defined?(target_dir) && target_dir && File.directory?(target_dir) FileUtils.chmod(0o755, target_dir) diff --git a/test/cli/kit/error_handler_test.rb b/test/cli/kit/error_handler_test.rb index 98eec9d..09568d0 100644 --- a/test/cli/kit/error_handler_test.rb +++ b/test/cli/kit/error_handler_test.rb @@ -87,6 +87,37 @@ def test_out_of_space end end + def test_config_write_failure_omits_values_from_stderr_and_reporter + Dir.mktmpdir do |dir| + with_env('XDG_CONFIG_HOME' => dir) do + config = Config.new(tool_name: 'tool') + config.set('buildkite', 'api_token', 'old_secret_token') + Tempfile.stubs(:new).raises(Errno::EACCES.new(config.file)) + + reported_error = nil + @rep.expects(:report).once.with do |error, _logs| + reported_error = error + true + end + + out, err, code = with_handler do + config.set('buildkite', 'api_token', 'new_secret_token') + end + + assert_equal('', out) + assert_equal(CLI::Kit::EXIT_BUG, code) + assert_kind_of(Config::ConfigWriteError, reported_error) + assert_includes(err, 'api_token') + assert_includes(reported_error.message, 'api_token') + ['old_secret_token', 'new_secret_token'].each do |value| + refute_includes(err, value) + refute_includes(reported_error.message, value) + refute_includes(Marshal.dump(reported_error), value) + end + end + end + end + def test_out_of_space_with_name @eh = error_handler(tool_name: 'foo') run_test(