fix: log the rejected progressToken's type, not its value
The warning added one commit earlier echoed the rejected value. That is the
same leak class as the `servers/show` fix: `_meta` now carries arbitrary JSON,
so an invalid `progressToken` can be an object holding credentials — and this
logger is persisted by real clients (`clients/tui/src/logger.ts` writes it to
`~/.mcp-inspector/auth.log` via `pino.destination`, append mode).
Log `{ received: describeJsonType(value) }` instead — "object", "array",
"number". That is what makes the warning actionable; the value itself only
leaks.
Test asserts the record is exactly `{ received: "object" }` and that the
planted secret appears nowhere in the captured calls.
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
85e4a29990
commit
ae9536b171
2 changed files with 60 additions and 1 deletions
|
|
@ -55,6 +55,7 @@ import type {
|
|||
FetchRequestEntryBase,
|
||||
RequestMetadata,
|
||||
} from "@inspector/core/mcp/types.js";
|
||||
import type { InspectorLogger } from "@inspector/core/logging/logger.js";
|
||||
import type { JsonValue } from "@inspector/core/json/jsonUtils.js";
|
||||
import type {
|
||||
TypedEvent,
|
||||
|
|
@ -1464,6 +1465,49 @@ describe("InspectorClient", () => {
|
|||
},
|
||||
);
|
||||
|
||||
it("warns with the rejected progressToken's type, never its value", async () => {
|
||||
// This logger is persisted by real clients (the TUI writes it to
|
||||
// `~/.mcp-inspector/auth.log`), and an invalid `progressToken` can now be
|
||||
// an object holding credentials — so the warning must not echo it.
|
||||
const warn = vi.fn();
|
||||
const noop = () => {};
|
||||
const logger: InspectorLogger = {
|
||||
level: "warn",
|
||||
fatal: noop,
|
||||
error: noop,
|
||||
warn,
|
||||
info: noop,
|
||||
debug: noop,
|
||||
trace: noop,
|
||||
silent: noop,
|
||||
child: () => logger,
|
||||
};
|
||||
client = new InspectorClient(
|
||||
{
|
||||
type: "stdio",
|
||||
command: serverCommand.command,
|
||||
args: serverCommand.args,
|
||||
},
|
||||
{
|
||||
environment: { transport: createTransportNode, logger },
|
||||
progress: false,
|
||||
defaultMetadata: {
|
||||
progressToken: { accessToken: "sk-live-should-not-appear" },
|
||||
},
|
||||
},
|
||||
);
|
||||
await client.connect();
|
||||
await client.listTools();
|
||||
|
||||
const call = warn.mock.calls.find((c) =>
|
||||
String(c[1]).includes("progressToken"),
|
||||
);
|
||||
expect(call).toBeDefined();
|
||||
expect(call![0]).toEqual({ received: "object" });
|
||||
// Belt and braces: the secret must not appear anywhere in the record.
|
||||
expect(JSON.stringify(warn.mock.calls)).not.toContain("sk-live");
|
||||
});
|
||||
|
||||
it.each([
|
||||
["a string", "tok-1"],
|
||||
["an integer", 7],
|
||||
|
|
|
|||
|
|
@ -369,6 +369,16 @@ 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.
|
||||
/**
|
||||
* A one-word name for a JSON value's type, for diagnostics that must not
|
||||
* disclose the value itself.
|
||||
*/
|
||||
function describeJsonType(value: unknown): string {
|
||||
if (value === null) return "null";
|
||||
if (Array.isArray(value)) return "array";
|
||||
return typeof value;
|
||||
}
|
||||
|
||||
/**
|
||||
* Whether a `_meta.progressToken` value is one the protocol allows.
|
||||
*
|
||||
|
|
@ -1078,8 +1088,13 @@ export class InspectorClient extends InspectorClientEventTarget {
|
|||
// 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)) {
|
||||
// Log the rejected value's *type*, never the value. `_meta` now carries
|
||||
// arbitrary JSON, so an invalid `progressToken` can be an object holding
|
||||
// credentials — and this logger is persisted by real clients (the TUI
|
||||
// writes it to `~/.mcp-inspector/auth.log`). The type is what makes the
|
||||
// warning actionable anyway; the value would only leak.
|
||||
this.logger.warn(
|
||||
{ progressToken: merged.progressToken },
|
||||
{ received: describeJsonType(merged.progressToken) },
|
||||
"Dropping `_meta.progressToken` — expected a string or an integer.",
|
||||
);
|
||||
delete merged.progressToken;
|
||||
|
|
|
|||
Loading…
Reference in a new issue