diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx index 4b6e524c..a4855e55 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx @@ -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 diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx index 0d697044..ac0e2173 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx @@ -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({ 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. - {!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. - {child.name} (outside this skill) + {child.name} + {!inRoot + ? " (outside this skill)" + : " (not a direct child)"} ) : ( 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( diff --git a/clients/web/src/test/core/mcp/skillsVerification.test.ts b/clients/web/src/test/core/mcp/skillsVerification.test.ts index 0bbdc822..0f874339 100644 --- a/clients/web/src/test/core/mcp/skillsVerification.test.ts +++ b/clients/web/src/test/core/mcp/skillsVerification.test.ts @@ -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 diff --git a/core/mcp/skillsVerification.ts b/core/mcp/skillsVerification.ts index fa65057a..c4ee2da9 100644 --- a/core/mcp/skillsVerification.ts +++ b/core/mcp/skillsVerification.ts @@ -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