Skip to content

fix(control): make anchor ACL verification locale-independent - #88

Merged
LogicDuke merged 1 commit into
repair/pr84-f1-control-anchor-ownerfrom
repair/pr85-f3-localized-icacls
Sep 7, 2026
Merged

fix(control): make anchor ACL verification locale-independent#88
LogicDuke merged 1 commit into
repair/pr84-f1-control-anchor-ownerfrom
repair/pr85-f3-localized-icacls

Conversation

@LogicDuke

@LogicDuke LogicDuke commented Sep 7, 2026

Copy link
Copy Markdown
Owner

Isolated D062 Amendment-B repair for PR #85 F3.

Finding:

  • CURRENT P2 availability defect.
  • control-anchor authorization depended on localized icacls footer/principal text.
  • valid localized Windows ACLs could fail closed as ACL_UNREADABLE or FOREIGN_PRINCIPAL.
  • parser-only repair was blocked because default icacls does not expose resolved well-known principals as canonical SIDs.

Adopted architecture amendment:

  • reuse the existing build-provenanced native Windows helper;
  • add one bounded READ-ONLY owner+DACL snapshot mode;
  • snapshot source is GetNamedSecurityInfoW with OWNER_SECURITY_INFORMATION | DACL_SECURITY_INFORMATION;
  • emit canonical SID data only;
  • no ACL write, owner mutation, shell, PowerShell, cmd, second executable, or network authority.

Repair:

  • authorization now uses one native owner+DACL snapshot as the security truth source;
  • owner must equal exact operator SID;
  • SYSTEM is recognized only by canonical SID S-1-5-18;
  • foreign/inherited/unknown/unrepresentable ACL data fails closed;
  • localized display names and English icacls footer text no longer participate in authorization;
  • icacls is removed from the control-anchor authorization path;
  • F1 owner semantics preserved;
  • F2 untouched.

Exact identity:

  • base:
    72340b3
  • head:
    b3e221a
  • one commit
  • five changed files
  • patch SHA256:
    2d54971fe38b52e52151b61fbed3407e9413d16fc4c0d67d5589e16e203c1f29
  • patch bytes:
    77769

Validation:

  • fresh independent validation PASS;
  • real Windows helper integration exercised;
  • helper build PASS;
  • focused control-store PASS;
  • F1 regression PASS;
  • F2 regression PASS;
  • tests/control 136/136 PASS;
  • full suite 2027/2027 PASS;
  • typecheck PASS;
  • lint PASS;
  • build PASS;
  • git diff --check PASS.

Deferred non-blocking carry obligation:

  • operator effective-rights type/access-mask semantics remain CURRENT/P3/NON-BLOCKING/CARRY_OBLIGATION;
  • pre-existing, not introduced or worsened by Amendment B;
  • deliberately not repaired in this PR;
  • recorded in AGENTBRIDGE_DEFERRED_FINDINGS_REGISTER.

Scope:

  • no authority widening beyond the explicitly adopted bounded read-only DACL snapshot;
  • no runtime/deployment mutation;
  • no F2 change.

🤖 Generated with Claude Code

https://claude.ai/code/session_01KE5bkGWp2jPzaJaLvFhPPu

Summary by CodeRabbit

  • New Features

    • Added locale-independent control-anchor security validation using canonical owner and access-control snapshots.
    • Added support for validating inherited permissions, deny rules, missing permissions, and malformed security data.
    • Added a read-only ACL snapshot mode for the Windows control helper.
  • Bug Fixes

    • Improved reliability across localized Windows environments.
    • Strengthened fail-closed handling for invalid, unexpected, or tampered security-helper responses.

Decision 062 Amendment B (PR #85 F3): replace the localized icacls DACL
read with a single build-provenanced native OWNER + DACL security-descriptor
snapshot emitted as canonical SIDs only, so control-anchor authorization is
identical on any Windows locale. SYSTEM is recognized solely as S-1-5-18 and
the operator by exact canonical SID; a display name can never enter the
decision. F1 owner-only mode and its provenance/hash gate are preserved; F2
is untouched. The deferred P3 (operator effective-rights type/mask semantics)
is intentionally out of scope and not repaired here.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KE5bkGWp2jPzaJaLvFhPPu
@coderabbitai

coderabbitai Bot commented Sep 7, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: 4d231109-6e5c-40c6-a068-5b29a271f634

📥 Commits

Reviewing files that changed from the base of the PR and between 72340b3 and b3e221a.

📒 Files selected for processing (5)
  • src/control/control-store.ts
  • tests/control/authority-boundary.test.ts
  • tests/control/control-store.test.ts
  • tests/control/owner-helper.win.test.ts
  • tools/control-owner/agentbridge-win-owner.c

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The control-anchor security gate now uses a canonical OWNER and DACL snapshot from one build-provenanced native helper. TypeScript parses and evaluates SIDs, verifies the snapshot, and removes the localized icacls subprocess.

Changes

Control-anchor snapshot gate

Layer / File(s) Summary
Snapshot contract and policy
src/control/control-store.ts, tests/control/control-store.test.ts
Adds canonical snapshot types, parsing, SID-only evaluation, rejection codes, bounds, and malformed-input tests.
Native snapshot helper
tools/control-owner/agentbridge-win-owner.c, tests/control/owner-helper.win.test.ts
Adds --acl mode with bounded OWNER and DACL output. Tests cover canonical SIDs, inherited ACEs, argument rejection, and provenance checks.
Control-anchor verification wiring
src/control/control-store.ts, tests/control/control-store.test.ts, tests/control/authority-boundary.test.ts
Renames verification APIs, invokes one snapshot helper, propagates snapshot failures, and removes icacls from the authority boundary.

Estimated code review effort: 4 (Complex) | ~60 minutes

Merge Risk: ⚪ Minimal · up to b3e22

The native snapshot contract, policy evaluation, provenance checks, and runtime wiring are consistently covered, with no actionable merge-blocking issue remaining.

Sequence Diagram(s)

sequenceDiagram
  participant verifyControlAnchor
  participant whoami
  participant verifyAnchorSnapshot
  participant SnapshotHelper
  verifyControlAnchor->>whoami: Read operator SID
  verifyControlAnchor->>verifyAnchorSnapshot: Verify anchor path and operator SID
  verifyAnchorSnapshot->>SnapshotHelper: Run --acl anchor path
  SnapshotHelper-->>verifyAnchorSnapshot: Return OWNER and DACL snapshot
  verifyAnchorSnapshot->>verifyAnchorSnapshot: Parse and evaluate canonical SIDs
  verifyAnchorSnapshot-->>verifyControlAnchor: Return success or rejection reason
Loading

Poem

A rabbit checks the SID-lined gate
One snapshot makes the record straight
No locale clouds the helper’s song
The DACL bounds are crisp and strong
Two trusted tools hop along

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: making control-anchor ACL verification independent of Windows locale.
Docstring Coverage ✅ Passed Docstring coverage is 85.71% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 21 functions across 5 files.
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.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch repair/pr85-f3-localized-icacls

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.

@LogicDuke
LogicDuke marked this pull request as ready for review September 7, 2026 05:22
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 7, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-07T05:24:10.124066Z b3e221a Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@LogicDuke

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Sep 7, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@LogicDuke
LogicDuke merged commit aaa7ddc into repair/pr84-f1-control-anchor-owner Sep 7, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant