Protect unreadable registry values in the live backup/restore path

[devin-ai-integration]

Summary

This is the correct version of the destructive-restore fix attempted in #27 (and #28). Both of those patched Get-RegistrySnapshot, which has zero callers — the live backup path is New-Backup → Get-PolicySnapshot → Get-PolicyValue, so the guard never ran. Greptile flagged exactly this on #27 ("3/5").

What the live path did before: in Get-PolicyValue's registry branch, $key.GetValue(...) had no error handling. An unreadable value (e.g. access denied) threw and aborted the entire backup; and the dead snapshot function would have recorded existed=$false, which on restore means delete.

Fix, in the path that actually runs:

• Get-PolicyValue (registry) wraps the read in try/catch and returns a ReadError marker instead of throwing:

  catch { Write-Warning "...excluded from the backup..."; return @{ Exists=$false; Value=$null; Kind=$null; ReadError=$true } }
  

• Get-PolicySnapshot propagates it onto each entry as readError.
• Restore-RegistryBackup skips readError entries (back-compat: missing property → $false), so a value we couldn't read is left untouched rather than deleted:

  if ($readError) { Write-Warning "Skipping '$name' ... leaving the current value untouched."; continue }
  

• Removed the now-dead Get-RegistrySnapshot to avoid future confusion.

Notably, Remove-PolicyValue is left as-is (-ErrorAction SilentlyContinue); this avoids the spurious "failed to remove" warnings that #27's -ErrorAction Stop rewrite introduced on idempotent removes (the other issue Greptile flagged).

Supersedes #27. Test-PolicyManifest.ps1 and Test-Behavior.ps1 pass.

Link to Devin session: https://app.devin.ai/sessions/b703b48274874ae6b3df3015a1f741a7
Requested by: @osfv

[devin-ai-integration]

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

• Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
• Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

• Disable automatic comment, CI, and merge conflict monitoring

[greptile-apps]

Greptile Summary

This PR fixes a data-loss bug in the live Windows backup/restore path: when a registry value was unreadable (e.g. access denied), Get-PolicyValue would throw and abort the backup, and if the backup completed via the now-removed dead-code path Get-RegistrySnapshot, it would record existed=false and later delete the value on restore. The fix applies error handling exactly where the live path runs.

• Get-PolicyValue now wraps its registry Get-Item/GetValue calls in a try/catch that returns a ReadError=$true marker instead of throwing, and Get-PolicySnapshot propagates this flag into each backup snapshot entry.
• Restore-RegistryBackup checks the readError flag (with a back-compat PSObject.Properties default of $false) before the dry-run/apply branches, so any unreadable value is skipped and left untouched rather than deleted.
• The dead Get-RegistrySnapshot function (which had zero callers in the live path) is removed to prevent future confusion.

Confidence Score: 4/5

The fix correctly targets the live backup path and the restore guard is safe for both new and old backups. No registry values are deleted or overwritten by the new code paths.

The core logic is sound: the try/catch wraps the right operations, the readError flag propagates cleanly through the snapshot, and the back-compat PSObject.Properties guard means old backups restore without change. Two minor gaps exist: non-registry Get-PolicyValue branches omit ReadError from their return objects (relying on implicit null-to-false coercion in Get-PolicySnapshot), and Assert-BackupPolicyList does not enforce the invariant that readError=true entries must have existed=false, leaving a silent-skip risk for hand-crafted backups.

Both changed files are worth a second look: src/PlatformPolicy.ps1 for the implicit null-coercion in the non-registry snapshot path, and src/Backup.ps1 for the missing readError/existed consistency check in the schema validator.

Important Files Changed

Filename	Overview
src/PlatformPolicy.ps1	Removes dead Get-RegistrySnapshot; wraps the registry branch of Get-PolicyValue in try/catch to return a ReadError=$true marker on access-denied errors; Get-PolicySnapshot propagates readError into each snapshot entry. Non-registry return paths don't include ReadError, relying on implicit null-to-bool coercion in the snapshot builder — correct but implicit.
src/Backup.ps1	Adds readError guard in Restore-RegistryBackup before the dry-run/apply branches, using back-compat PSObject.Properties lookup (defaults to $false for old backups). Schema validator Assert-BackupPolicyList is not updated to enforce the readError=true → existed=false invariant, leaving a minor gap for hand-crafted backups.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["New-Backup()"] --> B["Get-PolicySnapshot(Target, PolicyNames)"]
    B --> C["For each policyName: Get-PolicyValue(Target, Name)"]
    C --> D{Target.Kind == Registry?}
    D -- Yes --> E{Test-Path Registry Key}
    E -- Exists --> F["try: Get-Item + GetValue"]
    F -- value != null --> G["Return {Exists=true, ReadError=false}"]
    F -- value == null --> H["Return {Exists=false, ReadError=false}"]
    F -- catch --> I["Write-Warning\nReturn {Exists=false, ReadError=true}"]
    E -- Missing --> H
    D -- No: JsonFile/macOS --> J["Return {Exists=..., ReadError=implicit null}"]
    G & H & I & J --> K["Snapshot entry: existed, value, kind, readError"]
    K --> L["Save to JSON backup file"]
    M["Restore-RegistryBackup()"] --> N["Assert-BackupObject"]
    N --> O["For each policy in backup"]
    O --> P{readError == true?}
    P -- Yes --> Q["Write-Warning: Skip, leave untouched"]
    P -- No --> R{DoApply?}
    R -- No --> S["Write-DryRun"]
    R -- Yes --> T{existed?}
    T -- Yes --> U["Set-PolicyValue"]
    T -- No --> V["Remove-PolicyValue"]

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A["New-Backup()"] --> B["Get-PolicySnapshot(Target, PolicyNames)"]
    B --> C["For each policyName: Get-PolicyValue(Target, Name)"]
    C --> D{Target.Kind == Registry?}
    D -- Yes --> E{Test-Path Registry Key}
    E -- Exists --> F["try: Get-Item + GetValue"]
    F -- value != null --> G["Return {Exists=true, ReadError=false}"]
    F -- value == null --> H["Return {Exists=false, ReadError=false}"]
    F -- catch --> I["Write-Warning\nReturn {Exists=false, ReadError=true}"]
    E -- Missing --> H
    D -- No: JsonFile/macOS --> J["Return {Exists=..., ReadError=implicit null}"]
    G & H & I & J --> K["Snapshot entry: existed, value, kind, readError"]
    K --> L["Save to JSON backup file"]
    M["Restore-RegistryBackup()"] --> N["Assert-BackupObject"]
    N --> O["For each policy in backup"]
    O --> P{readError == true?}
    P -- Yes --> Q["Write-Warning: Skip, leave untouched"]
    P -- No --> R{DoApply?}
    R -- No --> S["Write-DryRun"]
    R -- Yes --> T{existed?}
    T -- Yes --> U["Set-PolicyValue"]
    T -- No --> V["Remove-PolicyValue"]

Loading

Comments Outside Diff (2)

1. src/PlatformPolicy.ps1, line 123-151 (link)

   Non-registry Get-PolicyValue returns omit ReadError; Get-PolicySnapshot silently coerces $null → $false

   The JsonFile, MacOSDefaults, and MacOSPlist branches return objects without a ReadError property. Get-PolicySnapshot then evaluates [bool]$value.ReadError, where $value.ReadError resolves to $null on a pscustomobject with no such property, and [bool]$null is $false in PowerShell 5.1. The behavior is correct today — these platforms have no registry-style ACL issue and no error path sets ReadError = $true — but the implicit null-coercion makes the contract invisible to future maintainers. If someone adds error handling to the JsonFile branch and returns ReadError = $true, the skip-on-restore logic in Restore-RegistryBackup will fire for all platforms, not just Registry, which could silently omit entries from a non-Registry restore.

   

2. src/Backup.ps1, line 31-69 (link)

   Assert-BackupPolicyList does not validate the readError/existed invariant

   The live generator always produces entries where readError = true implies existed = false, but Assert-BackupPolicyList does not enforce this. A backup with readError = true alongside existed = true (and a valid kind/value) passes schema validation. During restore the entry is then silently skipped — the policy is present in the backup with a value, but it is never written back. The real risk is low because such an entry can only come from a hand-crafted backup file and the path is guarded by Assert-BackupRegistryPath, but adding a validation check — e.g., if ($policy.PSObject.Properties['readError'] -and [bool]$policy.readError -and $existed) { throw "..." } — would make the invariant explicit and catch any future generator bug early.

   

_{Reviews (1): Last reviewed commit: "Protect unreadable registry values in th..." | Re-trigger Greptile}

[osfv]

@greptileai can i merge w/o fixing these

[greptile-apps]

Yes, you can merge safely. Both flagged issues are P2 hardening gaps, not bugs:

1. Non-registry ReadError omission — behavior is correct today; non-registry platforms have no error path that sets ReadError = $true, so the implicit $null → $false coercion is harmless. It only becomes a risk if someone adds error handling to the JsonFile/macOS branches in the future.

2. Assert-BackupPolicyList invariant gap — only exploitable via a hand-crafted backup file with readError=true + existed=true. The live generator never produces that combination, so real users aren't affected.

The actual data-loss bug (the whole point of this PR) is correctly fixed. These two items would be good follow-up issues to track, but they're not blocking.