monitor: add per-browser blocklist/allowlist consistency check

[kad]

After adding extensions to a blocklist, remove them from the matching browser's allowlist. Chrome, Edge, and Firefox lists are checked independently since extension IDs are not shared across browsers.

Runs at startup (after ProcessExistingPolicies) and on every live forcelist detection (PrintDiff). Allowlist keys that become empty after cleanup are deleted.

[Copilot]

Pull request overview

Adds a new EnforceBlockAllowlistConsistency function that ensures no extension ID present in a browser's blocklist also appears in that same browser's allowlist. The check runs per-browser (Chrome, Edge, Firefox independently) both at startup and on live forcelist detection.

Changes:

• New EnforceBlockAllowlistConsistency function in pkg/monitor/monitor.go that iterates allowlist subkeys, reads the corresponding blocklist live from the registry, and removes conflicting entries.
• Calls the consistency check from PrintDiff (after forcelist processing) and from runApp (after ProcessExistingPolicies, before broader allowlist cleanup).
• Updates newState.Subkeys to track newly created blocklist keys in the live state.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
pkg/monitor/monitor.go	Adds EnforceBlockAllowlistConsistency function and calls it from PrintDiff after forcelist processing; updates state to include new blocklist key.
cmd/WindowsBrowserGuard/main.go	Calls EnforceBlockAllowlistConsistency at startup between ProcessExistingPolicies and CleanupAllowlists.

[Copilot]

Pull request overview

Adds a startup + live-change post-processing step in the monitor to keep each browser’s allowlist consistent with its blocklist (removing allowlist entries that are also blocked, and deleting now-empty allowlist keys).

Changes:

• Call monitor.EnforceBlockAllowlistConsistency after ProcessExistingPolicies at startup.
• Call monitor.EnforceBlockAllowlistConsistency after live Chrome forcelist handling in PrintDiff.
• Add EnforceBlockAllowlistConsistency implementation that checks allowlist keys against same-browser blocklists and cleans up conflicts.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.

File	Description
pkg/monitor/monitor.go	Adds the consistency enforcement function and invokes it after live forcelist processing.
cmd/WindowsBrowserGuard/main.go	Runs the new consistency enforcement step during startup after existing-policy processing.

[Copilot]

The comment says this post-process step is “for this browser only”, but EnforceBlockAllowlistConsistency iterates over all allowlist keys in state (across all browsers). Either update the comment to reflect actual behavior, or change EnforceBlockAllowlistConsistency to accept a browser/path scope and only enforce for the relevant browser.

Suggested change

	// Post-process: for this browser only, verify no blocked
	// extension remains in the same-browser allowlist
	// (Chrome vs Chrome, Edge vs Edge - never cross-browser).
	// Post-process: verify that no blocked extension remains present
	// in any browser's allowlist as represented in newState (cross-browser
	// consistency across all known policy keys).

[Copilot]

registry.ReadKeyValues errors are treated the same as “blocklist is empty or absent”. This can silently skip enforcement and emit a misleading message when the read fails (e.g., access denied/corrupt key). Handle err separately: log/record the error and continue, and only treat it as empty when err == nil && len(blocklistValues) == 0.

[Copilot]

Same issue for allowlist reads: err != nil is currently reported as “Allowlist is empty”, which can hide real failures and skip cleanup. Split the cases so errors are surfaced (and enforcement can be skipped intentionally) instead of being treated as an empty allowlist.

Suggested change

	if err != nil || len(allowlistValues) == 0 {
	if err != nil {
	telemetry.Printf(ctx, " ❌ Failed to read %s allowlist at %s\\%s: %v\n", browser, keyPath, subkeyPath, err)
	// Skip enforcement for this browser on read failure, but surface the error.
	continue
	}
	if len(allowlistValues) == 0 {

[Copilot]

remaining, _ := registry.ReadKeyValues(...) ignores errors; if the read fails, remaining will be nil and len(remaining) == 0, which can trigger deletion of the allowlist key even though it might still contain values (or the failure was transient). Handle the error and only delete the key when err == nil and the key is confirmed empty.

[Copilot]

This function returns immediately when not running as Administrator, which means dry-run mode without admin won’t perform the consistency check or report planned removals. ProcessExistingPolicies explicitly supports non-admin dry-run by doing read-only + “[DRY-RUN]” operations; consider using the same pattern here (e.g., only require admin when canWrite is true, but still run the check when !canWrite).

Suggested change

	if !admin.IsAdmin() {
	return
	if canWrite && !admin.IsAdmin() {
	telemetry.Println(ctx, "[DRY-RUN] Not running as Administrator; enforcing blocklist/allowlist consistency in read-only mode")
	canWrite = false

[Copilot]

This uses fmt.Println() for output. The project convention is to log via telemetry.Printf/Println so output is consistently routed to stdout/log file/OTel; prefer telemetry.Println(ctx, "") (or similar) instead of fmt.Println().

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

[Copilot]

In dry-run mode (canWrite == false), registry.RemoveFromAllowlist(..., dryRun=true) returns nil without changing the registry, but the success log still says the ID was "Removed". This makes dry-run output misleading. Consider branching the success message on canWrite (e.g., "Would remove" vs "Removed") so logs accurately reflect whether a write occurred.

Suggested change

	telemetry.Printf(ctx, " ✓ Removed %s from %s allowlist\n", extID, browser)
	if canWrite {
	telemetry.Printf(ctx, " ✓ Removed %s from %s allowlist\n", extID, browser)
	} else {
	telemetry.Printf(ctx, " ✓ Would remove %s from %s allowlist (dry run)\n", extID, browser)
	}

[Copilot]

newState.Subkeys[blocklistKeyPath] = true is applied unconditionally, even if AddToBlocklist failed or never created the key. Because previousState is set to newState after PrintDiff, this can desync the in-memory snapshot from the real registry and potentially hide future diffs (e.g., a later successful creation would no longer appear as a subkey add). Consider only updating newState after confirming the key exists (or after at least one successful AddToBlocklist), or simply recapture state after writes instead of mutating the snapshot based on an assumption.

[Copilot]

The dry-run / privilege banner logic is inconsistent with how the function actually performs writes. When canWrite is false but the process is admin (e.g., --dry-run or write-disabled), the code falls into the non-dry-run banner, yet all registry mutations are still invoked with dryRun = !canWrite. Consider keying the banner strictly off canWrite (dry-run vs real) and using the admin check only to decide whether to skip when writes are required.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

[Copilot]

The post-write confirmation re-reads the entire blocklist key for every successfully added extension ID, which adds extra registry I/O on the hot path. Since AddToBlocklist already creates/opens the key and returns nil on success, consider marking the blocklist as present in newState immediately on successful AddToBlocklist (or only once per forcelistKeyPath) instead of calling ReadKeyValues just to confirm existence.

[Copilot]

ReadKeyValues returns an error when the blocklist key doesn't exist (common case). Treating that as a ❌ and recording it as an error will spam logs/telemetry and may obscure real failures. Consider handling missing blocklist keys as "empty/absent" (no enforcement needed) and only logging/recording errors for unexpected cases (e.g., access denied). This may require updating registry.ReadKeyValues to wrap the underlying RegOpenKeyEx error with %w so windows.ERROR_FILE_NOT_FOUND can be detected via errors.Is.

[Copilot]

At startup this consistency check runs immediately before CleanupAllowlists, which (when running as admin) deletes all allowlist keys anyway. This adds extra registry reads/writes and log output without changing the final registry outcome. Consider gating this call to scenarios where CleanupAllowlists is skipped (e.g., non-admin/dry-run) or removing it from the startup path if the intent is only to clean up during live forcelist handling.

Suggested change

	monitor.EnforceBlockAllowlistConsistency(ctx, keyPath, previousState, canWrite)
	// At startup this is only useful when we are not going to perform a full
	// allowlist cleanup (e.g., dry-run or lack of write privileges).
	if dryRun || !canWrite {
	monitor.EnforceBlockAllowlistConsistency(ctx, keyPath, previousState, canWrite)
	}

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

[Copilot]

PR description says the consistency check runs at startup after ProcessExistingPolicies, but the current startup wiring only calls EnforceBlockAllowlistConsistency when !canWrite (dry-run). Either update the description to match the behavior, or run this pass in write mode as well (or remove/adjust the later CleanupAllowlists step if the intent is to preserve non-conflicting allowlist entries).

[Copilot]

EnforceBlockAllowlistConsistency rereads and reopens the allowlist for every conflicting entry via registry.RemoveFromAllowlist, which itself calls ReadKeyValues again. This makes the enforcement pass O(n²) per allowlist and can get expensive if policies contain many entries. Consider collecting the matching value names once from allowlistValues and deleting them in a single key open (or adding a registry helper that removes multiple entries given the already-read values).

[Copilot]

After deleting an allowlist key recursively, the in-memory state cleanup only removes state.Subkeys[subkeyPath] and values with that prefix, but it does not remove any potential descendant subkeys under that allowlist path. Since DeleteRegistryKeyRecursive removes the full subtree, this can leave state.Subkeys inconsistent. Use registry.RemoveSubtreeFromState(state, subkeyPath) (and delete the root key) to ensure all descendants are removed from the captured state.

Suggested change

	delete(state.Subkeys, subkeyPath)
	for valName := range state.Values {
	if strings.HasPrefix(valName, subkeyPath+"\\") {
	delete(state.Values, valName)
	}
	}
	registry.RemoveSubtreeFromState(state, subkeyPath)
	delete(state.Subkeys, subkeyPath)

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

[Copilot]

The docstring/PR description say Chrome, Edge, and Firefox allowlist/blocklist consistency is enforced, but this function only iterates over keys containing "ExtensionInstallAllowlist" (Chrome/Edge). If Firefox consistency is in scope, add equivalent logic for Firefox policies; otherwise, update the comment/PR description to avoid implying Firefox coverage.

[Copilot]

totalConflicts is incremented when conflicts are detected, but the final "Resolved %d" message uses that same count even if removals fail or values are already gone. Consider tracking a separate "resolved" count based on successful deletions (or decrementing on failure) so the summary can't over-report remediations.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

[Copilot]

This function mutates the registry (deletes allowlist values / possibly the allowlist key) but does not update the in-memory state.Values to reflect deleted value names. Because the watcher re-arms notifications only after PrintDiff returns, these self-induced registry writes may not generate a subsequent capture, so previousState can become stale. After a successful non-dry-run deletion, remove the corresponding entries from state.Values (e.g., subkeyPath + "\\" + valueName) for each deleted value name; key deletion is already handled separately.

Suggested change

	// Keep the in-memory state in sync with the registry after successful deletions.
	// We only mutate state when we actually write to the registry (non-dry-run).
	if canWrite {
	for _, valueName := range deletedValueNames {
	fullValuePath := subkeyPath
	if fullValuePath != "" {
	fullValuePath += `\` + valueName
	} else {
	fullValuePath = valueName
	}
	delete(state.Values, fullValuePath)
	}
	}

[Copilot]

When reading the allowlist key live, ReadKeyValues can return windows.ERROR_FILE_NOT_FOUND if the key was deleted between the captured state and this enforcement pass (or by a concurrent policy update). This currently logs/records an error, but should be treated as a no-op (empty/absent allowlist) similar to the blocklist handling above (use errors.Is(err, windows.ERROR_FILE_NOT_FOUND) and continue).

Suggested change

	if err != nil {
	if err != nil {
	if errors.Is(err, windows.ERROR_FILE_NOT_FOUND) {
	telemetry.Printf(ctx, " ✓ Allowlist is empty or absent - no conflicts possible\n")
	continue
	}

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

[Copilot]

In dry-run mode this pass reads the blocklist live from the registry, but earlier steps (e.g., AddToBlocklist in dry-run) don’t actually write those entries. That means the consistency check can miss conflicts that would exist after the planned blocklist updates, despite printing “showing planned operations”. Consider simulating pending blocklist additions in-memory for dry-run (or passing the intended blocked IDs into this function) so the planned allowlist removals are accurately reported.

[Copilot]

The PR description says Firefox lists are checked independently, but this function explicitly excludes Firefox (and the code only handles Chromium allowlist/blocklist keys). Either update the PR description to match the implementation, or extend the consistency pass to cover Firefox’s equivalent allow/block policy keys (if applicable).

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

[Copilot]

New errors created here use %v formatting, which drops error wrapping and makes it harder for callers to inspect root causes with errors.Is/As. Prefer %w when returning an underlying error (e.g., from UTF16PtrFromString / RegOpenKeyEx) so the original error is preserved.

[Copilot]

plannedBlockedIDs are merged into blockedIDs unconditionally, but the docstring says planned IDs are only for dry-run. In non-dry-run mode this can treat an ID as blocked even if the blocklist write failed, causing allowlist entries to be removed based on planned (not actual) registry state. Consider only merging plannedBlockedIDs when !canWrite (dry-run), or track per-ID write success before adding it to the planned set used here.

Suggested change

	for plannedID := range plannedBlockedIDs[blocklistPath] {
	blockedIDs[plannedID] = true
	// In dry-run mode, also include IDs that are planned to be blocked but not yet written.
	if !canWrite {
	for plannedID := range plannedBlockedIDs[blocklistPath] {
	blockedIDs[plannedID] = true
	}

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

[Copilot]

In RemoveAllowlistValueNames, the UTF16PtrFromString conversion error is formatted with %v, which prevents unwrapping and is inconsistent with the rest of the new error-wrapping style (and with callers using errors.Is()). Use %w here as well so the underlying error can be matched/inspected by callers.

Suggested change

	return nil, fmt.Errorf("error converting key path: %v", err)
	return nil, fmt.Errorf("error converting key path: %w", err)