fix(registry): stop deleting whole registry branches on a failed key parse - #261
Conversation
…parse reg.exe echoes long-form hive names (HKEY_LOCAL_MACHINE\...), but nine scans matched key headers against short forms (HKLM\...), so those matches never succeeded. The App Paths scan fell back to its container key when the match failed, so a single stale entry produced a delete of HKLM\...\CurrentVersion\App Paths itself — removing every App Paths registration on the machine rather than the one dead entry. The other eight scans guarded on the match and were silently dead, so they have never run in production. - normalize hive names in execReg output so key headers parse - drop the container-key fallbacks in the App Paths and shellex scans - refuse delete-key on hive roots and scan container keys at execution time - ship the eight revived scans deselected pending validation Refs #244
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8795b1f4fe
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const result = await execNativeUtf8('reg', args, opts) | ||
| return { ...result, stdout: normalizeHiveNames(result.stdout) } |
There was a problem hiding this comment.
Normalizing every reg response also makes the generic header matches in the ContextMenuHandlers, services, uninstall, and registered-client scans succeed for the first time, but those findings still use selected: true at lines 566/581, 802, 909, and 1041. On normal reg.exe output, a scan followed by the default fix action can therefore issue reg delete for heuristics that, according to the new safety comment, have never run against a real machine; apply UNVALIDATED_SCAN_SELECTED to these newly reachable delete-key findings as well.
Useful? React with 👍 / 👎.
| valueName: appName, | ||
| issue: `File association references unregistered app: ${appName}`, | ||
| risk: 'low', | ||
| selected: true, | ||
| selected: UNVALIDATED_SCAN_SELECTED, | ||
| fix: { op: 'delete-value' } |
There was a problem hiding this comment.
When an OpenWithList block contains a normal entry such as a REG_SZ firefox.exe, appMatch captures firefox.exe, and the revived scan stores that data as valueName. If the user opts into this finding, the fix consequently runs reg delete ... /v firefox.exe instead of deleting value a, so the operation fails and leaves the stale association intact; parse the value name and executable data separately.
Useful? React with 👍 / 👎.
| selected: UNVALIDATED_SCAN_SELECTED, | ||
| fix: { op: 'delete-key', key: parentTypeLibKey } |
There was a problem hiding this comment.
When a TypeLib GUID has multiple registered versions or both win32 and win64 entries, one missing win32 file causes this newly enabled scan to offer a fix whose explicit key is the entire HKCR\TypeLib\{GUID} parent. Opting into that finding deletes every valid sibling registration under the GUID, potentially breaking applications that use another version or architecture; target the affected version/leaf or verify that every registration below the GUID is stale before deleting the parent.
Useful? React with 👍 / 👎.
…scans
Two defects in scans this branch revives, both found in review.
TypeLib offered a fix whose key was the whole HKCR\TypeLib\{GUID} parent. A
GUID node holds every registered version and architecture — {00000300-...}
carries both 2.8 and 6.0 on a stock install — so one missing win32 file would
have deleted the surviving registrations. It now deletes the matched platform
key alone, and ignores a GUID node with no version segment below it.
OpenWithList stores the app in the value data under an ordinal value name
(`a REG_SZ firefox.exe`), but the scan matched REG_SZ alone and stored the
data as valueName, producing `reg delete /v firefox.exe` — a delete that can
only fail. It now parses name and data separately, checks every value in the
block rather than the first, and skips the MRUList ordering index.
|
Thanks — both P2s were real and are fixed in 746af3b. Pushing back on the P1. P2 — TypeLib parent delete ✅ fixedConfirmed on a stock Windows install: The fix targets the matched platform key alone ( P2 — OpenWithList value name ✅ fixedAlso confirmed against the real key: The app is the value data; the value name is an ordinal. Now parses name and data separately. Two related bugs fixed alongside it: the old code matched only the block's first P1 — newly reachable delete scans ❌ premise is incorrectThe claim is that normalization makes the ContextMenuHandlers, services, uninstall, and registered-client scans "succeed for the first time". They were already succeeding. Those four don't use short-form patterns — they match Measured against real So they were live and One thing the review missed that's worth noting: normalization does change the uninstall scan, just in the safe direction. Its |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 746af3b15b
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| fix: { op: 'delete-value' } | ||
| }) | ||
| } | ||
| entries.push({ |
There was a problem hiding this comment.
When an executable is registered only under HKCU\SOFTWARE\Microsoft\Windows\CurrentVersion\App Paths—a supported per-user App Paths location—the HKLM query fails and this catch reports its OpenWithList entry as obsolete. The user is therefore offered a fix that deletes a working association; query the HKCU location as well before creating the finding.
Useful? React with 👍 / 👎.
1 participant
Fixes a Registry Cleaner bug that could delete an entire registry branch, found while investigating #244.
The bug
reg.exeechoes results using long-form hive names (HKEY_LOCAL_MACHINE\...), but nine scans inregistry-cleaner.ipc.tsmatched key headers against the short form (HKLM\...). Those matches never succeeded — verified against realreg queryoutput, where the App Paths header pattern failed on 4/4 blocks.Eight of the nine scans guarded on the match (
if (keyMatch && ...)) and were silently dead. The App Paths scan did not. It fell back to the container key:So a single App Paths entry pointing at an uninstalled program produced:
…removing every App Paths registration on the machine instead of the one dead entry. The finding was pre-ticked and labelled
risk: 'low'. Stale App Paths entries are common on any machine that has uninstalled software, so this was reachable in ordinary use.The bug predates #68, so it has been present for a long time.
Changes
execRegoutput (normalizeHiveNames) so key headers parse. Only line-leading hive names are rewritten — value lines are indented four spaces, so registry data containing a hive name is untouched. This mirrorsnormalizeKeyPathalready used incontext-menu-cleaner.ipc.ts.isProtectedDeleteKey—delete-keyhard-refuses hive roots and the container keys the scans enumerate, at execution time, regardless of how the entry was produced. Defence in depth so a future parsing regression cannot wipe a branch again. Refusals surface as a normal per-entry failure.UNVALIDATED_SCAN_SELECTED). They have never executed in production and several delete whole COM registration keys (HKCR\CLSID\{…},HKCR\Interface\{…}), so they are opt-in per finding until validated on real machines.On that last point
Fixing the root cause activates eight scans that have never run. On this machine they now produce matches at scale — e.g. the COM Interface scan matches 28,630 blocks (capped at 30 findings). Landing those pre-ticked in a bugfix for a registry-corruption report would risk causing the next one, hence the deselect. Turning them on is a separate change that deserves its own validation.
Testing
New
registry-cleaner.scan.test.tsdrives the realscanRegistry/fixRegistryEntriesagainst verbatimreg.exeoutput:isProtectedDeleteKeyblocks hive roots and containers in both hive forms, and allows genuine leaf keysfixRegistryEntriesrefuses a container-key delete and issues noreg delete, but still deletes a real leaf keynpm test: 2229 passing. No new type errors (tscreports only pre-existing failures in unrelated files).Why this wasn't caught
registry-cleaner.ipc.test.tsreplicates the helpers under test rather than importing them, so a regex that never matched realreg.exeoutput could not fail a test. The new file imports the real module instead. Other replica-based suites in the repo likely share this blind spot — worth a follow-up.Note on #244
This does not explain that reporter's
PresentationCoreerror — that is a .NET Framework/GAC problem, and Kudu touches neither the GAC norC:\Windows\Microsoft.NET. Said so plainly in the issue and pointed them at .NET repair plus theirDocuments\Kudu Backups.regfiles. Fixing this bug is worthwhile on its own merits.Refs #244