fix: drop a schema-invalid _meta.progressToken instead of sending it
`progressToken` is a reserved `_meta` member the spec constrains to `string | integer` (`ProgressTokenSchema = z.union([z.string(), z.number().int()])`). Every other value may be any JSON after #1910, which is what makes this one key need its own check: before the widening a value was always a string and therefore always valid, but now a float, an object or an array can land there, and a conforming server would reject the whole request. `progressTokenOf` only governed whether a progress callback was wired up, so a bad value still rode out inside `_meta`. Sanitize in `mergeMeta` instead — the single point every `_meta` flows through — dropping just the offending member with a warning. The key is optional, so the request stays well-formed and simply asks for no progress. Note the exposure only exists with `progress: false`. With progress on (the default) the SDK sets `onprogress` and stamps its own message id over `_meta.progressToken`, so a caller's value never reaches the wire either way. The new tests therefore run with progress disabled, which is the configuration that can actually ship a bad token: five invalid shapes dropped (float, object, array, null, boolean) with the sibling keys left intact, and string/integer preserved. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
01572c31b9
commit
e9dc0e5e5f
2 changed files with 116 additions and 3 deletions
|
|
@ -1412,6 +1412,90 @@ describe("InspectorClient", () => {
|
|||
messageLogState.destroy();
|
||||
});
|
||||
|
||||
it.each([
|
||||
["a float", 3.5],
|
||||
["an object", { id: 1 }],
|
||||
["an array", [1]],
|
||||
["null", null],
|
||||
["a boolean", true],
|
||||
])(
|
||||
"drops a reserved progressToken that is %s rather than sending it",
|
||||
async (_label, badToken) => {
|
||||
// Every other `_meta` value may be any JSON (#1910), but the spec
|
||||
// constrains `progressToken` to `string | integer`
|
||||
// (`ProgressTokenSchema`). Sending a bad one would make a conforming
|
||||
// server reject the whole request, so the member is dropped — the key
|
||||
// is optional, so what goes out is still well-formed.
|
||||
//
|
||||
// `progress: false` is what makes this observable. With progress on
|
||||
// (the default) the SDK sets `onprogress` and stamps its own message
|
||||
// id over `_meta.progressToken`, so a caller's value never reaches the
|
||||
// wire either way; with it off nothing overwrites the member and a bad
|
||||
// one would ship.
|
||||
client = new InspectorClient(
|
||||
{
|
||||
type: "stdio",
|
||||
command: serverCommand.command,
|
||||
args: serverCommand.args,
|
||||
},
|
||||
{
|
||||
environment: { transport: createTransportNode },
|
||||
progress: false,
|
||||
defaultMetadata: { progressToken: badToken, keep: "me" },
|
||||
},
|
||||
);
|
||||
const messageLogState = new MessageLogState(client);
|
||||
await client.connect();
|
||||
await client.listTools();
|
||||
|
||||
const listToolsReq = messageLogState
|
||||
.getMessages()
|
||||
.find(
|
||||
(m) =>
|
||||
m.direction === "request" &&
|
||||
(m.message as { method?: string }).method === "tools/list",
|
||||
);
|
||||
expect(listToolsReq).toBeDefined();
|
||||
const meta = metaOf(listToolsReq!);
|
||||
expect(meta).not.toHaveProperty("progressToken");
|
||||
// Only the offending member is dropped, not the whole payload.
|
||||
expect(meta.keep).toBe("me");
|
||||
messageLogState.destroy();
|
||||
},
|
||||
);
|
||||
|
||||
it.each([
|
||||
["a string", "tok-1"],
|
||||
["an integer", 7],
|
||||
])("keeps a valid progressToken that is %s", async (_label, token) => {
|
||||
client = new InspectorClient(
|
||||
{
|
||||
type: "stdio",
|
||||
command: serverCommand.command,
|
||||
args: serverCommand.args,
|
||||
},
|
||||
{
|
||||
environment: { transport: createTransportNode },
|
||||
// See the note above: with progress on, the SDK overwrites this.
|
||||
progress: false,
|
||||
defaultMetadata: { progressToken: token },
|
||||
},
|
||||
);
|
||||
const messageLogState = new MessageLogState(client);
|
||||
await client.connect();
|
||||
await client.listTools();
|
||||
|
||||
const listToolsReq = messageLogState
|
||||
.getMessages()
|
||||
.find(
|
||||
(m) =>
|
||||
m.direction === "request" &&
|
||||
(m.message as { method?: string }).method === "tools/list",
|
||||
);
|
||||
expect(metaOf(listToolsReq!).progressToken).toBe(token);
|
||||
messageLogState.destroy();
|
||||
});
|
||||
|
||||
it("call-time metadata overrides defaultMetadata on key collision", async () => {
|
||||
client = new InspectorClient(
|
||||
{
|
||||
|
|
|
|||
|
|
@ -369,6 +369,22 @@ interface ToolCallRequest {
|
|||
// server can't spin a tight zero-delay loop; a successful acknowledgement resets
|
||||
// the count, and past the cap we give up (mark the stream ended) rather than
|
||||
// retry a persistently-failing re-list forever.
|
||||
/**
|
||||
* Whether a `_meta.progressToken` value is one the protocol allows.
|
||||
*
|
||||
* The spec (and the SDK's `ProgressTokenSchema`,
|
||||
* `z.union([z.string(), z.number().int()])`) constrains this reserved member to
|
||||
* a string or an **integer** — a float is as invalid as an object. Everything
|
||||
* else in `_meta` may be any JSON (#1910), which is exactly why this one key
|
||||
* needs its own check.
|
||||
*/
|
||||
function isProgressToken(value: unknown): value is ProgressToken {
|
||||
return (
|
||||
typeof value === "string" ||
|
||||
(typeof value === "number" && Number.isInteger(value))
|
||||
);
|
||||
}
|
||||
|
||||
const MODERN_RECONNECT_BASE_MS = 500;
|
||||
const MODERN_RECONNECT_MAX_MS = 15_000;
|
||||
const MODERN_RECONNECT_MAX_ATTEMPTS = 8;
|
||||
|
|
@ -1053,6 +1069,21 @@ export class InspectorClient extends InspectorClientEventTarget {
|
|||
...(logMeta ?? {}),
|
||||
...(callMetadata ?? {}),
|
||||
};
|
||||
// `_meta` values are arbitrary JSON (#1910), but `progressToken` is a
|
||||
// reserved member the spec constrains to `string | integer`
|
||||
// (`ProgressTokenSchema = z.union([z.string(), z.number().int()])`). Now
|
||||
// that a value can be an object, an array or a float, a bad one would ride
|
||||
// out on the wire and a conforming server would reject the whole request —
|
||||
// so drop the member rather than send something invalid. It is optional, so
|
||||
// the request stays well-formed and simply asks for no progress. Warn,
|
||||
// because a silently dropped key the user typed is worse than a noisy one.
|
||||
if ("progressToken" in merged && !isProgressToken(merged.progressToken)) {
|
||||
this.logger.warn(
|
||||
{ progressToken: merged.progressToken },
|
||||
"Dropping `_meta.progressToken` — expected a string or an integer.",
|
||||
);
|
||||
delete merged.progressToken;
|
||||
}
|
||||
return Object.keys(merged).length > 0 ? merged : undefined;
|
||||
}
|
||||
|
||||
|
|
@ -1071,9 +1102,7 @@ export class InspectorClient extends InspectorClientEventTarget {
|
|||
metadata: RequestMetadata | undefined,
|
||||
): ProgressToken | undefined {
|
||||
const token = metadata?.progressToken;
|
||||
return typeof token === "string" || typeof token === "number"
|
||||
? token
|
||||
: undefined;
|
||||
return isProgressToken(token) ? token : undefined;
|
||||
}
|
||||
|
||||
private getRequestOptions(
|
||||
|
|
|
|||
Loading…
Reference in a new issue