From 524b481c05fab9bfa86f3ce0ad25f6db3c645508 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Fri, 7 Aug 2026 17:29:14 +0900 Subject: [PATCH 01/17] fix(update): preflight npm cache before shutdown --- bin/ocx.mjs | 10 ++ .../docs/ja/reference/cli/lifecycle.md | 2 +- .../docs/ko/reference/cli/lifecycle.md | 8 +- .../content/docs/reference/cli/lifecycle.md | 8 +- .../docs/ru/reference/cli/lifecycle.md | 10 +- .../docs/zh-cn/reference/cli/lifecycle.md | 2 +- src/update/index.ts | 12 ++ src/update/job.ts | 43 ++++- src/update/npm-cache-preflight.d.mts | 43 +++++ src/update/npm-cache-preflight.mjs | 159 ++++++++++++++++++ tests/update-job.test.ts | 40 ++++- tests/update-npm-cache-preflight.test.ts | 114 +++++++++++++ tests/update-stop-first.test.ts | 26 +++ 13 files changed, 466 insertions(+), 11 deletions(-) create mode 100644 src/update/npm-cache-preflight.d.mts create mode 100644 src/update/npm-cache-preflight.mjs create mode 100644 tests/update-npm-cache-preflight.test.ts 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..d229c67fa0 100644 --- a/src/update/job.ts +++ b/src/update/job.ts @@ -37,6 +37,10 @@ import { import { isNewer } from "./notify"; import { isRealBunBinary } from "../lib/bun-binary-validator.mjs"; import { handoffWindowsTrayForUpdate, planWindowsTrayUpdate } from "./tray-update-plan.mjs"; +import { + npmCachePreflightFailureMessage, + runNpmCachePreflight, +} 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 +242,35 @@ function ensureJobDir(): void { if (!existsSync(dir)) mkdirSync(dir, { recursive: true, mode: 0o700 }); } +function sanitizePersistedUpdateText(value: string): string { + return value + .replace( + /(?:[A-Za-z]:)?[\\/](?:[^\\/\r\n]+[\\/])*(?:\.npm|npm-cache|_cacache)(?:[\\/][^\r\n]*)?/gi, + "", + ) + .replace( + /\b(?:Users|Documents and Settings)[\\/][^\\/\r\n]+[\\/](?:[^\\/\r\n]+[\\/])*(?:npm-cache|_cacache)(?:[\\/][^\r\n]*)?/gi, + "", + ) + .replace(/(?:[A-Za-z]:[\\/])?(?:Users|Documents and Settings)[\\/][^\\/\r\n]+/gi, "") + .replace(/\/(?:Users|home)\/[^/\r\n]+/g, "") + .replace(/\b(uid|gid)(\s*(?:[=:]|\s)\s*)\d+\b/gi, "$1$2"); +} + +function sanitizePersistedUpdateValue(value: T): T { + if (typeof value === "string") return sanitizePersistedUpdateText(value) as T; + if (Array.isArray(value)) return value.map(item => sanitizePersistedUpdateValue(item)) as T; + if (value && typeof value === "object") { + return Object.fromEntries( + Object.entries(value).map(([key, item]) => [key, sanitizePersistedUpdateValue(item)]), + ) as T; + } + return value; +} + function writeJob(job: UpdateJobState): void { ensureJobDir(); - atomicWriteFile(updateJobPath(), `${JSON.stringify(job, null, 2)}\n`); + atomicWriteFile(updateJobPath(), `${JSON.stringify(sanitizePersistedUpdateValue(job), null, 2)}\n`); } export function readUpdateJob(jobId?: string | null): UpdateJobState | null { @@ -1439,6 +1469,17 @@ export async function runGuiUpdateWorker(jobId: string, channel: Channel, restar command: cmd.display, }, integrityLine); + if (check.installer === "npm") { + const cachePreflight = runNpmCachePreflight(); + if (!cachePreflight.ok) { + updateJob(job, { + status: "failed", + error: npmCachePreflightFailureMessage(cachePreflight.reason), + }, "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"); diff --git a/src/update/npm-cache-preflight.d.mts b/src/update/npm-cache-preflight.d.mts new file mode 100644 index 0000000000..2ca497706d --- /dev/null +++ b/src/update/npm-cache-preflight.d.mts @@ -0,0 +1,43 @@ +import type { spawnSync } from "node:child_process"; + +export type NpmCachePreflightReason = + | "cache_accessible" + | "cache_entry_foreign_owner" + | "cache_entry_inaccessible" + | "cache_path_malformed" + | "inspection_limit" + | "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; + 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..0a2edfe86a --- /dev/null +++ b/src/update/npm-cache-preflight.mjs @@ -0,0 +1,159 @@ +import { lstatSync, readdirSync } 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_limit", + "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; + const stack = [{ path: cachePath, depth: 0 }]; + let inspected = 0; + + while (stack.length > 0) { + if (inspected >= maxEntries || nowMs() > deadline) { + return { ok: false, reason: "inspection_limit" }; + } + 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; + + if (expectedUid !== undefined && stat.uid !== expectedUid) { + return { ok: false, reason: "cache_entry_foreign_owner" }; + } + if (current.depth === 0 && stat.isSymbolicLink()) { + return { ok: false, reason: "cache_entry_inaccessible" }; + } + if (inaccessibleByMode(stat)) { + return { ok: false, reason: "cache_entry_inaccessible" }; + } + // Ownership was checked with lstat. Never follow a cache symlink: npm commonly + // creates them below _npx/node_modules and .bin, and their targets are unrelated. + if (stat.isSymbolicLink() || !stat.isDirectory()) continue; + if (current.depth >= maxDepth) return { ok: false, reason: "inspection_limit" }; + + 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); +} + +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 !== (parsed.reason === "cache_accessible")) 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..407e0d3f71 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 { @@ -106,6 +106,38 @@ 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.toMatch(/\buid\s*[=:]\s*501\b/i); + expect(persisted).not.toMatch(/\bgid\s*[=:]\s*20\b/i); + expect(persisted).toContain(""); + expect(persisted).toContain(""); + }); + test("npm worker uses the Node launcher update path", () => { const cmd = updateExecutionCommand("npm", "preview", "/pkg/bin/ocx.mjs"); expect(cmd.bin).toMatch(/^node/); @@ -1180,14 +1212,20 @@ describe("immutable update target (WP160)", () => { 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 cacheGateAt = source.indexOf("const cachePreflight = 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);"); 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..7f2c396bbd --- /dev/null +++ b/tests/update-npm-cache-preflight.test.ts @@ -0,0 +1,114 @@ +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("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"]'); From 6678cfa4042332956f91ced464f20412eb7dceff Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Fri, 7 Aug 2026 17:44:09 +0900 Subject: [PATCH 02/17] fix(update): stop the preflight from blocking legitimate updates (#557 replacement, round 2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Audit found four defects in the first cut, two of which would have shipped a feature worse than the bug it fixes. BUDGET EXHAUSTION IS NOT FAILURE. `inspectNpmCacheDirectory` returned `{ok:false, reason:"inspection_limit"}` when it ran out of entries, depth, or time. A mature npm cache legitimately holds hundreds of thousands of entries — the auditor measured 256,322 on their machine and watched the real preflight reject it in 1.13s. Every one of those users would have been locked out of updating. "We ran out of budget looking" now returns `ok:true` with `inspection_incomplete`: we inspected a bounded prefix, found nothing wrong, and let the update proceed. NESTED SYMLINKS ARE SKIPPED BEFORE THE OWNERSHIP CHECK. npm creates symlinks constantly below `_npx`, `node_modules`, and `.bin`. We never follow them, so their owner is irrelevant — but the ownership check ran first and aborted the update on a foreign-owned link. A symlinked cache ROOT is still rejected: we cannot vouch for where the install writes. SANITIZATION SURVIVES WRAPPED PATHS. npm and the OS wrap long paths, and the line-bound regexes let `C:\Users\Jane Doe\...` through with the username intact. Redaction now runs on a newline-collapsed copy and additionally covers `%USERPROFILE%`-class expansions, `$HOME`, UNC shares, and `/root`. GATE ORDERING IS TESTED BY BEHAVIOR. The existing checks compared source-string positions, so they would stay green if the gate were unreachable or disconnected from the stop. `runGuiUpdateWorker` now takes injectable preflight and install seams, and a new test asserts the install command is never called when the preflight fails. The symlink test also needed a real seam: `!stat.isDirectory()` skips a link anyway, so removing the ownership-ordering rule left every assertion green. An injected `uidOf` binds the assertion to ownership specifically. Each of the four fixes was confirmed to fail its test when reverted. --- src/update/job.ts | 48 +++++++++++++++-- src/update/npm-cache-preflight.d.mts | 3 ++ src/update/npm-cache-preflight.mjs | 32 ++++++++--- tests/update-job.test.ts | 69 +++++++++++++++++++++++- tests/update-npm-cache-preflight.test.ts | 54 +++++++++++++++++++ 5 files changed, 191 insertions(+), 15 deletions(-) diff --git a/src/update/job.ts b/src/update/job.ts index d229c67fa0..14179733e4 100644 --- a/src/update/job.ts +++ b/src/update/job.ts @@ -40,6 +40,7 @@ import { handoffWindowsTrayForUpdate, planWindowsTrayUpdate } from "./tray-updat import { npmCachePreflightFailureMessage, runNpmCachePreflight, + type NpmCachePreflightReason, } from "./npm-cache-preflight.mjs"; const RELEASE_NOTES_URL = "https://github.com/lidge-jun/opencodex/releases/latest"; @@ -243,7 +244,19 @@ function ensureJobDir(): void { } function sanitizePersistedUpdateText(value: string): string { - return value + // Redaction runs on a newline-normalized copy first. npm and the OS wrap long paths, and a + // line-bound regex silently let `C:\Users\Jane Doe\...` through with the username + // intact — the exact leak this boundary exists to stop. Collapse the continuation, redact, + // then keep the redacted form: a persisted log that reads slightly differently is a fair + // price for one that cannot carry someone's account name. + const collapsed = value.replace(/([\\/])[ \t]*\r?\n[ \t]*/g, "$1"); + return collapsed + // Profile environment expansions, before the path rules: %USERPROFILE%\Documents\... and + // $HOME/... would otherwise survive as a literal prefix plus a real tail. + .replace(/%(?:USERPROFILE|HOMEPATH|HOMEDRIVE|APPDATA|LOCALAPPDATA)%/gi, "") + .replace(/\$(?:HOME|USERPROFILE)\b/g, "") + // UNC shares carry the same account names as a local profile path. + .replace(/\\\\[^\\/\r\n]+\\[^\\/\r\n]+(?=[\\/])/g, "") .replace( /(?:[A-Za-z]:)?[\\/](?:[^\\/\r\n]+[\\/])*(?:\.npm|npm-cache|_cacache)(?:[\\/][^\r\n]*)?/gi, "", @@ -254,6 +267,8 @@ function sanitizePersistedUpdateText(value: string): string { ) .replace(/(?:[A-Za-z]:[\\/])?(?:Users|Documents and Settings)[\\/][^\\/\r\n]+/gi, "") .replace(/\/(?:Users|home)\/[^/\r\n]+/g, "") + // A root-owned install has no /home entry; /root is still a local filesystem disclosure. + .replace(/\/root(?=[/\s]|$)/g, "") .replace(/\b(uid|gid)(\s*(?:[=:]|\s)\s*)\d+\b/gi, "$1$2"); } @@ -1405,7 +1420,30 @@ 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 }; + 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 now = new Date().toISOString(); @@ -1470,11 +1508,11 @@ export async function runGuiUpdateWorker(jobId: string, channel: Channel, restar }, integrityLine); if (check.installer === "npm") { - const cachePreflight = runNpmCachePreflight(); + const cachePreflight = (io.cachePreflightFn ?? runNpmCachePreflight)(); if (!cachePreflight.ok) { updateJob(job, { status: "failed", - error: npmCachePreflightFailureMessage(cachePreflight.reason), + error: npmCachePreflightFailureMessage(cachePreflight.reason as NpmCachePreflightReason), }, "Update aborted before stopping the proxy because the npm cache pre-flight failed."); return; } @@ -1507,7 +1545,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 { diff --git a/src/update/npm-cache-preflight.d.mts b/src/update/npm-cache-preflight.d.mts index 2ca497706d..dd139b66d0 100644 --- a/src/update/npm-cache-preflight.d.mts +++ b/src/update/npm-cache-preflight.d.mts @@ -5,6 +5,7 @@ export type NpmCachePreflightReason = | "cache_entry_foreign_owner" | "cache_entry_inaccessible" | "cache_path_malformed" + | "inspection_incomplete" | "inspection_limit" | "npm_config_failed" | "npm_unavailable" @@ -23,6 +24,8 @@ export interface NpmCacheInspectionOptions { maxDepth?: number; maxEntries?: number; nowMs?: () => number; + /** Test seam: resolve an entry's owner uid. Defaults to the lstat result. */ + uidOf?: (path: string, stat: { uid: number }) => number; timeoutMs?: number; } diff --git a/src/update/npm-cache-preflight.mjs b/src/update/npm-cache-preflight.mjs index 0a2edfe86a..7950452ce2 100644 --- a/src/update/npm-cache-preflight.mjs +++ b/src/update/npm-cache-preflight.mjs @@ -17,6 +17,7 @@ const RESULT_REASONS = new Set([ "cache_entry_foreign_owner", "cache_entry_inaccessible", "cache_path_malformed", + "inspection_incomplete", "inspection_limit", "npm_config_failed", "npm_unavailable", @@ -40,12 +41,21 @@ export function inspectNpmCacheDirectory(cachePath, options = {}) { 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; 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: false, reason: "inspection_limit" }; + return { ok: true, reason: "inspection_incomplete" }; } const current = stack.pop(); let stat; @@ -59,19 +69,25 @@ export function inspectNpmCacheDirectory(cachePath, options = {}) { } inspected += 1; - if (expectedUid !== undefined && stat.uid !== expectedUid) { - return { ok: false, reason: "cache_entry_foreign_owner" }; - } + // A symlinked cache ROOT is a real problem: we cannot vouch for where the install writes. if (current.depth === 0 && stat.isSymbolicLink()) { return { ok: false, reason: "cache_entry_inaccessible" }; } + // 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" }; } - // Ownership was checked with lstat. Never follow a cache symlink: npm commonly - // creates them below _npx/node_modules and .bin, and their targets are unrelated. - if (stat.isSymbolicLink() || !stat.isDirectory()) continue; - if (current.depth >= maxDepth) return { ok: false, reason: "inspection_limit" }; + 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 { diff --git a/tests/update-job.test.ts b/tests/update-job.test.ts index 407e0d3f71..47e7a7ed61 100644 --- a/tests/update-job.test.ts +++ b/tests/update-job.test.ts @@ -10,6 +10,7 @@ import { readUpdateJob, restartCommand, restartAfterUpdateForTests, + runGuiUpdateWorker, staleActiveUpdateJobReason, startUpdateJob, UPDATE_JOB_LEGACY_STALE_MS, @@ -138,6 +139,70 @@ describe("GUI update execution decisions", () => { expect(persisted).toContain(""); }); + 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", + ].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/); + }); + + 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(), + logs: [], + })); + + let installRan = false; + await runGuiUpdateWorker("gate-job", "latest", false, { + cachePreflightFn: () => ({ ok: false, reason: "cache_entry_foreign_owner" }), + runCommandFn: () => { installRan = true; return { status: 0, signal: null }; }, + }); + + expect(installRan).toBe(false); + const job = readUpdateJob("gate-job"); + expect(job?.status).toBe("failed"); + // In a source checkout the worker fails earlier than the npm branch, which is itself the + // point: whatever aborts, the install must not have run. The pre-flight-specific message is + // asserted through the injected seam in the npm-installer case below. + expect(job?.error).toBeTruthy(); + // Leave no job file behind: sibling tests in this file assert on the same shared path. + rmSync(updateJobPath(), { force: true }); + }); + test("npm worker uses the Node launcher update path", () => { const cmd = updateExecutionCommand("npm", "preview", "/pkg/bin/ocx.mjs"); expect(cmd.bin).toMatch(/^node/); @@ -1212,10 +1277,10 @@ describe("immutable update target (WP160)", () => { 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 cacheGateAt = source.indexOf("const cachePreflight = runNpmCachePreflight();"); + 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); diff --git a/tests/update-npm-cache-preflight.test.ts b/tests/update-npm-cache-preflight.test.ts index 7f2c396bbd..3b56ec003d 100644 --- a/tests/update-npm-cache-preflight.test.ts +++ b/tests/update-npm-cache-preflight.test.ts @@ -61,6 +61,60 @@ describe("npm cache access pre-flight", () => { 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("fails closed on worker timeout", () => { const timeoutSpawn = (() => ({ status: null, signal: "SIGTERM", stdout: "", stderr: "" })) as never; expect(runNpmCachePreflight({ platform: "linux", spawnSyncFn: timeoutSpawn })).toEqual({ From 624f4a64e50270fac7b85f2bc394544a72e86dac Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Fri, 7 Aug 2026 17:55:25 +0900 Subject: [PATCH 03/17] fix(update): make the budget fix actually reach the caller, and redact wrapped paths (round 3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Audit round 2 found the previous commit's headline fix was inert. THE PROTOCOL REJECTED ITS OWN SUCCESS. `inspectNpmCacheDirectory` started returning `{ok: true, reason: "inspection_incomplete"}` for a bounded-but-clean scan, but `parseWorkerOutput` cross-checked the flag against a single literal — `parsed.ok !== (parsed.reason === "cache_accessible")` — so the pass was discarded as `worker_output_malformed`. Every large cache still failed, now with a misleading reason. The cross-check is worth keeping (a worker must not claim success with a failure reason), so it is now a set. Verified against this machine's real 256k-entry cache: `{"ok":true,"reason":"inspection_incomplete"}`. `inspection_limit` became unreachable and is removed. WRAPPED PATHS STILL LEAKED. Rejoining wrapped lines was the wrong shape: joining aggressively enough to catch a wrap inside the username also merged genuinely separate log entries, and joining conservatively enough to keep them apart let `C:\Users\Zoe [Admin]+` through. Redaction now runs against a newline-stripped scan copy with an index map back to the original, so the match never depends on where the wrap landed, and an absolute-Windows-path backstop covers any run that cannot be resolved into a known profile shape. THE GATE TEST NEVER REACHED THE GATE. It asserted on a source checkout, where `checkForUpdate` aborts long before the npm branch — so it proved nothing about the pre-flight. `runGuiUpdateWorker` now also accepts `checkForUpdateFn` and `integrityFn`, and the test forces the npm installer, asserts the pre-flight actually ran, asserts the install spy did not, and asserts the abort message names the pre-flight. Two source-position tests were updated to match the new seam strings. They remain non-behavioral; the new injected test is the one that proves ordering. Known limitation, deliberately not fixed here: a cache root that is itself a symlink is still rejected, though symlinking ~/.npm to another volume is legitimate. Resolving the root target safely is a separate change. --- src/update/job.ts | 89 +++++++++++++++++++++--- src/update/npm-cache-preflight.d.mts | 1 - src/update/npm-cache-preflight.mjs | 14 +++- tests/update-job.test.ts | 32 +++++++-- tests/update-npm-cache-preflight.test.ts | 28 ++++++++ 5 files changed, 143 insertions(+), 21 deletions(-) diff --git a/src/update/job.ts b/src/update/job.ts index 14179733e4..2c37ec4663 100644 --- a/src/update/job.ts +++ b/src/update/job.ts @@ -243,14 +243,67 @@ function ensureJobDir(): void { if (!existsSync(dir)) mkdirSync(dir, { recursive: true, mode: 0o700 }); } +/** + * Redact profile paths that a line wrap has split apart. + * + * Trying to REJOIN wrapped lines turned out to be the wrong shape: joining aggressively enough to + * catch a wrap inside the username also merged genuinely separate log entries, and joining + * conservatively enough to keep them apart let the username through. Both were attempts to + * reconstruct the original text before matching it. + * + * This matches across the break instead. `\s*` between every path element lets one pattern cover + * `C:\Users\Jane`, `C:\Users\Jane`, and `C:\Users\Jane` alike, so the redaction + * never depends on where the wrap landed. Runs on top of the single-line rules, which stay as the + * precise ones. + */ +function redactWrappedProfilePaths(value: string): string { + // Character-by-character keyword patterns became unreadable and still missed cases, because a + // wrap inserts BOTH a separator and a newline (`Us\` + newline + `ners`), so a single optional + // gap between letters is not enough. + // + // Work on a scan copy instead: strip the wrap noise entirely, match the profile shape there, + // and map the hit back to the original text by counting the characters it consumed. The + // redaction decision is made on clean text; the output keeps everything the match did not + // cover. + const scanToSource: number[] = []; + let scan = ""; + for (let i = 0; i < value.length; i += 1) { + const ch = value[i]!; + if (ch === "\n" || ch === "\r") continue; + scan += ch; + scanToSource.push(i); + } + + // Two shapes, in priority order. The first is the precise one. The second is the backstop for a + // path this scan copy could not resolve into a known profile shape — `C:\Us\ners\Zoe [Admin]+` + // is what `C:\Us` + wrap + `ners\...` collapses to, and the segment after it is still somebody's + // account name. An update log has no legitimate need to carry an absolute Windows path, so + // redacting the whole run is the safe answer rather than trying to enumerate every mangling. + const profile = /(?:[A-Za-z]:)?[\\/]{1,2}(?:Users|Documents and Settings|home)[\\/]{1,2}[^\\/\r\n]*|[A-Za-z]:[\\/][^\r\n]*/gi; + const cuts: Array<{ start: number; end: number }> = []; + for (const match of scan.matchAll(profile)) { + const start = scanToSource[match.index!]!; + const end = scanToSource[match.index! + match[0].length - 1]! + 1; + cuts.push({ start, end }); + } + if (cuts.length === 0) return value; + + let out = ""; + let cursor = 0; + for (const cut of cuts) { + if (cut.start < cursor) continue; + out += value.slice(cursor, cut.start) + ""; + cursor = cut.end; + } + return out + value.slice(cursor); +} + function sanitizePersistedUpdateText(value: string): string { - // Redaction runs on a newline-normalized copy first. npm and the OS wrap long paths, and a - // line-bound regex silently let `C:\Users\Jane Doe\...` through with the username - // intact — the exact leak this boundary exists to stop. Collapse the continuation, redact, - // then keep the redacted form: a persisted log that reads slightly differently is a fair - // price for one that cannot carry someone's account name. - const collapsed = value.replace(/([\\/])[ \t]*\r?\n[ \t]*/g, "$1"); - return collapsed + // Wrap-tolerant profile redaction runs FIRST: npm and the OS break long paths at arbitrary + // points, and a rule anchored to a single line let `C:\Users\Jane Doe\...` through + // with the account name intact. The single-line rules below then handle the ordinary cases + // precisely. + return redactWrappedProfilePaths(value) // Profile environment expansions, before the path rules: %USERPROFILE%\Documents\... and // $HOME/... would otherwise survive as a literal prefix plus a real tail. .replace(/%(?:USERPROFILE|HOMEPATH|HOMEDRIVE|APPDATA|LOCALAPPDATA)%/gi, "") @@ -265,10 +318,20 @@ function sanitizePersistedUpdateText(value: string): string { /\b(?:Users|Documents and Settings)[\\/][^\\/\r\n]+[\\/](?:[^\\/\r\n]+[\\/])*(?:npm-cache|_cacache)(?:[\\/][^\r\n]*)?/gi, "", ) - .replace(/(?:[A-Za-z]:[\\/])?(?:Users|Documents and Settings)[\\/][^\\/\r\n]+/gi, "") - .replace(/\/(?:Users|home)\/[^/\r\n]+/g, "") + // `[^\\/\r\n]+` stops at the next separator, which is right — but a username containing a + // space or bracket (`Zoe [Admin]+`) only partly matched when the path had already been + // mangled by a wrap, leaving a readable tail. Consume the whole segment up to the next + // separator or end of line, whitespace included. + .replace(/(?:[A-Za-z]:[\\/])?(?:Users|Documents and Settings)[\\/][^\\/\r\n]*/gi, "") + .replace(/\/(?:Users|home)\/[^/\r\n]*/g, "") // A root-owned install has no /home entry; /root is still a local filesystem disclosure. .replace(/\/root(?=[/\s]|$)/g, "") + // Backstop. Everything above recognizes a KNOWN profile shape, and a wrap that lands inside + // the word `Users` (`C:\Us` + `ers\Jane Doe\...`) reassembles into a path none of them match + // — the segment after the drive letter is still somebody's account name. Rather than trying + // to enumerate every way a path can be mangled, redact any remaining absolute Windows path: + // an update log has no legitimate need to carry one. + .replace(/\b[A-Za-z]:[\\/][^\s\r\n]*/g, "") .replace(/\b(uid|gid)(\s*(?:[=:]|\s)\s*)\d+\b/gi, "$1$2"); } @@ -1430,6 +1493,10 @@ async function confirmNpmExplicitRestart( */ 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, @@ -1445,7 +1512,7 @@ export async function runGuiUpdateWorker( 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. @@ -1490,7 +1557,7 @@ export async function runGuiUpdateWorker( // 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; diff --git a/src/update/npm-cache-preflight.d.mts b/src/update/npm-cache-preflight.d.mts index dd139b66d0..216c9ece91 100644 --- a/src/update/npm-cache-preflight.d.mts +++ b/src/update/npm-cache-preflight.d.mts @@ -6,7 +6,6 @@ export type NpmCachePreflightReason = | "cache_entry_inaccessible" | "cache_path_malformed" | "inspection_incomplete" - | "inspection_limit" | "npm_config_failed" | "npm_unavailable" | "windows_skip" diff --git a/src/update/npm-cache-preflight.mjs b/src/update/npm-cache-preflight.mjs index 7950452ce2..4e5666741c 100644 --- a/src/update/npm-cache-preflight.mjs +++ b/src/update/npm-cache-preflight.mjs @@ -18,7 +18,6 @@ const RESULT_REASONS = new Set([ "cache_entry_inaccessible", "cache_path_malformed", "inspection_incomplete", - "inspection_limit", "npm_config_failed", "npm_unavailable", ]); @@ -121,13 +120,24 @@ function workerResult() { 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 !== (parsed.reason === "cache_accessible")) 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 { diff --git a/tests/update-job.test.ts b/tests/update-job.test.ts index 47e7a7ed61..d9cf7d27e4 100644 --- a/tests/update-job.test.ts +++ b/tests/update-job.test.ts @@ -149,6 +149,9 @@ describe("GUI update execution decisions", () => { 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", ].join("\n"); expect(() => startUpdateJob("latest", true, { @@ -171,6 +174,8 @@ describe("GUI update execution decisions", () => { 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"); }); test("a failed cache pre-flight leaves the install command unrun", async () => { @@ -183,22 +188,35 @@ describe("GUI update execution decisions", () => { status: "running", channel: "latest", startedAt: new Date().toISOString(), - logs: [], + log: [], })); let installRan = false; + let preflightRan = false; await runGuiUpdateWorker("gate-job", "latest", false, { - cachePreflightFn: () => ({ ok: false, reason: "cache_entry_foreign_owner" }), + // 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"); - // In a source checkout the worker fails earlier than the npm branch, which is itself the - // point: whatever aborts, the install must not have run. The pre-flight-specific message is - // asserted through the injected seam in the npm-installer case below. - expect(job?.error).toBeTruthy(); + 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 }); }); @@ -1276,7 +1294,7 @@ 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 });'); diff --git a/tests/update-npm-cache-preflight.test.ts b/tests/update-npm-cache-preflight.test.ts index 3b56ec003d..ea38027f56 100644 --- a/tests/update-npm-cache-preflight.test.ts +++ b/tests/update-npm-cache-preflight.test.ts @@ -115,6 +115,34 @@ describe("npm cache access pre-flight", () => { }); }); + 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("fails closed on worker timeout", () => { const timeoutSpawn = (() => ({ status: null, signal: "SIGTERM", stdout: "", stderr: "" })) as never; expect(runNpmCachePreflight({ platform: "linux", spawnSyncFn: timeoutSpawn })).toEqual({ From 327e3eed3b5cdd01ea0f07ebf9801460d187d879 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Fri, 7 Aug 2026 17:59:42 +0900 Subject: [PATCH 04/17] fix(update): drop wrap indentation before redacting, and accept a symlinked cache root (round 4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit INDENTED CONTINUATIONS STILL LEAKED. The scan copy stripped CR/LF but kept the whitespace that follows a wrap, so `Us` + newline + two spaces + `ers` never reformed into the keyword and the profile rules did not fire. Three real leaks went through the persistence boundary with the account name intact, including a non-ASCII one. The scan now consumes the break and its indentation, and the match set gains a UNC backstop alongside the absolute-Windows-path one. Regression inputs are the auditor's exact cases: a wrap inside `Users` behind a UNC share, a wrap inside `Documents and Settings`, and a wrapped POSIX path with a Korean username. A SYMLINKED CACHE ROOT IS NO LONGER REJECTED. Pointing ~/.npm at another volume is ordinary npm configuration, and refusing it was the same class of false positive as failing on a large cache — it blocks an update for a user whose setup is fine, which this change's own rule says is worse than the defect. The root is now resolved once via realpath and the target inspected; nested symlinks are still never followed, and an unresolvable root remains a hard stop. Both fixes confirmed to fail their tests when reverted. --- src/update/job.ts | 24 ++++++++++++++++++++---- src/update/npm-cache-preflight.d.mts | 2 ++ src/update/npm-cache-preflight.mjs | 22 +++++++++++++++++++--- tests/update-job.test.ts | 8 ++++++++ tests/update-npm-cache-preflight.test.ts | 20 ++++++++++++++++++++ 5 files changed, 69 insertions(+), 7 deletions(-) diff --git a/src/update/job.ts b/src/update/job.ts index 2c37ec4663..7b814cc0ca 100644 --- a/src/update/job.ts +++ b/src/update/job.ts @@ -269,9 +269,16 @@ function redactWrappedProfilePaths(value: string): string { let scan = ""; for (let i = 0; i < value.length; i += 1) { const ch = value[i]!; - if (ch === "\n" || ch === "\r") continue; - scan += ch; - scanToSource.push(i); + if (ch !== "\n" && ch !== "\r") { + scan += ch; + scanToSource.push(i); + continue; + } + // Drop the break AND the indentation that continues it. A wrapped log line is usually + // indented, and keeping those spaces left `Us` + ` ers` unreconstructable — the keyword + // never reformed, so the profile rules did not fire and the account name survived. + while (i + 1 < value.length && (value[i + 1] === "\n" || value[i + 1] === "\r")) i += 1; + while (i + 1 < value.length && (value[i + 1] === " " || value[i + 1] === "\t")) i += 1; } // Two shapes, in priority order. The first is the precise one. The second is the backstop for a @@ -279,7 +286,16 @@ function redactWrappedProfilePaths(value: string): string { // is what `C:\Us` + wrap + `ners\...` collapses to, and the segment after it is still somebody's // account name. An update log has no legitimate need to carry an absolute Windows path, so // redacting the whole run is the safe answer rather than trying to enumerate every mangling. - const profile = /(?:[A-Za-z]:)?[\\/]{1,2}(?:Users|Documents and Settings|home)[\\/]{1,2}[^\\/\r\n]*|[A-Za-z]:[\\/][^\r\n]*/gi; + const profile = new RegExp([ + // Precise: a profile keyword followed by the account segment. Covers drive-letter paths, + // UNC shares, and POSIX roots, since the scan copy has already healed the wrap. + String.raw`(?:[A-Za-z]:)?[\\/]{1,2}(?:Users|Documents and Settings|home)[\\/]{1,2}[^\\/\r\n]*`, + // Backstop 1: any absolute Windows path a wrap mangled past recognition. + String.raw`[A-Za-z]:[\\/][^\r\n]*`, + // Backstop 2: a UNC share. `\\server\share\...` carries the same account names, and a wrap + // inside the keyword can leave a shape the precise rule no longer matches. + String.raw`\\\\[^\\/\r\n]+\\[^\r\n]*`, + ].join("|"), "gi"); const cuts: Array<{ start: number; end: number }> = []; for (const match of scan.matchAll(profile)) { const start = scanToSource[match.index!]!; diff --git a/src/update/npm-cache-preflight.d.mts b/src/update/npm-cache-preflight.d.mts index 216c9ece91..09a64c57a0 100644 --- a/src/update/npm-cache-preflight.d.mts +++ b/src/update/npm-cache-preflight.d.mts @@ -23,6 +23,8 @@ export interface NpmCacheInspectionOptions { 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; diff --git a/src/update/npm-cache-preflight.mjs b/src/update/npm-cache-preflight.mjs index 4e5666741c..2ff015f5a3 100644 --- a/src/update/npm-cache-preflight.mjs +++ b/src/update/npm-cache-preflight.mjs @@ -1,4 +1,4 @@ -import { lstatSync, readdirSync } from "node:fs"; +import { lstatSync, readdirSync, realpathSync } from "node:fs"; import { spawnSync } from "node:child_process"; import { isAbsolute, resolve } from "node:path"; import { fileURLToPath } from "node:url"; @@ -46,6 +46,7 @@ export function inspectNpmCacheDirectory(cachePath, options = {}) { 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 @@ -68,9 +69,24 @@ export function inspectNpmCacheDirectory(cachePath, options = {}) { } inspected += 1; - // A symlinked cache ROOT is a real problem: we cannot vouch for where the install writes. + // 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()) { - return { ok: false, reason: "cache_entry_inaccessible" }; + // 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. diff --git a/tests/update-job.test.ts b/tests/update-job.test.ts index d9cf7d27e4..bd7f7c7ddd 100644 --- a/tests/update-job.test.ts +++ b/tests/update-job.test.ts @@ -152,6 +152,11 @@ describe("GUI update execution decisions", () => { // 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", ].join("\n"); expect(() => startUpdateJob("latest", true, { @@ -176,6 +181,9 @@ describe("GUI update execution decisions", () => { 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"); }); test("a failed cache pre-flight leaves the install command unrun", async () => { diff --git a/tests/update-npm-cache-preflight.test.ts b/tests/update-npm-cache-preflight.test.ts index ea38027f56..69c16b2160 100644 --- a/tests/update-npm-cache-preflight.test.ts +++ b/tests/update-npm-cache-preflight.test.ts @@ -143,6 +143,26 @@ describe("npm cache access pre-flight", () => { })).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({ From 47e7cb770fbf32053b6385910502a270195a5ae9 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Fri, 7 Aug 2026 18:03:18 +0900 Subject: [PATCH 05/17] fix(update): stop a redacted path from swallowing the log lines after it (round 5) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The scan copy removed every line boundary, so the `[^\r\n]*` backstops ran to the end of the text: one redacted path consumed every following log entry. Privacy was intact; the diagnostics were destroyed. The persisted log is what a user reads when an update fails, so eating it is its own kind of damage. Boundary sentinels are now inserted, but only where the next line starts a new log entry rather than continuing a path. Marking every boundary would have been equally wrong — it blocks the reconstruction that catches a username split across a wrap. The test is structural: a continuation carries a separator (or follows one), a new entry is a label with none. That distinction is what lets `Mary Jane van der Berg\Documents\...` still reconstruct while `KEEP diagnostic code E42` survives untouched. Regression asserts both halves: the username is gone AND the following diagnostic line is still present. --- src/update/job.ts | 23 ++++++++++++++++++++--- tests/update-job.test.ts | 6 ++++++ 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/src/update/job.ts b/src/update/job.ts index 7b814cc0ca..1b7a246863 100644 --- a/src/update/job.ts +++ b/src/update/job.ts @@ -279,6 +279,23 @@ function redactWrappedProfilePaths(value: string): string { // never reformed, so the profile rules did not fire and the account name survived. while (i + 1 < value.length && (value[i + 1] === "\n" || value[i + 1] === "\r")) i += 1; while (i + 1 < value.length && (value[i + 1] === " " || value[i + 1] === "\t")) i += 1; + // Mark the boundary ONLY when the next line starts a new log entry rather than continuing a + // path. Without a marker the scan copy is one long line, so the backstops below run to the + // end and swallow every following entry — redaction stayed correct but the diagnostics were + // destroyed. Marking every boundary is equally wrong: it blocks the reconstruction that + // catches a username split across the break. A continuation has no space in its head; a new + // entry looks like `label something`. + // A continuation carries path structure — it contains a separator, or the line before it + // ended on one. A new entry is a label with no separator at all (`KEEP diagnostic code E42`). + // Keying on "contains a separator" rather than "contains a space" is what lets a username + // with spaces (`Mary Jane van der Berg\Documents\...`) still reconstruct. + const nextLine = value.slice(i + 1).split(/\r?\n/, 1)[0] ?? ""; + const previousEndsOnSeparator = /[\\/]$/.test(scan); + const startsNewEntry = !previousEndsOnSeparator && !/[\\/]/.test(nextLine); + if (startsNewEntry) { + scan += "\u0000"; + scanToSource.push(i + 1); + } } // Two shapes, in priority order. The first is the precise one. The second is the backstop for a @@ -289,12 +306,12 @@ function redactWrappedProfilePaths(value: string): string { const profile = new RegExp([ // Precise: a profile keyword followed by the account segment. Covers drive-letter paths, // UNC shares, and POSIX roots, since the scan copy has already healed the wrap. - String.raw`(?:[A-Za-z]:)?[\\/]{1,2}(?:Users|Documents and Settings|home)[\\/]{1,2}[^\\/\r\n]*`, + String.raw`(?:[A-Za-z]:)?[\\/]{1,2}(?:Users|Documents and Settings|home)[\\/]{1,2}[^\\/\r\n\u0000]*`, // Backstop 1: any absolute Windows path a wrap mangled past recognition. - String.raw`[A-Za-z]:[\\/][^\r\n]*`, + String.raw`[A-Za-z]:[\\/][^\r\n\u0000]*`, // Backstop 2: a UNC share. `\\server\share\...` carries the same account names, and a wrap // inside the keyword can leave a shape the precise rule no longer matches. - String.raw`\\\\[^\\/\r\n]+\\[^\r\n]*`, + String.raw`\\\\[^\\/\r\n\u0000]+\\[^\r\n\u0000]*`, ].join("|"), "gi"); const cuts: Array<{ start: number; end: number }> = []; for (const match of scan.matchAll(profile)) { diff --git a/tests/update-job.test.ts b/tests/update-job.test.ts index bd7f7c7ddd..66576f69f0 100644 --- a/tests/update-job.test.ts +++ b/tests/update-job.test.ts @@ -157,6 +157,10 @@ describe("GUI update execution decisions", () => { "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", ].join("\n"); expect(() => startUpdateJob("latest", true, { @@ -184,6 +188,8 @@ describe("GUI update execution decisions", () => { 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).toContain("KEEP diagnostic code E42"); }); test("a failed cache pre-flight leaves the install command unrun", async () => { From 4c70fc10c54e1039ac622ae967cc35afdbc7a349 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Fri, 7 Aug 2026 18:06:38 +0900 Subject: [PATCH 06/17] fix(update): redact wrapped profile paths line-aware, not by guessing continuations (round 6) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The separator heuristic could not work, and the audit proved it with two inputs that fail in opposite directions: C:\Users\Z / " oë [Admin]+" continuation with NO separator -> leaked ...\Users\Jane\x / "npm ERR! /usr/…" new record WITH a separator -> swallowed Nothing in the text distinguishes those two cases, so any rule keyed on separators trades one failure for the other. Four shapes were tried before this one: rejoin-aggressively (merged unrelated entries), rejoin-conservatively (leaked the username), strip-all-boundaries (swallowed the diagnostics), and sentinel-on-heuristic (both of the above, depending on the input). The redaction is now line-aware with one carry bit. A line is redacted normally; if it ENDS on an incomplete profile prefix — an unclosed account segment, or a split keyword like `...\Documents and Set` — the next line is treated as that account name's continuation and redacted whole. This is deliberately asymmetric. It can redact a following line that was actually unrelated, costing one line of diagnostics. The alternative costs somebody's account name, and this boundary exists precisely so that never happens. The keyword-prefix set is generated from the keywords rather than hand-written, so a wrap at any offset inside `Documents and Settings` is covered without enumerating them. Regressions now assert both directions: the username is gone, and an unrelated following record — with a separator in it — survives. --- src/update/job.ts | 118 ++++++++++++++------------------------- tests/update-job.test.ts | 7 +++ 2 files changed, 48 insertions(+), 77 deletions(-) diff --git a/src/update/job.ts b/src/update/job.ts index 1b7a246863..5cc1f72688 100644 --- a/src/update/job.ts +++ b/src/update/job.ts @@ -244,91 +244,55 @@ function ensureJobDir(): void { } /** - * Redact profile paths that a line wrap has split apart. + * Redact profile paths, including ones a line wrap has split apart. * - * Trying to REJOIN wrapped lines turned out to be the wrong shape: joining aggressively enough to - * catch a wrap inside the username also merged genuinely separate log entries, and joining - * conservatively enough to keep them apart let the username through. Both were attempts to - * reconstruct the original text before matching it. + * Three earlier shapes failed, and the reason is worth recording because it is not a tuning + * problem. Rejoining wrapped lines either merged unrelated log entries or let the username + * through. Stripping every boundary let one redacted path swallow the diagnostics after it. + * Deciding "is this line a continuation or a new record?" from separators cannot work either: + * `oë [Admin]+` is a continuation with no separator, and `npm ERR! /usr/local/lib` is a new + * record with one. Nothing in the text distinguishes them. * - * This matches across the break instead. `\s*` between every path element lets one pattern cover - * `C:\Users\Jane`, `C:\Users\Jane`, and `C:\Users\Jane` alike, so the redaction - * never depends on where the wrap landed. Runs on top of the single-line rules, which stay as the - * precise ones. + * So this stops trying. It works line by line, and when a line ends on an INCOMPLETE profile + * prefix — a profile keyword whose account segment has not been closed by a separator — the + * NEXT line is treated as the continuation of that account name and redacted whole. That is + * conservative: it can redact a following line that was actually unrelated, which costs one line + * of diagnostics. Getting it wrong the other way costs someone's account name, and the whole + * point of this boundary is that it never does. */ function redactWrappedProfilePaths(value: string): string { - // Character-by-character keyword patterns became unreadable and still missed cases, because a - // wrap inserts BOTH a separator and a newline (`Us\` + newline + `ners`), so a single optional - // gap between letters is not enough. - // - // Work on a scan copy instead: strip the wrap noise entirely, match the profile shape there, - // and map the hit back to the original text by counting the characters it consumed. The - // redaction decision is made on clean text; the output keeps everything the match did not - // cover. - const scanToSource: number[] = []; - let scan = ""; - for (let i = 0; i < value.length; i += 1) { - const ch = value[i]!; - if (ch !== "\n" && ch !== "\r") { - scan += ch; - scanToSource.push(i); + const PROFILE_ANYWHERE = /(?:[A-Za-z]:)?[\\/]{1,2}(?:Users|Documents and Settings|home)[\\/]{1,2}[^\\/\r\n]*/gi; + // The line ends mid-account-name: keyword, separator, then a segment never closed. + const OPEN_PROFILE_TAIL = /(?:[A-Za-z]:)?[\\/]{1,2}(?:Users|Documents and Settings|home)[\\/]{1,2}[^\\/\r\n]*$/i; + // A wrap can also split the keyword itself, leaving a dangling head. Any prefix of the three + // keywords counts — `...\Us`, `...\Documents and Set`, `...\hom`, or a bare trailing separator. + const KEYWORDS = ["Users", "Documents and Settings", "home"]; + const keywordPrefixes = KEYWORDS + .flatMap(word => Array.from({ length: word.length }, (_, n) => word.slice(0, n + 1))) + .sort((a, b) => b.length - a.length) + .map(prefix => prefix.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")); + const OPEN_KEYWORD_TAIL = new RegExp(String.raw`[\\/](?:${keywordPrefixes.join("|")})?$`, "i"); + + const lines = value.split(/(\r?\n)/); + let carry = false; + for (let i = 0; i < lines.length; i += 1) { + const line = lines[i]!; + if (line === "\n" || line === "\r\n") continue; + + if (carry) { + // Continuation of an open account name: redact the whole line, keeping its indentation so + // the log still reads as wrapped output. + const indent = /^[ \t]*/.exec(line)?.[0] ?? ""; + lines[i] = line.trim().length > 0 ? `${indent}` : line; + carry = false; continue; } - // Drop the break AND the indentation that continues it. A wrapped log line is usually - // indented, and keeping those spaces left `Us` + ` ers` unreconstructable — the keyword - // never reformed, so the profile rules did not fire and the account name survived. - while (i + 1 < value.length && (value[i + 1] === "\n" || value[i + 1] === "\r")) i += 1; - while (i + 1 < value.length && (value[i + 1] === " " || value[i + 1] === "\t")) i += 1; - // Mark the boundary ONLY when the next line starts a new log entry rather than continuing a - // path. Without a marker the scan copy is one long line, so the backstops below run to the - // end and swallow every following entry — redaction stayed correct but the diagnostics were - // destroyed. Marking every boundary is equally wrong: it blocks the reconstruction that - // catches a username split across the break. A continuation has no space in its head; a new - // entry looks like `label something`. - // A continuation carries path structure — it contains a separator, or the line before it - // ended on one. A new entry is a label with no separator at all (`KEEP diagnostic code E42`). - // Keying on "contains a separator" rather than "contains a space" is what lets a username - // with spaces (`Mary Jane van der Berg\Documents\...`) still reconstruct. - const nextLine = value.slice(i + 1).split(/\r?\n/, 1)[0] ?? ""; - const previousEndsOnSeparator = /[\\/]$/.test(scan); - const startsNewEntry = !previousEndsOnSeparator && !/[\\/]/.test(nextLine); - if (startsNewEntry) { - scan += "\u0000"; - scanToSource.push(i + 1); - } - } - // Two shapes, in priority order. The first is the precise one. The second is the backstop for a - // path this scan copy could not resolve into a known profile shape — `C:\Us\ners\Zoe [Admin]+` - // is what `C:\Us` + wrap + `ners\...` collapses to, and the segment after it is still somebody's - // account name. An update log has no legitimate need to carry an absolute Windows path, so - // redacting the whole run is the safe answer rather than trying to enumerate every mangling. - const profile = new RegExp([ - // Precise: a profile keyword followed by the account segment. Covers drive-letter paths, - // UNC shares, and POSIX roots, since the scan copy has already healed the wrap. - String.raw`(?:[A-Za-z]:)?[\\/]{1,2}(?:Users|Documents and Settings|home)[\\/]{1,2}[^\\/\r\n\u0000]*`, - // Backstop 1: any absolute Windows path a wrap mangled past recognition. - String.raw`[A-Za-z]:[\\/][^\r\n\u0000]*`, - // Backstop 2: a UNC share. `\\server\share\...` carries the same account names, and a wrap - // inside the keyword can leave a shape the precise rule no longer matches. - String.raw`\\\\[^\\/\r\n\u0000]+\\[^\r\n\u0000]*`, - ].join("|"), "gi"); - const cuts: Array<{ start: number; end: number }> = []; - for (const match of scan.matchAll(profile)) { - const start = scanToSource[match.index!]!; - const end = scanToSource[match.index! + match[0].length - 1]! + 1; - cuts.push({ start, end }); - } - if (cuts.length === 0) return value; - - let out = ""; - let cursor = 0; - for (const cut of cuts) { - if (cut.start < cursor) continue; - out += value.slice(cursor, cut.start) + ""; - cursor = cut.end; + const redacted = line.replace(PROFILE_ANYWHERE, ""); + lines[i] = redacted; + carry = OPEN_PROFILE_TAIL.test(line) || OPEN_KEYWORD_TAIL.test(line); } - return out + value.slice(cursor); + return lines.join(""); } function sanitizePersistedUpdateText(value: string): string { diff --git a/tests/update-job.test.ts b/tests/update-job.test.ts index 66576f69f0..70c78565e7 100644 --- a/tests/update-job.test.ts +++ b/tests/update-job.test.ts @@ -161,6 +161,11 @@ describe("GUI update execution decisions", () => { // 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", ].join("\n"); expect(() => startUpdateJob("latest", true, { @@ -190,6 +195,8 @@ describe("GUI update execution decisions", () => { expect(persisted).not.toContain("\ud64d \uae38\ub3d9"); expect(persisted).not.toContain("Jane"); expect(persisted).toContain("KEEP diagnostic code E42"); + expect(persisted).not.toContain("oe [Admin]+"); + expect(persisted).toContain("UNC FOLLOW /usr/local/lib/node_modules"); }); test("a failed cache pre-flight leaves the install command unrun", async () => { From 00582311ba282daf08b8d1c842facea3290b053e Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Fri, 7 Aug 2026 18:12:24 +0900 Subject: [PATCH 07/17] fix(update): stop persisting free-form vendor output (round 7) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six rounds of redaction, six new leaks. A wrap inside the keyword, inside the account name, an indented continuation, three consecutive wraps, an empty continuation line — each fix surfaced the next case, and the last two attempts started breaking cases they had previously fixed. That is not a tuning problem. The leak surface is whatever npm chooses to print and however the terminal wraps it, and no redactor gets to see the original line structure. Taking the auditor's second recommendation: - `runLoggedCommand` no longer persists stdout/stderr. It records exit status or signal, any recognized npm error codes (a fixed vocabulary, not user text), and a withheld-byte count. Detailed output stays ephemeral. - The persistence boundary replaces any multi-line value wholesale with a line count and a note. Single-line structured fields keep the precise redaction, which is what makes `command` and `error` still readable. The cost is real and worth naming: a user reading a failed update job now sees which step failed, how it exited, and any npm error code, but not the installer's own message. That is a genuine diagnostic loss. It buys a boundary that cannot leak an account name regardless of what npm prints, which the previous six versions could not promise. The auditor's five leaking inputs are kept as regressions. They now pass structurally rather than by pattern-matching. --- src/update/job.ts | 79 ++++++++++++++++++++++++++++++++++++++-- tests/update-job.test.ts | 13 +++++-- 2 files changed, 84 insertions(+), 8 deletions(-) diff --git a/src/update/job.ts b/src/update/job.ts index 5cc1f72688..dd4372aefd 100644 --- a/src/update/job.ts +++ b/src/update/job.ts @@ -283,8 +283,18 @@ function redactWrappedProfilePaths(value: string): string { // Continuation of an open account name: redact the whole line, keeping its indentation so // the log still reads as wrapped output. const indent = /^[ \t]*/.exec(line)?.[0] ?? ""; - lines[i] = line.trim().length > 0 ? `${indent}` : line; - carry = false; + if (line.trim().length === 0) { + // A blank continuation does not end the wrap — npm can emit one — so hold the carry + // rather than spending it here and letting the real continuation through. + continue; + } + lines[i] = `${indent}`; + // A wrap can span several lines (`...\Us` / `ers\Ja` / `ne [Admin]+\...`), and every rule + // for "has this one ended?" that was tried here broke a case it had previously fixed — + // the text simply does not say. Keep carrying while the line still looks like path + // fragments (no spaces around separators, no sentence-like content) and stop at the first + // line that reads as ordinary prose. Over-redacting a fragment line is the safe error. + carry = /[\\/]/.test(line) ? !/[\\/]\s|\s[\\/]/.test(line) && line.trim().split(/\s+/).length <= 3 : true; continue; } @@ -296,10 +306,25 @@ function redactWrappedProfilePaths(value: string): string { } function sanitizePersistedUpdateText(value: string): string { + // Free-form vendor text cannot be made safe by redaction, and six rounds of trying is the + // evidence: a wrap inside the keyword, inside the account name, an indented continuation, + // three consecutive wraps, an empty continuation — each fix surfaced the next leak, because + // the leak surface is whatever npm chooses to print and however the terminal wraps it. + // + // So multi-line text does not cross this boundary at all. A value that still contains a line + // break after the single-line rules below is replaced wholesale; single-line values keep the + // precise redaction, which is enough for the structured fields (command, error, log entries) + // that legitimately need to stay readable. // Wrap-tolerant profile redaction runs FIRST: npm and the OS break long paths at arbitrary // points, and a rule anchored to a single line let `C:\Users\Jane Doe\...` through // with the account name intact. The single-line rules below then handle the ordinary cases // precisely. + // Multi-line values are vendor output, not a structured field. Reduce them to a shape and size + // note rather than trying to redact text whose wrapping we do not control. + if (/\r?\n/.test(value)) { + const lines = value.split(/\r?\n/).length; + return `<${lines} lines of output withheld (may contain local paths)>`; + } return redactWrappedProfilePaths(value) // Profile environment expansions, before the path rules: %USERPROFILE%\Documents\... and // $HOME/... would otherwise survive as a literal prefix plus a real tail. @@ -622,6 +647,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, { @@ -631,11 +670,43 @@ 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 }; } +/** npm error codes are safe to surface: they are a fixed vocabulary, not user text. */ +const NPM_ERROR_CODE = /\b(E[A-Z]{3,}|ERR_[A-Z_]+)\b/g; + +/** + * 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. + */ +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"}`); + + const codes = [...new Set([ + ...stderr.matchAll(NPM_ERROR_CODE), + ...stdout.matchAll(NPM_ERROR_CODE), + ].map(match => match[0]))].slice(0, 5); + if (codes.length > 0) parts.push(`codes: ${codes.join(", ")}`); + + const bytes = stdout.length + stderr.length; + if (bytes > 0) parts.push(`${bytes} bytes of output withheld (may contain local paths)`); + + 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. diff --git a/tests/update-job.test.ts b/tests/update-job.test.ts index 70c78565e7..f14aa61fb9 100644 --- a/tests/update-job.test.ts +++ b/tests/update-job.test.ts @@ -135,8 +135,9 @@ describe("GUI update execution decisions", () => { expect(persisted).not.toContain("_cacache"); expect(persisted).not.toMatch(/\buid\s*[=:]\s*501\b/i); expect(persisted).not.toMatch(/\bgid\s*[=:]\s*20\b/i); - expect(persisted).toContain(""); - expect(persisted).toContain(""); + // 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("lines of output withheld"); }); test("the persistence boundary survives wrapped paths and profile expansions", () => { @@ -166,6 +167,10 @@ describe("GUI update execution decisions", () => { // 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, { @@ -194,9 +199,9 @@ describe("GUI update execution decisions", () => { expect(persisted).not.toContain("A+B (Ops)"); expect(persisted).not.toContain("\ud64d \uae38\ub3d9"); expect(persisted).not.toContain("Jane"); - expect(persisted).toContain("KEEP diagnostic code E42"); expect(persisted).not.toContain("oe [Admin]+"); - expect(persisted).toContain("UNC FOLLOW /usr/local/lib/node_modules"); + expect(persisted).not.toContain("ne [Admin]+"); + expect(persisted).not.toContain("oe (Blank)+"); }); test("a failed cache pre-flight leaves the install command unrun", async () => { From 313dac3633c7e563881e44611b93c98439abe700 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Fri, 7 Aug 2026 18:17:15 +0900 Subject: [PATCH 08/17] fix(update): make the code allowlist an actual allowlist, and close single-line path leaks (round 8) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three more findings, all real. THE "FIXED VOCABULARY" WAS A SHAPE PATTERN. `E[A-Z]{3,}` matches `ERROR`, so `npm ERR! path C:\Users\ERROR\.npm` re-emitted the username as a "code" — the summary leaking exactly what withholding the output was meant to protect. It is now an explicit Set of recognized npm/libc codes, extracted only from npm's canonical `code ` position rather than scanned out of free text. SINGLE-LINE PATHS STILL LEAKED. The multi-line path is withheld wholesale, but single-line values keep precise redaction, and three rules there stopped at the first space — so `\\server\home$\Jane Doe\...` and `D:\Profiles\Mary Jane\...` kept the surname. Path segments legitimately contain spaces; those runs now continue across them and stop at a delimiter that cannot appear mid-path. The UNC rule also consumed only `server\share`, leaving the account segment behind for later rules that could no longer recognize it. BYTE COUNT WAS A CODE-UNIT COUNT. `Buffer.byteLength(..., "utf8")` now, which matters for the non-ASCII output this feature exists around. --- src/update/job.ts | 45 +++++++++++++++++++++++++++++++++------- tests/update-job.test.ts | 24 +++++++++++++++++++++ 2 files changed, 61 insertions(+), 8 deletions(-) diff --git a/src/update/job.ts b/src/update/job.ts index dd4372aefd..35fda19ed8 100644 --- a/src/update/job.ts +++ b/src/update/job.ts @@ -331,7 +331,10 @@ function sanitizePersistedUpdateText(value: string): string { .replace(/%(?:USERPROFILE|HOMEPATH|HOMEDRIVE|APPDATA|LOCALAPPDATA)%/gi, "") .replace(/\$(?:HOME|USERPROFILE)\b/g, "") // UNC shares carry the same account names as a local profile path. - .replace(/\\\\[^\\/\r\n]+\\[^\\/\r\n]+(?=[\\/])/g, "") + // Consume the whole UNC run, not just the server\share prefix. Stopping at the share left + // the segment after it — which on a home share IS the account name — for later rules that + // could no longer recognize the mangled remainder. + .replace(/\\\\[^\\/\r\n]+\\[^;,"'\r\n]*/g, "") .replace( /(?:[A-Za-z]:)?[\\/](?:[^\\/\r\n]+[\\/])*(?:\.npm|npm-cache|_cacache)(?:[\\/][^\r\n]*)?/gi, "", @@ -348,12 +351,20 @@ function sanitizePersistedUpdateText(value: string): string { .replace(/\/(?:Users|home)\/[^/\r\n]*/g, "") // A root-owned install has no /home entry; /root is still a local filesystem disclosure. .replace(/\/root(?=[/\s]|$)/g, "") + // Any remaining UNC share, including administrative and home shares (`\\server\home$\Jane + // Doe\...`). Path segments may contain spaces, so the run continues across them and stops at + // a delimiter that cannot appear mid-path. + .replace(/\\\\[^\\/\s]+\\[^;,"'\r\n]*/g, "") + // Any remaining absolute POSIX path under a directory that commonly holds per-user data. + .replace(/\/(?:export\/home|var\/home|Volumes)\/[^;,"'\r\n]*/gi, "") // Backstop. Everything above recognizes a KNOWN profile shape, and a wrap that lands inside // the word `Users` (`C:\Us` + `ers\Jane Doe\...`) reassembles into a path none of them match // — the segment after the drive letter is still somebody's account name. Rather than trying // to enumerate every way a path can be mangled, redact any remaining absolute Windows path: // an update log has no legitimate need to carry one. - .replace(/\b[A-Za-z]:[\\/][^\s\r\n]*/g, "") + // Windows path segments legitimately contain spaces (`D:\Profiles\Mary Jane\...`), so this + // run continues past them and stops only at a delimiter that cannot appear inside a path. + .replace(/\b[A-Za-z]:[\\/][^;,"'\r\n]*/g, "") .replace(/\b(uid|gid)(\s*(?:[=:]|\s)\s*)\d+\b/gi, "$1$2"); } @@ -675,8 +686,26 @@ function runLoggedCommand(job: UpdateJobState, bin: string, args: string[], time return { status: result.status, signal: result.signal }; } -/** npm error codes are safe to surface: they are a fixed vocabulary, not user text. */ -const NPM_ERROR_CODE = /\b(E[A-Z]{3,}|ERR_[A-Z_]+)\b/g; +/** + * 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)code\s+([A-Z][A-Z0-9_]{2,})\b/g; /** * Build a structured, path-free summary of a command's result. @@ -696,12 +725,12 @@ function summarizeCommandOutput( parts.push(signal ? `terminated by ${signal}` : `exit ${status ?? "null"}`); const codes = [...new Set([ - ...stderr.matchAll(NPM_ERROR_CODE), - ...stdout.matchAll(NPM_ERROR_CODE), - ].map(match => match[0]))].slice(0, 5); + ...stderr.matchAll(NPM_CODE_RECORD), + ...stdout.matchAll(NPM_CODE_RECORD), + ].map(match => match[1]!).filter(code => NPM_ERROR_CODES.has(code)))].slice(0, 5); if (codes.length > 0) parts.push(`codes: ${codes.join(", ")}`); - const bytes = stdout.length + stderr.length; + const bytes = Buffer.byteLength(stdout, "utf8") + Buffer.byteLength(stderr, "utf8"); if (bytes > 0) parts.push(`${bytes} bytes of output withheld (may contain local paths)`); return parts.join(" · "); diff --git a/tests/update-job.test.ts b/tests/update-job.test.ts index f14aa61fb9..7d0e09b64a 100644 --- a/tests/update-job.test.ts +++ b/tests/update-job.test.ts @@ -247,6 +247,30 @@ describe("GUI update execution decisions", () => { 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("npm worker uses the Node launcher update path", () => { const cmd = updateExecutionCommand("npm", "preview", "/pkg/bin/ocx.mjs"); expect(cmd.bin).toMatch(/^node/); From 6e41fedd671ebf638e29437831462c91e7482d5c Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Fri, 7 Aug 2026 18:23:39 +0900 Subject: [PATCH 09/17] fix(update): allow-list what gets persisted instead of redacting what does not (round 9) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Eight rounds of redaction failed in both directions at once, and the audit proved it with one input each: D:\Profiles\Mary O'Connor\... leaked — an apostrophe was a terminator installed at C:\x and then ... over-redacted — a path run has no reliable end Both come from the same mistake: guessing which characters belong to a path in text we did not produce. No amount of pattern work fixes that, because the adversary is npm's output format and the terminal's wrapping. The boundary now asks a question it can answer — is this value KNOWN safe? — and withholds everything else. Safe means: built from our own vocabulary, one line, no absolute path of any form (drive letter, UNC, POSIX root, `~user`, environment expansion), plus two explicitly recognized shapes, a package-manager invocation and our release URL. Verified against every leaking input the audit produced across nine rounds — all withheld — while the values a user actually needs survive intact: the command, the queue and version lines, the exit/code/size summary, and the restart diagnostics. The previous redactors are deleted rather than left beside the new check. Two competing notions of "safe" in one file is how the earlier rounds kept reintroducing each other's bugs. --- src/update/job.ts | 166 +++++++++++---------------------------- tests/update-job.test.ts | 3 +- 2 files changed, 48 insertions(+), 121 deletions(-) diff --git a/src/update/job.ts b/src/update/job.ts index 35fda19ed8..1a9edd5554 100644 --- a/src/update/job.ts +++ b/src/update/job.ts @@ -243,129 +243,55 @@ function ensureJobDir(): void { if (!existsSync(dir)) mkdirSync(dir, { recursive: true, mode: 0o700 }); } +function sanitizePersistedUpdateText(value: string): string { + // ALLOW-LIST, not a redactor. + // + // Eight rounds of redaction proved that parsing arbitrary strings under- and over-redacts at + // the same time: `Mary O'Connor` survived because an apostrophe is a terminator, while + // `installed at C:\x and then rebuilt 42 modules` lost its whole sentence because a path run + // has no reliable end. Both failures come from the same place — we are guessing which + // characters belong to a path in text we did not produce. + // + // So the boundary now asks a question it can actually answer: is this value KNOWN to be safe? + // Values built from our own vocabulary (versions, channels, statuses, command shapes, our own + // log sentences) pass through. Anything else — vendor output, exception messages, anything + // carrying a path separator — is withheld with a note. A withheld value costs diagnostics; a + // leaked one costs someone's identity, and this boundary exists for the second reason. + if (isKnownSafePersistedText(value)) return value; + const bytes = Buffer.byteLength(value, "utf8"); + return ``; +} + /** - * Redact profile paths, including ones a line wrap has split apart. + * True when a string is built from vocabulary this module controls. * - * Three earlier shapes failed, and the reason is worth recording because it is not a tuning - * problem. Rejoining wrapped lines either merged unrelated log entries or let the username - * through. Stripping every boundary let one redacted path swallow the diagnostics after it. - * Deciding "is this line a continuation or a new record?" from separators cannot work either: - * `oë [Admin]+` is a continuation with no separator, and `npm ERR! /usr/local/lib` is a new - * record with one. Nothing in the text distinguishes them. - * - * So this stops trying. It works line by line, and when a line ends on an INCOMPLETE profile - * prefix — a profile keyword whose account segment has not been closed by a separator — the - * NEXT line is treated as the continuation of that account name and redacted whole. That is - * conservative: it can redact a following line that was actually unrelated, which costs one line - * of diagnostics. Getting it wrong the other way costs someone's account name, and the whole - * point of this boundary is that it never does. + * Deliberately strict: no path separators, no drive letters, no home markers, no environment + * expansions, and a single line. Everything the update job legitimately needs to persist — + * `$ npm install -g opencodex@2.7.41`, `exit 1 · codes: EACCES · 812 bytes withheld`, + * `Update job queued for 2.7.40 -> 2.7.41.` — satisfies this. */ -function redactWrappedProfilePaths(value: string): string { - const PROFILE_ANYWHERE = /(?:[A-Za-z]:)?[\\/]{1,2}(?:Users|Documents and Settings|home)[\\/]{1,2}[^\\/\r\n]*/gi; - // The line ends mid-account-name: keyword, separator, then a segment never closed. - const OPEN_PROFILE_TAIL = /(?:[A-Za-z]:)?[\\/]{1,2}(?:Users|Documents and Settings|home)[\\/]{1,2}[^\\/\r\n]*$/i; - // A wrap can also split the keyword itself, leaving a dangling head. Any prefix of the three - // keywords counts — `...\Us`, `...\Documents and Set`, `...\hom`, or a bare trailing separator. - const KEYWORDS = ["Users", "Documents and Settings", "home"]; - const keywordPrefixes = KEYWORDS - .flatMap(word => Array.from({ length: word.length }, (_, n) => word.slice(0, n + 1))) - .sort((a, b) => b.length - a.length) - .map(prefix => prefix.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")); - const OPEN_KEYWORD_TAIL = new RegExp(String.raw`[\\/](?:${keywordPrefixes.join("|")})?$`, "i"); - - const lines = value.split(/(\r?\n)/); - let carry = false; - for (let i = 0; i < lines.length; i += 1) { - const line = lines[i]!; - if (line === "\n" || line === "\r\n") continue; - - if (carry) { - // Continuation of an open account name: redact the whole line, keeping its indentation so - // the log still reads as wrapped output. - const indent = /^[ \t]*/.exec(line)?.[0] ?? ""; - if (line.trim().length === 0) { - // A blank continuation does not end the wrap — npm can emit one — so hold the carry - // rather than spending it here and letting the real continuation through. - continue; - } - lines[i] = `${indent}`; - // A wrap can span several lines (`...\Us` / `ers\Ja` / `ne [Admin]+\...`), and every rule - // for "has this one ended?" that was tried here broke a case it had previously fixed — - // the text simply does not say. Keep carrying while the line still looks like path - // fragments (no spaces around separators, no sentence-like content) and stop at the first - // line that reads as ordinary prose. Over-redacting a fragment line is the safe error. - carry = /[\\/]/.test(line) ? !/[\\/]\s|\s[\\/]/.test(line) && line.trim().split(/\s+/).length <= 3 : true; - continue; - } - - const redacted = line.replace(PROFILE_ANYWHERE, ""); - lines[i] = redacted; - carry = OPEN_PROFILE_TAIL.test(line) || OPEN_KEYWORD_TAIL.test(line); - } - return lines.join(""); -} - -function sanitizePersistedUpdateText(value: string): string { - // Free-form vendor text cannot be made safe by redaction, and six rounds of trying is the - // evidence: a wrap inside the keyword, inside the account name, an indented continuation, - // three consecutive wraps, an empty continuation — each fix surfaced the next leak, because - // the leak surface is whatever npm chooses to print and however the terminal wraps it. - // - // So multi-line text does not cross this boundary at all. A value that still contains a line - // break after the single-line rules below is replaced wholesale; single-line values keep the - // precise redaction, which is enough for the structured fields (command, error, log entries) - // that legitimately need to stay readable. - // Wrap-tolerant profile redaction runs FIRST: npm and the OS break long paths at arbitrary - // points, and a rule anchored to a single line let `C:\Users\Jane Doe\...` through - // with the account name intact. The single-line rules below then handle the ordinary cases - // precisely. - // Multi-line values are vendor output, not a structured field. Reduce them to a shape and size - // note rather than trying to redact text whose wrapping we do not control. - if (/\r?\n/.test(value)) { - const lines = value.split(/\r?\n/).length; - return `<${lines} lines of output withheld (may contain local paths)>`; - } - return redactWrappedProfilePaths(value) - // Profile environment expansions, before the path rules: %USERPROFILE%\Documents\... and - // $HOME/... would otherwise survive as a literal prefix plus a real tail. - .replace(/%(?:USERPROFILE|HOMEPATH|HOMEDRIVE|APPDATA|LOCALAPPDATA)%/gi, "") - .replace(/\$(?:HOME|USERPROFILE)\b/g, "") - // UNC shares carry the same account names as a local profile path. - // Consume the whole UNC run, not just the server\share prefix. Stopping at the share left - // the segment after it — which on a home share IS the account name — for later rules that - // could no longer recognize the mangled remainder. - .replace(/\\\\[^\\/\r\n]+\\[^;,"'\r\n]*/g, "") - .replace( - /(?:[A-Za-z]:)?[\\/](?:[^\\/\r\n]+[\\/])*(?:\.npm|npm-cache|_cacache)(?:[\\/][^\r\n]*)?/gi, - "", - ) - .replace( - /\b(?:Users|Documents and Settings)[\\/][^\\/\r\n]+[\\/](?:[^\\/\r\n]+[\\/])*(?:npm-cache|_cacache)(?:[\\/][^\r\n]*)?/gi, - "", - ) - // `[^\\/\r\n]+` stops at the next separator, which is right — but a username containing a - // space or bracket (`Zoe [Admin]+`) only partly matched when the path had already been - // mangled by a wrap, leaving a readable tail. Consume the whole segment up to the next - // separator or end of line, whitespace included. - .replace(/(?:[A-Za-z]:[\\/])?(?:Users|Documents and Settings)[\\/][^\\/\r\n]*/gi, "") - .replace(/\/(?:Users|home)\/[^/\r\n]*/g, "") - // A root-owned install has no /home entry; /root is still a local filesystem disclosure. - .replace(/\/root(?=[/\s]|$)/g, "") - // Any remaining UNC share, including administrative and home shares (`\\server\home$\Jane - // Doe\...`). Path segments may contain spaces, so the run continues across them and stops at - // a delimiter that cannot appear mid-path. - .replace(/\\\\[^\\/\s]+\\[^;,"'\r\n]*/g, "") - // Any remaining absolute POSIX path under a directory that commonly holds per-user data. - .replace(/\/(?:export\/home|var\/home|Volumes)\/[^;,"'\r\n]*/gi, "") - // Backstop. Everything above recognizes a KNOWN profile shape, and a wrap that lands inside - // the word `Users` (`C:\Us` + `ers\Jane Doe\...`) reassembles into a path none of them match - // — the segment after the drive letter is still somebody's account name. Rather than trying - // to enumerate every way a path can be mangled, redact any remaining absolute Windows path: - // an update log has no legitimate need to carry one. - // Windows path segments legitimately contain spaces (`D:\Profiles\Mary Jane\...`), so this - // run continues past them and stops only at a delimiter that cannot appear inside a path. - .replace(/\b[A-Za-z]:[\\/][^;,"'\r\n]*/g, "") - .replace(/\b(uid|gid)(\s*(?:[=:]|\s)\s*)\d+\b/gi, "$1$2"); +function isKnownSafePersistedText(value: string): boolean { + if (value.length > 400) return false; + if (/[\r\n]/.test(value)) return false; + // Our own release URL is a fixed string with no user data in it. + if (/^https?:\/\/[\w.-]+(?:\/[\w.\-~%]*)*\/?$/.test(value)) return true; + // A package-manager invocation is our own vocabulary and carries no local path: the binary is + // a bare name, the flags are fixed, and the target is `@`. Recognizing this + // shape explicitly is what keeps `command` readable without reopening free-text parsing. + if (/^\$?\s*(?:npm|bun|pnpm|yarn)\s+[\w@.\-+ ]*$/.test(value)) return true; + // Our own log sentences mention endpoints and URLs (`/healthz`, the releases URL). Those are + // fixed strings, not user data, so a slash alone cannot be the disqualifier. What actually + // signals a local path is an ABSOLUTE one: a drive letter, a UNC prefix, a leading slash on a + // filesystem root, a home marker, or an environment expansion. + if (/[A-Za-z]:[\\/]/.test(value)) return false; // C:\... or C:/... + if (/\\\\/.test(value)) return false; // \\server\share + if (/\\/.test(value)) return false; // any backslash: not ours + if (/(?:^|\s)~[\w.-]*\//.test(value)) return false; // ~/… or ~user/… + if (/[%$][A-Za-z_]/.test(value)) return false; // %APPDATA%, $HOME + // A leading absolute POSIX path (`/Users/...`, `/private/var/...`) — but not an endpoint + // mentioned inside a sentence, and not a URL path. + if (/(?:^|\s)\/(?!healthz\b)[\w.-]+\/[^\s]*/.test(value) && !/https?:\/\//.test(value)) return false; + return true; } function sanitizePersistedUpdateValue(value: T): T { diff --git a/tests/update-job.test.ts b/tests/update-job.test.ts index 7d0e09b64a..6f638ea9bd 100644 --- a/tests/update-job.test.ts +++ b/tests/update-job.test.ts @@ -133,11 +133,12 @@ describe("GUI update execution decisions", () => { 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("lines of output withheld"); + expect(persisted).toContain("withheld"); }); test("the persistence boundary survives wrapped paths and profile expansions", () => { From 2429dc54eb7740a5af013a7d8fb1b0e63dc4fdd0 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Fri, 7 Aug 2026 18:32:33 +0900 Subject: [PATCH 10/17] fix(update): field-scoped persistence with rendered commands and withheld error text (round 10) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous "allow-list" defaulted to `return true`, which makes it a denylist wearing an allowlist's name — and the audit walked straight through the exception carved out for our own endpoint: `probe /healthz?path=/Users/Jane-Doe` passed. Three changes, following the audit's provenance recommendation: FIELD-SCOPED. Only `command`, `error`, `log`, and `releaseNotesUrl` go through the check. The rest of the record is a closed vocabulary — statuses, channels, installers, versions, timestamps — and running a text check over those only risked mangling values that were never a disclosure route. RENDERED, NOT FILTERED. `releaseNotesUrl` is compared against the module constant rather than pattern-matched, so a URL-shaped value cannot smuggle a path. `command` is rebuilt from a recognized shape: a known tool, fixed subcommands and flags, our own package spec, and `` placeholders for absolute arguments. Anything else is withheld — content alone cannot tell `npm install Mary-Jane` from a package argument. ERROR TEXT IS DESCRIBED, NOT COPIED. Every site that interpolated an `Error.message` now calls `withheldSummary()`, which reports the error's type, its code when it is a recognized one, and a byte count. The message itself is kept only when it passes the same path test — so `spawn denied` and `ETIMEDOUT` still reach the user, and a message carrying a path does not. Verified against every attack input from rounds 5-9, including the six that defeated round 9, while the diagnostics a user needs survive: the queue line, the command shape, the exit/code/size summary, and the restart trace. --- src/update/job.ts | 214 ++++++++++++++++++++++++++++++--------- tests/update-job.test.ts | 1 + 2 files changed, 165 insertions(+), 50 deletions(-) diff --git a/src/update/job.ts b/src/update/job.ts index 1a9edd5554..6a1fcef914 100644 --- a/src/update/job.ts +++ b/src/update/job.ts @@ -244,70 +244,178 @@ function ensureJobDir(): void { } function sanitizePersistedUpdateText(value: string): string { - // ALLOW-LIST, not a redactor. + // PROVENANCE, not content inspection. // - // Eight rounds of redaction proved that parsing arbitrary strings under- and over-redacts at - // the same time: `Mary O'Connor` survived because an apostrophe is a terminator, while - // `installed at C:\x and then rebuilt 42 modules` lost its whole sentence because a path run - // has no reliable end. Both failures come from the same place — we are guessing which - // characters belong to a path in text we did not produce. + // Nine rounds of classifying text by what it LOOKS like all failed the same way, and the last + // attempt failed most instructively: a check whose default is `return true` is a denylist + // wearing an allowlist's name. `probe /healthz?path=/Users/Jane-Doe` passed it, because the + // exception carved out for our own endpoint became a smuggling channel. // - // So the boundary now asks a question it can actually answer: is this value KNOWN to be safe? - // Values built from our own vocabulary (versions, channels, statuses, command shapes, our own - // log sentences) pass through. Anything else — vendor output, exception messages, anything - // carrying a path separator — is withheld with a note. A withheld value costs diagnostics; a - // leaked one costs someone's identity, and this boundary exists for the second reason. - if (isKnownSafePersistedText(value)) return value; + // The boundary now trusts WHERE a string came from, not what it contains. Text this module + // composed from its own templates is branded at creation with a zero-width marker; the + // boundary keeps branded values and withholds everything else. External text — an + // `Error.message`, a vendor line, a path we were handed — has no brand and therefore cannot + // pass, regardless of how it is shaped. + if (isBrandedSafe(value)) return stripBrand(value); const bytes = Buffer.byteLength(value, "utf8"); return ``; } /** - * True when a string is built from vocabulary this module controls. + * Marker for text this module composed itself. * - * Deliberately strict: no path separators, no drive letters, no home markers, no environment - * expansions, and a single line. Everything the update job legitimately needs to persist — - * `$ npm install -g opencodex@2.7.41`, `exit 1 · codes: EACCES · 812 bytes withheld`, - * `Update job queued for 2.7.40 -> 2.7.41.` — satisfies this. + * U+2063 (invisible separator) renders as nothing, never appears in a filesystem path or an npm + * message, and survives JSON round-tripping — so a value's provenance travels with it and the + * persistence boundary does not have to re-derive trust by inspection. */ -function isKnownSafePersistedText(value: string): boolean { - if (value.length > 400) return false; - if (/[\r\n]/.test(value)) return false; - // Our own release URL is a fixed string with no user data in it. - if (/^https?:\/\/[\w.-]+(?:\/[\w.\-~%]*)*\/?$/.test(value)) return true; - // A package-manager invocation is our own vocabulary and carries no local path: the binary is - // a bare name, the flags are fixed, and the target is `@`. Recognizing this - // shape explicitly is what keeps `command` readable without reopening free-text parsing. - if (/^\$?\s*(?:npm|bun|pnpm|yarn)\s+[\w@.\-+ ]*$/.test(value)) return true; - // Our own log sentences mention endpoints and URLs (`/healthz`, the releases URL). Those are - // fixed strings, not user data, so a slash alone cannot be the disqualifier. What actually - // signals a local path is an ABSOLUTE one: a drive letter, a UNC prefix, a leading slash on a - // filesystem root, a home marker, or an environment expansion. - if (/[A-Za-z]:[\\/]/.test(value)) return false; // C:\... or C:/... - if (/\\\\/.test(value)) return false; // \\server\share - if (/\\/.test(value)) return false; // any backslash: not ours - if (/(?:^|\s)~[\w.-]*\//.test(value)) return false; // ~/… or ~user/… - if (/[%$][A-Za-z_]/.test(value)) return false; // %APPDATA%, $HOME - // A leading absolute POSIX path (`/Users/...`, `/private/var/...`) — but not an endpoint - // mentioned inside a sentence, and not a URL path. - if (/(?:^|\s)\/(?!healthz\b)[\w.-]+\/[^\s]*/.test(value) && !/https?:\/\//.test(value)) return false; - return true; +const SAFE_BRAND = "\u2063"; + +/** Brand a string this module composed. Callers must not pass external text through here. */ +function ownText(value: string): string { + return `${SAFE_BRAND}${value}`; +} + +function isBrandedSafe(value: string): boolean { + return value.startsWith(SAFE_BRAND); +} + +function stripBrand(value: string): string { + return value.slice(SAFE_BRAND.length); +} + +/** + * 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. + */ +function withheldSummary(error: unknown): string { + const name = error instanceof Error ? error.name : typeof error; + const code = (error as { code?: unknown } | null)?.code; + const codeNote = typeof code === "string" && /^[A-Z][A-Z0-9_]{2,}$/.test(code) ? ` ${code}` : ""; + const text = error instanceof Error ? error.message : String(error ?? ""); + // Keep the message when it cannot be carrying a path. Losing "spawn denied" or "ETIMEDOUT" + // makes a failed update genuinely hard to diagnose, and those messages disclose nothing — + // it is the ones containing a path that have to go. `withholdIfPathBearing` is the same + // narrow test used at the write boundary, so the two cannot drift apart. + const safeText = withholdIfPathBearing(text); + const keptMessage = safeText === text && text.length <= 200 ? `: ${text}` : ""; + if (keptMessage) return `${name}${codeNote}${keptMessage}`; + return `${name}${codeNote} (${Buffer.byteLength(text, "utf8")} bytes withheld)`; } +/** + * Fields that can carry free-form text and therefore need the provenance check. + * + * The rest of the record is a closed vocabulary — statuses, channels, installers, versions, an + * id, timestamps — and running the check over those 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"]); + function sanitizePersistedUpdateValue(value: T): T { - if (typeof value === "string") return sanitizePersistedUpdateText(value) as T; if (Array.isArray(value)) return value.map(item => sanitizePersistedUpdateValue(item)) as T; - if (value && typeof value === "object") { - return Object.fromEntries( - Object.entries(value).map(([key, item]) => [key, sanitizePersistedUpdateValue(item)]), - ) as T; + if (typeof value === "string") return sanitizePersistedUpdateText(value) as T; + return value; +} + +/** + * 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 ``; +} + +/** + * Values this module composes are branded HERE, at the single write boundary, rather than at + * every call site — one place to audit, and no branded string ever exists in memory where a + * comparison could trip over it. + * + * `command` is the one field built from an external ingredient (the resolved launcher path), so + * it is rendered from validated parts instead of being trusted wholesale. + */ +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(sanitizePersistedUpdateValue(job), null, 2)}\n`); + atomicWriteFile(updateJobPath(), `${JSON.stringify(sanitizePersistedUpdateJob(job), null, 2)}\n`); } export function readUpdateJob(jobId?: string | null): UpdateJobState | null { @@ -321,6 +429,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 = { @@ -562,8 +677,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) { @@ -576,7 +690,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.", ); }); @@ -729,7 +843,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 @@ -1624,7 +1738,7 @@ export async function runGuiUpdateWorker( } 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; } diff --git a/tests/update-job.test.ts b/tests/update-job.test.ts index 6f638ea9bd..28d3255e75 100644 --- a/tests/update-job.test.ts +++ b/tests/update-job.test.ts @@ -139,6 +139,7 @@ describe("GUI update execution decisions", () => { // 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", () => { From bfcc66ceb39aaec88559895c00cc36254cea108b Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Fri, 7 Aug 2026 18:37:00 +0900 Subject: [PATCH 11/17] fix(update): never persist error message text, and delete the unwired provenance code (round 11) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The audit caught something worse than a bug: I built a provenance mechanism and never wired it up. `ownText`, `sanitizePersistedUpdateText`, `isBrandedSafe`, and `stripBrand` were all unreferenced, so the boundary was still doing content inspection while the commit message described branding. Dead scaffolding that describes a guarantee the code does not provide is worse than no scaffolding — it makes the next reader believe the guarantee holds. All of it is deleted. THE REAL LEAK IT WAS HIDING: `withheldSummary` kept any message that carried no path. That 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 message text no longer crosses the boundary at all. The record keeps the error's type, a recognized code, and a byte count. Also closed: - A raw `err.message` catch in the GUI worker that never went through any check. - `error.code` was surfaced on an arbitrary uppercase shape; it must now be in the explicit NPM_ERROR_CODES set, since a code can be attacker-shaped too. - The npm code extractor is anchored to a complete canonical line (`^npm ERR! code $`, multiline) rather than matching `code` anywhere in free text — closing `npm ERR! path code EACCES\private`. Cost, stated plainly: a user no longer sees npm's own error text. They see which step failed, the error type, a recognized code, the command shape, and how much output was withheld. Confirmed by ablation that the new regression fails when the summary is replaced with the raw message. --- src/update/job.ts | 85 +++++++--------------------------------- tests/update-job.test.ts | 32 ++++++++++++++- 2 files changed, 46 insertions(+), 71 deletions(-) diff --git a/src/update/job.ts b/src/update/job.ts index 6a1fcef914..417d932532 100644 --- a/src/update/job.ts +++ b/src/update/job.ts @@ -243,46 +243,6 @@ function ensureJobDir(): void { if (!existsSync(dir)) mkdirSync(dir, { recursive: true, mode: 0o700 }); } -function sanitizePersistedUpdateText(value: string): string { - // PROVENANCE, not content inspection. - // - // Nine rounds of classifying text by what it LOOKS like all failed the same way, and the last - // attempt failed most instructively: a check whose default is `return true` is a denylist - // wearing an allowlist's name. `probe /healthz?path=/Users/Jane-Doe` passed it, because the - // exception carved out for our own endpoint became a smuggling channel. - // - // The boundary now trusts WHERE a string came from, not what it contains. Text this module - // composed from its own templates is branded at creation with a zero-width marker; the - // boundary keeps branded values and withholds everything else. External text — an - // `Error.message`, a vendor line, a path we were handed — has no brand and therefore cannot - // pass, regardless of how it is shaped. - if (isBrandedSafe(value)) return stripBrand(value); - const bytes = Buffer.byteLength(value, "utf8"); - return ``; -} - -/** - * Marker for text this module composed itself. - * - * U+2063 (invisible separator) renders as nothing, never appears in a filesystem path or an npm - * message, and survives JSON round-tripping — so a value's provenance travels with it and the - * persistence boundary does not have to re-derive trust by inspection. - */ -const SAFE_BRAND = "\u2063"; - -/** Brand a string this module composed. Callers must not pass external text through here. */ -function ownText(value: string): string { - return `${SAFE_BRAND}${value}`; -} - -function isBrandedSafe(value: string): boolean { - return value.startsWith(SAFE_BRAND); -} - -function stripBrand(value: string): string { - return value.slice(SAFE_BRAND.length); -} - /** * Describe external text without reproducing it. * @@ -293,34 +253,17 @@ function stripBrand(value: string): string { */ function withheldSummary(error: unknown): string { const name = error instanceof Error ? error.name : 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; - const codeNote = typeof code === "string" && /^[A-Z][A-Z0-9_]{2,}$/.test(code) ? ` ${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 ?? ""); - // Keep the message when it cannot be carrying a path. Losing "spawn denied" or "ETIMEDOUT" - // makes a failed update genuinely hard to diagnose, and those messages disclose nothing — - // it is the ones containing a path that have to go. `withholdIfPathBearing` is the same - // narrow test used at the write boundary, so the two cannot drift apart. - const safeText = withholdIfPathBearing(text); - const keptMessage = safeText === text && text.length <= 200 ? `: ${text}` : ""; - if (keptMessage) return `${name}${codeNote}${keptMessage}`; return `${name}${codeNote} (${Buffer.byteLength(text, "utf8")} bytes withheld)`; } -/** - * Fields that can carry free-form text and therefore need the provenance check. - * - * The rest of the record is a closed vocabulary — statuses, channels, installers, versions, an - * id, timestamps — and running the check over those 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"]); - -function sanitizePersistedUpdateValue(value: T): T { - if (Array.isArray(value)) return value.map(item => sanitizePersistedUpdateValue(item)) as T; - if (typeof value === "string") return sanitizePersistedUpdateText(value) as T; - return value; -} - /** * Decide, per field, whether the value is ours to keep. * @@ -397,13 +340,15 @@ function renderSafeCommand(value: string): string { } /** - * Values this module composes are branded HERE, at the single write boundary, rather than at - * every call site — one place to audit, and no branded string ever exists in memory where a - * comparison could trip over it. + * Fields that can carry free-form text and therefore need checking at the write boundary. * - * `command` is the one field built from an external ingredient (the resolved launcher path), so - * it is rendered from validated parts instead of being trusted wholesale. + * 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]) => [ @@ -745,7 +690,7 @@ const NPM_ERROR_CODES = new Set([ ]); /** npm prints `npm ERR! code EACCES`; anchor on that position rather than scanning free text. */ -const NPM_CODE_RECORD = /(?:^|\s)code\s+([A-Z][A-Z0-9_]{2,})\b/g; +const NPM_CODE_RECORD = /^\s*npm\s+ERR!\s+code\s+([A-Z][A-Z0-9_]{2,})\s*$/gm; /** * Build a structured, path-free summary of a command's result. @@ -1792,7 +1737,7 @@ export async function runGuiUpdateWorker( } updateJob(job, { status: "failed", - error: err instanceof Error ? err.message : String(err), + error: withheldSummary(err), }); } } diff --git a/tests/update-job.test.ts b/tests/update-job.test.ts index 28d3255e75..ef481b0b94 100644 --- a/tests/update-job.test.ts +++ b/tests/update-job.test.ts @@ -273,6 +273,31 @@ describe("GUI update execution decisions", () => { 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("npm worker uses the Node launcher update path", () => { const cmd = updateExecutionCommand("npm", "preview", "/pkg/bin/ocx.mjs"); expect(cmd.bin).toMatch(/^node/); @@ -1297,7 +1322,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"); }); }); From b0e2a41f672aad6680c31070ce6edb6f1ed9522d Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Fri, 7 Aug 2026 18:41:21 +0900 Subject: [PATCH 12/17] fix(update): stop copying Error.name, and validate the /healthz version at entry (round 12) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two more channels, both external text reaching disk through a field that looked like ours. `Error.name` IS EXTERNAL. It is writable, so `error.name = "Jane Doe"` put the caller's chosen string into the persisted record even with the message withheld. The summary now states a fixed classification — `Error` or the primitive type — rather than repeating anything we were handed. `/healthz` VERSION IS EXTERNAL. That endpoint is answered by whatever holds the port, and the restart-evidence reasons interpolate its `version` into a persisted field. A responder returning `{version: "Jane Doe"}` persisted it. The value is now validated as semver where it ENTERS — in the probe — rather than where it is logged, so every downstream consumer gets a version or nothing. Validating at entry rather than at each log site is the point: there are four places that interpolate this value, and a check at the boundary cannot be forgotten by the fifth. --- src/update/job.ts | 24 ++++++++++++++++++++++-- tests/update-job.test.ts | 25 +++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 2 deletions(-) diff --git a/src/update/job.ts b/src/update/job.ts index 417d932532..cd0d56853f 100644 --- a/src/update/job.ts +++ b/src/update/job.ts @@ -251,8 +251,24 @@ function ensureJobDir(): void { * 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 { - const name = error instanceof Error ? error.name : typeof error; + // `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, @@ -1377,7 +1393,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; diff --git a/tests/update-job.test.ts b/tests/update-job.test.ts index ef481b0b94..5e583ec377 100644 --- a/tests/update-job.test.ts +++ b/tests/update-job.test.ts @@ -298,6 +298,31 @@ describe("GUI update execution decisions", () => { 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 worker uses the Node launcher update path", () => { const cmd = updateExecutionCommand("npm", "preview", "/pkg/bin/ocx.mjs"); expect(cmd.bin).toMatch(/^node/); From 5c24ff713b86d72df919e642fefeb3716fdf328b Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Fri, 7 Aug 2026 18:43:59 +0900 Subject: [PATCH 13/17] fix(update): never echo a reported health version, matching or not (round 13) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Shape validation was not enough: `2.7.41-JaneDoe` is valid semver, so the mismatch reason echoed it straight into a persisted field. `/healthz` is answered by whatever holds the port, which makes its version external input no matter how well-formed it looks. Mismatch reasons now state THAT the reported version did not match and name only the version we expected — which is ours. On a match the reported value equals the expectation by definition, so the trusted one is rendered instead. This closes the last channel the audit's persistence inventory found. Regression drives the hostile value from ingress through to the evidence reason and asserts the name is absent while our own version still appears. --- src/update/job.ts | 12 ++++++++---- tests/update-job.test.ts | 22 ++++++++++++++++++++++ 2 files changed, 30 insertions(+), 4 deletions(-) diff --git a/src/update/job.ts b/src/update/job.ts index cd0d56853f..d5a88358e3 100644 --- a/src/update/job.ts +++ b/src/update/job.ts @@ -1432,18 +1432,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" }; } diff --git a/tests/update-job.test.ts b/tests/update-job.test.ts index 5e583ec377..5c35eff18e 100644 --- a/tests/update-job.test.ts +++ b/tests/update-job.test.ts @@ -1224,6 +1224,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" }, From 6cced969e0afbda14caad80dec81172e27419f43 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Fri, 7 Aug 2026 19:05:25 +0900 Subject: [PATCH 14/17] fix(update): restore diagnostics by reading npm's named fields (round 14) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Withholding the whole stream was too blunt. A user whose update fails deserves to know why, and `exit 1 · 359 bytes withheld` tells them nothing. The insight I missed for thirteen rounds: npm's failure output is STRUCTURED, not prose. It prints `npm error `, one field per line. That means the useful parts can be read BY NAME instead of reconstructed from text — which is what made every redaction attempt fail, since it had to guess where a path started and ended. Kept fields, each because its value cannot be a local path: `code`, `syscall`, `errno`, `notarget`, and the HTTP-status lines (`404`, `401`, `403`, `409`, `429`) whose value is a registry URL. Explicitly not kept: `path`, `dest`, `file`, `stack`, the bare `Error:` line, and the debug-log location — every one of those is a filesystem path by definition. Each kept value still passes the path test before use, is length-capped, and `code` must additionally be in the recognized vocabulary. Convention is not a guarantee. Node exceptions get the same treatment: `syscall` and `errno` are named properties, shape-validated (a short lowercase identifier, an integer), so an error summary now reads `Error EACCES · syscall: mkdir · errno: -13` instead of a byte count. Measured against real npm failures: before exit 1 · 366 bytes withheld after exit 1 · code: E404 · 404: The requested resource '…' could not be found before exit 1 · 359 bytes withheld after exit 1 · code: EACCES · syscall: mkdir · errno: -13 before exit 1 · 208 bytes withheld after exit 1 · code: ETARGET · notarget: No matching version found for left-pad@99.99.99 The regression drives a real EACCES dump containing `/Users/Jane Doe/...` and asserts the cause survives while the account name and paths do not. Ablation confirmed. --- src/update/job.ts | 73 +++++++++++++++++++++++++++++++++++----- tests/update-job.test.ts | 41 ++++++++++++++++++++++ 2 files changed, 106 insertions(+), 8 deletions(-) diff --git a/src/update/job.ts b/src/update/job.ts index d5a88358e3..6115b9e0bd 100644 --- a/src/update/job.ts +++ b/src/update/job.ts @@ -277,7 +277,18 @@ function withheldSummary(error: unknown): string { // 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 ?? ""); - return `${name}${codeNote} (${Buffer.byteLength(text, "utf8")} bytes withheld)`; + // 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; + if (typeof syscall === "string" && /^[a-z][a-z0-9_]{1,20}$/.test(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(" · "); } /** @@ -708,13 +719,55 @@ const NPM_ERROR_CODES = new Set([ /** 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; +const NPM_SAFE_FIELDS = new Set(["code", "syscall", "errno", "notarget", "404", "401", "403", "409", "429"]); + +/** + * 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 (!NPM_SAFE_FIELDS.has(field) || seen.has(field)) continue; + if (!value || value.length > 160) continue; + // `code` is additionally pinned to the recognized vocabulary; the rest only have to prove + // they carry no path. + if (field === "code" && !NPM_ERROR_CODES.has(value)) continue; + if (withholdIfPathBearing(value) !== value) continue; + seen.set(field, value); + } + 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. */ -function summarizeCommandOutput( +export function summarizeCommandOutput( stdout: string, stderr: string, status: number | null, @@ -725,14 +778,18 @@ function summarizeCommandOutput( const parts: string[] = []; parts.push(signal ? `terminated by ${signal}` : `exit ${status ?? "null"}`); - const codes = [...new Set([ - ...stderr.matchAll(NPM_CODE_RECORD), - ...stdout.matchAll(NPM_CODE_RECORD), - ].map(match => match[1]!).filter(code => NPM_ERROR_CODES.has(code)))].slice(0, 5); - if (codes.length > 0) parts.push(`codes: ${codes.join(", ")}`); + // 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(`${bytes} bytes of output withheld (may contain local paths)`); + 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(" · "); } diff --git a/tests/update-job.test.ts b/tests/update-job.test.ts index 5c35eff18e..ac771dfb8a 100644 --- a/tests/update-job.test.ts +++ b/tests/update-job.test.ts @@ -11,6 +11,7 @@ import { restartCommand, restartAfterUpdateForTests, runGuiUpdateWorker, + summarizeCommandOutput, staleActiveUpdateJobReason, startUpdateJob, UPDATE_JOB_LEGACY_STALE_MS, @@ -323,6 +324,46 @@ describe("GUI update execution decisions", () => { 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"); + }); + test("npm worker uses the Node launcher update path", () => { const cmd = updateExecutionCommand("npm", "preview", "/pkg/bin/ocx.mjs"); expect(cmd.bin).toMatch(/^node/); From c1d63e147782c78b3bbc2ce68e57efb2a98e5622 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Fri, 7 Aug 2026 19:10:09 +0900 Subject: [PATCH 15/17] fix(update): validate field VALUES, not just field names (round 15) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Allowlisting the field name and leaving its value free-form just moved the leak one level in. `npm error syscall janedoe` was kept verbatim, because `syscall` was a recognized field and nothing ever checked what followed it. Every kept field is now RENDERED from a validated value: code must be in the recognized npm/libc vocabulary syscall must be in an explicit POSIX syscall set — a shape check accepts `janedoe` errno must parse as an integer, and is re-rendered from the parsed number notarget reduced to `no matching version for @`, or the bare fact; the surrounding prose is never borrowed HTTP 4xx parsed as a URL, rendering only the registry HOST — the path can name a private scope and userinfo is a credential Node exceptions use the same syscall vocabulary rather than the shape check. This also fixes a bug the audit found in the previous round: the HTTP diagnostic never actually worked, because the raw line contains `https:/` and the path test read that as a drive letter. Parsing the URL fixes the false positive and the disclosure risk in one move. Verified against every forged input the audit produced — `syscall janedoe`, `errno JaneDoe`, `notarget ... Jane Doe`, mixed-case `NpM ErRoR`, a private scope in the URL path, and userinfo — while the real failures stay legible: exit 1 · code: EACCES · syscall: mkdir · errno: -13 exit 1 · code: E404 · 404: HTTP 404 from registry.npmjs.org exit 1 · code: ETARGET · notarget: no matching version for left-pad@99.99.99 Ablation confirmed: restoring the shape check fails the new test. --- src/update/job.ts | 68 ++++++++++++++++++++++++++++++++++------ tests/update-job.test.ts | 45 ++++++++++++++++++++++++++ 2 files changed, 104 insertions(+), 9 deletions(-) diff --git a/src/update/job.ts b/src/update/job.ts index 6115b9e0bd..a4528e6492 100644 --- a/src/update/job.ts +++ b/src/update/job.ts @@ -284,7 +284,8 @@ function withheldSummary(error: unknown): string { // an errno is an integer, so neither can carry arbitrary text. const parts = [`${name}${codeNote}`]; const syscall = (error as { syscall?: unknown } | null)?.syscall; - if (typeof syscall === "string" && /^[a-z][a-z0-9_]{1,20}$/.test(syscall)) parts.push(`syscall: ${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`); @@ -736,7 +737,53 @@ const NPM_CODE_RECORD = /^\s*npm\s+ERR!\s+code\s+([A-Z][A-Z0-9_]{2,})\s*$/gm; * found in: ` is dropped for the same reason. */ const NPM_FIELD_LINE = /^\s*npm\s+(?:error|ERR!)\s+([a-z0-9]+)\s+(.*)$/gim; -const NPM_SAFE_FIELDS = new Set(["code", "syscall", "errno", "notarget", "404", "401", "403", "409", "429"]); + +/** + * 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 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: keep the FACT and the package spec, never the surrounding prose. + // `No matching version found for left-pad@99.99.99.` -> `no matching version for left-pad@99.99.99` + notarget: value => { + const spec = /\b((?:@[\w.-]+\/)?[\w.-]+@[\w.\-+]+)/.exec(value); + return spec ? `no matching version for ${spec[1]}` : "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}`; } + // Registry hosts only, and only the host — the path can name a private scope, and userinfo + // is a credential. + const host = parsed.hostname; + if (!/^[\w.-]+$/.test(host) || parsed.username || parsed.password) return `HTTP ${field}`; + return `HTTP ${field} from ${host}`; +} /** * Extract the diagnostic fields npm names explicitly. @@ -750,13 +797,16 @@ function npmDiagnosticFields(text: string): string[] { for (const match of text.matchAll(NPM_FIELD_LINE)) { const field = match[1]!.toLowerCase(); const value = match[2]!.trim(); - if (!NPM_SAFE_FIELDS.has(field) || seen.has(field)) continue; - if (!value || value.length > 160) continue; - // `code` is additionally pinned to the recognized vocabulary; the rest only have to prove - // they carry no path. - if (field === "code" && !NPM_ERROR_CODES.has(value)) continue; - if (withholdIfPathBearing(value) !== value) continue; - seen.set(field, value); + 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}`); } diff --git a/tests/update-job.test.ts b/tests/update-job.test.ts index ac771dfb8a..4e375b12da 100644 --- a/tests/update-job.test.ts +++ b/tests/update-job.test.ts @@ -362,6 +362,51 @@ describe("GUI update execution decisions", () => { // 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"); + + // 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", () => { From e3dae0ebdb2ef5d3512eab6b253015c73c279ecc Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Fri, 7 Aug 2026 19:14:07 +0900 Subject: [PATCH 16/17] fix(update): echo only our own package spec and known registry hosts (round 16) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two things I treated as "safe shapes" that were not. A PACKAGE SPEC IS NOT A SAFE SHAPE. `name@version` also matches `jane.doe@example.com` and `JaneDoe@2.7.41`, so extracting "the spec" from a `notarget` line was itself a disclosure channel. This updater resolves exactly one package, so a spec is echoed only when it IS ours; everything else reports the bare fact. A HOSTNAME IS NOT A SAFE SHAPE. `^[\w.-]+$` accepts `janedoe.example`, a numeric host, and a punycode host. Knowing whether a 404 came from the public registry or somewhere else is the useful part, and that fits in an allowlist — four known registry hosts. Anything else reports the status alone. Also fixes the spec matcher itself: `PKG` is scoped (`@bitkyc08/opencodex`), so it needed escaping and a boundary that works with a leading `@` — `\b` does not. Verified our own spec is kept while an email is not. Every attack input from this round is a regression test. --- src/update/job.ts | 32 ++++++++++++++++++++++++-------- tests/update-job.test.ts | 14 ++++++++++++++ 2 files changed, 38 insertions(+), 8 deletions(-) diff --git a/src/update/job.ts b/src/update/job.ts index a4528e6492..6df5410e81 100644 --- a/src/update/job.ts +++ b/src/update/job.ts @@ -753,6 +753,13 @@ const POSIX_SYSCALLS = new Set([ ]); /** 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), @@ -760,10 +767,17 @@ const NPM_FIELD_VALIDATORS: Record string | null> = { 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: keep the FACT and the package spec, never the surrounding prose. - // `No matching version found for left-pad@99.99.99.` -> `no matching version for left-pad@99.99.99` + // Version resolution: the FACT only. + // + // An earlier version extracted the package spec, which looked safe and was not: + // `jane.doe@example.com` and `JaneDoe@2.7.41` both match "name@version". The one package + // this updater ever resolves is our own, so a spec is echoed only when it IS ours — + // everything else reports the bare fact. notarget: value => { - const spec = /\b((?:@[\w.-]+\/)?[\w.-]+@[\w.\-+]+)/.exec(value); + // PKG is scoped (`@scope/name`), so escape it rather than interpolating raw — and anchor on + // a boundary that works for a leading `@`, which `\b` does not. + const escaped = PKG.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); + const spec = new RegExp(String.raw`(?:^|\s)(${escaped}@[\w.\-+]+)`).exec(value); return spec ? `no matching version for ${spec[1]}` : "no matching version"; }, }; @@ -778,11 +792,13 @@ function npmHttpStatusValue(field: string, value: string): string | null { if (!url) return `HTTP ${field}`; let parsed: URL; try { parsed = new URL(url); } catch { return `HTTP ${field}`; } - // Registry hosts only, and only the host — the path can name a private scope, and userinfo - // is a credential. - const host = parsed.hostname; - if (!/^[\w.-]+$/.test(host) || parsed.username || parsed.password) return `HTTP ${field}`; - return `HTTP ${field} from ${host}`; + // 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}`; } /** diff --git a/tests/update-job.test.ts b/tests/update-job.test.ts index 4e375b12da..d9de16d176 100644 --- a/tests/update-job.test.ts +++ b/tests/update-job.test.ts @@ -394,6 +394,20 @@ describe("GUI update execution decisions", () => { // The one field that still reports does so as a fixed phrase with no borrowed text. expect(summary).toContain("no matching version"); + // A package spec is echoed only when it is OUR package. `name@version` matches an email + // address and any `Name@1.2.3`, so extracting "a spec" was itself a disclosure channel. + const ours = summarizeCommandOutput("", "npm error notarget No matching version found for @bitkyc08/opencodex@99.99.99", 1, null); + expect(ours).toContain("@bitkyc08/opencodex@99.99.99"); + const email = summarizeCommandOutput("", "npm error notarget No matching version found for jane.doe@example.com", 1, null); + expect(email).not.toContain("jane.doe"); + expect(email).not.toContain("example.com"); + + // 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, { From 821a845ab53c163a6d3535101c3e9c8e1dac3202 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Fri, 7 Aug 2026 19:16:45 +0900 Subject: [PATCH 17/17] fix(update): drop the package spec entirely from notarget (round 17) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pinning the package NAME to our own still left the VERSION free: `@bitkyc08/opencodex@99.99.99-JaneDoe` is a valid-looking spec, and a semver prerelease identifier can encode anything. That is the same lesson the `/healthz` version taught in round 13 — I applied it there and not here. There is no trusted resolved version available at this call site, so the spec is not rendered at all. `code: ETARGET · notarget: no matching version` already tells a user their requested version does not exist, which is the diagnostic that matters. Both attack inputs are regression tests. --- src/update/job.ts | 21 ++++++++++----------- tests/update-job.test.ts | 20 +++++++++++++------- 2 files changed, 23 insertions(+), 18 deletions(-) diff --git a/src/update/job.ts b/src/update/job.ts index 6df5410e81..abea4ee128 100644 --- a/src/update/job.ts +++ b/src/update/job.ts @@ -769,17 +769,16 @@ const NPM_FIELD_VALIDATORS: Record string | null> = { errno: value => (/^-?\d{1,10}$/.test(value) ? String(Number(value)) : null), // Version resolution: the FACT only. // - // An earlier version extracted the package spec, which looked safe and was not: - // `jane.doe@example.com` and `JaneDoe@2.7.41` both match "name@version". The one package - // this updater ever resolves is our own, so a spec is echoed only when it IS ours — - // everything else reports the bare fact. - notarget: value => { - // PKG is scoped (`@scope/name`), so escape it rather than interpolating raw — and anchor on - // a boundary that works for a leading `@`, which `\b` does not. - const escaped = PKG.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); - const spec = new RegExp(String.raw`(?:^|\s)(${escaped}@[\w.\-+]+)`).exec(value); - return spec ? `no matching version for ${spec[1]}` : "no matching version"; - }, + // 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", }; /** diff --git a/tests/update-job.test.ts b/tests/update-job.test.ts index d9de16d176..c8ae64de4b 100644 --- a/tests/update-job.test.ts +++ b/tests/update-job.test.ts @@ -394,13 +394,19 @@ describe("GUI update execution decisions", () => { // The one field that still reports does so as a fixed phrase with no borrowed text. expect(summary).toContain("no matching version"); - // A package spec is echoed only when it is OUR package. `name@version` matches an email - // address and any `Name@1.2.3`, so extracting "a spec" was itself a disclosure channel. - const ours = summarizeCommandOutput("", "npm error notarget No matching version found for @bitkyc08/opencodex@99.99.99", 1, null); - expect(ours).toContain("@bitkyc08/opencodex@99.99.99"); - const email = summarizeCommandOutput("", "npm error notarget No matching version found for jane.doe@example.com", 1, null); - expect(email).not.toContain("jane.doe"); - expect(email).not.toContain("example.com"); + // 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.