fix(media): restrict FFmpeg/FFprobe input protocols - #562
Conversation
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
현재 exact head
수리 기준: (a) 세 input call site의 최소 protocol allowlist를 실제 I/O contract로 검증, (b) 근거: FFmpeg Protocols 문서는 |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
OpenCode reviewed the current-head product diff. Coverage is a separate gate.
Changed files
.jules/sentinel.md— repository behavioraudio_library.py— Python module behaviortests/test_audio_library.py— regression suite
Changed behavior
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Repository file: sentinel.md"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Repository file: sentinel.md"]
R1 --> V1["required checks"]
Evidence --> S2["Python: audio_library.py"]
S2 --> I2["Python module behavior"]
I2 --> R2["Review risk: Python: audio_library.py"]
R2 --> V2["pytest plus coverage"]
Evidence --> S3["Test: test_audio_library.py"]
S3 --> I3["regression suite"]
I3 --> R3["Review risk: Test: test_audio_library.py"]
R3 --> V3["targeted test run"]
Findings
No source-backed product finding is synthesized from the coverage gate. A coverage miss belongs in the status comment.
- Head SHA:
e9a69c6f4d3f0b105eb3346391c1160770927243 - Workflow run: 34628506363
- Workflow attempt: 1
- Coverage gate:
failure
Review outcome
Coverage is a gate, not the review. This body reviews the changed product files.
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Repository file: sentinel.md"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Repository file: sentinel.md"]
R1 --> V1["required checks"]
Evidence --> S2["Python: audio_library.py"]
S2 --> I2["Python module behavior"]
I2 --> R2["Review risk: Python: audio_library.py"]
R2 --> V2["pytest plus coverage"]
Evidence --> S3["Test: test_audio_library.py"]
S3 --> I3["regression suite"]
I3 --> R3["Review risk: Test: test_audio_library.py"]
R3 --> V3["targeted test run"]
OpenCode Review Overview
Coverage evidence did not pass, so approval is blocked. The formal pull-request review is the source-backed diff review, not this status comment. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
@jules 현재 exact head
공식 FFmpeg 문서 기준으로 |
|
Fresh current-head review of @jules ordinary-forward repair only:
After the dependency blobs are restored and focused regression exists, provide fresh compare and exact-head CI/Security/SAST/CodeQL. Draft remains correct. |
seonghobae
changed the title
말씀하신 피드백을 반영하여 |
seonghobae
added
bug
ChatGPT Codex Connector
2 participants
Finding
The approved FFmpeg/FFprobe subprocesses parse media that can originate from staged product inputs, but three
audio_library.pyinput paths did not set FFmpeg's input-protocol_whitelist. FFmpeg enables supported protocols by default; its official protocol documentation definesprotocol_whitelistas a comma-separated allow-list for input protocols. This is therefore a real network-protocol admission gap at the media parser boundary.Current evidence does not establish Local File Inclusion remediation:
fileremains intentionally allowed. It also does not by itself prove a remotely exploitable SSRF path for every media format. The bounded claim is that untrusted media parsing must not be permitted to open network protocols such as HTTP/TCP/UDP through these three subprocess builders.Primary reference: FFmpeg Protocols Documentation,
protocol_whitelistinput option: https://ffmpeg.org/ffmpeg-protocols.htmlCurrent exact repair
main@47c6fd27de13b0da37a7db64697b8699419093516e371b9c9e6e3f72b9be6cb5949d77dbed24e19faudio_duration_seconds: FFprobe receives-protocol_whitelist file,crypto,dataimmediately before its input URL/path;decode_audio_for_mlx: FFmpeg receives the same whitelist immediately before-i, including the existing/dev/fd/<n>descriptor-bound path through thefileprotocol;detect_silence_intervals: FFmpeg receives the same whitelist immediately before-i;tests/test_audio_library.pypins the MLX decode command shape;tests/test_ffmpeg_protocol_whitelist_contract.pyexecutes the duration and silence builders with mocked approved binaries, verifies the exact{file, crypto, data}allow-list, verifies network protocols are absent, and checks placement immediately before the relevant input.FFmpeg documents
cryptoas a wrapper protocol (crypto:URL/crypto+URL). The explicit whitelist remains authoritative for allowed protocol names; HTTP/HTTPS/TCP/UDP and other network protocols are not admitted by this command contract. This PR does not claim thatfileprotects against local-file access.Intervening-delta repair
The generated branch also changed
httpx2from 2.5.0 to 2.6.0. That was unrelated to the media parser boundary and conflicted with canonical dependency owner #558, which removes the unused vulnerablehttpx2contract entirely. Ordinary descendantc6d35a65b8589c7cc908534781e49b320202885arestoredpyproject.toml,requirements.txt, andrequirements-lock.txtto the protected-base blobs; no force/rebase was used. This PR no longer owns dependency resolution.The existing Sentinel note was corrected rather than expanded: it now says the whitelist narrows network-protocol/SSRF exposure and explicitly records that allowing
filedoes not remediate LFI.RED → GREEN / verification boundary
Protected
mainis the executable predecessor for the missing-whitelist RED obligation. This lane did not manufacture a separate hosted RED commit after production had already been generated, so no hosted RED is claimed. The current tests make removal or broadening of the whitelist fail at the command-construction boundary.Fresh workflows were generated for exact head
6e371b9c...; at the latest read CI/fuzz/CodeQL were queued and Security/SAST were queued or in progress. Predecessor results do not transfer. Keep Draft until this unchanged head has terminal applicable repository/security gates and qualifying independent current-head review.A stronger acceptance step remains desirable before promotion: run a right-cleared hostile media fixture that attempts a network protocol and prove the approved FFmpeg/FFprobe process rejects it without outbound access while ordinary local media still succeeds. Do not substitute a synthetic timing/result claim for that integration evidence.
No force push, destructive rebase, self-approval, dependency-owner duplication, gate weakening, scanner suppression, or source-neutral retrigger.