fix: accept a path-less skill root URI in normalizeSkillUri (#2295)
`skill://data-analysis` has an authority and an empty path, which RFC 3986 permits, but the guard required a pathname starting with "/". A server echoing the skill root back in a directory read was therefore labelled "outside this skill" instead of "not a direct child". Opaque URIs (`skill:demo`) stay rejected; a path-less root still names no skill file, so it fails every check that needs one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VQ2eszDCUqTmhtTMaznw3W Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
3e728a3b8c
commit
528a37be38
3 changed files with 63 additions and 1 deletions
|
|
@ -2184,6 +2184,31 @@ describe("SkillsScreen directory browsing (#2248)", () => {
|
||||||
).not.toBeInTheDocument();
|
).not.toBeInTheDocument();
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("names a skill root echoed back among its own children as not a direct child (#2295)", async () => {
|
||||||
|
// The root is path-less (`skill://data-analysis`), which
|
||||||
|
// `normalizeSkillUri` used to reject — so the containment check failed and
|
||||||
|
// the row read "outside this skill" while being the skill itself.
|
||||||
|
const user = userEvent.setup();
|
||||||
|
const ROOT_SELF = {
|
||||||
|
uri: ROOT,
|
||||||
|
name: "data-analysis",
|
||||||
|
mimeType: "inode/directory",
|
||||||
|
};
|
||||||
|
await openRoot(
|
||||||
|
user,
|
||||||
|
directoryReader({ [ROOT]: { resources: [CHILD_FILE, ROOT_SELF] } }),
|
||||||
|
);
|
||||||
|
const table = within(screen.getByTestId("skill-directory"));
|
||||||
|
expect(table.getByText(/not a direct child/)).toBeInTheDocument();
|
||||||
|
expect(table.queryByText(/outside this skill/)).not.toBeInTheDocument();
|
||||||
|
expect(
|
||||||
|
table.queryByRole("button", { name: `Open directory ${ROOT}` }),
|
||||||
|
).not.toBeInTheDocument();
|
||||||
|
expect(
|
||||||
|
table.getByRole("button", { name: `View ${CHILD_FILE.uri}` }),
|
||||||
|
).toBeInTheDocument();
|
||||||
|
});
|
||||||
|
|
||||||
it("navigates on the normalized URI, so Up cannot walk into a `..` segment", async () => {
|
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
|
// Containment was decided on the normalized URI while navigation sent and
|
||||||
// stored the raw one, so for `skill://root/a/../templates` the first Up
|
// stored the raw one, so for `skill://root/a/../templates` the first Up
|
||||||
|
|
|
||||||
|
|
@ -174,6 +174,31 @@ describe("normalizeSkillUri", () => {
|
||||||
// `skill:demo/../x.md` parses but keeps its `..` verbatim, so containment
|
// `skill:demo/../x.md` parses but keeps its `..` verbatim, so containment
|
||||||
// could not be decided on it — accepting it would reopen the hole.
|
// could not be decided on it — accepting it would reopen the hole.
|
||||||
expect(normalizeSkillUri("skill:demo/SKILL.md")).toBeUndefined();
|
expect(normalizeSkillUri("skill:demo/SKILL.md")).toBeUndefined();
|
||||||
|
// Path-less but still opaque: no authority, so it stays rejected.
|
||||||
|
expect(normalizeSkillUri("skill:demo")).toBeUndefined();
|
||||||
|
});
|
||||||
|
|
||||||
|
it("accepts a path-less authority URI — a skill's root directory (#2295)", () => {
|
||||||
|
// RFC 3986 gives an authority URI an empty path, so `skill://data-analysis`
|
||||||
|
// is well-formed; rejecting it mislabelled a root echoed back by a
|
||||||
|
// directory read as "outside this skill".
|
||||||
|
expect(normalizeSkillUri("skill://data-analysis")).toBe(
|
||||||
|
"skill://data-analysis",
|
||||||
|
);
|
||||||
|
expect(normalizeSkillUri("skill://%64ata-analysis")).toBe(
|
||||||
|
"skill://data-analysis",
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("still treats a path-less root as naming no skill file", () => {
|
||||||
|
// Accepting the root must not let it pass a check that needs a file.
|
||||||
|
expect(skillNameFromUri("skill://data-analysis")).toBeUndefined();
|
||||||
|
const issues = checkSkillConformance({
|
||||||
|
uri: "skill://data-analysis",
|
||||||
|
frontmatter: { name: "data-analysis", description: "d" },
|
||||||
|
resources: [],
|
||||||
|
});
|
||||||
|
expect(issues.map((issue) => issue.code)).toContain("malformed-uri");
|
||||||
});
|
});
|
||||||
|
|
||||||
it("decodes escapes that stand for unreserved characters", () => {
|
it("decodes escapes that stand for unreserved characters", () => {
|
||||||
|
|
|
||||||
|
|
@ -171,6 +171,16 @@ export function isSkillsExtensionSupported(
|
||||||
* rejected too: containment could not be decided on it, and silently accepting
|
* rejected too: containment could not be decided on it, and silently accepting
|
||||||
* one would reintroduce exactly the hole this function closes.
|
* one would reintroduce exactly the hole this function closes.
|
||||||
*
|
*
|
||||||
|
* A **path-less authority** URI (`skill://data-analysis`) is accepted, and is
|
||||||
|
* not the opaque case above: RFC 3986 gives a URI with an authority an empty
|
||||||
|
* path, and the parser reports that as `pathname === ""`. It has no segments,
|
||||||
|
* so nothing survives unnormalized. It is what a skill's root directory looks
|
||||||
|
* like once `/SKILL.md` is removed, so rejecting it made a server echoing that
|
||||||
|
* root back in a directory read read as "outside this skill" when it is the
|
||||||
|
* skill itself (#2295). It names no file, so it still fails every check that
|
||||||
|
* needs one — `skillNameFromUri` wants the `/SKILL.md` suffix, and a manifest
|
||||||
|
* entry spelled this way does not start with `<root>/`.
|
||||||
|
*
|
||||||
* The scheme is deliberately **not** constrained. `skill://` is what SEP-2640
|
* The scheme is deliberately **not** constrained. `skill://` is what SEP-2640
|
||||||
* recommends and what this repo's fixture serves, but the SEP only says servers
|
* recommends and what this repo's fixture serves, but the SEP only says servers
|
||||||
* SHOULD use it and explicitly allows a domain-native scheme (`github://…`), so
|
* SHOULD use it and explicitly allows a domain-native scheme (`github://…`), so
|
||||||
|
|
@ -186,7 +196,9 @@ export function normalizeSkillUri(uri: string): string | undefined {
|
||||||
} catch {
|
} catch {
|
||||||
return undefined;
|
return undefined;
|
||||||
}
|
}
|
||||||
if (!parsed.pathname.startsWith("/")) return undefined;
|
if (parsed.pathname !== "" && !parsed.pathname.startsWith("/")) {
|
||||||
|
return undefined;
|
||||||
|
}
|
||||||
return canonicalizePercentEncoding(parsed.href);
|
return canonicalizePercentEncoding(parsed.href);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue