diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index 930b52a1..617ce438 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -835,6 +835,7 @@ function App() { onRefreshResources, onRefreshSkills, onReadSkillFile, + onGetSkill, onRefreshTasks, onTogglePaginatedLists, onLoadMoreTools, @@ -1832,6 +1833,7 @@ function App() { onSkillsUiChange: setUi.setSkillsUi, onRefreshSkills, onReadSkillFile, + onGetSkill, }; const tasksPanelProps: TasksPanelProps = { diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx index e3cd52db..54ca1a25 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx @@ -107,6 +107,13 @@ const baseProps: SkillsScreenProps = { onUiChange: vi.fn(), onRefreshList: vi.fn(), onReadSkillFile: readFixtureFile, + // Echoes back the very entry `skills/list` advertised, so the default is the + // agreeing case; tests that care about a disagreement override it. + onGetSkill: vi.fn(async (uri: string) => { + const found = ALL_SKILLS.find((skill) => skill.uri === uri); + if (!found) throw new Error(`Unknown skill uri: ${uri}`); + return found; + }), }; // SkillsScreen is controlled: the selection and the sidebar search live in the @@ -430,6 +437,97 @@ describe("SkillsScreen", () => { expect(within(issues).getAllByText("duplicate-resource")).toHaveLength(2); }); + it("fetches the selected entry through skills/get and reports agreement", async () => { + // The acceptance criterion this exists for: `skills/get` is one of the two + // methods the extension requires, and a server author's handler is only + // exercisable if something actually calls it. + const user = userEvent.setup(); + const onGetSkill = vi.fn().mockResolvedValue(CLEAN_SKILL); + renderWithMantine(); + await user.click(screen.getByText("data-analysis")); + await user.click( + screen.getByRole("button", { name: /Fetch with skills\/get/ }), + ); + expect(onGetSkill).toHaveBeenCalledWith(CLEAN_SKILL.uri); + expect( + await screen.findByText("skills/get agrees with skills/list"), + ).toBeInTheDocument(); + }); + + it("reports a skills/get entry that disagrees with the listing", async () => { + // Both describe the same skill, so a disagreement is a server bug that + // only a side-by-side fetch can surface. + const user = userEvent.setup(); + const onGetSkill = vi.fn().mockResolvedValue({ + ...CLEAN_SKILL, + frontmatter: { ...CLEAN_SKILL.frontmatter, description: "different" }, + }); + renderWithMantine(); + await user.click(screen.getByText("data-analysis")); + await user.click( + screen.getByRole("button", { name: /Fetch with skills\/get/ }), + ); + expect( + await screen.findByText("skills/get disagrees with skills/list"), + ).toBeInTheDocument(); + // The fetched entry is rendered beside the verdict so the difference is + // inspectable rather than merely asserted. (Its JSON goes through + // `ContentViewer`'s highlighter, which splits tokens across elements, so + // the presence of the block is what is pinned here — the copy above is + // what states the finding.) + expect(screen.getByTestId("skills-get-result")).toBeInTheDocument(); + }); + + it("reports a failed skills/get", async () => { + const user = userEvent.setup(); + const onGetSkill = vi.fn().mockRejectedValue(new Error("-32602")); + renderWithMantine(); + await user.click(screen.getByText("data-analysis")); + await user.click( + screen.getByRole("button", { name: /Fetch with skills\/get/ }), + ); + expect(await screen.findByText("skills/get failed")).toBeInTheDocument(); + expect(screen.getByText("-32602")).toBeInTheDocument(); + }); + + it("discards a skills/get that resolves after the selection moved on", async () => { + const user = userEvent.setup(); + let release: ((value: SkillEntry) => void) | undefined; + const onGetSkill = vi.fn( + () => + new Promise((resolve) => { + release = resolve; + }), + ); + renderWithMantine(); + await user.click(screen.getByText("data-analysis")); + await user.click( + screen.getByRole("button", { name: /Fetch with skills\/get/ }), + ); + await user.click(screen.getByText("tampered")); + release?.(CLEAN_SKILL); + expect(screen.queryByTestId("skills-get-result")).not.toBeInTheDocument(); + }); + + it("frees Verify all for a newly selected skill while the old batch is hung", async () => { + // A global flag would leave the new skill's button disabled until the + // previous skill's reads settled — forever, if one of them hangs. + const user = userEvent.setup(); + const onReadSkillFile = vi.fn( + () => new Promise<{ text: string }>(() => {}), + ); + renderWithMantine( + , + ); + await user.click(screen.getByText("data-analysis")); + await user.click(screen.getByRole("button", { name: /Verify all/ })); + expect(screen.getByRole("button", { name: /Verify all/ })).toBeDisabled(); + await user.click(screen.getByText("tampered")); + expect( + screen.getByRole("button", { name: /Verify all/ }), + ).not.toBeDisabled(); + }); + it("shows the SKILL.md preview on demand", async () => { const user = userEvent.setup(); renderWithMantine(); diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx index 6dc6fa9a..7f801460 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx @@ -80,6 +80,18 @@ interface PreviewState { message?: string; } +/** + * The result of the on-demand `skills/get`, plus the manifest it belongs to. + * `agrees` records whether the fetched entry matched the one `skills/list` + * returned — the reason for making the call at all. + */ +interface FetchedEntryState { + key: string | null; + entry?: SkillEntry; + agrees?: boolean; + message?: string; +} + export interface SkillsScreenProps { skills: SkillEntry[]; /** Pages the last `skills/list` walk took; shown so pagination is visible. */ @@ -91,6 +103,13 @@ export interface SkillsScreenProps { onRefreshList: () => void; /** Fetch one skill file's contents via `resources/read`, on demand. */ onReadSkillFile: (uri: string) => Promise; + /** + * Re-fetch the selected entry through `skills/get` (SEP-2640). Distinct from + * the entry `skills/list` already returned, and the point of exercising it is + * that the two must agree: a server whose `skills/get` disagrees with its own + * listing is broken in a way only a side-by-side fetch can show. + */ + onGetSkill: (uri: string) => Promise; } /** @@ -149,6 +168,15 @@ const ControlsRow = Group.withProps({ gap: "sm", }); +// The Resources header carries three action buttons beside its count badge, so +// it wraps rather than truncating the badge on a narrow detail pane — unlike +// the sidebar row above, where the search field is meant to absorb the space. +const SectionControlsRow = Group.withProps({ + justify: "space-between", + wrap: "wrap", + gap: "sm", +}); + const SearchInput = TextInput.withProps({ size: "xs", flex: 1, @@ -269,6 +297,7 @@ export function SkillsScreen({ onUiChange, onRefreshList, onReadSkillFile, + onGetSkill, }: SkillsScreenProps) { const { selectedSkillUri, search } = ui; // Both slices carry the manifest key they belong to, and every async @@ -284,9 +313,14 @@ export function SkillsScreen({ files: {}, }); const [previewState, setPreviewState] = useState({ key: null }); - // True while a "Verify all" batch is in flight; disables the button so a - // second click cannot stack another pool of workers on top. - const [batchRunning, setBatchRunning] = useState(false); + const [fetchedEntry, setFetchedEntry] = useState({ + key: null, + }); + // The manifest whose "Verify all" batch is in flight, or `null`. Keyed rather + // than a bare boolean: a global flag would leave a NEWLY selected skill's + // button disabled until the previous skill's reads settled — indefinitely, if + // one of them hangs. + const [batchKey, setBatchKey] = useState(null); // Monotonic per-row attempt token. A ref because it is claimed inside an // event handler, never during render. const nextAttempt = useRef(0); @@ -342,6 +376,7 @@ export function SkillsScreen({ useValueChange(manifestKey, (next) => { setVerification({ key: next, files: {} }); setPreviewState({ key: next }); + setFetchedEntry({ key: next }); }); const fileStates = verification.key === manifestKey ? verification.files : {}; @@ -407,12 +442,14 @@ export function SkillsScreen({ } }; const workers = Math.min(VERIFY_CONCURRENCY, manifest.length); - setBatchRunning(true); + setBatchKey(key); // The concurrency cap is per invocation, so without the button being // disabled below, a second click would start a second pool of four and a // third would make it twelve — the flood the cap exists to prevent. void Promise.all(Array.from({ length: workers }, () => worker())).finally( - () => setBatchRunning(false), + // Clears only ITS OWN batch: a stale finalizer must not free a button the + // user has since re-armed on another skill. + () => setBatchKey((prev) => (prev === key ? null : prev)), ); }, [manifest, manifestKey, verifyRow]); @@ -438,6 +475,33 @@ export function SkillsScreen({ }); }, [manifestKey, onReadSkillFile, selected]); + const fetchEntry = useCallback(() => { + if (!selected) return; + const key = manifestKey; + // Same shape as the SKILL.md read: a click handler cannot await, the chain + // ends in its own `catch`, and both arms compare the manifest key they + // started under so a late answer cannot land under another skill. + void onGetSkill(selected.uri) + .then((entry) => { + // Compared field-by-field against what `skills/list` advertised. The + // two describe the same skill, so a disagreement is a server bug that + // only shows up when both are fetched. + const agrees = JSON.stringify(entry) === JSON.stringify(selected); + setFetchedEntry((prev) => + prev.key !== null && prev.key !== key ? prev : { key, entry, agrees }, + ); + }) + .catch((err: unknown) => { + const message = err instanceof Error ? err.message : String(err); + setFetchedEntry((prev) => + prev.key !== null && prev.key !== key ? prev : { key, message }, + ); + }); + }, [manifestKey, onGetSkill, selected]); + + const fetched = fetchedEntry.key === manifestKey ? fetchedEntry : undefined; + const batchRunning = batchKey === manifestKey; + const preview = previewState.key === manifestKey ? previewState.contents : undefined; const previewError = @@ -561,7 +625,7 @@ export function SkillsScreen({ - + Resources @@ -570,6 +634,9 @@ export function SkillsScreen({ + + Fetch with skills/get + View SKILL.md @@ -581,7 +648,7 @@ export function SkillsScreen({ Verify all - + {selected.resources === DYNAMIC_RESOURCES ? ( This skill declares{" "} @@ -692,6 +759,41 @@ export function SkillsScreen({ })} + {fetched?.message !== undefined && ( + + {fetched.message} + + )} + {fetched?.entry !== undefined && ( + + + + {fetched.agrees + ? "The entry this server returns for this URI is identical to the one it listed." + : "The entry this server returns for this URI differs from the one it listed; both describe the same skill, so one of them is wrong."} + + {!fetched.agrees && ( + + )} + + + )} + {previewError && ( {previewError} diff --git a/clients/web/src/components/views/InspectorView/InspectorView.stories.tsx b/clients/web/src/components/views/InspectorView/InspectorView.stories.tsx index 0c227379..3ca51998 100644 --- a/clients/web/src/components/views/InspectorView/InspectorView.stories.tsx +++ b/clients/web/src/components/views/InspectorView/InspectorView.stories.tsx @@ -433,6 +433,11 @@ const skillsArgs: SkillsPanelProps = { onSkillsUiChange: fn(), onRefreshSkills: fn(), onReadSkillFile: fn(async () => ({ text: "" })), + onGetSkill: fn(async (uri: string) => ({ + uri, + frontmatter: {}, + resources: [], + })), }; const tasksArgs: TasksPanelProps = { diff --git a/clients/web/src/components/views/InspectorView/InspectorView.test.tsx b/clients/web/src/components/views/InspectorView/InspectorView.test.tsx index d720d1e4..4f784cb5 100644 --- a/clients/web/src/components/views/InspectorView/InspectorView.test.tsx +++ b/clients/web/src/components/views/InspectorView/InspectorView.test.tsx @@ -178,6 +178,7 @@ function makeProps(...overrides: PropOverrides[]): InspectorViewProps { onSkillsUiChange: vi.fn(), onRefreshSkills: vi.fn(), onReadSkillFile: vi.fn().mockResolvedValue({ text: "" }), + onGetSkill: vi.fn(), ...mergeBundle("skills", overrides), }, tasks: { diff --git a/clients/web/src/components/views/InspectorView/InspectorView.tsx b/clients/web/src/components/views/InspectorView/InspectorView.tsx index 01c3d756..d620333e 100644 --- a/clients/web/src/components/views/InspectorView/InspectorView.tsx +++ b/clients/web/src/components/views/InspectorView/InspectorView.tsx @@ -463,6 +463,7 @@ export function InspectorView({ onSkillsUiChange, onRefreshSkills, onReadSkillFile, + onGetSkill, } = skillsPanel; const { tasks, @@ -1061,6 +1062,7 @@ export function InspectorView({ onUiChange: onSkillsUiChange, onRefreshList: onRefreshSkills, onReadSkillFile, + onGetSkill, }; const tasksScreenProps = { tasks, diff --git a/clients/web/src/components/views/InspectorView/types.ts b/clients/web/src/components/views/InspectorView/types.ts index 4950765c..15e2a249 100644 --- a/clients/web/src/components/views/InspectorView/types.ts +++ b/clients/web/src/components/views/InspectorView/types.ts @@ -322,6 +322,8 @@ export interface SkillsPanelProps { onRefreshSkills: () => void; /** Read one skill file (`resources/read`) so its digest can be checked. */ onReadSkillFile: (uri: string) => Promise; + /** Re-fetch the selected entry through `skills/get`. */ + onGetSkill: (uri: string) => Promise; } /** The Tasks monitor: the task list, its progress map, and actions. */ diff --git a/clients/web/src/hooks/useServerCommands.test.tsx b/clients/web/src/hooks/useServerCommands.test.tsx index a675a284..25c015f0 100644 --- a/clients/web/src/hooks/useServerCommands.test.tsx +++ b/clients/web/src/hooks/useServerCommands.test.tsx @@ -916,6 +916,28 @@ describe("onReadSkillFile (#2234)", () => { }); }); +describe("onGetSkill (#2234)", () => { + it("routes the uri through the client's skills/get", async () => { + const getSkill = vi.fn().mockResolvedValue({ + uri: "skill://demo/SKILL.md", + frontmatter: {}, + resources: [], + }); + const h = harness({ client: client({ getSkill }) }); + await expect( + h.api().onGetSkill("skill://demo/SKILL.md"), + ).resolves.toMatchObject({ uri: "skill://demo/SKILL.md" }); + expect(getSkill).toHaveBeenCalledWith("skill://demo/SKILL.md"); + }); + + it("throws when there is no client", async () => { + const h = harness(); + await expect(h.api().onGetSkill("skill://demo/SKILL.md")).rejects.toThrow( + "Client is not connected", + ); + }); +}); + describe("onRefreshSkills (#2234)", () => { it("drives the store refresh in the background", () => { const h = harness(); diff --git a/clients/web/src/hooks/useServerCommands.tsx b/clients/web/src/hooks/useServerCommands.tsx index 9488e352..73eaa194 100644 --- a/clients/web/src/hooks/useServerCommands.tsx +++ b/clients/web/src/hooks/useServerCommands.tsx @@ -35,6 +35,7 @@ import type { import type { GetPromptState } from "../components/screens/PromptsScreen/PromptsScreen"; import type { ReadResourceState } from "../components/screens/ResourcesScreen/ResourcesScreen"; import type { SkillFileContents } from "../utils/skillFileBytes"; +import type { SkillEntry } from "@inspector/core/mcp/skillsSchemas.js"; import { UrlElicitationErrorToastMessage } from "../components/elements/Toasts/UrlElicitationErrorToastMessage"; import { errorCodeOf, @@ -219,6 +220,8 @@ export interface ServerCommands { * the digest is taken over. */ onReadSkillFile: (uri: string) => Promise; + /** Re-fetch one skill entry through `skills/get` (SEP-2640). */ + onGetSkill: (uri: string) => Promise; onSubscribeResource: (uri: string) => void; onUnsubscribeResource: (uri: string) => void; onCompleteArgument: ( @@ -973,6 +976,17 @@ export function useServerCommands({ [onReadResourceContents], ); + // `skills/get` is the extension's second required method, and the Skills tab + // calls it on demand so a server author can see their own handler answer — + // and see whether it agrees with what their `skills/list` advertised. + const onGetSkill = useCallback( + async (uri: string): Promise => { + if (!inspectorClient) throw new Error("Client is not connected"); + return inspectorClient.getSkill(uri); + }, + [inspectorClient], + ); + const onRefreshSkills = useCallback(() => { runCommandInBackground( () => refreshSkills(), @@ -997,6 +1011,7 @@ export function useServerCommands({ onReadResource, onReadResourceContents, onReadSkillFile, + onGetSkill, onSubscribeResource, onUnsubscribeResource, onCompleteArgument, diff --git a/clients/web/src/test/core/mcp/sha256.test.ts b/clients/web/src/test/core/mcp/sha256.test.ts new file mode 100644 index 00000000..a0c66be3 --- /dev/null +++ b/clients/web/src/test/core/mcp/sha256.test.ts @@ -0,0 +1,85 @@ +import { describe, it, expect } from "vitest"; +import { sha256Bytes } from "@inspector/core/mcp/sha256"; +import { sha256Digest, textToBytes } from "@inspector/core/mcp/skills"; + +const hex = (bytes: Uint8Array) => + Array.from(bytes) + .map((b) => b.toString(16).padStart(2, "0")) + .join(""); + +/** + * The fallback exists because `crypto.subtle` is absent in a non-secure + * context, and the web client is documented as servable over plain HTTP on a + * LAN IP. So it is checked two ways: against the published FIPS 180-4 vectors, + * and differentially against WebCrypto — "it agrees with the real thing" is the + * property that matters, and it is asserted rather than assumed. + */ +describe("sha256Bytes (the non-secure-context fallback)", () => { + it("matches the FIPS 180-4 vector for the empty message", () => { + expect(hex(sha256Bytes(new Uint8Array()))).toBe( + "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", + ); + }); + + it("matches the FIPS 180-4 vector for 'abc'", () => { + expect(hex(sha256Bytes(textToBytes("abc")))).toBe( + "ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad", + ); + }); + + it("matches the FIPS 180-4 two-block vector", () => { + expect( + hex( + sha256Bytes( + textToBytes( + "abcdbcdecdefdefgefghfghighijhijkijkljklmklmnlmnomnopnopq", + ), + ), + ), + ).toBe("248d6a61d20638b8e5c026930c3e6039a33ce45964ff2167f6ecedd419db06c1"); + }); + + it("agrees with WebCrypto across the block boundaries", async () => { + // 55/56/63/64/65 bracket the padding cases: the last block that still fits + // its length field, the one that forces an extra block, and the exact + // multiple of 64. + for (const length of [0, 1, 55, 56, 63, 64, 65, 200, 1000]) { + const bytes = new Uint8Array(length); + for (let i = 0; i < length; i += 1) bytes[i] = (i * 7 + 13) % 256; + const reference = new Uint8Array( + await crypto.subtle.digest("SHA-256", bytes), + ); + expect(hex(sha256Bytes(bytes))).toBe(hex(reference)); + } + }); + + it("hashes only the view, not its whole backing buffer", () => { + const backing = new Uint8Array([0xff, ...textToBytes("abc"), 0xff]); + expect(hex(sha256Bytes(backing.subarray(1, 4)))).toBe( + "ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad", + ); + }); +}); + +describe("sha256Digest without crypto.subtle", () => { + it("falls back rather than throwing, and returns the same digest", async () => { + // Exactly the shape a plain-HTTP LAN page presents: `crypto` exists, + // `crypto.subtle` does not. Before the fallback this threw for every file + // and the UI reported a read failure for a file it had fetched fine. + const withSubtle = await sha256Digest(textToBytes("abc")); + const real = globalThis.crypto; + try { + Object.defineProperty(globalThis, "crypto", { + configurable: true, + value: { getRandomValues: real.getRandomValues.bind(real) }, + }); + expect(globalThis.crypto.subtle).toBeUndefined(); + await expect(sha256Digest(textToBytes("abc"))).resolves.toBe(withSubtle); + } finally { + Object.defineProperty(globalThis, "crypto", { + configurable: true, + value: real, + }); + } + }); +}); diff --git a/clients/web/src/test/core/mcp/skills.test.ts b/clients/web/src/test/core/mcp/skills.test.ts index f489c7fe..4fd5f527 100644 --- a/clients/web/src/test/core/mcp/skills.test.ts +++ b/clients/web/src/test/core/mcp/skills.test.ts @@ -113,6 +113,10 @@ describe("skillNameFromUri", () => { expect(skillNameFromUri("SKILL.md")).toBeUndefined(); }); + it("returns undefined for a non-skill scheme", () => { + expect(skillNameFromUri("https://demo/SKILL.md")).toBeUndefined(); + }); + it("returns undefined for a relative string", () => { // SEP-2640 requires a full resource URI; treating `demo/SKILL.md` as one // would let a non-conforming entry report a name and pass the path check. @@ -141,6 +145,14 @@ describe("normalizeSkillUri", () => { expect(normalizeSkillUri("demo/SKILL.md")).toBeUndefined(); }); + it("rejects a non-skill scheme", () => { + // Checking only that a URI is hierarchical would let this through and then + // pass the name and root checks — a manifest pointing anywhere on the web, + // reported as conforming. + expect(normalizeSkillUri("https://demo/SKILL.md")).toBeUndefined(); + expect(normalizeSkillUri("file:///demo/SKILL.md")).toBeUndefined(); + }); + it("rejects an opaque-path URI, which the parser does not normalize", () => { // `skill:demo/../x.md` parses but keeps its `..` verbatim, so containment // could not be decided on it — accepting it would reopen the hole. @@ -229,7 +241,7 @@ describe("checkSkillConformance", () => { expect(issues[0].severity).toBe("warning"); }); - it("reports a manifest entry with no digest as unverifiable", () => { + it("reports a manifest entry with no digest as an error", () => { const issues = checkSkillConformance( entry({ resources: [ @@ -239,6 +251,7 @@ describe("checkSkillConformance", () => { }), ); expect(issues.map((i) => i.code)).toEqual(["missing-digest"]); + expect(issues[0].severity).toBe("error"); expect(issues[0].resourceUri).toBe("skill://demo/ref.md"); }); @@ -331,7 +344,7 @@ describe("checkSkillConformance", () => { expect(issues.map((i) => i.code)).toContain("malformed-uri"); }); - it("reports a manifest entry with no size as a warning", () => { + it("reports a manifest entry with no size as an error", () => { const issues = checkSkillConformance( entry({ resources: [ @@ -341,7 +354,7 @@ describe("checkSkillConformance", () => { }), ); expect(issues.map((i) => i.code)).toEqual(["missing-size"]); - expect(issues[0].severity).toBe("warning"); + expect(issues[0].severity).toBe("error"); }); it("reports a digest that is not sha256 + 64 lowercase hex", () => { diff --git a/core/mcp/sha256.ts b/core/mcp/sha256.ts new file mode 100644 index 00000000..365bc94e --- /dev/null +++ b/core/mcp/sha256.ts @@ -0,0 +1,118 @@ +/** + * A dependency-free SHA-256, used when `crypto.subtle` is unavailable. + * + * ⚠️ **This is not an optimization — it is what makes digest verification work + * at all in a documented deployment.** `SubtleCrypto` is exposed only in a + * [secure context](https://developer.mozilla.org/en-US/docs/Web/Security/Secure_Contexts), + * and `clients/web/README.md#hosting-on-a-network` documents serving the + * Inspector over plain HTTP on a LAN IP (`HOST=192.168.1.50`). A browser there + * has `globalThis.crypto` but **no** `crypto.subtle`, so every skill-file + * verification would throw and the UI would report a read failure for files + * that were fetched perfectly well (#2234). + * + * `crypto.subtle` is still preferred wherever it exists — see `sha256Digest` in + * `skills.ts`. This is the fallback, and it is exercised directly by its own + * tests against the published FIPS 180-4 vectors plus a differential check + * against WebCrypto, so "it agrees with the real thing" is asserted rather than + * assumed. + * + * The implementation is the standard FIPS 180-4 construction; it is short + * enough that adding a dependency for it would cost more than it saves, and + * per [Dependency placement] a new runtime dependency here would have to be + * declared at the repo root and threaded through three bundler `external` + * lists. + */ + +/** SHA-256 round constants: the first 32 bits of the fractional parts of the + * cube roots of the first 64 primes (FIPS 180-4 §4.2.2). */ +const K = new Uint32Array([ + 0x428a2f98, 0x71374491, 0xb5c0fbcf, 0xe9b5dba5, 0x3956c25b, 0x59f111f1, + 0x923f82a4, 0xab1c5ed5, 0xd807aa98, 0x12835b01, 0x243185be, 0x550c7dc3, + 0x72be5d74, 0x80deb1fe, 0x9bdc06a7, 0xc19bf174, 0xe49b69c1, 0xefbe4786, + 0x0fc19dc6, 0x240ca1cc, 0x2de92c6f, 0x4a7484aa, 0x5cb0a9dc, 0x76f988da, + 0x983e5152, 0xa831c66d, 0xb00327c8, 0xbf597fc7, 0xc6e00bf3, 0xd5a79147, + 0x06ca6351, 0x14292967, 0x27b70a85, 0x2e1b2138, 0x4d2c6dfc, 0x53380d13, + 0x650a7354, 0x766a0abb, 0x81c2c92e, 0x92722c85, 0xa2bfe8a1, 0xa81a664b, + 0xc24b8b70, 0xc76c51a3, 0xd192e819, 0xd6990624, 0xf40e3585, 0x106aa070, + 0x19a4c116, 0x1e376c08, 0x2748774c, 0x34b0bcb5, 0x391c0cb3, 0x4ed8aa4a, + 0x5b9cca4f, 0x682e6ff3, 0x748f82ee, 0x78a5636f, 0x84c87814, 0x8cc70208, + 0x90befffa, 0xa4506ceb, 0xbef9a3f7, 0xc67178f2, +]); + +/** Initial hash value: fractional parts of the square roots of the first eight + * primes (FIPS 180-4 §5.3.3). */ +const H0 = new Uint32Array([ + 0x6a09e667, 0xbb67ae85, 0x3c6ef372, 0xa54ff53a, 0x510e527f, 0x9b05688c, + 0x1f83d9ab, 0x5be0cd19, +]); + +const rotr = (x: number, n: number): number => (x >>> n) | (x << (32 - n)); + +/** + * The raw 32-byte SHA-256 digest of `bytes`. + * + * Operates on a copy of the view's own range, so a `Uint8Array` that is a + * window into a larger buffer hashes only what it spans — the same guarantee + * the WebCrypto path makes. + */ +export function sha256Bytes(bytes: Uint8Array): Uint8Array { + const message = new Uint8Array(bytes); + const bitLength = message.length * 8; + // Padded length: message + the mandatory 0x80 byte + zeros + a 64-bit length, + // rounded up to a whole number of 64-byte blocks. + const withLength = message.length + 9; + const padded = new Uint8Array(Math.ceil(withLength / 64) * 64); + padded.set(message); + padded[message.length] = 0x80; + + const view = new DataView(padded.buffer); + // The length field is 64 bits. A message long enough to overflow the low 32 + // would be 512 MiB, well past the extension's 16 MiB per-skill limit, but the + // high word is written correctly rather than assumed zero. + view.setUint32(padded.length - 8, Math.floor(bitLength / 0x100000000)); + view.setUint32(padded.length - 4, bitLength >>> 0); + + const h = new Uint32Array(H0); + const w = new Uint32Array(64); + + for (let offset = 0; offset < padded.length; offset += 64) { + for (let i = 0; i < 16; i += 1) w[i] = view.getUint32(offset + i * 4); + for (let i = 16; i < 64; i += 1) { + const s0 = rotr(w[i - 15], 7) ^ rotr(w[i - 15], 18) ^ (w[i - 15] >>> 3); + const s1 = rotr(w[i - 2], 17) ^ rotr(w[i - 2], 19) ^ (w[i - 2] >>> 10); + w[i] = (w[i - 16] + s0 + w[i - 7] + s1) >>> 0; + } + + let [a, b, c, d, e, f, g, hh] = h; + for (let i = 0; i < 64; i += 1) { + const S1 = rotr(e, 6) ^ rotr(e, 11) ^ rotr(e, 25); + const ch = (e & f) ^ (~e & g); + const temp1 = (hh + S1 + ch + K[i] + w[i]) >>> 0; + const S0 = rotr(a, 2) ^ rotr(a, 13) ^ rotr(a, 22); + const maj = (a & b) ^ (a & c) ^ (b & c); + const temp2 = (S0 + maj) >>> 0; + hh = g; + g = f; + f = e; + e = (d + temp1) >>> 0; + d = c; + c = b; + b = a; + a = (temp1 + temp2) >>> 0; + } + + h[0] = (h[0] + a) >>> 0; + h[1] = (h[1] + b) >>> 0; + h[2] = (h[2] + c) >>> 0; + h[3] = (h[3] + d) >>> 0; + h[4] = (h[4] + e) >>> 0; + h[5] = (h[5] + f) >>> 0; + h[6] = (h[6] + g) >>> 0; + h[7] = (h[7] + hh) >>> 0; + } + + const digest = new Uint8Array(32); + const out = new DataView(digest.buffer); + for (let i = 0; i < 8; i += 1) out.setUint32(i * 4, h[i]); + return digest; +} diff --git a/core/mcp/skills.ts b/core/mcp/skills.ts index 586b110d..610299fe 100644 --- a/core/mcp/skills.ts +++ b/core/mcp/skills.ts @@ -41,6 +41,7 @@ import { type SkillEntry, type SkillResource, } from "./skillsSchemas.js"; +import { sha256Bytes } from "./sha256.js"; /** Maximum resource entries a single skill may declare (SEP-2640). */ export const SKILL_MAX_RESOURCE_ENTRIES = 512; @@ -51,6 +52,9 @@ export const SKILL_MAX_TOTAL_BYTES = 16 * 1024 * 1024; /** The suffix every skill URI ends with; the segment before it is the name. */ export const SKILL_FILE_SUFFIX = "/SKILL.md"; +/** The URI scheme SEP-2640 defines for skills, as `URL.protocol` spells it. */ +export const SKILL_URI_SCHEME = "skill:"; + /** `sha256:` followed by exactly 64 lowercase hex characters. */ const DIGEST_PATTERN = /^sha256:[0-9a-f]{64}$/; @@ -107,6 +111,10 @@ export function isSkillsExtensionSupported( * NOT normalized by the parser — its `..` segments survive verbatim — so it is * rejected too: containment could not be decided on it, and silently accepting * one would reintroduce exactly the hole this function closes. + * + * The `skill:` scheme is required. Checking only that the URI is hierarchical + * would let `https://demo/SKILL.md` through and then pass the name and root + * checks — a manifest pointing anywhere on the web, reported as conforming. */ export function normalizeSkillUri(uri: string): string | undefined { let parsed: URL; @@ -115,6 +123,7 @@ export function normalizeSkillUri(uri: string): string | undefined { } catch { return undefined; } + if (parsed.protocol !== SKILL_URI_SCHEME) return undefined; return parsed.pathname.startsWith("/") ? parsed.href : undefined; } @@ -167,9 +176,10 @@ export type SkillIssueCode = | "size-limit-exceeded"; /** - * `error` marks a stated requirement of SEP-2640 that the server broke. - * `warning` marks something that is legal but leaves the Inspector unable to - * verify integrity — `"dynamic"` resources above all, which is the case most + * `error` marks a stated requirement of SEP-2640 that the server broke — + * every MUST, so a manifest reporting "0 errors" really is one the spec + * accepts. `warning` is reserved for what is **legal** yet leaves the + * Inspector unable to verify integrity: `"dynamic"` resources, the case most * worth surfacing and the one most easily buried. */ export type SkillIssueSeverity = "error" | "warning"; @@ -216,7 +226,7 @@ export function checkSkillConformance(entry: SkillEntry): SkillIssue[] { issues.push({ code: "malformed-uri", severity: "error", - message: `Skill URI must end with "${SKILL_FILE_SUFFIX}" and carry a non-empty path segment before it.`, + message: `Skill URI must be a "${SKILL_URI_SCHEME}//" URI ending with "${SKILL_FILE_SUFFIX}" and carrying a non-empty path segment before it.`, }); } else if (declaredName && uriName !== declaredName) { // The one structural invariant the spec states outright: the segment before @@ -309,10 +319,15 @@ export function checkSkillConformance(entry: SkillEntry): SkillIssue[] { } } if (resource.digest === undefined) { + // An error, not a warning: SEP-2640 requires `digest` on every manifest + // entry, so an entry without one is invalid — and reporting it as a + // warning would let such a manifest show "0 errors", which is the + // affirmative pass this checker must never give. issues.push({ code: "missing-digest", - severity: "warning", - message: "Manifest entry declares no digest, so it cannot be verified.", + severity: "error", + message: + "Manifest entry declares no digest, which is required — and without it the file cannot be verified.", resourceUri: resource.uri, }); } else if (!DIGEST_PATTERN.test(resource.digest)) { @@ -324,14 +339,15 @@ export function checkSkillConformance(entry: SkillEntry): SkillIssue[] { }); } if (resource.size === undefined) { - // A warning, not an error: an absent size costs the length cross-check in - // `verifySkillResource` and silently understates the 16 MiB total, but - // the digest still verifies the bytes. + // Also an error: `size` is a required field, not an integrity hint, and + // an omitted one is what lets a server slip past the 16 MiB pre-fetch + // limit — the entry is excluded from the total — while the UI reports no + // conformance errors at all. issues.push({ code: "missing-size", - severity: "warning", + severity: "error", message: - "Manifest entry declares no size, so it is excluded from the 16 MiB total and its length cannot be cross-checked.", + "Manifest entry declares no size, which is required — and without it the entry is excluded from the 16 MiB total and its length cannot be cross-checked.", resourceUri: resource.uri, }); } else if (!isUsableSize(resource.size)) { @@ -410,6 +426,15 @@ function toHex(bytes: Uint8Array): string { * context, so `subtle` is present there too. */ export async function sha256Digest(bytes: Uint8Array): Promise { + // `crypto.subtle` is exposed only in a SECURE CONTEXT, and this app is + // documented as servable over plain HTTP on a LAN IP + // (`clients/web/README.md#hosting-on-a-network`). There, `crypto` exists but + // `crypto.subtle` does not — so without this fallback every verification + // would throw and the UI would report a read failure for a file it fetched + // perfectly well. `sha256Bytes` is checked against the published FIPS 180-4 + // vectors and differentially against WebCrypto, so the two paths agree. + const subtle = globalThis.crypto?.subtle; + if (!subtle) return `sha256:${toHex(sha256Bytes(bytes))}`; // Copy the VIEW into a fresh typed array rather than slicing its backing // store. Two things depend on that: a `Uint8Array` can be a window into a // larger buffer, so hashing the buffer would digest neighbouring bytes; and @@ -419,7 +444,7 @@ export async function sha256Digest(bytes: Uint8Array): Promise { // `new Uint8Array(view)` always allocates a plain `ArrayBuffer`, which is // also why no cast is needed here. const copy = new Uint8Array(bytes); - const hash = await crypto.subtle.digest("SHA-256", copy.buffer); + const hash = await subtle.digest("SHA-256", copy.buffer); return `sha256:${toHex(new Uint8Array(hash))}`; } diff --git a/core/mcp/skillsSchemas.ts b/core/mcp/skillsSchemas.ts index 1441b3ea..2ebb7181 100644 --- a/core/mcp/skillsSchemas.ts +++ b/core/mcp/skillsSchemas.ts @@ -96,7 +96,21 @@ export const SkillEntrySchema = z.looseObject({ export type SkillEntry = z.infer; -/** `skills/list` result: a page of entries plus the opaque cursor. */ +/** + * `skills/list` result: a page of entries plus the opaque cursor. + * + * ⚠️ **Whether a modern-era (2026-07-28) result must also carry the SEP-2549 + * caching attributes `ttlMs` / `cacheScope` is unsettled here and deliberately + * not guessed.** #2234's analysis records it as an open point; a review of that + * PR asserted the opposite. Neither reading was checked against the normative + * text, and the two mistakes are not symmetric: leaving the schema permissive + * means a server that omits them is accepted (they pass through untouched when + * sent), while tightening on a wrong reading would *reject* conforming + * responses. `resources/directory/read` was removed from this module for the + * same reason. #2248 settles it against the spec. Note the SDK is no help + * either way — `skills/list` is consumer-owned, so it is absent from the + * cacheable-method registry and nothing stamps or validates these fields. + */ export const ListSkillsResultSchema = z.looseObject({ skills: z.array(SkillEntrySchema), nextCursor: z.string().optional(),