diff --git a/js/net/src/ietf/ietf.test.ts b/js/net/src/ietf/ietf.test.ts index 3120418b4b..0eb6d5c70b 100644 --- a/js/net/src/ietf/ietf.test.ts +++ b/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, diff --git a/js/net/src/ietf/parameters.ts b/js/net/src/ietf/parameters.ts index 263d4080d7..abf91a6ea6 100644 --- a/js/net/src/ietf/parameters.ts +++ b/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; /** 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; @@ -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()}`); diff --git a/js/net/src/ietf/subscribe_namespace.ts b/js/net/src/ietf/subscribe_namespace.ts index a36090c910..f0b699c529 100644 --- a/js/net/src/ietf/subscribe_namespace.ts +++ b/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 }); } diff --git a/rs/moq-net/src/ietf/fetch.rs b/rs/moq-net/src/ietf/fetch.rs index 394857f4d8..af86f128bd 100644 --- a/rs/moq-net/src/ietf/fetch.rs +++ b/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, - 0x0A => fill_timeout: Option, + 0x0A => fill_timeout: Option where !matches!(version, Version::Draft15 | Version::Draft16 | Version::Draft17), 0x20 => subscriber_priority: Option, 0x22 => group_order: Option, - 0x25 => subgroup_filter: Vec, - 0x26 => object_id_filter: Vec, - 0x27 => priority_filter: Vec, - 0x28 => object_property_filter: Vec, + 0x25 => subgroup_filter: Vec where has_range_filters(version), + 0x26 => object_id_filter: Vec where has_range_filters(version), + 0x27 => priority_filter: Vec where has_range_filters(version), + 0x28 => object_property_filter: Vec 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::::decode(buf, version)?; decode_params!(buf, version, 0x03 => _authorization_token: Vec, - 0x0A => fill_timeout: Option, + 0x0A => fill_timeout: Option where !matches!(version, Version::Draft15 | Version::Draft16 | Version::Draft17), 0x20 => subscriber_priority: Option, 0x21 => filter: Option, 0x22 => group_order: Option, - 0x25 => subgroup_filter: Vec, - 0x26 => object_id_filter: Vec, - 0x27 => priority_filter: Vec, - 0x28 => object_property_filter: Vec, + 0x25 => subgroup_filter: Vec where has_range_filters(version), + 0x26 => object_id_filter: Vec where has_range_filters(version), + 0x27 => priority_filter: Vec where has_range_filters(version), + 0x28 => object_property_filter: Vec where has_range_filters(version), 0x35 => _include_properties: Option, ); let range_filters = [ diff --git a/rs/moq-net/src/ietf/parameters.rs b/rs/moq-net/src/ietf/parameters.rs index ca4b67e49c..b51ff74962 100644 --- a/rs/moq-net/src/ietf/parameters.rs +++ b/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`. /// @@ -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 { + >::decode($r, _version)?; + } else { + let _value = 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, DecodeError> = (|| { + decode_params!(&mut bytes, version, 0x20 => priority: Option); + Ok(priority) + })(); + assert_eq!(result.unwrap(), Some(7)); + assert!(bytes.is_empty()); + } } } diff --git a/rs/moq-net/src/ietf/publish.rs b/rs/moq-net/src/ietf/publish.rs index 379dbb7bd1..ec55793155 100644 --- a/rs/moq-net/src/ietf/publish.rs +++ b/rs/moq-net/src/ietf/publish.rs @@ -336,28 +336,17 @@ impl Message for Publish<'_> { // parameter fails the whole message, which would kill the session instead of // letting the request reach its NOT_SUPPORTED response. decode_params!(r, version, - 0x02 => object_delivery_timeout: Option, + 0x02 => _object_delivery_timeout: Option where version == Version::Draft15 || Filter::is_draft20(version), 0x03 => _authorization_token: Vec, - 0x06 => subgroup_delivery_timeout: Option, + 0x06 => _subgroup_delivery_timeout: Option where Filter::is_draft20(version), 0x08 => _expires: Option, 0x09 => largest_location: Option, 0x10 => forward: Option, - 0x20 => subscriber_priority: Option, - 0x21 => filter: Option, + 0x20 => _subscriber_priority: Option where Filter::is_draft20(version), + 0x21 => _filter: Option where Filter::is_draft20(version), 0x22 => group_order: Option, ); - // The values are dropped: we refuse the PUBLISH itself, so the subscription - // settings it proposes never take effect. They still have to be consumed. - let subscription_params = [ - object_delivery_timeout.is_some(), - subgroup_delivery_timeout.is_some(), - subscriber_priority.is_some(), - filter.is_some(), - ]; - if subscription_params.contains(&true) && !Filter::is_draft20(version) { - return Err(DecodeError::InvalidValue); - } let mut properties = Properties::decode(r, version)?; properties.group_order = properties.group_order.or(group_order); @@ -508,6 +497,17 @@ impl Message for PublishError<'_> { #[cfg(test)] mod tests { + #[test] + fn publish_draft15_ignores_subscription_only_parameters() { + for parameter in [&[0x02, 7][..], &[0x20, 0x41, 0], &[0x21, 1, 255]] { + let mut body = vec![1, 1, 1, b't', 1, b'v', 1, 2]; + body.extend_from_slice(parameter); + body.extend_from_slice(&[0x10, 0]); + let decoded: Publish = decode_message(&body, Version::Draft15).unwrap(); + assert!(!decoded.forward); + } + } + use super::*; /// Draft-20 moved the subscription parameters into PUBLISH. We refuse the request diff --git a/rs/moq-net/src/ietf/publish_namespace.rs b/rs/moq-net/src/ietf/publish_namespace.rs index ae090688bd..a305380bc5 100644 --- a/rs/moq-net/src/ietf/publish_namespace.rs +++ b/rs/moq-net/src/ietf/publish_namespace.rs @@ -38,10 +38,11 @@ impl PublishNamespace<'_> { let track_namespace = decode_namespace(r, version)?; // The token is ignored: the session's grant is what authorizes the request. + // The cluster extension defines its parameters only where it can be negotiated. decode_params!(r, version, 0x03 => _authorization_token: Vec, - cluster::HOP_PATH => hops: Option, - cluster::ROUTE_COST => cost: Option, + cluster::HOP_PATH => hops: Option where cluster::supported(version), + cluster::ROUTE_COST => cost: Option where cluster::supported(version), ); let cluster = match negotiated { @@ -572,6 +573,25 @@ mod tests { assert!(decode_message::(&[0x02, 0x00], Version::Draft16).is_err()); } + /// Drafts 14 and 15 predate the cluster extension, so its parameters are unrecognized + /// there and ignored, repeats included. + #[test] + fn legacy_ignores_cluster_parameters() { + for version in [Version::Draft14, Version::Draft15] { + let msg = PublishNamespace { + request_id: RequestId(1), + track_namespace: Path::new("test"), + cluster: None, + }; + let mut encoded = encode_message(&msg, version); + // Replace the empty parameter block with two ROUTE_COST (0x40B58) parameters. + assert_eq!(encoded.pop(), Some(0)); + encoded.extend_from_slice(&[2, 0x80, 0x04, 0x0B, 0x58, 5, 0x80, 0x04, 0x0B, 0x58, 6]); + let decoded: PublishNamespace = decode_message(&encoded, version).unwrap(); + assert!(decoded.cluster.is_none(), "{version}"); + } + } + #[test] fn test_announce_rejects_parameters() { #[rustfmt::skip] diff --git a/rs/moq-net/src/ietf/subscribe.rs b/rs/moq-net/src/ietf/subscribe.rs index f7c584cd90..7bf472ca96 100644 --- a/rs/moq-net/src/ietf/subscribe.rs +++ b/rs/moq-net/src/ietf/subscribe.rs @@ -37,11 +37,6 @@ pub struct Subscribe<'a> { pub range_filters: bool, } -/// NEW_GROUP_REQUEST (0x32) arrived in draft-16. -fn has_new_group_request(version: Version) -> bool { - !matches!(version, Version::Draft14 | Version::Draft15) -} - /// The Range Filters (0x25-0x29) arrived in draft-19. pub(super) fn has_range_filters(version: Version) -> bool { !matches!( @@ -91,19 +86,19 @@ impl Message for Subscribe<'_> { decode_params!(r, version, 0x02 => _object_delivery_timeout: Option, 0x03 => _authorization_token: Vec, - 0x04 => rendezvous_timeout: Option, - 0x06 => _subgroup_delivery_timeout: Option, + 0x04 => rendezvous_timeout: Option where !matches!(version, Version::Draft15 | Version::Draft16), + 0x06 => _subgroup_delivery_timeout: Option where !matches!(version, Version::Draft14 | Version::Draft15 | Version::Draft16 | Version::Draft17), 0x10 => forward: Option, 0x20 => subscriber_priority: Option, 0x21 => filter: Option, 0x22 => group_order: Option, - 0x23 => fill: Option, - 0x25 => subgroup_filter: Vec, - 0x26 => object_id_filter: Vec, - 0x27 => priority_filter: Vec, - 0x28 => object_property_filter: Vec, - 0x32 => new_group_request: Option, - 0x35 => include_properties: Option, + 0x23 => fill: Option where Filter::is_draft20(version), + 0x25 => subgroup_filter: Vec where has_range_filters(version), + 0x26 => object_id_filter: Vec where has_range_filters(version), + 0x27 => priority_filter: Vec where has_range_filters(version), + 0x28 => object_property_filter: Vec where has_range_filters(version), + 0x32 => _new_group_request: Option, + 0x35 => include_properties: Option where Filter::is_draft20(version), ); let range_filters = [ @@ -115,25 +110,9 @@ impl Message for Subscribe<'_> { .iter() .any(|filter| !filter.is_empty()); - // An unknown message parameter is a protocol violation, so each stays rejected - // on the drafts that predate it rather than being quietly tolerated. - if ((fill.is_some() || include_properties.is_some()) && !Filter::is_draft20(version)) - || (range_filters && !has_range_filters(version)) - || (new_group_request.is_some() && !has_new_group_request(version)) - { - return Err(DecodeError::InvalidValue); - } - // Defaults to 1, so an absent parameter means the subscriber wants them. let properties_wanted = include_properties.unwrap_or(true); - // RENDEZVOUS_TIMEOUT arrived in draft-17; 0x04 means MAX_CACHE_DURATION in - // draft-15, which is a publisher parameter with no business in a SUBSCRIBE. - // An unknown message parameter is a protocol violation, so reject it there. - if rendezvous_timeout.is_some() && matches!(version, Version::Draft15 | Version::Draft16) { - return Err(DecodeError::InvalidValue); - } - // The value is deliberately dropped: we always answer a SUBSCRIBE with what is // published right now, which is the shorter timeout the draft lets a relay pick. // We still have to parse it, or the parameter alone would kill the session. @@ -460,18 +439,13 @@ impl Message for SubscribeUpdate { decode_params!(r, version, 0x02 => _object_delivery_timeout: Option, 0x03 => _authorization_token: Vec, - 0x06 => _subgroup_delivery_timeout: Option, + 0x06 => _subgroup_delivery_timeout: Option where !matches!(version, Version::Draft14 | Version::Draft15 | Version::Draft16 | Version::Draft17), 0x10 => forward: Option, 0x20 => subscriber_priority: Option, 0x21 => _filter: Option, - 0x32 => new_group_request: Option, + 0x32 => _new_group_request: Option, ); - // NEW_GROUP_REQUEST arrived in draft-16. - if new_group_request.is_some() && !has_new_group_request(version) { - return Err(DecodeError::InvalidValue); - } - let subscriber_priority = subscriber_priority.unwrap_or(128); let forward = forward.unwrap_or(true); @@ -496,16 +470,16 @@ impl Message for SubscribeUpdate { decode_params!(r, version, 0x02 => _object_delivery_timeout: Option, 0x03 => _authorization_token: Vec, - 0x06 => _subgroup_delivery_timeout: Option, + 0x06 => _subgroup_delivery_timeout: Option where !matches!(version, Version::Draft14 | Version::Draft15 | Version::Draft16 | Version::Draft17), 0x10 => forward: Option, 0x20 => subscriber_priority: Option, 0x21 => _filter: Option, - 0x23 => fill: Option, - 0x25 => subgroup_filter: Vec, - 0x26 => object_id_filter: Vec, - 0x27 => priority_filter: Vec, - 0x28 => object_property_filter: Vec, - 0x29 => track_property_filter: Vec, + 0x23 => fill: Option where Filter::is_draft20(version), + 0x25 => subgroup_filter: Vec where has_range_filters(version), + 0x26 => object_id_filter: Vec where has_range_filters(version), + 0x27 => priority_filter: Vec where has_range_filters(version), + 0x28 => object_property_filter: Vec where has_range_filters(version), + 0x29 => track_property_filter: Vec where has_range_filters(version), 0x32 => _new_group_request: Option, ); @@ -640,14 +614,11 @@ mod tests { } } - /// 0x04 only means RENDEZVOUS_TIMEOUT from draft-17 on; in draft-15 it is - /// MAX_CACHE_DURATION, a publisher parameter that has no business in a SUBSCRIBE. + /// Draft-16 has no parameter 0x04; draft-15 ignores it on SUBSCRIBE. #[test] - fn rendezvous_timeout_is_rejected_before_draft17() { - for version in [Version::Draft15, Version::Draft16] { - let encoded = subscribe_with_rendezvous(0, version); - decode_message::(&encoded, version).expect_err(&format!("{version} must reject parameter 0x04")); - } + fn rendezvous_timeout_is_rejected_in_draft16() { + let encoded = subscribe_with_rendezvous(0, Version::Draft16); + decode_message::(&encoded, Version::Draft16).expect_err("draft-16 rejects parameter 0x04"); } /// The first message parameter key on an encoded SUBSCRIBE, or `None` when it carries no @@ -1441,13 +1412,33 @@ mod tests { } } - /// A parameter from a later draft is still an unknown parameter, which the draft makes - /// a protocol violation. + /// Draft-15 accepts NEW_GROUP_REQUEST and skips future definitions before parsing them. + #[test] + fn subscribe_draft15_recognizes_only_its_parameters() { + for parameter in [ + &[0x32, 7][..], + &[0x04, 0], + &[0x23, 1, 255], + &[0x25, 1, 255], + &[0x35, 1, 255], + ] { + let mut params = vec![2]; + params.extend_from_slice(parameter); + params.extend_from_slice(&[0x20, 7]); + let decoded: Subscribe = decode_message(&subscribe_body(¶ms), Version::Draft15).unwrap(); + assert_eq!(decoded.subscriber_priority, 7); + assert!(decoded.fill.is_none()); + assert!(!decoded.range_filters); + } + let body = [2, 1, 1, 0x32, 7]; + decode_message::(&body, Version::Draft15).unwrap(); + } + #[test] fn subscribe_refuses_parameters_before_their_draft() { for (version, params) in [ - // NEW_GROUP_REQUEST arrived in draft-16. - (Version::Draft15, &[0x01, 0x32, 0x00][..]), + // SUBGROUP_DELIVERY_TIMEOUT arrived in draft-18. + (Version::Draft16, &[0x01, 0x06, 0x00][..]), // The Range Filters arrived in draft-19. (Version::Draft18, &[0x01, 0x25, 0x00][..]), // INCLUDE_PROPERTIES arrived in draft-20. diff --git a/rs/moq-net/src/ietf/subscribe_namespace.rs b/rs/moq-net/src/ietf/subscribe_namespace.rs index 252ba94b6b..1084144d8f 100644 --- a/rs/moq-net/src/ietf/subscribe_namespace.rs +++ b/rs/moq-net/src/ietf/subscribe_namespace.rs @@ -138,10 +138,20 @@ impl Message for SubscribeNamespaceLegacy<'_> { }; // The token is ignored: the session's grant is what authorizes the request. - decode_params!(r, version, - 0x03 => _authorization_token: Vec, - HIDDEN_PARAM => hidden: Option, - ); + let hidden = if version == Version::Draft14 { + decode_params!(r, version, + 0x03 => _authorization_token: Vec, + HIDDEN_PARAM => hidden: Option, + ); + hidden + } else { + decode_params!(r, version, + 0x03 => _authorization_token: Vec, + 0x10 => _forward: Option, + HIDDEN_PARAM => hidden: Option, + ); + hidden + }; Ok(Self { request_id, @@ -433,6 +443,36 @@ mod tests { assert!(super::super::PublishNamespace::decode_body(&mut bytes, version, false).is_err()); } + #[test] + fn forward_follows_the_draft() { + for version in [Version::Draft14, Version::Draft15, Version::Draft16, Version::Draft17] { + for value in [0u64, 1, 2] { + let mut buf = BytesMut::new(); + RequestId(1).encode(&mut buf, version).unwrap(); + if version == Version::Draft17 { + 0u64.encode(&mut buf, version).unwrap(); + } + encode_namespace(&mut buf, &Path::default(), version).unwrap(); + if matches!(version, Version::Draft16 | Version::Draft17) { + 1u64.encode(&mut buf, version).unwrap(); + } + 1u64.encode(&mut buf, version).unwrap(); + 0x10u64.encode(&mut buf, version).unwrap(); + value.encode(&mut buf, version).unwrap(); + let mut buf = buf.freeze(); + let result = SubscribeNamespaceLegacy::decode_msg(&mut buf, version); + assert_eq!( + result.is_ok(), + version == Version::Draft14 || value <= 1, + "{version} FORWARD={value}: {result:?}" + ); + if version == Version::Draft14 || value <= 1 { + assert!(buf.is_empty()); + } + } + } + } + #[test] fn message_ids() { // 0x11 through draft-17 (legacy), renumbered to 0x50 in draft-18 (#1542).