fix(agent): harden policy storage transactions - #1978
Open
Benoît Cortier (CBenoit) wants to merge 25 commits into
Open
Benoît Cortier (CBenoit) wants to merge 25 commits into
Benoît Cortier (CBenoit) wants to merge 25 commits into
Conversation
Benoît Cortier (CBenoit)
changed the base branch from
cbenoit-plan-agent-policy-endpoint
to
cbenoit-policy-caller-hardening
September 8, 2026 01:49
Copilot started reviewing on behalf of
Benoît Cortier (CBenoit)
September 8, 2026 01:50
View session
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Default-path migration, directory bootstrap, and probe races can violate availability and preservation guarantees.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Hardens Windows policy persistence against filesystem races and interrupted writes.
Changes:
- Adds handle-relative transactional create/replace and crash recovery.
- Secures and validates default/custom policy directories.
- Expands filesystem security and concurrency tests.
File summaries
| File | Description |
|---|---|
policy_store/windows.rs |
Implements secure Windows storage primitives and recovery. |
policy_store/mod.rs |
Integrates transactional persistence into policy management. |
policy_security.rs |
Adds managed-policy ACL, identity, and ancestor checks. |
Review details
- Files reviewed: 2/3 changed files
- Comments generated: 4
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Benoît Cortier (CBenoit)
force-pushed
the
cbenoit-policy-storage-hardening
branch
from
September 8, 2026 04:31
7190ec5 to
cddee20
Compare
Copilot started reviewing on behalf of
Benoît Cortier (CBenoit)
September 8, 2026 04:45
View session
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Failed probes can self-trigger repeated reloads, and legacy installations lose write capability without an in-band migration path.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 3/4 changed files
- Comments generated: 5
- Review effort level: Balanced
Copilot started reviewing on behalf of
Benoît Cortier (CBenoit)
September 8, 2026 05:08
View session
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Create transactions miss an ancestor-security check, and custom-path publication and authoritative state reporting have correctness issues.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
crates/now-package-broker/src/policy_security.rs:309
- The retained hosting-directory handle for configured paths omits
FILE_TRAVERSE, but this exact handle is later passed asFILE_RENAME_INFO.RootDirectorybyatomic_createandatomic_replace. Windows requires traverse access on that root handle; meanwhile the capability probe uses a separate handle that does requestFILE_TRAVERSE, so a custom path can be reportedWritableand then every handle-relative publication fails. IncludeFILE_TRAVERSEin these retained handles.
crates/now-package-broker/src/policy_store/mod.rs:130 - This authoritative re-observation discards its current write capability and read-only reason. The success path later copies those fields from the pre-write observation, so an ACL change after the post-write check but before this read can be observed as read-only while the response still advertises
Writable. Carry the authoritative observation's capability fields into the persisted result and success snapshot.
- Files reviewed: 4/5 changed files
- Comments generated: 1
- Review effort level: Balanced
Copilot started reviewing on behalf of
Benoît Cortier (CBenoit)
September 8, 2026 05:29
View session
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Crash recovery can discard the only verified policy copy, while unsafe probes can misclassify state or leave storage persistently read-only.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 4/5 changed files
- Comments generated: 2
- Review effort level: Balanced
Copilot started reviewing on behalf of
Benoît Cortier (CBenoit)
September 8, 2026 13:25
View session
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Pre-marker crashes remain unrecoverable, and watcher gaps can temporarily leave stale policy active.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 4/5 changed files
- Comments generated: 3
- Review effort level: Balanced
Benoît Cortier (CBenoit)
force-pushed
the
cbenoit-policy-storage-hardening
branch
from
September 8, 2026 13:47
e5aba80 to
8cb1f4b
Compare
Copilot started reviewing on behalf of
Benoît Cortier (CBenoit)
September 8, 2026 13:55
View session
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Transaction security digests are architecture-dependent, which can break crash recovery across 32-bit and 64-bit builds.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 4/5 changed files
- Comments generated: 1
- Review effort level: Balanced
Benoît Cortier (CBenoit)
force-pushed
the
cbenoit-policy-storage-hardening
branch
from
September 8, 2026 14:56
8c5e332 to
76d017d
Compare
Copilot started reviewing on behalf of
Benoît Cortier (CBenoit)
September 8, 2026 14:57
View session
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Recovery incorrectly rejects valid warning-bearing policies, and unread watcher fields fail warnings-denied Clippy.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 4/5 changed files
- Comments generated: 2
- Review effort level: Balanced
Copilot started reviewing on behalf of
Benoît Cortier (CBenoit)
September 8, 2026 15:19
View session
Benoît Cortier (CBenoit)
force-pushed
the
cbenoit-policy-storage-hardening
branch
from
September 15, 2026 15:37
bfdefdc to
5def35b
Compare
Benoît Cortier (CBenoit)
force-pushed
the
cbenoit-policy-storage-hardening
branch
from
September 16, 2026 11:15
5def35b to
e35e100
Compare
Preserve external policy changes by binding writes to retained file and directory identities and publishing with handle-relative no-replace renames. Recover interrupted replacements from durable verified markers and create the default policy tree under retained, reparse-safe ancestors with strict ACL and filesystem capability checks. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Keep unsafe path shapes distinct from unsupported file formats after replacing the path-based storage backend. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Preserve legacy policy continuity while making managed-store selection sticky and observable across startup and watcher races. Harden default-directory bootstrap, capability probing, and unsupported-filesystem reporting without weakening retained-handle publication or recovery. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Thread the original configured path through hardened writes and use it for authoritative post-write observation so self-generated watcher events keep stable store tokens. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Deny write and delete sharing during ordinary policy observations so pre-existing mutable handles cannot alter content after ACL verification. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Clarify managed and legacy default paths, bound failed capability probes, suppress probe watcher churn, and preserve the installer-owned migration boundary. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Revalidate ancestor evidence before Create publication, retain traversal rights on directory handles, and publish authoritative post-write capabilities. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Persist prepared replacement identity and security in durable markers before tombstoning, and retry bounded probe collisions without restart. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Recover verified pre-marker states, bind published replacements to durable identity evidence, bound probe retries, and refresh watchers without monitoring gaps. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Use architecture-independent security descriptor encoding and reject older transaction marker digest semantics during recovery. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Retain watcher handles explicitly and recover exact warning-bearing policies using the same validity criterion as normal activation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Digest the retained root-to-leaf ancestor identity sequence and reject dangling reparse or directory leaves before Missing-state classification. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Require a definitive destination collision before accepting the atomic rename capability probe. Preserve Win32 HRESULT and native NTSTATUS failure categories so sharing, permission, and unsupported errors cannot certify the filesystem. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Reopen probe leaves relative to the retained directory and compare file identities after a no-replace collision. This avoids rejecting equivalent Windows path representations while still detecting retargeted entries. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Inspect policy leaves without following reparses when their containing directory cannot be verified. Existing or ambiguous entries remain invalid, while only genuine absence is reported missing and non-writable. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Watch the verified canonical custom policy path so alternate configured path representations do not delay reloads. Refresh complete watcher sets without gaps while preserving the original configured path for secure writes and authoritative re-observation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Keep only the live watcher handles during canonical path refresh. Replacement sets still register completely before assignment, so failed refreshes preserve the active monitoring set without unused path bookkeeping. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Create and verify durable protected evidence before managed default policy activation succeeds. Restarts then remain bound to managed storage after its final policy disappears, without treating an untouched empty managed directory as authoritative over legacy policy. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Document the boundary between verified canonical paths used for storage and watching and the original configured path retained for authoritative re-observation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Use a versioned zero-length marker whose protected CREATE_NEW operation is already semantically complete. Existing valid markers remain authoritative, while invalid collisions are preserved and fail closed without staging or cleanup assumptions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Adopt PolicyFormatVersion fixtures and retain precise legacy diagnostics when observing secured policy files. Bind compatible format versions to validator receipts and exact persisted content to storage tokens. Keep interrupted Repair recoverable before marker publication even when the unchanged original is malformed or uses the legacy contract. Retain path and security checks without requiring valid original content. Installer conversion remains a separate migration step. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Use descriptive serialization expectations and structured tracing for privilege-limited recovery fixtures to satisfy workspace lint policy. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Release prepared replacement handles before recovery reopens the published policy in recovery validation tests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Create transaction files with the verified managed-directory owner when development agents run as an Administrator. Keep LocalSystem publication SYSTEM-owned and retain protected SYSTEM/Administrators-only DACLs. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Use the managed PackageBroker path as the sole default policy store. Remove legacy path arbitration, authority markers, fallback reads, and dual watching while preserving secure transactional publication. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Benoît Cortier (CBenoit)
force-pushed
the
cbenoit-policy-storage-hardening
branch
from
September 16, 2026 23:56
176a71c to
45036ce
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hardens managed policy persistence so writes remain bound to the exact file and directory state that was observed.
Retains the policy target and every directory component from root to leaf, rejects reparse ancestors and leaves, and publishes with handle-relative no-replace renames so concurrent administrator changes are preserved. Durable marker and tombstone state provides deterministic crash recovery without discarding the only valid policy copy.
Creates the default
Devolutions\PackageBrokertree component by component with protected ACLs, permits only compatible create-only grants on the shared vendor directory, and verifies custom NTFS/ReFS storage with nondestructive capability probes.