fix(security): close path traversal and shell injection paths

.changeset/olive-pots-repeat.md

@@ -0,0 +1,19 @@
+---
+'@srcbook/api': patch
+---
+
+Close path traversal and shell injection paths.
+
+`DELETE /api/srcbooks/:id` could delete directories outside the srcbooks directory, and
+`POST /api/file` could read any file on disk. Both are now confined to `SRCBOOKS_DIR`.
+
+Cell filenames are validated when a `.src.md` is decoded rather than only on rename, so an
+imported notebook can no longer introduce a filename that reaches a shell. Prettier and
+depcheck are invoked with argument arrays instead of interpolated command strings.
+
+`POST /api/settings` now validates its body against an allowlist. It previously wrote any
+column of the config row, including `aiBaseUrl`, which controls where AI requests and the
+user's API key are sent.
+
+Also fixes formatting reporting failure when prettier writes a warning to stderr but exits
+successfully.

packages/api/deps.mts

@@ -1,5 +1,5 @@
 import Path from 'node:path';
-import { exec } from 'node:child_process';
+import { execFile } from 'node:child_process';
 import { readFile } from './fs-utils.mjs';
 import { DIST_DIR } from './constants.mjs';
 
@@ -50,25 +50,32 @@ export async function shouldNpmInstall(dirPath: string): Promise<boolean> {
 }
 
 export async function missingUndeclaredDeps(dirPath: string): Promise<string[]> {
-  return new Promise((resolve) => {
-    // Ignore the err argument because depcheck exists with a non zero code (255) when there are missing deps.
-    exec(
-      `npm run depcheck ${Path.resolve(dirPath)} -- --json`,
+  return new Promise((resolve, reject) => {
+    // execFile rather than exec: dirPath must not reach a shell.
+    // The err argument is ignored because depcheck exits non-zero (255) precisely
+    // when it finds missing deps, which is the case we care about.
+    execFile(
+      'npm',
+      ['run', 'depcheck', Path.resolve(dirPath), '--', '--json'],
       { cwd: DIST_DIR },
       (_err, stdout) => {
-        const output = stdout || '';
+        // Everything below runs in a callback, so a throw here would escape the
+        // promise entirely and take the process down rather than rejecting. This
+        // is on the cell-execution path, so that was a crash on every run whose
+        // depcheck output didn't parse.
+        try {
+          const jsonMatch = (stdout || '').match(/{.*}/s);
 
-        // Use regex to extract JSON object
-        const jsonMatch = output.match(/{.*}/s);
-        if (!jsonMatch) {
-          throw new Error('Failed to extract JSON from depcheck output.');
-        }
-
-        // Parse the JSON
-        const parsedResult = JSON.parse(jsonMatch[0]);
+          if (!jsonMatch) {
+            reject(new Error('Failed to extract JSON from depcheck output.'));
+            return;
+          }
 
-        // Process and return the data as needed
-        resolve(Object.keys(parsedResult.missing));
+          const parsedResult = JSON.parse(jsonMatch[0]);
+          resolve(Object.keys(parsedResult.missing ?? {}));
+        } catch (e) {
+          reject(e instanceof Error ? e : new Error(String(e)));
+        }
       },
     );
   });

packages/api/path-utils.mts

@@ -0,0 +1,42 @@
+import Path from 'node:path';
+
+/**
+ * Resolve `segments` against `root` and assert the result stays inside it.
+ *
+ * User-supplied path segments reach the filesystem from several directions —
+ * srcbook ids in URLs, cell filenames decoded from a `.src.md`, file paths that
+ * tsserver reports back to the client — and any of them can contain `..`. Express
+ * URL-decodes route params, so `..%2F..%2F` arrives as a real traversal.
+ *
+ * Returns the resolved path, or null when it would escape.
+ */
+export function containedPath(root: string, ...segments: string[]): string | null {
+  const resolvedRoot = Path.resolve(root);
+  const resolved = Path.resolve(resolvedRoot, ...segments);
+
+  if (resolved === resolvedRoot) {
+    return resolved;
+  }
+
+  // The separator matters: without it, `/foo/barbaz` looks like it is inside `/foo/bar`.
+  if (!resolved.startsWith(resolvedRoot + Path.sep)) {
+    return null;
+  }
+
+  return resolved;
+}
+
+/**
+ * Like `containedPath`, but throws instead of returning null.
+ *
+ * Use at trust boundaries where there is no sensible way to continue.
+ */
+export function requireContainedPath(root: string, ...segments: string[]): string {
+  const path = containedPath(root, ...segments);
+
+  if (path === null) {
+    throw new Error(`Refusing to operate on a path outside of ${root}`);
+  }
+
+  return path;
+}

packages/api/schemas/config.mts

@@ -0,0 +1,31 @@
+import z from 'zod';
+import { AiProvider } from '@srcbook/shared';
+
+/**
+ * Fields a client is allowed to write via POST /api/settings.
+ *
+ * Deliberately an allowlist rather than a partial of the drizzle row type. The
+ * body used to be handed straight to `db.update(configs).set(...)`, which made
+ * every column writable — including `installId`, and `aiBaseUrl`, which decides
+ * where AI requests (and the user's API key) get sent.
+ */
+export const ConfigUpdateSchema = z
+  .object({
+    baseDir: z.string().min(1),
+    defaultLanguage: z.enum(['javascript', 'typescript']),
+    openaiKey: z.string().nullable(),
+    anthropicKey: z.string().nullable(),
+    xaiKey: z.string().nullable(),
+    geminiKey: z.string().nullable(),
+    openrouterKey: z.string().nullable(),
+    customApiKey: z.string().nullable(),
+    aiProvider: z.enum(Object.values(AiProvider) as [string, ...string[]]),
+    aiModel: z.string().nullable(),
+    aiBaseUrl: z.string().url().nullable(),
+    subscriptionEmail: z.string().nullable(),
+    enabledAnalytics: z.boolean(),
+  })
+  .partial()
+  .strict();
+
+export type ConfigUpdateType = z.infer<typeof ConfigUpdateSchema>;

packages/api/server/http.mts

@@ -33,9 +33,11 @@ import {
 } from '../srcbook/index.mjs';
 import { readdir } from '../fs-utils.mjs';
 import { EXAMPLE_SRCBOOKS } from '../srcbook/examples.mjs';
-import { pathToSrcbook } from '../srcbook/path.mjs';
+import { isSrcbookCellPath } from '../srcbook/path.mjs';
 import { isSrcmdPath } from '../srcmd/paths.mjs';
 import { corsOptions, verifyOrigin } from './security.mjs';
+import { containedPath } from '../path-utils.mjs';
+import { ConfigUpdateSchema } from '../schemas/config.mjs';
 
 const app: Application = express();
 
@@ -47,21 +49,40 @@ router.use(cors(corsOptions));
 router.use(verifyOrigin);
 router.use(express.json());
 
+// Read a file so the client can show it — used by go-to-definition, which follows
+// tsserver into a srcbook's own sources and into its node_modules type declarations.
+//
+// Reads are confined to SRCBOOKS_DIR. This endpoint used to read any path on disk.
 router.post('/file', async (req, res) => {
   const { file } = req.body as {
     file: string;
   };
 
+  if (typeof file !== 'string' || file === '') {
+    return res.status(400).json({ error: true, result: 'file must be a non-empty string' });
+  }
+
+  const path = containedPath(SRCBOOKS_DIR, file);
+
+  if (path === null) {
+    console.warn(`Refused to read '${file}': outside of ${SRCBOOKS_DIR}`);
+    return res
+      .status(403)
+      .json({ error: true, result: 'Only files inside the srcbooks directory can be read' });
+  }
+
   try {
-    const content = await fs.readFile(file, 'utf8');
-    const cell = file.includes('.srcbook/srcbooks') && !file.includes('node_modules');
-    const filename = cell ? file.split('/').pop() || file : file;
+    const content = await fs.readFile(path, 'utf8');
+
+    // A path under a srcbook's src/ is one of its own cells, so the client scrolls to
+    // that cell rather than opening a read-only view of the file.
+    const cell = isSrcbookCellPath(path);
 
     return res.json({
       error: false,
       result: {
         content: cell ? '' : content,
-        filename,
+        filename: Path.basename(path),
         type: cell ? 'cell' : 'filepath',
       },
     });
@@ -105,8 +126,24 @@ router.post('/srcbooks', async (req, res) => {
 
 router.delete('/srcbooks/:id', async (req, res) => {
   const { id } = req.params;
-  const srcbookDir = pathToSrcbook(id);
-  removeSrcbook(srcbookDir);
+
+  // This ends in fs.rm(recursive: true), and express URL-decodes params, so an id of
+  // `..%2F..%2FDocuments` would otherwise delete a directory outside SRCBOOKS_DIR.
+  const srcbookDir = containedPath(SRCBOOKS_DIR, id);
+
+  if (srcbookDir === null || srcbookDir === Path.resolve(SRCBOOKS_DIR)) {
+    console.warn(`Refused to delete srcbook '${id}': not a path inside ${SRCBOOKS_DIR}`);
+    return res.status(400).json({ error: true, result: 'Invalid srcbook id' });
+  }
+
+  try {
+    await removeSrcbook(srcbookDir);
+  } catch (e) {
+    const error = e as unknown as Error;
+    console.error(error);
+    return res.json({ error: true, result: error.stack });
+  }
+
   posthog.capture({ event: 'user deleted srcbook' });
   await deleteSessionByDirname(srcbookDir);
   return res.json({ error: false, deleted: true });
@@ -273,12 +310,24 @@ router.get('/settings', async (_req, res) => {
 });
 
 router.post('/settings', async (req, res) => {
+  // The body used to be passed straight to `db.update(configs).set(...)`, so any
+  // column was writable by any caller — including aiBaseUrl, which redirects
+  // subsequent AI calls, and the user's key along with them.
+  const parsed = ConfigUpdateSchema.safeParse(req.body);
+
+  if (!parsed.success) {
+    return res.status(400).json({
+      error: true,
+      result: parsed.error.issues.map((issue) => `${issue.path.join('.')}: ${issue.message}`),
+    });
+  }
+
   try {
-    const updated = await updateConfig(req.body);
+    const updated = await updateConfig(parsed.data);
 
     posthog.capture({
       event: 'user updated settings',
-      properties: { setting_changed: Object.keys(req.body) },
+      properties: { setting_changed: Object.keys(parsed.data) },
     });
 
     return res.json({ result: updated });

packages/api/session.mts

@@ -29,8 +29,9 @@ import {
 import { fileExists } from './fs-utils.mjs';
 import { validFilename } from '@srcbook/shared';
 import { pathToCodeFile } from './srcbook/path.mjs';
-import { exec } from 'node:child_process';
+import { execFile } from 'node:child_process';
 import { npmInstall } from './exec.mjs';
+import { requireContainedPath } from './path-utils.mjs';
 
 const sessions: Record<string, SessionType> = {};
 
@@ -326,16 +327,28 @@ export async function formatCode(dir: string, fileName: string) {
   try {
     await ensurePrettierInstalled(dir);
 
-    const codeFilePath = pathToCodeFile(dir, fileName);
-    const command = `npx prettier ${codeFilePath}`;
+    // Contained because a filename can arrive from a decoded .src.md, which is
+    // attacker-controlled for an imported notebook.
+    const codeFilePath = requireContainedPath(dir, 'src', fileName);
+
+    return new Promise<string>((resolve, reject) => {
+      // execFile, not exec: exec runs through a shell, so a filename like
+      // `$(id).mjs` would be interpreted rather than passed as an argument.
+      // Also runs in the srcbook's directory so npx resolves that copy of
+      // prettier rather than whatever is near the server's cwd.
+      execFile('npx', ['prettier', codeFilePath], { cwd: dir }, (error, stdout, stderr) => {
+        if (error) {
+          console.error(`Error formatting ${fileName}: ${stderr || error.message}`);
+          reject(new Error(stderr || error.message));
+          return;
+        }
 
-    return new Promise((resolve, reject) => {
-      exec(command, async (_, stdout, stderr) => {
+        // Prettier warns on stderr while still succeeding, so only a non-zero
+        // exit is a failure. Previously any stderr output was treated as one.
         if (stderr) {
-          console.error(`exec error: ${stderr}`);
-          reject(stderr);
-          return;
+          console.warn(`prettier: ${stderr}`);
         }
+
         resolve(stdout);
       });
     });

packages/api/srcbook/index.mts

@@ -222,7 +222,9 @@ function buildPackageJson(language: CodeLanguageType) {
 }
 
 export function removeSrcbook(srcbookDir: string) {
-  fs.rm(srcbookDir, { recursive: true });
+  // Returned rather than fire-and-forget: a rejection here used to surface as an
+  // unhandled rejection instead of an error the caller could report.
+  return fs.rm(srcbookDir, { recursive: true, force: true });
 }
 
 export function removeCodeCellFromDisk(srcbookDir: string, filename: string) {

packages/api/srcbook/path.mts

@@ -24,3 +24,16 @@ export function pathToCodeFile(baseDir: string, filename: string) {
 export function filenameFromPath(filePath: string) {
   return Path.basename(filePath);
 }
+
+/**
+ * True when `filePath` is one of a srcbook's own code cells, i.e. it looks like
+ * `<SRCBOOKS_DIR>/<sessionId>/src/<filename>`.
+ *
+ * Distinguishes a cell the client can scroll to from a file it has to open in a
+ * read-only view, such as a type declaration under node_modules.
+ */
+export function isSrcbookCellPath(filePath: string) {
+  const relative = Path.relative(SRCBOOKS_DIR, Path.resolve(filePath));
+  const segments = relative.split(Path.sep);
+  return segments.length === 3 && segments[1] === 'src';
+}