fix:bundle import can overwrite the audit log

[jonathanchang31]

Fixes #166.

This PR is the replacement for #167, retargeted to test because GitHub does not allow changing the base branch of a closed pull request.

Validation:

• pytest tests/test_bundle.py -q
• pytest
• ruff check src tests

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: b7de1730-b782-49cc-a66c-22433baa0c9e

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

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

[jonathanchang31]

@coderabbitai Could you plz review my PR?

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Fetch PR details
gh pr view 183 --repo vouchdev/vouch --json title,body,files,baseRefName,headRefName,additions,deletions,changedFiles

echo "---DIFF---"
gh pr diff 183 --repo vouchdev/vouch

echo "---ISSUE 166---"
gh issue view 166 --repo vouchdev/vouch

Length of output: 10445

---

🏁 Script executed:

#!/bin/bash
# Get the full import_apply function from the PR branch
gh api repos/vouchdev/vouch/contents/src/vouch/bundle.py \
  -H "Accept: application/vnd.github.v3.raw" \
  --jq '.content' 2>/dev/null | base64 -d 2>/dev/null || \
  gh api repos/vouchdev/vouch/git/refs/heads/fix/bundle-import-overwrite-audit-log \
  --jq '.object.sha' | xargs -I{} gh api repos/vouchdev/vouch/git/blobs/{} 2>/dev/null

# Try direct content fetch on the PR branch
gh api "repos/vouchdev/vouch/contents/src/vouch/bundle.py?ref=fix/bundle-import-overwrite-audit-log" \
  --jq '.content' | base64 -d

Length of output: 928

---

🏁 Script executed:

#!/bin/bash
# Get head SHA from the PR
HEAD_SHA=$(gh pr view 183 --repo vouchdev/vouch --json headRefOid --jq '.headRefOid')
echo "Head SHA: $HEAD_SHA"

# Fetch bundle.py blob via tree
TREE_SHA=$(gh api "repos/vouchdev/vouch/git/commits/$HEAD_SHA" --jq '.tree.sha')
echo "Tree SHA: $TREE_SHA"

# Get file sha from tree (recursive)
FILE_SHA=$(gh api "repos/vouchdev/vouch/git/trees/$TREE_SHA?recursive=1" \
  --jq '.tree[] | select(.path == "src/vouch/bundle.py") | .sha')
echo "File SHA: $FILE_SHA"

# Get blob content
gh api "repos/vouchdev/vouch/git/blobs/$FILE_SHA" --jq '.content' | base64 -d

Length of output: 25660

---

---

@jonathanchang31 — great work tracking down and fixing this security gap! Here's my full review.

---

Review of PR #183 — fix: bundle import can overwrite the audit log

Overall assessment ✅

The fix is correct and well-layered. The two independent guards complement each other nicely:

Layer	Where	What it catches
Manifest safety flags	_manifest_safety_issues → import_check	An honest (non-malicious) bundle that accurately declares forbidden content
Path-level block	_import_path_issue → _safe_member_path	A dishonest bundle that lies about its safety flags but tries to sneak in a forbidden file

import_apply calls import_check first and raises on the first issue, so the safety-flag check propagates correctly through import_apply without needing to be duplicated. ✓

---

Issues

1. safety or {...} is falsy for an empty dict (test helper)

# tests/test_bundle.py
"safety": safety
or {"has_proposed": False, "has_state_db": False, "has_audit_log": False},

If a caller ever passes safety={}, this silently uses the default instead of the empty dict. The current call sites only pass None or a non-empty dict, so it doesn't break anything today — but the idiomatic guard is clearer:

"safety": (
    {"has_proposed": False, "has_state_db": False, "has_audit_log": False}
    if safety is None
    else safety
),

---

2. Explicit forbidden-path checks are redundant with the EXPORT_SUBDIRS guard (minor)

In _import_path_issue:

if name == "audit.log.jsonl":
    return "forbidden path in bundle: 'audit.log.jsonl'"
if name == "state.db" or name.startswith("state.db-"):
    return f"forbidden path in bundle: {name!r}"
if name == "proposed" or name.startswith("proposed/"):
    return f"forbidden path in bundle: {name!r}"
subdir = name.split("/", 1)[0]
if subdir not in EXPORT_SUBDIRS:
    return f"path outside importable bundle artifacts: {name!r}"

audit.log.jsonl, state.db, and proposed are not in EXPORT_SUBDIRS, so the final guard would catch all of them anyway. The explicit checks exist only to produce a more specific error message ("forbidden path in bundle" vs "path outside importable bundle artifacts"). That UX trade-off is reasonable, but a comment explaining the intent would help future readers understand why the code isn't just relying on the generic guard:

# Explicit checks to produce actionable "forbidden" messages rather than
# the generic "path outside importable bundle artifacts" error. All three
# would be caught by the EXPORT_SUBDIRS guard below, but the specific
# message helps operators debug a malicious or misbuilt bundle faster.

---

3. _manifest_safety_issues inconsistency: has_audit_log path description doesn't match audit.log.jsonl check

FORBIDDEN_SAFETY_FLAGS = {
    "has_proposed": "proposed/",
    "has_state_db": "state.db",
    "has_audit_log": "audit.log.jsonl",
}

The value for has_audit_log is "audit.log.jsonl" (no trailing slash), while has_proposed's value ends with /. This is correct and intentional, but it's inconsistent in format. Not a bug — just noting it in case the values are ever used for prefix matching later.

---

4. Missing test: bare proposed name

_import_path_issue handles name == "proposed" (no trailing slash), but test_import_rejects_non_committable_bundle_paths only tests proposed/pending.yaml. A tar archive typically doesn't emit a bare directory entry, so this is a low-risk gap, but a cheap parametrize entry would make the intent explicit:

("proposed", b"", "proposed"),  # bare dir name

---

5. PR title formatting nit

The title is "fix:bundle import can overwrite the audit log" — missing a space after the colon. Conventional commits style is fix: bundle import....

---

Strengths

• Defense-in-depth is the right approach — both the manifest declaration and the actual extracted path are independently validated.
• The import_apply TOCTOU guard (re-verifying sha256 at write time) was already in place and still applies.
• The parametrized tests cover every explicitly forbidden path and all three safety flags individually — very thorough.
• The canary test (audit_path.write_text → confirm unchanged after RuntimeError) is exactly the right way to verify the audit log is not corrupted on attempted attack.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Fetch PR details
gh pr view 183 --repo vouchdev/vouch

echo "=== PR FILES CHANGED ==="
gh pr diff 183 --repo vouchdev/vouch

echo "=== RELATED ISSUE ==="
gh issue view 166 --repo vouchdev/vouch

Length of output: 10303

---

🏁 Script executed:

#!/bin/bash
# Get the full import_apply function
rg -n "def import_apply" --type py -A 80 src/vouch/bundle.py

echo "=== _safe_member_path ==="
rg -n "def _safe_member_path" --type py -A 20 src/vouch/bundle.py

echo "=== import_check full ==="
rg -n "def import_check" --type py -A 80 src/vouch/bundle.py

Length of output: 8643

---

---

@jonathanchang31 Great fix! The approach is sound — import_apply calls import_check first (line 575), so the _manifest_safety_issues check is enforced end-to-end, and _safe_member_path (now using _import_path_issue) guards the write path. Below is my full review.

---

Overall Assessment ✅

The fix correctly addresses the security issue in #166 with a nice defense-in-depth approach:

1. Path-level guard — _import_path_issue() blocks forbidden file names/prefixes in _safe_member_path().
2. Manifest-level guard — _manifest_safety_issues() rejects bundles whose safety flags claim they contain forbidden artifacts.

Both are exercised by import_apply (via its delegation to import_check), so neither can be bypassed.

---

Issues Found

1. 🐛 Minor — safety or {...} is falsy for {} in _write_malicious_bundle

tests/test_bundle.py (line ~113)

"safety": safety
or {"has_proposed": False, "has_state_db": False, "has_audit_log": False},

An empty dict {} is falsy in Python, so a caller passing safety={} would silently receive the default instead. The intent is clearly "use the default when None is passed". Prefer an explicit None guard:

"safety": safety
if safety is not None
else {"has_proposed": False, "has_state_db": False, "has_audit_log": False},

2. 🔍 Observation — Minor DRY gap between FORBIDDEN_SAFETY_FLAGS and _import_path_issue

FORBIDDEN_SAFETY_FLAGS maps flag names to their path descriptions:

FORBIDDEN_SAFETY_FLAGS = {
    "has_proposed": "proposed/",
    "has_state_db": "state.db",
    "has_audit_log": "audit.log.jsonl",
}

But _import_path_issue hard-codes the same strings ("audit.log.jsonl", "state.db", "proposed/") independently. If a path name ever changes, both places need updating. Not a bug today, but worth noting. You could extract constants or derive the forbidden path checks from FORBIDDEN_SAFETY_FLAGS, though the current readability may justify the duplication.

3. 🔍 Observation — audit.log.jsonl explicit check is redundant (by design)

audit.log.jsonl would also be caught by the final subdir not in EXPORT_SUBDIRS guard. The explicit check before it just produces a more precise error message ("forbidden path in bundle" vs "path outside importable bundle artifacts"). This is intentional and fine.

---

Test Coverage ✅

Both new parametrized tests are correct and comprehensive:

• test_import_rejects_non_committable_bundle_paths — covers audit.log.jsonl, state.db, state.db-wal, proposed/pending.yaml at the path level, and also verifies the audit log canary is preserved after a blocked import_apply.
• test_import_rejects_manifest_safety_flags — covers all three safety flags at the manifest level for both import_check and import_apply.

---

Summary: The only actionable change I'd suggest is the safety or {...} → safety if safety is not None else {...} fix. The rest is either a design choice or a minor observation. The security fix itself is solid. 🐇