fix(net): backport repeated unknown SETUP option handling

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

@@ -7,7 +7,7 @@ import { Fetch } from "./fetch.ts";
 import * as GoAway from "./goaway.ts";
 import * as Namespace from "./namespace.ts";
 import { FetchFrame, type FetchPosition, Frame, Group, type GroupFlags } from "./object.ts";
-import { Parameters, SetupOptions } from "./parameters.ts";
+import { Parameters, SetupOption, SetupOptions } from "./parameters.ts";
 import { Publish, PublishDone } from "./publish.ts";
 import * as Announce from "./publish_namespace.ts";
 import { RequestError, RequestOk } from "./request.ts";
@@ -1945,3 +1945,75 @@ test("Subscribe v16: rejects INCLUDE_PROPERTIES", async () => {
 	const body = framed([...TRACK_HEAD, 0x01, 0x35, 0x01, 0x00]);
 	await expect(decodeVersioned(body, Subscribe.Subscribe.decode, Version.DRAFT_16)).rejects.toThrow();
 });
+
+const SETUP_VERSIONS = [
+	Version.DRAFT_14,
+	Version.DRAFT_15,
+	Version.DRAFT_16,
+	Version.DRAFT_17,
+	Version.DRAFT_18,
+	Version.DRAFT_19,
+	Version.DRAFT_20,
+	Version.DRAFT_21,
+	Version.DRAFT_22,
+] as const;
+
+const legacySetup = (version: IetfVersion) => version <= Version.DRAFT_16;
+
+// A SETUP block carrying the option `kind` twice.
+async function repeatedSetupOption(kind: bigint, version: IetfVersion): Promise<Uint8Array> {
+	const { stream, written } = createTestWritableStream();
+	const writer = new Writer(stream, version);
+	if (legacySetup(version)) await writer.u53(2);
+	await writer.u62(kind);
+	for (let i = 0; i < 2; i++) {
+		if (i > 0) await writer.u62(version <= Version.DRAFT_15 ? kind : 0n);
+		if (kind % 2n === 0n) await writer.u62(1n);
+		else {
+			await writer.u53(1);
+			await writer.u8(1);
+		}
+	}
+	await writer.close();
+	await writer.closed;
+	return concatChunks(written);
+}
+
+const SETUP_UNIQUENESS: [bigint, (version: IetfVersion) => boolean][] = [
+	[0x20n, () => false],
+	[0x21n, () => false],
+	[0x9dn, () => false],
+	[0x11cn, () => false],
+	[SetupOption.MaxRequestId, legacySetup],
+	[SetupOption.HopId, (v) => !legacySetup(v)],
+	[SetupOption.RelayCost, (v) => !legacySetup(v)],
+	[SetupOption.MaxAuthTokenCacheSize, () => true],
+	[SetupOption.Solicit, () => true],
+	[SetupOption.Hidden, () => true],
+	[SetupOption.Implementation, () => true],
+];
+
+for (const version of SETUP_VERSIONS) {
+	for (const [kind, unique] of SETUP_UNIQUENESS) {
+		test(`SETUP ${version}: option ${kind} must be unique only when defined`, async () => {
+			const bytes = await repeatedSetupOption(kind, version);
+			const decoded = SetupOptions.decode(new Reader(undefined, bytes, version), version);
+			if (unique(version)) await expect(decoded).rejects.toThrow(/duplicate/);
+			else await decoded;
+		});
+	}
+}
+
+for (const version of [
+	Version.DRAFT_17,
+	Version.DRAFT_18,
+	Version.DRAFT_19,
+	Version.DRAFT_20,
+	Version.DRAFT_21,
+	Version.DRAFT_22,
+]) {
+	test(`SETUP ${version}: repeated unknown options still require complete values`, async () => {
+		const bytes = new Uint8Array([0x21, 1, 97, 0, 2, 98]);
+		await expect(SetupOptions.decode(new Reader(undefined, bytes, version), version)).rejects.toThrow();
+	});
+}

js/net/src/ietf/parameters.ts

@@ -21,6 +21,24 @@ export const SetupOption = {
 	Hidden: 0x40b5cn,
 } as const;
 
+const SETUP_OPTIONS: readonly bigint[] = Object.values(SetupOption);
+
+// Whether `version` defines a SETUP option. Only defined options must be unique; others,
+// including GREASE, may repeat.
+function definedSetupOption(id: bigint, version: IetfVersion): boolean {
+	const legacy = version === Version.DRAFT_14 || version === Version.DRAFT_15 || version === Version.DRAFT_16;
+	switch (id) {
+		case SetupOption.MaxRequestId:
+			return legacy;
+		// The MoQ Cluster extension is only negotiated on draft-17+ (see `cluster.ts`).
+		case SetupOption.HopId:
+		case SetupOption.RelayCost:
+			return !legacy;
+		default:
+			return SETUP_OPTIONS.includes(id);
+	}
+}
+
 /// Setup Options — used in SETUP messages.
 ///
 /// In d14-d16 these are count-prefixed ("Setup Parameters").
@@ -140,13 +158,13 @@ export class SetupOptions {
 				i++;
 
 				if (id % 2n === 0n) {
-					if (params.vars.has(id)) {
+					if (definedSetupOption(id, version) && params.vars.has(id)) {
 						throw new Error(`duplicate parameter id: ${id.toString()}`);
 					}
 					const varint = await r.u62();
 					params.setVarint(id, varint);
 				} else {
-					if (params.bytes.has(id)) {
+					if (definedSetupOption(id, version) && params.bytes.has(id)) {
 						throw new Error(`duplicate parameter id: ${id.toString()}`);
 					}
 					const size = await r.u53();
@@ -169,13 +187,13 @@ export class SetupOptions {
 				}
 
 				if (id % 2n === 0n) {
-					if (params.vars.has(id)) {
+					if (definedSetupOption(id, version) && params.vars.has(id)) {
 						throw new Error(`duplicate parameter id: ${id.toString()}`);
 					}
 					const varint = await r.u62();
 					params.setVarint(id, varint);
 				} else {
-					if (params.bytes.has(id)) {
+					if (definedSetupOption(id, version) && params.bytes.has(id)) {
 						throw new Error(`duplicate parameter id: ${id.toString()}`);
 					}
 					const size = await r.u53();

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

@@ -1,4 +1,4 @@
-use std::collections::{HashMap, hash_map};
+use std::collections::HashMap;
 
 use bytes::Buf;
 use num_enum::{FromPrimitive, IntoPrimitive};
@@ -32,6 +32,18 @@ pub enum ParameterVarInt {
 	Unknown(u64),
 }
 
+impl ParameterVarInt {
+	/// Whether `version` defines this option. Only defined options must be unique.
+	fn defined(self, version: Version) -> bool {
+		match self {
+			Self::MaxRequestId => matches!(version, Version::Draft14 | Version::Draft15 | Version::Draft16),
+			Self::HopId | Self::RelayCost => super::cluster::supported(version),
+			Self::Unknown(_) => false,
+			Self::MaxAuthTokenCacheSize | Self::Solicit | Self::Hidden => true,
+		}
+	}
+}
+
 #[derive(Debug, Copy, Clone, FromPrimitive, IntoPrimitive, Eq, Hash, PartialEq)]
 #[repr(u64)]
 pub enum ParameterBytes {
@@ -82,20 +94,21 @@ impl Decode<Version> for Parameters {
 
 					if kind % 2 == 0 {
 						let kind = ParameterVarInt::from(kind);
-						match vars.entry(kind) {
-							hash_map::Entry::Occupied(_) => return Err(DecodeError::Duplicate),
-							hash_map::Entry::Vacant(entry) => entry.insert(u64::decode(&mut r, version)?),
-						};
+						// Unknown SETUP options may repeat, but their values must still decode.
+						if kind.defined(version) && vars.contains_key(&kind) {
+							return Err(DecodeError::Duplicate);
+						}
+						vars.insert(kind, u64::decode(&mut r, version)?);
 					} else {
 						let kind = ParameterBytes::from(kind);
 						let val = Vec::<u8>::decode(&mut r, version)?;
 						if val.len() > MAX_KVP_VALUE_LEN {
 							return Err(DecodeError::BoundsExceeded);
 						}
-						match bytes.entry(kind) {
-							hash_map::Entry::Occupied(_) => return Err(DecodeError::Duplicate),
-							hash_map::Entry::Vacant(entry) => entry.insert(val),
-						};
+						if !matches!(kind, ParameterBytes::Unknown(_)) && bytes.contains_key(&kind) {
+							return Err(DecodeError::Duplicate);
+						}
+						bytes.insert(kind, val);
 					}
 				}
 			}
@@ -119,20 +132,21 @@ impl Decode<Version> for Parameters {
 
 					if abs % 2 == 0 {
 						let kind = ParameterVarInt::from(abs);
-						match vars.entry(kind) {
-							hash_map::Entry::Occupied(_) => return Err(DecodeError::Duplicate),
-							hash_map::Entry::Vacant(entry) => entry.insert(u64::decode(&mut r, version)?),
-						};
+						// Unknown SETUP options may repeat, but their values must still decode.
+						if kind.defined(version) && vars.contains_key(&kind) {
+							return Err(DecodeError::Duplicate);
+						}
+						vars.insert(kind, u64::decode(&mut r, version)?);
 					} else {
 						let kind = ParameterBytes::from(abs);
 						let val = Vec::<u8>::decode(&mut r, version)?;
 						if val.len() > MAX_KVP_VALUE_LEN {
 							return Err(DecodeError::BoundsExceeded);
 						}
-						match bytes.entry(kind) {
-							hash_map::Entry::Occupied(_) => return Err(DecodeError::Duplicate),
-							hash_map::Entry::Vacant(entry) => entry.insert(val),
-						};
+						if !matches!(kind, ParameterBytes::Unknown(_)) && bytes.contains_key(&kind) {
+							return Err(DecodeError::Duplicate);
+						}
+						bytes.insert(kind, val);
 					}
 				}
 			}
@@ -598,6 +612,94 @@ mod tests {
 	use super::*;
 	use bytes::{Buf, Bytes, BytesMut};
 
+	const SETUP_VERSIONS: [Version; 9] = [
+		Version::Draft14,
+		Version::Draft15,
+		Version::Draft16,
+		Version::Draft17,
+		Version::Draft18,
+		Version::Draft19,
+		Version::Draft20,
+		Version::Draft21,
+		Version::Draft22,
+	];
+
+	fn legacy(version: Version) -> bool {
+		matches!(version, Version::Draft14 | Version::Draft15 | Version::Draft16)
+	}
+
+	/// A SETUP block carrying the option `kind` twice.
+	fn repeated_setup_option(kind: u64, version: Version) -> Vec<u8> {
+		let mut buf = Vec::new();
+		if legacy(version) {
+			2u64.encode(&mut buf, version).unwrap();
+		}
+		kind.encode(&mut buf, version).unwrap();
+		for i in 0..2 {
+			if i > 0 {
+				let next = if matches!(version, Version::Draft14 | Version::Draft15) {
+					kind
+				} else {
+					0
+				};
+				next.encode(&mut buf, version).unwrap();
+			}
+			if kind.is_multiple_of(2) {
+				1u64.encode(&mut buf, version).unwrap();
+			} else {
+				vec![1u8].encode(&mut buf, version).unwrap();
+			}
+		}
+		buf
+	}
+
+	#[test]
+	fn setup_only_defined_options_must_be_unique() {
+		// Whether the option must be unique on a given draft.
+		type Unique = fn(Version) -> bool;
+		let cases: [(u64, Unique); 11] = [
+			(0x20, |_| false),
+			(0x21, |_| false),
+			(0x9d, |_| false),
+			(0x11c, |_| false),
+			(ParameterVarInt::MaxRequestId.into(), legacy),
+			(ParameterVarInt::HopId.into(), |v| !legacy(v)),
+			(ParameterVarInt::RelayCost.into(), |v| !legacy(v)),
+			(ParameterVarInt::MaxAuthTokenCacheSize.into(), |_| true),
+			(ParameterVarInt::Solicit.into(), |_| true),
+			(ParameterVarInt::Hidden.into(), |_| true),
+			(ParameterBytes::Implementation.into(), |_| true),
+		];
+		for version in SETUP_VERSIONS {
+			for (kind, unique) in cases {
+				let buf = repeated_setup_option(kind, version);
+				let result = Parameters::decode(&mut buf.as_slice(), version);
+				if unique(version) {
+					assert!(
+						matches!(result, Err(DecodeError::Duplicate)),
+						"{kind:#x} on {version:?}"
+					);
+				} else {
+					result.unwrap_or_else(|err| panic!("{kind:#x} on {version:?}: {err:?}"));
+				}
+			}
+		}
+	}
+
+	#[test]
+	fn setup_repeated_options_require_valid_values() {
+		for version in [
+			Version::Draft17,
+			Version::Draft18,
+			Version::Draft19,
+			Version::Draft20,
+			Version::Draft21,
+			Version::Draft22,
+		] {
+			assert!(Parameters::decode(&mut &[0x21, 1, b'a', 0, 2, b'b'][..], version).is_err());
+		}
+	}
+
 	// ---- Setup Parameters tests (unchanged) ----
 
 	#[test]