From f85cc340742a04b3ae54c0ea3304f5ddb215052d Mon Sep 17 00:00:00 2001 From: Arseny Kravchenko Date: Fri, 17 Jul 2026 15:45:50 +0200 Subject: [PATCH 1/3] fix(skills-sandbox): reject inputs that poison a sandbox's replay log Three validation gaps let a value that later fails Rust replay conversion get persisted into the append-only sandbox log, permanently bricking a conversation's default sandbox (every later run_command/download_file fails). Fixed at the input boundary so no new poison is persisted; the Rust replay boundary is left as-is (tightening it would strand already-persisted mounts). - upload/artifact paths: reject a `.` path segment as well as `..`. A path like /home/sandbox/. resolves to a directory, so the replayed `base64 -d > ` redirect fails on every run. Fixed in both the TS twin (resolveArtifactPath) and the Rust validators (validate_upload_path, validate_artifact_path), with the mirrored vector tables kept in sync. - skill name: reject a name that is `.` or contains `/` or `..` at parseSkillManifest, mirroring the Rust skill_root_path mount boundary. - resource file path: reject empty, `.`, and `..` segments in SkillFileInputSchema (was: only leading-/ and `..`). Forward-only: existing rows with poison names/paths are tracked as a separate data-migration follow-up. --- .../archestra-rs/sandbox-core/src/validation.rs | 13 +++++++++++-- .../src/routes/skill/create.skill.route.test.ts | 14 ++++++++++++++ .../skill-sandbox-runtime-service.test.ts | 6 ++++++ .../skill-sandbox-runtime-service.ts | 9 ++++++++- platform/backend/src/skills/parser.test.ts | 16 ++++++++++++++++ platform/backend/src/skills/parser.ts | 9 +++++++++ platform/backend/src/skills/validation.ts | 7 ++++--- 7 files changed, 68 insertions(+), 6 deletions(-) diff --git a/platform/archestra-rs/sandbox-core/src/validation.rs b/platform/archestra-rs/sandbox-core/src/validation.rs index a1c13742a0a..64c33e5e43f 100644 --- a/platform/archestra-rs/sandbox-core/src/validation.rs +++ b/platform/archestra-rs/sandbox-core/src/validation.rs @@ -32,7 +32,7 @@ pub(crate) fn validate_snapshot_file_path(path: &str) -> Result<()> { } pub(crate) fn validate_artifact_path(path: &str) -> Result<()> { - if path.contains('\0') || path.split('/').any(|segment| segment == "..") { + if path.contains('\0') || path.split('/').any(|segment| matches!(segment, ".." | ".")) { return Err(SandboxError::InvalidInput(format!( "invalid artifact path: {path:?}" ))); @@ -58,7 +58,10 @@ pub(crate) fn validate_artifact_path(path: &str) -> Result<()> { /// absolute file under the sandbox roots, free of traversal, null bytes, and /// shell metacharacters (defense in depth on top of the single-quoting). pub(crate) fn validate_upload_path(path: &str) -> Result<()> { - if path.contains('\0') || path.split('/').any(|segment| segment == "..") { + // reject `.` segments as well as `..`: a path like `/home/sandbox/.` + // resolves to a directory, so the replayed `base64 -d > ` redirect + // fails on every run and permanently wedges the sandbox. + if path.contains('\0') || path.split('/').any(|segment| matches!(segment, ".." | ".")) { return Err(SandboxError::InvalidInput(format!( "invalid upload path: {path:?}" ))); @@ -198,6 +201,10 @@ mod tests { ("/home/sandbox/../etc/passwd", false, false), // directory, not a file ("/home/sandbox/", false, false), + // `.` segment resolves to a directory: rejected before it can be + // persisted as a replay event that fails `base64 -d > ` forever + ("/home/sandbox/.", false, false), + ("/home/sandbox/./x", false, false), // a root itself: replay would fail on the existing directory, so the // TS layer rejects it before it is persisted as an unreplayable event ("/home/sandbox", true, false), @@ -220,6 +227,8 @@ mod tests { ("/etc/passwd", false, false), // traversal ("a/../b.txt", false, false), + // `.` segment resolves to a directory + ("/skills/alpha/.", false, false), // null byte ("a\0b.txt", false, false), // shell metacharacters: only this boundary rejects them; the TS layer diff --git a/platform/backend/src/routes/skill/create.skill.route.test.ts b/platform/backend/src/routes/skill/create.skill.route.test.ts index 1b0d518e017..658c7bac756 100644 --- a/platform/backend/src/routes/skill/create.skill.route.test.ts +++ b/platform/backend/src/routes/skill/create.skill.route.test.ts @@ -130,6 +130,20 @@ describe("POST /api/skills", () => { expect(response.statusCode).toBe(400); }); + test("rejects a resource path that would break the sandbox mount with a 400", async () => { + for (const path of ["scripts/", "./run.py", "a//b.py", "run.py/."]) { + const response = await ctx.app.inject({ + method: "POST", + url: "/api/skills", + payload: { + content: MANIFEST, + files: [{ path, content: "print(1)" }], + }, + }); + expect(response.statusCode, `path: ${JSON.stringify(path)}`).toBe(400); + } + }); + test("rejects a manifest larger than the size cap", async () => { const response = await ctx.app.inject({ method: "POST", diff --git a/platform/backend/src/skills-sandbox/skill-sandbox-runtime-service.test.ts b/platform/backend/src/skills-sandbox/skill-sandbox-runtime-service.test.ts index 56576d95cf9..402adfc8e1e 100644 --- a/platform/backend/src/skills-sandbox/skill-sandbox-runtime-service.test.ts +++ b/platform/backend/src/skills-sandbox/skill-sandbox-runtime-service.test.ts @@ -761,6 +761,10 @@ describe("path validation vectors (mirrored with sandbox-core)", () => { ["/home/sandbox/../etc/passwd", false, false], // directory, not a file ["/home/sandbox/", false, false], + // `.` segment resolves to a directory: rejected before it can be persisted + // as a replay event that fails `base64 -d > ` forever + ["/home/sandbox/.", false, false], + ["/home/sandbox/./x", false, false], // a root itself: rejected here before it is persisted as an unreplayable // event; the boundary alone would accept it ["/home/sandbox", false, true], @@ -782,6 +786,8 @@ describe("path validation vectors (mirrored with sandbox-core)", () => { ["/etc/passwd", false, false], // traversal ["a/../b.txt", false, false], + // `.` segment resolves to a directory + ["/skills/alpha/.", false, false], // null byte ["a\0b.txt", false, false], // shell metacharacters: pass through here; the Rust boundary rejects them diff --git a/platform/backend/src/skills-sandbox/skill-sandbox-runtime-service.ts b/platform/backend/src/skills-sandbox/skill-sandbox-runtime-service.ts index 3095905ef6f..e082c8be774 100644 --- a/platform/backend/src/skills-sandbox/skill-sandbox-runtime-service.ts +++ b/platform/backend/src/skills-sandbox/skill-sandbox-runtime-service.ts @@ -879,7 +879,14 @@ function resolveArtifactPath(params: { `invalid artifact path: ${JSON.stringify(params.path)}`, ); } - if (params.path.split("/").some((segment) => segment === "..")) { + // reject `.` segments as well as `..`: `/home/sandbox/.` resolves to a + // directory, so a persisted upload event would fail `base64 -d > ` on + // every later replay and wedge the sandbox permanently. + if ( + params.path + .split("/") + .some((segment) => segment === ".." || segment === ".") + ) { rejectPath( "artifact_path_traversal", `invalid artifact path: ${JSON.stringify(params.path)}`, diff --git a/platform/backend/src/skills/parser.test.ts b/platform/backend/src/skills/parser.test.ts index 3bfe5963a06..e0ecc907212 100644 --- a/platform/backend/src/skills/parser.test.ts +++ b/platform/backend/src/skills/parser.test.ts @@ -134,6 +134,22 @@ describe("parseSkillManifest", () => { expect(() => parseSkillManifest(raw)).toThrow(/description/); }); + test("rejects a name that breaks the sandbox mount root", () => { + for (const name of ["foo/bar", "..", "a/../b", ".", "skills/../x"]) { + const raw = [ + "---", + `name: "${name}"`, + "description: A skill.", + "---", + "Body.", + ].join("\n"); + expect( + () => parseSkillManifest(raw), + `name: ${JSON.stringify(name)}`, + ).toThrow(SkillParseError); + } + }); + test("throws on invalid YAML frontmatter", () => { const raw = ["---", "name: : :", " bad", "---", "Body."].join("\n"); expect(() => parseSkillManifest(raw)).toThrow(SkillParseError); diff --git a/platform/backend/src/skills/parser.ts b/platform/backend/src/skills/parser.ts index a0768e1c4af..f52c24586f1 100644 --- a/platform/backend/src/skills/parser.ts +++ b/platform/backend/src/skills/parser.ts @@ -75,6 +75,15 @@ export function parseSkillManifest(raw: string): ParsedSkill { if (!name) { throw new SkillParseError("SKILL.md frontmatter is missing `name`"); } + // the name becomes the sandbox mount root `/skills/`; a name that is + // `.` or carries `/` or `..` collapses or escapes that root and makes every + // later mount replay-fail, permanently wedging the sandbox. mirror the Rust + // `skill_root_path` boundary (archestra-rs/sandbox-core/src/validation.rs). + if (name === "." || name.includes("/") || name.includes("..")) { + throw new SkillParseError( + "SKILL.md `name` must not be `.` or contain `/` or `..`", + ); + } if (!description) { throw new SkillParseError("SKILL.md frontmatter is missing `description`"); } diff --git a/platform/backend/src/skills/validation.ts b/platform/backend/src/skills/validation.ts index 138194e655c..ddf118b96c3 100644 --- a/platform/backend/src/skills/validation.ts +++ b/platform/backend/src/skills/validation.ts @@ -20,10 +20,11 @@ export const SkillFileInputSchema = z.object({ .string() .min(1) .refine( - (p) => !p.startsWith("/") && !p.split("/").some((s) => s === ".."), + (p) => + !p.startsWith("/") && + p.split("/").every((s) => s !== "" && s !== "." && s !== ".."), { - message: - "path must be relative and must not contain directory traversal sequences", + message: "path must be relative, with no empty, `.`, or `..` segments", }, ) .describe("Resource path, e.g. references/API.md or scripts/run.py"), From 5acdbbdd40f86f2d90bedb6321c56bd6b18e03fe Mon Sep 17 00:00:00 2001 From: Arseny Kravchenko Date: Fri, 17 Jul 2026 16:06:34 +0200 Subject: [PATCH 2/3] =?UTF-8?q?fix(skills-sandbox):=20address=20review=20?= =?UTF-8?q?=E2=80=94=20narrow=20`.`=20rejection,=20close=20bypasses?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of the poison-fix diff surfaced three issues: - B1/C7 over-rejected: rejecting every `.` segment wrongly refused conventional relative paths (`./result.txt`, `/home/sandbox/./x`, `a//b.py`) that resolve to a regular file and the shell handles fine. Narrow to a TERMINAL `.` or empty segment (the only forms that resolve to a directory) in the upload/ artifact validators (TS + Rust) and the resource-path schema. Mirrored vector tables updated to accept the non-terminal cases. - C6 bypass: built-in skills seeded with a white-label app name skip parseSkillManifest, so an app name with `/` or `..` produced a poison skill name. Add validateSkillMountName at the mount chokepoint, mirroring the Rust skill_root_path boundary, so no source can persist an unreplayable mount. - C7 bypass: GitHub import built resource paths without the schema. Extract a shared isSafeSkillResourcePath predicate and apply it in both the input schema and the importer. Also: distinct `artifact_path_directory` reject reason for the terminal-`.` case. Existing poison rows and the app-name→built-in-skill-name normalization are tracked as follow-ups (forward-only fix). --- .../sandbox-core/src/validation.rs | 34 +++++++++++---- .../routes/skill/create.skill.route.test.ts | 18 +++++++- .../skill-sandbox-runtime-service.test.ts | 27 ++++++++++-- .../skill-sandbox-runtime-service.ts | 43 +++++++++++++++---- platform/backend/src/skills/github-import.ts | 6 ++- platform/backend/src/skills/parser.ts | 18 ++++++++ platform/backend/src/skills/validation.ts | 18 ++++---- 7 files changed, 134 insertions(+), 30 deletions(-) diff --git a/platform/archestra-rs/sandbox-core/src/validation.rs b/platform/archestra-rs/sandbox-core/src/validation.rs index 64c33e5e43f..67de46d0a3e 100644 --- a/platform/archestra-rs/sandbox-core/src/validation.rs +++ b/platform/archestra-rs/sandbox-core/src/validation.rs @@ -32,7 +32,10 @@ pub(crate) fn validate_snapshot_file_path(path: &str) -> Result<()> { } pub(crate) fn validate_artifact_path(path: &str) -> Result<()> { - if path.contains('\0') || path.split('/').any(|segment| matches!(segment, ".." | ".")) { + if path.contains('\0') + || path.split('/').any(|segment| segment == "..") + || resolves_to_directory(path) + { return Err(SandboxError::InvalidInput(format!( "invalid artifact path: {path:?}" ))); @@ -58,10 +61,14 @@ pub(crate) fn validate_artifact_path(path: &str) -> Result<()> { /// absolute file under the sandbox roots, free of traversal, null bytes, and /// shell metacharacters (defense in depth on top of the single-quoting). pub(crate) fn validate_upload_path(path: &str) -> Result<()> { - // reject `.` segments as well as `..`: a path like `/home/sandbox/.` - // resolves to a directory, so the replayed `base64 -d > ` redirect - // fails on every run and permanently wedges the sandbox. - if path.contains('\0') || path.split('/').any(|segment| matches!(segment, ".." | ".")) { + // a path whose final component is `.` (e.g. `/home/sandbox/.`) resolves to a + // directory, so the replayed `base64 -d > ` redirect fails on every + // run and permanently wedges the sandbox. a non-terminal `.` (`a/./b`) + // resolves to a regular file and is left alone. + if path.contains('\0') + || path.split('/').any(|segment| segment == "..") + || resolves_to_directory(path) + { return Err(SandboxError::InvalidInput(format!( "invalid upload path: {path:?}" ))); @@ -156,6 +163,14 @@ fn within_sandbox_roots(path: &str) -> bool { .any(|root| path == *root || path.strip_prefix(root).is_some_and(|r| r.starts_with('/'))) } +/// true when the path's final component makes it resolve to a directory rather +/// than a file — a terminal `.` (`/home/sandbox/.` or a bare `.`). Such a path +/// breaks the `> ` redirect that writes an uploaded/exported file. a +/// non-terminal `.` (`a/./b`) normalizes to a regular file and is left alone. +fn resolves_to_directory(path: &str) -> bool { + path == "." || path.ends_with("/.") +} + #[cfg(test)] mod tests { use super::*; @@ -201,10 +216,11 @@ mod tests { ("/home/sandbox/../etc/passwd", false, false), // directory, not a file ("/home/sandbox/", false, false), - // `.` segment resolves to a directory: rejected before it can be + // terminal `.` resolves to a directory: rejected before it can be // persisted as a replay event that fails `base64 -d > ` forever ("/home/sandbox/.", false, false), - ("/home/sandbox/./x", false, false), + // non-terminal `.` normalizes to a regular file: accepted + ("/home/sandbox/./x", true, true), // a root itself: replay would fail on the existing directory, so the // TS layer rejects it before it is persisted as an unreplayable event ("/home/sandbox", true, false), @@ -227,8 +243,10 @@ mod tests { ("/etc/passwd", false, false), // traversal ("a/../b.txt", false, false), - // `.` segment resolves to a directory + // terminal `.` resolves to a directory ("/skills/alpha/.", false, false), + // non-terminal `.` normalizes to a regular file: accepted + ("/skills/alpha/./x", true, true), // null byte ("a\0b.txt", false, false), // shell metacharacters: only this boundary rejects them; the TS layer diff --git a/platform/backend/src/routes/skill/create.skill.route.test.ts b/platform/backend/src/routes/skill/create.skill.route.test.ts index 658c7bac756..d485c301401 100644 --- a/platform/backend/src/routes/skill/create.skill.route.test.ts +++ b/platform/backend/src/routes/skill/create.skill.route.test.ts @@ -131,7 +131,8 @@ describe("POST /api/skills", () => { }); test("rejects a resource path that would break the sandbox mount with a 400", async () => { - for (const path of ["scripts/", "./run.py", "a//b.py", "run.py/."]) { + // only paths that traverse or resolve to a directory are rejected. + for (const path of ["scripts/", "run.py/.", "../escape", "a/b/"]) { const response = await ctx.app.inject({ method: "POST", url: "/api/skills", @@ -144,6 +145,21 @@ describe("POST /api/skills", () => { } }); + test("accepts a resource path with a non-terminal `.` or `//` alias", async () => { + const response = await ctx.app.inject({ + method: "POST", + url: "/api/skills", + payload: { + content: MANIFEST, + files: [ + { path: "./scripts/run.py", content: "print(1)" }, + { path: "references//API.md", content: "# API" }, + ], + }, + }); + expect(response.statusCode).toBe(200); + }); + test("rejects a manifest larger than the size cap", async () => { const response = await ctx.app.inject({ method: "POST", diff --git a/platform/backend/src/skills-sandbox/skill-sandbox-runtime-service.test.ts b/platform/backend/src/skills-sandbox/skill-sandbox-runtime-service.test.ts index 402adfc8e1e..4939a810f3c 100644 --- a/platform/backend/src/skills-sandbox/skill-sandbox-runtime-service.test.ts +++ b/platform/backend/src/skills-sandbox/skill-sandbox-runtime-service.test.ts @@ -380,6 +380,24 @@ describe("__internals", () => { } }); + test("validateSkillMountName rejects names that collapse or escape the mount root", () => { + // covers names that skip parseSkillManifest (e.g. a built-in skill seeded + // with a white-label app name containing "/") and would otherwise persist + // an unreplayable mount. + for (const bad of ["", ".", "a/b", "..", "a..b", "skills/../x"]) { + expect( + () => __internals.validateSkillMountName(bad), + `name: ${JSON.stringify(bad)}`, + ).toThrow(); + } + for (const ok of ["alpha", "my-skill", "Data Analysis"]) { + expect( + () => __internals.validateSkillMountName(ok), + `name: ${JSON.stringify(ok)}`, + ).not.toThrow(); + } + }); + test("sanitizeAttachmentName strips unsafe chars and directory/leading dots", () => { const { sanitizeAttachmentName } = __internals; expect(sanitizeAttachmentName("pi mc.gif", "id")).toBe("pi_mc.gif"); @@ -761,10 +779,11 @@ describe("path validation vectors (mirrored with sandbox-core)", () => { ["/home/sandbox/../etc/passwd", false, false], // directory, not a file ["/home/sandbox/", false, false], - // `.` segment resolves to a directory: rejected before it can be persisted + // terminal `.` resolves to a directory: rejected before it can be persisted // as a replay event that fails `base64 -d > ` forever ["/home/sandbox/.", false, false], - ["/home/sandbox/./x", false, false], + // non-terminal `.` normalizes to a regular file: accepted + ["/home/sandbox/./x", true, true], // a root itself: rejected here before it is persisted as an unreplayable // event; the boundary alone would accept it ["/home/sandbox", false, true], @@ -786,8 +805,10 @@ describe("path validation vectors (mirrored with sandbox-core)", () => { ["/etc/passwd", false, false], // traversal ["a/../b.txt", false, false], - // `.` segment resolves to a directory + // terminal `.` resolves to a directory ["/skills/alpha/.", false, false], + // non-terminal `.` normalizes to a regular file: accepted + ["/skills/alpha/./x", true, true], // null byte ["a\0b.txt", false, false], // shell metacharacters: pass through here; the Rust boundary rejects them diff --git a/platform/backend/src/skills-sandbox/skill-sandbox-runtime-service.ts b/platform/backend/src/skills-sandbox/skill-sandbox-runtime-service.ts index e082c8be774..8ca3e778314 100644 --- a/platform/backend/src/skills-sandbox/skill-sandbox-runtime-service.ts +++ b/platform/backend/src/skills-sandbox/skill-sandbox-runtime-service.ts @@ -462,6 +462,7 @@ class SkillSandboxRuntimeService { this.ensureEnabled(); return this.runWithSandbox(params.sandboxId, async (sandbox) => { + validateSkillMountName(params.skill.skillName); const files = await SkillVersionModel.findFiles( params.skill.skillVersionId, ); @@ -879,19 +880,22 @@ function resolveArtifactPath(params: { `invalid artifact path: ${JSON.stringify(params.path)}`, ); } - // reject `.` segments as well as `..`: `/home/sandbox/.` resolves to a - // directory, so a persisted upload event would fail `base64 -d > ` on - // every later replay and wedge the sandbox permanently. - if ( - params.path - .split("/") - .some((segment) => segment === ".." || segment === ".") - ) { + if (params.path.split("/").some((segment) => segment === "..")) { rejectPath( "artifact_path_traversal", `invalid artifact path: ${JSON.stringify(params.path)}`, ); } + // a terminal `.` (`/home/sandbox/.`, or a bare `.` against the cwd) resolves + // to a directory, so a persisted upload event would fail `base64 -d > ` + // on every later replay and wedge the sandbox permanently. a non-terminal `.` + // (`a/./b`) normalizes to a regular file and is left alone. + if (params.path === "." || params.path.endsWith("/.")) { + rejectPath( + "artifact_path_directory", + `artifact path must be a file, not a directory: ${JSON.stringify(params.path)}`, + ); + } if (params.path.startsWith("/")) { const allowedRoots = [SKILL_SANDBOX_ROOT, SKILL_SANDBOX_HOME]; const isAllowed = allowedRoots.some( @@ -963,6 +967,28 @@ function validateSkillMountFilePath(skillName: string, path: string): void { } } +/** + * Reject a skill name that cannot become the mount root `/skills/` before + * it is persisted as a mount event. Skill names authored through create/update + * are already gated at `parseSkillManifest`, but other sources reach the mount + * unvalidated — built-in skills seeded with a white-label app name, for one — + * so this mount-boundary check mirrors the Rust `skill_root_path` boundary + * (archestra-rs/sandbox-core/src/validation.rs) to keep an unreplayable mount + * out of the log regardless of where the name came from. + */ +function validateSkillMountName(skillName: string): void { + if ( + skillName === "" || + skillName === "." || + skillName.includes("/") || + skillName.includes("..") + ) { + throw new SkillSandboxError( + `Skill "${skillName}" has a name that cannot be mounted into a sandbox (it must not be "." or contain "/" or ".."). Rename the skill, then load it again.`, + ); + } +} + /** * One install command per `requirements.txt` the version ships — root or * nested (skills commonly keep tool deps in `tools/requirements.txt`) — in @@ -1165,6 +1191,7 @@ export const __internals = { resolveArtifactPath, validateUploadPath, validateSkillMountFilePath, + validateSkillMountName, requirementsInstallCommands, stageConversationAttachments, planAttachmentStaging, diff --git a/platform/backend/src/skills/github-import.ts b/platform/backend/src/skills/github-import.ts index d628b1fa9b5..171dc0f8ccd 100644 --- a/platform/backend/src/skills/github-import.ts +++ b/platform/backend/src/skills/github-import.ts @@ -6,6 +6,7 @@ import logger from "@/logging"; import type { SkillFileEncoding, SkillFileKind } from "@/types"; import { deriveSkillFileKind, + isSafeSkillResourcePath, type ParsedSkill, parseSkillManifest, SKILL_MANIFEST_FILENAME, @@ -312,7 +313,10 @@ export async function importSkills(params: { const fetched = fetchedFiles[cursor]; cursor += 1; const relativePath = plan.toRelative(absolutePath); - if (fetched === null) { + // git blob paths are structurally clean, but this keeps the importer + // behind the same resource-path boundary as create/update so a path that + // would wedge the sandbox on replay is never persisted from any source. + if (fetched === null || !isSafeSkillResourcePath(relativePath)) { plan.skippedFiles.push(relativePath); continue; } diff --git a/platform/backend/src/skills/parser.ts b/platform/backend/src/skills/parser.ts index f52c24586f1..c2f58b6ab35 100644 --- a/platform/backend/src/skills/parser.ts +++ b/platform/backend/src/skills/parser.ts @@ -126,6 +126,24 @@ export function deriveSkillFileKind(path: string): SkillFileKind { return /\.(md|mdx|txt|markdown)$/.test(normalized) ? "reference" : "asset"; } +/** + * A skill resource path is safe to persist when it is relative (no leading + * `/`), carries no `..` traversal segment, and does not resolve to a directory + * — its final segment is neither empty (a trailing slash) nor `.`. A path that + * resolves to a directory makes the Rust replay writer's `base64 -d > ` + * redirect fail on every run, permanently wedging the sandbox. Non-terminal + * `.`/empty segments (`a/./b`, `a//b`) normalize to a regular file and are + * allowed. Shared by the input schema and the GitHub importer so every + * persistence path applies the same boundary. + */ +export function isSafeSkillResourcePath(path: string): boolean { + if (path.startsWith("/")) return false; + const segments = path.split("/"); + if (segments.some((s) => s === "..")) return false; + const last = segments[segments.length - 1]; + return last !== "" && last !== "."; +} + // ===== Internal helpers ===== function readString(value: unknown): string { diff --git a/platform/backend/src/skills/validation.ts b/platform/backend/src/skills/validation.ts index ddf118b96c3..e45a18d583d 100644 --- a/platform/backend/src/skills/validation.ts +++ b/platform/backend/src/skills/validation.ts @@ -3,7 +3,11 @@ import { MAX_SKILL_FILE_BYTES, MAX_SKILL_FILE_CONTENT_CHARS, } from "@/skills/github-import"; -import { deriveSkillFileKind, type ParsedSkill } from "@/skills/parser"; +import { + deriveSkillFileKind, + isSafeSkillResourcePath, + type ParsedSkill, +} from "@/skills/parser"; import { type InsertSkill, SkillFileEncodingSchema, @@ -19,14 +23,10 @@ export const SkillFileInputSchema = z.object({ path: z .string() .min(1) - .refine( - (p) => - !p.startsWith("/") && - p.split("/").every((s) => s !== "" && s !== "." && s !== ".."), - { - message: "path must be relative, with no empty, `.`, or `..` segments", - }, - ) + .refine(isSafeSkillResourcePath, { + message: + "path must be relative, must not traverse (`..`), and must not resolve to a directory (no trailing slash or `.`)", + }) .describe("Resource path, e.g. references/API.md or scripts/run.py"), content: z .string() From 8bf8b51bfdd4e14dea456a32d6574ea2c70c4943 Mon Sep 17 00:00:00 2001 From: Arseny Kravchenko Date: Fri, 17 Jul 2026 16:21:17 +0200 Subject: [PATCH 3/3] docs(skills): note unsafe-path skips in github-import skippedFiles --- platform/backend/src/routes/skill/skill.routes.ts | 2 +- platform/backend/src/skills/github-import.ts | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/platform/backend/src/routes/skill/skill.routes.ts b/platform/backend/src/routes/skill/skill.routes.ts index 09f83f92f54..d908101ab98 100644 --- a/platform/backend/src/routes/skill/skill.routes.ts +++ b/platform/backend/src/routes/skill/skill.routes.ts @@ -931,7 +931,7 @@ const skillRoutes: FastifyPluginAsyncZod = async (fastify) => { }), ) .describe( - "Per created skill, resource paths not imported: oversized, beyond the per-skill file cap, or unfetchable", + "Per created skill, resource paths not imported: oversized, beyond the per-skill file cap, unfetchable, or unsafe to persist", ), }), ), diff --git a/platform/backend/src/skills/github-import.ts b/platform/backend/src/skills/github-import.ts index 171dc0f8ccd..bf73dc84441 100644 --- a/platform/backend/src/skills/github-import.ts +++ b/platform/backend/src/skills/github-import.ts @@ -87,8 +87,9 @@ interface ImportedSkill { }[]; /** * Resource paths (relative to the skill dir) that were not imported: - * oversized files, files beyond the per-skill cap, and files whose fetch - * failed. Surfaced to the caller so drops are never silent. + * oversized files, files beyond the per-skill cap, files whose fetch failed, + * and paths that would not be safe to persist (see `isSafeSkillResourcePath`). + * Surfaced to the caller so drops are never silent. */ skippedFiles: string[]; /** Provenance string, e.g. `owner/repo@main:skills/pdf`. */