Skip to content

fix: harden path validation against traversal and Windows drive escape - #809

Merged
curdriceaurora merged 3 commits into
mainfrom
fix/path-security-hardening
Mar 14, 2026
Merged

fix: harden path validation against traversal and Windows drive escape#809
curdriceaurora merged 3 commits into
mainfrom
fix/path-security-hardening

Conversation

@curdriceaurora

@curdriceaurora curdriceaurora commented Mar 14, 2026

Copy link
Copy Markdown
Owner

Summary

Test plan

  • Existing web route tests pass (732 total, confirmed locally)
  • New traversal regression tests pass (test_path_prefix_attack_blocked, test_dotdot_traversal_via_subdirectory_blocked, test_symlink_escaping_allowed_root_blocked)
  • New Windows drive regression tests pass (test_rejects_windows_drive_in_category, test_rejects_windows_drive_with_path_in_category, test_rejects_windows_drive_in_filename, test_rejects_windows_drive_in_filename_via_setattr)
  • mypy --strict clean on changed source files (confirmed: zero errors in api/utils.py, interfaces/pipeline.py, file_mover.py)
  • Pre-commit validation passes (ruff, format, CI guardrails, codespell)

Closes #672, closes #760

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Improved path security by enforcing stricter validation of file paths against allowed directories.
    • Enhanced symlink and cross-platform path handling to prevent directory escape attacks.
    • Strengthened validation to block Windows drive-qualified paths and traversal sequences.
  • Tests

    • Added comprehensive security tests for path validation including symlink, prefix, and drive-path scenarios.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

@coderabbitai

coderabbitai Bot commented Mar 14, 2026

Copy link
Copy Markdown

Caution

Review failed

Pull request was closed or merged during review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: cd9351d0-b0a1-463f-9ed7-6956f617e7cd

📥 Commits

Reviewing files that changed from the base of the PR and between 29026dc and 6b0339b.

📒 Files selected for processing (6)
  • src/file_organizer/api/utils.py
  • src/file_organizer/interfaces/pipeline.py
  • src/file_organizer/methodologies/para/ai/file_mover.py
  • tests/api/test_utils.py
  • tests/ci/test_path_security_contract.py
  • tests/pipeline/test_stages.py

📝 Walkthrough

Walkthrough

Path validation logic is hardened across multiple modules by replacing string-based comparisons with resolved Path containment checks. The API utils now enforce allow-lists using Path.is_relative_to(), pipeline validation blocks Windows drive-qualified components, and the PARA module improves directory containment detection. Security tests are added to verify these protections.

Changes

Cohort / File(s) Summary
API Path Validation Hardening
src/file_organizer/api/utils.py, tests/api/test_utils.py, tests/ci/test_path_security_contract.py
Replaced string prefix checks with Path.resolve() and Path.is_relative_to() for robust allow-list enforcement. Function now returns Path object instead of string. Added 6 comprehensive regression tests covering prefix attacks, traversal via subdirectory, symlink escaping, Windows drive-qualified paths, and UNC paths. Updated security contract tests to use canonical path resolution.
Pipeline Windows Path Component Validation
src/file_organizer/interfaces/pipeline.py, tests/pipeline/test_stages.py
Enhanced _validate_path_component() to reject Windows drive-qualified values (e.g., "C:", "C:docs") via PureWindowsPath.drive/anchor checks. Added early return for empty values. Added tests verifying rejection of drive specifications in category and filename fields, plus positive tests for valid inputs.
PARA Module Path Containment
src/file_organizer/methodologies/para/ai/file_mover.py
Updated _is_already_organized() to use Path.is_relative_to() for containment check instead of string prefix comparison. Broadened exception handling to catch both OSError and ValueError.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 A rabbit hops through safer paths today,
No symlinks twist or .. find their way,
Windows drives now blocked from straying far,
Containment checks shine bright like stars,
Security tightens, locks are strong! 🔐

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 26.32% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly and specifically describes the main changes: hardening path validation against traversal attacks and Windows drive escape vulnerabilities.
Linked Issues check ✅ Passed All coding requirements from #672 and #760 are met: unsafe string-based checks replaced with Path.resolve().is_relative_to() logic, Windows drive/anchor validation added with PureWindowsPath, and comprehensive regression tests added.
Out of Scope Changes check ✅ Passed All changes are directly related to the security hardening objectives in #672 and #760; no unrelated modifications detected.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/path-security-hardening
📝 Coding Plan
  • Generate coding plan for human review comments

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

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

Issue #672: Replace os.path.commonpath string comparison in resolve_path()
with Path.resolve().is_relative_to() — correctly handles symlinks, ".."
sequences, and path-prefix spoofing (e.g. /allowed_dir_extra passing a
startswith check against /allowed_dir).  Also replace str.startswith()
in FileMover._is_already_in_category() with the same pattern.

Issue #760: Extend StageContext._validate_path_component() to reject
Windows drive-qualified values such as "C:" and "C:docs" that carry no
slash but still produce a non-empty PureWindowsPath.drive/.anchor,
allowing output_dir / category to escape the intended output directory
on Windows.  Adds regression tests for both construction-time and
setattr assignment paths.

Update tests/ci/test_path_security_contract.py allowlists to match the
new resolve_path() implementation (Path.expanduser().resolve() instead
of os.path.realpath).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@curdriceaurora
curdriceaurora force-pushed the fix/path-security-hardening branch from 05fd5e0 to aa2b3a2 Compare March 14, 2026 11:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment