Skip to content

Navigation Menu

Sign in
Sign up

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
nichochar wants to merge 2 commits into fix/network-security
base: fix/network-security
Choose a base branch
Loading
from fix/injection
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions .changeset/olive-pots-repeat.md
View file Open in desktop
Original file line number Diff line number Diff line change
@@ -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.
39 changes: 23 additions & 16 deletions packages/api/deps.mts
View file Open in desktop
Original file line number Diff line number Diff line change
@@ -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';

Expand Down Expand Up @@ -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)));
}
},
);
});
Expand Down
42 changes: 42 additions & 0 deletions packages/api/path-utils.mts
View file Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
}
31 changes: 31 additions & 0 deletions packages/api/schemas/config.mts
View file Open in desktop
Original file line number Diff line number Diff line change
@@ -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>;
67 changes: 58 additions & 9 deletions packages/api/server/http.mts
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand All @@ -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',
},
});
Expand Down Expand Up @@ -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 });
Expand Down Expand Up @@ -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 });
Expand Down
29 changes: 21 additions & 8 deletions packages/api/session.mts
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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> = {};

Expand Down Expand Up @@ -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);
});
});
Expand Down
4 changes: 3 additions & 1 deletion packages/api/srcbook/index.mts
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
13 changes: 13 additions & 0 deletions packages/api/srcbook/path.mts
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
}
Loading
Loading

AltStyle によって変換されたページ (->オリジナル) /