mirror of
https://github.com/garrytan/gstack.git
synced 2026-08-09 07:46:08 +02:00
merge: origin/main (v1.41.1.0 audit wave) into garrytan/daegu-v3
Conflicts resolved: - VERSION: keep 1.42.0.0 (queue-advance past #1592's v1.41.1.0 claim, per CLAUDE.md workspace-aware ship rule) - CHANGELOG.md: keep both entries — v1.42.0.0 Daegu wave on top, v1.41.1.0 audit wave below in reverse-chronological order - package.json: bump version field to 1.42.0.0 to match VERSION Auto-merged cleanly: - browse/src/meta-commands.ts: both the screenshot-size-guard wiring from C16 (this branch) and the parsePdfFromFile JSON validation from PR #1592 (main) survive in the merged file. Verified by inspecting the imports + call sites + running both test surfaces green. - All other v1.41.1.0 changes from main (scripts/build-app.sh sed hardening, mktemp fallback drops, security-classifier download cleanup, global-discover 64KB cap) pulled in unchanged. Verified 77 wave + audit tests green after merge: - browse/test/screenshot-size-guard.test.ts (7) - browse/test/regression-pr1169-pdf-from-file-invalid-json.test.ts - browse/test/security-classifier-download-cleanup.test.ts - browse/test/find-browse.test.ts (5) - test/gstack-paths.test.ts (9), test/gstack-gbrain-sync.test.ts (37) - test/memory-ingest-no-put_page.test.ts (2) - test/resolvers-gbrain-put-rewrite.test.ts (2) - test/extension-pty-inject-invariant.test.ts (3) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Garry Tan
@@ -137,7 +137,7 @@ function parsePdfArgs(args: string[]): ParsedPdfArgs {
|
||||
return result;
|
||||
}
|
||||
|
||||
function parsePdfFromFile(payloadPath: string): ParsedPdfArgs {
|
||||
export function parsePdfFromFile(payloadPath: string): ParsedPdfArgs {
|
||||
// Parity with load-html --from-file (browse/src/write-commands.ts) and
|
||||
// the direct load-html <file> path: every caller-supplied file path
|
||||
// must pass validateReadPath so the safe-dirs policy can't be skirted
|
||||
@@ -150,7 +150,16 @@ function parsePdfFromFile(payloadPath: string): ParsedPdfArgs {
|
||||
);
|
||||
}
|
||||
const raw = fs.readFileSync(payloadPath, 'utf8');
|
||||
const json = JSON.parse(raw);
|
||||
let json: any;
|
||||
try {
|
||||
json = JSON.parse(raw);
|
||||
} catch (err) {
|
||||
const msg = err instanceof Error ? err.message : String(err);
|
||||
throw new Error(`pdf: --from-file ${payloadPath} is not valid JSON (${msg}).`);
|
||||
}
|
||||
if (json === null || typeof json !== 'object' || Array.isArray(json)) {
|
||||
throw new Error(`pdf: --from-file ${payloadPath} must be a JSON object, got ${Array.isArray(json) ? 'array' : typeof json}.`);
|
||||
}
|
||||
const out: ParsedPdfArgs = {
|
||||
output: json.output || `${TEMP_DIR}/browse-page.pdf`,
|
||||
format: json.format,
|
||||
|
||||
@@ -135,7 +135,7 @@ export function getClassifierStatus(): ClassifierStatus {
|
||||
|
||||
// ─── Model download + staging ────────────────────────────────
|
||||
|
||||
async function downloadFile(url: string, dest: string): Promise<void> {
|
||||
export async function downloadFile(url: string, dest: string): Promise<void> {
|
||||
const res = await fetch(url);
|
||||
if (!res.ok || !res.body) {
|
||||
throw new Error(`Failed to fetch ${url}: ${res.status} ${res.statusText}`);
|
||||
@@ -144,16 +144,30 @@ async function downloadFile(url: string, dest: string): Promise<void> {
|
||||
const writer = fs.createWriteStream(tmp);
|
||||
// @ts-ignore — Node stream compat
|
||||
const reader = res.body.getReader();
|
||||
let done = false;
|
||||
while (!done) {
|
||||
const chunk = await reader.read();
|
||||
if (chunk.done) { done = true; break; }
|
||||
writer.write(chunk.value);
|
||||
try {
|
||||
let done = false;
|
||||
while (!done) {
|
||||
const chunk = await reader.read();
|
||||
if (chunk.done) { done = true; break; }
|
||||
writer.write(chunk.value);
|
||||
}
|
||||
await new Promise<void>((resolve, reject) => {
|
||||
writer.end((err?: Error | null) => (err ? reject(err) : resolve()));
|
||||
});
|
||||
fs.renameSync(tmp, dest);
|
||||
} catch (err) {
|
||||
// Drop the half-written tmp so we don't ship a truncated model file to
|
||||
// a retry's renameSync. Wait for the writer to close fully before
|
||||
// unlinking: Node's createWriteStream lazily opens the FD and flushes
|
||||
// buffered writes during destroy(), so a naive unlinkSync hits ENOENT
|
||||
// first and the writer re-creates the file on the next tick.
|
||||
await new Promise<void>((resolve) => {
|
||||
writer.once('close', () => resolve());
|
||||
writer.destroy();
|
||||
});
|
||||
try { fs.unlinkSync(tmp); } catch { /* nothing to clean */ }
|
||||
throw err;
|
||||
}
|
||||
await new Promise<void>((resolve, reject) => {
|
||||
writer.end((err?: Error | null) => (err ? reject(err) : resolve()));
|
||||
});
|
||||
fs.renameSync(tmp, dest);
|
||||
}
|
||||
|
||||
async function ensureTestsavantStaged(onProgress?: (msg: string) => void): Promise<void> {
|
||||
|
||||
@@ -0,0 +1,83 @@
|
||||
/**
|
||||
* Regression test for PR #1169 bug #7 — `pdf --from-file` ran JSON.parse on
|
||||
* user-supplied file contents with no try/catch. A malformed payload crashed
|
||||
* the pdf handler with a raw SyntaxError. Codex flagged that JSON.parse
|
||||
* accepts primitives too (numbers, strings, null) and Array.isArray must be
|
||||
* checked separately, so the fix added an explicit object-shape gate.
|
||||
*
|
||||
* Test surface: parsePdfFromFile, exported for tests at meta-commands.ts:139.
|
||||
* All fixtures land in process.cwd() (SAFE_DIRECTORIES allows TEMP_DIR or cwd;
|
||||
* cwd is universally safe on every platform our CI runs on).
|
||||
*/
|
||||
import { describe, expect, test, beforeAll, afterAll } from "bun:test";
|
||||
import * as fs from "node:fs";
|
||||
import * as path from "node:path";
|
||||
|
||||
import { parsePdfFromFile } from "../src/meta-commands";
|
||||
|
||||
const FIXTURE_DIR = fs.mkdtempSync(path.join(process.cwd(), "pr1169-pdf-"));
|
||||
|
||||
beforeAll(() => {
|
||||
// mkdtempSync already created the dir
|
||||
});
|
||||
|
||||
afterAll(() => {
|
||||
fs.rmSync(FIXTURE_DIR, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
function writeFixture(name: string, body: string): string {
|
||||
const p = path.join(FIXTURE_DIR, name);
|
||||
fs.writeFileSync(p, body);
|
||||
return p;
|
||||
}
|
||||
|
||||
describe("parsePdfFromFile — invalid JSON regression (PR #1169 bug #7)", () => {
|
||||
test("invalid JSON: throws with file path AND parser detail", () => {
|
||||
const p = writeFixture("invalid.json", "{ not-json");
|
||||
expect(() => parsePdfFromFile(p)).toThrow(/not valid JSON/);
|
||||
expect(() => parsePdfFromFile(p)).toThrow(p);
|
||||
});
|
||||
|
||||
test("empty file: throws JSON-parse style error", () => {
|
||||
const p = writeFixture("empty.json", "");
|
||||
// Empty string is invalid JSON per ECMA-404.
|
||||
expect(() => parsePdfFromFile(p)).toThrow(/not valid JSON/);
|
||||
});
|
||||
|
||||
test("top-level array: throws 'must be a JSON object' with type", () => {
|
||||
const p = writeFixture("array.json", JSON.stringify(["a", "b"]));
|
||||
expect(() => parsePdfFromFile(p)).toThrow(/must be a JSON object/);
|
||||
expect(() => parsePdfFromFile(p)).toThrow(/array/);
|
||||
});
|
||||
|
||||
test("top-level number: throws with 'number' type label", () => {
|
||||
const p = writeFixture("number.json", "42");
|
||||
expect(() => parsePdfFromFile(p)).toThrow(/must be a JSON object/);
|
||||
expect(() => parsePdfFromFile(p)).toThrow(/number/);
|
||||
});
|
||||
|
||||
test("top-level string: throws with 'string' type label", () => {
|
||||
const p = writeFixture("string.json", JSON.stringify("hello"));
|
||||
expect(() => parsePdfFromFile(p)).toThrow(/must be a JSON object/);
|
||||
expect(() => parsePdfFromFile(p)).toThrow(/string/);
|
||||
});
|
||||
|
||||
test("top-level null: throws with 'object' type label (JS null typeof === object)", () => {
|
||||
const p = writeFixture("null.json", "null");
|
||||
// null passes typeof === 'object' but the fix's `=== null` branch catches it.
|
||||
expect(() => parsePdfFromFile(p)).toThrow(/must be a JSON object/);
|
||||
});
|
||||
|
||||
test("top-level boolean: throws with 'boolean' type label", () => {
|
||||
const p = writeFixture("bool.json", "true");
|
||||
expect(() => parsePdfFromFile(p)).toThrow(/must be a JSON object/);
|
||||
expect(() => parsePdfFromFile(p)).toThrow(/boolean/);
|
||||
});
|
||||
|
||||
test("valid object: parses successfully (happy-path regression)", () => {
|
||||
const p = writeFixture("valid.json", JSON.stringify({ format: "A4", pageNumbers: true }));
|
||||
const result = parsePdfFromFile(p);
|
||||
expect(result.format).toBe("A4");
|
||||
expect(result.pageNumbers).toBe(true);
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,138 @@
|
||||
/**
|
||||
* Regression test for PR #1169 bug #6 — downloadFile opened a WriteStream to
|
||||
* `<dest>.tmp.<pid>` but never closed it on error paths. If the reader or
|
||||
* writer threw mid-download, the FD leaked and the half-written tmp could
|
||||
* be promoted by a retry's renameSync.
|
||||
*
|
||||
* The fix wraps the read loop in try/catch and runs `writer.destroy()` +
|
||||
* `fs.unlinkSync(tmp)` before rethrowing.
|
||||
*
|
||||
* Per codex's pushback, this test must exercise BOTH the reader-throws path
|
||||
* and the non-2xx-response path, and it must NOT assume the specific tmp
|
||||
* filename — only that no `<dest>.tmp.*` sibling remains.
|
||||
*/
|
||||
import { describe, expect, test, beforeAll, afterAll, beforeEach, afterEach } from "bun:test";
|
||||
import * as fs from "node:fs";
|
||||
import * as path from "node:path";
|
||||
|
||||
import { downloadFile } from "../src/security-classifier";
|
||||
|
||||
function tmpSiblings(destDir: string, destBase: string): string[] {
|
||||
if (!fs.existsSync(destDir)) return [];
|
||||
return fs.readdirSync(destDir).filter((f) =>
|
||||
f.startsWith(destBase + ".tmp.")
|
||||
);
|
||||
}
|
||||
|
||||
let FIXTURE_DIR = "";
|
||||
let originalFetch: typeof fetch;
|
||||
|
||||
beforeAll(() => {
|
||||
FIXTURE_DIR = fs.mkdtempSync(path.join(process.cwd(), "pr1169-dl-"));
|
||||
});
|
||||
|
||||
afterAll(() => {
|
||||
if (FIXTURE_DIR) {
|
||||
fs.rmSync(FIXTURE_DIR, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
beforeEach(() => {
|
||||
originalFetch = globalThis.fetch;
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
globalThis.fetch = originalFetch;
|
||||
});
|
||||
|
||||
describe("downloadFile error-path cleanup (PR #1169 bug #6)", () => {
|
||||
test("reader rejects mid-stream: throws, no dest, no tmp sibling left", async () => {
|
||||
const dest = path.join(FIXTURE_DIR, "reader-fail-model.bin");
|
||||
const destDir = path.dirname(dest);
|
||||
const destBase = path.basename(dest);
|
||||
|
||||
// Build a ReadableStream that emits one chunk then errors on second pull.
|
||||
const body = new ReadableStream<Uint8Array>({
|
||||
start(controller) {
|
||||
controller.enqueue(new Uint8Array([1, 2, 3, 4]));
|
||||
},
|
||||
pull(controller) {
|
||||
// Second pull triggers the failure path the fix protects against.
|
||||
controller.error(new Error("simulated mid-stream read failure"));
|
||||
},
|
||||
});
|
||||
|
||||
// @ts-expect-error — overwrite global fetch for the test
|
||||
globalThis.fetch = async () =>
|
||||
new Response(body, { status: 200, statusText: "OK" });
|
||||
|
||||
await expect(downloadFile("https://example.com/model.bin", dest)).rejects.toThrow(
|
||||
/simulated mid-stream read failure/
|
||||
);
|
||||
|
||||
expect(fs.existsSync(dest)).toBe(false);
|
||||
expect(tmpSiblings(destDir, destBase)).toEqual([]);
|
||||
});
|
||||
|
||||
test("non-2xx response: throws with status, no tmp file created", async () => {
|
||||
const dest = path.join(FIXTURE_DIR, "http500-model.bin");
|
||||
const destDir = path.dirname(dest);
|
||||
const destBase = path.basename(dest);
|
||||
|
||||
// @ts-expect-error — overwrite global fetch for the test
|
||||
globalThis.fetch = async () =>
|
||||
new Response("server boom", { status: 500, statusText: "Server Error" });
|
||||
|
||||
await expect(downloadFile("https://example.com/model.bin", dest)).rejects.toThrow(
|
||||
/Failed to fetch.*500/
|
||||
);
|
||||
|
||||
expect(fs.existsSync(dest)).toBe(false);
|
||||
expect(tmpSiblings(destDir, destBase)).toEqual([]);
|
||||
});
|
||||
|
||||
test("missing body: throws, no tmp file created", async () => {
|
||||
const dest = path.join(FIXTURE_DIR, "nobody-model.bin");
|
||||
const destDir = path.dirname(dest);
|
||||
const destBase = path.basename(dest);
|
||||
|
||||
// Response with null body (some upstreams send this on edge errors).
|
||||
// @ts-expect-error — overwrite global fetch for the test
|
||||
globalThis.fetch = async () =>
|
||||
new Response(null, { status: 200, statusText: "OK" });
|
||||
|
||||
await expect(downloadFile("https://example.com/model.bin", dest)).rejects.toThrow(
|
||||
/Failed to fetch/
|
||||
);
|
||||
|
||||
expect(fs.existsSync(dest)).toBe(false);
|
||||
expect(tmpSiblings(destDir, destBase)).toEqual([]);
|
||||
});
|
||||
|
||||
test("happy path: 2xx body completes, dest exists, no tmp sibling remains", async () => {
|
||||
const dest = path.join(FIXTURE_DIR, "ok-model.bin");
|
||||
const destDir = path.dirname(dest);
|
||||
const destBase = path.basename(dest);
|
||||
|
||||
const body = new ReadableStream<Uint8Array>({
|
||||
start(controller) {
|
||||
controller.enqueue(new Uint8Array([9, 9, 9, 9]));
|
||||
controller.close();
|
||||
},
|
||||
});
|
||||
|
||||
// @ts-expect-error — overwrite global fetch for the test
|
||||
globalThis.fetch = async () =>
|
||||
new Response(body, { status: 200, statusText: "OK" });
|
||||
|
||||
await downloadFile("https://example.com/model.bin", dest);
|
||||
|
||||
expect(fs.existsSync(dest)).toBe(true);
|
||||
expect(tmpSiblings(destDir, destBase)).toEqual([]);
|
||||
const written = fs.readFileSync(dest);
|
||||
expect(Array.from(written)).toEqual([9, 9, 9, 9]);
|
||||
|
||||
fs.unlinkSync(dest);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user