From 2a03426f3bed902738685a905090eb65ebdfd516 Mon Sep 17 00:00:00 2001 From: Serhii Vasylenko Date: Fri, 15 May 2026 02:12:44 +0200 Subject: [PATCH 1/2] chore: fix 4 open SonarCloud MAINTAINABILITY issues - src/cli.ts: flip savedTo branch to drop negated condition (S7735) - src/sandbox.ts: extract resolveAllowedRoot helper + early-return default path to drop buildAllowedRoots cognitive complexity (S3776) - src/sandbox.ts: extract walkToExtantAncestor + isContainedIn helpers to drop checkPath cognitive complexity (S3776) - src/sandbox.ts: hoist roots list into rootsList const to remove nested template literal in the not-contained error (S4624) Public exports (buildAllowedRoots, checkPath, CheckResult) keep identical signatures and error-message wording. All existing tests pass; tsc build succeeds. --- src/cli.ts | 8 +-- src/sandbox.ts | 132 +++++++++++++++++++++++++++++-------------------- 2 files changed, 83 insertions(+), 57 deletions(-) diff --git a/src/cli.ts b/src/cli.ts index b8d0579..20af7f2 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -44,12 +44,12 @@ program url, savePath, }); - if (savedTo !== undefined) { - // Confirmation message — the only stdout newline the CLI ever adds. - console.log(`Saved ${bytes} bytes to ${savedTo}`); - } else { + if (savedTo === undefined) { // Raw markdown body — no added newline, matches MCP content[0].text. process.stdout.write(markdown); + } else { + // Confirmation message — the only stdout newline the CLI ever adds. + console.log(`Saved ${bytes} bytes to ${savedTo}`); } } catch (err) { const { code, message } = classifyError(err); diff --git a/src/sandbox.ts b/src/sandbox.ts index e3de74a..f0e367f 100644 --- a/src/sandbox.ts +++ b/src/sandbox.ts @@ -29,44 +29,82 @@ export type CheckResult = | { ok: true; resolved: string } | { ok: false; reason: string }; +// Per-entry validation for MARKFETCH_ALLOWED_WRITE_ROOTS. Each entry must be +// absolute, resolvable via realpath, and a directory; any failure throws with +// the entry quoted in the message so misconfiguration is easy to diagnose. +async function resolveAllowedRoot(entry: string): Promise { + if (!isAbsolute(entry)) { + throw new Error( + `Invalid ${ENV_VAR} entry ${JSON.stringify(entry)} — every entry must be an absolute path.`, + ); + } + let resolvedEntry: string; + try { + resolvedEntry = await realpath(entry); + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + throw new Error( + `Invalid ${ENV_VAR} entry ${JSON.stringify(entry)} — could not resolve: ${message}`, + ); + } + const stats = await stat(resolvedEntry); + if (!stats.isDirectory()) { + throw new Error( + `Invalid ${ENV_VAR} entry ${JSON.stringify(entry)} — resolved to ${JSON.stringify(resolvedEntry)} which is not a directory.`, + ); + } + return resolvedEntry; +} + // Defaults: realpath(os.tmpdir()) ∪ realpath(process.cwd()). // MARKFETCH_ALLOWED_WRITE_ROOTS REPLACES the defaults (no merge) — deliberate; // setting it is asserting a policy, so additive defaults would weaken it. -// Callers who want tmpdir/cwd back must list them explicitly. Every entry -// must be absolute, resolvable via realpath, and a directory. Bad config +// Callers who want tmpdir/cwd back must list them explicitly. Bad config // throws at module init — same fail-fast contract as intEnv(). export async function buildAllowedRoots( env: NodeJS.ProcessEnv, ): Promise { const raw = env[ENV_VAR]; - if (raw != null && raw !== "") { - const resolved: string[] = []; - for (const entry of raw.split(delimiter)) { - if (!isAbsolute(entry)) { - throw new Error( - `Invalid ${ENV_VAR} entry ${JSON.stringify(entry)} — every entry must be an absolute path.`, - ); - } - let resolvedEntry: string; - try { - resolvedEntry = await realpath(entry); - } catch (err) { - const message = err instanceof Error ? err.message : String(err); - throw new Error( - `Invalid ${ENV_VAR} entry ${JSON.stringify(entry)} — could not resolve: ${message}`, - ); - } - const stats = await stat(resolvedEntry); - if (!stats.isDirectory()) { - throw new Error( - `Invalid ${ENV_VAR} entry ${JSON.stringify(entry)} — resolved to ${JSON.stringify(resolvedEntry)} which is not a directory.`, - ); - } - resolved.push(resolvedEntry); + if (raw == null || raw === "") { + return [await realpath(tmpdir()), await realpath(process.cwd())]; + } + // Sequential (not Promise.all) to preserve ordering and fail-fast-on-first- + // error semantics that the multi-entry test relies on. + const resolved: string[] = []; + for (const entry of raw.split(delimiter)) { + resolved.push(await resolveAllowedRoot(entry)); + } + return resolved; +} + +// Walk up `start` until an extant ancestor is found, accumulating the +// synthetic suffix that has to be reattached afterwards. Returns null when +// the filesystem root is reached without finding anything that exists — the +// caller fails closed in that case. +async function walkToExtantAncestor( + start: string, +): Promise<{ ancestor: string; trailing: string[] } | null> { + let ancestor = start; + const trailing: string[] = []; + while (true) { + try { + await stat(ancestor); + return { ancestor, trailing }; + } catch { + const parent = dirname(ancestor); + if (parent === ancestor) return null; + trailing.unshift(parse(ancestor).base); + ancestor = parent; } - return resolved; } - return [await realpath(tmpdir()), await realpath(process.cwd())]; +} + +// True iff `target` is `root` itself or a descendant. Both inputs must be +// pre-folded by the caller when running on a case-insensitive filesystem. +function isContainedIn(target: string, root: string): boolean { + const rel = relative(root, target); + if (rel === "") return true; + return !rel.startsWith("..") && !isAbsolute(rel); } // Resolve savePath through fs.realpath (defeating symlink escape) and check @@ -78,31 +116,19 @@ export async function checkPath( ): Promise { const normalized = resolve(savePath); - let ancestor = normalized; - const trailing: string[] = []; - while (true) { - try { - await stat(ancestor); - break; - } catch { - const parent = dirname(ancestor); - if (parent === ancestor) { - // Reached filesystem root with no extant ancestor — fail closed. - return { - ok: false, - reason: `cannot resolve any extant ancestor for '${savePath}'`, - }; - } - trailing.unshift(parse(ancestor).base); - ancestor = parent; - } + const walked = await walkToExtantAncestor(normalized); + if (walked === null) { + return { + ok: false, + reason: `cannot resolve any extant ancestor for '${savePath}'`, + }; } - const resolvedAncestor = await realpath(ancestor); + const resolvedAncestor = await realpath(walked.ancestor); const reattached = - trailing.length === 0 + walked.trailing.length === 0 ? resolvedAncestor - : join(resolvedAncestor, ...trailing); + : join(resolvedAncestor, ...walked.trailing); // Win32 case-fold: filesystem is case-insensitive and fs.realpath doesn't // reliably canonicalize case, so compare both sides lowercased. @@ -113,14 +139,14 @@ export async function checkPath( const foldedTarget = fold(reattached); for (const root of roots) { - const rel = relative(fold(root), foldedTarget); - if (rel === "") return { ok: true, resolved: reattached }; - if (!rel.startsWith("..") && !isAbsolute(rel)) { + if (isContainedIn(foldedTarget, fold(root))) { return { ok: true, resolved: reattached }; } } + + const rootsList = roots.map((r) => `'${r}'`).join(", "); return { ok: false, - reason: `'${reattached}' is outside the allowed write roots: [${roots.map((r) => `'${r}'`).join(", ")}]`, + reason: `'${reattached}' is outside the allowed write roots: [${rootsList}]`, }; } From b472be8fe63799b7f141fd391b1d573f4154d34f Mon Sep 17 00:00:00 2001 From: Serhii Vasylenko Date: Fri, 15 May 2026 02:54:04 +0200 Subject: [PATCH 2/2] refactor(sandbox): trim helper extraction overhead MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Followup to the Sonar cleanup commit: re-inline two helpers whose extraction was net cosmetic, drop a redundant ternary, and remove two comments that didn't pull weight. Public API and behavior unchanged; Sonar rules still satisfied. Changes: - Inline resolveAllowedRoot back into buildAllowedRoots: the early- return for the default path alone drops S3776 complexity (17 to ~9 by Sonar's counting), so the helper was redundant. - Inline isContainedIn back into checkPath's for-loop: with the redundant 'rel === ""' check gone, the body is two lines — inlining doesn't push checkPath above the 15-point threshold (still ~6 points with walkToExtantAncestor still extracted). - Collapse 'reattached' ternary to join(resolvedAncestor, ...walked.trailing). path.join(x) returns x when no extra segments are passed, so the trailing.length === 0 guard was dead code. - Drop the 'Sequential (not Promise.all)' meta-comment (explains an alternative not taken). - Drop the walkToExtantAncestor header comment (function name + return type already document it). Combined PR diff vs main: +59 / -55 = +4 LOC net. --- src/sandbox.ts | 74 ++++++++++++++++++-------------------------------- 1 file changed, 26 insertions(+), 48 deletions(-) diff --git a/src/sandbox.ts b/src/sandbox.ts index f0e367f..c291255 100644 --- a/src/sandbox.ts +++ b/src/sandbox.ts @@ -29,37 +29,11 @@ export type CheckResult = | { ok: true; resolved: string } | { ok: false; reason: string }; -// Per-entry validation for MARKFETCH_ALLOWED_WRITE_ROOTS. Each entry must be -// absolute, resolvable via realpath, and a directory; any failure throws with -// the entry quoted in the message so misconfiguration is easy to diagnose. -async function resolveAllowedRoot(entry: string): Promise { - if (!isAbsolute(entry)) { - throw new Error( - `Invalid ${ENV_VAR} entry ${JSON.stringify(entry)} — every entry must be an absolute path.`, - ); - } - let resolvedEntry: string; - try { - resolvedEntry = await realpath(entry); - } catch (err) { - const message = err instanceof Error ? err.message : String(err); - throw new Error( - `Invalid ${ENV_VAR} entry ${JSON.stringify(entry)} — could not resolve: ${message}`, - ); - } - const stats = await stat(resolvedEntry); - if (!stats.isDirectory()) { - throw new Error( - `Invalid ${ENV_VAR} entry ${JSON.stringify(entry)} — resolved to ${JSON.stringify(resolvedEntry)} which is not a directory.`, - ); - } - return resolvedEntry; -} - // Defaults: realpath(os.tmpdir()) ∪ realpath(process.cwd()). // MARKFETCH_ALLOWED_WRITE_ROOTS REPLACES the defaults (no merge) — deliberate; // setting it is asserting a policy, so additive defaults would weaken it. -// Callers who want tmpdir/cwd back must list them explicitly. Bad config +// Callers who want tmpdir/cwd back must list them explicitly. Every entry +// must be absolute, resolvable via realpath, and a directory. Bad config // throws at module init — same fail-fast contract as intEnv(). export async function buildAllowedRoots( env: NodeJS.ProcessEnv, @@ -68,19 +42,33 @@ export async function buildAllowedRoots( if (raw == null || raw === "") { return [await realpath(tmpdir()), await realpath(process.cwd())]; } - // Sequential (not Promise.all) to preserve ordering and fail-fast-on-first- - // error semantics that the multi-entry test relies on. const resolved: string[] = []; for (const entry of raw.split(delimiter)) { - resolved.push(await resolveAllowedRoot(entry)); + if (!isAbsolute(entry)) { + throw new Error( + `Invalid ${ENV_VAR} entry ${JSON.stringify(entry)} — every entry must be an absolute path.`, + ); + } + let resolvedEntry: string; + try { + resolvedEntry = await realpath(entry); + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + throw new Error( + `Invalid ${ENV_VAR} entry ${JSON.stringify(entry)} — could not resolve: ${message}`, + ); + } + const stats = await stat(resolvedEntry); + if (!stats.isDirectory()) { + throw new Error( + `Invalid ${ENV_VAR} entry ${JSON.stringify(entry)} — resolved to ${JSON.stringify(resolvedEntry)} which is not a directory.`, + ); + } + resolved.push(resolvedEntry); } return resolved; } -// Walk up `start` until an extant ancestor is found, accumulating the -// synthetic suffix that has to be reattached afterwards. Returns null when -// the filesystem root is reached without finding anything that exists — the -// caller fails closed in that case. async function walkToExtantAncestor( start: string, ): Promise<{ ancestor: string; trailing: string[] } | null> { @@ -99,14 +87,6 @@ async function walkToExtantAncestor( } } -// True iff `target` is `root` itself or a descendant. Both inputs must be -// pre-folded by the caller when running on a case-insensitive filesystem. -function isContainedIn(target: string, root: string): boolean { - const rel = relative(root, target); - if (rel === "") return true; - return !rel.startsWith("..") && !isAbsolute(rel); -} - // Resolve savePath through fs.realpath (defeating symlink escape) and check // containment against allowed roots. Walks up to the deepest extant ancestor // because the leaf usually doesn't exist yet — that's the point of "save". @@ -125,10 +105,7 @@ export async function checkPath( } const resolvedAncestor = await realpath(walked.ancestor); - const reattached = - walked.trailing.length === 0 - ? resolvedAncestor - : join(resolvedAncestor, ...walked.trailing); + const reattached = join(resolvedAncestor, ...walked.trailing); // Win32 case-fold: filesystem is case-insensitive and fs.realpath doesn't // reliably canonicalize case, so compare both sides lowercased. @@ -139,7 +116,8 @@ export async function checkPath( const foldedTarget = fold(reattached); for (const root of roots) { - if (isContainedIn(foldedTarget, fold(root))) { + const rel = relative(fold(root), foldedTarget); + if (!rel.startsWith("..") && !isAbsolute(rel)) { return { ok: true, resolved: reattached }; } }