Omit config values from write failure diagnostics

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,28 +82,23 @@ 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|
               old_val = old_section[key]
               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

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)

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(