sindresorhus / macos-trash

Move files and folders to the trash
MIT License
376 stars 14 forks source link

[BUG] trash moves files to wrong user when executing with elevated privileges #11

Open JayBrown opened 3 years ago

JayBrown commented 3 years ago

Let's say we have a file /Users/Shared/foo.txt owned by root:wheel.

Moving this to the currently logged-in user's trash works fine in Finder: you just have to enter your admin password.

Problems arise when using trash instead:

So it seems that the trash CLI is only checking which user is executing the command (0), but not if that's also the same as the logged-in user, e.g. 501.

If that's true, the trash CLI should determine the actual home folder of the executing user by running the equivalent of eval echo "~$(scutil <<< \"show State:/Users/ConsoleUser\" | awk '/Name :/ && ! /loginwindow/ { print $3 }')", and then use the trash directory of that user.

This bug should be fixed, because users should be able to move non-writable files to their own trash, in the same way it's possible in the Finder.

sindresorhus commented 3 years ago

Currently, it's just reverting back to non-sudo user: https://github.com/sindresorhus/macos-trash/blob/e5250fa3971a504d22820ccecfe4df42e22e938c/Sources/trash/util.swift#L26-L37

I would be happy to see this improved, but it's not something I plan to work on.


Related issue: https://github.com/sindresorhus/trash-cli/issues/28

sindresorhus commented 3 years ago

This is how to get the console username:

import SystemConfiguration

func getConsoleUser() -> String? {
    SCDynamicStoreCopyConsoleUser(nil, nil, nil) as String?
}

getConsoleUser()

And: https://github.com/Nikhil0487/privileges-CLI/blob/32686cf01f26d697479bca978d129eff4d7d538e/privilegehelper/privilegehelper/com_privilege_helper.swift#L128-L140

JayBrown commented 3 years ago

I have read the issue you pointed to (#28), and your reasoning there makes sense, also what @russelldavis wrote. Personally I think it wouldn't be a good idea to change the ownership or permissions of the parent directory of the file specified for removal, when the user runs sudo trash or the above osascript command with admin privileges.

Maybe, for these cases, you could rather add an -For --finder argument, and when trash -F foo fails because of permissions when using the system API, trash will just call Finder to activate & perform the deletion, and Finder would then ask the user for the password in the GUI. That would probably be the safest way.

Or you concoct some weird workaround that relies on neither the system API nor the Finder. But the users can probably do that themselves with a shellscript using sudo somehow. That's what I'm testing now, but I also kinda tend to just calling Finder instead.

sindresorhus commented 3 years ago

I think falling back to Finder automatically if it fails and the terminal is interactive is an ok workaround.

JayBrown commented 3 years ago

Ah, yes, the interactive issue. Maybe you could use the trash -F argument for users who run trash in shell scripts in the background (detached), but still need user interaction.

Another thing would be the following: when executing trash foo bar, and both foo & bar are protected, the program will fail already with the first one. Maybe trash could cycle through the list of files and try to move them to the trash, but continue if the system API fails, and come back to the ones that failed at the end, to then remove the remaining files by calling Finder.

JayBrown commented 3 years ago

If anyone's interested… this should probably do it in a shell script context:

rmlist=""
for filepath in "$@"
do
    ! trash "$filepath" &>/dev/null && rmlist="$rmlist\n$filepath"
done
rmlist=$(echo "$rmlist | grep "^$")
if [[ $rmlist ]] ; then
    osascript 2>/dev/null << EOR
set theFiles to the paragraphs of "$rmlist"
repeat with aFile in theFiles
    set aFile's contents to (POSIX file aFile) as Unicode text
end repeat
theFiles
tell application "Finder"
   move theFiles to the trash
end tell
EOR
fi
sindresorhus commented 3 years ago

Maybe trash could cycle through the list of files and try to move them to the trash, but continue if the system API fails

It's much faster to pass all paths to the system API than to iterate through them.

JayBrown commented 3 years ago

Definitely. I use it in a shell script anyway and cycle through the filepaths, similar to the snippet above… so I have my own workaround for that. ;) The only implementation I could think of is another argument, e.g. trash -c or trash --cycle, but not as the default functionality.

eugenesvk commented 1 year ago

Have you by any chance found a non-Finder solution? I don't have a running Finder process and would prefer not to launch one for moving files to the proper trash folder

JayBrown commented 1 year ago

I have disabled Finder too, and I'm using the following workaround.

When I need the Finder, I'm toggling it with the following shell script that's mapped to my TouchBar using BetterTouchTool. (You can also map it differently, of course.)

#!/bin/zsh
# toggleFinder.sh
tempdir="$HOME/.cache/Finder"
! [[ -d $tempdir ]] && mkdir -p "$tempdir" &>/dev/null
if pgrep -x Finder &>/dev/null ; then
    osascript -e 'tell application "Finder" to quit' &>/dev/null
    rm -f "$tempdir/manual" 2>/dev/null
else
    touch "$tempdir/manual"
    osascript -e 'tell application "Finder" to activate' &>/dev/null
fi
exit

Additionally, I have a enabled a user LaunchAgent which runs every 30 seconds and auto-quits Finder whenever Finder has launched without the above toggleFinder script:

#!/bin/zsh
# checkFinder.sh
! pgrep -x Finder &>/dev/null && exit
[[ -e "$HOME/.cache/Finder/manual" ]] && exit
osascript -e 'tell application "Finder" to quit' &>/dev/null
localdate=$(date)
echo "Finder automatic quit: $localdate"
exit

The .plist for the agent:

<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<dict>
    <key>Label</key>
    <string>local.checkFinder</string>
    <key>LowPriorityBackgroundIO</key>
    <true/>
    <key>LowPriorityIO</key>
    <true/>
    <key>Nice</key>
    <integer>20</integer>
    <key>ProcessType</key>
    <string>Background</string>
    <key>Program</key>
    <string>/usr/local/bin/checkFinder.sh</string>
    <key>RunAtLoad</key>
    <true/>
    <key>StandardErrorPath</key>
    <string>/tmp/local.checkFinder.stderr</string>
    <key>StandardOutPath</key>
    <string>/tmp/local.checkFinder.stdout</string>
    <key>StartInterval</key>
    <integer>30</integer>
</dict>
</plist>

In addition, I'm using macos-trash as part of a workflow to delete my files, and in that workflow I've added a simple check: if Finder is already running before trash commences, leave Finder running at the end; if Finder is not running, quit Finder at the end using osascript.

eugenesvk commented 1 year ago

Thanks for your detailed guide on how to get rid of the pesky Finder, but am I correct to assume from this that there is no known fix, i.e., it's impossible to correctly identify the user's Trash and move files there instead of using the system Trash?