Allow sudo in sandboxed Cask install steps - #23476
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request restores support for structured Cask install steps that explicitly require elevation (sudo: true) when those steps are executed inside Homebrew’s sandboxed subprocesses. It does so by adding an explicit sandbox rule permitting /usr/bin/sudo to exec with the no-sandbox modifier when a step requests sudo, while keeping all other structured step execution sandboxed.
Changes:
- Add
Sandbox#allow_process_execto emitprocess-execrules with an optionalno-sandboxmodifier. - Allow
/usr/bin/sudoto run outside the sandbox for sandboxed structured install steps when any step hassudo: true. - Add unit tests covering both the sandbox rule generation and the Cask install-steps behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
Library/Homebrew/sandbox.rb |
Adds a helper to allow process-exec rules with an optional no-sandbox modifier. |
Library/Homebrew/cask/artifact/install_steps.rb |
Conditionally adds a sandbox exception for /usr/bin/sudo when structured steps request sudo: true. |
Library/Homebrew/test/sandbox_shared_spec.rb |
Tests that allow_process_exec produces a process-exec rule with no-sandbox and the expected path filter. |
Library/Homebrew/test/cask/artifact/install_steps_spec.rb |
Tests that sandboxed structured steps requesting sudo: true add the /usr/bin/sudo (no-sandbox) exec allowance. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
|
It might be better to add a sudo check and run it outside the sandbox. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Library/Homebrew/test/cask/artifact/install_steps_spec.rb:150
- The new tests cover only cases that grant the
no-sandboxexception. Because this exception removes the main security boundary, also assert that an ordinary structured step does not callallow_process_exec; otherwise the PR's stated negative guarantee can regress unnoticed.
Cask::Installer.new(cask, command: NeverSudoSystemCommand).install_artifacts
end
end
end
brewcommands to reproduce the bug?brew lgtm(style, typechecking and tests) locally?Codex (GPT-5.6) was used to investigate the regression, help implement the change, and draft the tests and PR description. I reviewed the diff, manually verified
While investigating Homebrew/homebrew-cask#279650, I installed the
parallelsCask to reproduce its reported uninstall failure. The installation itself failed with the sameOperation not permitted - /usr/bin/sudoerror on multiple Macs.Tracing the failure showed that structured Cask install steps have been executed inside a sandbox since #23461. Steps that explicitly specify
sudo: truecannot launch/usr/bin/sudofrom that sandbox on macOS and fail withEPERM.For example, the
parallelsCask contains:I think this regression occurred because #23461 moved structured Cask step blocks into a sandboxed subprocess without accounting for
sudo: true. The tests and automated review appear to have covered only unprivileged commands, so the interaction between the macOS sandbox and/usr/bin/sudomay not have been exercised before merge.Reproduction
Change
This change adds a sandbox rule that allows
/usr/bin/sudoto execute without inheriting the sandbox when a structured step explicitly setssudo: true.All other structured step processing remains sandboxed. The exception is not added to step blocks that do not explicitly request
sudo.This preserves the structured install-step authoring enforced by #23475 while restoring support for steps that explicitly require elevated privileges.
Unit tests cover:
process-execrule for/usr/bin/sudowith theno-sandboxmodifier.sudo: true.The unit tests verify that the correct sandbox rule is added without invoking real
sudo. I separately verified the actual privileged execution by manually installing theparallelsCask.Verification
After applying this change, I manually verified that the original installation failure was resolved:
I also verified that another Cask could be removed successfully:
$ brew remove --cask lg-onscreen-controlbrew remove --cask parallelsstill fails due to a separate issue:must_succeed: falseis lost while serialising Cask artifact data, causing thepkillcommand in its uninstall stanza to be treated as mandatory.That issue is unrelated to the sandboxed
sudoregression addressed by this PR and will be submitted separately.