Skip to content

Fix symlink handling in CleanupLocalProcess + real-filesystem tests - #2

Open
nitrobass24 wants to merge 1 commit into
JAAdventurer:developfrom
nitrobass24:fix/663-cleanup-symlink-handling
Open

Fix symlink handling in CleanupLocalProcess + real-filesystem tests#2
nitrobass24 wants to merge 1 commit into
JAAdventurer:developfrom
nitrobass24:fix/663-cleanup-symlink-handling

Conversation

@nitrobass24

@nitrobass24 nitrobass24 commented Aug 26, 2026

Copy link
Copy Markdown

Follow-up to my review on nitrobass24#663. Everything else from that review landed cleanly in d65ab79 — this covers the three things still outstanding on the delete_process.py comment, plus one missing Angular spec. Targets this branch so it rides along in nitrobass24#663 rather than becoming a separate PR.

1. Symlink pointing outside the folder was never removed

islink was still checked after the containment check. _is_contained resolves the final component via realpath, so a local-only symlink pointing outside the folder resolves outside the base and is reported as path traversal — it never gets unlinked.

This is reachable: the scanner's entry.is_dir() follows symlinks, so a symlink to an external directory is scanned as a directory, enters the model with remote_size = None, and is returned by _find_local_only_paths.

Because failures now raise (correctly), it became a permanently failing cleanup — the other paths get deleted, but the command always reports failure, isCleanupLocalable stays true, and every retry fails the same way.

Replaced with _resolve_child_path, which resolves ancestor components — so a symlinked directory still can't be used to escape the base — but leaves the final component unresolved, so the link is unlinked where it sits without touching its target.

It also rejects . / .. / empty basenames. .. previously passed commonpath (commonpath(["/base/f", "/base/f/.."]) returns /base/f) and would have rmtree'd the parent folder.

2. Dangling symlinks

os.path.exists is False for a dangling symlink, so it was logged as non-existing and left in place. Now os.path.lexists.

Low severity in practice — the scanner's entry.stat() raises FileNotFoundError on a dangling link and skips it, so it only reaches this code through a scan/cleanup race — but it's a one-word fix.

3. Tests now run against a real filesystem

The mocked tests are what hid issue 1: test_relative_path_escaping_base_blocked asserted against a realpath stub returning /etc/passwd, so it proved nothing about real resolution, and symlink-to-dir / dangling-symlink had no coverage at all.

TestCleanupLocalProcess now uses real temp directories and covers symlink-to-dir, symlink-pointing-outside, dangling symlink, real ../../outside.txt traversal (asserting the outside file survives), symlinked-ancestor escape, .. basename, and batch continuation.

The PermissionError case faults os.unlink for a single path rather than using chmod, which doesn't deny root in the CI container. Note it has to pass through **kwargsrmtree unlinks its own entries via the same patch.

4. Missing Angular spec

Added the spec for the modelFile.remote_size !== null guard in isCleanupLocalable. The guard is load-bearing (a DEFAULT folder maps to a status in LOCAL_ACTION_STATUSES) but had no coverage. Includes a positive re-emit so it can't pass on the status gate instead.

Test plan

  • The two symlink tests fail against d65ab79 and pass here — verified by reverting delete_process.py alone
  • TestCleanupLocalProcess 19/19 pass
  • Full Python unit suite: 1006 passed (one pre-existing macOS-only failure, test_scan_file_with_latin_chars, which also fails on a clean develop — APFS rejects the non-UTF8 filename)
  • ruff check / ruff format --check / C901 clean, pyright 0 errors
  • ng test 578 passed, ng lint clean
  • Cleanup integration tests skip outside the container, so CI is their first real run

One thing left for you to call

I left "path does not exist" counting as a failure rather than a warning. With lexists it now only fires when something genuinely vanished, but a benign scan/cleanup race will still fail the whole batch. Happy to soften it to a warning if you'd rather.

…ing them (#663)

Follow-up to the #663 review. Three gaps remained after d65ab79:

1. `islink` was still checked after the containment check. `_is_contained`
   resolves the final component, so a local-only symlink pointing outside the
   folder resolved outside the base and was reported as path traversal — never
   unlinked. Since failures now raise, that turned into a permanently failing
   cleanup: the button stays enabled and every retry fails.

   Replaced with `_resolve_child_path`, which resolves ancestor components (a
   symlinked directory still cannot be used to escape) but leaves the final
   component unresolved, so the link is unlinked where it sits without touching
   its target. Also rejects `.`/`..`/empty basenames — `..` passed commonpath
   and would have rmtree'd the parent folder.

2. `os.path.exists` is False for a dangling symlink, so it was logged as
   non-existing and left in place. Switched to `os.path.lexists`.

3. The new tests mocked realpath/islink/isfile/exists, which is what hid (1) —
   the traversal test asserted against a fake realpath. TestCleanupLocalProcess
   now runs against real temp directories and covers symlink-to-dir,
   symlink-pointing-outside, dangling symlink, real `../../outside.txt`
   traversal, symlinked-ancestor escape, `..` basename, and batch continuation.
   The PermissionError case faults os.unlink for a single path instead of using
   chmod, which does not deny root in the CI container.

Also adds the missing Angular spec for the `remote_size !== null` guard in
isCleanupLocalable, with a positive re-emit so it cannot pass on the status gate.

Verified: the two symlink tests fail against d65ab79 and pass here.
@nitrobass24
nitrobass24 force-pushed the fix/663-cleanup-symlink-handling branch from a9521fc to 9d67497 Compare August 26, 2026 02:42
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