Merge pull request #24241 from Homebrew/cask-privileged-step-errors

Read protected install step symlinks with sudo

Library/Homebrew/install_steps.rb

@@ -1050,7 +1050,7 @@ module Homebrew
           paths = step_paths(step, "paths").flat_map { |path| expand_path_glob(path) }
           if step.key?("symlink_target_contains")
             paths.select! do |path|
-              path.symlink? && path.readlink.to_s.include?(step_string(step, "symlink_target_contains"))
+              path.symlink? && read_symlink(path, step).to_s.include?(step_string(step, "symlink_target_contains"))
             end
           end
           if step.key?("content_contains")
@@ -1307,6 +1307,15 @@ module Homebrew
         end
       end
 
+      sig { params(path: Pathname, step: Step).returns(Pathname) }
+      def read_symlink(path, step)
+        path.readlink
+      rescue Errno::EACCES
+        raise if step["sudo"] != true && step["sudo"] != "if_needed"
+
+        Pathname(@command.run!("/usr/bin/readlink", args: ["-n", path], sudo: true).stdout)
+      end
+
       sig { params(step: Step).void }
       def run_set_permissions(step)
         paths = existing_step_paths(step)
@@ -1352,7 +1361,7 @@ module Homebrew
 
         target = resolve_path(step_path(step, "target"))
         return unless target.symlink?
-        return if target.readlink != Pathname(link_source(step_path(step, "source")))
+        return if read_symlink(target, step) != Pathname(link_source(step_path(step, "source")))
 
         if step["sudo"] == true || (step["sudo"] == "if_needed" && !target.dirname.writable?)
           require "cask/utils"

Library/Homebrew/test/install_steps_spec.rb

@@ -1691,6 +1691,101 @@ RSpec.describe Homebrew::InstallSteps do
     end
   end
 
+  context "when reading a symlink requires elevation" do
+    let(:target) { root/"stage/linked-target" }
+    let(:command) { class_double(SystemCommand) }
+    let(:sudo) { :if_needed }
+    let(:steps) do
+      elevation = sudo
+      Homebrew::InstallSteps::DSL.build(default_target_base: :staged_path) do
+        symlink "target", "linked-target", source_base: :relative,
+                                          remove_on_uninstall: true, sudo: elevation
+      end
+    end
+
+    before do
+      target.dirname.mkpath
+      File.symlink "target", target
+      allow(File).to receive(:readlink).and_call_original
+      allow(File).to receive(:readlink).with(target.to_s).and_raise(Errno::EACCES)
+      allow(command).to receive(:run!).with("/usr/bin/readlink", args: anything,
+                                                                 sudo: true) do |executable, args:, **|
+        SystemCommand.run!(executable, args:)
+      end
+    end
+
+    it "removes a matching symlink after reading its target with sudo" do
+      Homebrew::InstallSteps::Runner.new(context:, command:).run(steps, phase: :uninstall)
+
+      expect(target).not_to be_a_symlink
+    end
+
+    it "preserves a symlink whose target does not match" do
+      target.unlink
+      File.symlink "different-target", target
+
+      Homebrew::InstallSteps::Runner.new(context:, command:).run(steps, phase: :uninstall)
+
+      expect(target).to be_a_symlink
+    end
+
+    it "preserves a symlink whose target differs by a trailing newline" do
+      target.unlink
+      File.symlink "target\n", target
+
+      Homebrew::InstallSteps::Runner.new(context:, command:).run(steps, phase: :uninstall)
+
+      expect(target).to be_a_symlink
+    end
+
+    it "preserves the symlink and propagates a failed elevated inspection" do
+      allow(command).to receive(:run!).with("/usr/bin/readlink", args: anything, sudo: true)
+                                      .and_raise(ErrorDuringExecution.new([], status: 1))
+
+      expect do
+        Homebrew::InstallSteps::Runner.new(context:, command:).run(steps, phase: :uninstall)
+      end.to raise_error(ErrorDuringExecution) { expect(target).to be_a_symlink }
+    end
+
+    it "checks the target before filtered removal" do
+      steps = Homebrew::InstallSteps::DSL.build(default_base: :staged_path) do
+        remove "linked-target", sudo: :if_needed, symlink_target_contains: "target"
+      end
+      Homebrew::InstallSteps::Runner.new(context:, command:).run(steps)
+
+      expect(target).not_to be_a_symlink
+    end
+
+    it "preserves a symlink whose target does not match the removal filter" do
+      steps = Homebrew::InstallSteps::DSL.build(default_base: :staged_path) do
+        remove "linked-target", sudo: :if_needed, symlink_target_contains: "other-target"
+      end
+      Homebrew::InstallSteps::Runner.new(context:, command:).run(steps)
+
+      expect(target).to be_a_symlink
+    end
+
+    context "when sudo is required by the step" do
+      let(:sudo) { true }
+
+      it "removes a matching symlink after reading its target with sudo" do
+        Homebrew::InstallSteps::Runner.new(context:, command:).run(steps, phase: :uninstall)
+
+        expect(target).not_to be_a_symlink
+      end
+    end
+
+    context "when sudo is not permitted by the step" do
+      let(:sudo) { false }
+
+      it "preserves the permission error" do
+        expect do
+          Homebrew::InstallSteps::Runner.new(context:, command:).run(steps, phase: :uninstall)
+        end.to raise_error(Errno::EACCES)
+      end
+    end
+  end
+
   specify "does not expose the surrounding formula or cask DSL" do
     expect do
       Homebrew::InstallSteps::DSL.build(default_base: :var) do

Library/Homebrew/test/utils/fork_spec.rb

@@ -29,6 +29,30 @@ RSpec.describe Utils do
   end
 
   describe "#safe_fork" do
+    it "reports only the parent error when the child reports a failure after disconnection" do
+      handler = proc do |_message|
+        raise Errno::EACCES, "/usr/local/bin/kubectl"
+      end
+
+      expect do
+        Timeout.timeout(10) do
+          described_class.safe_fork(child_message_handler: handler) do
+            exec RUBY_PATH.to_s, "-I", HOMEBREW_LIBRARY_PATH.to_s, "-rglobal", "-rutils/fork", "-e", <<~RUBY
+              channel = Utils.forked_child_channel
+              channel.puts "privileged step"
+              channel.flush
+              exit! 0 if channel.gets
+
+              Utils.report_forked_child_error(channel, RuntimeError.new("parent step failed"))
+              exit! 1
+            RUBY
+          end
+        end
+      rescue Errno::EACCES => e
+        warn e.message
+      end.to output("Permission denied - /usr/local/bin/kubectl\n").to_stderr_from_any_process
+    end
+
     it "preserves a parent failure while the child error pipe is open" do
       IO.pipe do |ready_read, ready_write|
         expect do

Library/Homebrew/utils/fork.rb

@@ -99,6 +99,9 @@ module Utils
 
     error_pipe&.puts coder.dump(error)
     error_pipe&.close
+  rescue Errno::EPIPE
+    # The parent has already failed and closed its end of the pipe.
+    nil
   end
 
   sig { params(child_error: T::Hash[String, T.untyped]).returns(Exception) }