fix(analysis): bound temporal decode size and duration - #121
Conversation
|
Warning Review limit reached
More reviews will be available in 31 minutes and 1 second. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. Comment |
![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 the current code and only fix it if needed.
Inline comments:
In `@services/analysis-engine/src/bandscope_analysis/temporal/analyzer.py`:
- Around line 44-57: The code does a TOCTOU by calling path.stat() then later
calling librosa.load(path_str); instead open the file once and use the same file
handle to both check size and decode to prevent replacement: use Path.open() to
get a file object, obtain size via fileobj.seek(0,2)/tell() or
os.fstat(fileobj.fileno()) and compare with MAX_AUDIO_FILE_BYTES, then pass that
same file object to librosa.load (it accepts a file-like object) with the
existing TARGET_SR and MAX_ANALYSIS_DURATION_SECONDS; replace usages of
path.stat() and path_str with the single file handle so the check and load occur
on the same descriptor.
In `@services/analysis-engine/tests/test_temporal.py`:
- Line 107: The failing Ruff E501 is caused by an overlong line where
monkeypatch.setattr is called with an inline lambda for librosa.beat.beat_track;
break the long statement into multiple shorter lines (under 100 chars) by either
defining a small helper function (e.g., def fake_beat_track(y, sr): return
np.array([120.0]), np.array([0])) and passing that to monkeypatch.setattr, or
assign the lambda to a named variable before calling monkeypatch.setattr; update
the call in test_temporal.py to use the helper or named variable to satisfy the
line-length limit while keeping the same behavior.
- Line 113: Replace the magic number assertion in test_temporal.py—assert
captured_kwargs["duration"] == 15 * 60—with a direct comparison to the
implementation constant from the module under test: import the appropriate
constant (the duration/timeout constant used by the code that populates
captured_kwargs, e.g., DURATION_SECONDS or DEFAULT_TIMEOUT) and assert
captured_kwargs["duration"] == <IMPORTED_CONSTANT>; remove the literal 15 * 60
so the test tracks the implementation constant.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro
Run ID: d3ecdc96-7696-455b-beaf-681dcb290b14
📒 Files selected for processing (2)
services/analysis-engine/src/bandscope_analysis/temporal/analyzer.pyservices/analysis-engine/tests/test_temporal.py
|
@coderabbitai resolve |
✅ Action performedComments resolved and changes approved. |
1 participant
Motivation
librosa.loadwithout duration or file-size guards which allowed decoding arbitrarily large local audio files into memory and created a local DoS vector when the CLI invoked analysis early in the flow.Description
MAX_AUDIO_FILE_BYTES = 100 * 1024 * 1024andMAX_ANALYSIS_DURATION_SECONDS = 15 * 60and validatePath.stat().st_sizeto reject oversized files before any decode.duration=MAX_ANALYSIS_DURATION_SECONDSintolibrosa.loadand normalizeaudio_pathto aPathfor reliable stat checks.services/analysis-engine/tests/test_temporal.pyto assert oversized files are rejected beforelibrosa.loadis called and that the duration kwarg is passed intolibrosa.load.ValueErrorso existing CLI exception handling preserves safe failure, no new sensitive logging is added, and test points cover the protections.Testing
uv run --project services/analysis-engine pytest services/analysis-engine/tests/test_temporal.py services/analysis-engine/tests/test_cli.pywhich executed the temporal and CLI tests.21 passed(with harmless deprecation warnings) and no regressions observed.Codex Task