镜像站点 · 本页由第三方 GitHub 只读镜像提供,非 GitHub 官方站点,不接受任何登录或凭据输入。前往 github.com
Skip to content

FMA uninstall: clean up trash paths when no one is logged in - #56333

Closed
allenhouchins wants to merge 4 commits into
mainfrom
allenhouchins/fma-trash-no-gui-user
Closed

allenhouchins wants to merge 4 commits into
mainfrom
allenhouchins/fma-trash-no-gui-user

Conversation

@allenhouchins

@allenhouchins allenhouchins commented Oct 22, 2026 •

Copy link
Copy Markdown
Member

Related issue: Came up in review of #56085 (Logitech G HUB FMA), where CodeRabbit flagged that /Users/Shared/LGHUB isn't removed when no one is logged in. The cause is in the shared generator, so it's fixed here for every macOS FMA.

Problem

Fleet runs FMA uninstall scripts as root at any time, including at the login window. The generated zap cleanup was:

LOGGED_IN_USER=$(scutil <<< "show State:/Users/ConsoleUser" | awk '/Name :/ { print $3 }')
trash $LOGGED_IN_USER '/Users/Shared/LGHUB'
  • At the login window, LOGGED_IN_USER is loginwindow, so trash() moved every path into /Users/loginwindow/.Trash. That folder doesn't exist, so the mv failed and app files outside home folders (e.g. /Users/Shared/<app>, /Library/Application Support/<app>, /Library/LaunchDaemons/<app>.plist) were left behind.
  • When LOGGED_IN_USER was empty, the unquoted variable disappeared and the arguments shifted. The path became logged_in_user, target_file was empty, and nothing was removed.

968 macOS outputs use trash, and 106 of their trash calls are system paths rather than ~ paths.

Fix

  • Generated calls quote the user: trash "$LOGGED_IN_USER" '<path>'.
  • In trash(), when the console user is empty, root or loginwindow:
    • ~ paths are skipped with Not logged into a non-root GUI; skipping <path>., which matches the wording of the quit and relaunch helpers.
    • All other paths are deleted with rm -rf. Glob paths still expand through compgen -G and each match is removed.
    • The only paths deleted are the app-specific entries in the cask's zap stanza, such as /Users/Shared/GOG.com or /Library/Application Support/TeamViewer. When someone is logged in, these same paths are already moved to their Trash. I checked all 106 absolute paths in the outputs, and none is a shared parent folder like /Users/Shared or /Library. The shallowest are /Library/Tailscale, /etc/anydesk and /Applications/Omnissa Horizon Client.app.
  • With a real console user, nothing changes: paths are still moved to that user's Trash.
  • quit_application and relaunch_application are byte-identical to before.

Exit status: cleanup stays best-effort

A failed removal prints Failed to remove <path>. (or Failed to move <path> to the Trash.) and does not change the script's exit status. Reasons:

  • trash comes from the cask's zap stanza and runs after the app bundle and pkg files are already gone. Those earlier steps keep their own exit handling. Failing the uninstall because of leftover data would report a failed uninstall for an app that's no longer installed, and retrying wouldn't help.
  • Before this change, the exit status was effectively random. trash() returned the last mv's status, so a failure only counted when the failing path happened to be the script's last statement. Now trash() always returns 0, and the logged-in path gets the same || echo treatment, so the result no longer depends on path order.

Outputs

  • Non-frozen outputs: updated by applying the generator's change to each existing uninstall script, then recomputing sha256(script)[:8] and sorting refs. I didn't commit a raw go run ./cmd/maintained-apps because it also pulls whatever upstream version bumps landed since the last bot run. Instead I ran the generator (macOS ingester) in a scratch worktree and compared the results:

    • 950 outputs are byte-identical.
    • 12 differ only by an upstream version bump (Arc, Brave, Claude, and others). Their uninstall scripts are identical.
    • 3 same-version casks have new upstream zap entries (Blender, Dayflow, Notesnook).
    • The 13 frozen outputs differ because the generator never rewrites them.

    So outside of upstream drift, the generator produces exactly these uninstall scripts. The next bot run picks up the drift.

  • Frozen outputs (13): hand-edited the same way, since ingest never rewrites them. Evernote's frozen script carried an older trash() without glob support; it now has the current helper.

  • Custom uninstall scripts that embed their own trash() (Camtasia, CleanMyMac, Cycling '74 Max, ForkLift, GPG Suite, Homebrew, KiCad, Microsoft Word, Teleport) now carry the generator's copy verbatim, with the call sites quoted. Five of these had the pre-glob version, so glob zap paths that never matched before now do. Examples: GPG Suite's ~/Library/Preferences/org.gpgtools.* and Word's ~/Library/Application Support/CrashReporter/Microsoft Word_*.plist.

The only change in every output is the uninstall script and its ref.

Checklist for submitter

  • Changes file added for user-visible changes in changes/, orbit/changes/ or ee/fleetd-chrome/changes.
    See Changes files for more information.

  • Input data is properly validated, SELECT * is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters.

    • Paths are still passed through shellSingleQuote; the only new interpolation is the quoted "$LOGGED_IN_USER".

Testing

  • Added/updated automated tests
    • TestUninstallScriptTrashWithoutGUIUser runs a generated uninstall script in bash with scutil stubbed. It covers loginwindow, an empty user, root and a normal user, each against a ~ path, a ~ glob, an absolute directory, and an absolute glob with spaces. It also checks that a failing rm as the script's last command still exits 0. The test fails on main and passes here, under both /bin/bash 3.2 and Homebrew bash 5.
    • TestCustomUninstallScriptsUseCurrentTrash fails if a custom uninstall script's trash() drifts from the generator's copy or calls it with an unquoted user.
    • go test ./ee/maintained-apps/... ./cmd/maintained-apps/... passes.
  • QA'd all new/changed functionality manually

AI

AI: Claude Code (claude-opus-5-5)

Quote $LOGGED_IN_USER in generated trash calls. When the console user is
empty, root or loginwindow, trash() skips ~ paths and deletes system paths
with rm -rf instead of moving them into a Trash that doesn't exist. Removal
failures are reported but never fail the uninstall.

Sync the trash() copies in custom uninstall scripts and update all macOS
outputs, including frozen ones.
@allenhouchins

Copy link
Copy Markdown
Member Author

Combined into #56457 with the other macOS FMA uninstall changes (#56016, #56314, #56333, #56336, #56454). Closing in favor of that PR.

allenhouchins added a commit that referenced this pull request Oct 23, 2026
…one is logged in (#56457)

**Related issue:** Combines #56016, #56314, #56333, #56336, #56337,
#56453 and #56454 into one PR. Those PRs are closed in favor of this
one. Follow-up to #55157 (#54170) and #54210. #56333 came up in review
of #56085 (Logitech G HUB).

## Summary

All seven change how macOS FMA scripts are generated or tested. Fleet
runs these scripts as root at any time, including at the login window.

### Quit apps by bundle ID when the cask lists names (#56016)

A few casks list app names in `uninstall quit:` instead of bundle IDs.
`tell application id "DeskTime"` fails with -1728, so `quit_application`
treated the app as not running and the script removed the bundle while
the app was open. A dotless `quit:` value now becomes the app's
`unique_identifier`, and repeated IDs in a stanza are quit once. A
dotless value with `*` (such as `*WaveLink*`) is still a pattern.
Affected: DeskTime, DisplayLink, NotesOllama and Fathom.

### Stop the uninstall when the app won't quit (#56314)

The uninstall `quit_application` helper had the bug that #55157 fixed in
the install helper. It confirmed the quit with `pgrep -f "$bundle_id"`,
which never matches, and only logged "did not quit" before `sudo rm -rf`
removed the bundle.
- `quit_application` now polls `osascript … is running` after a 1s
sleep, and returns 1 at the 10s timeout. `quit_matching_applications`
returns 1 if any match didn't quit.
- Generated scripts add `|| exit 1` only when the quit ID names the app
being uninstalled: its `unique_identifier`, case-insensitively, or a
wildcard that matches it. Helpers, shared updaters such as Microsoft
AutoUpdate, and other vendors' apps keep Homebrew's warn-and-continue. A
helper that launchd relaunches would otherwise block the uninstall for
good.
- With #56016, a name in `quit:` becomes the app's own ID first, so
DeskTime, DisplayLink, NotesOllama and Fathom stop too.
- The six custom uninstall scripts with stale copies of the helper now
carry the current one: Box Tools, CleanMyMac, ForkLift, GPG Suite,
Microsoft Word and P4V.

**Behavior change:** if the app won't quit, the uninstall fails with
"Application '<id>' did not quit." It no longer removes the app while
the app is running.

### Quit the app when the cask doesn't (#56337)

Generated uninstall scripts only quit the IDs in the cask's `uninstall
quit:`. Most casks list none, and Excel, Outlook, PowerPoint and Teams
quit only Microsoft AutoUpdate. So the app could be removed while it was
still running.
- The generator now writes `quit_application '<unique_identifier>' ||
exit 1` once, before anything removes the bundle. With an uninstall
stanza, it goes at the stanza's quit step: after `launchctl` and the
cask's own quits, and before `signal`, `early_script`, `script`,
`pkgutil` and `delete`. Without one, it goes right before the app's `rm
-rf`. A zap quit doesn't count, because zap runs after the app is
removed.
- It's left out when an uninstall stanza already quits or signals the
app. 17 casks signal apps that don't quit cleanly, such as DBeaver,
Parallels, Webex and Zoom for IT Admins. A quit that stops the script
mustn't run ahead of the signal. Perimeter 81 splits its stanza across
three artifacts, so every stanza is checked first.
- A cask quit that lists the app's name counts as quitting the app,
through #56016's mapping. So DeskTime, DisplayLink, Fathom and
NotesOllama aren't quit twice.
- Vendor `script` and `early_script` directives aren't exempt. A
directive doesn't say whether it stops the app, and the install script
already quits the same ID with `|| exit 1` on every update.
- Some apps' `unique_identifier` differs from the cask's quit ID, such
as KeePassXC, CleanShot X, Tower, JASP and Sequel Ace. They now also
quit the `unique_identifier`, which is the ID Fleet sees installed. If
LaunchServices doesn't know that ID, the helper returns 0.

The new-fma skill's guidance for custom uninstall scripts moved to
#56459.

### Trash cleanup with no GUI user (#56333)

At the login window, `LOGGED_IN_USER` is `loginwindow`, so `trash()`
tried to move every path into `/Users/loginwindow/.Trash` and failed.
When the user was empty, the unquoted variable disappeared and the
arguments shifted. Either way, system paths like `/Users/Shared/LGHUB`
were left behind.
- Generated calls quote the user: `trash "$LOGGED_IN_USER" '<path>'`.
- With no real console user (empty, `root` or `loginwindow`), `~` paths
are skipped with `Not logged into a non-root GUI; skipping <path>.`
Other paths are deleted with `rm -rf`, and globs still expand through
`compgen -G`. These are only the cask's app-specific `zap` entries. None
of the absolute paths in the outputs is a shared parent folder.
- Cleanup stays best-effort. A failed removal is logged, and `trash()`
always returns 0. Before, the exit status depended on which path
happened to be last.
- Nine custom uninstall scripts carry the current `trash()` verbatim,
with quoted call sites: Camtasia, CleanMyMac, Cycling '74 Max, ForkLift,
GPG Suite, Homebrew, KiCad, Microsoft Word and Teleport.

### Script directives with no GUI user (#56336)

Cask `script` and `early_script` directives were generated as `(cd
/Users/$LOGGED_IN_USER && …)`. At the login window the `cd` failed, so
vendor uninstallers never ran (Anka, Cloudflare WARP, Chrome Remote
Desktop Host, Microsoft Defender, Santa, Mullvad VPN and 11 more).
- Executable directives run as root, so they now start in the user's
home when there is one and in `/` otherwise: `(cd
"/Users/$LOGGED_IN_USER" 2>/dev/null || cd /; sudo '<exe>' …)`.
`must_succeed: false` still appends `|| true`.
- String directives run as the user, so they're skipped with the same
"Not logged into a non-root GUI" message when there's no real user. Box
Drive is the only output with one.
- Box Drive's `pre_uninstall_scripts` run their per-user steps only when
someone is logged in. With an empty user, the old sudoers line wasn't
valid syntax. GPG Suite's custom uninstaller call uses the same `cd … ||
cd /` form.

### Quit 1Password with SIGTERM in the uninstall (#56453)

With "Keep 1Password in the menu bar" on (the default), 1Password's
Electron `before-quit` handler cancels a quit event and drops to the
menu bar. Its `SIGTERM` handler quits it fully, the same as Quit in its
menu bar menu. Without a custom script, the bot would regenerate
1Password's uninstall with `quit_application 'com.1password.1password'
|| exit 1`, and every uninstall with 1Password running would fail with
"did not quit."
- `1password_uninstall.sh` is this branch's generated uninstall with one
change: `quit_application` is replaced by `quit_1password`. It sends
`pkill -TERM -x 1Password`, polls `is running` for up to 10s, and
returns 1 if 1Password is still running. The call is `quit_1password ||
exit 1`, right before `rm -rf`.
- `quit_1password` is the install script's `quit_and_track_1password`
(#56452) without the relaunch tracking.
- The script was built from this branch's generator, so it already has
the #56333 `trash()` and quoted call sites.

### Check every `quit_*` call in custom install scripts (#56454)

`TestCustomInstallScriptsStopWhenAppWontQuit` only matched
`quit_application` and `quit_and_track_application`. It now matches any
`quit_*` call, so a script with its own quit function, like
`quit_and_track_1password` (#56452), can't drop `|| exit 1` unnoticed.
`qgis-ltr-install.sh` is allowlisted: its `quit_app_at_path` sends
SIGTERM, then SIGKILL after 10 seconds.

## Merge order

- #56459 (new-fma skill guidance, split out of #56337) should merge
after this PR.
- #55865 (`rmdir` with `~`) also changes `scripts.go` and many of the
same outputs. Whichever merges second needs a rebuild of the overlapping
outputs.

## Outputs

975 of 980 macOS outputs change. In each one, only the uninstall script
and its ref change. Version, URL, sha256, install script and queries are
byte-identical to `main`.

- **Generated outputs:** they have the #56333 and #56336 changes,
applied to the uninstall script that's on `main` today. The 116 outputs
the bot bumped since #56333 was cut were rebuilt from `main`'s file the
same way. LM Studio and Logitech G HUB were added after #56333 was cut,
so they're new in this PR. The edit reproduces all 955 generated outputs
in #56333 and #56336 byte for byte.
- **#56314's quit change isn't applied to generated outputs.** That
follows #56314 and the precedent of #55157, #43842 and #42951: the FMA
bot picks up the new uninstall refs for the apps that quit something.
For 35 of those apps, the bot also adds #56337's quit, for example
Excel, Outlook, PowerPoint and Teams.
- **#56337's quit is applied to 589 outputs.** None of them quits
anything yet, and each already matched this branch's generator. So each
gains only `quit_application '<unique_identifier>' || exit 1` and the
helper. Eight are frozen apps, which the bot never rewrites: BrickLink
Studio, Cisco Jabber, Comet, Mobirise, Twobird, Usage, Visual Paradigm
and Whispering. Angry IP Scanner, InVesalius and KeyboardCleanTool
weren't changed by the trash or directive edits, so they're new in this
PR.
- **Frozen outputs (14):** hand-edited with the trash and directive
changes, since ingest never rewrites them. Logitech G HUB is newly
frozen since #56333. As in #56314, frozen apps keep the old quit helper
until they're unfrozen. Avast Secure Browser and Intune Company Portal
also don't get #56337's quit until then.
- **Custom-script outputs:** each uninstall script equals its input
file. Microsoft Word was rebuilt on `main`'s newer version. 1Password's
uninstall now comes from its new custom script (#56453). `go run
./cmd/maintained-apps --slug=1password/darwin` matches the committed
output, and its install script is unchanged from `main`.
- **DeskTime, DisplayLink, Fathom, NotesOllama:** #56016's regenerated
quit line, plus the trash change.

Generator cross-check: I ran the combined ingester for 20 apps,
including the four from #56016, LM Studio, Logitech G HUB, Box Drive,
Cloudflare WARP, Docker Desktop, the custom-script apps and a bot-bumped
app. Every uninstall script is identical to this PR's, except for
#56314's quit helpers and `|| exit 1` on the app's own quit, which the
bot adds.

#56337 cross-check: I generated all 964 non-custom macOS uninstall
scripts from today's cask API with this branch's generator, before and
after #56337. 624 change, each by one `quit_application
'<unique_identifier>' || exit 1` line, plus the helper in the 587 that
didn't have it. The next statement is always a removal: `sudo rm -rf`
(553), `remove_pkg_files` (60) or the cask's uninstall script (8).
DeskTime, DisplayLink, Fathom and NotesOllama don't change. For Asana,
Google Chrome, Microsoft Defender and Obsidian, `go run
./cmd/maintained-apps` produces the same uninstall script as the
committed output.

# Checklist for submitter

- [ ] Changes file added for user-visible changes in `changes/`,
`orbit/changes/` or `ee/fleetd-chrome/changes`.
See [Changes
files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files)
for more information.
  - None. The combined PRs' changes files were removed.

- [x] Input data is properly validated, `SELECT *` is avoided, SQL
injection is prevented (using placeholders for values in statements), JS
inline code is prevented especially for url redirects, and untrusted
data interpolated into shell scripts/commands is validated against shell
metacharacters.
- Cask-supplied paths, executables, args and string scripts still go
through `shellSingleQuote`. The only new interpolation is the quoted
`"$LOGGED_IN_USER"`.
- [x] Timeouts are implemented and retries are limited to avoid infinite
loops

## Testing

- [x] Added/updated automated tests
- `TestUninstallScriptStopsWhenAppWontQuit`, `TestQuitTargetsApp`,
`TestCustomUninstallScriptsUseCurrentQuitHelper` (#56314)
  - `TestUninstallScriptQuitsAppByBundleIDWhenCaskListsNames` (#56016)
- `TestUninstallScriptTrashWithoutGUIUser`,
`TestCustomUninstallScriptsUseCurrentTrash` (#56333)
  - `TestUninstallScriptScriptDirectiveWithoutGUIUser` (#56336)
- `TestCustomInstallScriptsStopWhenAppWontQuit` checks every `quit_*`
call (#56454)
- `TestUninstallScriptQuitsAppCaskDoesNotName`,
`TestUninstallScriptQuitsAppOnce` (#56337), and a quit-by-name case in
`TestUninstallScriptQuitsAppByBundleIDWhenCaskListsNames` that fails if
the app is quit twice
- 1Password's uninstall (#56453): I ran it with `/bin/bash` 3.2 and
`osascript`, `pkill`, `sudo`, `launchctl`, `stat`, `scutil`, `mv` and
`rm` stubbed. The `osascript` stub ignores quit events, as 1Password
does in menu-bar mode. Not running: no `pkill`, the app is removed, exit
0. Quits on SIGTERM: one `pkill -TERM -x 1Password`, the app is removed,
exit 0. Ignores SIGTERM: "did not quit", exit 1, nothing is removed.
shellcheck reports the same warnings as the generated script. The
SIGTERM behavior was checked on a real Mac for #56452.
- `go test ./ee/maintained-apps/... ./cmd/maintained-apps/...` passes
with Homebrew bash 5.3 and with `/bin/bash` 3.2 first on `PATH`.
golangci-lint (full and incremental) reports 0 issues on those packages.
Two incremental findings in #56314's tests now use `errors.AsType` and
`require.Contains`, like the other tests. A gosec G602 false positive in
#56337's order check is restructured.
- [ ] QA'd all new/changed functionality manually

## AI

**AI:** Claude Code (claude-opus-5-5)

---------

Co-authored-by: Claude <noreply@anthropic.com>
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.

1 participant