From 3b03ca69a153d62d196cf309603158c2b6b3ebcb Mon Sep 17 00:00:00 2001 From: Ingwannu Date: Sat, 1 Aug 2026 06:41:16 +0000 Subject: [PATCH] fix Windows ACL temp memo lifecycle --- src/config.ts | 21 ++++++- src/lib/windows-secret-acl.ts | 19 ++++++ src/server/management-auth.ts | 6 +- src/tray/windows.ts | 5 +- structure/02_config-and-codex-home.md | 11 ++++ tests/config.test.ts | 90 ++++++++++++++++++++++++++- tests/windows-secret-acl.test.ts | 36 +++++++++++ tests/windows-tray.test.ts | 3 +- 8 files changed, 181 insertions(+), 10 deletions(-) diff --git a/src/config.ts b/src/config.ts index 7ca2c0d4e..a8b301743 100644 --- a/src/config.ts +++ b/src/config.ts @@ -13,7 +13,12 @@ import { MAIN_CODEX_ACCOUNT_NAMESPACE_TARGET, } from "./codex/account-namespace-match"; import { COMBO_NAMESPACE, comboConfigIssues } from "./combos/types"; -import { hardenSecretDir, hardenSecretPath, hardenSecretPathAsync } from "./lib/windows-secret-acl"; +import { + forgetEphemeralSecretPath, + hardenSecretDir, + hardenSecretPath, + hardenSecretPathAsync, +} from "./lib/windows-secret-acl"; import { recordOwnedConfigPath } from "./lib/config-ownership"; import { assertNotRealHomeUnderTest } from "./lib/test-home-guard"; import { providerDestinationConfigError } from "./lib/destination-policy"; @@ -97,7 +102,7 @@ export function atomicWriteFile(path: string, content: string, io: AtomicWriteIO write: (target, value) => writeFileSync(target, value, { encoding: "utf-8", mode: 0o600 }), harden: target => { try { chmodSync(target, 0o600); } catch { /* platform may ignore chmod */ } - if (process.platform === "win32") hardenSecretPath(target, { required: true }); + if (process.platform === "win32") hardenSecretPath(target, { required: true, timeoutMemoKey: path }); }, rename: renameAtomicFile, truncate: target => truncateSync(target, 0), @@ -111,6 +116,7 @@ export function atomicWriteFile(path: string, content: string, io: AtomicWriteIO io.harden(tmp); hardened = true; io.rename(tmp, path); + forgetEphemeralSecretPath(tmp); } catch (cause) { let scrubbed = false; try { @@ -137,6 +143,7 @@ export function atomicWriteFile(path: string, content: string, io: AtomicWriteIO if (!removed && !hardened) { try { io.harden(tmp); hardened = true; } catch { /* zero-byte residual is reported honestly */ } } + if (removed) forgetEphemeralSecretPath(tmp); if (!removed) throw new AtomicWriteResidualTempError(tmp, hardened, { cause }); throw cause; } @@ -195,6 +202,7 @@ export async function atomicWriteFileAsync( await effective.harden(tmp); hardened = true; await effective.rename(tmp, path); + forgetEphemeralSecretPath(tmp); } catch (cause) { let scrubbed = false; try { @@ -221,6 +229,7 @@ export async function atomicWriteFileAsync( if (!removed && !hardened) { try { await effective.harden(tmp); hardened = true; } catch { /* zero-byte residual is reported honestly */ } } + if (removed) forgetEphemeralSecretPath(tmp); if (!removed) throw new AtomicWriteResidualTempError(tmp, hardened, { cause }); throw cause; } @@ -337,7 +346,10 @@ export function backupConfigBeforeOpenAiTierMigration( const scrubUnpublishedTemp = (): void => { cleanupAttempted = true; - if (!io.exists(temp)) return; + if (!io.exists(temp)) { + forgetEphemeralSecretPath(temp); + return; + } let scrubbed = false; try { io.truncate(temp); @@ -361,6 +373,7 @@ export function backupConfigBeforeOpenAiTierMigration( } } } + if (removed) forgetEphemeralSecretPath(temp); if (!removed && !scrubbed) throw new OpenAiTierBackupSecretResidualError(temp); if (!removed) throw new OpenAiTierBackupCleanupError(); }; @@ -381,9 +394,11 @@ export function backupConfigBeforeOpenAiTierMigration( published = true; try { io.unlink(temp); + forgetEphemeralSecretPath(temp); } catch { try { io.unlink(temp); + forgetEphemeralSecretPath(temp); } catch { // temp and backup are hard links to the same inode. Roll back the backup // link before any truncation so the downgrade snapshot is never zeroed. diff --git a/src/lib/windows-secret-acl.ts b/src/lib/windows-secret-acl.ts index fa898c3b2..424c5b561 100644 --- a/src/lib/windows-secret-acl.ts +++ b/src/lib/windows-secret-acl.ts @@ -170,6 +170,25 @@ export function resetHardenedStateForTests(): void { timedOutPaths.clear(); } +/** + * Drop path-scoped memo state only after an ephemeral file no longer exists. + * + * Atomic writers intentionally create a fresh pathname for every replacement. Keeping either a + * success or timeout memo for a renamed/unlinked temp turns those unique names into a process-life + * string leak on Windows. Stable destination and directory memos are not passed here and retain the + * existing fast path. + */ +export function forgetEphemeralSecretPath(targetPath: string): void { + hardenedPaths.delete(targetPath); + timedOutPaths.delete(`required:${targetPath}`); + timedOutPaths.delete(`optional:${targetPath}`); +} + +/** Test-only observability for proving ephemeral path memos return to baseline. */ +export function secretPathMemoCountsForTests(): { hardened: number; timedOut: number } { + return { hardened: hardenedPaths.size, timedOut: timedOutPaths.size }; +} + function effectivePlatform(): string { return platformOverride ?? platform; } diff --git a/src/server/management-auth.ts b/src/server/management-auth.ts index 164b843f0..523fb4fa3 100644 --- a/src/server/management-auth.ts +++ b/src/server/management-auth.ts @@ -2,6 +2,7 @@ import { randomBytes, randomUUID, timingSafeEqual } from "node:crypto"; import { chmodSync, closeSync, + existsSync, fsyncSync, linkSync, lstatSync, @@ -13,7 +14,7 @@ import { } from "node:fs"; import { dirname, join } from "node:path"; import { adminApiTokenFilePath } from "../lib/admin-secrets"; -import { hardenSecretDir, hardenSecretPath } from "../lib/windows-secret-acl"; +import { forgetEphemeralSecretPath, hardenSecretDir, hardenSecretPath } from "../lib/windows-secret-acl"; import type { OcxConfig } from "../types"; import { isAllowedManagementOrigin, @@ -97,7 +98,7 @@ function createTokenFile(path: string): string { closeSync(fd); fd = null; chmodSync(temporary, 0o600); - const temporaryHardened = hardenSecretPath(temporary, { required: true }); + const temporaryHardened = hardenSecretPath(temporary, { required: true, timeoutMemoKey: path }); if (!temporaryHardened.ok) { throw new Error( "management token temporary ACL hardening did not complete; set OPENCODEX_ADMIN_AUTH_TOKEN to use an environment token instead of a file-backed token", @@ -125,6 +126,7 @@ function createTokenFile(path: string): string { try { closeSync(fd); } catch { /* best effort */ } } removeBestEffort(temporary); + if (!existsSync(temporary)) forgetEphemeralSecretPath(temporary); } } diff --git a/src/tray/windows.ts b/src/tray/windows.ts index eec7251af..b7ac56231 100644 --- a/src/tray/windows.ts +++ b/src/tray/windows.ts @@ -5,7 +5,7 @@ import { homedir } from "node:os"; import { join, resolve } from "node:path"; import { expandUserPath, getConfigDir } from "../config"; import { durableBunPath } from "../lib/bun-runtime"; -import { hardenSecretDir, hardenSecretPath } from "../lib/windows-secret-acl"; +import { forgetEphemeralSecretPath, hardenSecretDir, hardenSecretPath } from "../lib/windows-secret-acl"; import { recordOwnedConfigPath } from "../lib/config-ownership"; const RUN_KEY = "HKCU\\Software\\Microsoft\\Windows\\CurrentVersion\\Run"; @@ -224,12 +224,13 @@ function replaceOwnedFile(path: string, contents: string | Buffer): void { try { try { chmodSync(temporary, 0o600); } catch { /* best-effort */ } if (process.platform === "win32") { - const hardened = hardenSecretPath(temporary, { required: true }); + const hardened = hardenSecretPath(temporary, { required: true, timeoutMemoKey: path }); if (!hardened.ok) throw new Error("Windows tray ACL hardening did not complete; refusing to persist executable state."); } renameSync(temporary, path); } finally { try { if (existsSync(temporary)) unlinkSync(temporary); } catch { /* best-effort */ } + if (!existsSync(temporary)) forgetEphemeralSecretPath(temporary); } } diff --git a/structure/02_config-and-codex-home.md b/structure/02_config-and-codex-home.md index af519117f..372f010f0 100644 --- a/structure/02_config-and-codex-home.md +++ b/structure/02_config-and-codex-home.md @@ -40,6 +40,17 @@ the recorded service ownership. `atomicWriteFile` uses a temp file named `{path}.ocx.{pid}.{seq}.tmp` (process ID + incrementing sequence number) to avoid collisions when concurrent writers (e.g. `ocx stop` and the proxy's own shutdown handler) both restore Codex config simultaneously. The temp is renamed atomically into place. +On Windows, ACL success/timeout memos for that unique temp pathname are released only after rename or +cleanup proves the temp no longer exists. Stable destination and directory memos remain cached, while +deleted one-shot names cannot accumulate for the life of the proxy. + +[Decision Log] +- 목적과 의도: Keep Windows ACL hardening fast without retaining every unique atomic-write pathname forever. +- 기존 구현 및 제약 조건: ACL results were memoized by path, while response-state and tray writers mint a new temp path for every replacement; a residual temp must keep its memo until later cleanup. +- 검토한 주요 대안: Remove ACL memoization entirely; cap the memo with arbitrary LRU eviction; release only a temp whose rename/unlink has proven it absent. +- 선택한 방식: Explicitly forget success and timeout memos after proven temp removal, while preserving stable-path and residual-file memos. +- 다른 대안 대신 이 방식을 선택한 이유: Removing or randomly evicting memos repeats expensive `icacls` work and may lose the residual-file fast path; lifecycle-owned release follows the real file lifetime exactly. +- 장점, 단점 및 영향: Repeated Windows state writes return memo counts to baseline; cleanup call sites must continue to release a memo whenever they add a new ephemeral hardened path. Response-state loading performs a bounded recovery pass for interrupted snapshot writes. It only matches regular files named `responses-state.json.ocx...tmp`, waits at least 15 diff --git a/tests/config.test.ts b/tests/config.test.ts index 1a8f5930a..330ebcbff 100644 --- a/tests/config.test.ts +++ b/tests/config.test.ts @@ -1,5 +1,16 @@ import { afterEach, beforeEach, describe, expect, spyOn, test } from "bun:test"; -import { existsSync, mkdirSync, mkdtempSync, readdirSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { + existsSync, + mkdirSync, + mkdtempSync, + readdirSync, + readFileSync, + renameSync, + rmSync, + truncateSync, + unlinkSync, + writeFileSync, +} from "node:fs"; import { homedir, tmpdir } from "node:os"; import { join, resolve } from "node:path"; import { @@ -27,7 +38,14 @@ import { } from "../src/config"; import * as windowsAcl from "../src/lib/windows-secret-acl"; -import { hardenConfigDir, hardenExistingSecret, renameAtomicFile, saveConfig } from "../src/config"; +import { + atomicWriteFile, + AtomicWriteResidualTempError, + hardenConfigDir, + hardenExistingSecret, + renameAtomicFile, + saveConfig, +} from "../src/config"; let testDir = ""; beforeEach(() => { @@ -87,6 +105,74 @@ function writeAccountNamespaceConfig( } describe("opencodex config defaults", () => { + test("atomic replacements release Windows ACL memos for renamed temp paths", () => { + const destination = join(testDir, "atomic.json"); + windowsAcl.setPlatformForTests("win32"); + windowsAcl.resetHardenedStateForTests(); + windowsAcl.setIcaclsRunnerForTests(() => ({ + success: true, + exitCode: 0, + timedOut: false, + stdout: "", + })); + process.env.USERNAME ??= "tester"; + + try { + for (let index = 0; index < 25; index += 1) { + atomicWriteFile(destination, String(index), { + write: (target, value) => writeFileSync(target, value, "utf-8"), + harden: target => { + windowsAcl.hardenSecretPath(target, { + required: true, + timeoutMemoKey: destination, + }); + }, + rename: renameSync, + truncate: target => truncateSync(target, 0), + unlink: unlinkSync, + }); + expect(windowsAcl.secretPathMemoCountsForTests()).toEqual({ hardened: 0, timedOut: 0 }); + } + } finally { + windowsAcl.setIcaclsRunnerForTests(null); + windowsAcl.setPlatformForTests(null); + windowsAcl.resetHardenedStateForTests(); + } + }); + + test("atomic write retains the ACL memo while a hardened residual temp remains", () => { + const destination = join(testDir, "residual.json"); + windowsAcl.setPlatformForTests("win32"); + windowsAcl.resetHardenedStateForTests(); + windowsAcl.setIcaclsRunnerForTests(() => ({ + success: true, + exitCode: 0, + timedOut: false, + stdout: "", + })); + process.env.USERNAME ??= "tester"; + + try { + expect(() => atomicWriteFile(destination, "secret", { + write: (target, value) => writeFileSync(target, value, "utf-8"), + harden: target => { + windowsAcl.hardenSecretPath(target, { + required: true, + timeoutMemoKey: destination, + }); + }, + rename: () => { throw Object.assign(new Error("rename failed"), { code: "EIO" }); }, + truncate: target => truncateSync(target, 0), + unlink: () => { throw Object.assign(new Error("unlink failed"), { code: "EPERM" }); }, + })).toThrow(AtomicWriteResidualTempError); + expect(windowsAcl.secretPathMemoCountsForTests()).toEqual({ hardened: 1, timedOut: 0 }); + } finally { + windowsAcl.setIcaclsRunnerForTests(null); + windowsAcl.setPlatformForTests(null); + windowsAcl.resetHardenedStateForTests(); + } + }); + test("atomic rename retries transient Windows sharing violations", () => { const sleeps: number[] = []; let attempts = 0; diff --git a/tests/windows-secret-acl.test.ts b/tests/windows-secret-acl.test.ts index 55c445474..9edc10156 100644 --- a/tests/windows-secret-acl.test.ts +++ b/tests/windows-secret-acl.test.ts @@ -14,10 +14,12 @@ import { existsSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { + forgetEphemeralSecretPath, hardenSecretDir, hardenSecretPath, hardenSecretPathAsync, resetHardenedStateForTests, + secretPathMemoCountsForTests, setAsyncIcaclsRunnerForTests, setIcaclsRunnerForTests, setNowForTests, @@ -446,6 +448,40 @@ describe("icacls failure paths (injected seams)", () => { expect(steps).toEqual(["grant-owner", "remove-inheritance", "remove-broad"]); }); + test("ephemeral success and timeout memos can be released after cleanup", () => { + const successfulTemp = secretFile("successful.tmp"); + setIcaclsRunnerForTests(() => ok); + expect(hardenSecretPath(successfulTemp, { required: true })).toEqual({ ok: true }); + expect(secretPathMemoCountsForTests()).toEqual({ hardened: 1, timedOut: 0 }); + + forgetEphemeralSecretPath(successfulTemp); + expect(secretPathMemoCountsForTests()).toEqual({ hardened: 0, timedOut: 0 }); + + const timedOutTemp = secretFile("timed-out.tmp"); + setIcaclsRunnerForTests(() => timeout); + expect(hardenSecretPath(timedOutTemp, { required: true }).ok).toBe(false); + expect(secretPathMemoCountsForTests()).toEqual({ hardened: 0, timedOut: 1 }); + + forgetEphemeralSecretPath(timedOutTemp); + expect(secretPathMemoCountsForTests()).toEqual({ hardened: 0, timedOut: 0 }); + }); + + test("forgetting a temp does not erase its stable destination timeout memo", () => { + const destination = join(testDir, "config.json"); + const temporary = secretFile("config.json.ocx.1.tmp"); + setIcaclsRunnerForTests(() => timeout); + + const result = hardenSecretPath(temporary, { + required: true, + timeoutMemoKey: destination, + }); + expect(result.ok).toBe(false); + expect(secretPathMemoCountsForTests()).toEqual({ hardened: 0, timedOut: 1 }); + + forgetEphemeralSecretPath(temporary); + expect(secretPathMemoCountsForTests()).toEqual({ hardened: 0, timedOut: 1 }); + }); + test("remove:g timeout after owner grant leaves explicit Full Control (#596)", () => { // Models the production strand: inheritance already removed, then a later step // times out. With owner-first ordering the writer still has an explicit ACE. diff --git a/tests/windows-tray.test.ts b/tests/windows-tray.test.ts index 2a9af6035..56dee1392 100644 --- a/tests/windows-tray.test.ts +++ b/tests/windows-tray.test.ts @@ -358,7 +358,8 @@ describe("Windows tray packaging and command safety", () => { expect(tray).toContain('join(getConfigDir(), "opencodex-tray.ps1")'); expect(tray).toContain('join(import.meta.dir, "assets", name)'); expect(tray).toContain("installedTrayIconPaths()"); - expect(tray).toContain("const hardened = hardenSecretPath(temporary, { required: true })"); + expect(tray).toContain("const hardened = hardenSecretPath(temporary, { required: true, timeoutMemoKey: path })"); + expect(tray).toContain("if (!existsSync(temporary)) forgetEphemeralSecretPath(temporary)"); expect(tray).toContain("if (!hardened.ok)"); expect(tray).toContain("if (!hardenedDir.ok)"); expect(tray).toContain("refusing to replace its persistent script");