From ead7327bb11d6339766ba0ccbe88bb6ce0c6fecc Mon Sep 17 00:00:00 2001 From: Lucas Winther Date: Fri, 30 Oct 2026 04:48:56 +0100 Subject: [PATCH] fix(ai): solution and hidden-file guard runs on the canonical real path read_file and read_lesson URL-decode, posix-normalise, reject .. and absolute paths, resolve through CourseRepository.resolvePath (symlinks followed), then guard the real course-relative path and compare real paths with every manifest browser.solution. Hidden files are refused too. Co-Authored-By: Claude Opus 5.5 --- packages/feature-ai/src/assistant/tools.ts | 20 +++--- .../feature-ai/src/retrieval/course-index.ts | 66 ++++++++++++++++-- packages/feature-ai/test/assistant.test.ts | 67 +++++++++++++++++++ 3 files changed, 139 insertions(+), 14 deletions(-) diff --git a/packages/feature-ai/src/assistant/tools.ts b/packages/feature-ai/src/assistant/tools.ts index 8ab1118..8cf603f 100644 --- a/packages/feature-ai/src/assistant/tools.ts +++ b/packages/feature-ai/src/assistant/tools.ts @@ -3,7 +3,7 @@ import type { ToolSpec } from "@stc/contracts"; import type { CourseRepository, LoadedCourse } from "@stc/course-format"; import type { McpServerConfig } from "../settings"; import { connectMcp, mcpToolName, type McpClient } from "../mcp/client"; -import { isSolutionPath, noteChunks, type Chunk, type CourseIndex } from "../retrieval/course-index"; +import { canonicalCoursePath, isGuardedFile, noteChunks, type Chunk, type CourseIndex } from "../retrieval/course-index"; export interface ToolSet { specs: ToolSpec[]; @@ -86,22 +86,24 @@ export function courseTools(o: { const id = str(a.lessonId); const ref = lessonChunk(id); if (!ref) return `No lesson with id "${id}". Lessons:\n${lessonList()}`; - if (isSolutionPath(course, ref.path)) return "That file is for authors only."; + const canon = ref.path ? canonicalCoursePath(o.repo, course, ref.path) : null; + if (canon && isGuardedFile(o.repo, course, canon)) return "That file is for authors only."; const md = await o.repo.readLessonMarkdown(course.id, id); if (md === null) return `Lesson "${id}" has no readable text (it may be a video without a transcript).`; sources.push(ref); return md.slice(0, MAX_READ); } case "read_file": { - const rel = str(a.path).replace(/^\/+/, ""); - if (isSolutionPath(course, rel)) return "That file is for authors only and is not available."; - const abs = o.repo.resolvePath(course.id, rel); - const info = abs ? await stat(abs).catch(() => null) : null; - if (!abs || !info?.isFile()) return `No such file in this course: ${rel}`; + // Canonicalise first (decode, normalise, follow symlinks), then guard the real path. + const canon = canonicalCoursePath(o.repo, course, str(a.path)); + if (!canon) return `No such file in this course: ${str(a.path)}`; + if (isGuardedFile(o.repo, course, canon)) return "That file is for authors only and is not available."; + const info = await stat(canon.abs).catch(() => null); + if (!info?.isFile()) return `No such file in this course: ${canon.rel}`; if (info.size > 2_000_000) return "That file is too large to read here."; - const text = await Bun.file(abs).text(); + const text = await Bun.file(canon.abs).text(); if (text.includes("\u0000")) return "That is a binary file."; - sources.push({ path: rel, kind: /\.(md|markdown)$/i.test(rel) ? "markdown" : "code", text: "" }); + sources.push({ path: canon.rel, kind: /\.(md|markdown)$/i.test(canon.rel) ? "markdown" : "code", text: "" }); return text.slice(0, MAX_READ); } case "list_notes": { diff --git a/packages/feature-ai/src/retrieval/course-index.ts b/packages/feature-ai/src/retrieval/course-index.ts index 0dadd53..61a71e0 100644 --- a/packages/feature-ai/src/retrieval/course-index.ts +++ b/packages/feature-ai/src/retrieval/course-index.ts @@ -1,6 +1,7 @@ +import { realpathSync } from "node:fs"; import { readdir, stat } from "node:fs/promises"; -import { basename, extname, join, relative, sep } from "node:path"; -import { slugify, type LoadedCourse } from "@stc/course-format"; +import { basename, extname, join, posix as posixPath, relative, sep } from "node:path"; +import { slugify, type CourseRepository, type LoadedCourse } from "@stc/course-format"; import { Bm25 } from "./bm25"; export type ChunkKind = "lesson" | "transcript" | "markdown" | "code" | "deck" | "note"; @@ -32,13 +33,68 @@ const posix = (p: string) => p.split(sep).join("/"); /** * Files only authors may see: manifest `browser.solution` files and anything under a `solution`, `solutions` * or `reference` folder (Pure Snake `js/solutions/`, Systemic Design `reference/` and `expansion/solution/`). - * The assistant never retrieves or reads them. + * The assistant never retrieves or reads them. `rel` is normalised first; for paths that come from a user or a + * model, use `canonicalCoursePath` + `isGuardedFile`, which also follow symlinks. */ export function isSolutionPath(course: LoadedCourse, rel: string): boolean { - const p = posix(rel).replace(/^\.\//, ""); + const p = normalizeRel(rel); + if (p === null) return true; // not a course-relative path at all: refuse if (p.split("/").slice(0, -1).some((s) => SOLUTION_SEGMENTS.has(s.toLowerCase()))) return true; if (/(^|\/)solution\.[a-z]+$/i.test(p)) return true; - for (const m of course.manifest?.modules ?? []) for (const l of m.lessons) if (l.browser?.solution && posix(l.browser.solution) === p) return true; + for (const m of course.manifest?.modules ?? []) for (const l of m.lessons) if (l.browser?.solution && normalizeRel(l.browser.solution) === p) return true; + return false; +} + +/** Posix-normalised course-relative path, or null when it is absolute or climbs out with `..`. */ +export function normalizeRel(raw: string): string | null { + if (raw.includes("\u0000")) return null; + const p = posixPath.normalize(raw.replace(/\\/g, "/")).replace(/^(\.\/)+/, ""); + if (!p || p === "." || p.startsWith("/") || /^[a-zA-Z]:/.test(p)) return null; + if (p.split("/").includes("..")) return null; + return p.replace(/\/+$/, ""); +} + +/** Undoes URL encoding (up to three layers). A malformed escape leaves the string as it is. */ +function decodeLayers(raw: string): string { + let s = raw; + for (let i = 0; i < 3 && /%[0-9a-fA-F]{2}/.test(s); i++) { + try { s = decodeURIComponent(s); } catch { break; } + } + return s; +} + +/** + * Canonical form of a path a user or the model asked for: URL-decoded, backslashes as slashes, posix-normalised, + * leading `./` and `/` stripped, `..` rejected, then resolved through `CourseRepository.resolvePath` (real path, + * symlinks followed, must stay inside the course root). `rel` is the real path relative to the real root. + * Null when the path is invalid, escapes the course or does not exist. + */ +export function canonicalCoursePath(repo: CourseRepository, course: LoadedCourse, raw: string): { rel: string; abs: string } | null { + const norm = normalizeRel(decodeLayers(raw.trim()).replace(/\\/g, "/").replace(/^\/+/, "")); + if (!norm) return null; + const abs = repo.resolvePath(course.id, norm); + if (!abs) return null; + let realRoot: string; + try { realRoot = realpathSync(course.root); } catch { return null; } + const rel = relative(realRoot, abs).split(sep).join("/"); + if (!rel || rel.startsWith("..")) return null; + return { rel, abs }; +} + +/** + * The guard for tool reads, on canonical paths only: solution files (by the folder and name rules on the real + * relative path, and by comparing real paths with every manifest `browser.solution`) and hidden files or folders. + */ +export function isGuardedFile(repo: CourseRepository, course: LoadedCourse, canonical: { rel: string; abs: string }): boolean { + if (canonical.rel.split("/").some((s) => s.startsWith("."))) return true; + if (isSolutionPath(course, canonical.rel)) return true; + for (const m of course.manifest?.modules ?? []) { + for (const l of m.lessons) { + if (!l.browser?.solution) continue; + const sol = repo.resolvePath(course.id, l.browser.solution); + if (sol && sol === canonical.abs) return true; + } + } return false; } diff --git a/packages/feature-ai/test/assistant.test.ts b/packages/feature-ai/test/assistant.test.ts index 8b533d4..c4e6307 100644 --- a/packages/feature-ai/test/assistant.test.ts +++ b/packages/feature-ai/test/assistant.test.ts @@ -102,3 +102,70 @@ test("my_progress reports the learner's progress per lesson", async () => { expect(out).toContain("Adding numbers (m2-text): not started"); expect(out).toMatch(/1 of \d+ lessons done/); }); + +// The solution guard must hold for every spelling of a path: it checks the canonical real path. +test("read_file refuses solutions however the path is written, including through symlinks", async () => { + const { cpSync, mkdtempSync, mkdirSync, readFileSync, rmSync, symlinkSync, writeFileSync } = await import("node:fs"); + const { tmpdir } = await import("node:os"); + const dir = mkdtempSync(join(tmpdir(), "stc-ai-guard-")); + try { + const root = join(dir, "courses/sample"); + cpSync(join(FIXTURES, "sample"), root, { recursive: true }); + const yaml = join(root, "stcourse.yaml"); + writeFileSync(yaml, readFileSync(yaml, "utf8").replace("solution: exercises/add/solution.js", "solution: exercises/add/answer.js")); + writeFileSync(join(root, "exercises/add/answer.js"), "export const add = (a, b) => a + b; // ANSWER\n"); + mkdirSync(join(root, "solutions")); + writeFileSync(join(root, "solutions/ref.js"), "// REFERENCE SOLUTION\n"); + writeFileSync(join(root, ".env"), "TOKEN=hidden\n"); + symlinkSync("exercises/add/answer.js", join(root, "peek.js")); + symlinkSync("solutions", join(root, "material")); + symlinkSync("../solutions/ref.js", join(root, "lessons/extra.js")); + const r = await createCourseRepository([join(dir, "courses")]); + const c = r.get("sample")!; + expect(c.status).toBe("ok"); + const t = courseTools({ repo: r, course: c, index: await buildCourseIndex(c, "/nonexistent"), listNotes: async () => [] }); + const read = (path: string) => t.run("read_file", { path }); + + const refused = [ + "exercises/add/answer.js", + "exercises/add/../add/answer.js", + "./exercises/add/answer.js", + "exercises//add/answer.js", + "./exercises/./add/answer.js", + "/exercises/add/answer.js", + "exercises\\add\\answer.js", + "exercises%2Fadd%2Fanswer.js", + "exercises/add/%2E%2E/add/answer.js", + "exercises%252Fadd%252Fanswer.js", + "%2E/exercises/add/answer.js", + "peek.js", + "material/ref.js", + "lessons/extra.js", + "lessons/../solutions/ref.js", + "exercises/add/solution.js", + ".env", + "lessons/../.env", + ]; + for (const p of refused) { + const out = await read(p); + expect({ p, out }).toEqual({ p, out: expect.stringMatching(/authors only|not available|No such file/) }); + expect(out).not.toContain("ANSWER"); + expect(out).not.toContain("REFERENCE"); + expect(out).not.toContain("hidden"); + } + expect(await read("exercises/add/answer.js")).toMatch(/authors only/); + expect(await read("peek.js")).toMatch(/authors only/); + expect(await read("../../etc/passwd")).toMatch(/No such file/); + // Ordinary files still read, in any spelling. + expect(await read("exercises/add/starter.js")).toContain("add"); + expect(await read("./exercises/./add/../add/starter.js")).toContain("add"); + expect(await read("exercises%2Fadd%2Fstarter.js")).toContain("add"); + expect(t.sources.at(-1)?.path).toBe("exercises/add/starter.js"); + // The index never holds them either. + const idx = await buildCourseIndex(c, "/nonexistent"); + const hits = idx.search("ANSWER REFERENCE SOLUTION add", 50); + expect(hits.filter((x) => /answer|ref\.js|peek|material|extra/.test(x.path) || x.text.includes("ANSWER") || x.text.includes("REFERENCE"))).toEqual([]); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +});