Skip to content

Fix destructive restore from unreadable registry values - #28

Merged
osfv merged 1 commit into
mainfrom
devin/1782395858-fix-registry-snapshot-destructive-restore
Jun 25, 2026
Merged

Fix destructive restore from unreadable registry values#28
osfv merged 1 commit into
mainfrom
devin/1782395858-fix-registry-snapshot-destructive-restore

Conversation

@devin-ai-integration

Copy link
Copy Markdown
Contributor

Summary

Fixes a latent P1 Greptile flagged on PR #22 that survived the #25 modular refactor and still lives in main (now in src/PlatformPolicy.ps1).

Get-RegistrySnapshot builds the backup of current registry state. When reading a value threw (e.g. access denied), the catch block recorded existed = $false:

  catch {
-     $entry.existed = $false   # then [void]$snapshot.Add($entry)
+     Write-Warning "Could not read registry value '$policyName' ... excluded from the backup"
+     continue                  # omit the entry entirely
  }

The problem is Restore-RegistryBackup treats existed = $false as "this value did not exist before" and removes it:

if ($existed) { Set-PolicyValue ... }   # restore
else          { Remove-PolicyValue ... } # delete

So a value that genuinely existed but was merely unreadable at backup time would be silently deleted on a later restore. Since we can't distinguish "absent" from "unreadable", the fix omits the entry from the snapshot (and warns), leaving it untouched on restore.

Note: the other finding on PR #22 (profile-enumeration switching to -ErrorAction Stop) does not apply here — main already uses the safer per-item SilentlyContinue skip, which is the behavior Greptile preferred.

Validation

  • scripts/Test-PolicyManifest.ps1 — passes.
  • scripts/Test-Behavior.ps1 — passes.

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

@osfv osfv self-assigned this Jun 25, 2026
@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

🤖 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-apps Bot commented Jun 25, 2026

Copy link
Copy Markdown

Greptile Summary

This PR fixes a latent data-destructive bug in Get-RegistrySnapshot: when reading a registry value threw an exception (e.g., access denied), the old code recorded existed = $false, causing Restore-RegistryBackup to delete that value on restore — even though it may have genuinely existed. The fix omits the unreadable entry from the snapshot entirely and emits a Write-Warning, so the value is never touched by a later restore.

  • Core change: replaces $entry.existed = $false in the catch block with Write-Warning … ; continue, removing the unsafe "treat unreadable as absent" assumption.
  • Restore path is unaffected: Restore-RegistryBackup iterates only over entries present in the backup JSON, so an omitted entry is simply skipped — no deletion, no corruption.

Confidence Score: 5/5

Safe to merge — the change is a single, targeted catch-block fix that removes a data-destructive code path with no collateral risk to the rest of the backup/restore flow.

The fix is surgical: one catch block, one loop, one clear invariant restored. The continue correctly bypasses snapshot.Add, Restore-RegistryBackup only iterates over entries that are actually in the backup JSON, and the Write-Warning gives operators visibility when values are excluded. PowerShell 5.1 continue semantics in a foreach loop are unambiguous, the warning message correctly surfaces $_.Exception.Message, and no other function in the backup/restore pipeline is touched.

No files require special attention.

Important Files Changed

Filename Overview
src/PlatformPolicy.ps1 Surgical fix in Get-RegistrySnapshot catch block: unreadable values are now omitted from the backup (with a warning) instead of being falsely recorded as absent, preventing destructive deletes on restore.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant Caller
    participant GRS as Get-RegistrySnapshot
    participant Reg as Registry Key
    participant RRB as Restore-RegistryBackup

    Caller->>GRS: PolicyNames[]
    loop for each policyName
        GRS->>Reg: GetValue(policyName, null)
        alt value read OK
            Reg-->>GRS: value or null
            GRS->>GRS: Add entry with existed flag
        else throws - access denied
            Reg-->>GRS: exception
            Note over GRS: OLD: existed=false, add entry
            Note over GRS: NEW: Write-Warning and continue - entry omitted
        end
    end

    Caller->>RRB: BackupPath
    loop for each policy in backup
        alt "existed = true"
            RRB->>Reg: Set-PolicyValue - restore
        else "existed = false - OLD path only"
            RRB->>Reg: Remove-PolicyValue - DESTRUCTIVE BUG
        else entry absent - NEW path
            Note over RRB: Not in backup, skipped and untouched
        end
    end
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"}}}%%
sequenceDiagram
    participant Caller
    participant GRS as Get-RegistrySnapshot
    participant Reg as Registry Key
    participant RRB as Restore-RegistryBackup

    Caller->>GRS: PolicyNames[]
    loop for each policyName
        GRS->>Reg: GetValue(policyName, null)
        alt value read OK
            Reg-->>GRS: value or null
            GRS->>GRS: Add entry with existed flag
        else throws - access denied
            Reg-->>GRS: exception
            Note over GRS: OLD: existed=false, add entry
            Note over GRS: NEW: Write-Warning and continue - entry omitted
        end
    end

    Caller->>RRB: BackupPath
    loop for each policy in backup
        alt "existed = true"
            RRB->>Reg: Set-PolicyValue - restore
        else "existed = false - OLD path only"
            RRB->>Reg: Remove-PolicyValue - DESTRUCTIVE BUG
        else entry absent - NEW path
            Note over RRB: Not in backup, skipped and untouched
        end
    end
Loading

Reviews (1): Last reviewed commit: "Do not back up unreadable registry value..." | Re-trigger Greptile

@osfv
osfv merged commit c5cb621 into main Jun 25, 2026
5 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