mirror of
https://github.com/garrytan/gstack.git
synced 2026-06-04 17:18:11 +02:00
fix(design): reload guard rejects directory paths
design/src/serve.ts:200-212 used to accept a path that resolved to the
allowedDir itself (the OR branch `|| resolvedReload === allowedDir`),
which then crashed readFileSync with EISDIR. Now:
1. startsWith(allowedDir + path.sep) must pass — rejects the dir itself
and anything outside (403).
2. statSync(resolvedReload).isFile() must pass — rejects subdirectories
inside allowedDir with a clear "Path must be a file" 400.
The test stub in serve.test.ts mirrors prod; both updated, plus two new
test cases for the previously-broken paths. Codex caught this in the
plan-review pass; it's a latent bug in shipping code, not a regression
from the daemon work.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
+13
-7
@@ -197,19 +197,25 @@ export async function serve(options: ServeOptions): Promise<void> {
|
||||
);
|
||||
}
|
||||
|
||||
// Security: resolve symlinks and validate the reload path is within the
|
||||
// allowed directory (anchored to the initial HTML file's parent).
|
||||
// Prevents path traversal via /api/reload reading arbitrary files.
|
||||
// Security: resolve symlinks and validate the reload path is a FILE
|
||||
// inside the allowed directory (anchored to the initial HTML file's
|
||||
// parent). Prevents path traversal via /api/reload reading arbitrary
|
||||
// files. A path resolving to the allowedDir itself (a directory) used
|
||||
// to pass the guard and then crash readFileSync with EISDIR — reject
|
||||
// it explicitly with a clear 400 instead.
|
||||
const resolvedReload = fs.realpathSync(path.resolve(newHtmlPath));
|
||||
if (
|
||||
!resolvedReload.startsWith(allowedDir + path.sep) &&
|
||||
resolvedReload !== allowedDir
|
||||
) {
|
||||
if (!resolvedReload.startsWith(allowedDir + path.sep)) {
|
||||
return Response.json(
|
||||
{ error: `Path must be within: ${allowedDir}` },
|
||||
{ status: 403 },
|
||||
);
|
||||
}
|
||||
if (!fs.statSync(resolvedReload).isFile()) {
|
||||
return Response.json(
|
||||
{ error: `Path must be a file, not a directory: ${newHtmlPath}` },
|
||||
{ status: 400 },
|
||||
);
|
||||
}
|
||||
|
||||
// Swap the HTML content
|
||||
htmlContent = fs.readFileSync(resolvedReload, "utf-8");
|
||||
|
||||
@@ -311,9 +311,12 @@ describe('Serve /api/reload — path traversal protection', () => {
|
||||
}
|
||||
// Production path validation — same as design/src/serve.ts
|
||||
const resolvedReload = fs.realpathSync(path.resolve(body.html));
|
||||
if (!resolvedReload.startsWith(allowedDir + path.sep) && resolvedReload !== allowedDir) {
|
||||
if (!resolvedReload.startsWith(allowedDir + path.sep)) {
|
||||
return Response.json({ error: `Path must be within: ${allowedDir}` }, { status: 403 });
|
||||
}
|
||||
if (!fs.statSync(resolvedReload).isFile()) {
|
||||
return Response.json({ error: `Path must be a file, not a directory: ${body.html}` }, { status: 400 });
|
||||
}
|
||||
htmlContent = fs.readFileSync(resolvedReload, 'utf-8');
|
||||
return Response.json({ reloaded: true });
|
||||
})();
|
||||
@@ -372,6 +375,39 @@ describe('Serve /api/reload — path traversal protection', () => {
|
||||
const page = await fetch(baseUrl);
|
||||
expect(await page.text()).toContain('Safe reload');
|
||||
});
|
||||
|
||||
// Regression for the directory-instead-of-file guard (Codex finding).
|
||||
// Before: resolvedReload === allowedDir passed the guard and then
|
||||
// readFileSync threw EISDIR with no helpful message.
|
||||
test('blocks reload when path resolves to the allowed directory itself', async () => {
|
||||
const res = await fetch(`${baseUrl}/api/reload`, {
|
||||
method: 'POST',
|
||||
headers: { 'Content-Type': 'application/json' },
|
||||
body: JSON.stringify({ html: tmpDir }),
|
||||
});
|
||||
// tmpDir does not satisfy startsWith(allowedDir + sep), so the within-dir
|
||||
// check rejects with 403 — but importantly, no EISDIR crash.
|
||||
expect(res.status).toBe(403);
|
||||
});
|
||||
|
||||
test('blocks reload when path is a subdirectory (not a file)', async () => {
|
||||
const subdir = path.join(tmpDir, 'subdir-not-a-file');
|
||||
fs.mkdirSync(subdir, { recursive: true });
|
||||
try {
|
||||
const res = await fetch(`${baseUrl}/api/reload`, {
|
||||
method: 'POST',
|
||||
headers: { 'Content-Type': 'application/json' },
|
||||
body: JSON.stringify({ html: subdir }),
|
||||
});
|
||||
// Inside allowedDir but a directory — must fail before readFileSync,
|
||||
// with a clear "must be a file" error instead of EISDIR.
|
||||
expect(res.status).toBe(400);
|
||||
const data = await res.json();
|
||||
expect(data.error).toContain('must be a file');
|
||||
} finally {
|
||||
try { fs.rmSync(subdir, { recursive: true, force: true }); } catch {}
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
// ─── Full lifecycle: regeneration round-trip ──────────────────────
|
||||
|
||||
Reference in New Issue
Block a user