Merge pull request #14735 from reitermarkus/revert-14729-revert-14711-extract-permissions

Revert "Revert "Fix permissions before moving extracted files.""

Library/Homebrew/test/.rubocop.yml

@@ -4,3 +4,13 @@ inherit_from:
 RSpec:
   Include:
     - ./*
+
+RSpec/ContextWording:
+  Prefixes:
+    - when
+    - with
+    - without
+    - if
+    - unless
+    - for
+    - which

Library/Homebrew/test/unpack_strategy_spec.rb

@@ -28,11 +28,22 @@ describe UnpackStrategy do
 
     context "when extracting a directory with nested directories" do
       let(:directories) { "A/B/C" }
+      let(:executable) { "#{directories}/executable" }
+      let(:writable) { true }
       let(:path) {
         (mktmpdir/"file.tar").tap do |path|
-          mktmpdir do |dir|
+          Dir.mktmpdir do |dir|
+            dir = Pathname(dir)
             (dir/directories).mkpath
-            system "tar", "--create", "--file", path, "--directory", dir, "A/"
+            FileUtils.touch dir/executable
+            FileUtils.chmod 0555, dir/executable
+
+            FileUtils.chmod "-w", dir/directories unless writable
+            begin
+              system "tar", "--create", "--file", path, "--directory", dir, "A/"
+            ensure
+              FileUtils.chmod "+w", dir/directories unless writable
+            end
           end
         end
       }
@@ -41,6 +52,23 @@ describe UnpackStrategy do
         strategy.extract_nestedly(to: unpack_dir)
         expect(Pathname.glob(unpack_dir/"**/*")).to include unpack_dir/directories
       end
+
+      context "which are not writable" do
+        let(:writable) { false }
+
+        it "makes them writable but not world-writable" do
+          strategy.extract_nestedly(to: unpack_dir)
+
+          expect(unpack_dir/directories).to be_writable
+          expect(unpack_dir/directories).not_to be_world_writable
+        end
+
+        it "does not make other files writable" do
+          strategy.extract_nestedly(to: unpack_dir)
+
+          expect(unpack_dir/executable).not_to be_writable
+        end
+      end
     end
 
     context "when extracting a nested archive" do

Library/Homebrew/unpack_strategy.rb

@@ -3,6 +3,27 @@
 
 require "system_command"
 
+# Helper module for iterating over directory trees.
+#
+# @api private
+module PathnameEachDirectory
+  refine Pathname do
+    extend T::Sig
+
+    sig {
+      type_parameters(:T)
+        .params(
+          _block: T.proc.params(path: Pathname).returns(T.type_parameter(:T)),
+        ).returns(T.type_parameter(:T))
+    }
+    def each_directory(&_block)
+      find do |path|
+        yield path if path.directory?
+      end
+    end
+  end
+end
+
 # Module containing all available strategies for unpacking archives.
 #
 # @api private
@@ -12,6 +33,8 @@ module UnpackStrategy
 
   include SystemCommand::Mixin
 
+  using PathnameEachDirectory
+
   # Helper module for identifying the file type.
   module Magic
     # Length of the longest regex (currently Tar).
@@ -164,17 +187,21 @@ module UnpackStrategy
       children = tmp_unpack_dir.children
 
       if children.count == 1 && !children.first.directory?
-        FileUtils.chmod "+rw", children.first, verbose: verbose
-
         s = UnpackStrategy.detect(children.first, prioritize_extension: prioritize_extension)
 
         s.extract_nestedly(to: to, verbose: verbose, prioritize_extension: prioritize_extension)
+
         next
       end
 
-      Directory.new(tmp_unpack_dir).extract(to: to, verbose: verbose)
+      # Ensure all extracted directories are writable.
+      tmp_unpack_dir.each_directory do |path|
+        next if path.writable?
 
-      FileUtils.chmod_R "+w", tmp_unpack_dir, force: true, verbose: verbose
+        FileUtils.chmod "u+w", path, verbose: verbose
+      end
+
+      Directory.new(tmp_unpack_dir).extract(to: to, verbose: verbose)
     end
   end