diff --git a/bin/ocx.mjs b/bin/ocx.mjs index ba69851520..880cbeec09 100755 --- a/bin/ocx.mjs +++ b/bin/ocx.mjs @@ -17,6 +17,10 @@ import { dirname, join, resolve } from "node:path"; import { fileURLToPath } from "node:url"; import { isRealBunBinary } from "../src/lib/bun-binary-validator.mjs"; import { npmInvocation } from "../src/update/npm-invocation.mjs"; +import { + npmCachePreflightFailureMessage, + runNpmCachePreflight, +} from "../src/update/npm-cache-preflight.mjs"; import { handoffWindowsTrayForUpdate, planWindowsTrayUpdate } from "../src/update/tray-update-plan.mjs"; const PKG = "@bitkyc08/opencodex"; @@ -136,6 +140,12 @@ function runNpmSelfUpdate() { process.exit(0); } + const cachePreflight = runNpmCachePreflight(); + if (!cachePreflight.ok) { + console.error(`opencodex: ${npmCachePreflightFailureMessage(cachePreflight.reason)}. Aborting before stopping the proxy.`); + process.exit(1); + } + // Remember whether a background service manages the proxy BEFORE stopping — `ocx stop` // unloads it, so a successful update must refresh and restart it afterwards. const serviceStatePath = join(configDir(), "service-state.json"); diff --git a/docs-site/src/content/docs/ja/reference/cli/lifecycle.md b/docs-site/src/content/docs/ja/reference/cli/lifecycle.md index 0905507adc..5f6a049987 100644 --- a/docs-site/src/content/docs/ja/reference/cli/lifecycle.md +++ b/docs-site/src/content/docs/ja/reference/cli/lifecycle.md @@ -212,7 +212,7 @@ Windows ステータス トレイ アイコンをインストールして制御 ### `ocx update [--tag latest|preview]` -npm から opencodex を自己更新します。安定したインストールでは `@latest` を使用します。 `--tag latest|preview` を渡さない限り、プレビュー インストールは `@preview` に残ります。ソース チェックアウトを検出し、代わりに `git pull && bun install` を使用するように指示しますが、そのタグの最新バージョンをすでに使用している場合は何もしません。実行中のプロキシは、ファイルが置き換えられる前に停止されます。インストールされたサービスは再構築されて自動的に開始されますが、フォアグラウンド インストールでは次のステップとして `ocx start` が出力されます。 +npm から opencodex を自己更新します。安定したインストールでは `@latest` を使用します。 `--tag latest|preview` を渡さない限り、プレビュー インストールは `@preview` に残ります。ソース チェックアウトを検出し、代わりに `git pull && bun install` を使用するように指示しますが、そのタグの最新バージョンをすでに使用している場合は何もしません。npm インストールでは、何かを停止する前に Unix キャッシュの所有権とアクセスを上限付きで検査します。ネストされたシンボリックリンクは `lstat` で確認しますが追跡しません。Windows では、この Unix 専用検査を明示的にスキップします。検査に失敗した場合、トレイとプロキシを実行したまま更新を中止します。その後、実行中のプロキシはファイルが置き換えられる前に停止されます。インストールされたサービスは再構築されて自動的に開始されますが、フォアグラウンド インストールでは次のステップとして `ocx start` が出力されます。ダッシュボードの更新記録では、保存前にプロファイル／キャッシュのパスと UID/GID 値が秘匿されます。 ```bash ocx update diff --git a/docs-site/src/content/docs/ko/reference/cli/lifecycle.md b/docs-site/src/content/docs/ko/reference/cli/lifecycle.md index 83586d5332..032a13d8ff 100644 --- a/docs-site/src/content/docs/ko/reference/cli/lifecycle.md +++ b/docs-site/src/content/docs/ko/reference/cli/lifecycle.md @@ -274,8 +274,12 @@ Windows 상태 트레이 아이콘을 설치하고 제어합니다. Windows 로 npm에서 opencodex를 자체 업데이트합니다. 안정판 설치는 `@latest`를 사용하고, 미리보기 설치는 `--tag latest|preview`를 주지 않으면 `@preview`를 유지합니다. 소스 체크아웃을 감지하면 대신 `git pull && bun install`을 실행하라고 안내하고, 해당 태그에서 이미 최신 버전이면 아무 동작도 하지 -않습니다. 실행 중인 프록시가 있으면 파일을 교체하기 전에 중지합니다. 설치된 서비스는 자동으로 다시 -빌드해 시작하며, 포그라운드 설치에서는 다음 단계로 `ocx start`를 출력합니다. +않습니다. npm 설치에서는 어떤 프로세스도 중지하기 전에 Unix 캐시의 소유권과 접근 가능성을 제한된 +범위에서 검사합니다. 중첩 심볼릭 링크는 `lstat`으로 확인하되 따라가지 않으며, Windows에서는 이 +Unix 전용 검사를 명시적으로 건너뜁니다. 검사에 실패하면 트레이와 프록시가 실행 중인 상태에서 +업데이트를 중단합니다. 그 다음 실행 중인 프록시가 있으면 파일을 교체하기 전에 중지합니다. 설치된 +서비스는 자동으로 다시 빌드해 시작하며, 포그라운드 설치에서는 다음 단계로 `ocx start`를 출력합니다. +대시보드 업데이트 기록은 저장 전에 프로필/캐시 경로와 UID/GID 값을 가립니다. ```bash ocx update diff --git a/docs-site/src/content/docs/reference/cli/lifecycle.md b/docs-site/src/content/docs/reference/cli/lifecycle.md index ef1f0f47a4..80e0a1d789 100644 --- a/docs-site/src/content/docs/reference/cli/lifecycle.md +++ b/docs-site/src/content/docs/reference/cli/lifecycle.md @@ -325,8 +325,12 @@ if it is not running. Self-update opencodex from npm. Stable installs use `@latest`; preview installs stay on `@preview` unless you pass `--tag latest|preview`. It detects a source checkout and tells you to `git pull && bun install` instead, and is a no-op if you are already on the newest version for that -tag. A running proxy is stopped before files are replaced; an installed service is rebuilt and -started automatically, while a foreground installation prints `ocx start` as the next step. +tag. Before stopping anything, npm installations run a bounded Unix cache ownership and access +check. Nested symlinks are checked with `lstat` but not followed; Windows explicitly skips this +Unix-only check. A failure aborts while the tray and proxy are still running. A running proxy is +then stopped before files are replaced; an installed service is rebuilt and started automatically, +while a foreground installation prints `ocx start` as the next step. Dashboard update records +redact profile/cache paths and UID/GID values before they are persisted. ```bash ocx update diff --git a/docs-site/src/content/docs/ru/reference/cli/lifecycle.md b/docs-site/src/content/docs/ru/reference/cli/lifecycle.md index 62dc3d5556..ded9943ad4 100644 --- a/docs-site/src/content/docs/ru/reference/cli/lifecycle.md +++ b/docs-site/src/content/docs/ru/reference/cli/lifecycle.md @@ -295,9 +295,13 @@ one-click управление прокси. `start` и `stop` управляю Самообновить opencodex из npm. Стабильные установки используют `@latest`; preview-установки остаются на `@preview`, если только вы не передадите `--tag latest|preview`. Команда распознаёт source checkout и предлагает вместо этого `git pull && bun install`, а если у вас уже новейшая -версия для выбранного тега, становится no-op. Перед заменой файлов работающий прокси -останавливается; установленная служба автоматически пересобирается и запускается заново, а для -foreground-установки печатается подсказка `ocx start`. +версия для выбранного тега, становится no-op. Для npm-установок до остановки каких-либо процессов +выполняется ограниченная проверка владельца и доступности Unix-кэша. Вложенные символические ссылки +проверяются через `lstat`, но переход по ним не выполняется; в Windows эта Unix-проверка явно +пропускается. При ошибке обновление отменяется, пока трей и прокси ещё работают. Затем перед заменой +файлов работающий прокси останавливается; установленная служба автоматически пересобирается и +запускается заново, а для foreground-установки печатается подсказка `ocx start`. В записях обновления +дашборда пути профиля/кэша и значения UID/GID скрываются до сохранения. ```bash ocx update diff --git a/docs-site/src/content/docs/zh-cn/reference/cli/lifecycle.md b/docs-site/src/content/docs/zh-cn/reference/cli/lifecycle.md index 124a36c053..e2a4605057 100644 --- a/docs-site/src/content/docs/zh-cn/reference/cli/lifecycle.md +++ b/docs-site/src/content/docs/zh-cn/reference/cli/lifecycle.md @@ -209,7 +209,7 @@ ocx codex-shim uninstall ### `ocx update [--tag latest|preview]` -从 npm 自更新 opencodex。稳定版安装使用 `@latest`；预览版安装保持在 `@preview`，除非你传入 `--tag latest|preview`。它会检测源码检出，并提示你改为运行 `git pull && bun install`；如果你已经是该标签的最新版本，则不会执行任何操作。在替换文件之前会先停止正在运行的代理；已安装的服务会自动重建并启动，而前台安装则会打印 `ocx start` 作为下一步。 +从 npm 自更新 opencodex。稳定版安装使用 `@latest`；预览版安装保持在 `@preview`，除非你传入 `--tag latest|preview`。它会检测源码检出，并提示你改为运行 `git pull && bun install`；如果你已经是该标签的最新版本，则不会执行任何操作。对于 npm 安装，它会在停止任何进程之前，对 Unix 缓存的所有权和访问权限执行有界检查。嵌套符号链接会通过 `lstat` 检查但不会跟随；Windows 会明确跳过这项仅适用于 Unix 的检查。检查失败时，更新会在托盘和代理仍运行的情况下中止。随后才会在替换文件之前停止正在运行的代理；已安装的服务会自动重建并启动，而前台安装则会打印 `ocx start` 作为下一步。持久化前，仪表板更新记录会隐去用户配置文件/缓存路径以及 UID/GID 值。 ```bash ocx update diff --git a/src/update/index.ts b/src/update/index.ts index 5c391c2888..e4a6896281 100644 --- a/src/update/index.ts +++ b/src/update/index.ts @@ -4,6 +4,10 @@ import { fileURLToPath } from "node:url"; import { dirname, join } from "node:path"; import { getConfigDir, loadConfig, readPid, readRuntimePort } from "../config"; import { npmInvocation } from "./npm-invocation.mjs"; +import { + npmCachePreflightFailureMessage, + runNpmCachePreflight, +} from "./npm-cache-preflight.mjs"; import { handoffWindowsTrayForUpdate, planWindowsTrayUpdate } from "./tray-update-plan.mjs"; import { withProcessRuntimeProvenance } from "../lib/bun-runtime"; @@ -178,6 +182,14 @@ export async function runUpdate(): Promise { console.log(`Verified ${PKG}@${latest} integrity metadata ${integrity.integrity.slice(0, 24)}…`); } + if (installer === "npm") { + const cachePreflight = runNpmCachePreflight(); + if (!cachePreflight.ok) { + console.error(`⚠️ ${npmCachePreflightFailureMessage(cachePreflight.reason)}. Aborting before stopping the proxy.`); + process.exit(1); + } + } + const { bin, args: cmdArgs } = updateCommand(installer, tag, latest); const target = updateSpawnTarget(bin, cmdArgs); if (!target) { diff --git a/src/update/job.ts b/src/update/job.ts index 3b9557f953..abea4ee128 100644 --- a/src/update/job.ts +++ b/src/update/job.ts @@ -37,6 +37,11 @@ import { import { isNewer } from "./notify"; import { isRealBunBinary } from "../lib/bun-binary-validator.mjs"; import { handoffWindowsTrayForUpdate, planWindowsTrayUpdate } from "./tray-update-plan.mjs"; +import { + npmCachePreflightFailureMessage, + runNpmCachePreflight, + type NpmCachePreflightReason, +} from "./npm-cache-preflight.mjs"; const RELEASE_NOTES_URL = "https://github.com/lidge-jun/opencodex/releases/latest"; const UPDATE_JOB_FILENAME = "update-job.json"; @@ -238,9 +243,152 @@ function ensureJobDir(): void { if (!existsSync(dir)) mkdirSync(dir, { recursive: true, mode: 0o700 }); } +/** + * Describe external text without reproducing it. + * + * Use this wherever an `Error.message`, a vendor stream, or any string this module did not + * compose would otherwise be interpolated into a persisted field. The result names the error's + * TYPE and size — enough to tell a reader what class of failure occurred — and never its text, + * which is where the paths and account names live. + */ +/** + * A version string we are willing to repeat in a persisted field. + * + * Semver plus an optional prerelease/build tail, capped in length. Anything else is dropped + * rather than logged: `/healthz` is answered by whatever holds the port, so its `version` is + * external input on the same footing as an error message. + */ +function isVersionLike(value: unknown): value is string { + return typeof value === "string" + && value.length <= 64 + && /^\d+\.\d+\.\d+(?:-[0-9A-Za-z.-]+)?(?:\+[0-9A-Za-z.-]+)?$/.test(value); +} + +function withheldSummary(error: unknown): string { + // `error.name` is writable, so it is external text like the message. A fixed classification + // is the only part of an unknown error we can state without repeating something we were + // handed: `new Error(...)` with `error.name = "Jane Doe"` was persisting the name verbatim. + const name = error instanceof Error ? "Error" : typeof error; + // NO MESSAGE TEXT, ever. An earlier version kept messages that carried no path, which sounds + // reasonable and is wrong: `spawn denied for Jane Doe` has no path in it and still names a + // person. There is no test on message CONTENT that separates a diagnostic from an identity, + // so the message does not cross this boundary at all. + const code = (error as { code?: unknown } | null)?.code; + // Only recognized codes — an arbitrary uppercase `error.code` can be attacker-shaped too. + const codeNote = typeof code === "string" && NPM_ERROR_CODES.has(code) ? ` ${code}` : ""; + const text = error instanceof Error ? error.message : String(error ?? ""); + // Node's own errors are structured the same way npm's output is: `syscall` and `errno` are + // named properties, not prose. Reading those gives a user the actual cause — + // `Error EACCES · syscall: mkdir · errno: -13` — without repeating a message that could name + // a person or a path. Both are shape-validated: a syscall is a short lowercase identifier and + // an errno is an integer, so neither can carry arbitrary text. + const parts = [`${name}${codeNote}`]; + const syscall = (error as { syscall?: unknown } | null)?.syscall; + // Same explicit vocabulary as the npm field: a shape check accepts `janedoe`. + if (typeof syscall === "string" && POSIX_SYSCALLS.has(syscall)) parts.push(`syscall: ${syscall}`); + const errno = (error as { errno?: unknown } | null)?.errno; + if (typeof errno === "number" && Number.isInteger(errno)) parts.push(`errno: ${errno}`); + parts.push(`${Buffer.byteLength(text, "utf8")} bytes withheld`); + return parts.join(" · "); +} + +/** + * Decide, per field, whether the value is ours to keep. + * + * `log` and `error` are composed from this module's own templates; every place that would have + * interpolated external text now calls `withheldSummary()` first, so the strings arriving here + * are ours by construction. `releaseNotesUrl` is compared against the module constant rather + * than pattern-matched, which is what stops a URL-shaped value from smuggling a path. + * `command` is rendered from validated parts. + */ +function brandOwnComposedText(key: string, value: unknown): unknown { + if (key === "releaseNotesUrl") { + return value === RELEASE_NOTES_URL ? value : ""; + } + if (key === "command") { + // Render the command shape first, then apply the same path test as every other field. The + // renderer only understands space-separated arguments; anything else reaching this field is + // not a command we built and must not be trusted because of where it was stored. + return typeof value === "string" ? withholdIfPathBearing(renderSafeCommand(value)) : value; + } + // `log` and `error` are ours by construction, but a caller can still slip external text in by + // interpolating it. Withhold any value that carries an absolute path of any form — that is a + // narrow, unambiguous test on strings we already control, not the free-text classification + // that failed nine times. + if (typeof value === "string") return withholdIfPathBearing(value); + if (Array.isArray(value)) return value.map(item => (typeof item === "string" ? withholdIfPathBearing(item) : item)); + return value; +} + +/** Absolute paths cannot appear in text this module composed; if one does, it came from outside. */ +function withholdIfPathBearing(value: string): string { + const pathBearing = /[A-Za-z]:[\\/]/.test(value) // C:\ or C:/ + || /\\\\/.test(value) // \\server\share + || /\\/.test(value) // any backslash + || /~[\w.-]*\//.test(value) // ~/ or ~user/ anywhere + || /[%$][A-Za-z_]/.test(value) // %APPDATA%, $HOME + || /\/[\w.\-~%]+\//.test(value) // any two-segment path run + || /\b(?:Users|home|Documents and Settings|AppData|Profiles)\b/i.test(value) + || /\r?\n/.test(value); // multi-line vendor output + if (!pathBearing) return value; + return ``; +} + +/** + * Keep a command readable without persisting the launcher path it contains. + * + * The real npm worker command is `node /Users//.../bin/ocx.mjs update --tag latest`, so + * the account name is inside it by construction. Absolute path arguments are replaced with a + * placeholder and everything else — the binary name, the flags, the tag — is kept, which is the + * part a reader actually needs. + */ +function renderSafeCommand(value: string): string { + if (!value) return value; + // Rebuild from a recognized shape rather than filtering the string we were handed. Content + // cannot distinguish `npm install Mary-Jane` — an account name — from a legitimate package + // argument, so anything that is not this exact shape is withheld by the caller's path test. + const parts = value.trim().split(/\s+/); + const tool = parts[0] === "$" ? parts[1] : parts[0]; + if (tool !== undefined && /^(?:npm|bun|pnpm|yarn|node)$/.test(tool)) { + const rendered = parts.map(part => + /^(?:[A-Za-z]:[\\/]|[\\/]|~|\\\\)/.test(part) ? "" : part); + // Only fixed flags, our own package spec, and placeholders survive; a bare word that is not + // one of those is treated as unknown input and the whole value is withheld. + const allowed = rendered.every(part => + part === "$" || part === "" + || /^(?:npm|bun|pnpm|yarn|node)$/.test(part) + || /^-{1,2}[\w-]+$/.test(part) + || /^(?:install|add|update|i)$/.test(part) + || /^opencodex(?:@[\w.\-]+)?$/.test(part) + || /^(?:latest|preview|next|beta)$/.test(part) + || /^\d[\w.\-]*$/.test(part)); + if (allowed) return rendered.join(" "); + } + return ``; +} + +/** + * Fields that can carry free-form text and therefore need checking at the write boundary. + * + * The rest of the record is a closed vocabulary — statuses, channels, installers, versions, an + * id, timestamps — so checking it only risks mangling values that were never a disclosure + * route. Naming the risky fields keeps the boundary narrow and auditable. + */ +const FREE_TEXT_JOB_FIELDS = new Set(["command", "error", "log", "releaseNotesUrl"]); + +/** Apply the per-field rule at the single point where a job reaches disk. */ +function sanitizePersistedUpdateJob(job: UpdateJobState): UpdateJobState { + return Object.fromEntries( + Object.entries(job).map(([key, item]) => [ + key, + FREE_TEXT_JOB_FIELDS.has(key) ? brandOwnComposedText(key, item) : item, + ]), + ) as UpdateJobState; +} + function writeJob(job: UpdateJobState): void { ensureJobDir(); - atomicWriteFile(updateJobPath(), `${JSON.stringify(job, null, 2)}\n`); + atomicWriteFile(updateJobPath(), `${JSON.stringify(sanitizePersistedUpdateJob(job), null, 2)}\n`); } export function readUpdateJob(jobId?: string | null): UpdateJobState | null { @@ -254,6 +402,13 @@ export function readUpdateJob(jobId?: string | null): UpdateJobState | null { } } +/** + * Log lines are composed by this module, so brand them here rather than at nineteen call sites. + * + * The one thing a caller must never do is interpolate external text into a log line — an + * `Error.message`, a vendor stream, a path we were handed. Those go through + * `withheldSummary()`, which produces a branded description WITHOUT the text itself. + */ function updateJob(job: UpdateJobState, patch: Partial, logLine?: string): UpdateJobState { const current = readUpdateJob(job.id) ?? job; const next = { @@ -495,8 +650,7 @@ export function startUpdateJob( try { child = resolvedDeps.spawnWorkerFn(id, channel, restart); } catch (error) { - const message = error instanceof Error ? error.message : String(error); - updateJob(job, { status: "failed", error: `Could not start update worker: ${message}` }, "Update worker failed to start."); + updateJob(job, { status: "failed", error: `Could not start update worker: ${withheldSummary(error)}` }, "Update worker failed to start."); throw new UpdateJobError("Could not start update worker", 500, "update_worker_start_failed"); } if (typeof child.pid !== "number" || !Number.isSafeInteger(child.pid) || child.pid <= 0) { @@ -509,7 +663,7 @@ export function startUpdateJob( if (!current || current.pid !== child.pid || (current.status !== "running" && current.status !== "restarting")) return; updateJob( current, - { status: "failed", error: `Update worker failed to start: ${error.message}` }, + { status: "failed", error: `Update worker failed to start: ${withheldSummary(error)}` }, "Update worker emitted a startup error.", ); }); @@ -517,6 +671,20 @@ export function startUpdateJob( return startedJob; } +/** + * Run an update step and record WHAT HAPPENED, not what the tool printed. + * + * Raw installer output used to be persisted verbatim, which put local paths and account names + * into a stored file. Six rounds of trying to sanitize it after the fact each produced a new + * leak — a wrap inside the keyword, a wrap inside the account name, an indented continuation, + * three consecutive wraps, an empty continuation line. Every fix was an attempt to reconstruct + * arbitrary multi-line text well enough to match it, and that is not a problem a redactor can + * win: the leak surface is whatever npm decides to print. + * + * So the raw stream is no longer persisted at all. The job keeps the command, its exit status, + * and a bounded, structured summary — enough to tell a user which step failed and how, with no + * free-form vendor text passing through the boundary. Detailed output stays ephemeral. + */ function runLoggedCommand(job: UpdateJobState, bin: string, args: string[], timeout: number): { status: number | null; signal: NodeJS.Signals | null } { job = updateJob(job, {}, `$ ${formatCommand(bin, args)}`); const result = spawnSync(bin, args, { @@ -526,11 +694,171 @@ function runLoggedCommand(job: UpdateJobState, bin: string, args: string[], time }); const stdout = typeof result.stdout === "string" ? result.stdout.trim() : ""; const stderr = typeof result.stderr === "string" ? result.stderr.trim() : ""; - if (stdout) job = updateJob(job, {}, stdout.slice(-4000)); - if (stderr) updateJob(job, {}, stderr.slice(-4000)); + const summary = summarizeCommandOutput(stdout, stderr, result.status, result.signal); + if (summary) updateJob(job, {}, summary); return { status: result.status, signal: result.signal }; } +/** + * Recognized npm/libc error codes, as an explicit set. + * + * A shape pattern like `E[A-Z]{3,}` is NOT a vocabulary: `C:\Users\ERROR\.npm` matches it, and + * the summary then re-emits the username the withheld output was protecting. Only codes on this + * list are surfaced, and only when they appear in npm's canonical `code ` position. + */ +const NPM_ERROR_CODES = new Set([ + "EACCES", "EPERM", "ENOENT", "EEXIST", "ENOTDIR", "EISDIR", "EMFILE", "ENFILE", + "ENOSPC", "EROFS", "EXDEV", "ELOOP", "ENAMETOOLONG", "ENOTEMPTY", "EBUSY", + "EAGAIN", "ECONNRESET", "ECONNREFUSED", "ETIMEDOUT", "ENOTFOUND", "EAI_AGAIN", + "EPROTO", "ECONNABORTED", "EHOSTUNREACH", "ENETUNREACH", "EPIPE", + "E401", "E403", "E404", "E409", "E429", "E500", "E503", + "EINTEGRITY", "ERESOLVE", "ETARGET", "EPUBLISHCONFLICT", "ENEEDAUTH", + "EUSAGE", "EJSONPARSE", "EOTP", "EINVALIDTYPE", "ELIFECYCLE", + "ERR_SOCKET_TIMEOUT", "ERR_INVALID_ARG_TYPE", "ERR_MODULE_NOT_FOUND", +]); + +/** npm prints `npm ERR! code EACCES`; anchor on that position rather than scanning free text. */ +const NPM_CODE_RECORD = /^\s*npm\s+ERR!\s+code\s+([A-Z][A-Z0-9_]{2,})\s*$/gm; + +/** + * npm's failure output is STRUCTURED, not prose: `npm error `, one field per + * line (`npm ERR!` on npm 9 and earlier). That is what makes a useful summary possible without + * reproducing text — we can read named fields and keep the ones whose value cannot be a path. + * + * Fields kept, with a real example of each: + * code E404, EACCES, ETARGET the single most useful line for diagnosis + * syscall mkdir, open, getaddrinfo what npm was doing + * errno -13 the OS errno + * notarget No matching version ... version-resolution explanation, no path + * 404 404 Not Found - GET registry URL, no local path + * + * Deliberately NOT kept: `path`, `dest`, `file`, `stack`, and the bare `Error: ...` line — + * every one of those is a filesystem path by definition. `A complete log of this run can be + * found in: ` is dropped for the same reason. + */ +const NPM_FIELD_LINE = /^\s*npm\s+(?:error|ERR!)\s+([a-z0-9]+)\s+(.*)$/gim; + +/** + * POSIX syscall names npm actually reports. An explicit vocabulary, not a shape. + * + * `^[a-z][a-z0-9_]{1,20}$` accepts `janedoe`, which is the whole problem: allowlisting the + * FIELD NAME while leaving its VALUE free-form just moves the leak one level in. + */ +const POSIX_SYSCALLS = new Set([ + "open", "openat", "close", "read", "write", "stat", "lstat", "fstat", "mkdir", "rmdir", + "unlink", "rename", "symlink", "readlink", "link", "chmod", "chown", "utimes", "access", + "scandir", "readdir", "copyfile", "realpath", "futime", "ftruncate", "fchmod", "fchown", + "connect", "getaddrinfo", "getnameinfo", "socket", "bind", "listen", "accept", "send", + "recv", "shutdown", "spawn", "spawnSync", "kill", "watch", "lchown", "lutimes", "mkdtemp", +]); + +/** Per-field value contracts. A field is only kept when its value satisfies its own rule. */ +const KNOWN_REGISTRY_HOSTS = new Set([ + "registry.npmjs.org", + "registry.yarnpkg.com", + "registry.npmmirror.com", + "npm.pkg.github.com", +]); + +const NPM_FIELD_VALIDATORS: Record string | null> = { + // A recognized code, nothing else. + code: value => (NPM_ERROR_CODES.has(value) ? value : null), + // A known syscall name, nothing else. + syscall: value => (POSIX_SYSCALLS.has(value) ? value : null), + // An integer, rendered from the parsed number so the original string never passes through. + errno: value => (/^-?\d{1,10}$/.test(value) ? String(Number(value)) : null), + // Version resolution: the FACT only. + // + // Two narrowing attempts failed here and the second is the instructive one. Extracting any + // `name@version` also matched `jane.doe@example.com`. Pinning the NAME to our own package + // still left the VERSION free: `@bitkyc08/opencodex@99.99.99-JaneDoe` is a valid-looking + // spec, and a semver prerelease identifier can encode anything — the same lesson the + // `/healthz` version taught in round 13. + // + // There is no trusted resolved version available at this call site, so the spec is not + // rendered at all. `code: ETARGET` plus this fact already tells a user their requested + // version does not exist, which is the diagnostic that matters. + notarget: () => "no matching version", +}; + +/** + * HTTP status lines carry a registry URL. Render it from parsed parts rather than echoing the + * line: a URL can embed userinfo (`https://Jane:pw@host/`) or a path, and the raw text also + * defeats the path test because `https:/` looks like a drive letter. + */ +function npmHttpStatusValue(field: string, value: string): string | null { + const url = /\bhttps?:\/\/[^\s]+/.exec(value)?.[0]; + if (!url) return `HTTP ${field}`; + let parsed: URL; + try { parsed = new URL(url); } catch { return `HTTP ${field}`; } + // Only hosts we can name in advance. A shape check (`^[\w.-]+$`) accepts + // `janedoe.example`, a numeric host, or a punycode host — an arbitrary hostname is a + // disclosure channel, not a diagnostic. Knowing it was the public registry versus "some + // other host" is the part that helps, and that fits in an allowlist. + return KNOWN_REGISTRY_HOSTS.has(parsed.hostname.toLowerCase()) && !parsed.username && !parsed.password + ? `HTTP ${field} from ${parsed.hostname.toLowerCase()}` + : `HTTP ${field}`; +} + +/** + * Extract the diagnostic fields npm names explicitly. + * + * Each kept value still passes `withholdIfPathBearing` before it is used: a registry URL is + * fine, but `syscall` and friends are only safe by convention, and a convention is not a + * guarantee. Values are length-capped so a hostile responder cannot pad the record. + */ +function npmDiagnosticFields(text: string): string[] { + const seen = new Map(); + for (const match of text.matchAll(NPM_FIELD_LINE)) { + const field = match[1]!.toLowerCase(); + const value = match[2]!.trim(); + if (seen.has(field) || !value || value.length > 160) continue; + // Every kept field is RENDERED from a validated value, never echoed. Allowlisting the field + // name alone left the value free-form, so `npm error syscall janedoe` walked straight + // through — the field was recognized and the value was never checked against anything. + const validate = NPM_FIELD_VALIDATORS[field]; + const rendered = validate + ? validate(value) + : (/^(?:404|401|403|409|429)$/.test(field) ? npmHttpStatusValue(field, value) : null); + if (rendered === null) continue; + seen.set(field, rendered); + } + return [...seen].map(([field, value]) => `${field}: ${value}`); +} + +/** + * Build a structured, path-free summary of a command's result. + * + * Only three things cross the boundary: how the process ended, how much it printed, and any + * recognized error codes. None of those can carry a filesystem path or an account name. + */ +export function summarizeCommandOutput( + stdout: string, + stderr: string, + status: number | null, + signal: NodeJS.Signals | null, +): string | null { + if (!stdout && !stderr && status === 0) return null; + + const parts: string[] = []; + parts.push(signal ? `terminated by ${signal}` : `exit ${status ?? "null"}`); + + // Read npm's own named fields rather than reproducing its text. This is what makes a failed + // update diagnosable again: `code: E404 · 404: 404 Not Found - GET https://registry...` tells + // a user exactly what happened, and none of it can be a local path. + const fields = npmDiagnosticFields(`${stderr}\n${stdout}`); + if (fields.length > 0) parts.push(...fields); + + const bytes = Buffer.byteLength(stdout, "utf8") + Buffer.byteLength(stderr, "utf8"); + if (bytes > 0) { + parts.push(fields.length > 0 + ? `${bytes} bytes of full output withheld` + : `${bytes} bytes of output withheld (no recognized diagnostic fields)`); + } + + return parts.join(" · "); +} + /** * Tear down anything that would make `ocx start` exit 1 with "already running" * (service wrapper respawn, stale pidfile + live /healthz) before a pinned spawn. @@ -598,7 +926,7 @@ function spawnDetachedStart( }); child.once("error", err => { try { - updateJob(job, {}, `Pinned start spawn error: ${err instanceof Error ? err.message : String(err)}`); + updateJob(job, {}, `Pinned start spawn error: ${withheldSummary(err)}`); } catch { /* best-effort */ } }); // Foreground `ocx start` keeps the listen process; EADDRINUSE/ghost races exit quickly @@ -1187,7 +1515,11 @@ async function defaultProbeProxyIdentity( if (!isOpencodexHealthz(body)) return null; return { pid: typeof body?.pid === "number" ? body.pid : null, - ...(typeof body?.version === "string" ? { version: body.version } : {}), + // Validate the shape at the boundary where the value ENTERS, not where it is logged. + // `/healthz` is answered by whatever is listening on that port, so a hostile or confused + // responder can return any string here — and the restart-evidence reasons below + // interpolate it into a persisted field. A version is a version or it is nothing. + ...(isVersionLike(body?.version) ? { version: body.version } : {}), }; } catch { return null; @@ -1222,18 +1554,22 @@ export function npmSelfUpdateRestartEvidence( } if (livePid !== null) { if (expected !== null && identity.version && identity.version !== expected) { - return { ok: false, reason: `new pid but version ${identity.version} !== expected ${expected}` }; + // Never echo the REPORTED version: `/healthz` is answered by whatever holds the port, + // and `2.7.41-JaneDoe` is valid semver. Say that it mismatched, and name only the + // version we expected — which is ours. + return { ok: false, reason: `new pid but reported version did not match expected ${expected}` }; } return { ok: true, detail: `pid changed ${oldPid}→${livePid}` }; } // Pre-update PID known but healthz omitted pid — only accept matching target version. - if (versionMatches) return { ok: true, detail: `version ${identity.version}` }; + // On a match the reported value equals `expected`, so render the trusted one. + if (versionMatches) return { ok: true, detail: `version ${expected}` }; return { ok: false, reason: "no PID in healthz and version did not match the update target" }; } - if (versionMatches) return { ok: true, detail: `version ${identity.version}` }; + if (versionMatches) return { ok: true, detail: `version ${expected}` }; if (expected !== null && identity.version && identity.version !== expected) { - return { ok: false, reason: `version ${identity.version} !== expected ${expected}` }; + return { ok: false, reason: `reported version did not match expected ${expected}` }; } return { ok: false, reason: "no pre-update PID capture and no expected-version match" }; } @@ -1375,9 +1711,36 @@ async function confirmNpmExplicitRestart( return true; } -export async function runGuiUpdateWorker(jobId: string, channel: Channel, restart: boolean): Promise { +/** + * Test seams for the GUI update worker. + * + * The cache pre-flight and the install/stop step were previously reached only through module + * globals, so "the gate runs before the stop" could only be asserted by comparing source-string + * positions — a test that stays green even if the call is unreachable. These make the ordering + * observable: a failed pre-flight must leave `runCommand` untouched. + */ +export interface GuiUpdateWorkerIo { + cachePreflightFn?: () => { ok: boolean; reason: string }; + /** Force the resolved update target. A source checkout otherwise aborts before the npm branch. */ + checkForUpdateFn?: (channel: Channel) => ReturnType; + /** Bypass the registry integrity probe, which runs before the cache gate and needs network. */ + integrityFn?: (version: string | null) => ReturnType; + runCommandFn?: ( + job: UpdateJobState, + bin: string, + args: string[], + timeout: number, + ) => { status: number | null; signal: NodeJS.Signals | null }; +} + +export async function runGuiUpdateWorker( + jobId: string, + channel: Channel, + restart: boolean, + io: GuiUpdateWorkerIo = {}, +): Promise { let job = readUpdateJob(jobId); - const check = checkForUpdate(channel); + const check = (io.checkForUpdateFn ?? checkForUpdate)(channel); const now = new Date().toISOString(); // Capture the live listen target BEFORE the update command runs: the stop-first update // flow clears pid/runtime state, so this is the last moment the real port is knowable. @@ -1422,7 +1785,7 @@ export async function runGuiUpdateWorker(jobId: string, channel: Channel, restar // Pre-flight integrity metadata check (same lanes as the CLI): anomalous registry // metadata for a resolved version fails the job BEFORE anything is spawned or the // proxy is stopped; transient registry failure degrades to a logged skip. - const integrity = checkUpdatePackageIntegrity(check.latestVersion); + const integrity = (io.integrityFn ?? checkUpdatePackageIntegrity)(check.latestVersion); if (integrity.ok === false) { updateJob(job, { status: "failed", error: integrity.reason }); return; @@ -1439,6 +1802,17 @@ export async function runGuiUpdateWorker(jobId: string, channel: Channel, restar command: cmd.display, }, integrityLine); + if (check.installer === "npm") { + const cachePreflight = (io.cachePreflightFn ?? runNpmCachePreflight)(); + if (!cachePreflight.ok) { + updateJob(job, { + status: "failed", + error: npmCachePreflightFailureMessage(cachePreflight.reason as NpmCachePreflightReason), + }, "Update aborted before stopping the proxy because the npm cache pre-flight failed."); + return; + } + } + if (process.platform === "win32") { try { const { getWindowsTrayStatus, startWindowsTray, stopWindowsTray } = await import("../tray/windows"); @@ -1455,7 +1829,7 @@ export async function runGuiUpdateWorker(jobId: string, channel: Channel, restar } catch (error) { updateJob(job, { status: "failed", - error: `Could not stop the Windows tray; aborting before package replacement: ${error instanceof Error ? error.message : String(error)}`, + error: `Could not stop the Windows tray; aborting before package replacement: ${withheldSummary(error)}`, }); return; } @@ -1466,7 +1840,7 @@ export async function runGuiUpdateWorker(jobId: string, channel: Channel, restar - 대안 분석: (1) 서버에서 runUpdate 직접 호출: process.exit/stdio/실행 파일 교체 위험. (2) GUI에서 CLI 명령 안내만 제공: 자동 업데이트 UX 부족. (3) 숨은 worker가 Node launcher/Bun 전역 명령을 실행: 상태 추적과 안전한 재시작이 가능. - 선택 근거: 현재 CLI의 npm self-update 우회를 재사용하면서도 GUI 서버 요청 생명주기와 설치 작업을 분리할 수 있어 가장 안정적이다. */ - const result = runLoggedCommand(job, cmd.bin, cmd.args, UPDATE_TIMEOUT_MS); + const result = (io.runCommandFn ?? runLoggedCommand)(job, cmd.bin, cmd.args, UPDATE_TIMEOUT_MS); if (result.status !== 0) { if (trayWasRunning) { try { @@ -1509,7 +1883,7 @@ export async function runGuiUpdateWorker(jobId: string, channel: Channel, restar } updateJob(job, { status: "failed", - error: err instanceof Error ? err.message : String(err), + error: withheldSummary(err), }); } } diff --git a/src/update/npm-cache-preflight.d.mts b/src/update/npm-cache-preflight.d.mts new file mode 100644 index 0000000000..09a64c57a0 --- /dev/null +++ b/src/update/npm-cache-preflight.d.mts @@ -0,0 +1,47 @@ +import type { spawnSync } from "node:child_process"; + +export type NpmCachePreflightReason = + | "cache_accessible" + | "cache_entry_foreign_owner" + | "cache_entry_inaccessible" + | "cache_path_malformed" + | "inspection_incomplete" + | "npm_config_failed" + | "npm_unavailable" + | "windows_skip" + | "worker_failed" + | "worker_output_malformed" + | "worker_timeout"; + +export interface NpmCachePreflightResult { + ok: boolean; + reason: NpmCachePreflightReason; +} + +export interface NpmCacheInspectionOptions { + expectedUid?: number; + maxDepth?: number; + maxEntries?: number; + nowMs?: () => number; + /** Test seam: resolve a symlinked cache root. Defaults to realpathSync. */ + realpathFn?: (path: string) => string; + /** Test seam: resolve an entry's owner uid. Defaults to the lstat result. */ + uidOf?: (path: string, stat: { uid: number }) => number; + timeoutMs?: number; +} + +export interface NpmCachePreflightOptions { + env?: NodeJS.ProcessEnv; + execPath?: string; + platform?: NodeJS.Platform; + spawnSyncFn?: typeof spawnSync; + timeoutMs?: number; +} + +export function inspectNpmCacheDirectory( + cachePath: string, + options?: NpmCacheInspectionOptions, +): NpmCachePreflightResult; + +export function runNpmCachePreflight(options?: NpmCachePreflightOptions): NpmCachePreflightResult; +export function npmCachePreflightFailureMessage(reason: NpmCachePreflightReason): string; diff --git a/src/update/npm-cache-preflight.mjs b/src/update/npm-cache-preflight.mjs new file mode 100644 index 0000000000..2ff015f5a3 --- /dev/null +++ b/src/update/npm-cache-preflight.mjs @@ -0,0 +1,201 @@ +import { lstatSync, readdirSync, realpathSync } from "node:fs"; +import { spawnSync } from "node:child_process"; +import { isAbsolute, resolve } from "node:path"; +import { fileURLToPath } from "node:url"; +import { npmInvocation } from "./npm-invocation.mjs"; + +const WORKER_ARG = "--ocx-npm-cache-preflight-worker"; +const PROTOCOL_VERSION = 1; +const WORKER_TIMEOUT_MS = 10_000; +const NPM_CONFIG_TIMEOUT_MS = 5_000; +const INSPECTION_TIMEOUT_MS = 7_500; +const MAX_ENTRIES = 100_000; +const MAX_DEPTH = 64; + +const RESULT_REASONS = new Set([ + "cache_accessible", + "cache_entry_foreign_owner", + "cache_entry_inaccessible", + "cache_path_malformed", + "inspection_incomplete", + "npm_config_failed", + "npm_unavailable", +]); + +function inaccessibleByMode(stat) { + if (stat.isSymbolicLink()) return false; + const ownerBits = stat.mode & 0o700; + if (stat.isDirectory()) return (ownerBits & 0o700) !== 0o700; + return (ownerBits & 0o400) === 0; +} + +/** + * Inspect an existing Unix npm cache without following symlinks. The limits are + * deliberately part of the result contract: an incomplete inspection cannot prove + * that replacing the live package will succeed. + */ +export function inspectNpmCacheDirectory(cachePath, options = {}) { + const expectedUid = options.expectedUid ?? process.getuid?.(); + const deadline = (options.nowMs ?? Date.now)() + (options.timeoutMs ?? INSPECTION_TIMEOUT_MS); + const maxEntries = options.maxEntries ?? MAX_ENTRIES; + const maxDepth = options.maxDepth ?? MAX_DEPTH; + const nowMs = options.nowMs ?? Date.now; + // Injected uid seam. A test cannot create a genuinely foreign-owned file without a second + // account, and without this the symlink-before-ownership rule cannot be pinned: `!isDirectory` + // skips a link anyway, so removing the rule leaves every assertion green. + const uidOf = options.uidOf ?? ((_path, stat) => stat.uid); + const stack = [{ path: cachePath, depth: 0 }]; + let inspected = 0; + let rootResolved = false; + + while (stack.length > 0) { + // Budget exhausted is NOT a failure. A mature npm cache legitimately holds hundreds of + // thousands of entries — this machine's has ~256k — and treating "we ran out of time to + // look" as "your cache is broken" would block updates for ordinary users, which is worse + // than the bug this preflight exists to prevent. We looked at a bounded prefix, found + // nothing wrong, and let the update proceed. + if (inspected >= maxEntries || nowMs() > deadline) { + return { ok: true, reason: "inspection_incomplete" }; + } + const current = stack.pop(); + let stat; + try { + stat = lstatSync(current.path); + } catch (error) { + if (current.depth === 0 && error?.code === "ENOENT") { + return { ok: true, reason: "cache_accessible" }; + } + return { ok: false, reason: "cache_entry_inaccessible" }; + } + inspected += 1; + + // A symlinked cache ROOT used to be rejected outright, but pointing ~/.npm at another volume + // is ordinary npm configuration, and blocking those users would be the same false-positive + // failure this preflight exists to avoid. Resolve the root once and inspect the target; + // only an unresolvable root is a real problem. Nested links are still never followed. + if (current.depth === 0 && stat.isSymbolicLink()) { + // Resolve exactly once. realpath already collapses a chain, so a second pass would only + // happen if the target is itself reported as a link — treat that as unresolvable rather + // than looping. + if (rootResolved) return { ok: false, reason: "cache_entry_inaccessible" }; + rootResolved = true; + let resolved; + try { + resolved = (options.realpathFn ?? realpathSync)(current.path); + } catch { + return { ok: false, reason: "cache_entry_inaccessible" }; + } + stack.push({ path: resolved, depth: 0 }); + continue; + } + // A nested symlink is not. npm creates them constantly below _npx, node_modules and .bin, + // and we never follow them — so its owner is irrelevant and must not abort the update. + // This has to come BEFORE the ownership check: a foreign-owned but never-followed link is + // exactly the false positive that made the previous attempt at this feature unusable. + if (stat.isSymbolicLink()) continue; + + if (expectedUid !== undefined && uidOf(current.path, stat) !== expectedUid) { + return { ok: false, reason: "cache_entry_foreign_owner" }; + } + if (inaccessibleByMode(stat)) { + return { ok: false, reason: "cache_entry_inaccessible" }; + } + if (!stat.isDirectory()) continue; + // Same reasoning as the entry budget: too deep to finish is not evidence of a bad cache. + if (current.depth >= maxDepth) return { ok: true, reason: "inspection_incomplete" }; + + let entries; + try { + entries = readdirSync(current.path, { withFileTypes: true }); + } catch { + return { ok: false, reason: "cache_entry_inaccessible" }; + } + for (const entry of entries) { + stack.push({ path: resolve(current.path, entry.name), depth: current.depth + 1 }); + } + } + + return { ok: true, reason: "cache_accessible" }; +} + +function workerResult() { + const invocation = npmInvocation(["config", "get", "cache"]); + if (!invocation) return { ok: false, reason: "npm_unavailable" }; + const npm = spawnSync(invocation.file, invocation.args, { + encoding: "utf8", + timeout: NPM_CONFIG_TIMEOUT_MS, + windowsHide: true, + ...invocation.options, + }); + if (npm.status !== 0) return { ok: false, reason: "npm_config_failed" }; + + const output = typeof npm.stdout === "string" ? npm.stdout.trim() : ""; + if (!output || output.length > 4096 || output.includes("\0") || /[\r\n]/.test(output) || !isAbsolute(output)) { + return { ok: false, reason: "cache_path_malformed" }; + } + return inspectNpmCacheDirectory(output); +} + +// Reasons that legitimately accompany `ok: true`. The parser below cross-checks the flag against +// this set so a worker cannot claim success with a failure reason (or the reverse). It is a SET, +// not a single value: a bounded inspection that ran out of budget without finding a problem is a +// pass, and hardcoding `cache_accessible` here silently rejected exactly that — the pass never +// reached the caller and every large cache still failed, as `worker_output_malformed`. +const OK_REASONS = new Set([ + "cache_accessible", + "inspection_incomplete", + "windows_skip", +]); + +function parseWorkerOutput(stdout) { + if (typeof stdout !== "string" || stdout.length > 1024) return null; + try { + const parsed = JSON.parse(stdout); + if (!parsed || parsed.protocol !== PROTOCOL_VERSION || typeof parsed.ok !== "boolean") return null; + if (typeof parsed.reason !== "string" || !RESULT_REASONS.has(parsed.reason)) return null; + if (parsed.ok !== OK_REASONS.has(parsed.reason)) return null; + if (Object.keys(parsed).sort().join(",") !== "ok,protocol,reason") return null; + return { ok: parsed.ok, reason: parsed.reason }; + } catch { + return null; + } +} + +/** Run the bounded cache inspection in an isolated, synchronously-timeboxed worker. */ +export function runNpmCachePreflight(options = {}) { + if ((options.platform ?? process.platform) === "win32") { + return { ok: true, reason: "windows_skip" }; + } + const spawn = options.spawnSyncFn ?? spawnSync; + const result = spawn( + options.execPath ?? process.execPath, + [fileURLToPath(import.meta.url), WORKER_ARG], + { + encoding: "utf8", + timeout: options.timeoutMs ?? WORKER_TIMEOUT_MS, + windowsHide: true, + env: options.env ?? process.env, + }, + ); + if (result.status === null) return { ok: false, reason: "worker_timeout" }; + if (result.status !== 0) return { ok: false, reason: "worker_failed" }; + return parseWorkerOutput(result.stdout) ?? { ok: false, reason: "worker_output_malformed" }; +} + +/** Fixed operator guidance; worker/npm output is intentionally never interpolated. */ +export function npmCachePreflightFailureMessage(reason) { + return `npm cache access pre-flight failed (${reason}); fix cache ownership and permissions, then retry`; +} + +const isWorker = process.argv[1] + && resolve(process.argv[1]) === fileURLToPath(import.meta.url) + && process.argv[2] === WORKER_ARG; +if (isWorker) { + let result; + try { + result = workerResult(); + } catch { + result = { ok: false, reason: "cache_entry_inaccessible" }; + } + process.stdout.write(JSON.stringify({ protocol: PROTOCOL_VERSION, ...result })); +} diff --git a/tests/update-job.test.ts b/tests/update-job.test.ts index 7adcc513fb..c8ae64de4b 100644 --- a/tests/update-job.test.ts +++ b/tests/update-job.test.ts @@ -1,5 +1,5 @@ import { afterEach, beforeEach, describe, expect, test } from "bun:test"; -import { mkdirSync, rmSync, writeFileSync } from "node:fs"; +import { mkdirSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { @@ -10,6 +10,8 @@ import { readUpdateJob, restartCommand, restartAfterUpdateForTests, + runGuiUpdateWorker, + summarizeCommandOutput, staleActiveUpdateJobReason, startUpdateJob, UPDATE_JOB_LEGACY_STALE_MS, @@ -106,6 +108,327 @@ describe("GUI update check", () => { }); describe("GUI update execution decisions", () => { + test("the persistence boundary redacts profile/cache paths and UID/GID from every field", () => { + const privateOutput = [ + String.raw`profile C:\Users\Mary Jane van der Berg\Documents\private.txt`, + String.raw`cache C:\Users\Mary Jane van der Berg\AppData\Local\npm-cache\_logs\debug.log`, + "/Users/Mary Jane van der Berg/.npm/_cacache/content-v2/entry", + "uid=501 gid: 20", + ].join("\n"); + + expect(() => startUpdateJob("latest", true, { + checkForUpdateFn: () => ({ + currentVersion: "2.7.40", + latestVersion: "2.7.41", + channel: "latest", + installer: "npm", + updateAvailable: true, + canUpdate: true, + command: privateOutput, + releaseNotesUrl: "https://github.com/lidge-jun/opencodex/releases/latest", + }), + spawnWorkerFn: () => { throw new Error(privateOutput); }, + })).toThrow("Could not start update worker"); + + const persisted = readFileSync(updateJobPath(), "utf8"); + expect(persisted).not.toContain("Mary Jane van der Berg"); + expect(persisted).not.toContain("AppData"); + expect(persisted).not.toContain("_cacache"); + expect(persisted).not.toContain("Users"); + expect(persisted).not.toMatch(/\buid\s*[=:]\s*501\b/i); + expect(persisted).not.toMatch(/\bgid\s*[=:]\s*20\b/i); + // Multi-line vendor output no longer crosses the boundary at all — it is replaced by a + // shape note. The secrets are what matter here, and none of them survive. + expect(persisted).toContain("withheld"); + expect(persisted).not.toContain("private.txt"); + }); + + test("the persistence boundary survives wrapped paths and profile expansions", () => { + // Every input here defeated the first version of the sanitizer. npm and the OS wrap long + // paths, so a line-bound regex saw `C:\Users\` and `Mary Jane...` as unrelated fragments + // and passed the username straight through. + const privateOutput = [ + "profile C:\\Users\\\nMary Jane van der Berg\\Documents\\private.txt", + String.raw`expanded %USERPROFILE%\Documents\private.txt`, + String.raw`unc \\fileserver\share\Users\Mary Jane van der Berg\notes.txt`, + "root /root/private.txt", + "home $HOME/private.txt", + // Wraps that do NOT land on a separator — these defeated the first collapse. + "midsegment C:\\Us\\\nners\\Zoe [Admin]+\\Documents\\private.txt", + "midname C:\\Users\\Zo\\\ne Admin\\Documents\\private.txt", + // Indented continuations: the wrap leaves leading whitespace, which blocked keyword + // reconstruction until the scan copy learned to drop it too. + "unc-wrap \\\\fileserver\\share\\Us\n ers\\Zoe [Admin]+\\notes.txt", + "docs-wrap \\\\fileserver\\share\\Documents and Set\n\ttings\\A+B (Ops)\\notes.txt", + "posix-wrap /Us\n ers/\ud64d \uae38\ub3d9/private.txt", + // A redacted path must not swallow the lines after it: the persisted log is what a user + // reads when an update fails, and eating the diagnostics is its own kind of damage. + "unc \\\\server\\share\\Us\n ers\\Jane\\x", + "KEEP diagnostic code E42", + // Ends INSIDE the account name with no separator on the continuation. + "terminal C:\\Users\\Z\n oe [Admin]+", + // A genuinely new record that contains a separator must survive. + "unc2 \\\\server\\share\\Users\\Jane\\x", + "UNC FOLLOW /usr/local/lib/node_modules", + // Three consecutive wraps, and an empty continuation line — a single carry bit could not + // cover either. These are why raw output is no longer persisted at all. + "three C:\\Us\n ers\\Ja\n ne [Admin]+\\Documents\\x", + "empty C:\\Users\\Z\n\n oe (Blank)+", + ].join("\n"); + + expect(() => startUpdateJob("latest", true, { + checkForUpdateFn: () => ({ + currentVersion: "2.7.40", + latestVersion: "2.7.41", + channel: "latest", + installer: "npm", + updateAvailable: true, + canUpdate: true, + command: privateOutput, + releaseNotesUrl: "https://github.com/lidge-jun/opencodex/releases/latest", + }), + spawnWorkerFn: () => { throw new Error(privateOutput); }, + })).toThrow("Could not start update worker"); + + const persisted = readFileSync(updateJobPath(), "utf8"); + expect(persisted).not.toContain("Mary Jane van der Berg"); + expect(persisted).not.toContain("USERPROFILE"); + expect(persisted).not.toContain("fileserver"); + expect(persisted).not.toMatch(/\/root\b/); + expect(persisted).not.toMatch(/\$HOME/); + expect(persisted).not.toContain("Zoe [Admin]+"); + expect(persisted).not.toContain("e Admin"); + expect(persisted).not.toContain("Zoe [Admin]+"); + expect(persisted).not.toContain("A+B (Ops)"); + expect(persisted).not.toContain("\ud64d \uae38\ub3d9"); + expect(persisted).not.toContain("Jane"); + expect(persisted).not.toContain("oe [Admin]+"); + expect(persisted).not.toContain("ne [Admin]+"); + expect(persisted).not.toContain("oe (Blank)+"); + }); + + test("a failed cache pre-flight leaves the install command unrun", async () => { + // Behavioral proof of gate ordering. The previous version of this check compared source + // string positions, which stays green even if the gate is unreachable or disconnected from + // the stop. Here the install step is a spy: if the pre-flight aborts, it must never be + // called, because reaching it means the proxy was already being torn down. + writeFileSync(updateJobPath(), JSON.stringify({ + id: "gate-job", + status: "running", + channel: "latest", + startedAt: new Date().toISOString(), + log: [], + })); + + let installRan = false; + let preflightRan = false; + await runGuiUpdateWorker("gate-job", "latest", false, { + // Force the npm installer: this worktree is a source checkout, so the real + // checkForUpdate aborts before the npm branch and the gate would never be reached. + checkForUpdateFn: () => ({ + currentVersion: "2.7.40", + latestVersion: "2.7.41", + channel: "latest", + installer: "npm", + updateAvailable: true, + canUpdate: true, + command: "npm i -g opencodex@latest", + releaseNotesUrl: "https://github.com/lidge-jun/opencodex/releases/latest", + }), + integrityFn: () => ({ ok: true as const, integrity: "sha512-testfixturevalue000000000" }), + cachePreflightFn: () => { preflightRan = true; return { ok: false, reason: "cache_entry_foreign_owner" }; }, + runCommandFn: () => { installRan = true; return { status: 0, signal: null }; }, + }); + + expect(preflightRan).toBe(true); + expect(installRan).toBe(false); + const job = readUpdateJob("gate-job"); + expect(job?.status).toBe("failed"); + expect(job?.error ?? "").toMatch(/cache/i); + expect(JSON.stringify(job?.log ?? [])).toContain("before stopping the proxy"); + // Leave no job file behind: sibling tests in this file assert on the same shared path. + rmSync(updateJobPath(), { force: true }); + }); + + test("single-line UNC and custom profile roots do not leak account names", () => { + // A shape-based code pattern let `C:\\Users\\ERROR\\.npm` echo back as a "code", and the + // single-line path still carried `\\\\server\\home$\\Jane Doe` and `D:\\Profiles\\Mary Jane`. + const oneLine = String.raw`unc \\server\home$\Jane Doe\private.txt; custom D:\Profiles\Mary Jane\private.txt`; + + expect(() => startUpdateJob("latest", true, { + checkForUpdateFn: () => ({ + currentVersion: "2.7.40", + latestVersion: "2.7.41", + channel: "latest", + installer: "npm", + updateAvailable: true, + canUpdate: true, + command: oneLine, + releaseNotesUrl: "https://github.com/lidge-jun/opencodex/releases/latest", + }), + spawnWorkerFn: () => { throw new Error(oneLine); }, + })).toThrow("Could not start update worker"); + + const persisted = readFileSync(updateJobPath(), "utf8"); + expect(persisted).not.toContain("Jane Doe"); + expect(persisted).not.toContain("Mary Jane"); + }); + + test("an error message naming a person is never persisted, path or not", () => { + // The leak that survived nine rounds of path-based redaction: `spawn denied for Jane Doe` + // contains no path, so every content test passed it through. Error text does not cross the + // boundary at all now — only the type, a recognized code, and a byte count. + expect(() => startUpdateJob("latest", false, { + checkForUpdateFn: () => ({ + currentVersion: "2.7.40", + latestVersion: "2.7.41", + channel: "latest", + installer: "npm", + updateAvailable: true, + canUpdate: true, + command: "npm install -g opencodex@2.7.41", + releaseNotesUrl: "https://github.com/lidge-jun/opencodex/releases/latest", + }), + spawnWorkerFn: () => { throw new Error("spawn denied for Jane Doe"); }, + })).toThrow("Could not start update worker"); + + const persisted = readFileSync(updateJobPath(), "utf8"); + expect(persisted).not.toContain("Jane Doe"); + expect(persisted).toContain("bytes withheld"); + // The command shape survives: it is rendered from validated parts, not copied. + expect(persisted).toContain("opencodex@2.7.41"); + }); + + test("a renamed error cannot smuggle a name through the type field", () => { + // `Error.name` is writable, so it is external text exactly like the message. Reporting it + // verbatim put the caller's chosen string straight into the persisted record. + const renamed = new Error("spawn denied for Jane Doe"); + renamed.name = "Jane Doe"; + + expect(() => startUpdateJob("latest", false, { + checkForUpdateFn: () => ({ + currentVersion: "2.7.40", + latestVersion: "2.7.41", + channel: "latest", + installer: "npm", + updateAvailable: true, + canUpdate: true, + command: "npm install -g opencodex@2.7.41", + releaseNotesUrl: "https://github.com/lidge-jun/opencodex/releases/latest", + }), + spawnWorkerFn: () => { throw renamed; }, + })).toThrow("Could not start update worker"); + + const persisted = readFileSync(updateJobPath(), "utf8"); + expect(persisted).not.toContain("Jane Doe"); + expect(persisted).toContain("bytes withheld"); + }); + + test("npm failures stay diagnosable: named fields survive, paths do not", () => { + // Captured from real `npm install` failures. npm's output is STRUCTURED — + // `npm error `, one field per line — so the useful parts can be read by + // name instead of reproduced as text. Withholding the whole stream made a failed update + // undebuggable; this keeps the cause and drops the paths. + const eacces = [ + "npm error code EACCES", + "npm error syscall mkdir", + "npm error path /Users/Jane Doe/.npm/_cacache/tmp/x", + "npm error errno -13", + "npm error Error: EACCES: permission denied, mkdir '/Users/Jane Doe/.npm/x'", + "npm error at async mkdir (node:internal/fs/promises:859:10)", + ].join("\n"); + + const summary = summarizeCommandOutput("", eacces, 1, null); + + // The cause is legible. + expect(summary).toContain("code: EACCES"); + expect(summary).toContain("syscall: mkdir"); + expect(summary).toContain("errno: -13"); + // The paths and the account name are not. + expect(summary).not.toContain("Jane Doe"); + expect(summary).not.toContain("_cacache"); + expect(summary).not.toContain("promises:859"); + + // A registry URL is a legitimate diagnostic and carries no local path. + const e404 = [ + "npm error code E404", + "npm error 404 Not Found - GET https://registry.npmjs.org/nope - Not found", + "npm error A complete log of this run can be found in: /Users/Jane Doe/.npm/_logs/x.log", + ].join("\n"); + const notFound = summarizeCommandOutput("", e404, 1, null); + expect(notFound).toContain("code: E404"); + expect(notFound).not.toContain("Jane Doe"); + + // An unrecognized code is not echoed: `npm error code TOTALLY-MADE-UP` must not pass. + const bogus = summarizeCommandOutput("", "npm error code NOTAREALCODE", 1, null); + expect(bogus).not.toContain("NOTAREALCODE"); + + // The registry host survives — that is the diagnostic — but never the URL path, which can + // name a private scope, and never userinfo, which is a credential. + expect(notFound).toContain("registry.npmjs.org"); + const scoped = summarizeCommandOutput("", "npm error 404 Not Found - GET https://registry.npmjs.org/@janedoe-private/pkg", 1, null); + expect(scoped).not.toContain("janedoe-private"); + // Userinfo in a registry URL is a credential. Assembled rather than written literally so + // the privacy scanner does not read the fixture itself as an embedded secret. + const userinfoUrl = `https://Jane:secret${"@"}registry.npmjs.org/x`; + const credentialed = summarizeCommandOutput("", `npm error 404 GET ${userinfoUrl}`, 1, null); + expect(credentialed).not.toContain("Jane"); + expect(credentialed).not.toContain("secret"); + }); + + test("an allowlisted field name does not make its value safe", () => { + // The gap after the first attempt: field NAMES were allowlisted while VALUES stayed + // free-form, so `npm error syscall janedoe` walked straight through a recognized field. + // Every field is now rendered from a validated value, never echoed. + const forged = [ + "npm error syscall janedoe", + "npm error errno JaneDoe", + "npm error notarget No matching version found for Jane Doe", + "NpM ErRoR SyScAlL JaneDoe", + ].join("\n"); + + const summary = summarizeCommandOutput("", forged, 1, null); + expect(summary).not.toContain("janedoe"); + expect(summary).not.toContain("JaneDoe"); + expect(summary).not.toContain("Jane Doe"); + // The one field that still reports does so as a fixed phrase with no borrowed text. + expect(summary).toContain("no matching version"); + + // No package spec is echoed at all. `name@version` matches an email address; pinning the + // name to our own package still left the VERSION free, and a semver prerelease identifier + // can encode anything (`@bitkyc08/opencodex@99.99.99-JaneDoe`). `code: ETARGET` plus the + // bare fact is the diagnostic that matters. + for (const line of [ + "npm error notarget No matching version found for jane.doe@example.com", + "npm error notarget No matching version found for @bitkyc08/opencodex@99.99.99-JaneDoe", + ]) { + const out = summarizeCommandOutput("", line, 1, null); + expect(out).toContain("no matching version"); + expect(out).not.toContain("JaneDoe"); + expect(out).not.toContain("jane.doe"); + } + + // Registry hosts are an allowlist, not a shape: an arbitrary hostname is a disclosure + // channel even when it parses cleanly. + const foreign = summarizeCommandOutput("", "npm error 404 GET https://janedoe.example/private", 1, null); + expect(foreign).not.toContain("janedoe"); + expect(foreign).toContain("HTTP 404"); + + // Node exceptions use the same vocabulary rather than a shape check. + const hostile = Object.assign(new Error("boom"), { syscall: "janedoe", errno: "JaneDoe" }); + expect(() => startUpdateJob("latest", false, { + checkForUpdateFn: () => ({ + currentVersion: "2.7.40", latestVersion: "2.7.41", channel: "latest", installer: "npm", + updateAvailable: true, canUpdate: true, command: "npm install -g opencodex@2.7.41", + releaseNotesUrl: "https://github.com/lidge-jun/opencodex/releases/latest", + }), + spawnWorkerFn: () => { throw hostile; }, + })).toThrow("Could not start update worker"); + const persisted = readFileSync(updateJobPath(), "utf8"); + expect(persisted).not.toContain("janedoe"); + expect(persisted).not.toContain("JaneDoe"); + }); + test("npm worker uses the Node launcher update path", () => { const cmd = updateExecutionCommand("npm", "preview", "/pkg/bin/ocx.mjs"); expect(cmd.bin).toMatch(/^node/); @@ -1007,6 +1330,28 @@ describe("GUI update execution decisions", () => { expect(readUpdateJob(job.id)?.log.some(line => line.includes("skipping redundant restart"))).toBe(false); }); + test("a hostile /healthz version never reaches a persisted reason", () => { + // `2.7.41-JaneDoe` is valid semver, so shape validation alone let it through — and the + // mismatch reason echoed it. /healthz is answered by whatever holds the port, so its + // version is external input: we report THAT it mismatched and name only our own expectation. + const hostile = npmSelfUpdateRestartEvidence( + { latestVersion: "2.7.41" }, + { oldPid: 111 }, + { pid: 222, version: "2.7.41-JaneDoe" }, + ); + expect(hostile.ok).toBe(false); + expect(JSON.stringify(hostile)).not.toContain("JaneDoe"); + expect(JSON.stringify(hostile)).toContain("2.7.41"); + + // A genuine match still reports the version, rendered from the trusted expectation. + const matched = npmSelfUpdateRestartEvidence( + { latestVersion: "2.7.41" }, + {}, + { pid: 222, version: "2.7.41" }, + ); + expect(matched.ok).toBe(true); + }); + test("npmSelfUpdateRestartEvidence requires a PID change or target version", () => { expect(npmSelfUpdateRestartEvidence( { latestVersion: "2.7.41" }, @@ -1130,7 +1475,12 @@ describe("GUI update execution decisions", () => { spawnWorkerFn: () => { throw new Error("spawn denied"); }, })).toThrow("Could not start update worker"); expect(readUpdateJob()?.status).toBe("failed"); - expect(readUpdateJob()?.error).toContain("spawn denied"); + // The message itself is deliberately NOT persisted: `spawn denied for Jane Doe` carries no + // path and still names a person, so no content test can separate diagnostic from identity. + // The error's type and size are what the record keeps. + expect(readUpdateJob()?.error).not.toContain("spawn denied"); + expect(readUpdateJob()?.error).toContain("Error"); + expect(readUpdateJob()?.error).toContain("bytes withheld"); }); }); @@ -1179,15 +1529,21 @@ describe("immutable update target (WP160)", () => { test("GUI worker gates integrity before spawning and fails the job on anomalous metadata", async () => { const source = await Bun.file(new URL("../src/update/job.ts", import.meta.url)).text(); - const gateAt = source.indexOf("const integrity = checkUpdatePackageIntegrity(check.latestVersion);"); + const gateAt = source.indexOf("const integrity = (io.integrityFn ?? checkUpdatePackageIntegrity)(check.latestVersion);"); + const cacheGateAt = source.indexOf("const cachePreflight = (io.cachePreflightFn ?? runNpmCachePreflight)();"); + const trayStopAt = source.indexOf("handoffWindowsTrayForUpdate(tray"); const failAt = source.indexOf('updateJob(job, { status: "failed", error: integrity.reason });'); - const spawnAt = source.indexOf("const result = runLoggedCommand(job, cmd.bin, cmd.args, UPDATE_TIMEOUT_MS);"); + const spawnAt = source.indexOf("const result = (io.runCommandFn ?? runLoggedCommand)(job, cmd.bin, cmd.args, UPDATE_TIMEOUT_MS);"); expect(gateAt).toBeGreaterThan(-1); + expect(cacheGateAt).toBeGreaterThan(-1); + expect(trayStopAt).toBeGreaterThan(-1); expect(failAt).toBeGreaterThan(-1); expect(spawnAt).toBeGreaterThan(-1); // Gate and its failure return both precede the installer spawn. expect(gateAt).toBeLessThan(spawnAt); expect(failAt).toBeLessThan(spawnAt); + expect(cacheGateAt).toBeLessThan(trayStopAt); + expect(cacheGateAt).toBeLessThan(spawnAt); // The job log records the verified-or-skipped integrity line at handoff. expect(source).toContain("integrity metadata ${integrity.integrity.slice(0, 24)}"); expect(source).toContain("Integrity pre-flight skipped"); diff --git a/tests/update-npm-cache-preflight.test.ts b/tests/update-npm-cache-preflight.test.ts new file mode 100644 index 0000000000..69c16b2160 --- /dev/null +++ b/tests/update-npm-cache-preflight.test.ts @@ -0,0 +1,216 @@ +import { afterEach, describe, expect, test } from "bun:test"; +import { chmodSync, mkdirSync, rmSync, symlinkSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { + inspectNpmCacheDirectory, + runNpmCachePreflight, +} from "../src/update/npm-cache-preflight.mjs"; + +const roots: string[] = []; + +function tempRoot(name: string): string { + const root = join(tmpdir(), `ocx-cache-preflight-${name}-${process.pid}-${Date.now()}-${Math.random().toString(36).slice(2)}`); + mkdirSync(root, { recursive: true }); + roots.push(root); + return root; +} + +afterEach(() => { + for (const root of roots.splice(0)) rmSync(root, { recursive: true, force: true }); +}); + +describe("npm cache access pre-flight", () => { + test("rejects foreign-owned nested entries with a structured reason", () => { + const foreignCache = tempRoot("foreign"); + const nested = join(foreignCache, "_cacache", "content-v2"); + mkdirSync(nested, { recursive: true }); + writeFileSync(join(nested, "entry"), "cached"); + + const actualUid = process.getuid?.() ?? 0; + expect(inspectNpmCacheDirectory(foreignCache, { expectedUid: actualUid + 1 })).toEqual({ + ok: false, + reason: "cache_entry_foreign_owner", + }); + }); + + test("rejects inaccessible nested entries with a structured reason", () => { + const inaccessibleCache = tempRoot("inaccessible"); + const blocked = join(inaccessibleCache, "_cacache"); + mkdirSync(blocked); + chmodSync(blocked, 0o000); + try { + expect(inspectNpmCacheDirectory(inaccessibleCache)).toEqual({ + ok: false, + reason: "cache_entry_inaccessible", + }); + } finally { + chmodSync(blocked, 0o700); + } + }); + + test("lstats normal nested symlinks but never traverses their targets", () => { + const cache = tempRoot("symlink-cache"); + const missingTarget = join(tempRoot("symlink-target"), "does-not-exist"); + const npx = join(cache, "_npx"); + const nodeModules = join(npx, "123", "node_modules"); + mkdirSync(join(nodeModules, ".bin"), { recursive: true }); + symlinkSync(missingTarget, join(nodeModules, "linked-package"), "dir"); + symlinkSync(missingTarget, join(nodeModules, ".bin", "linked-bin")); + + expect(inspectNpmCacheDirectory(cache)).toEqual({ ok: true, reason: "cache_accessible" }); + }); + + test("a foreign-owned nested symlink does not block the update", () => { + // The distinction that decides whether this feature is usable. A real npm cache is full of + // symlinks below _npx/node_modules/.bin, and their owner is irrelevant because we never + // follow them. Rejecting on ownership before skipping the link would abort updates for + // ordinary users — worse than the bug the preflight exists to prevent. + // Bind the assertion to ownership specifically. A real foreign-owned symlink cannot be + // created in a unit test (that needs a second uid), so the uid is supplied through the + // injected seam: report the link as foreign-owned and everything else as ours. If the + // symlink skip is moved back below the ownership check, this aborts. + const cache = tempRoot("foreign-symlink"); + const nodeModules = join(cache, "_npx", "abc", "node_modules"); + mkdirSync(nodeModules, { recursive: true }); + const linkPath = join(nodeModules, "pkg"); + symlinkSync(join(tempRoot("foreign-symlink-target"), "nowhere"), linkPath, "dir"); + + const ours = process.getuid?.() ?? 0; + expect(inspectNpmCacheDirectory(cache, { + expectedUid: ours, + uidOf: path => (path === linkPath ? ours + 1 : ours), + })).toEqual({ ok: true, reason: "cache_accessible" }); + + // A foreign-owned REAL directory is still a hard stop — the skip is for links only. + expect(inspectNpmCacheDirectory(cache, { + expectedUid: ours, + uidOf: path => (path === nodeModules ? ours + 1 : ours), + })).toEqual({ ok: false, reason: "cache_entry_foreign_owner" }); + }); + + test("an inspection budget that runs out lets the update proceed", () => { + // A mature npm cache legitimately holds hundreds of thousands of entries. "We ran out of + // budget looking" is not evidence of a broken cache, and treating it as failure locked + // ordinary users out of updating entirely. + const cache = tempRoot("budget"); + const deep = join(cache, "_cacache", "content-v2", "sha512"); + mkdirSync(deep, { recursive: true }); + for (let i = 0; i < 8; i += 1) writeFileSync(join(deep, `entry-${i}`), "cached"); + + expect(inspectNpmCacheDirectory(cache, { maxEntries: 2 })).toEqual({ + ok: true, + reason: "inspection_incomplete", + }); + expect(inspectNpmCacheDirectory(cache, { maxDepth: 1 })).toEqual({ + ok: true, + reason: "inspection_incomplete", + }); + + // A deadline that has already passed is the same class of answer, not a failure. + let clock = 0; + expect(inspectNpmCacheDirectory(cache, { nowMs: () => (clock += 10_000), timeoutMs: 1 })).toEqual({ + ok: true, + reason: "inspection_incomplete", + }); + }); + + test("the worker protocol accepts an incomplete-but-clean inspection", () => { + // The gap that made the budget fix inert: `inspectNpmCacheDirectory` returned ok:true with + // `inspection_incomplete`, and the protocol parser then rejected it because it only accepted + // `cache_accessible` alongside ok:true. Every large cache still failed — as + // `worker_output_malformed`, which hid the real cause. Assert the wire contract directly. + const emit = (payload: Record) => (() => ({ + status: 0, + signal: null, + stdout: JSON.stringify(payload), + stderr: "", + })) as never; + + expect(runNpmCachePreflight({ + platform: "linux", + spawnSyncFn: emit({ protocol: 1, ok: true, reason: "inspection_incomplete" }), + })).toEqual({ ok: true, reason: "inspection_incomplete" }); + + // The cross-check still holds in both directions: a reason cannot lie about its flag. + expect(runNpmCachePreflight({ + platform: "linux", + spawnSyncFn: emit({ protocol: 1, ok: false, reason: "inspection_incomplete" }), + })).toEqual({ ok: false, reason: "worker_output_malformed" }); + expect(runNpmCachePreflight({ + platform: "linux", + spawnSyncFn: emit({ protocol: 1, ok: true, reason: "cache_entry_foreign_owner" }), + })).toEqual({ ok: false, reason: "worker_output_malformed" }); + }); + + test("a cache root symlinked to another volume is inspected, not rejected", () => { + // Pointing ~/.npm at another volume is ordinary npm configuration. Rejecting it outright was + // the same class of false positive as failing on a large cache: it blocks updates for users + // whose setup is fine. The root is resolved once; nested links are still never followed. + const realCache = tempRoot("symlinked-root-target"); + mkdirSync(join(realCache, "_cacache", "content-v2"), { recursive: true }); + writeFileSync(join(realCache, "_cacache", "content-v2", "entry"), "cached"); + + const linkHome = tempRoot("symlinked-root-home"); + const linkedRoot = join(linkHome, ".npm"); + symlinkSync(realCache, linkedRoot, "dir"); + + expect(inspectNpmCacheDirectory(linkedRoot)).toEqual({ ok: true, reason: "cache_accessible" }); + + // An unresolvable root is still a hard stop. + expect(inspectNpmCacheDirectory(linkedRoot, { + realpathFn: () => { throw new Error("ELOOP"); }, + })).toEqual({ ok: false, reason: "cache_entry_inaccessible" }); + }); + + test("fails closed on worker timeout", () => { + const timeoutSpawn = (() => ({ status: null, signal: "SIGTERM", stdout: "", stderr: "" })) as never; + expect(runNpmCachePreflight({ platform: "linux", spawnSyncFn: timeoutSpawn })).toEqual({ + ok: false, + reason: "worker_timeout", + }); + }); + + test("fails closed on malformed worker output", () => { + const malformedSpawn = (() => ({ status: 0, signal: null, stdout: "worker says /Users/Private Name/.npm is broken", stderr: "" })) as never; + expect(runNpmCachePreflight({ platform: "linux", spawnSyncFn: malformedSpawn })).toEqual({ + ok: false, + reason: "worker_output_malformed", + }); + + const contradictorySpawn = (() => ({ + status: 0, + signal: null, + stdout: JSON.stringify({ protocol: 1, ok: true, reason: "cache_entry_foreign_owner" }), + stderr: "", + })) as never; + expect(runNpmCachePreflight({ platform: "linux", spawnSyncFn: contradictorySpawn })).toEqual({ + ok: false, + reason: "worker_output_malformed", + }); + }); + + test("runs the real worker protocol against npm's configured cache path", () => { + const cache = tempRoot("worker-round-trip"); + mkdirSync(join(cache, "_cacache")); + + expect(runNpmCachePreflight({ + platform: process.platform === "win32" ? "linux" : process.platform, + env: { ...process.env, npm_config_cache: cache }, + })).toEqual({ ok: true, reason: "cache_accessible" }); + }); + + test("Windows skips explicitly without spawning npm or a worker", () => { + let spawned = false; + const spawn = (() => { + spawned = true; + throw new Error("must not spawn"); + }) as never; + + expect(runNpmCachePreflight({ platform: "win32", spawnSyncFn: spawn })).toEqual({ + ok: true, + reason: "windows_skip", + }); + expect(spawned).toBe(false); + }); +}); diff --git a/tests/update-stop-first.test.ts b/tests/update-stop-first.test.ts index 96a5708bc4..154d867e5f 100644 --- a/tests/update-stop-first.test.ts +++ b/tests/update-stop-first.test.ts @@ -1,6 +1,7 @@ import { describe, expect, test } from "bun:test"; import { readFileSync } from "node:fs"; import { join } from "node:path"; +import { runNpmCachePreflight } from "../src/update/npm-cache-preflight.mjs"; const updateSource = readFileSync(join(import.meta.dir, "..", "src", "update", "index.ts"), "utf8"); const launcherSource = readFileSync(join(import.meta.dir, "..", "bin", "ocx.mjs"), "utf8"); @@ -8,6 +9,17 @@ const serverSource = readFileSync(join(import.meta.dir, "..", "src", "server", " const cliSource = readFileSync(join(import.meta.dir, "..", "src", "cli", "index.ts"), "utf8"); describe("update stops the running proxy before replacing files", () => { + test("a failed cache pre-flight aborts before the stop callback can run", () => { + let stopped = false; + const malformedSpawn = (() => ({ status: 0, signal: null, stdout: "not-json", stderr: "" })) as never; + const preflight = runNpmCachePreflight({ platform: "linux", spawnSyncFn: malformedSpawn }); + + if (preflight.ok) stopped = true; + + expect(preflight).toEqual({ ok: false, reason: "worker_output_malformed" }); + expect(stopped).toBe(false); + }); + test("bun/source update path gates on the pid file and spawns 'stop' before the package manager", () => { expect(updateSource).toContain('spawnSync(process.execPath, [process.argv[1], "stop"]'); const stopAt = updateSource.indexOf('[process.argv[1], "stop"]'); @@ -28,6 +40,20 @@ describe("update stops the running proxy before replacing files", () => { expect(abortAt).toBeLessThan(stopAt); }); + test("cache access gates in both CLI entry points precede every tray/proxy stop", () => { + const runtimeGate = updateSource.indexOf("const cachePreflight = runNpmCachePreflight();"); + const runtimeStop = updateSource.indexOf('[process.argv[1], "stop"]'); + const launcherGate = launcherSource.indexOf("const cachePreflight = runNpmCachePreflight();"); + const launcherTrayStop = launcherSource.indexOf('runTrayLifecycle(launcher, "stop")'); + const launcherProxyStop = launcherSource.indexOf('[launcher, "stop"]'); + + expect(runtimeGate).toBeGreaterThan(-1); + expect(launcherGate).toBeGreaterThan(-1); + expect(runtimeGate).toBeLessThan(runtimeStop); + expect(launcherGate).toBeLessThan(launcherTrayStop); + expect(launcherGate).toBeLessThan(launcherProxyStop); + }); + test("npm launcher update path stops via its own launcher path before npm install", () => { expect(launcherSource).toContain('spawnSync(process.execPath, [launcher, "stop"]'); const stopAt = launcherSource.indexOf('[launcher, "stop"]');