[WRONG BRANCH] fix(config): fail closed on Windows ACL hardening - #53
[WRONG BRANCH] fix(config): fail closed on Windows ACL hardening#53luvs01 wants to merge 1 commit into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughWindows config-directory ACL hardening now fails closed. Hardening errors propagate from ChangesConfig hardening
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
changed the title
⏳ DRAFT
What to do
Its title has been prefixed with |
|
✅ Deterministic PR hygiene checks passed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@tests/config.test.ts`:
- Around line 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.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a39f4df3-0c1c-4997-a1ff-5e9052288d59
📒 Files selected for processing (2)
src/config.tstests/config.test.ts
| 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); |
There was a problem hiding this comment.
🗄️ 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.
| 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.
1 participant
Motivation
Description
hardenSecretDir(dir, { required: true, ... })inconfigMutationDatabasePathso required directory ACL hardening now propagates failures instead of continuing.warnedConfigMutationDirectoryAclpath and add a clarifying comment that required hardening remains fail-closed.tests/config.test.tsto assert thatsaveConfigthrows on required ACL harden failure and that the config file is not created.src/config.tsandtests/config.test.tsand preserve the existing atomic writer and secret hardening semantics elsewhere.Testing
git diff --checkto verify no whitespace or trivial diff issues; it passed.bun test tests/config.test.ts tests/windows-secret-acl.test.tswhich passed (269 tests, 0 failed).bun run typecheckwhich completed successfully.bun run privacy:scanwhich reportedPrivacy scan passed.bun run testsuite; most tests ran and many passed, but the broad suite encountered a pre-existing unrelated 30s timeout in one test during that run (test harness timeout), not caused by this change.Codex Task
Summary by CodeRabbit