Harden stem-id sanitizer against '..' and cap the extracted mix size - #4
Open
carochacs wants to merge 4 commits into
Open
Harden stem-id sanitizer against '..' and cap the extracted mix size#4carochacs wants to merge 4 commits into
carochacs wants to merge 4 commits into
Conversation
Two LOW hardening items found during a security audit: - _sanitize()'s character-class allowlist includes '.', so a literal ".." survives it untouched (only the OTHER characters in a run, like '/', get replaced). Not reachable today — the id is always derived from the separator engine's own output filename, never attacker/HTTP-supplied input — but a stray ".." reaching a later path-join would be a traversal primitive if that ever changes. Collapse any run of 2+ dots to a single underscore as defense-in-depth. - No sanity cap on the mix track extracted from a pak before handing it to any split engine (remote upload, or local audio-separator/demucs). The mix comes from the user's own already-owned library pak, not an untrusted network caller, so this isn't attacker-reachable — it's a defense-in-depth bound against a malformed/corrupt pak. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018J1NvPtPQZd3cEt6aAbGzG
📝 WalkthroughWalkthrough
ChangesMix processing safeguards
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant split_pak
participant pak_io.extract_mix
participant separation_engine
split_pak->>pak_io.extract_mix: request extraction with 4 GB max_bytes
pak_io.extract_mix-->>split_pak: return mix or MixTooLargeError
split_pak->>separation_engine: start separation for accepted mix
Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (19 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.
Inline comments:
In `@pak_io.py`:
- Around line 97-114: Update extract_mix to accept the maximum allowed size and
enforce it during both ZIP and directory-pak extraction. Validate regular source
files before copying, stream/count bytes without writing beyond the limit, and
delete any partial output before raising when the cap is exceeded; update
split_pak to pass the size limit into extract_mix.
In `@split_stems.py`:
- Around line 66-73: The comment above _sanitize incorrectly claims current
callers do not derive input from attacker/HTTP data. Update only that
trust-boundary explanation to acknowledge that _run_remote sanitizes name from
the split-server HTTP response before writing the stem file, while retaining the
existing dot-run sanitization.
In `@tests/test_mix_size_cap.py`:
- Around line 50-63: Update test_mix_under_cap_proceeds_past_the_check to mock
ss._run_remote with a known sentinel RuntimeError, then assert that exact
exception is raised and _run_remote is called once. Remove the live
remote-server dependency while preserving the assertion that the exception does
not contain “sanity limit”.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 685542e5-3731-4eb8-861a-61125902a9d6
📒 Files selected for processing (5)
pak_io.pysplit_stems.pytests/test_mix_size_cap.pytests/test_pak_io_extract_mix.pytests/test_stem_ids.py
Greptile review on this PR, both valid: 1. The fixed 1GB threshold rejected legitimately large mixes (a long or high-res-master full mix can plausibly exceed that). Raised to 4GB — generous for a ~45min stereo mix at 32-bit float/192kHz (~4GB) while still bounding a runaway decompression. 2. More importantly: split_pak()'s cap check ran AFTER pak_io.extract_mix() had already fully decompressed and written the mix to disk via copyfileobj — so a real decompression bomb would still consume all that disk/CPU before being rejected, defeating the point of a size cap entirely. Moved enforcement into extract_mix() itself via a new `max_bytes` parameter (default None, fully backward compatible for any other caller): the zip-form path checks the archive's own declared info.file_size up front — reading zip central-directory metadata, no decompression needed — before ever opening the entry, and also tracks the running total during the chunked streaming copy as defense in depth. The directory-form path checks stat().st_size before copying. Either way rejection happens before/during the write, and any partial output is removed. New MixTooLargeError (a RuntimeError subclass, so existing broad exception handling still works). Verified experimentally: a 50MB-of-zeros bomb (~51KB compressed) against a 1KB cap is now rejected in ~2ms, based purely on declared size — never decompressed, no file left in the output directory. Rewrote tests/test_mix_size_cap.py to test what it actually should now: that split_pak() wires _MAX_MIX_BYTES through to extract_mix() as max_bytes (the enforcement itself moved to pak_io and is tested in tests/test_pak_io_extract_mix.py, which gained 4 new cases: the real bomb-shape rejection, an under-cap positive case, the directory-form equivalent, and a no-cap-passed backward-compatibility check). All new tests confirmed to fail against the pre-fix code and pass after. Verified: full suite 147 passed (142 baseline + 5 net new), zero regressions. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018J1NvPtPQZd3cEt6aAbGzG
There was a problem hiding this comment.
carochacs has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
…m tests
Two follow-up review comments (CodeRabbit) on this PR:
1. _sanitize()'s comment claimed "nothing currently derives this from
attacker/HTTP input," which is false: _run_remote's `name` comes
directly from the split server's JSON response ("stems" dict keys),
a few lines above the exact comment that already documents that
response as attacker-choosable via a malicious/compromised server.
The sanitizer's actual safety property is unaffected (result_dir /
f"{_sanitize(name)}{ext}" can never produce a literal ".." since an
extension is always appended, and '/' is stripped regardless of the
dot-collapse fix) — this was a documentation-accuracy fix, not a
behavior change.
2. Two tests let split_pak() reach _run_remote for real against
"http://split.invalid", relying on DNS failing fast and asserting
only a bare `Exception` — flaky under network policies where that
doesn't fail fast, and too broad to distinguish "cap correctly not
tripped" from an unrelated failure. Mocked _run_remote directly
with a sentinel RuntimeError instead: no live I/O, and the assertion
now checks for that exact sentinel plus a single call, which is
only reachable if the cap check already passed.
Verified: full suite still 147 passed, zero regressions; the two
fixed tests now run in ~0.02s instead of depending on network timing.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018J1NvPtPQZd3cEt6aAbGzG
There was a problem hiding this comment.
carochacs has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pak_io.py (1)
146-151: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winKeep the directory-form copy bounded after the metadata check.
The
stat()check occurs beforeshutil.copyfilereopens the source at Line 152. A source file can grow or be replaced between these operations. The copy can then exceedmax_bytesand exhaust temporary storage.Use the same bounded read/write loop for directory-form extraction. Delete
outif the running total exceeds the limit. Add a regression test that changes the source after the metadata check.🤖 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 `@pak_io.py` around lines 146 - 151, The directory-form extraction path around the metadata size check and subsequent shutil.copyfile must use a bounded read/write loop that tracks copied bytes, deletes out when the running total exceeds max_bytes, and raises MixTooLargeError. Add a regression test that modifies or replaces the source after the stat check to verify oversized copies are stopped and temporary output is removed.
🧹 Nitpick comments (1)
tests/test_pak_io_extract_mix.py (1)
72-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that declared-size rejection does not open the ZIP member.
The current test verifies only the final exception and output state. An implementation could inflate the member before raising and still pass. Patch
zipfile.ZipFile.opento fail in this test.extract_mixmust raiseMixTooLargeErrorbefore it opens the member.Proposed test change
- with self.assertRaises(pak_io.MixTooLargeError): - pak_io.extract_mix( - pak, {"original_audio": "stems/full.ogg"}, out_dir, max_bytes=1024, - ) + with mock.patch.object( + zipfile.ZipFile, + "open", + side_effect=AssertionError("oversized member must not be opened"), + ): + with self.assertRaises(pak_io.MixTooLargeError): + pak_io.extract_mix( + pak, {"original_audio": "stems/full.ogg"}, out_dir, max_bytes=1024, + )🤖 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_pak_io_extract_mix.py` around lines 72 - 75, Update the test around pak_io.extract_mix to patch zipfile.ZipFile.open so any attempt to open the ZIP member fails, while retaining the MixTooLargeError assertion. Verify the declared-size limit is rejected before the member is opened and preserve the existing output-state checks.
🤖 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.
Inline comments:
In `@pak_io.py`:
- Around line 117-133: The broad OSError handler around extraction incorrectly
converts destination I/O failures on out, including open, write, close, or
unlink operations, into FileNotFoundError. Narrow the exception handling in the
relevant pak extraction function so only archive opening and member lookup
errors receive the missing-mix translation, while destination I/O errors
propagate unchanged and cleanup does not leave a partial file.
---
Duplicate comments:
In `@pak_io.py`:
- Around line 146-151: The directory-form extraction path around the metadata
size check and subsequent shutil.copyfile must use a bounded read/write loop
that tracks copied bytes, deletes out when the running total exceeds max_bytes,
and raises MixTooLargeError. Add a regression test that modifies or replaces the
source after the stat check to verify oversized copies are stopped and temporary
output is removed.
---
Nitpick comments:
In `@tests/test_pak_io_extract_mix.py`:
- Around line 72-75: Update the test around pak_io.extract_mix to patch
zipfile.ZipFile.open so any attempt to open the ZIP member fails, while
retaining the MixTooLargeError assertion. Verify the declared-size limit is
rejected before the member is opened and preserve the existing output-state
checks.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bb42187b-b5f0-4f37-95d6-4ef1837fac8a
📒 Files selected for processing (4)
pak_io.pysplit_stems.pytests/test_mix_size_cap.pytests/test_pak_io_extract_mix.py
Comment on lines
+117
to
+133
| with zf.open(info, "r") as src, out.open("wb") as dst: | ||
| written = 0 | ||
| while True: | ||
| chunk = src.read(1024 * 1024) | ||
| if not chunk: | ||
| break | ||
| written += len(chunk) | ||
| if max_bytes is not None and written > max_bytes: | ||
| dst.close() | ||
| out.unlink(missing_ok=True) | ||
| raise MixTooLargeError( | ||
| f"mix track exceeded the {max_bytes} byte sanity limit " | ||
| f"while extracting (declared size did not match actual " | ||
| f"decompressed size) — refusing to extract what looks " | ||
| f"like a corrupt pak" | ||
| ) | ||
| dst.write(chunk) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Do not translate destination I/O errors to FileNotFoundError.
Lines 117-133 execute inside the except OSError handler below. If opening, writing, closing, or deleting out fails, the function reports that the mix is missing. It can also leave a partial file. Restrict that translation to archive and member lookup errors. Let destination I/O errors propagate with their actual cause.
🤖 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 `@pak_io.py` around lines 117 - 133, The broad OSError handler around
extraction incorrectly converts destination I/O failures on out, including open,
write, close, or unlink operations, into FileNotFoundError. Narrow the exception
handling in the relevant pak extraction function so only archive opening and
member lookup errors receive the missing-mix translation, while destination I/O
errors propagate unchanged and cleanup does not leave a partial file.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Two LOW hardening items found during a security audit (follow-up to #2, merged):
1. Stem-id sanitizer allowed literal
..through._sanitize()'s character-class allowlist includes., so a literal".."survives it untouched (only the other characters in a run, like/, get replaced). Not reachable today — the id is always derived from the separator engine's own output filename, never attacker/HTTP-supplied input — but a stray".."reaching a later path-join would be a traversal primitive if that ever changes. Fixed by collapsing any run of 2+ dots to a single underscore as defense-in-depth.2. No sanity cap on the extracted mix track. Nothing bounded the size of the mix audio extracted from a pak before handing it to any split engine (remote upload, or local audio-separator/demucs). The mix comes from the user's own already-owned library pak, not an untrusted network caller, so this isn't attacker-reachable — added a 1GB sanity cap as defense-in-depth against a malformed/corrupt pak.
Added regression tests: 3 for the sanitizer (
tests/test_stem_ids.py) and 2 for the mix-size cap (tests/test_mix_size_cap.py, using a fakepak_iomodule injected viasys.modulessince the real one pulls in the core sloppak lib not importable stand-alone here).Verified: 140 passed (135 baseline + 5 new), zero regressions (4 pre-existing environment-only collection errors — missing
httpx2— unchanged before/after, excluded from this count the same way on both runs).Checklist
Generated by Claude Code
Greptile Summary
The PR hardens fallback stem IDs by collapsing repeated dots and changes mix extraction to stream archive members before applying a 1 GiB sanity limit.
Confidence Score: 4/5
The PR does not yet appear safe to merge because the mix-size hardening still rejects valid large audio and does not bound resource use while extracting it.
The current HEAD retains both previously reported failures: the fixed threshold rejects every mix above 1 GiB regardless of validity, while copyfileobj can fill temporary storage with the entire decompressed member before that threshold is evaluated.
Files Needing Attention: pak_io.py and split_stems.py
Important Files Changed
Reviews (2): Last reviewed commit: "Stream pak mix extraction" | Re-trigger Greptile