feat(w1): external egress destination mgmt + SSRF guard + key encryption (O3, #17, #18) - #316
Conversation
…ion (wre#289, #17, #18) Security foundation for the O1/O2 external-RTMP/SRT restream legs. Adds /v1/egress/destinations CRUD (POST/GET/GET-by-id/DELETE, org-scoped, gateway- gated, mirrors whep-sources/#310 auth+ownership shape), a deny-by-default DNS-rebind-safe SSRF guard for user-supplied rtmp/rtmps/srt destination urls (ssrf-guard.ts), and AES-256-GCM encrypt-at-rest for streamKey/passphrase via WebCrypto (dest-key-crypto.ts) with a redacted response shape — plaintext/ ciphertext never leaves the module in a GET/list response or a log line. New surface ships INERT behind its own EGRESS_DEST_MGMT_ENABLED flag (sibling of INGRESS_ROUTER_ENABLED, not a reuse) pending the DEST_KEY_ENCRYPTION_KEY Doppler secret + a Jake-named ◆ arm. egress-arm.ts is untouched; O1/O2 consume resolveDestinationForArm(env, org, destId) next, re-running the SSRF check at connect time per its docstring. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 20 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_ce46350c-35f1-4e24-8236-47328f444623) |
![semgrep-code-wave-av[bot]](https://avatars.githubusercontent.com/u/211066493?s=60&v=4){kind=small}
| aesKey = await getAesKey(env); | ||
| } catch (e) { | ||
| // no-silent-failure: an unconfigured/malformed encryption key must 503, never silently store plaintext. | ||
| console.error(`egress-destinations create: encryption key unavailable org=${org}: ${(e as Error)?.message}`); |
There was a problem hiding this comment.
Semgrep identified a blocking 🔴 issue in your code:
Detected a logger that logs user input without properly neutralizing the output. The log message could contain characters like and and cause an attacker to forge log entries or include malicious content into the logs. Use proper input validation and/or output encoding to prevent log entries from being forged.
Dataflow graph
flowchart LR
classDef invis fill:white, stroke: none
classDef default fill:#e7f5ff, color:#1c7fd6, stroke: none
subgraph File0["<b>src/egress-destinations.ts</b>"]
direction LR
%% Source
subgraph Source
direction LR
v0["<a href=https://github.com/wave-av/wave-realtime-edge/blob/07196d6a962ccc93d77cafd7f26feb8e5fd94077/src/egress-destinations.ts#L299 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 299] request</a>"]
end
%% Intermediate
subgraph Traces0[Traces]
direction TB
v2["<a href=https://github.com/wave-av/wave-realtime-edge/blob/07196d6a962ccc93d77cafd7f26feb8e5fd94077/src/egress-destinations.ts#L299 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 299] org</a>"]
v3["<a href=https://github.com/wave-av/wave-realtime-edge/blob/07196d6a962ccc93d77cafd7f26feb8e5fd94077/src/egress-destinations.ts#L306 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 306] handleEgressDestinations</a>"]
v4["<a href=https://github.com/wave-av/wave-realtime-edge/blob/07196d6a962ccc93d77cafd7f26feb8e5fd94077/src/egress-destinations.ts#L252 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 252] org</a>"]
v5["<a href=https://github.com/wave-av/wave-realtime-edge/blob/07196d6a962ccc93d77cafd7f26feb8e5fd94077/src/egress-destinations.ts#L278 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 278] createDestination</a>"]
v6["<a href=https://github.com/wave-av/wave-realtime-edge/blob/07196d6a962ccc93d77cafd7f26feb8e5fd94077/src/egress-destinations.ts#L163 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 163] org</a>"]
v7["<a href=https://github.com/wave-av/wave-realtime-edge/blob/07196d6a962ccc93d77cafd7f26feb8e5fd94077/src/egress-destinations.ts#L188 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 188] `</a>"]
end
v2 --> v3
v3 --> v4
v4 --> v5
v5 --> v6
v6 --> v7
%% Sink
subgraph Sink
direction LR
v1["<a href=https://github.com/wave-av/wave-realtime-edge/blob/07196d6a962ccc93d77cafd7f26feb8e5fd94077/src/egress-destinations.ts#L188 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 188] console.error(`egress-destinations create: encryption key unavailable org=${org}: ${(e as Error)?.message}`)</a>"]
end
end
%% Class Assignment
Source:::invis
Sink:::invis
Traces0:::invis
File0:::invis
%% Connections
Source --> Traces0
Traces0 --> Sink
To resolve this comment:
🔧 No guidance has been designated for this issue. Fix according to your organization's approved methods.
💬 Ignore this finding
Reply with Semgrep commands to ignore this finding.
/fp <comment>for false positive/ar <comment>for acceptable risk/other <comment>for all other reasons
Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by console-log-express.
You can view more details about this finding in the Semgrep AppSec Platform.
…nical form CRITICAL: checkIpv6's mapped-address check only matched the dotted-quad textual form (`::ffff:a.b.c.d`) via regex. rtmp/srt are non-special URL schemes, so WHATWG URL canonicalizes a bracketed IPv4-mapped literal to hex-group form (`[::ffff:169.254.169.254]` -> `[::ffff:a9fe:a9fe]`) before ssrf-guard ever sees the hostname string — the regex never matched, the mapped check no-op'd, and the address was ALLOWED. Confirmed live path to cloud metadata via `srt://[::ffff:169.254.169.254]:5000`. Adds `expandIpv6Groups`/`mappedIpv4HexGroupOctets` to detect the `::ffff:0:0/96` prefix regardless of textual form, reconstruct the embedded 32 bits from the last two hex groups, and re-run `checkIpv4` on them. The existing dotted-quad path is preserved alongside it. Also strips a single trailing "." from the hostname before all hostname comparisons (metadata literal, `.local`, dotted-quad parse) — previously `169.254.169.254.` / `foo.local.` skipped those fast paths. test/ssrf-guard.test.ts: adds hex-group-form deny cases (metadata + loopback, both textual and hex-group), trailing-dot deny cases, and allow-path cases for legit public mapped addresses (both forms) to prove no over-blocking. The pre-existing `[::ffff:10.0.0.5]` case (line 27) was silently red-in- reality under this bug (rtmp is ALSO a non-special scheme) and now passes for real. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ress dest access (CI reds)
- content-policy: annotate the 100.64.0.0/10 CGNAT literal in ssrf-guard.ts and its
test with `# guard:allow` (legitimate denylist target, not a leaked fleet address).
- egress-destinations.ts: forward KV record was keyed by CALLER-claimed org + id
(`egress-dest:{org}:{id}`), so a foreign-org GET/DELETE simply missed (404 / idempotent
200) instead of ever reaching the `record.org !== org` ownership check — the 403 branch
was dead code, and DELETE on a foreign-org id was a silent no-op masquerading as success.
Re-keyed the forward record by id ALONE (mirrors whep-sources.ts's
`STREAM_INPUT_ORG_PREFIX + uid` forward-key pattern) so ownership is checked BEFORE any
read/delete decision, regardless of which org the caller claims.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The per-org index only ever holds this org's ids, but re-check record.org===org after the id-only lookup so a corrupted/mis-written index can never leak another org's destination — matches getDestination/deleteDestination's explicit guard. Addresses the background security review's authorization finding on the list path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
What / why
W1 slice-2a: the security foundation the external-RTMP/SRT restream legs (O1/O2) will consume next. Adds:
src/egress-destinations.ts.POST/GET/GET-{id}/DELETE /v1/egress/destinations, gateway-gated +x-wave-org(same chokepoint every other paid route uses), org-scoped KV (forwardegress-dest:{org}:{id}+ reverseegress-dest-index:{org}, mirroring thestream-input-org:/org-stream-inputs:shapewhep-sources.tsalready uses).DELETEmirrors feat(w1): DELETE /v1/whep/sources/{uid} teardown route #310 exactly: absent → idempotent 200, foreign org → 403.src/ssrf-guard.ts. Deny-by-default validation of every create-time destinationurl, DNS-rebind-safe (resolves the hostname — via injectable resolver, defaulting to Cloudflare DoH since Workers have no raw DNS socket — and checks the RESOLVED ip, not the string).src/dest-key-crypto.ts. AES-256-GCM via WebCrypto (same primitive family already used for HMAC in this repo —event-emitter.ts/rtms-auth.ts/stream-bridge.ts). Only ciphertext+iv ever reach KV; a fresh random IV per encrypt call; decrypt only at the (future) arm call site.SSRF deny matrix implemented
Reject: loopback (127/8, ::1), RFC1918 (10/8, 172.16/12, 192.168/16), link-local incl. metadata (169.254/16, 169.254.169.254,
fe80::/10), CGNAT (100.64/10), ULA (fc00::/7),0/8, broadcast/reserved, multicast (v4224+/v6ff00::/8), IPv4-mapped IPv6 wrapping a private v4,.local/metadata.google.internalhostnames (pre-resolution), non-allowlisted scheme (onlyrtmp/rtmps/srt), non-allowlisted port (rtmp:1935, rtmps:443, srt: configurable range), zero-IP resolution, and ANY resolver exception — all fail CLOSED, never open.Crypto scheme
AES-256-GCM, key from
env.DEST_KEY_ENCRYPTION_KEY(base64 32-byte). Redaction:redactDestination()replacesstreamKey/passphrasewith a[redacted]marker in every response; raw plaintext/ciphertext never logged (grepped everyconsole.*call site in the new files).Wiring
resolveDestinationForArm(env, org, destId)exported for O1/O2 — decrypts stored key material but does NOT re-run SSRF (documented on the function: the caller must re-validate immediately before dialing, since DNS can rebind between create and connect).egress-arm.tsis untouched.Ships INERT behind its own
EGRESS_DEST_MGMT_ENABLEDflag (a sibling ofINGRESS_ROUTER_ENABLED, not a reuse — new CRUD+crypto surface gets its own Jake-named ◆ arm) pending theDEST_KEY_ENCRYPTION_KEYDoppler ◆ secret.Part of W1 slice-2 (external RTMP/SRT restream foundation; O1/O2 consume this).
Test plan
test/ssrf-guard.test.ts— full deny matrix (14 deny cases) + allow path (public v4, rtmps:443, resolved-hostname, srt port range) + rebind-safety + fail-closed-on-throw + zero-IPtest/dest-key-crypto.test.ts— key validation (absent/bad-base64/wrong-length throws), round-trip, fresh-IV-per-call, tamper detection, wrong-key rejectiontest/egress-destinations.test.ts— SSRF-reject-persists-nothing, create+redact+ciphertext-in-KV, 503 on unconfigured key, org-scoped list, 404/403 on get, feat(w1): DELETE /v1/whep/sources/{uid} teardown route #310-mirrored delete (idempotent/403/cleanup),resolveDestinationForArmdecrypt + null cases, flag-gating fall-throughnpm test— gated: private registry (npm.pkg.github.com) 401s in this sandbox, nonode_modules. Typechecked the 3 new files against a DOM-lib substitute tsconfig (workers-types also registry-gated) — clean, no errors.🤖 Generated with Claude Code
Note
High Risk
Introduces SSRF validation, symmetric encryption of stream credentials, and a new gateway-gated CRUD surface; misconfiguration or future arm-path bugs could expose secrets or allow abusive outbound targets, though the feature ships disabled by default.
Overview
Adds the W1 O3 control-plane foundation for org-owned external RTMP/SRT restream targets, ahead of the O1/O2 dial path.
/v1/egress/destinations(POST create, GET list, GET by id, DELETE) is wired throughroute-dispatchviamaybeHandleEgressDestinations, using the same gateway gate +x-wave-orgpattern aswhep-sources. Records live inRT_MEETING_ORGKV (egress-dest:{org}:{id}+ org index). Create runsvalidateDestinationUrl(ssrf-guard.ts) before any write;streamKey/passphraseare encrypted with AES-256-GCM (dest-key-crypto.ts,DEST_KEY_ENCRYPTION_KEY) and API responses always redact key material.resolveDestinationForArmis exported for future arm code to decrypt credentials (SSRF re-check left to the caller at dial time).Everything stays inert until
EGRESS_DEST_MGMT_ENABLEDis armed (wrangler.tomldefault"0"). Vitest covers SSRF deny/allow matrix, crypto round-trip/tamper, and CRUD org isolation / idempotent delete.Reviewed by Cursor Bugbot for commit 07196d6. Configure here.
Summary by cubic
Adds org‑scoped external egress destination CRUD for RTMP/SRT with a deny‑by‑default SSRF guard and AES‑256‑GCM encryption of
streamKey/passphraseat rest. Ownership is enforced by id‑keyed records with a defense‑in‑depth org check on list to prevent cross‑org leakage; feature ships inert behindEGRESS_DEST_MGMT_ENABLED(wre#289 O3, #17, #18).New Features
/v1/egress/destinations(POST, GET, GET {id}, DELETE): gateway‑gated,x-wave-orgscoped; forward record keyed by id with an org index; list re‑checksrecord.org===orgto avoid leaks (feat(w1): external egress destination mgmt + SSRF guard + key encryption (O3, #17, #18) #316); idempotent delete; strict 403 on foreign‑org GET/DELETE.rtmp|rtmps|srt; rtmp:1935/rtmps:443 and bounded SRT ports; rejects loopback/private/link‑local/CGNAT/ULA/multicast,.localand metadata; blocks IPv4‑mapped IPv6 (hex‑group and dotted forms) and trailing‑dot hostnames; fail‑closed.resolveDestinationForArm(env, org, id)decrypts for arm callers (must re‑run SSRF before dialing).Migration
EGRESS_DEST_MGMT_ENABLED=1to enable the routes.DEST_KEY_ENCRYPTION_KEYas a base64‑encoded 32‑byte key; without it, creates 503 and nothing is stored.Written for commit 4113997. Summary will update on new commits.