Skip to content

fix: uninstall protected items with admin fallback#100

Open
3manu31 wants to merge 2 commits into
momenbasel:mainfrom
3manu31:fix/uninstall-admin-fallback
Open

fix: uninstall protected items with admin fallback#100
3manu31 wants to merge 2 commits into
momenbasel:mainfrom
3manu31:fix/uninstall-admin-fallback

Conversation

@3manu31
Copy link
Copy Markdown

@3manu31 3manu31 commented May 17, 2026

What does this PR do?

Addresses the issue where uninstalling apps fails, with a warning that FDA has not been granted, even though it has been.
It basically fixes the uninstall flow for protected items by retrying with administrator privileges when needed, treating missing files as already removed, and removing deleted apps from the list. Addresses issues #93, #95, #97.

Type of change

  • Bug fix
  • New feature
  • Performance improvement
  • UI/UX enhancement
  • Documentation
  • Other (describe)

Testing

  • Tested on macOS Ventura (13.x)
  • Tested on macOS Sonoma (14.x)
  • Tested on macOS Sequoia (15.x)
  • Tested on macOS Tahoe (26.x)

AI assistance disclosure

Drafted using Claude Code. Built the project locally to confirm my version compiles and the issue is addressed.

@sam-gurdus
Copy link
Copy Markdown

Nice catch on the underlying bug — the (removed, needsAdmin, failed) split is the right structural change, the per-file post-sudo re-stat is good, and pruneMissingInstalledApps is a real UX improvement. I just tested this scenario against a pkg-installed app (Microsoft Outlook.app + the two /var/db/receipts/com.microsoft.package.Microsoft_Outlook.app.{plist,bom} files), confirmed the current main fails exactly as #93/#95/#97 describe, and the diff here would have routed all three through the admin pass and succeeded.

Two concerns before this merges:

1. The single-quote escape in removeWithAdminPrivileges is broken.

"'\(url.path.replacingOccurrences(of: "'", with: "'\\\"'\\\"'"))'"

The standard bash trick for embedding a single quote inside a single-quoted string is '\'' (close-single, escaped-single, open-single). What's emitted here is '"'"' — a different (also valid) trick — but the Swift source produces '\"'\"' after the AppleScript escape pass doubles the backslashes, which is not the same and breaks. For paths without apostrophes (/Applications/Microsoft Outlook.app) this is fine, but for e.g. /Applications/Don't Starve.app you get a malformed shell command and, worse, the failure mode is shell injection by filename.

2. There's no safety allow-list before sudo rm -rf. CleaningEngine.cleanWithAdminPrivileges validates every path against isSafeToDelete / isExplicitSingleFileDeletable before escalating — refuses to admin-rm anything outside the cache/log roots. removeWithAdminPrivileges here will rm whatever the caller hands it. The uninstaller currently feeds only paths from AppPathFinder, but a future scanner false-positive (or a malicious app bundle name designed to confuse heuristics) becomes a sudo rm -rf on something dangerous.

Both concerns disappear if you reuse CleaningEngine.cleanWithAdminPrivileges directly. It already:

  • Stages NUL-separated paths to a temp file and uses xargs -0 rm -rf, so no quoting issues regardless of filename
  • Re-validates per path against an allow-list
  • Re-stats per path post-sudo to verify (matches what this PR does inline)

An uninstall-aware allow-list could extend isSafeToDelete to also accept /Applications/*.app exactly, /var/db/receipts/com.*.{plist,bom}, /Library/LaunchDaemons/*.plist, etc. — the categories that legitimately require root for app removal but nothing broader.

Two smaller things while you're in there:

  • NSFileReadNoPermissionError == 257 and NSFileWriteNoPermissionError == 513, so the literal 257/513 in permissionDeniedCodes are duplicates of the named constants
  • Including NSFileWriteUnknownError will route some genuinely failed-and-not-fixable-by-sudo cases into the admin retry path. Probably worth dropping or splitting from the EACCES/EPERM bucket

Drafted with Claude Code; happy to send a follow-up PR on top of this one if useful.

…ne for admin removal, add minimal uninstall allow-list, and tighten permission checks
@3manu31
Copy link
Copy Markdown
Author

3manu31 commented May 18, 2026

Thanks, I fixed the quoting/injection issue by removing the ad-hoc osascript shell-building and now call CleaningEngine.cleanWithAdminPrivileges (inherits NUL+xargs staging and per-path re-validation). I also tightened permission checks and added a minimal uninstall allow-list for /Applications/.app, /var/db/receipts/.{plist,bom}, and /Library/Launch{Agents,Daemons}/*.plist. Happy to add a small test for quoted filenames or further tighten the allow-list if reviewers prefer

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.

2 participants