Skip to content

Prevent SSRF and stalled opens in WebRTC MJPEG input - #2958

Merged
PawelPeczek-Roboflow merged 2 commits into
mainfrom
fix/webrtc-mjpeg-url-security
Sep 11, 2026
Merged

Prevent SSRF and stalled opens in WebRTC MJPEG input#2958
PawelPeczek-Roboflow merged 2 commits into
mainfrom
fix/webrtc-mjpeg-url-security

Conversation

@grzegorz-roboflow

Copy link
Copy Markdown
Collaborator

What does this PR do?

Caller-controlled mjpeg_url values could access local files and internal services
or block worker initialization.

Route MJPEG through the existing protected HTTP transport, validate and pin
destination IPs on every redirect, restrict decoding to MJPEG, and add timeouts
and connection cleanup.

Private cameras now require WEBRTC_MJPEG_ALLOW_NON_GLOBAL_ADDRESSES=True on the
server.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)

Testing

106 tests passed using an isolated transport harness

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code where necessary, particularly in hard-to-understand areas
  • My changes generate no new warnings or errors
  • I have updated the documentation accordingly (if applicable)

Additional Context

N/A

Co-authored-by: Codex <noreply@openai.com>
@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

👋 Thanks for the pull request! Here is how automated Claude review works here, so you spend credits (and reviewer time) wisely.

🚦 This PR is marked Ready for review, so automated Claude review will run — and every pass spends real credits.

Warning

💸 The Claude reviewer bills in credits, not vibes

Automated review spins up a real agent that reads real code and spends real credits on every pass. It is glad to help — but it is not a rubber duck, a linter you poke in a loop, or a substitute for reading the contributing guide. Treat it like an expensive senior reviewer whose time you booked, and show up prepared.

Draft when unsure, Ready when you mean it:

  • 🌱 Not sure the PR is in good shape yet? Keep it (or set it back) as a draft — drafts pause review, so you can push and iterate without burning credits on a moving target.
  • 💪 Feel strong about the contents? Mark it Ready for review and the reviewer will take a look.

However you get there, arrive prepared:

  • 🧱 Bring a SOLID, thorough PR. Point your local agent at our skills/ to tune it to our guidelines first — or, if you are one of those fabled carbon-based contributors, read them yourself. A half-baked diff costs exactly the same to review as a finished one.
  • Resolve every comment before you re-request review. Re-requesting with threads still open means paying twice for the same conversation.
  • 🔁 Do not use CI review as an inner loop for a local agent. The reviewer is not a step-by-step debugger — do the unfolding locally and arrive with the answer, not the search.
  • 🙋 If something looks off, ask a human. One question to a maintainer is cheaper and faster than three rounds of agent re-review chasing a misread.

Reviews are not free. A draft costs nothing to review; a Ready PR is a promise that it is worth reviewing.

  • Prefer to skip automated review entirely? Add the skip-claude-review label.

@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

🤖 Claude review started at commit 400a26ed493ae03a71b42686fde6334d99b526e1.

New commits are not auto-reviewed. Add the claude-review label to request a re-review — the label is consumed when the review starts, so just add it again next time.

@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Skills: review-topic-input-boundary-security, review-core-infra, review-topic-secure-gateway-url-wrapping, review-topic-test-hygiene.

No dedicated surface skill covers inference/core/interfaces/webrtc_worker/** or examples/** — generic review plus topic skills only.

Review summary — no blocking findings. MJPEG opening now routes through SSRFProtectedHTTPAdapter (resolve + validate + pin the destination IP so DNS rebinding can't swap the target, HTTP(S)-only with backslash/host sanity gate, per-hop redirect revalidation capped at 5, proxies={} to bypass env proxies, socket + open-deadline timeouts, and protocol_whitelist="" so FFmpeg can't fan out to playlist sub-resources). Errors are reduced to a sanitized reference + exception type, so credentialed request targets don't leak. Resource lifetime is handed off to the on("ended") handler via ExitStack.pop_all(), keeping the response/adapter alive for the track and closing them on EOF/shutdown. The address_is_global change (and not parsed.is_multicast) is strictly more restrictive and affects the shared image-URL path only in the safe direction.

Verified: open_mjpeg_player is the sole server-side MJPEG open path (webrtc.py:1135); the SDK only packages mjpeg_url. Tests in tests/inference/unit_tests/core/utils/test_mjpeg.py exercise the changed paths and assert real behavior (no-connect on rejection, URLAddressNotAllowedError on non-global hosts, DNS-pin assertion that a second resolution can't retarget, redirect cap, stall/trickle timeouts, playlist non-fetch, credential redaction); aiortc/av are core deps so these run in the standard unit-test job.

Note for maintainers: this is a user-visible behavior change for WebRTC MJPEG — existing private/LAN cameras now require WEBRTC_MJPEG_ALLOW_NON_GLOBAL_ADDRESSES=True. Intended and documented in examples/webrtc_sdk/mjpeg_basic.py. Not in a versioned subsystem (inference-models / Execution Engine), so no changelog entry is required.

Commands that informed this review: gh pr diff, gh api .../{issues,pulls}/2958/comments and .../reviews, and reads of url_input.py, mjpeg.py, webrtc.py, and the test file.

Reviewed at HEAD: 400a26e

@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

😎 PR passes the vibe-check and trust-me-bro verification.

@PawelPeczek-Roboflow
PawelPeczek-Roboflow merged commit 3e69d06 into main Sep 11, 2026
7 checks passed
@PawelPeczek-Roboflow
PawelPeczek-Roboflow deleted the fix/webrtc-mjpeg-url-security branch September 11, 2026 06:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants