fix(net): backport legacy request parameter decoding

js/net/src/ietf/ietf.test.ts

@@ -242,14 +242,8 @@ test("Message Parameters: Location preserves bigint precision", async () => {
 
 	expect(params.largest).toEqual(largest);
 
-	for (const version of [
-		Version.DRAFT_14,
-		Version.DRAFT_15,
-		Version.DRAFT_16,
-		Version.DRAFT_17,
-		Version.DRAFT_18,
-		Version.DRAFT_19,
-	]) {
+	// Draft 14 carries Largest in the SUBSCRIBE_OK body, so it defines no LARGEST_OBJECT parameter.
+	for (const version of [Version.DRAFT_15, Version.DRAFT_16, Version.DRAFT_17, Version.DRAFT_18, Version.DRAFT_19]) {
 		const encoded = await encodeVersioned(params, version);
 		const decoded = await decodeVersioned(encoded, Parameters.decode, version);
 		expect(decoded.largest).toEqual(largest);
@@ -1946,6 +1940,91 @@ test("Subscribe v16: rejects INCLUDE_PROPERTIES", async () => {
 	await expect(decodeVersioned(body, Subscribe.Subscribe.decode, Version.DRAFT_16)).rejects.toThrow();
 });
 
+for (const version of [Version.DRAFT_14, Version.DRAFT_15]) {
+	// DELIVERY_TIMEOUT (0x02) is the known key: both drafts define it.
+	test(`Parameters ${version}: unknown key does not hide a later known value`, async () => {
+		for (const bytes of [new Uint8Array([2, 0x3e, 1, 0x02, 7]), new Uint8Array([2, 0x3d, 1, 42, 0x02, 7])]) {
+			const params = await Parameters.decode(new Reader(undefined, bytes, version), version);
+			expect(params.deliveryTimeout).toBe(7n);
+		}
+	});
+
+	test(`Parameters ${version}: only a recognized key must be unique`, async () => {
+		for (const bytes of [
+			new Uint8Array([3, 0x3e, 1, 0x3e, 2, 0x02, 7]),
+			new Uint8Array([3, 0x3d, 1, 42, 0x3d, 1, 43, 0x02, 7]),
+			// ROUTE_COST (0x40b58): the cluster extension is negotiated only from draft 17.
+			new Uint8Array([3, 0x80, 0x04, 0x0b, 0x58, 5, 0x80, 0x04, 0x0b, 0x58, 6, 0x02, 7]),
+		]) {
+			const params = await Parameters.decode(new Reader(undefined, bytes, version), version);
+			expect(params.deliveryTimeout).toBe(7n);
+		}
+		const known = new Uint8Array([2, 0x02, 7, 0x02, 8]);
+		await expect(Parameters.decode(new Reader(undefined, known, version), version)).rejects.toThrow(/duplicate/);
+	});
+}
+
+// Draft 14 defines only DELIVERY_TIMEOUT, AUTHORIZATION TOKEN and MAX CACHE DURATION, so
+// later definitions such as SUBSCRIBER_PRIORITY and LOCATION_FILTER are unrecognized and may repeat.
+test("Parameters draft14: later definitions are unrecognized", async () => {
+	for (const bytes of [new Uint8Array([2, 0x20, 7, 0x20, 8]), new Uint8Array([2, 0x21, 0, 0x21, 0])]) {
+		const params = await Parameters.decode(new Reader(undefined, bytes, Version.DRAFT_14), Version.DRAFT_14);
+		expect(params.subscriberPriority).toBeUndefined();
+		expect(params.subscriptionFilter).toBeUndefined();
+	}
+});
+
+test("Subscribe draft15 recognizes only its parameters", async () => {
+	for (const parameter of [
+		[0x32, 7],
+		[0x04, 0],
+		[0x23, 1, 255],
+		[0x25, 1, 255],
+		[0x35, 1, 255],
+	]) {
+		const body = framed([...TRACK_HEAD, 2, ...parameter, 0x20, 7]);
+		const decoded = await decodeVersioned(body, Subscribe.Subscribe.decode, Version.DRAFT_15);
+		expect(decoded.subscriberPriority).toBe(7);
+		expect(decoded.fill).toBeUndefined();
+		expect(decoded.rangeFilters).toBe(false);
+	}
+	await expect(
+		decodeVersioned(framed([...TRACK_HEAD, 1, 0x21, 1, 255]), Subscribe.Subscribe.decode, Version.DRAFT_15),
+	).rejects.toThrow();
+});
+
+test("Publish draft15 ignores subscription-only parameters", async () => {
+	for (const parameter of [
+		[0x02, 7],
+		[0x20, 0x41, 0],
+		[0x21, 1, 255],
+	]) {
+		const body = framed([1, 1, 1, 116, 1, 118, 1, 2, ...parameter, 0x10, 0]);
+		const decoded = await decodeVersioned(body, Publish.decode, Version.DRAFT_15);
+		expect(decoded.forward).toBe(false);
+	}
+});
+
+for (const version of [Version.DRAFT_14, Version.DRAFT_15, Version.DRAFT_16, Version.DRAFT_17]) {
+	test(`SubscribeNamespace ${version}: FORWARD follows the draft`, async () => {
+		for (const value of [0, 1, 2]) {
+			const head = [
+				1,
+				...(version === Version.DRAFT_17 ? [0] : []),
+				0,
+				...(version >= Version.DRAFT_16 ? [1] : []),
+			];
+			const decoded = decodeVersioned(
+				framed([...head, 1, 0x10, value]),
+				SubscribeNamespace.SubscribeNamespaceLegacy.decode,
+				version,
+			);
+			if (version === Version.DRAFT_14 || value <= 1) await decoded;
+			else await expect(decoded).rejects.toThrow();
+		}
+	});
+}
+
 const SETUP_VERSIONS = [
 	Version.DRAFT_14,
 	Version.DRAFT_15,

js/net/src/ietf/parameters.ts

@@ -255,11 +255,34 @@ const MSG_PARAM_REPEATABLE: readonly bigint[] = [
 	MSG_PARAM_TRACK_PROPERTY_FILTER,
 ];
 
+/// The message parameters draft 14 defines, plus HIDDEN, whose extension any draft can
+/// negotiate. Every other id is unrecognized there.
+const MSG_PARAM_DRAFT14: readonly bigint[] = [
+	MSG_PARAM_DELIVERY_TIMEOUT,
+	MSG_PARAM_AUTHORIZATION_TOKEN,
+	MSG_PARAM_MAX_CACHE_DURATION,
+	MSG_PARAM_HIDDEN,
+];
+
+// These definitions, including the cluster extension negotiated only from draft 17, are
+// unknown KVPs in draft 15.
+const MSG_PARAM_AFTER_DRAFT15: readonly bigint[] = [
+	MSG_PARAM_SUBGROUP_DELIVERY_TIMEOUT,
+	MSG_PARAM_FILL_TIMEOUT,
+	MSG_PARAM_FILL_PARAMETERS,
+	...MSG_PARAM_RANGE_FILTERS,
+	MSG_PARAM_TRACK_PROPERTY_FILTER,
+	MSG_PARAM_INCLUDE_PROPERTIES,
+	MSG_PARAM_HOP_PATH,
+	MSG_PARAM_ROUTE_COST,
+];
+
 type MessageParamKind = "varint" | "uint8" | "bool" | "location" | "filter" | "bytes";
 /** A `{Group, Object}` pair carried by a message parameter, such as LARGEST_OBJECT. */
 export type MessageLocation = { groupId: bigint; objectId: bigint };
 
-function getMessageParamKind(id: bigint): MessageParamKind {
+/** The kind of a message parameter we define, or undefined for an unrecognized id. */
+function messageParamKind(id: bigint): MessageParamKind | undefined {
 	switch (id) {
 		case MSG_PARAM_DELIVERY_TIMEOUT:
 		case MSG_PARAM_FILL_TIMEOUT:
@@ -286,11 +309,16 @@ function getMessageParamKind(id: bigint): MessageParamKind {
 		case MSG_PARAM_HOP_PATH:
 			return "bytes";
 		default:
-			if (MSG_PARAM_REPEATABLE.includes(id)) return "bytes";
-			throw new Error(`unknown message parameter id: ${id.toString()}`);
+			return MSG_PARAM_REPEATABLE.includes(id) ? "bytes" : undefined;
 	}
 }
 
+function getMessageParamKind(id: bigint): MessageParamKind {
+	const kind = messageParamKind(id);
+	if (kind === undefined) throw new Error(`unknown message parameter id: ${id.toString()}`);
+	return kind;
+}
+
 /**
  * The draft-16 and earlier form: draft-16 section 9.2 serializes every Message Parameter as
  * a Key-Value-Pair, and section 9.2.2.7 calls LARGEST_OBJECT "a length-prefixed Location
@@ -321,6 +349,7 @@ export class Parameters {
 	#locations: Map<bigint, MessageLocation>;
 	/** LOCATION_FILTER, kept decoded because its framing depends on the draft. */
 	#filter: Filter.Filter | undefined;
+	#filterBytes: { value: Uint8Array; version: IetfVersion } | undefined;
 	/** Every instance of a parameter that may repeat, decoded only; we never send one. */
 	#repeated: Map<bigint, Uint8Array[]>;
 
@@ -443,11 +472,15 @@ export class Parameters {
 
 	/** LOCATION_FILTER: which Objects the subscription delivers. See `filter.ts`. */
 	get subscriptionFilter(): Filter.Filter | undefined {
+		if (this.#filterBytes && this.#filter === undefined) {
+			this.#filter = Filter.decode(this.#filterBytes.value, this.#filterBytes.version);
+		}
 		return this.#filter;
 	}
 
 	set subscriptionFilter(v: Filter.Filter) {
 		this.#filter = v;
+		this.#filterBytes = undefined;
 	}
 
 	/** FILL_PARAMETERS: the draft-20 backfill request, as its raw parameter value. */
@@ -494,7 +527,7 @@ export class Parameters {
 	}
 
 	async encode(w: Writer, version: IetfVersion) {
-		const filter = this.#filter;
+		const filter = this.subscriptionFilter;
 		await w.u53(this.vars.size + this.bytes.size + this.#locations.size + (filter ? 1 : 0));
 
 		if (version === Version.DRAFT_14 || version === Version.DRAFT_15) {
@@ -625,6 +658,20 @@ export class Parameters {
 				prevType = id;
 			}
 
+			// Drafts 14 and 15 have a receiver ignore unrecognized parameters, repeats included.
+			// Consume their KVP framing without interpreting a value or exposing it to message
+			// validation.
+			const unrecognized =
+				version === Version.DRAFT_14
+					? !MSG_PARAM_DRAFT14.includes(id)
+					: version === Version.DRAFT_15 &&
+						(MSG_PARAM_AFTER_DRAFT15.includes(id) || messageParamKind(id) === undefined);
+			if (unrecognized) {
+				if (id % 2n === 0n) await r.u62();
+				else await r.read(await r.u53());
+				continue;
+			}
+
 			if (version === Version.DRAFT_14 || version === Version.DRAFT_15 || version === Version.DRAFT_16) {
 				if (id % 2n === 0n) {
 					if (params.vars.has(id)) {
@@ -642,12 +689,17 @@ export class Parameters {
 							throw new Error(`duplicate message parameter id: ${id.toString()}`);
 						}
 						params.#locations.set(id, decodeLocation(bytes));
-					} else if (id === MSG_PARAM_SUBSCRIPTION_FILTER && version !== Version.DRAFT_14) {
-						// Draft-14 carries the filter in the SUBSCRIBE body, so 0x21 is undefined there.
-						if (params.#filter !== undefined) {
+					} else if (id === MSG_PARAM_SUBSCRIPTION_FILTER) {
+						if (params.#filterBytes !== undefined || params.#filter !== undefined) {
 							throw new Error(`duplicate message parameter id: ${id.toString()}`);
 						}
-						params.#filter = Filter.decode(bytes, version);
+						// Legacy messages can carry a known parameter on the wrong message.
+						// Validate its semantics only when that message consumes the filter.
+						if (version === Version.DRAFT_15) {
+							params.#filterBytes = { value: bytes, version };
+						} else {
+							params.#filter = Filter.decode(bytes, version);
+						}
 					} else {
 						if (params.bytes.has(id)) {
 							throw new Error(`duplicate message parameter id: ${id.toString()}`);

js/net/src/ietf/subscribe_namespace.ts

@@ -145,6 +145,11 @@ export class SubscribeNamespaceLegacy {
 			subscribeOptions = await r.u53();
 		}
 		const params = await Parameters.decode(r, version);
+		// FORWARD is defined here from draft-15. Its value is ignored, but must be a boolean.
+		const forward = params.vars.get(0x10n);
+		if (version !== Version.DRAFT_14 && forward !== undefined && forward > 1n) {
+			throw new Error(`invalid FORWARD parameter: ${forward}`);
+		}
 
 		return new SubscribeNamespaceLegacy({ namespace, requestId, subscribeOptions, hidden: params.hidden });
 	}

rs/moq-net/src/ietf/fetch.rs

@@ -204,13 +204,13 @@ impl Message for Fetch<'_> {
 				let fetch_type = FetchType::decode(buf, version)?;
 				decode_params!(buf, version,
 					0x03 => _authorization_token: Vec<Opaque>,
-					0x0A => fill_timeout: Option<u64>,
+					0x0A => fill_timeout: Option<u64> where !matches!(version, Version::Draft15 | Version::Draft16 | Version::Draft17),
 					0x20 => subscriber_priority: Option<u8>,
 					0x22 => group_order: Option<GroupOrder>,
-					0x25 => subgroup_filter: Vec<Opaque>,
-					0x26 => object_id_filter: Vec<Opaque>,
-					0x27 => priority_filter: Vec<Opaque>,
-					0x28 => object_property_filter: Vec<Opaque>,
+					0x25 => subgroup_filter: Vec<Opaque> where has_range_filters(version),
+					0x26 => object_id_filter: Vec<Opaque> where has_range_filters(version),
+					0x27 => priority_filter: Vec<Opaque> where has_range_filters(version),
+					0x28 => object_property_filter: Vec<Opaque> where has_range_filters(version),
 				);
 				let range_filters = [
 					subgroup_filter,
@@ -221,13 +221,6 @@ impl Message for Fetch<'_> {
 				.iter()
 				.any(|filter| !filter.is_empty());
 
-				// An unknown message parameter is a protocol violation, so each stays rejected
-				// on the drafts that predate it: FILL_TIMEOUT arrived in draft-18.
-				let has_fill_timeout = !matches!(version, Version::Draft15 | Version::Draft16 | Version::Draft17);
-				if (fill_timeout.is_some() && !has_fill_timeout) || (range_filters && !has_range_filters(version)) {
-					return Err(DecodeError::InvalidValue);
-				}
-
 				(
 					fetch_type,
 					subscriber_priority,
@@ -242,14 +235,14 @@ impl Message for Fetch<'_> {
 				let track = Cow::<str>::decode(buf, version)?;
 				decode_params!(buf, version,
 					0x03 => _authorization_token: Vec<Opaque>,
-					0x0A => fill_timeout: Option<u64>,
+					0x0A => fill_timeout: Option<u64> where !matches!(version, Version::Draft15 | Version::Draft16 | Version::Draft17),
 					0x20 => subscriber_priority: Option<u8>,
 					0x21 => filter: Option<Filter>,
 					0x22 => group_order: Option<GroupOrder>,
-					0x25 => subgroup_filter: Vec<Opaque>,
-					0x26 => object_id_filter: Vec<Opaque>,
-					0x27 => priority_filter: Vec<Opaque>,
-					0x28 => object_property_filter: Vec<Opaque>,
+					0x25 => subgroup_filter: Vec<Opaque> where has_range_filters(version),
+					0x26 => object_id_filter: Vec<Opaque> where has_range_filters(version),
+					0x27 => priority_filter: Vec<Opaque> where has_range_filters(version),
+					0x28 => object_property_filter: Vec<Opaque> where has_range_filters(version),
 					0x35 => _include_properties: Option<bool>,
 				);
 				let range_filters = [

rs/moq-net/src/ietf/parameters.rs

@@ -529,7 +529,8 @@ macro_rules! encode_params {
 /// optional parameters (defaults to `None` when absent) and bare types like `u8`
 /// for parameters where `T::default()` is an acceptable fallback.
 ///
-/// Unknown parameters cause `DecodeError::InvalidValue`.
+/// Unknown parameters are ignored in drafts 14/15 and otherwise cause `DecodeError::InvalidValue`.
+/// A false `where` gate treats a declared key as unknown before decoding its value.
 /// Duplicate parameters cause `DecodeError::Duplicate`, unless the type allows a repeat
 /// (see [`Param::param_repeat`]), such as `Vec<T>`.
 ///
@@ -542,7 +543,7 @@ macro_rules! encode_params {
 /// let subscriber_priority = subscriber_priority.unwrap_or(128);
 /// ```
 macro_rules! decode_params {
-	($r:expr, $version:expr, $($key:expr => $name:ident: $ty:ty),* $(,)?) => {
+	($r:expr, $version:expr, $($key:expr => $name:ident: $ty:ty $(where $gate:expr)?),* $(,)?) => {
 		#[allow(unused)]
 		const _: () = {
 			let _keys: &[u64] = &[$($key),*];
@@ -588,7 +589,7 @@ macro_rules! decode_params {
 				// An if-chain rather than a `match`, so a key can be a named constant:
 				// the macro captures it as an expression, which is not a legal pattern.
 				$(
-					if _key == $key {
+					if _key == $key $( && ($gate) )? {
 						let _value = <$ty as $crate::ietf::Param>::param_decode($r, _version)?;
 						$name = Some(match $name.take() {
 							None => _value,
@@ -597,6 +598,16 @@ macro_rules! decode_params {
 						continue;
 					}
 				)*
+				// Drafts 14 and 15 use Key-Value-Pairs and require unknown ids to be ignored.
+				if matches!(_version, $crate::ietf::Version::Draft14 | $crate::ietf::Version::Draft15) {
+					if _key % 2 == 0 {
+						<u64 as $crate::coding::Decode<$crate::ietf::Version>>::decode($r, _version)?;
+					} else {
+						let _value = <Vec<u8> as $crate::coding::Decode<$crate::ietf::Version>>::decode($r, _version)?;
+						if _value.len() > (1 << 16) - 1 { return Err($crate::coding::DecodeError::BoundsExceeded); }
+					}
+					continue;
+				}
 				return Err($crate::coding::DecodeError::InvalidValue);
 			}
 		}
@@ -1117,10 +1128,33 @@ mod tests {
 				let _ = val;
 				Ok(())
 			})();
-			assert!(
-				matches!(result, Err(DecodeError::InvalidValue)),
-				"expected InvalidValue for unknown param in {version}"
-			);
+			if matches!(version, Version::Draft14 | Version::Draft15) {
+				result.unwrap();
+				assert!(!bytes.has_remaining());
+			} else {
+				assert!(matches!(result, Err(DecodeError::InvalidValue)), "{version}");
+			}
+		}
+	}
+
+	#[test]
+	fn legacy_unknown_parameter_is_ignored() {
+		for version in [Version::Draft14, Version::Draft15] {
+			for data in [
+				&[2, 0x3e, 1, 0x20, 7][..],
+				&[2, 0x3d, 1, 42, 0x20, 7][..],
+				// Unknown ids may repeat.
+				&[3, 0x3e, 1, 0x3e, 2, 0x20, 7][..],
+				&[3, 0x3d, 1, 42, 0x3d, 1, 43, 0x20, 7][..],
+			] {
+				let mut bytes = data;
+				let result: Result<Option<u8>, DecodeError> = (|| {
+					decode_params!(&mut bytes, version, 0x20 => priority: Option<u8>);
+					Ok(priority)
+				})();
+				assert_eq!(result.unwrap(), Some(7));
+				assert!(bytes.is_empty());
+			}
 		}
 	}