Skip to content

cask: do not follow a symlinked rmdir path - #23761

Merged
MikeMcQuaid merged 1 commit into
mainfrom
cask-rmdir-symlink-guard
Sep 3, 2026
Merged

cask: do not follow a symlinked rmdir path#23761
MikeMcQuaid merged 1 commit into
mainfrom
cask-rmdir-symlink-guard

Conversation

@dduugg

@dduugg dduugg commented Sep 3, 2026

Copy link
Copy Markdown
Member

uninstall rmdir: and zap rmdir: paths come from the cask, so one can name a location that a local user is able to replace with a symlink. recursive_rmdir recursed through such a link and removed empty directories inside its target, and gain_permissions_rmdir then ran its chflags, chmod and sudo chown recovery round against the target too, because it passed no -h. The uninstall aborted afterwards with an uncaught Errno::ENOTDIR, so that path never removed anything successfully anyway.

Skipping symlinks in recursive_rmdir and always passing -h in gain_permissions_rmdir means a symlinked rmdir path can no longer redirect the privileged permission changes onto whatever it points at, and the abort becomes a clean "not empty" skip. -h needs no condition because it is a no-op on a real directory, so nothing is decided from a path that could change before the recovery runs. Real directories are unaffected.

This narrows the traversal rather than closing it: children and the per-directory rmdir can still be redirected if the path is swapped between the check and the use. Doing better needs descriptor-relative traversal with O_NOFOLLOW, which Ruby cannot express without FFI and which would replace recursive_rmdir rather than guard it. Before this change the link was followed on every run, so the window goes from certain to raced.


  • Have you followed our Contributing guidelines?
  • Have you checked for other open Pull Requests for the same change?
  • Have you explained what your changes do? Performance claims (e.g. "this is faster") must include brew benchmark results.
  • Have you explained why you'd like these changes included, not just what they do?
  • For bug fixes, have you given step-by-step brew commands to reproduce the bug? Reproducing needs a cask naming a path a second account can replace, covered by the added specs instead.
  • Have you written new tests (excluding integration tests)? Here's an example.
  • Have you successfully run brew lgtm (style, typechecking and tests) locally?

  • I did not use AI/LLM to create this PR, or I disclosed the tool/model below and reviewed its output; I did not attribute commits to AI and will answer maintainer questions and review comments myself without AI/LLM.

Claude Code with Opus 5, with local review and testing.


Copilot AI balanced review requested due to automatic review settings September 3, 2026 16:30

@MikeMcQuaid MikeMcQuaid left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, one nit and then ready to self-merge!

Comment thread Library/Homebrew/cask/utils.rb Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

馃煛 Changes recommended

Pathname checks remain vulnerable to races that can redirect traversal and privileged permission changes.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Hardens cask rmdir handling against symlink redirection.

Changes:

  • Skips symlinks during recursive directory removal.
  • Applies no-follow permission flags to symlink paths.
  • Adds utility and zap regression tests.
File summaries
File Description
Library/Homebrew/cask/utils.rb Adds symlink-aware permission flags.
Library/Homebrew/cask/artifact/abstract_uninstall.rb Skips symlinked directories.
Library/Homebrew/test/cask/utils_spec.rb Tests permission recovery safety.
Library/Homebrew/test/cask/artifact/zap_spec.rb Tests symlinked zap paths.
Review details
  • Files reviewed: 4/4 changed files
  • Comments generated: 2
  • Review effort level: Balanced

馃挕 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

Comment thread Library/Homebrew/cask/artifact/abstract_uninstall.rb
Comment thread Library/Homebrew/cask/utils.rb Outdated
`uninstall rmdir:` and `zap rmdir:` paths are resolved from the cask, so
one can name a location a local user is able to replace with a symlink.
`recursive_rmdir` then recursed through the link and removed empty
directories inside its target, and the `gain_permissions_rmdir` recovery
round ran `chflags`, `chmod` and `sudo chown` against the target because
it passed no `-h`.

- skip symlinks in `recursive_rmdir`, which also replaces an uncaught
  `Errno::ENOTDIR` with treating the path as not empty
- always pass `-h` in `gain_permissions_rmdir`, which is a no-op on a
  real directory and so needs no decision from a path that could change
  before the recovery runs
@dduugg
dduugg force-pushed the cask-rmdir-symlink-guard branch from c97bc3a to 2abb5d2 Compare September 3, 2026 16:51
@MikeMcQuaid
MikeMcQuaid added this pull request to the merge queue Sep 3, 2026
Merged via the queue into main with commit a6c9749 Sep 3, 2026
58 checks passed
@MikeMcQuaid
MikeMcQuaid deleted the cask-rmdir-symlink-guard branch September 3, 2026 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants