Skip to content

fix(security): reject inherited writes at Windows creation parents - #8011

Merged
lidge-jun merged 3 commits into
lidge-jun:devfrom
luvs01:codex/propose-fix-for-inherited-acl-race
Nov 3, 2026
Merged

lidge-jun merged 3 commits into
lidge-jun:devfrom
luvs01:codex/propose-fix-for-inherited-acl-race

Conversation

@luvs01

@luvs01 luvs01 commented Oct 30, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Reject inherited write/create grants at Windows child-creation parents before a new child can be hardened. Keep inherit-only templates permitted on existing trusted ancestors, so a standard volume-root template does not reject an otherwise private descendant chain. Private leaf/directory validation remains unchanged.

Recovery drivers retain write-private for actual creation parents and private for created directories/leaves. The ACL change is limited to four files.

Current validation

Head: 966a1d78efab1499558c7e356e5940b61b614120.

  • Upstream Cross-platform CI 41086898751 and Catalog 41086898671 passed: 10 jobs succeeded; 11 policy-skipped jobs, including docs/native lanes, are not executed coverage.
  • Author-reported Windows 11 x64/Bun 1.4.4 evidence covers 83 distinct cases across the job-store, parent-async and Windows-pending suites. The first run passed 75 cases without an assertion failure before a 15-minute outer limit; a follow-up passed 13 cases / 58 assertions (eight unfinished cases plus five rechecks). This is not one uninterrupted full-run pass.
  • Native follow-up covered asynchronous ACL recovery, pending-inode adoption, publication refusal and child/Job lifetime fencing. Both new policy regressions passed; original-code RED/current-code GREEN evidence is separate.
  • Author-reported typecheck: 2,252 files; structure, privacy, file-size and diff checks passed. The file-size check was repeated after restoring missing tracked sparse-checkout roots.
  • No unresolved upstream inline threads are present. Independent source review reported no conflicting caller contract; explicit maintainer/security acceptance remains separate.

Scope and limitations

Native fixtures substitute trusted ancestry above their private root. Cross-account and ARM64 behavior remain unverified. Tests use isolated homes; the explicit real-home deny-list handoff was verified for the follow-up, not retrospectively for the first run. Full import-graph coverage ran in hosted CI due to local Windows execution cost.

Two baseline findings are handled separately: #8009 covers feature-toggle selection ownership and remains open; #8010 covers hosted-tool eligibility and was merged into dev. Neither fix is part of this four-file ACL head. No host ACL, live configuration, account or permission mode was changed.

Checklist

  • ACL scope and owning documentation are updated.
  • Exact-head upstream CI passed; bounded native evidence and limitations are recorded above.
  • Explicit maintainer/security merge acceptance.

Summary by CodeRabbit

  • Bug Fixes

    • Windows permission checks now distinguish inherited permission templates from permissions that apply directly to a folder. Inherited templates no longer authorize creating a child under a parent unless that parent meets the configured creation policy.
  • Documentation

    • Clarified how ownership and inherited permissions are handled by the available Windows ACL policies, including which policies apply to existing ancestors versus direct parents of newly created items.

@coderabbitai

coderabbitai Bot commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between 3cab4a7 and 966a1d7.

Files selected for processing: 4
  • src/lib/windows-owner-acl.ts
  • structure/codex-cli-update.md
  • structure/remote-link.md
  • tests/windows/windows-codex-cli-job-store.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review.

Walkthrough

The Windows ACL helper now applies different inherit-only ACE rules to write-private and trusted-ancestry. Documentation describes the policy distinction, and parser tests cover creation-parent checks and mixed-policy batches.

Changes: Windows ACL policy handling

Layer / File(s) Summary
ACL policy semantics and validation
src/lib/windows-owner-acl.ts, tests/windows/windows-codex-cli-job-store.test.ts, structure/codex-cli-update.md, structure/remote-link.md
write-private evaluates inherit-only ACEs, while trusted-ancestry skips them on existing ancestors. The code allows broad create rights under trusted-ancestry only for non-inherit-only ACEs of sticky trusted owners. Tests cover inherit-only grants and mixed-policy batches. Documentation describes the policies and the separate requirements for direct creation parents.

Priority: Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Possibly related issues

  • Issue 7872 in lidge-jun/opencodex: The ACL helper and Windows updater tests overlap with its parent-admission scope. This change addresses the creation-parent ACL portion, but does not establish completion of the synchronous recovery refactor or all follow-up items.
  • Issue 2811 in lidge-jun/opencodex: This change covers an ACL check within the broader update workflow. It does not implement the complete provenance-aware update manager.

Suggested reviewers: lidge-jun

Merge Risk: Minimal · up to 966a1

Windows ACL checks now reject inherited write/create grants at directories where new children are created, while inherit-only templates on existing trusted ancestors are still ignored. No concrete merge-blocking risk was established. A rare edge case with an explicit inherit-only CREATOR OWNER template may be refused, and it can be handled as a follow-up policy decision.

Pre-merge checks: Passed 5
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title names the main change: rejecting inherited write grants at Windows creation parents. The raw summary confirms the changes in windows-owner-acl.ts, the docs, and the tests all target that beh…
Docstring Coverage Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Title check

Explanation

The title names the main change: rejecting inherited write grants at Windows creation parents. The raw summary confirms the changes in windows-owner-acl.ts, the docs, and the tests all target that behavior. The title is 66 characters, which is within a reasonable length for a single-sentence title and uses the conventional fix(security) prefix.

Full details: Docstring Coverage

Explanation

Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 1 functions across 2 files. (2 skipped: 2 unsupported.)

Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the bug Something isn't working label Oct 30, 2026
@luvs01
luvs01 marked this pull request as ready for review October 30, 2026 16:20
@lidge-jun

Copy link
Copy Markdown
Owner

Maintainer integration into dev (account lidge-jun) at head 966a1d78efab1499558c7e356e5940b61b614120.

  • Exact-head required CI passed (ci aggregate, enforce-target, hygiene, and the test selection for this head).
  • Independent review (gpt-6.1-terra, high) merged this head onto current dev (c0e23c14c) in a scratch tree: frozen install and typecheck exit 0; the PR's focused and adjacent tests, the test-layout guards and the file-size ratchet all passed with 0 failures. Security-relevant paths were reviewed; no blocking finding.
  • No unresolved review threads and no changes-requested reviews.

Thanks for the fix.

@lidge-jun
lidge-jun merged commit 2bd1958 into lidge-jun:dev Nov 3, 2026
40 of 41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants