fix(hls): percent-encode DASH rendition names as one path segment

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

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.

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 (`/<broadcast>/manifest.mpd`), so a rendition's
-//! `<kind>/<name>/...` resources resolve under the broadcast directory, shared byte-for-byte
-//! with the HLS routes (only the time-addressed segment alias `seg/t<pts>.m4s` is DASH's own).
+//! URIs are relative to the manifest (`/<broadcast>/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<pts>.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 `<name>` in its `<kind>/<name>/...` 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<Item = &'a Representa
 		.max(1)
 }
 
-fn render_representation(out: &mut String, rep: &Representation, suffix: &str) {
+fn render_representation(out: &mut String, rep: &Representation, query: Option<&str>) {
 	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 `&amp;` 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,
-		"        <SegmentTemplate timescale=\"{}\" initialization=\"{kind}/{name}/init.{init}.mp4{suffix}\" media=\"{kind}/{name}/seg/{generation}{tag}.t$Time$.m4s{suffix}\">",
+		"        <SegmentTemplate timescale=\"{}\" initialization=\"{initialization}\" media=\"{media}\">",
 		rep.timescale.max(1)
 	);
 	let _ = writeln!(out, "          <SegmentTimeline>");
@@ -189,7 +202,7 @@ fn render_representation(out: &mut String, rep: &Representation, suffix: &str) {
 	let _ = writeln!(out, "      </Representation>");
 }
 
-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
 			"    <AdaptationSet contentType=\"{content}\" mimeType=\"{mime}\" segmentAlignment=\"true\" startWithSAP=\"1\">"
 		);
 		for rep in representations {
-			render_representation(out, rep, suffix);
+			render_representation(out, rep, query);
 		}
 		let _ = writeln!(out, "    </AdaptationSet>");
 	}
@@ -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, "  <Period id=\"0\" start=\"PT0.000S\">");
-	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, "  </Period>");
 	let _ = writeln!(out, "</MPD>");
 	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&amp;x=1\""),
+			"{out}"
+		);
+		assert!(
+			out.contains("media=\"video/video%2F1080p/seg/ab12cd34.t$Time$.m4s?jwt=abc.def&amp;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&amp;b&quot;c\""), "{out}");
+		assert!(
+			out.contains("initialization=\"video/a%26b%22c/init.0123abcd.mp4?x=1&amp;y=2\""),
+			"{out}"
+		);
+		assert!(
+			out.contains("media=\"video/a%26b%22c/seg/ab12cd34.t$Time$.m4s?x=1&amp;y=2\""),
+			"{out}"
+		);
+		assert!(!out.contains("&amp;amp;"), "{out}");
+		assert!(!out.contains("%26amp"), "{out}");
+	}
 }

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);