modelcontextprotocol/inspector
Watch
1
0
Fork
You've already forked inspector
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 2

fix: address Copilot review round 18 on #2248

Browse source 
Three findings, all in paths hardened by earlier rounds.

- The 16 MiB read budget was charged only after a content block had
  matched by URI *and* decoded. A server could answer every manifest row
  with one enormous block labelled a different URI (so `contentsFor`
  found nothing) or one enormous invalid-base64 blob (so `skillFileBytes`
  threw), bank zero against the cap, and have the walk issue up to 512
  more. `responseBytes` now charges the raw response before selection or
  decoding; the exact decoded length is substituted when there is one, so
  the common path is still measured precisely.

- The Directory section checked that a child was inside the skill root
  but not that it was a child of the directory actually being read. A
  grandchild, or the directory echoing itself back, passed containment
  and was rendered as navigable. Non-direct entries are now shown and
  labelled rather than linked — `resources/directory/read` answers with
  direct children, so an entry that is not one is itself the finding.

- Containment was decided on the normalized URI while navigation sent and
  stored the raw one. For `skill://r/a/../templates` the first Up produced
  `skill://r/a/..` and a second walked into `skill://r/a`, a directory
  nothing had validated. Directory descent passes the normalized URI, and
  the Up arithmetic moved into `parentOfSkillUri`, documented as valid
  only on a normalized URI.

The suppressed comment (two stderr lines on a failing `--verify`) is
declined again: `--strict` produces the same shape in `emit-result.ts`,
and clients/cli/README.md already documents it as such.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ND46Ci64DC3tzBy9aG81Nc
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-09-07 23:33:30 -04:00
commit a3f36ce863
4 changed files with 253 additions and 42 deletions
Download patch file Download diff file Expand all files Collapse all files

84
clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx
View file

@ -2134,6 +2134,90 @@ describe("SkillsScreen directory browsing (#2248)", () => {
).toBeInTheDocument();
});
it("refuses a child that is not a DIRECT child of the directory read", async () => {
// `resources/directory/read` answers with the directory's direct children.
// A grandchild, or the directory itself echoed back, is still inside the
// root — so it passed the containment check and was rendered as though the
// server had said it lives here (Copilot). The row is shown, because that
// is the finding, but it is not a link.
const user = userEvent.setup();
const GRANDCHILD = {
uri: "skill://data-analysis/templates/invoice.md",
name: "invoice.md",
mimeType: "text/markdown",
};
await openRoot(
user,
directoryReader({
[ROOT]: { resources: [CHILD_FILE, GRANDCHILD, CHILD_DIR] },
// The second page lists the directory ITSELF alongside its child.
[CHILD_DIR.uri]: { resources: [CHILD_DIR, NESTED] },
}),
);
const table = () => within(screen.getByTestId("skill-directory"));
expect(table().getByText(/not a direct child/)).toBeInTheDocument();
// Named for what is wrong with it — it is inside the skill, so calling it
// "outside this skill" would send the reader after the wrong defect.
expect(table().queryByText(/outside this skill/)).not.toBeInTheDocument();
expect(
table().queryByRole("button", { name: `View ${GRANDCHILD.uri}` }),
).not.toBeInTheDocument();
// The real direct children are unaffected.
expect(
table().getByRole("button", { name: `View ${CHILD_FILE.uri}` }),
).toBeInTheDocument();
// …and the same holds one level down, where the offender is the directory
// being read. Left navigable it would be a link back to the page you are
// already on.
await user.click(
screen.getByRole("button", { name: `Open directory ${CHILD_DIR.uri}` }),
);
await waitFor(() =>
expect(table().getByText(NESTED.uri)).toBeInTheDocument(),
);
expect(table().getByText(/not a direct child/)).toBeInTheDocument();
expect(
table().queryByRole("button", {
name: `Open directory ${CHILD_DIR.uri}`,
}),
).not.toBeInTheDocument();
});
it("navigates on the normalized URI, so Up cannot walk into a `..` segment", async () => {
// Containment was decided on the normalized URI while navigation sent and
// stored the raw one, so for `skill://root/a/../templates` the first Up
// produced `skill://root/a/..` and a second walked into `skill://root/a`
// — a directory the check never validated (Copilot).
const user = userEvent.setup();
const DOTTED_DIR = {
uri: "skill://data-analysis/nested/../templates",
name: "templates",
mimeType: "inode/directory",
};
const reader = directoryReader({
[ROOT]: { resources: [DOTTED_DIR] },
// Keyed by the NORMALIZED URI: that is what must be sent.
"skill://data-analysis/templates": { resources: [NESTED] },
});
await openRoot(user, reader);
await user.click(
screen.getByRole("button", { name: `Open directory ${DOTTED_DIR.uri}` }),
);
await waitFor(() =>
expect(reader).toHaveBeenCalledWith(
"skill://data-analysis/templates",
undefined,
),
);
await user.click(screen.getByRole("button", { name: "Up" }));
// One hop, straight back to the root — not to `skill://data-analysis/nested`.
await waitFor(() => expect(reader).toHaveBeenCalledWith(ROOT, undefined));
expect(reader.mock.calls.map((call) => call[0])).not.toContain(
"skill://data-analysis/nested",
);
});
it("refuses a sibling whose path merely starts with the same characters", async () => {
// The reason the check appends a separator: a bare `startsWith(skillRoot)`
// would accept `skill://data-analysis-other/...` as a child of

60
clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx
View file

@ -698,6 +698,18 @@ function resourceFileName(uri: string): string {
}
/** `sha256:abcd…wxyz`, so a long digest stays readable in a table cell. */
/**
* The parent directory of a skill URI, by path arithmetic.
*
* Only ever applied to a NORMALIZED URI — one with no `.`/`..` segments left —
* so the last `/` really is the boundary between a directory and its child. On
* a raw URI the same slice is meaningless: the parent of
* `skill://r/a/../templates` is `skill://r`, not `skill://r/a/..`.
*/
function parentOfSkillUri(uri: string): string {
return uri.slice(0, uri.lastIndexOf("/"));
}
function shortDigest(digest: string | undefined): string {
if (!digest) return "—";
return digest.length <= 24 ? digest : `${digest.slice(0, 16)}…`;
@ -1351,6 +1363,9 @@ export function SkillsScreen({
// same guard every other async slot on this screen uses.
const directoryCurrent = directory.key === manifestKey;
const directoryUri = directoryCurrent ? directory.uri : undefined;
// The directory these children were read FROM — the root until the reader
// descends. Every child row is judged against this, so the two cannot drift.
const readingUri = directoryUri ?? skillRoot;
const directoryChildren = directoryCurrent ? directory.children : undefined;
const directoryError = directoryCurrent ? directory.message : undefined;
const directoryLoading = directoryCurrent && directory.loading === true;
@ -2154,10 +2169,7 @@ export function SkillsScreen({
<FetchButton
onClick={() =>
readDirectory(
directoryUri.slice(
0,
directoryUri.lastIndexOf("/"),
),
parentOfSkillUri(directoryUri),
manifestKey,
)
}
@ -2231,6 +2243,19 @@ export function SkillsScreen({
skillRoot !== undefined &&
(childUri === skillRoot ||
childUri.startsWith(`${skillRoot}/`));
// `resources/directory/read` answers with the
// directory's DIRECT children. A grandchild, a
// sibling's file, or the directory itself is
// inside the root and so passed `inRoot`, and
// was then rendered as though the server had
// said it lives here (Copilot). This screen
// exists to report what a server sent, so an
// entry that is not a direct child is shown
// and named rather than quietly navigable.
const directChild =
childUri !== undefined &&
readingUri !== undefined &&
parentOfSkillUri(childUri) === readingUri;
// A directory is not a manifest entry in the
// first place — a manifest lists files — so it
// is neither listed nor unlisted and gets no
@ -2245,13 +2270,18 @@ export function SkillsScreen({
// collapse into one.
<Table.Tr key={index}>
<Table.Td>
{!inRoot ? (
{!inRoot || !directChild ? (
// Shown, never navigable. The reader
// should see what the server sent, and
// a child outside the skill it was
// asked about is itself the finding.
// asked about — or one that is not a
// child of this directory at all — is
// itself the finding.
<NoVerdictText>
{child.name} (outside this skill)
{child.name}
{!inRoot
? " (outside this skill)"
: " (not a direct child)"}
</NoVerdictText>
) : (
<ResourceUriButton
@ -2267,8 +2297,20 @@ export function SkillsScreen({
}
onClick={() =>
isDir
? readDirectory(
child.uri,
? // ⚠️ The NORMALIZED URI, which
// is what `directChild` and
// `inRoot` were decided on.
// Storing the raw one instead
// meant "Up" did its path
// arithmetic on an identity
// nothing had validated — from
// `skill://r/a/../templates`
// the first Up produced
// `skill://r/a/..` and the
// second walked into
// `skill://r/a` (Copilot).
readDirectory(
childUri,
manifestKey,
)
: showResource(

34
clients/web/src/test/core/mcp/skillsVerification.test.ts
View file

@ -657,6 +657,40 @@ describe("verifySkills (#2248)", () => {
expect(report.incomplete).toMatch(/actually served/);
});
it("charges the budget for a response whose block never matched", async () => {
// The budget was charged only after a matching block had been decoded, so
// a server could answer every row with one enormous block labelled some
// OTHER URI: `contentsFor` found nothing, zero was banked, and the walk
// went on to issue up to 512 more of them (Copilot). The bytes crossed the
// wire either way, so the transfer is what pays.
const junk = "z".repeat(6 * 1024 * 1024);
const { skill } = await truncatable({ name: "junk", count: 20 });
const readResource = vi.fn(async () => ({
// Labelled a URI nobody asked for — the whole point.
result: { contents: [{ uri: "skill://elsewhere/huge.md", text: junk }] },
}));
const client = { readResource } as unknown as InspectorClientProtocol;
const [report] = await verifySkills(client, [skill]);
// Three reads of 6 MiB crosses 16 MiB; without the fix all 20 were issued.
expect(readResource.mock.calls.length).toBe(3);
expect(report.incomplete).toMatch(/actually served/);
expect(report.outcome).toBe("failed");
});
it("charges the budget for a response that could not be decoded", async () => {
// The second free route: an enormous `blob` that is not valid base64, so
// `skillFileBytes` throws before anything is counted.
const junk = "!".repeat(6 * 1024 * 1024);
const { skill } = await truncatable({ name: "junk", count: 20 });
const readResource = vi.fn(async (uri: string) => ({
result: { contents: [{ uri, blob: junk }] },
}));
const client = { readResource } as unknown as InspectorClientProtocol;
const [report] = await verifySkills(client, [skill]);
expect(readResource.mock.calls.length).toBe(3);
expect(report.incomplete).toMatch(/actually served/);
});
it("is not incomplete when the budget is crossed by the LAST entry", async () => {
// Crossing the line on the final row stopped nothing: every manifest entry
// was fetched and checked. Reporting "Stopped after 4 of 4" there both

117
core/mcp/skillsVerification.ts
View file

@ -219,6 +219,37 @@ function contentsFor(result: unknown, uri: string): ReadContents | undefined {
return undefined;
}
/**
* What a `resources/read` response cost to receive, charged against the byte
* budget regardless of whether any of it is usable.
*
* ⚠️ Deliberately measured on the RAW result rather than on decoded content.
* The budget exists to stop a server from making this walk transfer unbounded
* data, and a server that wants to do that has two free routes if only decoded
* bytes are counted: label an enormous block with a URI that was not asked for,
* or send an enormous blob that is not valid base64. Both leave the decode
* paths empty-handed while the bytes have already crossed the wire.
*
* The figure is an **approximation, and deliberately never an undercount by
* more than a small factor**: `text` is charged in UTF-16 code units (UTF-8 is
* between 1× and 3× that for the same string) and `blob` in base64 characters
* (roughly 4/3 of the bytes it decodes to, so an overcharge). Exactness is not
* the point — this is a safety limit, not an accounting figure, and the caller
* substitutes the exact decoded length whenever it has one.
*/
function responseBytes(result: unknown): number {
const contents = (result as { contents?: unknown })?.contents;
if (!Array.isArray(contents)) return 0;
let total = 0;
for (const block of contents) {
if (typeof block !== "object" || block === null) continue;
const { text, blob } = block as { text?: unknown; blob?: unknown };
if (typeof text === "string") total += text.length;
if (typeof blob === "string") total += blob.length;
}
return total;
}
/**
* Verify every skill in `entries` against the connected server.
*
@ -316,49 +347,69 @@ export async function verifySkills(
// rather than re-attempted by the fallback.
selfAttempted = true;
}
let contents: ReadContents | undefined;
// Bytes attributed to THIS response, charged whether or not any of them
// turn out to be usable — see `responseBytes`.
let charged = 0;
try {
const invocation = await client.readResource(resource.uri, metadata);
contents = contentsFor(invocation.result, resource.uri);
// ⚠️ Charged from the RAW response, BEFORE the block is selected and
// before it is decoded. Charging only the decoded bytes let a server
// spend the budget for free: return one enormous block labelled some
// other URI (so `contentsFor` finds nothing) or one enormous invalid
// base64 blob (so `skillFileBytes` throws), and the walk banked zero
// against the cap and went on to issue up to 512 more of them
// (Copilot). The safeguard has to be paid for by the transfer, not by
// the parse.
charged = responseBytes(invocation.result);
const contents = contentsFor(invocation.result, resource.uri);
if (!contents) {
files.push({
uri: resource.uri,
status: "read-error",
reason:
"resources/read returned no content block for this URI, so there are no bytes that can be checked against its digest.",
});
} else {
let bytes: Uint8Array | undefined;
try {
bytes = skillFileBytes(contents);
} catch (err) {
files.push({
uri: resource.uri,
status: "read-error",
reason: reasonOf(err),
});
}
if (bytes) {
// The decoded length is exact where `responseBytes` is only an
// estimate, so the larger of the two is charged: never less than
// what this file actually cost, and never less than what the rest
// of the response was estimated to cost.
charged = Math.max(charged, bytes.byteLength);
if (skillUriIdentity(resource.uri) === entryIdentity)
entryBytes = bytes;
const verification = await verifySkillResource(resource, bytes);
files.push({ uri: resource.uri, ...verification });
}
}
} catch (err) {
if (err instanceof AuthRecoveryRequiredError) throw err;
// Nothing to charge: a rejected read never handed us a payload to
// measure. Whatever the transport moved before failing is invisible
// at this layer.
files.push({
uri: resource.uri,
status: "read-error",
reason: reasonOf(err),
});
continue;
}
if (!contents) {
files.push({
uri: resource.uri,
status: "read-error",
reason:
"resources/read returned no content block for this URI, so there are no bytes that can be checked against its digest.",
});
continue;
}
let bytes: Uint8Array;
try {
bytes = skillFileBytes(contents);
} catch (err) {
files.push({
uri: resource.uri,
status: "read-error",
reason: reasonOf(err),
});
continue;
}
if (skillUriIdentity(resource.uri) === entryIdentity) entryBytes = bytes;
const verification = await verifySkillResource(resource, bytes);
files.push({ uri: resource.uri, ...verification });
// Counted AFTER verifying this file, so the one that crosses the line is
// still reported rather than fetched and discarded. The next read is what
// stops. ⚠️ This bounds the total across responses, not the size of any
// single one: a first response larger than the cap is already in memory
// by the time it can be measured, which would need a streaming read to
// prevent and is not something this API exposes.
receivedBytes += bytes.byteLength;
// Counted AFTER this row is recorded, so the response that crosses the
// line is still reported rather than fetched and discarded. The next
// read is what stops. ⚠️ This bounds the total across responses, not the
// size of any single one: a first response larger than the cap is
// already in memory by the time it can be measured, which would need a
// streaming read to prevent and is not something this API exposes.
receivedBytes += charged;
if (receivedBytes > SKILL_MAX_TOTAL_BYTES) {
// ⚠️ Only *incomplete* when the budget actually cost a read. Crossing
// the line on the final entry stopped nothing — every manifest row was