fix!: reject a catalog whose broadcast reference escapes the root - #2630
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fee5945212
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
WalkthroughThe change adds checked path resolution in JavaScript and Rust. References that escape the catalog root now remain unresolved instead of clamping to the catalog path. Source requests and export-source constructors represent unresolved references as optional results. Broadcast consumers, codec exporters, container exporters, and RTC track selection filter or skip those renditions. Direct resolution and subscription report 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches✨ Simplify code
Thanks 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 |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@doc/concept/layer/hang.md`:
- Line 83: Update the root-boundary rule in the reference-resolution
documentation to state that references resolving exactly to the root, such as
../.. from a/b, name no broadcast, as do references that walk above the root;
ensure the wording does not imply only above-root walks are ignored.
In `@rs/moq-mux/src/container/source.rs`:
- Around line 91-93: Update update_catalog in
rs/moq-mux/src/container/ts/export.rs to exclude renditions whose
ExportSource::for_video or ExportSource::for_audio result is Ok(None) before
computing active renditions, PIDs, or the MPEG-TS header layout. Preserve valid
renditions and ensure later catalog snapshots do not perform HeaderAddedTrack
checks for skipped escaping references. Add a regression test covering one valid
rendition, one escaping rendition, and a subsequent catalog snapshot.
In `@rs/moq-mux/src/container/ts/export.rs`:
- Around line 427-429: Filter renditions with
self.source.resolvable(config.broadcast.as_ref()) at the start of each
update_catalog path before processing: in rs/moq-mux/src/container/ts/export.rs
lines 427-429 filter video before building active or assigning PIDs, and lines
446-448 filter audio likewise; in rs/moq-mux/src/container/flv/export.rs lines
352-354 filter video before bind_video, and lines 382-384 filter audio before
bind_audio. Add regression tests covering an escaping rendition with an
FLV-incompatible container and repeated MPEG-TS catalog snapshots after PSI
creation, documenting the reproduction and ensuring both fail without the fix.
In `@rs/moq-mux/src/source.rs`:
- Around line 233-264: Extend the escaping-reference loop in
escaping_reference_is_ignored to call Source::resolve for each invalid reference
and assert it returns Error::EscapingBroadcast. Keep the existing resolvable,
request, and subscribe_track assertions unchanged, and verify other resolve
results fail the test.
🪄 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: b3339604-bd80-447e-b0fa-7496435c6200
📒 Files selected for processing (19)
doc/concept/layer/hang.mdjs/hang/src/catalog/path.tsjs/net/src/path.test.tsjs/net/src/path.tsjs/watch/src/broadcast.test.tsjs/watch/src/broadcast.tsrs/hang/src/catalog/audio/mod.rsrs/hang/src/catalog/video/mod.rsrs/moq-mux/src/codec/h264/export.rsrs/moq-mux/src/codec/h265/export.rsrs/moq-mux/src/container/flv/export.rsrs/moq-mux/src/container/fmp4/export.rsrs/moq-mux/src/container/mkv/export.rsrs/moq-mux/src/container/source.rsrs/moq-mux/src/container/ts/export.rsrs/moq-mux/src/error.rsrs/moq-mux/src/source.rsrs/moq-net/src/path.rsrs/moq-rtc/src/egress.rs
c472c01 to
43f2234
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 43f2234244
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
9089116 to
abda12e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: abda12e37e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3c308ad94
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 96a6378939
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d406e2b8ec
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
draft-lcurley-moq-hang-02 had a consumer ignore a rendition whose `broadcast`
field escapes above the root. Both stacks clamped instead, so
`"broadcast": "../../../../x"` resolved to `x` and the consumer silently
subscribed to an unrelated broadcast. Root cause: `resolve` treated an excess
`..` as a no-op, turning an out-of-range reference into a valid-looking one.
`Path::resolve` / `Path.resolve` now return `Option`/`undefined` instead of
clamping. A reference that stops on the root is fine (a broadcast can live
there), so `None` means exactly one thing: a `..` with nothing left to pop.
An escaping reference rejects the whole catalog, and the draft changes to
require that (`MUST ignore` -> `MUST reject`, with the rationale added to its
Security Considerations). The root is the consumer's authorized subtree, so
such a reference names content the consumer cannot reach: a malformed catalog,
not a rendition to skip. The check lives in the catalog stream (covering
video, audio, and text renditions), so no consumer can obtain an unvalidated
snapshot; consumers key their bookkeeping off the catalog, so a reference
caught any later would already have reached a track layout, a playlist, an SDP
codec list, or a quality selector.
The broadcast knows its own path: `create_broadcast` stamps the
origin-root-relative path it already computed onto `broadcast::Info`, and
track handles expose the `Arc<Info>` they already held
(`track::{Consumer,Subscriber}::broadcast()`), so a catalog track names its
own base and no constructor grew a parameter. Origin-root-relative is the
right coordinate system: token-scoped cursors exist only inside the relay,
which is media-agnostic and never reads catalogs. A standalone producer has an
empty path and is its own root, so any `..` reference escapes.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
d406e2b to
3a0b1da
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a0b1da18c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // the catalog, the timeline subscription, and the closed check below all refer to the same | ||
| // resolution. A same-name replacement between the two lookups would otherwise split them. | ||
| let mut consumer = loop { | ||
| match catalog::Consumer::<()>::new(&broadcast, CatalogFormat::Hang).await { |
There was a problem hiding this comment.
Bind HLS media fetches to the resolved broadcast
When a same-name source is replaced after Broadcaster::new captures broadcast A, this now keeps the catalog and timeline on A, but renditions.sync(&source, ...) still stores the path-based Source, and Rendition::track at rs/moq-hls/src/export/rendition.rs:335 resolves it lazily to replacement B. A segment request in that window therefore uses A's catalog and timeline ranges to fetch B's media, potentially returning unrelated or mismatched bytes. Fresh evidence after the earlier fix is that the remaining lazy source.resolve still performs this second lookup; bind same-broadcast media fetches to the supplied broadcast handle as well.
Useful? React with 👍 / 👎.
Targets
dev:Path::resolve/Path.resolvechange signature.Summary
draft-lcurley-moq-hang-02had a consumer ignore a rendition whosebroadcastfield escapes above the root. Both stacks clamped instead, so"broadcast": "../../../../x"resolved toxand the consumer silently subscribed to an unrelated broadcast. Root cause:resolvetreated an excess..as a no-op, turning an out-of-range reference into a valid-looking one.resolvenow returnsOption/undefinedrather than clamping. A reference that stops on the root is fine (a broadcast can live there), soNonemeans exactly one thing: a..with nothing left to pop.create_broadcastalready computed the origin-root-relative path; it now stamps it onbroadcast::Info, and track handles expose theArc<Info>they already held (track::{Consumer,Subscriber}::broadcast()). A catalog track can therefore name its own base, so no reader has to be told it and no constructor grew a parameter.catalog::hang::Consumervalidates every snapshot against its track's broadcast path.moq-cli transcodeemits exactly one..per nesting level, so it cannot produce an escaping reference.Why the path lives on
broadcast::InfoOrigin-root-relative is the right coordinate system. Token-scoped cursors (
origin::Consumer::with_root, applied per session by the relay's auth) mean the same broadcast is seen at different paths by different clients, so a client-relative path cannot live on shared state. But those scoped cursors exist only inside the relay, which is media-agnostic and never reads catalogs. Every catalog reader is an endpoint whose origin has exactly one coordinate system, which is also its authorized view: paths above its root do not exist on its wire.A standalone producer (
Info::new().produce()) has an empty path and is its own root, so any..reference escapes. That is the honest semantic: such refs are relative to an announce location that does not exist yet. The transcodeend_to_endtest tripped exactly this and now mints its output through an origin, asmoq-clidoes in production.Why reject rather than ignore
Because "escapes the root" and "crosses an authorization boundary" are the same condition. Clamping silently redirects the subscription; ignoring the rendition declines to follow it but still tells nobody. Rejecting the catalog puts the failure in front of the operator who can fix the publisher.
This changes
draft-lcurley-moq-hangnormatively (MUST ignore->MUST reject), and adds the rationale to its Security Considerations section, which was a bare TODO. hang is published at-02; the draft has no changelog appendix, so perdrafts/CLAUDE.mdnone was added.Operational consequence, stated plainly: a gateway exporting a third-party broadcast (
moq-hls,moq-rtmp, the container exporters) now aborts that export on an escaping reference, where before it clamped. Contained to the one broadcast, not the process, but live-stream-killing on malformed input. That is the intended reading of a hard error.Public API changes
Breaking (hence
dev):moq-net:Path::resolvereturnsOption<PathOwned>(wasPathOwned).js/net:Path.resolvereturnsValid | undefined(wasValid).Additive:
moq-net:broadcast::Info.path(Infois#[non_exhaustive]), stamped bycreate_broadcast;track::Consumer::broadcast()andtrack::Subscriber::broadcast().moq-mux:Source::catalog(format), andError::EscapingBroadcast, raised by the catalog stream as well as bySource::{resolve,subscribe_track}.Behavior changes no signature states:
catalog::hang::Consumercan now fail on a catalog it previously yielded (escaping reference).@moq/watchpreviously treated../..froma/bas a self-reference...rejects it. Mint the broadcast throughcreate_broadcastwhen its catalog uses cross-broadcast references.Internal:
Source::requestreturnsResult,ExportSource::for_*returnResult<Self>.libmoq,moq-ffi, andmoq_transcode::runare untouched relative todevin their signatures; the catalog constructors (catalog::hang::Consumer::{new,compressed},catalog::Consumer::new,catalog::Producer::consume) keep their pre-branch shapes.Cross-package sync
rs/moq-net->js/netmirrored forPath.resolve;Info.pathhas no JS counterpart because the JS consumer model already carries the name where catalogs are read (@moq/watch'sBroadcast.in.name).doc/concept/layer/hang.mdand thebroadcastfield docs inrs/hang+js/hangupdated;drafts/draft-lcurley-moq-hang.mdupdated normatively (above). No wire-format change.Test plan
just check,just rs doctest,just drafts check, andRUSTDOCFLAGS="-D warnings" cargo docon the touched crates all pass.moq-net:resolveunit tests + doctests -- append/pop, empty rel, escape rejected (including a..that escapes mid-way then walks back down), root-landing allowed.moq-muxcatalog/hang/consumer.rs: a catalog carrying a legal../sourceand a root-landing../..is accepted; one that also carries an escaping../../../elsewhereis rejected wholesale, including via a text (caption) rendition. The broadcast is minted through an origin, which is what stamps the path being resolved against. The check covers every section carrying renditions (video, audio, text); adversarial review caught the text omission.moq-muxsource.rs:escaping_reference_is_rejectedbuilds a realelsewherebroadcast at the exact path a clamp would land on, and asserts the reference resolves to neither it nor the catalog broadcast, viarequest,resolve, andsubscribe_track.moq-muxcontainer/source.rs: the rendition fails on a test origin whose dynamic handler would happily serve the clamped path, and is kept for absent /""/../../source/sub.moq-transcodeend_to_end: regression for the standalone-root semantics; fails if the output broadcast loses its path.js/net/src/path.test.tsandjs/watch/src/broadcast.test.ts: the JS mirrors, plus a catalog with an escaping rendition being rejected and one within the root being accepted.state.read()guard living acrossstate.consume()), fixed with a hoist.🤖 Generated with Claude Code
(Written by Opus 5)