From 59ddac339dee3de2877cb01dcd04248dfec7d25c Mon Sep 17 00:00:00 2001
From: Selvi Arora <selvi.arora@shopify.com>
Date: Tue, 15 Sep 2026 15:03:56 -0400
Subject: [PATCH] Restrict config files to owner access

Assisted-By: devx/736bac94-4b2a-4b19-b5cc-1826ad565f21
---
 lib/cli/kit/config.rb       | 15 ++++++--------
 test/cli/kit/config_test.rb | 39 ++++++++++++++++++++++---------------
 2 files changed, 29 insertions(+), 25 deletions(-)

diff --git a/lib/cli/kit/config.rb b/lib/cli/kit/config.rb
index e817adf..4573394 100644
--- a/lib/cli/kit/config.rb
+++ b/lib/cli/kit/config.rb
@@ -285,17 +285,14 @@ def write_config_atomic(config_path, config_dir, new_content)
         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.
+          # Configs may contain credentials. Keep them private to the
+          # owner, including when replacing a previously shared file.
+          # Preserve stricter owner permissions on existing files and
+          # respect the umask when creating new ones.
           mode = if File.exist?(config_path)
-            File.stat(config_path).mode
+            File.stat(config_path).mode & 0o600
           else
-            0o666 & ~File.umask
+            0o600 & ~File.umask
           end
           File.chmod(mode, tmpfile_path)
           File.rename(tmpfile_path, config_path)
diff --git a/test/cli/kit/config_test.rb b/test/cli/kit/config_test.rb
index bf1859f..b724054 100644
--- a/test/cli/kit/config_test.rb
+++ b/test/cli/kit/config_test.rb
@@ -124,32 +124,38 @@ def test_atomic_write_leaves_no_stale_tmpfile
         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)
+      def test_atomic_write_limits_new_file_permissions_to_owner
+        original_umask = File.umask
         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)}")
+          [[0o000, 0o600], [0o002, 0o600], [0o022, 0o600], [0o077, 0o600], [0o277, 0o400]].each do |umask, expected_mode|
+            config = Config.new(tool_name: "tool-#{umask}")
+            File.umask(original_umask)
+            FileUtils.mkdir_p(File.dirname(config.file))
+            File.umask(umask)
+            config.set('service', 'api_token', 'secret_token')
+
+            mode = File.stat(config.file).mode & 0o777
+            assert_equal(expected_mode, mode, "unexpected permissions with umask #{umask.to_s(8)}")
+            assert_includes(File.read(config.file), 'api_token = secret_token')
+          end
         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.
+      def test_atomic_write_removes_group_and_other_permissions
         config_dir = File.dirname(@file)
         FileUtils.mkdir_p(config_dir)
-        File.write(@file, "[section]\nkey = original\n")
-        FileUtils.chmod(0o640, @file)
+        [[0o666, 0o600], [0o644, 0o600], [0o640, 0o600], [0o600, 0o600], [0o400, 0o400]].each do |original_mode, expected_mode|
+          File.write(@file, "[service]\napi_token = old_secret_token\n")
+          FileUtils.chmod(original_mode, @file)
 
-        @config.set('section', 'key', 'updated')
+          Config.new(tool_name: 'tool').set('service', 'api_token', 'new_secret_token')
 
-        mode = File.stat(@file).mode & 0o777
-        assert_equal(0o640, mode, "expected mode to be preserved at 0o640, got #{mode.to_s(8)}")
+          mode = File.stat(@file).mode & 0o777
+          assert_equal(expected_mode, mode)
+          assert_includes(File.read(@file), 'api_token = new_secret_token')
+        end
       end
 
       def test_write_to_readonly_dir_raises_config_write_error
@@ -285,6 +291,7 @@ def test_write_preserves_symlink_on_successful_write
           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')
+          assert_equal(0o600, File.stat(target_path).mode & 0o777)
         end
       end