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 <[email protected]>
This commit is contained in:
Lucas Winther
co-authored by
Claude Opus 5.5
Claude Opus 5.5
parent
cfc41c6a63
commit
ead7327bb1
@@ -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": {
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
|
||||
@@ -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 });
|
||||
}
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user