fix(bundle): reject path traversal in import (CVE-2007-4559, #9)

[Crystora]

What changed

bundle.import_apply now resolves each tar member path under kb_dir and rejects any name that escapes it via ../, an absolute path, or a NUL byte. The same check is applied in import_check and export_check so malicious bundles fail at the diff and verify stages too. A new _safe_member_path() helper centralizes the logic.

Why

Fixes #9

Bundle import joined attacker-controlled tar member names directly onto kb_dir (CVE-2007-4559) - a crafted .tar.gz with a member named ../../evil.txt could write anywhere the process had permission. The pre-existing manifest allow-list did not help, because the manifest lives inside the same attacker-controlled tarball.

What might break

Nothing for existing users:

• No file on disk moves or changes shape.
• No kb.* method changes signature or default behavior on well-formed bundles.
• The only behavior change is that bundles containing unsafe paths now raise RuntimeError instead of silently extracting - which is the fix.

Not a surface change.

VEP

Tests

• make check passes locally (lint + mypy + pytest) - 57/57 passing, ruff clean
• New / changed behaviour has a test — three new tests in tests/test_bundle.py:
  • test_import_apply_rejects_path_traversal reproduces the ../../evil.txt attack from the issue
  • test_import_apply_rejects_absolute_path covers the /abs/path variant
  • test_import_check_flags_path_traversal confirms the diff phase reports it
• CHANGELOG.md updated under ## [Unreleased]

Summary by CodeRabbit

• Bug Fixes

  • Resolved a path traversal security issue in bundle imports so crafted archives can no longer write files outside the target directory; manifest and archive member paths are validated to block traversal and absolute paths (CVE-2007-4559).

• Tests

  • Added security-focused tests to verify unsafe archive entries are detected and rejected.

• Documentation

  • Updated changelog with the security fix.

[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: d9901e60-5f9b-43ce-a6c1-6444a51f07cf

📥 Commits

Reviewing files that changed from the base of the PR and between fa83d3d and 072c932.

📒 Files selected for processing (2)

• src/vouch/bundle.py
• tests/test_bundle.py

---

📝 Walkthrough

Walkthrough

Adds path-safety checks for tar member names and manifest paths (rejecting empty, absolute, NUL, or .. components), integrates these checks into export/import flows, makes import_apply abort on validation errors, adds tests covering malicious tar members, and documents the fix in CHANGELOG.md.

Changes

Bundle Path Traversal Security Fix

Layer / File(s)	Summary
Changelog security fix entry CHANGELOG.md	Adds Unreleased "Fixed" entry documenting the tar path traversal mitigation and CVE-2007-4559 reference.
Path-safety helpers and export check src/vouch/bundle.py	Adds _unsafe_name_reason() and _safe_member_path() and updates export_check() to validate manifest paths and tar member names for emptiness, absolute paths, NUL bytes, and .. traversal.
Import check path safety integration src/vouch/bundle.py	import_check() resolves manifest entries via _safe_member_path(), records path errors into issues, skips unsafe entries, and sets ok = not issues.
Import apply validation enforcement and safe extraction src/vouch/bundle.py	import_apply() raises RuntimeError when import_check() reports issues and uses _safe_member_path() for extraction destinations to prevent writes outside the KB directory.
Path traversal security tests tests/test_bundle.py	Adds tar/IO/hash imports, _UNSAFE_PATH_RE, _write_malicious_bundle() helper, and tests asserting import_check() flags unsafe paths and import_apply() rejects traversal and absolute member names without escaping KB dir.

Sequence Diagram

sequenceDiagram
  participant Client
  participant import_apply
  participant import_check
  participant _safe_member_path
  participant KBDir
  Client->>import_apply: request import-apply(bundle)
  import_apply->>import_check: validate manifest entries
  import_check->>_safe_member_path: resolve member name -> dest
  _safe_member_path-->>import_check: dest path or raise RuntimeError
  alt issues found
    import_check-->>import_apply: issues, ok=False
    import_apply-->>Client: raise RuntimeError (abort)
  else ok
    import_apply->>_safe_member_path: resolve extraction destinations
    _safe_member_path-->>KBDir: resolved safe path
    import_apply->>Client: success
  end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 A tar came hopping with names that would roam,

../ here, /there — they tried to leave home.
We resolve and we check each soft thread,
Now bundles stay tidy where bunnies make bed.
Safe hops, safe KB — the rabbit nods, well fed.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 45.45% 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 clearly and specifically describes the main security fix: rejecting path traversal in bundle import with CVE reference and issue number.
Linked Issues check	✅ Passed	The PR successfully implements all coding requirements from issue #9: validates tar member paths to prevent traversal, implements resolve/relative_to approach, rejects absolute paths and NUL bytes, and raises RuntimeError on unsafe paths.
Out of Scope Changes check	✅ Passed	All changes are directly related to issue #9: path-safety checks in bundle.py, CHANGELOG documentation, and comprehensive test coverage for path traversal scenarios.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

[coderabbitai]

🧹 Nitpick comments (1)

tests/test_bundle.py (1)

121-126: 💤 Low value

Consider case-insensitive string matching for robustness.

The substring check uses lowercase "traversal" and "unsafe", which could fail if the implementation's error messages use different casing (e.g., "Path traversal" or "Unsafe path"). Since tests pass now, this is fine, but a case-insensitive check would be more resilient to future message changes.

♻️ Suggested improvement

-    assert any("traversal" in i or "unsafe" in i for i in result.issues)
+    assert any("traversal" in i.lower() or "unsafe" in i.lower() for i in result.issues)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_bundle.py` around lines 121 - 126, The test
test_import_check_flags_path_traversal currently checks result.issues for
substrings "traversal" or "unsafe" using a case-sensitive match; make this
resilient by performing a case-insensitive check: iterate over result.issues and
compare lowercase forms (e.g., use i.lower() and check for "traversal" or
"unsafe") so messages like "Path traversal" or "Unsafe path" are matched; update
the assertion that inspects result.issues in the test to use this lowercase
comparison.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@tests/test_bundle.py`:
- Around line 121-126: The test test_import_check_flags_path_traversal currently
checks result.issues for substrings "traversal" or "unsafe" using a
case-sensitive match; make this resilient by performing a case-insensitive
check: iterate over result.issues and compare lowercase forms (e.g., use
i.lower() and check for "traversal" or "unsafe") so messages like "Path
traversal" or "Unsafe path" are matched; update the assertion that inspects
result.issues in the test to use this lowercase comparison.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: a2bda4bd-6824-4299-9a18-9916ed7e0915

📥 Commits

Reviewing files that changed from the base of the PR and between 89172aa and fa83d3d.

📒 Files selected for processing (3)

• CHANGELOG.md
• src/vouch/bundle.py
• tests/test_bundle.py

[Crystora]

Hi @plind-junior Could you plz review this PR and share any feedback? Thanks

[plind-junior]

Nice. The resolve + relative_to approach in _safe_member_path is what makes this stick. It handles subdir/../../escape as well as the simple leading case, and stays correct if kb_dir itself is a symlink. Good call having the apply loop re-check too, even though import_check already screened the manifest.

The repro test is the right kind: it builds the actual attacker bundle and asserts the canary never appears.

A few small things, none blocking:

• export_check rolls its own unsafe-path predicate at src/vouch/bundle.py:128 and :137. It catches today's attack but isn't the same check as _safe_member_path. If we ever tighten the import side, export will drift. Could we pull the predicate into one place so they can't diverge?

• Not really this PR's problem, but worth a follow-up issue: import_apply writes the tar bytes without verifying SHA256 against the manifest. Path traversal is closed now, but a bundle can still ship payload bytes that don't match what its manifest claims.

• The canary in test_import_apply_rejects_path_traversal lands at tmp_path.parent / "evil.txt", which is pytest's shared tmp root. Works, but pointing the bundle at tmp_path / "kb" as kb_dir and asserting nothing escapes that subdir would make the test self-contained.

LGTM once the export side uses the same predicate.

[Crystora]

Thanks for your feedback. Addressed the centralization point : both export_check and _safe_member_path now go through a single _unsafe_name_reason() predicate. Could you plz review it again?

[plind-junior]

LTGM!