Skip to content

Harden Content-Disposition filename sanitization in download_file to prevent path traversal#181

Open
Copilot wants to merge 3 commits into
mainfrom
copilot/check-security-advisory-ghsa-4c6f-3rvc-56cx
Open

Harden Content-Disposition filename sanitization in download_file to prevent path traversal#181
Copilot wants to merge 3 commits into
mainfrom
copilot/check-security-advisory-ghsa-4c6f-3rvc-56cx

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 14, 2026

Describe your changes
download_file accepted an untrusted filename from Content-Disposition without full cross-platform normalization, allowing traversal-like path components to be preserved in some cases.
This update hardens filename extraction so header-derived values cannot escape the target directory.

  • Scope

    • Updated src/fsutil/operations.py inside download_file:
      • normalize backslashes to /
      • then apply os.path.basename(...)
    • Kept the inline security comment requested in review.
    • Added a regression test in tests/test_operations.py for Content-Disposition: filename="..\\..\\evil.txt".
  • Behavior change

    • Header-derived filenames are sanitized for both POSIX (/) and Windows (\) separators before path join.
    • URL-derived filename and UUID fallback logic are unchanged.
    • Added test coverage to ensure the resulting file path remains inside dirpath.
  • Patch

if filename_match:
    # sanitize Content-Disposition filename to prevent path traversal
    filename = filename_match.group(1).replace("\\", "/")
    filename = os.path.basename(filename)

Related issue
GHSA-4c6f-3rvc-56cx

Checklist before requesting a review

  • I have performed a self-review of my code.
  • I have added tests for the proposed changes.
  • I have run the tests and there are not errors.

@fabiocaccamo fabiocaccamo marked this pull request as ready for review May 14, 2026 20:31
Copilot AI review requested due to automatic review settings May 14, 2026 20:31
Comment thread src/fsutil/operations.py Outdated
@codecov
Copy link
Copy Markdown

codecov Bot commented May 14, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.46%. Comparing base (c98dbf6) to head (a47e9c7).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #181      +/-   ##
==========================================
+ Coverage   98.33%   98.46%   +0.13%     
==========================================
  Files          13       13              
  Lines         780      781       +1     
==========================================
+ Hits          767      769       +2     
+ Misses         13       12       -1     
Flag Coverage Δ
unittests 98.46% <100.00%> (+0.13%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/fsutil/operations.py Outdated
filename_match = re.search(filename_pattern, content_disposition)
if filename_match:
filename = filename_match.group(1)
filename = os.path.basename(filename_match.group(1))
Comment thread src/fsutil/operations.py Outdated
filename_match = re.search(filename_pattern, content_disposition)
if filename_match:
filename = filename_match.group(1)
filename = os.path.basename(filename_match.group(1))
Copilot AI requested a review from fabiocaccamo May 14, 2026 20:35
@fabiocaccamo
Copy link
Copy Markdown
Owner

@copilot apply changes based on the comments in this thread

Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 14, 2026

@copilot apply changes based on the comments in this thread

Applied in a47e9c7: I updated filename sanitization to normalize Windows separators before basename, and added a regression test covering traversal input from Content-Disposition (..\\..\\evil.txt) to ensure output stays inside dirpath.

Copilot AI changed the title Sanitize Content-Disposition filename in download_file to prevent path traversal Harden Content-Disposition filename sanitization in download_file to prevent path traversal May 14, 2026
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.

3 participants