From afcc0467d168b0c68304452a4f882baf38cd796c Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Fri, 9 Oct 2026 13:07:43 -0700 Subject: [PATCH 1/2] chore: claim quest/m2/dash-rendition-uri From 090a3bba319f92518691f10ae0f758c24f381ae0 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Fri, 9 Oct 2026 13:20:52 -0700 Subject: [PATCH 2/2] fix(hls): percent-encode DASH rendition names as one path segment DASH init and media URLs encode the rendition name with the HLS path-segment set, then XML-escape the finished URL. The Representation id stays raw. --- quest/m2/README.md | 1 - quest/m2/dash-rendition-uri.md | 22 ------ rs/moq-hls/src/export/master.rs | 5 +- rs/moq-hls/src/export/mpd.rs | 115 +++++++++++++++++++++++++++----- rs/moq-hls/src/server/routes.rs | 33 +++++++++ 5 files changed, 136 insertions(+), 40 deletions(-) delete mode 100644 quest/m2/dash-rendition-uri.md diff --git a/quest/m2/README.md b/quest/m2/README.md index cb2a402eaa..148fd8bfe8 100644 --- a/quest/m2/README.md +++ b/quest/m2/README.md @@ -62,7 +62,6 @@ consumer yet, waits in [m3](/quest/m3/README.md). - [Archive S3 wire proof](/quest/m2/archive-s3.md) - the archive proof also runs through the S3 client against an in-process S3-compatible server - [Archive recovery listing](/quest/m2/archive-recovery-listing.md) - a resumed DVR lists what changed since its checkpoint, not every stored group - [Range-addressed HLS playlists](/quest/m2/hls-ranges.md) - `moq-hls` lists a start-to-end range of a recording as its own playlist, for moq.pro's managed HLS -- [DASH rendition URLs](/quest/m2/dash-rendition-uri.md) - DASH init and segment URLs percent-encode the rendition name, so a name with a slash resolves to its own rendition - [IETF on the ring](/quest/m2/uring-ietf.md) - the io_uring workers serve moq-transport sessions too, so a uring relay drops no client protocol - [Dropped uring session closes](/quest/m2/uring-drop-close.md) - a moq-uring session dropped without close() closes its connection - [Relay io_uring packages](/quest/m2/relay-io-uring-package.md) - Linux relay packages ship io_uring once the ring is on par with tokio diff --git a/quest/m2/dash-rendition-uri.md b/quest/m2/dash-rendition-uri.md deleted file mode 100644 index e2504f5465..0000000000 --- a/quest/m2/dash-rendition-uri.md +++ /dev/null @@ -1,22 +0,0 @@ -# [XS] Percent-encode DASH rendition URLs - -## Goal - -The MPD's `initialization` and `media` URLs carry each rendition name as one -percent-encoded path segment, as the HLS master playlist already does, so a -name containing `/`, `?`, `#`, `%`, or `$` resolves to its own rendition. Today -`video/1080p` under broadcast `live` routes to broadcast `live/video`, -rendition `1080p`. - -## Plan - -Found by the final-head audit of -[#4034](https://github.com/moq-dev/moq/pull/4034); the bug predates it. -Decided 2026-10-07: m2, since first-party publishers name renditions like -`video0`, though the catalog allows slashes. - -In `rs/moq-hls/src/export/mpd.rs` `render_representation`, encode the name -with the HLS renderer's `PATH_SEGMENT` set (`rs/moq-hls/src/export/master.rs`), -shared rather than copied, then XML-escape the finished URL. The -`Representation` `id` stays as is. Regress the names above through rendering -and `Route::parse` in `rs/moq-hls/src/server/routes.rs`. diff --git a/rs/moq-hls/src/export/master.rs b/rs/moq-hls/src/export/master.rs index 42845da5f4..79aca598fa 100644 --- a/rs/moq-hls/src/export/master.rs +++ b/rs/moq-hls/src/export/master.rs @@ -16,7 +16,10 @@ const VERSION: u32 = 9; const AUDIO_GROUP: &str = "aud"; /// RFC 3986 unreserved characters, which are safe in one URL path segment. -const PATH_SEGMENT: &AsciiSet = &NON_ALPHANUMERIC.remove(b'-').remove(b'.').remove(b'_').remove(b'~'); +/// +/// The DASH manifest encodes rendition names with this same set, so a name stays one segment +/// in both layouts. +pub(crate) const PATH_SEGMENT: &AsciiSet = &NON_ALPHANUMERIC.remove(b'-').remove(b'.').remove(b'_').remove(b'~'); /// The live layout's media-playlist URI for a rendition, relative to the master, with an /// optional query (without the leading `?`) appended. diff --git a/rs/moq-hls/src/export/mpd.rs b/rs/moq-hls/src/export/mpd.rs index 6a076dd2bc..32f51496d0 100644 --- a/rs/moq-hls/src/export/mpd.rs +++ b/rs/moq-hls/src/export/mpd.rs @@ -7,20 +7,25 @@ //! that skips a span (a gap) or a client that joins mid-window never mis-addresses a segment //! the way positional numbering would. //! -//! URIs are relative to the manifest (`//manifest.mpd`), so a rendition's -//! `//...` resources resolve under the broadcast directory, shared byte-for-byte -//! with the HLS routes (only the time-addressed segment alias `seg/t.m4s` is DASH's own). +//! URIs are relative to the manifest (`//manifest.mpd`). The rendition name is +//! one percent-encoded path segment, using the HLS master's set, and the finished URL is +//! then XML-escaped, so a name containing `/` still resolves under the broadcast. The +//! `Representation` id keeps the raw name. The resources are shared byte-for-byte with the +//! HLS routes; only the time-addressed segment alias `seg/t.m4s` is DASH's own. use std::collections::BTreeMap; use std::fmt::Write; use std::sync::Arc; use std::time::{Duration, SystemTime}; +use percent_encoding::utf8_percent_encode; + use super::Kind; +use super::master::PATH_SEGMENT; /// One DASH representation: master-level metadata plus its segment timeline. pub(crate) struct Representation { - /// Rendition name (the `` in its `//...` paths). + /// Rendition name. URLs carry it as one percent-encoded path segment; the id keeps it raw. pub name: String, /// Whether this is a video or audio rendition (also the URL path component). pub kind: Kind, @@ -139,19 +144,27 @@ fn max_segment_duration<'a>(representations: impl Iterator) { let kind = rep.kind.as_str(); - let name = escape(&rep.name); - let init = escape(&rep.init); - let tag = escape(&rep.tag); + // The id is the raw name. The URL encodes it first, as one path segment: a slash left + // raw would name a different broadcast, and XML-escaping before encoding would turn `&` + // into `&` inside that segment. `$Time$` stays literal because only the name is encoded. + let id = escape(&rep.name); + let segment = utf8_percent_encode(&rep.name, PATH_SEGMENT); let generation = rep .generation .as_deref() - .map(|g| format!("{}.", escape(g))) + .map(|generation| format!("{generation}.")) .unwrap_or_default(); + let suffix = query.map(|query| format!("?{query}")).unwrap_or_default(); + let initialization = escape(&format!("{kind}/{segment}/init.{}.mp4{suffix}", rep.init)); + let media = escape(&format!( + "{kind}/{segment}/seg/{generation}{}.t$Time$.m4s{suffix}", + rep.tag + )); let mut attrs = format!( - "id=\"{kind}/{name}\" bandwidth=\"{}\" codecs=\"{}\"", + "id=\"{kind}/{id}\" bandwidth=\"{}\" codecs=\"{}\"", rep.bandwidth, escape(&rep.codec) ); @@ -175,7 +188,7 @@ fn render_representation(out: &mut String, rep: &Representation, suffix: &str) { let _ = writeln!( out, - " ", + " ", rep.timescale.max(1) ); let _ = writeln!(out, " "); @@ -189,7 +202,7 @@ fn render_representation(out: &mut String, rep: &Representation, suffix: &str) { let _ = writeln!(out, " "); } -fn render_adaptation_sets(out: &mut String, kind: Kind, representations: &[Representation], suffix: &str) { +fn render_adaptation_sets(out: &mut String, kind: Kind, representations: &[Representation], query: Option<&str>) { // Representations in one AdaptationSet must be seamlessly switchable, so group by codec // (mirroring the HLS master's audio groups). let mut codecs = BTreeMap::<&str, Vec<&Representation>>::new(); @@ -214,7 +227,7 @@ fn render_adaptation_sets(out: &mut String, kind: Kind, representations: &[Repre " " ); for rep in representations { - render_representation(out, rep, suffix); + render_representation(out, rep, query); } let _ = writeln!(out, " "); } @@ -226,7 +239,6 @@ fn render_adaptation_sets(out: &mut String, kind: Kind, representations: &[Repre /// to every child URL (each representation's init and media templates), so a stock player that /// does not replay request headers still carries a credential on its follow-up requests. pub(crate) fn render_manifest(manifest: &Manifest, query: Option<&str>) -> String { - let suffix = query.map(|q| format!("?{}", escape(q))).unwrap_or_default(); let representations = || manifest.video.iter().chain(&manifest.audio); let target = max_segment_duration(representations()); @@ -280,8 +292,8 @@ pub(crate) fn render_manifest(manifest: &Manifest, query: Option<&str>) -> Strin ); let _ = writeln!(out, " "); - render_adaptation_sets(&mut out, Kind::Video, &manifest.video, &suffix); - render_adaptation_sets(&mut out, Kind::Audio, &manifest.audio, &suffix); + render_adaptation_sets(&mut out, Kind::Video, &manifest.video, query); + render_adaptation_sets(&mut out, Kind::Audio, &manifest.audio, query); let _ = writeln!(out, " "); let _ = writeln!(out, ""); out @@ -451,4 +463,75 @@ mod tests { assert!(out.contains("id=\"video/video0\"")); assert!(!out.contains("id=\"audio/audio0\""), "an empty timeline is unplayable"); } + + fn render_named(name: &str, query: Option<&str>) -> String { + let mut rep = video(vec![(0, 2_000)], false); + rep.name = name.into(); + render_manifest( + &Manifest { + availability_start: Some(SystemTime::UNIX_EPOCH), + publish: SystemTime::UNIX_EPOCH, + window: Some(Duration::from_secs(16)), + finished: false, + video: vec![rep], + audio: Vec::new(), + }, + query, + ) + } + + #[test] + fn rendition_name_is_one_percent_encoded_path_segment() { + // A raw slash under broadcast `live` would route to broadcast `live/video`, + // rendition `1080p`. The id keeps that slash; the URLs do not. + let out = render_named("video/1080p", Some("jwt=abc.def&x=1")); + assert!(out.contains("id=\"video/video/1080p\""), "{out}"); + assert!( + out.contains("initialization=\"video/video%2F1080p/init.0123abcd.mp4?jwt=abc.def&x=1\""), + "{out}" + ); + assert!( + out.contains("media=\"video/video%2F1080p/seg/ab12cd34.t$Time$.m4s?jwt=abc.def&x=1\""), + "{out}" + ); + assert!(!out.contains("initialization=\"video/video/1080p/"), "{out}"); + + let cases = [ + ("cam?1", "cam%3F1"), + ("cam#1", "cam%231"), + ("100%", "100%25"), + ("price$9", "price%249"), + ]; + for (name, segment) in cases { + let out = render_named(name, None); + assert!( + out.contains(&format!("id=\"video/{name}\"")), + "id stays raw for {name}: {out}" + ); + assert!( + out.contains(&format!("initialization=\"video/{segment}/init.0123abcd.mp4\"")), + "init for {name}: {out}" + ); + assert!( + out.contains(&format!("media=\"video/{segment}/seg/ab12cd34.t$Time$.m4s\"")), + "media for {name}: {out}" + ); + } + } + + #[test] + fn finished_url_is_xml_escaped_after_encoding() { + let out = render_named("a&b\"c", Some("x=1&y=2")); + assert!(out.contains("id=\"video/a&b"c\""), "{out}"); + assert!( + out.contains("initialization=\"video/a%26b%22c/init.0123abcd.mp4?x=1&y=2\""), + "{out}" + ); + assert!( + out.contains("media=\"video/a%26b%22c/seg/ab12cd34.t$Time$.m4s?x=1&y=2\""), + "{out}" + ); + assert!(!out.contains("&amp;"), "{out}"); + assert!(!out.contains("%26amp"), "{out}"); + } } diff --git a/rs/moq-hls/src/server/routes.rs b/rs/moq-hls/src/server/routes.rs index afb99720ae..fefa789a1a 100644 --- a/rs/moq-hls/src/server/routes.rs +++ b/rs/moq-hls/src/server/routes.rs @@ -392,6 +392,39 @@ mod tests { ); } + #[test] + fn parses_encoded_dash_rendition_as_one_segment() { + let parsed = |path| { + let route = Route::parse(path)?; + let rendition = match &route.resource { + Resource::Media { rendition, .. } + | Resource::Init { rendition, .. } + | Resource::Segment { rendition, .. } + | Resource::SegmentAt { rendition, .. } => rendition.clone(), + Resource::Master | Resource::Manifest => return None, + }; + Some((route.broadcast, rendition)) + }; + + // The manifest for broadcast `live` and rendition `video/1080p` emits these paths. + // Unencoded, the same name is broadcast `live/video`, rendition `1080p`. + let cases = [ + ("/live/video/video%2F1080p/init.0123abcd.mp4", "video/1080p"), + ("/live/video/video%2F1080p/seg/ab12cd34.t2000.m4s", "video/1080p"), + ("/live/video/cam%3F1/init.0123abcd.mp4", "cam?1"), + ("/live/video/cam%231/media.m3u8", "cam#1"), + ("/live/audio/100%25/init.0123abcd.mp4", "100%"), + ("/live/video/price%249/seg/ab12cd34.t0.m4s", "price$9"), + ]; + for (path, rendition) in cases { + assert_eq!(parsed(path), Some(("live".into(), rendition.into())), "{path}"); + } + assert_eq!( + parsed("/live/video/video/1080p/init.0123abcd.mp4"), + Some(("live/video".into(), "1080p".into())) + ); + } + #[test] fn parses_all_resource_routes() { let resource = |path| Route::parse(path).map(|route| route.resource);