diff --git a/lib/cli/kit/config.rb b/lib/cli/kit/config.rb index 73124c8..e817adf 100644 --- a/lib/cli/kit/config.rb +++ b/lib/cli/kit/config.rb @@ -2,12 +2,131 @@ require 'cli/kit' require 'fileutils' +require 'tempfile' module CLI module Kit class Config XDG_CONFIG_HOME = 'XDG_CONFIG_HOME' + # Raised when the config file cannot be written (e.g. read-only + # directory, read-only filesystem, nix/home-manager-managed symlink + # 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. + class ConfigWriteError < SystemCallError + class << self + # Ruby's +SystemCallError.===+ uses errno-based matching, + # which means +rescue ConfigWriteError+ would otherwise fail + # to catch a ConfigWriteError instance (it would match + # +Errno::EACCES+ or +Errno::EPERM+ instead, based on the + # wrapped errno). Override with plain class-hierarchy + # matching so +rescue ConfigWriteError+ works as callers + # expect. +rescue SystemCallError+ still matches via normal + # inheritance. + #: (untyped other) -> bool + def ===(other) + other.is_a?(self) + end + + # +SystemCallError.new+ has a factory-style signature defined + # in Sorbet's stdlib RBI (+(String | Integer?) -> + # SystemCallError+) which doesn't match this subclass's + # four-argument constructor. Override +.new+ here so type + # 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) + instance = allocate + instance.__send__(:initialize, config_path, old_content, new_content, cause) + instance + end + end + + #: String + attr_reader :config_path + + #: String + attr_reader :old_content + + #: String + attr_reader :new_content + + # rubocop:disable Lint/MissingSuper + # +SystemCallError#initialize+ has a factory-style signature that + # cannot be invoked from a plain subclass (it expects a matching + # Errno constant on the class). We bypass it deliberately and + # 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) + @config_path = config_path + @old_content = old_content + @new_content = new_content + @wrapped_errno = cause.errno + message = <<~MSG.rstrip + Could not write to #{config_path}: #{cause.message} + + Attempted changes (unchanged keys omitted): + #{diff} + MSG + Exception.instance_method(:initialize).bind(self).call(message) + end + # rubocop:enable Lint/MissingSuper + + # Preserve the original errno from the wrapped Errno so callers + # that inspect +errno+ continue to see the real underlying code. + #: -> Integer? + 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 + + lines = [] + (old_ini.keys | new_ini.keys).each do |section| + old_section = old_ini[section] || {} + new_section = new_ini[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 + end + next if changes.empty? + + prefix = if old_section.empty? + '+ ' + elsif new_section.empty? + '- ' + else + ' ' + end + lines << "#{prefix}#{section}" + lines.concat(changes) + end + lines.join("\n") + end + end + #: (tool_name: String) -> void def initialize(tool_name:) @tool_name = tool_name @@ -129,8 +248,83 @@ def write_config all_configs.each do |section, sub_config| all_configs.delete(section) if sub_config.empty? end - FileUtils.mkdir_p(File.dirname(file)) - File.write(file, to_s) + + # Capture +file+ and +to_s+ up front so they are visible (and + # known to be non-nil) inside the rescue block below. + config_path = file + new_content = to_s + + begin + # Always write atomically (tmpfile + rename) so readers never + # observe a partial write. If +config_path+ is a symlink (e.g. + # a nix/home-manager-managed config), resolve it so the rename + # replaces the real file at the end of the chain and leaves + # the managed symlink itself intact. + target_path = resolved_write_path(config_path) + target_dir = File.dirname(target_path) + FileUtils.mkdir_p(target_dir) + write_config_atomic(target_path, target_dir, new_content) + rescue Errno::EACCES, Errno::EPERM, Errno::EROFS => e + # +EROFS+ (read-only filesystem) is the canonical error raised + # when the underlying filesystem is mounted read-only — common + # 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)) + end + end + + # Write +new_content+ into +config_path+ atomically, via a tmpfile + # in the same directory followed by rename. Readers never observe + # a partial write. + #: (String config_path, String config_dir, String new_content) -> void + def write_config_atomic(config_path, config_dir, new_content) + tmpfile = Tempfile.new([File.basename(config_path), '.tmp'], config_dir) + tmpfile_path = tmpfile.path #: as !nil + begin + tmpfile.write(new_content) + tmpfile.close + # Tempfile defaults to 0o600. Match the permissions a plain + # +File.write+ would have produced: preserve the existing + # mode when the config is being updated, and use the + # umask-adjusted default (matching +open(2)+ for new files) + # otherwise. This avoids silently tightening permissions on + # an existing config and avoids creating new configs with + # the more restrictive Tempfile default. + mode = if File.exist?(config_path) + File.stat(config_path).mode + else + 0o666 & ~File.umask + end + File.chmod(mode, tmpfile_path) + File.rename(tmpfile_path, config_path) + rescue Exception # rubocop:disable Lint/RescueException + tmpfile.close unless tmpfile.closed? + File.unlink(tmpfile_path) if File.exist?(tmpfile_path) + raise + end + end + + #: (String config_path) -> String + def read_config_for_diff(config_path) + File.read(config_path) + rescue SystemCallError + '' + end + + # Resolve +config_path+ through any symlinks so the atomic rename + # in +write_config_atomic+ replaces the real file at the end of + # the chain instead of clobbering the symlink with a regular file. + # For broken symlinks, fall back to +config_path+ itself so we + # still create a config there. + #: (String config_path) -> String + def resolved_write_path(config_path) + return config_path unless File.symlink?(config_path) + + File.realpath(config_path) + rescue Errno::ENOENT + config_path end end end diff --git a/test/cli/kit/config_test.rb b/test/cli/kit/config_test.rb index f913e4a..bf1859f 100644 --- a/test/cli/kit/config_test.rb +++ b/test/cli/kit/config_test.rb @@ -108,6 +108,223 @@ def test_get_section @config.set('srcpath', 'default', 'Shopify') assert_equal({ 'other' => 'test', 'default' => 'Shopify' }, @config.get_section('srcpath')) end + + def test_atomic_write_produces_valid_config + @config.set('section', 'key', 'value') + + assert_equal('value', @config.get('section', 'key')) + assert(File.exist?(@file), 'config file should exist after set') + assert_includes(File.read(@file), 'key = value') + end + + def test_atomic_write_leaves_no_stale_tmpfile + @config.set('section', 'key', 'value') + + siblings = Dir.entries(File.dirname(@file)) - ['.', '..'] + assert_equal(['config'], siblings, "expected only 'config', got #{siblings.inspect}") + end + + def test_atomic_write_applies_umask_for_new_file_permissions + # Tempfile defaults to 0o600. New configs should instead use + # the umask-adjusted default that +File.write+ would produce. + original_umask = File.umask(0o022) + begin + @config.set('section', 'key', 'value') + + mode = File.stat(@file).mode & 0o777 + assert_equal(0o644, mode, "expected 0o644 with umask 0o022, got #{mode.to_s(8)}") + ensure + File.umask(original_umask) + end + end + + def test_atomic_write_preserves_existing_file_permissions + # When the config already exists, its mode should be preserved + # across the rename rather than replaced by the umask default. + config_dir = File.dirname(@file) + FileUtils.mkdir_p(config_dir) + File.write(@file, "[section]\nkey = original\n") + FileUtils.chmod(0o640, @file) + + @config.set('section', 'key', 'updated') + + mode = File.stat(@file).mode & 0o777 + assert_equal(0o640, mode, "expected mode to be preserved at 0o640, got #{mode.to_s(8)}") + end + + def test_write_to_readonly_dir_raises_config_write_error + config_dir = File.dirname(@file) + FileUtils.mkdir_p(config_dir) + File.write(@file, "[section]\nkey = original\n") + FileUtils.chmod(0o555, config_dir) + + error = assert_raises(CLI::Kit::Config::ConfigWriteError) do + @config.set('section', 'key', 'updated') + end + + assert_includes(error.message, @file) + assert_includes(error.message, '- key = original') + assert_includes(error.message, '+ key = updated') + ensure + FileUtils.chmod(0o755, config_dir) if config_dir && File.directory?(config_dir) + end + + def test_config_write_error_includes_new_sections + config_dir = File.dirname(@file) + FileUtils.mkdir_p(config_dir) + File.write(@file, '') + FileUtils.chmod(0o555, config_dir) + + error = assert_raises(CLI::Kit::Config::ConfigWriteError) do + @config.set('hooks', 'path_check_enabled', 'false') + end + + assert_includes(error.message, '+ [hooks]') + assert_includes(error.message, '+ path_check_enabled = false') + ensure + FileUtils.chmod(0o755, config_dir) if config_dir && File.directory?(config_dir) + end + + def test_config_write_error_omits_unchanged_sensitive_values + config_dir = File.dirname(@file) + FileUtils.mkdir_p(config_dir) + File.write(@file, <<~INI) + [buildkite] + api_token = super_secret_token_xyz + + [hooks] + path_check_enabled = true + INI + FileUtils.chmod(0o555, config_dir) + + error = assert_raises(CLI::Kit::Config::ConfigWriteError) do + @config.set('hooks', 'path_check_enabled', 'false') + end + + refute_includes( + error.message, + 'super_secret_token_xyz', + 'unchanged sensitive values must not appear in the error message', + ) + refute_includes( + error.message, + 'api_token', + 'unchanged keys must not appear in the error message', + ) + refute_includes( + error.message, + '[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') + ensure + FileUtils.chmod(0o755, config_dir) if config_dir && File.directory?(config_dir) + end + + def test_config_write_error_is_rescued_as_system_call_error + config_dir = File.dirname(@file) + FileUtils.mkdir_p(config_dir) + File.write(@file, "[section]\nkey = original\n") + FileUtils.chmod(0o555, config_dir) + + # Existing callers may rescue SystemCallError around Config#set. + # ConfigWriteError must still be catchable that way. + caught = begin + @config.set('section', 'key', 'updated') + nil + rescue SystemCallError => e + e + end + + assert_kind_of(CLI::Kit::Config::ConfigWriteError, caught) + assert_kind_of(SystemCallError, caught) + assert_equal(Errno::EACCES::Errno, caught.errno) + ensure + FileUtils.chmod(0o755, config_dir) if config_dir && File.directory?(config_dir) + end + + def test_write_to_readonly_filesystem_raises_config_write_error + config_dir = File.dirname(@file) + FileUtils.mkdir_p(config_dir) + File.write(@file, "[section]\nkey = original\n") + + # Simulate a read-only filesystem (e.g. /nix/store): Tempfile.new + # raises EROFS rather than EACCES/EPERM. The wrapper must still + # produce ConfigWriteError so callers see the diff and the + # wrapped errno. + Tempfile.stubs(:new).raises(Errno::EROFS.new(@file)) + + error = assert_raises(CLI::Kit::Config::ConfigWriteError) do + @config.set('section', 'key', 'updated') + end + + assert_includes(error.message, @file) + assert_includes(error.message, '- key = original') + assert_includes(error.message, '+ key = updated') + assert_equal(Errno::EROFS::Errno, error.errno) + end + + def test_write_preserves_symlink_on_successful_write + Dir.mktmpdir do |dir| + target_dir = File.join(dir, 'target', 'tool') + link_dir = File.join(dir, 'link', 'tool') + FileUtils.mkdir_p(target_dir) + FileUtils.mkdir_p(link_dir) + + target_path = File.join(target_dir, 'config') + link_path = File.join(link_dir, 'config') + File.write(target_path, "[section]\nkey = original\n") + File.symlink(target_path, link_path) + + with_env('XDG_CONFIG_HOME' => File.join(dir, 'link')) do + config = Config.new(tool_name: 'tool') + config.set('section', 'key', 'updated') + end + + assert(File.symlink?(link_path), 'symlink must be preserved after write') + assert_equal(target_path, File.readlink(link_path)) + assert_includes(File.read(target_path), 'key = updated') + end + end + + def test_write_preserves_symlink_when_target_is_readonly + Dir.mktmpdir do |dir| + target_dir = File.join(dir, 'nix-store', 'tool') + link_dir = File.join(dir, 'home', 'tool') + FileUtils.mkdir_p(target_dir) + FileUtils.mkdir_p(link_dir) + + target_path = File.join(target_dir, 'config') + link_path = File.join(link_dir, 'config') + File.write(target_path, "[section]\nkey = original\n") + File.symlink(target_path, link_path) + # Mirror the nix/home-manager scenario: both the store + # directory and the target file are read-only. + FileUtils.chmod(0o444, target_path) + FileUtils.chmod(0o555, target_dir) + + error = with_env('XDG_CONFIG_HOME' => File.join(dir, 'home')) do + config = Config.new(tool_name: 'tool') + assert_raises(CLI::Kit::Config::ConfigWriteError) do + config.set('section', 'key', 'updated') + end + end + + assert( + File.symlink?(link_path), + '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') + ensure + if defined?(target_dir) && target_dir && File.directory?(target_dir) + FileUtils.chmod(0o755, target_dir) + FileUtils.chmod(0o644, target_path) if defined?(target_path) && target_path && File.file?(target_path) + end + end + end end end end