diff --git a/platform/archestra-rs/sandbox-core/src/validation.rs b/platform/archestra-rs/sandbox-core/src/validation.rs index a1c13742a0a..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| segment == "..") { + if path.contains('\0') + || path.split('/').any(|segment| segment == "..") + || resolves_to_directory(path) + { return Err(SandboxError::InvalidInput(format!( "invalid artifact path: {path:?}" ))); @@ -58,7 +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<()> { - if path.contains('\0') || path.split('/').any(|segment| 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:?}" ))); @@ -153,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::*; @@ -198,6 +216,11 @@ mod tests { ("/home/sandbox/../etc/passwd", false, false), // directory, not a file ("/home/sandbox/", false, false), + // terminal `.` resolves to a directory: rejected before it can be + // persisted as a replay event that fails `base64 -d > ` forever + ("/home/sandbox/.", 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), @@ -220,6 +243,10 @@ mod tests { ("/etc/passwd", false, false), // traversal ("a/../b.txt", false, false), + // 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 1b0d518e017..d485c301401 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,36 @@ describe("POST /api/skills", () => { expect(response.statusCode).toBe(400); }); + test("rejects a resource path that would break the sandbox mount with a 400", async () => { + // 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", + payload: { + content: MANIFEST, + files: [{ path, content: "print(1)" }], + }, + }); + expect(response.statusCode, `path: ${JSON.stringify(path)}`).toBe(400); + } + }); + + 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/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-sandbox/skill-sandbox-runtime-service.test.ts b/platform/backend/src/skills-sandbox/skill-sandbox-runtime-service.test.ts index 56576d95cf9..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,6 +779,11 @@ describe("path validation vectors (mirrored with sandbox-core)", () => { ["/home/sandbox/../etc/passwd", false, false], // directory, not a file ["/home/sandbox/", false, false], + // terminal `.` resolves to a directory: rejected before it can be persisted + // as a replay event that fails `base64 -d > ` forever + ["/home/sandbox/.", 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], @@ -782,6 +805,10 @@ describe("path validation vectors (mirrored with sandbox-core)", () => { ["/etc/passwd", false, false], // traversal ["a/../b.txt", false, false], + // 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 3095905ef6f..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, ); @@ -885,6 +886,16 @@ function resolveArtifactPath(params: { `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( @@ -956,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 @@ -1158,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..bf73dc84441 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, @@ -86,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`. */ @@ -312,7 +314,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.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..c2f58b6ab35 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`"); } @@ -117,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 138194e655c..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,13 +23,10 @@ export const SkillFileInputSchema = z.object({ path: z .string() .min(1) - .refine( - (p) => !p.startsWith("/") && !p.split("/").some((s) => s === ".."), - { - message: - "path must be relative and must not contain directory traversal sequences", - }, - ) + .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()