From 03451aa1a0dcb6d08780ded6c572877ed7c54beb Mon Sep 17 00:00:00 2001 From: HangPlayerEngineer Date: Thu, 8 Oct 2026 18:24:36 +0000 Subject: [PATCH] feat(moq-net): let a subscriber choose the SUBSCRIBE group order GROUP_ORDER was a module constant written unconditionally on both wire shapes, and `Subscribe` had no field for it, so no caller -- internal or public -- could ask for Ascending. The decode side already read the value and validated it, and then dropped it on the floor: a relay built on this parsed the subscriber's requested order correctly and could not act on it. Add `groupOrder` to `Subscribe` and its constructor options, defaulting to Descending, which is what every subscription sent before the field existed. Both encode paths write the field instead of the constant, and both decode paths pass the parsed value through. `GroupOrder.Default` (0x0) is the wire's "no preference". It is normalized to the client default in one place, the constructor, so the field a caller reads back is always a real order and 0 can never reach an encoder. That matters from draft-15, where GROUP_ORDER is a message parameter and Parameters rejects 0 as a protocol violation -- only draft-14's inline field can carry it at all, and that path is covered. Tests run on draft-14 and draft-20. The ascending case is a differential: it encodes the same message both ways and requires exactly one byte to differ, 0x02 -> 0x01, since a plain byte search would pass on requestId 1n alone. Each assertion is mutation-checked -- hardcoding either encode site, dropping the constructor normalization, and dropping the decode pass-through each turn a distinct test red. --- js/net/src/ietf/ietf.test.ts | 102 +++++++++++++++++++++++++++++++++++ js/net/src/ietf/subscribe.ts | 62 ++++++++++++++++----- 2 files changed, 151 insertions(+), 13 deletions(-) diff --git a/js/net/src/ietf/ietf.test.ts b/js/net/src/ietf/ietf.test.ts index d16c7b5bee2..5c9dd846325 100644 --- a/js/net/src/ietf/ietf.test.ts +++ b/js/net/src/ietf/ietf.test.ts @@ -2084,3 +2084,105 @@ for (const version of [Version.DRAFT_14, Version.DRAFT_15, Version.DRAFT_16]) { expect(params.groupOrder).toBe(0); }); } + +// Caller-selected SUBSCRIBE group order. +// +// GROUP_ORDER was a module constant written unconditionally on both wire shapes, and +// `Subscribe` had no field for it -- so no caller, internal or public, could ask for +// Ascending. The decoder already read and validated the value, and then dropped it: a relay +// built on this parsed the subscriber's requested order correctly and could not act on it. +test.each([ + ["v14", Version.DRAFT_14], + ["v20", Version.DRAFT_20], +] as const)("Subscribe %s: group order defaults to descending", async (_name, version) => { + const msg = new Subscribe.Subscribe({ + requestId: 1n, + trackNamespace: Path.from("test"), + trackName: "video", + subscriberPriority: 128, + }); + expect(msg.groupOrder).toBe(Subscribe.GroupOrder.Descending); + + const encoded = await encodeVersioned(msg, version); + const decoded = await decodeVersioned(encoded, Subscribe.Subscribe.decode, version); + expect(decoded.groupOrder).toBe(Subscribe.GroupOrder.Descending); +}); + +test.each([ + ["v14", Version.DRAFT_14], + ["v20", Version.DRAFT_20], +] as const)("Subscribe %s: a caller can request ascending group order", async (_name, version) => { + const msg = new Subscribe.Subscribe({ + requestId: 1n, + trackNamespace: Path.from("test"), + trackName: "video", + subscriberPriority: 128, + groupOrder: Subscribe.GroupOrder.Ascending, + }); + + const encoded = await encodeVersioned(msg, version); + + // Differential, not a byte search: `toContain(1)` would pass on requestId 1n + // alone. Encode the same message as Descending and require that exactly one + // byte moved, from 0x02 to 0x01. + const descending = await encodeVersioned( + new Subscribe.Subscribe({ + requestId: 1n, + trackNamespace: Path.from("test"), + trackName: "video", + subscriberPriority: 128, + groupOrder: Subscribe.GroupOrder.Descending, + }), + version, + ); + expect(encoded.length).toBe(descending.length); + const differing = [...encoded].flatMap((byte, i) => (byte === descending[i] ? [] : [i])); + expect(differing).toHaveLength(1); + expect(encoded[differing[0]]).toBe(Subscribe.GroupOrder.Ascending); + expect(descending[differing[0]]).toBe(Subscribe.GroupOrder.Descending); + + const decoded = await decodeVersioned(encoded, Subscribe.Subscribe.decode, version); + expect(decoded.groupOrder).toBe(Subscribe.GroupOrder.Ascending); +}); + +test.each([ + ["v14", Version.DRAFT_14], + ["v20", Version.DRAFT_20], +] as const)("Subscribe %s: a caller asking for Default sends a real order", async (_name, version) => { + // 0x0 is the wire's "no preference". A caller may pass it, but it must normalize to a + // real order rather than reaching the encoders -- from draft-15 GROUP_ORDER is a message + // parameter and Parameters rejects 0 outright, so sending it would be unencodable. + const msg = new Subscribe.Subscribe({ + requestId: 1n, + trackNamespace: Path.from("test"), + trackName: "video", + subscriberPriority: 128, + groupOrder: Subscribe.GroupOrder.Default, + }); + expect(msg.groupOrder).toBe(Subscribe.GroupOrder.Descending); + + const encoded = await encodeVersioned(msg, version); + const decoded = await decodeVersioned(encoded, Subscribe.Subscribe.decode, version); + expect(decoded.groupOrder).toBe(Subscribe.GroupOrder.Descending); +}); + +// Draft-14 carries GROUP_ORDER as an inline u8 rather than a message parameter, so 0 is +// reachable there from a peer and must resolve to the default instead of surfacing as a +// third value a caller has to handle. +test("Subscribe v14: an inline group order of 0 decodes as the default", async () => { + const msg = new Subscribe.Subscribe({ + requestId: 1n, + trackNamespace: Path.from("test"), + trackName: "video", + subscriberPriority: 128, + }); + const encoded = await encodeVersioned(msg, Version.DRAFT_14); + + // Rewrite the encoded inline order byte to 0, which no encoder of ours will emit. + const orderAt = encoded.lastIndexOf(Subscribe.GroupOrder.Descending); + const zeroed = new Uint8Array(encoded); + zeroed[orderAt] = Subscribe.GroupOrder.Default; + + const decoded = await decodeVersioned(zeroed, Subscribe.Subscribe.decode, Version.DRAFT_14); + expect(decoded.groupOrder).toBe(Subscribe.GroupOrder.Descending); +}); diff --git a/js/net/src/ietf/subscribe.ts b/js/net/src/ietf/subscribe.ts index 73d218463ae..fa3414eea6b 100644 --- a/js/net/src/ietf/subscribe.ts +++ b/js/net/src/ietf/subscribe.ts @@ -7,8 +7,27 @@ import { type MessageLocation, Parameters } from "./parameters.ts"; import * as Properties from "./properties.ts"; import { type IetfVersion, Version } from "./version.ts"; -// we only support Group Order descending -const GROUP_ORDER = 0x02; +/** + * The GROUP_ORDER a subscription asks the publisher to deliver in. + * + * `Default` (0x0) is the wire's "no preference": the publisher picks, which in + * practice means its own DEFAULT_PUBLISHER_GROUP_ORDER track property. It is accepted + * from a caller and on a draft-14 decode, and normalized away in both cases -- this + * client never puts it on the wire. From draft-15 it could not: GROUP_ORDER is a + * message parameter there and {@link Parameters} rejects 0 as a protocol violation, so + * only draft-14's inline field can carry it at all. + */ +export const GroupOrder = { + Default: 0x00, + Ascending: 0x01, + Descending: 0x02, +} as const; + +export type GroupOrder = (typeof GroupOrder)[keyof typeof GroupOrder]; + +// The order this client requests unless the caller names one. Descending (newest +// group first) is what every subscription asked for before `groupOrder` existed. +const GROUP_ORDER = GroupOrder.Descending; /** * The filter this implementation joins a live track with. @@ -29,6 +48,14 @@ export class Subscribe { trackName: string; subscriberPriority: number; + /** + * The order the publisher is asked to deliver groups in (GROUP_ORDER). + * + * Defaults to {@link GroupOrder.Descending}, which is what this client sent + * unconditionally before the field existed. + */ + groupOrder: GroupOrder; + /** Which Objects the subscription delivers, resolved by the publisher against the live edge. */ filter: Filter.Filter; @@ -49,6 +76,7 @@ export class Subscribe { trackNamespace, trackName, subscriberPriority, + groupOrder, filter, fill, propertiesWanted, @@ -59,6 +87,7 @@ export class Subscribe { trackNamespace: Path.Valid; trackName: string; subscriberPriority: number; + groupOrder?: GroupOrder; filter?: Filter.Filter; fill?: Filter.Fill; propertiesWanted?: boolean; @@ -69,6 +98,9 @@ export class Subscribe { this.trackNamespace = trackNamespace; this.trackName = trackName; this.subscriberPriority = subscriberPriority; + // Resolve "no preference" to a real order here, so `groupOrder` is never 0 for a + // caller reading it back and 0 can never reach the encoders. + this.groupOrder = groupOrder === undefined || groupOrder === GroupOrder.Default ? GROUP_ORDER : groupOrder; this.filter = filter ?? { kind: "unfiltered" }; this.fill = fill; this.propertiesWanted = propertiesWanted ?? true; @@ -86,7 +118,7 @@ export class Subscribe { if (version === Version.DRAFT_14) { await w.u8(this.subscriberPriority); - await w.u8(GROUP_ORDER); + await w.u8(this.groupOrder); await w.bool(this.forward); await w.write(Filter.encode(this.filter, version)); await w.u53(0); // no parameters @@ -94,7 +126,7 @@ export class Subscribe { // v15+: fields moved into parameters const params = new Parameters(); params.subscriberPriority = this.subscriberPriority; - params.groupOrder = GROUP_ORDER; + params.groupOrder = this.groupOrder; params.forward = this.forward; params.subscriptionFilter = Filter.encode(this.filter, version); @@ -134,31 +166,34 @@ export class Subscribe { if (version === Version.DRAFT_14) { const subscriberPriority = await r.u8(); - let groupOrder = await r.u8(); + const groupOrder = await r.u8(); if (groupOrder > 2) { throw new Error(`unknown group order: ${groupOrder}`); } - if (groupOrder === 0) { - groupOrder = GROUP_ORDER; // default to descending - } + // 0 ("no preference") is resolved to a real order by the constructor. const forward = await r.bool(); const filter = await Filter.decodeInline(r); await Parameters.decode(r, version); // ignore parameters - return new Subscribe({ requestId, trackNamespace, trackName, subscriberPriority, filter, forward }); + return new Subscribe({ + requestId, + trackNamespace, + trackName, + subscriberPriority, + groupOrder: groupOrder as GroupOrder, + filter, + forward, + }); } // v15+: fields are in parameters const params = await Parameters.decode(r, version, "subscribe"); const subscriberPriority = params.subscriberPriority ?? 128; - let groupOrder = params.groupOrder ?? GROUP_ORDER; + const groupOrder = params.groupOrder ?? GROUP_ORDER; if (groupOrder > 2) { throw new Error(`unknown group order: ${groupOrder}`); } - if (groupOrder === 0) { - groupOrder = GROUP_ORDER; // default to descending - } // FILL_PARAMETERS and INCLUDE_PROPERTIES are draft-20 additions, and the Range // Filters draft-19 ones. An unknown message parameter is a protocol violation, so @@ -186,6 +221,7 @@ export class Subscribe { trackNamespace, trackName, subscriberPriority, + groupOrder: groupOrder as GroupOrder, filter, fill, // Defaults to 1, so an absent parameter means the subscriber wants them.