fix: Path Traversal Safety for MedCheck

[dewhush]

Hey there! 👋

I was reviewing the codebase and noticed a potential security issue that I thought I'd flag and fix.

What I found

• [HIGH] path_traversal in src/medcheck/providers/local.py: Zip Slip / Path Traversal due to insecure path prefix validation. The startswith check without a trailing path separat

What I changed

The fix is minimal and targeted — I added proper validation/sanitization where user-controlled or untrusted data enters sensitive operations. No changes to existing functionality or public APIs.

Testing

Ran the existing test suite locally, everything passes. The change is backward-compatible.

Happy to discuss if you have questions!

Relates to: #100

---

💛 If this fix helps, donations are appreciated (ETH/ERC-20): 0x1478f1BDEACc7b434b4405350A15993cDcddc79F (Etherscan)

Summary by CodeRabbit

• Bug Fixes
  • Strengthened ZIP file extraction security by improving path validation to prevent directory traversal vulnerabilities.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 7b4133ac-e277-4fbf-90e5-d6fff9b07c11

📥 Commits

Reviewing files that changed from the base of the PR and between cce065f and f222a20.

📒 Files selected for processing (1)

• src/medcheck/providers/local.py

---

📝 Walkthrough

Walkthrough

LocalProvider._scan_zip now resolves the temporary extraction directory to an absolute path and validates each ZIP member's intended extraction path using Path.is_relative_to(), replacing the prior string-prefix check. The inline comments are updated to reflect this stronger boundary enforcement.

Changes

ZIP Traversal Guard

Layer / File(s)	Summary
ZIP member path boundary validation src/medcheck/providers/local.py	Resolves the temp extraction directory and switches the traversal guard from a string-prefix test to Path.is_relative_to(), with updated comments.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~5 minutes

Possibly related issues

• [repo-health] Medium: LocalProvider.fetch() accepts arbitrary filesystem paths — path traversal risk when /api/analyze is wired up #61: This PR directly implements the Path.is_relative_to() fix in _scan_zip() that the issue identifies as the traversal prevention mechanism; the issue also notes a complementary vulnerability in the fetch() method's directory path branch that remains unaddressed.

Poem

🐇 A string once guarded the ZIP's domain,
But symlinks and dots could slip through the chain.
Now is_relative_to() stands at the gate,
Resolved paths checked — no escape from their fate.
No traversal tricks past this vigilant hare! 🛡️

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description identifies the security issue and explains the fix, but is incomplete against the template. It lacks key sections: no numbered issue link in Summary, no detailed bullet-point Changes list, and no Testing section with checkboxes confirming test execution and coverage validation.	Complete the description by: (1) adding 'Fixes #100' in Summary section, (2) expanding Changes with specific implementation details (e.g., 'Replaced startswith check with Path.is_relative_to()'), (3) adding Testing section with checkboxes confirming all tests pass and coverage thresholds met.
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'fix: Path Traversal Safety for MedCheck' clearly and concisely summarizes the main change - fixing a path traversal security vulnerability. It is specific, relevant to the changeset, and follows conventional commit style.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.}

[Liohtml]

Thanks for the contribution! The core change is correct and a real improvement.

The fix is sound. The previous str(...).startswith(str(tmp_dir.resolve())) check has a genuine sibling-directory bypass: if the temp dir resolves to /tmp/abc, a member resolving to /tmp/abc-evil/... would pass the prefix check. Path.is_relative_to() enforces a true directory-boundary check, so this closes that gap. is_relative_to() is available on Python 3.9+ and the project requires >=3.10, so compatibility is fine.

Two things before this can merge:

1. Issue reference is wrong. This PR links #100, but #100 is the CSRF on POST /api/analyze finding. The zip-slip/path-traversal hardening you're fixing here is tracked in #61 — please update "Relates to" to #61.

2. Please add a regression test. There's currently no test covering _scan_zip traversal handling (in tests/unit/test_providers/test_local.py), so this fix could silently regress. A small test that builds a ZIP with a ../evil.dcm member and asserts _scan_zip raises ValueError("Unsafe path in ZIP: ...") would lock the behavior in. Something like:

   import zipfile
   import pytest
   
   def test_scan_zip_rejects_traversal(tmp_path):
       zip_path = tmp_path / "evil.zip"
       with zipfile.ZipFile(zip_path, "w") as zf:
           zf.writestr("../escape.dcm", b"not really dicom")
       provider = LocalProvider()  # adjust to however the suite constructs it
       with pytest.raises(ValueError, match="Unsafe path in ZIP"):
           provider._scan_zip(zip_path)

(Note for maintainer: CI hasn't run yet — first-time-contributor workflows need approval.)

---

Generated by Claude Code

[Liohtml]

Thanks for this fix! The is_relative_to() change landed on main as part of #122, which additionally rejects symlink ZIP members and adds ZIP-bomb guards in the same code path — so this PR is now superseded. Closing it, but the approach here was exactly right; much appreciated.

---

Generated by Claude Code