Skip to content

[WRONG BRANCH] fix(config): fail closed on Windows ACL hardening #53

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
luvs01 wants to merge 1 commit into
base: main
Choose a base branch
from
+9 −18
Draft

[WRONG BRANCH] fix(config): fail closed on Windows ACL hardening #53

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 6 additions & 15 deletions src/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1949,7 +1949,6 @@ export function readConfigAdmissionSnapshot(): ConfigAdmissionSnapshot {

const CONFIG_MUTATION_DB_FILENAME = "config-mutation.sqlite";
const CONFIG_MUTATION_DB_SIDECARS = ["-journal", "-wal", "-shm"] as const;
let warnedConfigMutationDirectoryAcl = false;

export class ConfigMutationLockError extends Error {
readonly code = "CONFIG_MUTATION_LOCK_UNAVAILABLE";
Expand All @@ -1971,20 +1970,12 @@ function configMutationDatabasePath(): string {
try { chmodSync(dir, 0o700); } catch { /* best-effort on existing dir */ }
}
if (windowsSecretAclApplies()) {
try {
// Distinct timeout memo from management-token directory harden: a required
// management-dir timeout must not poison config mutation on the same home
// (windows-latest server-management-auth cases).
hardenSecretDir(dir, { required: true, timeoutMemoKey: `${dir}::config-mutation` });
} catch (error) {
if (!warnedConfigMutationDirectoryAcl) {
warnedConfigMutationDirectoryAcl = true;
const diagnostics = error instanceof Error ? error.message : "ACL hardening failed";
console.warn(
`[opencodex] Config mutation coordination directory ACL hardening did not complete; continuing without it. ${diagnostics}`,
);
}
}
// Distinct timeout memo from management-token directory harden: a required
// management-dir timeout must not poison config mutation on the same home
// (windows-latest server-management-auth cases). Required hardening remains
// fail-closed so config and credential writes are never published in a
// directory whose inherited ACLs could not be restricted.
hardenSecretDir(dir, { required: true, timeoutMemoKey: `${dir}::config-mutation` });
}
const path = join(dir, CONFIG_MUTATION_DB_FILENAME);
recordOwnedConfigPath(dir, path);
Expand Down
6 changes: 3 additions & 3 deletions tests/config.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1903,16 +1903,16 @@ describe("config.ts – Windows ACL hardening integration", () => {
}
});

test("saveConfig degrades when config-mutation directory hardening fails on win32", () => {
test("saveConfig fails closed when config-mutation directory hardening fails on win32", () => {
const origPlatform = process.platform;
Object.defineProperty(process, "platform", { value: "win32", configurable: true });
try {
const spy = spyOn(windowsAcl, "hardenSecretDir").mockImplementation((_path, opts) => {
if (opts?.required) throw new Error("ACL hardening failed: access denied");
return { ok: true };
});
expect(() => saveConfig(getDefaultConfig())).not.toThrow();
expect(existsSync(getConfigPath())).toBe(true);
expect(() => saveConfig(getDefaultConfig())).toThrow(/ACL hardening failed/);
expect(existsSync(getConfigPath())).toBe(false);
Comment on lines +1906 to +1915

@coderabbitai coderabbitai Bot Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Cover the isolated timeout memo in the regression test.

The test verifies required: true and the absent config file. It does not verify the timeoutMemoKey passed by src/config.ts Line 1978. If that key regresses to the directory path, a required management-token timeout can poison config mutations while this test still passes. Assert that the spy receives a key ending in ::config-mutation.

Suggested assertion 
       expect(existsSync(getConfigPath())).toBe(false);
+      expect(spy).toHaveBeenCalledWith(
+        expect.any(String),
+        expect.objectContaining({
+          required: true,
+          timeoutMemoKey: expect.stringMatching(/::config-mutation$/),
+        }),
+      );
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
test("saveConfig fails closed when config-mutation directory hardening fails on win32", () => {
const origPlatform = process.platform;
Object.defineProperty(process, "platform", { value: "win32", configurable: true });
try {
const spy = spyOn(windowsAcl, "hardenSecretDir").mockImplementation((_path, opts) => {
if (opts?.required) throw new Error("ACL hardening failed: access denied");
return { ok: true };
});
expect(() => saveConfig(getDefaultConfig())).not.toThrow();
expect(existsSync(getConfigPath())).toBe(true);
expect(() => saveConfig(getDefaultConfig())).toThrow(/ACL hardening failed/);
expect(existsSync(getConfigPath())).toBe(false);
test("saveConfig fails closed when config-mutation directory hardening fails on win32", () => {
const origPlatform = process.platform;
Object.defineProperty(process, "platform", { value: "win32", configurable: true });
try {
const spy = spyOn(windowsAcl, "hardenSecretDir").mockImplementation((_path, opts) => {
if (opts?.required) throw new Error("ACL hardening failed: access denied");
return { ok: true };
});
expect(() => saveConfig(getDefaultConfig())).toThrow(/ACL hardening failed/);
expect(existsSync(getConfigPath())).toBe(false);
expect(spy).toHaveBeenCalledWith(
expect.any(String),
expect.objectContaining({
required: true,
timeoutMemoKey: expect.stringMatching(/::config-mutation$/),
}),
);
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/config.test.ts` around lines 1906 - 1915, Update the saveConfig
regression test’s hardenSecretDir spy assertion to verify that the timeout memo
key argument ends with “::config-mutation,” while preserving the existing
required-error and absent-config assertions.

spy.mockRestore();
} finally {
Object.defineProperty(process, "platform", { value: origPlatform, configurable: true });
Expand Down
Loading