From d245ba0923cf5dd4466ced841c07ec1a1ec8ff52 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Sat, 10 Oct 2026 09:22:48 -0700 Subject: [PATCH 1/5] chore: claim repeated SETUP options backport Co-Authored-By: GPT-6 From 5149f97ecd024b9571c541ad6728f3198c952121 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Sat, 10 Oct 2026 10:03:42 -0700 Subject: [PATCH 2/5] fix(net): accept repeated unknown SETUP options on release Co-Authored-By: GPT-6 --- js/net/src/ietf/ietf.test.ts | 53 +++++++++++++++++ js/net/src/ietf/parameters.ts | 11 ++-- rs/moq-net/src/ietf/parameters.rs | 96 +++++++++++++++++++++++++------ 3 files changed, 139 insertions(+), 21 deletions(-) diff --git a/js/net/src/ietf/ietf.test.ts b/js/net/src/ietf/ietf.test.ts index fef65c8f9f..76b4d746ff 100644 --- a/js/net/src/ietf/ietf.test.ts +++ b/js/net/src/ietf/ietf.test.ts @@ -1945,3 +1945,56 @@ 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(); }); + +for (const version of [ + 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, +]) { + for (const kind of [0x20n, 0x21n, 0x9dn, 0x11cn]) { + test(`SETUP ${version}: repeated unknown option ${kind}`, async () => { + const { stream, written } = createTestWritableStream(); + const writer = new Writer(stream, version); + if (version <= Version.DRAFT_16) 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; + await SetupOptions.decode(new Reader(undefined, concatChunks(written), version), version); + }); + } +} + +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}: known options cannot repeat`, async () => { + for (const bytes of [new Uint8Array([4, 1, 0, 2]), new Uint8Array([7, 1, 97, 0, 1, 98])]) { + await expect(SetupOptions.decode(new Reader(undefined, bytes, version), version)).rejects.toThrow( + /duplicate/, + ); + } + }); + 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..60175c1594 100644 --- a/js/net/src/ietf/parameters.ts +++ b/js/net/src/ietf/parameters.ts @@ -21,6 +21,9 @@ export const SetupOption = { Hidden: 0x40b5cn, } as const; +// Unknown SETUP options may repeat, including GREASE. +const KNOWN_SETUP_OPTIONS: readonly bigint[] = Object.values(SetupOption); + /// Setup Options — used in SETUP messages. /// /// In d14-d16 these are count-prefixed ("Setup Parameters"). @@ -140,13 +143,13 @@ export class SetupOptions { i++; if (id % 2n === 0n) { - if (params.vars.has(id)) { + if (KNOWN_SETUP_OPTIONS.includes(id) && 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 (KNOWN_SETUP_OPTIONS.includes(id) && params.bytes.has(id)) { throw new Error(`duplicate parameter id: ${id.toString()}`); } const size = await r.u53(); @@ -169,13 +172,13 @@ export class SetupOptions { } if (id % 2n === 0n) { - if (params.vars.has(id)) { + if (KNOWN_SETUP_OPTIONS.includes(id) && 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 (KNOWN_SETUP_OPTIONS.includes(id) && 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..a97f0e1a21 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}; @@ -82,20 +82,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 !matches!(kind, ParameterVarInt::Unknown(_)) && 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 +120,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 !matches!(kind, ParameterVarInt::Unknown(_)) && 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 +600,66 @@ mod tests { use super::*; use bytes::{Buf, Bytes, BytesMut}; + #[test] + fn setup_allows_repeated_unknown_options() { + for version in [ + Version::Draft14, + Version::Draft15, + Version::Draft16, + Version::Draft17, + Version::Draft18, + Version::Draft19, + Version::Draft20, + Version::Draft21, + Version::Draft22, + ] { + for kind in [0x20u64, 0x21, 0x9d, 0x11c] { + let mut buf = Vec::new(); + if matches!(version, Version::Draft14 | Version::Draft15 | Version::Draft16) { + 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 % 2 == 0 { + 1u64.encode(&mut buf, version).unwrap(); + } else { + vec![1u8].encode(&mut buf, version).unwrap(); + } + } + Parameters::decode(&mut buf.as_slice(), version).expect("unknown SETUP options may repeat"); + } + } + } + + #[test] + fn setup_repeated_options_require_valid_values() { + for version in [ + Version::Draft17, + Version::Draft18, + Version::Draft19, + Version::Draft20, + Version::Draft21, + Version::Draft22, + ] { + for bytes in [&[4, 1, 0, 2][..], &[7, 1, b'a', 0, 1, b'b'][..]] { + assert!(matches!( + Parameters::decode(&mut &bytes[..], version), + Err(DecodeError::Duplicate) + )); + } + assert!(Parameters::decode(&mut &[0x21, 1, b'a', 0, 2, b'b'][..], version).is_err()); + } + } + + // ---- Setup Parameters tests (unchanged) ---- #[test] From 69ab205e58b0f1398538335997979d6dae27572f Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Sat, 10 Oct 2026 15:51:24 -0700 Subject: [PATCH 3/5] style(net): format SETUP repetition regression Co-Authored-By: GPT-6 --- rs/moq-net/src/ietf/parameters.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/rs/moq-net/src/ietf/parameters.rs b/rs/moq-net/src/ietf/parameters.rs index a97f0e1a21..bf3432499d 100644 --- a/rs/moq-net/src/ietf/parameters.rs +++ b/rs/moq-net/src/ietf/parameters.rs @@ -659,7 +659,6 @@ mod tests { } } - // ---- Setup Parameters tests (unchanged) ---- #[test] From ed7441b71ef279fadb6dd523a3c05ecddde28d42 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Sat, 10 Oct 2026 22:32:29 -0700 Subject: [PATCH 4/5] fix(net): classify retired setup options by draft Co-Authored-By: GPT-6 Astra --- js/net/src/ietf/ietf.test.ts | 25 +++++++++++++++++++++++ js/net/src/ietf/parameters.ts | 2 +- rs/moq-net/src/ietf/parameters.rs | 34 ++++++++++++++++++++++++++++++- 3 files changed, 59 insertions(+), 2 deletions(-) diff --git a/js/net/src/ietf/ietf.test.ts b/js/net/src/ietf/ietf.test.ts index 76b4d746ff..5c6e9f0f36 100644 --- a/js/net/src/ietf/ietf.test.ts +++ b/js/net/src/ietf/ietf.test.ts @@ -1998,3 +1998,28 @@ for (const version of [ await expect(SetupOptions.decode(new Reader(undefined, bytes, version), version)).rejects.toThrow(); }); } + +for (const version of [ + 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, +]) { + test(`SETUP ${version}: retired MAX_REQUEST_ID may repeat`, async () => { + const bytes = new Uint8Array( + version <= Version.DRAFT_15 + ? [2, 2, 1, 2, 2] + : version === Version.DRAFT_16 + ? [2, 2, 1, 0, 2] + : [2, 1, 0, 2], + ); + const decoded = SetupOptions.decode(new Reader(undefined, bytes, version), version); + if (version <= Version.DRAFT_16) await expect(decoded).rejects.toThrow(/duplicate/); + else await decoded; + }); +} diff --git a/js/net/src/ietf/parameters.ts b/js/net/src/ietf/parameters.ts index 60175c1594..19a6c69c52 100644 --- a/js/net/src/ietf/parameters.ts +++ b/js/net/src/ietf/parameters.ts @@ -143,7 +143,7 @@ export class SetupOptions { i++; if (id % 2n === 0n) { - if (KNOWN_SETUP_OPTIONS.includes(id) && params.vars.has(id)) { + if (id !== SetupOption.MaxRequestId && KNOWN_SETUP_OPTIONS.includes(id) && params.vars.has(id)) { throw new Error(`duplicate parameter id: ${id.toString()}`); } const varint = await r.u62(); diff --git a/rs/moq-net/src/ietf/parameters.rs b/rs/moq-net/src/ietf/parameters.rs index bf3432499d..f7c3527206 100644 --- a/rs/moq-net/src/ietf/parameters.rs +++ b/rs/moq-net/src/ietf/parameters.rs @@ -121,7 +121,9 @@ impl Decode for Parameters { if abs % 2 == 0 { let kind = ParameterVarInt::from(abs); // Unknown SETUP options may repeat, but their values must still decode. - if !matches!(kind, ParameterVarInt::Unknown(_)) && vars.contains_key(&kind) { + if !matches!(kind, ParameterVarInt::Unknown(_) | ParameterVarInt::MaxRequestId) + && vars.contains_key(&kind) + { return Err(DecodeError::Duplicate); } vars.insert(kind, u64::decode(&mut r, version)?); @@ -600,6 +602,36 @@ mod tests { use super::*; use bytes::{Buf, Bytes, BytesMut}; + #[test] + fn setup_retired_max_request_id_may_repeat() { + for version in [ + Version::Draft14, + Version::Draft15, + Version::Draft16, + Version::Draft17, + Version::Draft18, + Version::Draft19, + Version::Draft20, + Version::Draft21, + Version::Draft22, + ] { + let bytes: &[u8] = match version { + Version::Draft14 | Version::Draft15 => &[2, 2, 1, 2, 2], + Version::Draft16 => &[2, 2, 1, 0, 2], + _ => &[2, 1, 0, 2], + }; + let result = Parameters::decode(&mut &bytes[..], version); + match version { + Version::Draft14 | Version::Draft15 | Version::Draft16 => { + assert!(matches!(result, Err(DecodeError::Duplicate))) + } + _ => { + result.expect("retired SETUP option is unknown"); + } + } + } + } + #[test] fn setup_allows_repeated_unknown_options() { for version in [ From e7c26512646e9c375d583da9644c5a287de278e1 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Sun, 11 Oct 2026 08:56:11 -0700 Subject: [PATCH 5/5] fix(net): classify SETUP options by negotiated draft HOP_ID and RELAY_COST are only negotiated on draft-17+, so on drafts 14-16 they are unknown options and may repeat, just like MAX_REQUEST_ID from draft-17 on. Replace the per-branch special cases with one draft-aware predicate in Rust and JS, and cover every named option on every draft. Co-Authored-By: Claude Opus 5.5 --- js/net/src/ietf/ietf.test.ts | 98 ++++++++++---------- js/net/src/ietf/parameters.ts | 27 ++++-- rs/moq-net/src/ietf/parameters.rs | 147 ++++++++++++++++-------------- 3 files changed, 145 insertions(+), 127 deletions(-) diff --git a/js/net/src/ietf/ietf.test.ts b/js/net/src/ietf/ietf.test.ts index 5c6e9f0f36..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"; @@ -1946,7 +1946,7 @@ test("Subscribe v16: rejects INCLUDE_PROPERTIES", async () => { await expect(decodeVersioned(body, Subscribe.Subscribe.decode, Version.DRAFT_16)).rejects.toThrow(); }); -for (const version of [ +const SETUP_VERSIONS = [ Version.DRAFT_14, Version.DRAFT_15, Version.DRAFT_16, @@ -1956,24 +1956,50 @@ for (const version of [ Version.DRAFT_20, Version.DRAFT_21, Version.DRAFT_22, -]) { - for (const kind of [0x20n, 0x21n, 0x9dn, 0x11cn]) { - test(`SETUP ${version}: repeated unknown option ${kind}`, async () => { - const { stream, written } = createTestWritableStream(); - const writer = new Writer(stream, version); - if (version <= Version.DRAFT_16) 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; - await SetupOptions.decode(new Reader(undefined, concatChunks(written), version), version); +] 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; }); } } @@ -1986,40 +2012,8 @@ for (const version of [ Version.DRAFT_21, Version.DRAFT_22, ]) { - test(`SETUP ${version}: known options cannot repeat`, async () => { - for (const bytes of [new Uint8Array([4, 1, 0, 2]), new Uint8Array([7, 1, 97, 0, 1, 98])]) { - await expect(SetupOptions.decode(new Reader(undefined, bytes, version), version)).rejects.toThrow( - /duplicate/, - ); - } - }); 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(); }); } - -for (const version of [ - 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, -]) { - test(`SETUP ${version}: retired MAX_REQUEST_ID may repeat`, async () => { - const bytes = new Uint8Array( - version <= Version.DRAFT_15 - ? [2, 2, 1, 2, 2] - : version === Version.DRAFT_16 - ? [2, 2, 1, 0, 2] - : [2, 1, 0, 2], - ); - const decoded = SetupOptions.decode(new Reader(undefined, bytes, version), version); - if (version <= Version.DRAFT_16) await expect(decoded).rejects.toThrow(/duplicate/); - else await decoded; - }); -} diff --git a/js/net/src/ietf/parameters.ts b/js/net/src/ietf/parameters.ts index 19a6c69c52..a6d5a52065 100644 --- a/js/net/src/ietf/parameters.ts +++ b/js/net/src/ietf/parameters.ts @@ -21,8 +21,23 @@ export const SetupOption = { Hidden: 0x40b5cn, } as const; -// Unknown SETUP options may repeat, including GREASE. -const KNOWN_SETUP_OPTIONS: readonly bigint[] = Object.values(SetupOption); +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. /// @@ -143,13 +158,13 @@ export class SetupOptions { i++; if (id % 2n === 0n) { - if (id !== SetupOption.MaxRequestId && KNOWN_SETUP_OPTIONS.includes(id) && 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 (KNOWN_SETUP_OPTIONS.includes(id) && 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(); @@ -172,13 +187,13 @@ export class SetupOptions { } if (id % 2n === 0n) { - if (KNOWN_SETUP_OPTIONS.includes(id) && 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 (KNOWN_SETUP_OPTIONS.includes(id) && 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 f7c3527206..ca4b67e49c 100644 --- a/rs/moq-net/src/ietf/parameters.rs +++ b/rs/moq-net/src/ietf/parameters.rs @@ -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 { @@ -83,7 +95,7 @@ impl Decode for Parameters { if kind % 2 == 0 { let kind = ParameterVarInt::from(kind); // Unknown SETUP options may repeat, but their values must still decode. - if !matches!(kind, ParameterVarInt::Unknown(_)) && vars.contains_key(&kind) { + if kind.defined(version) && vars.contains_key(&kind) { return Err(DecodeError::Duplicate); } vars.insert(kind, u64::decode(&mut r, version)?); @@ -121,9 +133,7 @@ impl Decode for Parameters { if abs % 2 == 0 { let kind = ParameterVarInt::from(abs); // Unknown SETUP options may repeat, but their values must still decode. - if !matches!(kind, ParameterVarInt::Unknown(_) | ParameterVarInt::MaxRequestId) - && vars.contains_key(&kind) - { + if kind.defined(version) && vars.contains_key(&kind) { return Err(DecodeError::Duplicate); } vars.insert(kind, u64::decode(&mut r, version)?); @@ -602,71 +612,76 @@ mod tests { use super::*; use bytes::{Buf, Bytes, BytesMut}; - #[test] - fn setup_retired_max_request_id_may_repeat() { - for version in [ - Version::Draft14, - Version::Draft15, - Version::Draft16, - Version::Draft17, - Version::Draft18, - Version::Draft19, - Version::Draft20, - Version::Draft21, - Version::Draft22, - ] { - let bytes: &[u8] = match version { - Version::Draft14 | Version::Draft15 => &[2, 2, 1, 2, 2], - Version::Draft16 => &[2, 2, 1, 0, 2], - _ => &[2, 1, 0, 2], - }; - let result = Parameters::decode(&mut &bytes[..], version); - match version { - Version::Draft14 | Version::Draft15 | Version::Draft16 => { - assert!(matches!(result, Err(DecodeError::Duplicate))) - } - _ => { - result.expect("retired SETUP option is unknown"); - } + 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_allows_repeated_unknown_options() { - for version in [ - Version::Draft14, - Version::Draft15, - Version::Draft16, - Version::Draft17, - Version::Draft18, - Version::Draft19, - Version::Draft20, - Version::Draft21, - Version::Draft22, - ] { - for kind in [0x20u64, 0x21, 0x9d, 0x11c] { - let mut buf = Vec::new(); - if matches!(version, Version::Draft14 | Version::Draft15 | Version::Draft16) { - 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 % 2 == 0 { - 1u64.encode(&mut buf, version).unwrap(); - } else { - vec![1u8].encode(&mut buf, version).unwrap(); - } + 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:?}")); } - Parameters::decode(&mut buf.as_slice(), version).expect("unknown SETUP options may repeat"); } } } @@ -681,12 +696,6 @@ mod tests { Version::Draft21, Version::Draft22, ] { - for bytes in [&[4, 1, 0, 2][..], &[7, 1, b'a', 0, 1, b'b'][..]] { - assert!(matches!( - Parameters::decode(&mut &bytes[..], version), - Err(DecodeError::Duplicate) - )); - } assert!(Parameters::decode(&mut &[0x21, 1, b'a', 0, 2, b'b'][..], version).is_err()); } }