mirror of https://github.com/garrytan/gstack.git
fix(make-pdf): reject directories when resolving the browse binary
access(X_OK) is true for directories (they carry the execute/traverse bit on POSIX and pass the Windows existence check too), so cwd-dependent resolution could pick the ~/.claude/skills/browse alias DIRECTORY as the browse binary. Every browse call then exited 4 with empty stderr, which make-pdf surfaced as "Chromium failed to launch" against a perfectly healthy Chromium (#2156). Guard isExecutable with statSync().isFile() so only regular files qualify. Contributed by @jwilk-hrep (PR #2538). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
parent
420b2f730d
commit
7ca68da785
|
|
@ -158,6 +158,11 @@ export function resolveBrowseBin(env: NodeJS.ProcessEnv = process.env): string {
|
|||
|
||||
function isExecutable(p: string): boolean {
|
||||
try {
|
||||
// Must be a regular FILE. access(X_OK) alone is true for directories — they carry the
|
||||
// execute/traverse bit on POSIX and pass the Windows check too — so discovery happily
|
||||
// "found" ~/.claude/skills/browse, which is the skill's docs folder containing nothing
|
||||
// but SKILL.md, and returned a directory as the browse binary.
|
||||
if (!fs.statSync(p).isFile()) return false;
|
||||
fs.accessSync(p, fs.constants.X_OK);
|
||||
return true;
|
||||
} catch {
|
||||
|
|
|
|||
|
|
@ -59,6 +59,41 @@ describe("findExecutable", () => {
|
|||
const found = findExecutable("/nonexistent/path/to/nothing");
|
||||
expect(found).toBeNull();
|
||||
});
|
||||
|
||||
// access(X_OK) is TRUE for directories — they carry the execute/traverse bit — so a
|
||||
// bare X_OK test returned ~/.claude/skills/browse, the skill's docs folder, as "the
|
||||
// browse binary". Every browse call then failed with an empty error, which surfaced
|
||||
// as make-pdf reporting "Chromium failed to launch".
|
||||
test("rejects a DIRECTORY even though it passes access(X_OK)", () => {
|
||||
const dir = fs.mkdtempSync(path.join(os.tmpdir(), "mkpdf-dir-"));
|
||||
try {
|
||||
// Prove the precondition: the directory really does pass the old test.
|
||||
let passesXok = true;
|
||||
try {
|
||||
fs.accessSync(dir, fs.constants.X_OK);
|
||||
} catch {
|
||||
passesXok = false;
|
||||
}
|
||||
expect(passesXok).toBe(true);
|
||||
|
||||
expect(findExecutable(dir)).toBeNull();
|
||||
} finally {
|
||||
fs.rmSync(dir, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
test("rejects a directory that shadows a real binary name", () => {
|
||||
// The exact shape of the bug: a directory named like the thing being looked for.
|
||||
const base = fs.mkdtempSync(path.join(os.tmpdir(), "mkpdf-shadow-"));
|
||||
const shadow = path.join(base, "browse");
|
||||
fs.mkdirSync(shadow);
|
||||
fs.writeFileSync(path.join(shadow, "SKILL.md"), "# not a binary\n");
|
||||
try {
|
||||
expect(findExecutable(shadow)).toBeNull();
|
||||
} finally {
|
||||
fs.rmSync(base, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe("resolveBrowseBin", () => {
|
||||
|
|
|
|||
Loading…
Reference in New Issue