Changed from fs.stat(p) to fs.unixFS.Lstat(p)

[ashap5]

SafeDeleteRecursively was failing with "openat2: bad path resolution" errors when encountering symlinks that point outside the filesystem root.

Changes :

• Replaced calls to fs.Stat with fs.unixFS.Lstat in both the initial file info retrieval and the directory existence check, which allows for correct processing of symbolic links and special files during recursive deletion.

I have tested this and it fixes the issue and doesnt seem to cause any side effects

This fixes the path resolution errors while maintaining the security boundaries that openat2 enforces.
Let me know if you see any issues with this!

Summary by CodeRabbit

• Bug Fixes
  • Improved handling of symlinks during recursive deletion operations for more consistent behavior.

[coderabbitai]

Walkthrough

The SafeDeleteRecursively function in the filesystem module was modified to use Lstat() instead of Stat() for path information retrieval. This change affects two locations: the initial path check and the post-deletion existence verification for child items.

Changes

Cohort / File(s)	Summary
Symlink Handling in Recursive Deletion server/filesystem/filesystem.go	Modified SafeDeleteRecursively to use fs.unixFS.Lstat() instead of fs.Stat() for initial path information and post-deletion child existence checks. This alters symlink traversal behavior—Lstat() does not follow symbolic links, whereas Stat() does.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

• Specific areas requiring attention:
  • Verify that the change from Stat() to Lstat() does not break existing symlink deletion behavior or create unintended security implications
  • Confirm the behavioral change aligns with intended semantics for recursive deletion of directories containing symlinks
  • Check if there are existing tests covering symlink edge cases that validate the new behavior

Poem

🐰 A rabbit's whisper on the wind so fair,
"Symlinks sorted, no longer crossed with care!"
Where Lstat stands firm and doesn't bend,
Deletion flows true from start to end. ✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title directly references the main technical change (replacing fs.Stat with fs.unixFS.Lstat) and is specific and clear.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

server/filesystem/filesystem.go (1)

404-407: Consider documenting symlink behavior.

The function comment doesn't mention how symbolic links are handled. After these changes, symlinks are treated as files and deleted directly without following them. Consider adding a note about this behavior to help users understand that symlinks won't be dereferenced during deletion.

Example addition:

 // SafeDeleteRecursively deletes a file or directory while respecting the denylist.
 // For files, deletion is skipped if the file matches the denylist. For directories,
 // it recursively deletes all non-denylisted files and subdirectories. Empty directories
 // are removed automatically, but directories containing denylisted files are preserved.
+// Symbolic links are treated as files and deleted without following them.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7b55e6b and fd3d97f.

📒 Files selected for processing (1)

• server/filesystem/filesystem.go (2 hunks)

🔇 Additional comments (2)

server/filesystem/filesystem.go (2)

409-409: Correct fix for symlink handling.

Changing from Stat to Lstat is the right approach here. This prevents following symbolic links, which was causing "openat2: bad path resolution" errors when symlinks pointed outside the filesystem root. With Lstat, symlinks are now treated as files and deleted directly without dereferencing them, which is the correct behavior for a deletion operation and maintains security boundaries.

---

439-439: Consistent with the initial path check.

The change to Lstat here maintains consistency with line 409. This ensures that the existence check after recursive deletion doesn't attempt to follow symlinks, which would cause the same openat2 errors that the initial check was experiencing.

[QuintenQVD0]

Good catch! LGTM!