diff --git a/js/net/src/ietf/ietf.test.ts b/js/net/src/ietf/ietf.test.ts index fef65c8f9f..3120418b4b 100644 --- a/js/net/src/ietf/ietf.test.ts +++ b/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 { + 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(); + }); +} diff --git a/js/net/src/ietf/parameters.ts b/js/net/src/ietf/parameters.ts index 1d316a048e..a6d5a52065 100644 --- a/js/net/src/ietf/parameters.ts +++ b/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(); diff --git a/rs/moq-net/src/ietf/parameters.rs b/rs/moq-net/src/ietf/parameters.rs index 2e7330a801..ca4b67e49c 100644 --- a/rs/moq-net/src/ietf/parameters.rs +++ b/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 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::::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 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::::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 { + 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]