Repository navigation
Windows legacy: refuse paths the Recycle Bin would delete permanently - #118
volkanncicek wants to merge 2 commits into
Conversation
SHFileOperationW deletes a tree that holds a path of MAX_PATH characters or more permanently instead of recycling it, and still returns success; shortening the top-level path to 8.3 names does not help because the names inside stay long. The legacy backend now walks each path first and raises TrashPermissionError, the error arsenetar#115 uses for the IFileOperation backend, leaving the files in place. The legacy long-path tests expect that now, and a new test covers a short folder holding a long path.
| """Return the first path at or under *path* that is MAX_PATH characters or longer, or None. | ||
|
|
||
| SHFileOperationW deletes such a tree permanently instead of moving it to the Recycle Bin, | ||
| and still reports success, even when the top-level path has been shortened to 8.3 names. |
There was a problem hiding this comment.
Could you clarify what an "8.3 name" is exactly? This is a bit confusing.
There was a problem hiding this comment.
It's the short DOS-style alias Windows keeps for a long name: up to 8 characters plus a 3-character extension, like PROGRA~1 for Program Files. get_short_path_name() passes the top-level path to SHFileOperationW in that form. It doesn't help here because the shell still checks the long names inside the tree. I reworded the docstring to say that.
| _multi_byte_unicode(test_dir, s2t_legacy) | ||
|
|
||
|
|
||
| # SHFileOperationW deletes a path of MAX_PATH characters or more permanently instead of recycling it, |
There was a problem hiding this comment.
Can you confirm "characters" and not bytes? I wonder if the path contains unicode characters, we may reach the 260 char limit with fewer characters than that.
There was a problem hiding this comment.
Neither, as it turns out: MAX_PATH counts UTF-16 code units. Most characters are one unit, but anything outside the BMP (an emoji, for example) is two, so len() could undercount.
I checked it on Windows 11 with the guard turned off: a folder holding a child path of 228 characters but 260 UTF-16 units was deleted permanently; at 259 units it went to the Recycle Bin.
Fixed in 8cbc6e6: the check now counts UTF-16 units, with tests on both sides of the limit. I also stopped counting the \\?\ prefix, which made it refuse top-level paths of 256-259 units.
MAX_PATH counts UTF-16 code units, not characters: a character outside the BMP, such as an emoji, takes two. len() undercounted such a path, so a tree holding one of 260 units or more slipped past the guard and was deleted permanently. Measured on Windows 11 with the guard disabled: a folder holding a child of 228 characters / 260 units went unrecycled, one of 259 units went to the Recycle Bin. The top-level check also counted the \?\ prefix, which never reaches SHFileOperationW, and refused paths of 256 to 259 units. The docstring now says what an 8.3 name is and why passing one does not help.
|
One more finding before this goes further. The permanent delete only happens when the process is long-path aware, which needs both Measured on Windows 11 (build 26200), legacy backend with the guard disabled, each case twice in a fresh folder:
So as written, the guard refuses paths that most users can trash fine today. Two ways to go:
I lean towards 1, with no guard when the export is missing. @arsenetar, which would you prefer? |
On Windows,
SHFileOperationWdeletes a tree that holds a path ofMAX_PATH(260) characters or more permanently instead of moving it to the Recycle Bin, and still returns success. Shortening the top-level path to 8.3 names, as the legacy backend does, does not help: the names inside the tree stay long.Measured on Windows 11 (build 26200) with send2trash 2.1.0, one file per folder at an exact full-path length: 259 characters went to the Recycle Bin, 260 and longer did not, with both the legacy and the IFileOperation backend. The existing
test_trash_long_file_*tests pass because the file is gone afterwards, which is the permanent delete; the NOTE above them intest_plat_win.pydescribes the same thing.Change
Before calling
SHFileOperationW, the legacy backend walks each path and raisesTrashPermissionError(path)if the path or anything under it isMAX_PATHcharacters or longer, leaving everything in place.TrashPermissionErroris the error #115 raises when the IFileOperation backend's progress sink sees that an item would not be recycled, so both backends report the same thing. The legacy backend has no progress sink, so a check before the call is the only lever it has.Tests
test_trash_long_file_legacyandtest_trash_long_multifile_legacynow expectTrashPermissionErrorand check the files are still there.test_trash_folder_holding_long_path_legacy: a short folder whose contents passMAX_PATH, the common real case (a deep.venvornode_modules).flake8is clean andtests/test_plat_win.pypasses (21 tests) locally.Not covered
The check costs one
os.walkper path. It does not catch the other cases where the shell deletes instead of recycling, such as an item larger than the bin's quota or a drive without a Recycle Bin; the legacy API gives no way to see those before the call.Relates to #28, #94 and #117; complements #115.