From 7adf9765b124bc23f1a0efbc1ab18733091ce790 Mon Sep 17 00:00:00 2001 From: Lucas Winther Date: Fri, 30 Oct 2026 03:26:22 +0100 Subject: [PATCH] fix(import): allowed import roots, hostile zip symlink removal, tests Co-Authored-By: Claude Opus 5.5 --- docs/contract-requests/import.md | 1 + packages/feature-import/src/index.ts | 2 + packages/feature-import/src/jobs/code-repo.ts | 2 + .../feature-import/src/jobs/packt-video.ts | 2 + packages/feature-import/src/jobs/pdf-deck.ts | 2 + .../feature-import/src/jobs/video-folder.ts | 2 + packages/feature-import/src/roots.ts | 30 ++++++++++++++ packages/feature-import/src/routes.ts | 15 ++++--- packages/feature-import/src/util/source.ts | 18 +++++++-- .../feature-import/src/web/ImportPage.tsx | 3 +- packages/feature-import/test/harness.ts | 3 +- .../feature-import/test/hostile-zip.test.ts | 39 +++++++++++++++++++ packages/feature-import/test/roots.test.ts | 26 +++++++++++++ packages/feature-import/test/routes.test.ts | 29 +++++++++++++- .../test/video-folder-pdf.test.ts | 2 +- 15 files changed, 164 insertions(+), 12 deletions(-) create mode 100644 packages/feature-import/src/roots.ts create mode 100644 packages/feature-import/test/hostile-zip.test.ts create mode 100644 packages/feature-import/test/roots.test.ts diff --git a/docs/contract-requests/import.md b/docs/contract-requests/import.md index 22513da..4831f36 100644 --- a/docs/contract-requests/import.md +++ b/docs/contract-requests/import.md @@ -9,3 +9,4 @@ - `/cache/transcripts//.json` (`{ lessonId, text, segments: [{ start, end, text }] }`). Suggest a `cacheDir`/`deckPageUrl` helper in core, and a media route for `cache/decks` so the deck viewer can load pages. 5. **Video lessons have no description field.** Packt clips come with a one-line description. It is dropped, because `transcript` is the only Markdown a video lesson can carry and the transcription job writes it. +6. **`STC_IMPORT_ROOTS`** (":"-separated, default `/import-inbox`, created when `importFeature` is built) is read inside the import package. Core config could own it later. diff --git a/packages/feature-import/src/index.ts b/packages/feature-import/src/index.ts index 1d7874a..367fe43 100644 --- a/packages/feature-import/src/index.ts +++ b/packages/feature-import/src/index.ts @@ -5,6 +5,7 @@ import { importPacktVideo } from "./jobs/packt-video"; import { importPdfDeck } from "./jobs/pdf-deck"; import { transcribeCourse } from "./jobs/transcribe"; import { importVideoFolder } from "./jobs/video-folder"; +import { ensureRoots } from "./roots"; import { importRoutes } from "./routes"; export type { ImportOptions } from "./context"; @@ -21,6 +22,7 @@ export function importFeature(ctx: ServerContext, opts: ImportOptions = {}): Fea return { ctx, env, tools: { ffmpeg: env.STC_FFMPEG || "ffmpeg", ffprobe: env.STC_FFPROBE || "ffprobe" }, python: env.STC_PYTHON || "python3" }; }; const job = (fn: (d: Deps, p: unknown, jc: Parameters[1]) => Promise): JobHandler => (p, jc) => fn(deps(), p, jc); + ensureRoots({ ctx, env: opts.env ?? process.env }); return { name: "import", routes: importRoutes(deps), diff --git a/packages/feature-import/src/jobs/code-repo.ts b/packages/feature-import/src/jobs/code-repo.ts index 7286ea1..29aff79 100644 --- a/packages/feature-import/src/jobs/code-repo.ts +++ b/packages/feature-import/src/jobs/code-repo.ts @@ -5,6 +5,7 @@ import { finish, managedCourse, saveManifest, stageDir, type Deps } from "../con import { attachDeck } from "../deck"; import { putFile } from "../media"; import { run } from "../util/exec"; +import { assertAllowedPath } from "../roots"; import { openSource } from "../util/source"; import { containment } from "../util/text"; import { optStr, parsePayload, withStage } from "./common"; @@ -50,6 +51,7 @@ export async function importCodeRepo(d: Deps, payload: unknown, jc: JobContext): const { path, options } = parsePayload(payload, "code-repo"); const warnings: string[] = []; const course = await managedCourse(d, optStr(options, "courseId")); + if (!GIT_URL.test(path)) await assertAllowedPath(d, path); const stage = await stageDir(d, "repo"); return withStage(stage, async () => { diff --git a/packages/feature-import/src/jobs/packt-video.ts b/packages/feature-import/src/jobs/packt-video.ts index 8c2dccf..0103736 100644 --- a/packages/feature-import/src/jobs/packt-video.ts +++ b/packages/feature-import/src/jobs/packt-video.ts @@ -4,12 +4,14 @@ import type { ImportResult, JobContext } from "@stc/contracts"; import { finish, frameCandidates, freshCourseId, publishCourse, saveManifest, sourceBlock, stageDir, type Deps } from "../context"; import { probeDuration } from "../media"; import { parsePacktIndex } from "../packt"; +import { assertAllowedPath } from "../roots"; import { openSource } from "../util/source"; import { adoptCover, maybeQueueTranscription, optBool, optStr, parsePayload, withStage } from "./common"; export async function importPacktVideo(d: Deps, payload: unknown, jc: JobContext): Promise { const { path, options } = parsePayload(payload, "packt-video"); const warnings: string[] = []; + await assertAllowedPath(d, path); await jc.reportProgress(0.02, "Reading the package"); const src = await openSource(path); diff --git a/packages/feature-import/src/jobs/pdf-deck.ts b/packages/feature-import/src/jobs/pdf-deck.ts index 7cfe3e4..855815a 100644 --- a/packages/feature-import/src/jobs/pdf-deck.ts +++ b/packages/feature-import/src/jobs/pdf-deck.ts @@ -2,12 +2,14 @@ import { stat } from "node:fs/promises"; import { join } from "node:path"; import type { ImportResult, JobContext } from "@stc/contracts"; import { finish, managedCourse, saveManifest, type Deps } from "../context"; +import { assertAllowedPath } from "../roots"; import { attachDeck } from "../deck"; import { optStr, parsePayload } from "./common"; export async function importPdfDeck(d: Deps, payload: unknown, jc: JobContext): Promise { const { path, options } = parsePayload(payload, "pdf-deck"); const warnings: string[] = []; + await assertAllowedPath(d, path); if (!(await stat(path).catch(() => null))?.isFile() || !/\.pdf$/i.test(path)) throw new Error(`not a PDF file: ${path}`); const course = await managedCourse(d, optStr(options, "courseId")); const attachTo = optStr(options, "attachTo"); diff --git a/packages/feature-import/src/jobs/video-folder.ts b/packages/feature-import/src/jobs/video-folder.ts index b4ff4f4..403f8af 100644 --- a/packages/feature-import/src/jobs/video-folder.ts +++ b/packages/feature-import/src/jobs/video-folder.ts @@ -3,6 +3,7 @@ import { basename, extname, join } from "node:path"; import type { ImportResult, JobContext } from "@stc/contracts"; import { finish, frameCandidates, freshCourseId, publishCourse, saveManifest, sourceBlock, stageDir, type Deps } from "../context"; import { probeDuration, putFile } from "../media"; +import { assertAllowedPath } from "../roots"; import { naturalSort } from "../util/text"; import { openSource } from "../util/source"; import { adoptCover, maybeQueueTranscription, optBool, optStr, parsePayload, withStage } from "./common"; @@ -19,6 +20,7 @@ export function lessonTitle(file: string): string { export async function importVideoFolder(d: Deps, payload: unknown, jc: JobContext): Promise { const { path, options } = parsePayload(payload, "video-folder"); const warnings: string[] = []; + await assertAllowedPath(d, path); if (!(await stat(path).catch(() => null))?.isDirectory()) throw new Error(`not a folder: ${path}`); const src = await openSource(path); const files = naturalSort(src.entries.filter((e) => VIDEO.test(e) && !e.split("/").some((p) => p.startsWith(".")))); diff --git a/packages/feature-import/src/roots.ts b/packages/feature-import/src/roots.ts new file mode 100644 index 0000000..621c96f --- /dev/null +++ b/packages/feature-import/src/roots.ts @@ -0,0 +1,30 @@ +import { mkdirSync } from "node:fs"; +import { realpath } from "node:fs/promises"; +import { resolve, sep } from "node:path"; +import type { Deps } from "./context"; + +export const OUTSIDE = "path is outside the allowed import folders"; + +/** STC_IMPORT_ROOTS (":"-separated), default /import-inbox. */ +export function importRoots(d: Pick): string[] { + const raw = d.env.STC_IMPORT_ROOTS; + const list = raw ? raw.split(":").filter(Boolean) : [resolve(d.ctx.config.dataDir, "import-inbox")]; + return list.map((r) => resolve(r)); +} + +export function ensureRoots(d: Pick): void { + for (const r of importRoots(d)) { try { mkdirSync(r, { recursive: true }); } catch { /* reported when used */ } } +} + +/** Real path (symlinks followed) must lie inside the real path of one root. Throws OUTSIDE otherwise. */ +export async function assertAllowedPath(d: Pick, path: string): Promise { + let real: string; + try { real = await realpath(resolve(path)); } catch { throw new Error(OUTSIDE); } + for (const root of importRoots(d)) { + const rr = await realpath(root).catch(() => null); + if (rr && (real === rr || real.startsWith(rr + sep))) return real; + } + throw new Error(OUTSIDE); +} + +export const isGitUrl = (p: string) => /^(https?:\/\/|ssh:\/\/|git@)\S+$/.test(p); diff --git a/packages/feature-import/src/routes.ts b/packages/feature-import/src/routes.ts index f1cd2b0..c44570f 100644 --- a/packages/feature-import/src/routes.ts +++ b/packages/feature-import/src/routes.ts @@ -1,9 +1,9 @@ import { copyFile, mkdir, readdir } from "node:fs/promises"; -import { existsSync } from "node:fs"; import { join, relative, resolve } from "node:path"; import type { Feature, RouteHandler } from "@stc/contracts"; import { serveMedia } from "@stc/core"; -import { coverCacheDir, coursesDir, exists, managedCourse, saveManifest, type Deps } from "./context"; +import { assertAllowedPath, importRoots, OUTSIDE } from "./roots"; +import { coverCacheDir, coursesDir, exists, isPlainId, managedCourse, saveManifest, type Deps } from "./context"; export const KINDS = ["packt-video", "code-repo", "pdf-deck", "video-folder"] as const; const OPTION_KEYS = ["title", "courseId", "attachTo", "media", "transcribe"] as const; @@ -26,7 +26,7 @@ export function importRoutes(deps: () => Deps): Feature["routes"] { const d = deps(); const dir = coursesDir(d); const served = d.ctx.config.courseDirs.some((c) => resolve(c) === dir); - return json({ transcribe: Boolean(d.env.STC_TRANSCRIBE_URL), coursesDir: dir, coursesDirServed: served }); + return json({ transcribe: Boolean(d.env.STC_TRANSCRIBE_URL), coursesDir: dir, coursesDirServed: served, importRoots: importRoots(d) }); }), }, "/api/import/jobs": { @@ -50,8 +50,10 @@ export function importRoutes(deps: () => Deps): Feature["routes"] { const path = typeof body?.path === "string" ? body.path.trim() : ""; if (!path) return json({ error: "path is required" }, 400); const isGit = kind === "code-repo" && GIT_URL.test(path); - if (!isGit && !existsSync(path)) return json({ error: `Path not found on the server: ${path}` }, 400); - if (!isGit && !path.startsWith("/")) return json({ error: "Use an absolute path" }, 400); + if (!isGit) { + if (!path.startsWith("/")) return json({ error: "Use an absolute path" }, 400); + try { await assertAllowedPath(d, path); } catch { return json({ error: OUTSIDE }, 400); } + } if ((kind === "code-repo" || kind === "pdf-deck") && typeof options.courseId !== "string") return json({ error: "courseId is required" }, 400); if (kind === "pdf-deck" && typeof options.attachTo !== "string") return json({ error: "attachTo is required" }, 400); return json({ id: await d.ctx.jobs.enqueue(`import.${kind}`, { kind, path, options }) }, 202); @@ -74,6 +76,7 @@ export function importRoutes(deps: () => Deps): Feature["routes"] { GET: owner(async (_req, rc) => { const d = deps(); const id = rc.params.courseId!; + if (!isPlainId(id)) return json({ error: "Invalid course id" }, 400); const c = d.ctx.courses.get(id); const names = (await readdir(coverCacheDir(d, id)).catch(() => [] as string[])).filter((n) => COVER_NAME.test(n)).sort(); return json({ current: c?.manifest?.cover ?? null, candidates: names }); @@ -84,6 +87,7 @@ export function importRoutes(deps: () => Deps): Feature["routes"] { try { body = await req.json(); } catch { return json({ error: "Body must be JSON" }, 400); } const name = String(body?.name ?? ""); if (!COVER_NAME.test(name)) return json({ error: "Invalid candidate name" }, 400); + if (!isPlainId(rc.params.courseId!)) return json({ error: "Invalid course id" }, 400); const src = join(coverCacheDir(d, rc.params.courseId!), name); if (!(await exists(src))) return json({ error: "No such candidate" }, 404); let course; @@ -100,6 +104,7 @@ export function importRoutes(deps: () => Deps): Feature["routes"] { GET: owner(async (req, rc) => { const name = rc.params.name!; if (!COVER_NAME.test(name)) return new Response("Not found", { status: 404 }); + if (!isPlainId(rc.params.courseId!)) return new Response("Not found", { status: 404 }); const file = join(coverCacheDir(deps(), rc.params.courseId!), name); if (!(await exists(file))) return new Response("Not found", { status: 404 }); return serveMedia(req, file); diff --git a/packages/feature-import/src/util/source.ts b/packages/feature-import/src/util/source.ts index 43bf664..40cb297 100644 --- a/packages/feature-import/src/util/source.ts +++ b/packages/feature-import/src/util/source.ts @@ -1,4 +1,4 @@ -import { cp, mkdir, readdir, readFile, stat } from "node:fs/promises"; +import { cp, mkdir, readdir, readFile, rm, stat } from "node:fs/promises"; import { dirname, join } from "node:path"; import { run } from "./exec"; @@ -22,6 +22,14 @@ async function walk(root: string, rel = "", out: string[] = []): Promise { + for (const e of await readdir(dir, { withFileTypes: true })) { + const p = join(dir, e.name); + if (e.isSymbolicLink()) await rm(p, { force: true }); + else if (e.isDirectory()) await removeSymlinks(p); + } +} + export async function openSource(path: string): Promise { const st = await stat(path).catch(() => null); if (!st) throw new Error(`source not found: ${path}`); @@ -47,8 +55,12 @@ export async function openSource(path: string): Promise { async extract(list, dest, signal) { await mkdir(dest, { recursive: true }); // unzip treats arguments as patterns, so glob characters in names are escaped; batches keep argv small. - for (let i = 0; i < list.length; i += 200) { - await run(["unzip", "-q", "-o", path, ...list.slice(i, i + 200).map(globEscape), "-d", dest], { signal }); + try { + for (let i = 0; i < list.length; i += 200) { + await run(["unzip", "-q", "-o", path, ...list.slice(i, i + 200).map(globEscape), "-d", dest], { signal }); + } + } finally { + await removeSymlinks(dest); // a zip can carry symlinks; none may survive extraction } }, }; diff --git a/packages/feature-import/src/web/ImportPage.tsx b/packages/feature-import/src/web/ImportPage.tsx index 44ebbdd..dd6a51d 100644 --- a/packages/feature-import/src/web/ImportPage.tsx +++ b/packages/feature-import/src/web/ImportPage.tsx @@ -5,7 +5,7 @@ import "./import.css"; type Kind = "packt-video" | "code-repo" | "pdf-deck" | "video-folder"; interface Job { id: string; kind: string; status: "queued" | "running" | "done" | "failed"; progress: number; message: string | null; payload: any; result: any; error: string | null; createdAt: string } interface CourseInfo { id: string; title: string; modules: { id: string; title: string; lessons: { id: string; title: string }[] }[] } -interface Info { transcribe: boolean; coursesDir: string; coursesDirServed: boolean } +interface Info { importRoots: string[]; transcribe: boolean; coursesDir: string; coursesDirServed: boolean } const KINDS: { value: Kind; label: string; hint: string; placeholder: string }[] = [ { value: "packt-video", label: "Packt video zip", hint: "A video package whose index.html lists the sections and clips.", placeholder: "/data/downloads/video_zip.zip" }, @@ -139,6 +139,7 @@ export default function ImportPage() { {!needsCourse ? (