Conversation
…ge on Windows 10 - Locate the uninstaller before mutating the system; bail out early when missing - Only create the SystemApps stub when the folder does not already exist, and only delete it if this run created it, so the genuine legacy Edge package on Windows 10 is never removed - Clean up the stub in a finally block so no fake MicrosoftEdge.exe is left in SystemApps when the uninstall fails - Invoke the UninstallString directly via Start-Process instead of interpolating the raw registry value into a cmd.exe command line Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ory before elevated execution The launcher previously downloaded and unpacked the script into the user-writable %TEMP%\Win11Debloat folder and then executed it elevated, leaving a window in which any non-elevated process could swap the unpacked script files and have its code run with administrator rights behind the UAC prompt the user intended for Win11Debloat. The launcher now records the SHA-256 of the archive at download time and elevates a bootstrap that: - creates (or takes over) %ProgramData%\Win11Debloat and restricts it to Administrators and SYSTEM with inheritance disabled, neutralizing a pre-created attacker directory - copies the archive inside and re-verifies the hash after the copy, so a swapped archive aborts with an integrity error - unpacks, preserves LastUsedSettings.json, runs Win11Debloat.ps1 and cleans up entirely within the protected directory - migrates existing Config, Logs and Backups from the old temp location Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR hardens Edge removal and replaces direct temporary execution with an elevated bootstrap. The bootstrap uses protected ChangesEdge removal hardening
Elevated launcher bootstrap
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Launcher as Get.ps1
participant ElevatedPowerShell
participant Staging as ProgramData staging
participant Script as Staged debloat script
Launcher->>ElevatedPowerShell: Start encoded elevated bootstrap
ElevatedPowerShell->>Staging: Secure staging directory
ElevatedPowerShell->>Staging: Download and extract release
ElevatedPowerShell->>Script: Launch staged script
Script-->>ElevatedPowerShell: Return exit status
ElevatedPowerShell->>Staging: Retain files on failure or clean on success
ElevatedPowerShell-->>Launcher: Return bootstrap exit code
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Scripts/AppRemoval/Invoke-ForceRemoveEdge.ps1`:
- Around line 52-55: Update the Invoke-NonBlocking script block to capture the
Start-Process result with -PassThru, validate its ExitCode against the accepted
success codes, and propagate failure before cleanup or success output runs.
Preserve the stub cleanup by keeping it in the existing finally path.
In `@Scripts/Get.ps1`:
- Around line 247-255: Track deployment completion across the extraction,
release-folder validation, and file-movement flow surrounding Expand-Archive,
then add a finally block that restores ConfigOld whenever deployment did not
complete. Only remove ConfigOld after the configuration has been restored
successfully, and preserve the existing cleanup behavior for successful
deployments.
- Line 122: Update the launcher flow around $tempArchivePath and the referenced
cleanup/bootstrap sections to isolate concurrent invocations: generate a unique
archive path per run and use a named mutex or unique protected run directory to
serialize access to shared Config, Logs, and Backups data. Ensure cleanup cannot
remove files while another bootstrap or Win11Debloat.ps1 process is using them,
and release the synchronization resource on every exit path.
- Around line 175-178: Add comment-based help immediately above
Format-EmbeddedLiteral, including .SYNOPSIS, .PARAMETER Value, and .OUTPUTS
sections that describe wrapping the input in single quotes, escaping embedded
single quotes, and returning the resulting string.
- Around line 191-207: Update the staging-directory setup around $stagingRoot to
obtain the application-data location through a trusted Windows known-folder API
rather than relying on the predictable environment path. Before applying ACLs,
reject any existing reparse point in the resolved path and its relevant parent
components, then create and protect the staging directory without following
attacker-controlled links before the archive copy flow.
- Around line 281-288: Update the Win11Debloat invocation around Start-Process
to include -PassThru and retain the returned process handle while waiting for
completion. Read the handle’s ExitCode after the process finishes and throw when
it is nonzero, ensuring cleanup and subsequent bootstrap success handling do not
proceed after a Win11Debloat.ps1 failure.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3a172f7d-5ee1-4775-9685-0c627127ff1d
📒 Files selected for processing (2)
Scripts/AppRemoval/Invoke-ForceRemoveEdge.ps1Scripts/Get.ps1
- Check the Edge uninstaller exit code and skip shortcut/registry cleanup and the success message when it fails (stub cleanup still runs) - Restore preserved config files in a finally block so LastUsedSettings.json survives a failed unpack instead of being stranded in ConfigOld - Resolve ProgramData via the known-folder API instead of the environment, which a non-elevated process can override through HKCU\Environment - Refuse to follow a reparse point planted at the staging path; delete the link entry itself without touching its target - Serialize concurrent runs with a global mutex and use a unique per-run download archive name - Check the Win11Debloat.ps1 exit code and fail the bootstrap (keeping the staged files for inspection) instead of cleaning up after a failure - Add comment-based help to Format-EmbeddedLiteral Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Scripts/Get.ps1 (1)
369-375: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPropagate the elevated bootstrap exit code.
Line 371 waits for the bootstrap, but the launcher does not inspect
ExitCode. The bootstrap usesExit 1on failure at line 354.Scripts/Get.ps1therefore returns success after an integrity, extraction, or release-script failure.Store the exit code after
WaitForExit(). Remove the temporary archive, then exit with that nonzero code.Proposed fix
if ($null -ne $elevatedProcess) { $elevatedProcess.WaitForExit() + $bootstrapExitCode = $elevatedProcess.ExitCode } # Remove the downloaded archive from the user temp folder Remove-Item -LiteralPath $tempArchivePath -Force -ErrorAction SilentlyContinue + +if ($null -ne $elevatedProcess -and $bootstrapExitCode -ne 0) { + Exit $bootstrapExitCode +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Scripts/Get.ps1` around lines 369 - 375, Update the elevatedProcess handling in Scripts/Get.ps1 to capture its ExitCode immediately after WaitForExit(). Perform the existing temporary archive cleanup, then exit with the captured code so bootstrap failures from the elevated process are propagated while successful execution remains successful.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Scripts/Get.ps1`:
- Around line 225-250: Replace the path-based staging setup around
$existingStaging, New-Item, and Set-Acl with handle-based open-or-create logic
that refuses reparse points, so an attacker cannot swap $stagingRoot between
inspection and securing it. Verify the opened handle refers to the intended
directory, apply the Administrators/SYSTEM ACL through that handle, and retain
the handle through the subsequent archive-copy operation.
---
Outside diff comments:
In `@Scripts/Get.ps1`:
- Around line 369-375: Update the elevatedProcess handling in Scripts/Get.ps1 to
capture its ExitCode immediately after WaitForExit(). Perform the existing
temporary archive cleanup, then exit with the captured code so bootstrap
failures from the elevated process are propagated while successful execution
remains successful.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f3e25af4-fef1-4642-a8be-950df23c489e
📒 Files selected for processing (2)
Scripts/AppRemoval/Invoke-ForceRemoveEdge.ps1Scripts/Get.ps1
🚧 Files skipped from review as they are similar to previous changes (1)
- Scripts/AppRemoval/Invoke-ForceRemoveEdge.ps1
- Reject an existing staging directory that is not owned by Administrators or SYSTEM (a non-elevated user's pre-created folder is owned by that user), in addition to the existing reparse-point rejection, so a planted directory is never adopted - Re-verify the staging directory after the admin-only ACL is applied: since the ACL then blocks non-elevated writers, a still-valid non-reparse, admin-owned directory could not have been swapped during securing - Capture the elevated bootstrap ExitCode after WaitForExit and exit the launcher with it, so integrity, extraction, or script failures are no longer reported as success Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Scripts/Get.ps1 (2)
383-387: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReplace bootstrap tokens in one pass.
At Line 386, an argument value such as
__WINDOWSTYLE__is inserted into$bootstrap. Line 387 then replaces that argument value with the window style. An archive path containing__HASH__fails in the same way.Use a single-pass token matcher, or reject reserved placeholder values before substitution.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Scripts/Get.ps1` around lines 383 - 387, Update the bootstrap construction around $bootstrapTemplate and the chained Replace calls to avoid recursive token substitution: perform all placeholder replacements in a single pass using a token matcher, or validate and reject argument/path values containing reserved tokens such as __ARCHIVE__, __HASH__, __ARGS__, and __WINDOWSTYLE__ before replacement.
122-147: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winProtect the archive hash from pre-elevation replacement.
The non-elevated script hashes
$tempArchivePathbefore elevation, but an attacker can replace the downloaded archive in%TEMP%beforeGet-FileHashruns. The elevated bootstrap then copies that attacker archive while the recorded hash still matches it. Do not hash a file in%TEMP%that a non-elevated process can modify; compute the digest in the elevated process after copying into an owner-secured staging directory, or compare the archive against an independently obtained release digest/signature.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Scripts/Get.ps1` around lines 122 - 147, The current $archiveHash calculation in the download flow trusts a replaceable file in $env:TEMP before elevation. Remove this pre-elevation hash as the trust check, and make the elevated bootstrap copy the archive into an owner-secured staging directory before computing and validating its SHA256 digest; alternatively, validate against an independently obtained release digest or signature.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Scripts/Get.ps1`:
- Around line 383-387: Update the bootstrap construction around
$bootstrapTemplate and the chained Replace calls to avoid recursive token
substitution: perform all placeholder replacements in a single pass using a
token matcher, or validate and reject argument/path values containing reserved
tokens such as __ARCHIVE__, __HASH__, __ARGS__, and __WINDOWSTYLE__ before
replacement.
- Around line 122-147: The current $archiveHash calculation in the download flow
trusts a replaceable file in $env:TEMP before elevation. Remove this
pre-elevation hash as the trust check, and make the elevated bootstrap copy the
archive into an owner-secured staging directory before computing and validating
its SHA256 digest; alternatively, validate against an independently obtained
release digest or signature.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8f64336b-6cee-4e3f-9688-d125bf4d1125
📒 Files selected for processing (1)
Scripts/Get.ps1
…ging directory The launcher previously downloaded the archive to %TEMP% and hashed it before elevating. That hash was not a meaningful integrity check: an attacker able to replace the %TEMP% file does so before the hash is computed, so the recorded hash simply fingerprints the attacker's file. Copying that file into the protected directory and re-checking the same hash detects only disk corruption. The download now happens inside the elevated bootstrap, straight into the Administrators/SYSTEM-only staging directory over TLS. The archive is never written to a location a non-elevated process can modify, so it cannot be swapped between download and use; the TLS-authenticated GitHub download is the integrity boundary. The pre-elevation download, temp archive, and SHA-256 comparison are removed. Bootstrap inputs (dev flag, script arguments, window style) are now injected as a header of escaped single-quoted literals concatenated ahead of the static body, replacing the chained placeholder-token substitution. This removes the recursive-substitution bug where an input value containing a token string such as __HASH__ could be corrupted by a later replacement pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
Scripts/Get.ps1 (3)
379-394: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReturn failure when elevation does not start.
Start-Process ... -PassThrureturns$nullwhen elevation is declined or launch fails. The wait block is skipped,$bootstrapExitCodestays0, and the script exits successfully even though the bootstrap did not launch. Set the bootstrap result to nonzero before elevation, use-ErrorAction Stopintry/catch, report$null, and exit nonzero for process creation failures.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Scripts/Get.ps1` around lines 379 - 394, Update the elevated bootstrap launch around $elevatedProcess so its default exit status is nonzero before calling Start-Process. Use -ErrorAction Stop within try/catch, report cases where Start-Process returns $null, and preserve the existing WaitForExit/ExitCode handling for successful launches so any process-creation or elevation failure exits nonzero.
187-198: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAvoid blocking on a global
Mutex.WaitOne()before the protected staging directory exists.
Scripts/Get.ps1:187-193createsGlobal\Win11DebloatBootstrapbefore$stagingRootis created or secured, and the no-argumentWaitOne()blocks indefinitely until another process owns the sameGlobal\mutex. A non-elevated process can create or hold this fixed mutex in advance. Move the lock acquisition until after$stagingRootis secured, use a timeout, and fail with a clear error when the lock cannot be acquired.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Scripts/Get.ps1` around lines 187 - 198, The bootstrap mutex acquisition currently occurs before staging-root security and can block indefinitely on a globally pre-created mutex. In the outer bootstrap flow, move creation and waiting on $bootstrapMutex until after $stagingRoot is created and secured, replace the no-argument WaitOne() with a finite timeout, and emit a clear failure error when acquisition times out while preserving abandoned-mutex ownership handling.
346-351: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winDo not load profiles in the elevated child.
Line 348 starts
powershell.exewithout-NoProfile, so profile code can run elevated beforeWin11Debloat.ps1, outside the protected staging flow. Add-NoProfileto this child process.Proposed fix
-$debloatProcess = Start-Process powershell.exe -WindowStyle $debloatWindowStyle -Wait -PassThru -ArgumentList "-executionpolicy bypass -File `"$debloatScriptPath`" $scriptArgs" +$debloatProcess = Start-Process powershell.exe -WindowStyle $debloatWindowStyle -Wait -PassThru -ArgumentList "-NoProfile -ExecutionPolicy Bypass -File `"$debloatScriptPath`" $scriptArgs"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Scripts/Get.ps1` around lines 346 - 351, Add the PowerShell -NoProfile option to the Start-Process invocation for the elevated child running Win11Debloat.ps1, ensuring no user profiles execute before the staged script while preserving the existing arguments and process handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Scripts/Get.ps1`:
- Around line 379-394: Update the elevated bootstrap launch around
$elevatedProcess so its default exit status is nonzero before calling
Start-Process. Use -ErrorAction Stop within try/catch, report cases where
Start-Process returns $null, and preserve the existing WaitForExit/ExitCode
handling for successful launches so any process-creation or elevation failure
exits nonzero.
- Around line 187-198: The bootstrap mutex acquisition currently occurs before
staging-root security and can block indefinitely on a globally pre-created
mutex. In the outer bootstrap flow, move creation and waiting on $bootstrapMutex
until after $stagingRoot is created and secured, replace the no-argument
WaitOne() with a finite timeout, and emit a clear failure error when acquisition
times out while preserving abandoned-mutex ownership handling.
- Around line 346-351: Add the PowerShell -NoProfile option to the Start-Process
invocation for the elevated child running Win11Debloat.ps1, ensuring no user
profiles execute before the staged script while preserving the existing
arguments and process handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1f494a48-ce0f-4655-bae5-f5409e864436
📒 Files selected for processing (1)
Scripts/Get.ps1
|
Thanks for the reviews — pushed fixes for all findings:
On opening the directory via a native handle with |
|
Heya, Thanks for taking the time to contribute! I haven't had a chance to test or fully review the code yet, but I do have some initial thoughts.
In the current state this adds a lot of complexity and introduces a lot of potential issues.
Before we continue with this, would you mind splitting these into two separate PRs? Keeping each PR focused on a single change makes reviewing much easier and lets us merge them independently. |
Summary
Two security fixes found during a review of the codebase:
1.
Scripts/Get.ps1— unpacked script files were executed elevated from a user-writable directoryThe launcher downloaded and unpacked the archive into
%TEMP%\Win11Debloatand then ranWin11Debloat.ps1elevated from there. Between unpacking and elevation (and during the elevated run), any non-elevated process running as the same user could swap the unpacked.ps1files - whichWin11Debloat.ps1dot-sources - and get its own code executed with administrator rights behind the UAC prompt the user intended for Win11Debloat.The launcher now:
%ProgramData%\Win11Debloat, restricting it to Administrators + SYSTEM with inheritance disabled - this also neutralizes a pre-created attacker directoryLastUsedSettings.json, runsWin11Debloat.ps1and cleans up entirely inside the protected directoryConfig,LogsandBackupsfrom the old%TEMP%\Win11Debloatlocation on first runBehavior change: the working directory moves from
%TEMP%\Win11Debloatto%ProgramData%\Win11Debloat, which is inherent to the fix - the staging area must be somewhere a non-admin process cannot write.2.
Scripts/AppRemoval/Invoke-ForceRemoveEdge.ps1— could delete the real EdgeHTML package on Windows 10ForceRemoveEdgeunconditionally created a stub at%SystemRoot%\SystemApps\Microsoft.MicrosoftEdge_8wekyb3d8bbweand later recursively deleted that path. On Windows 10 this folder is the genuine legacy Edge (EdgeHTML) system package:New-Itemfailed non-terminally, execution continued, and the real system component was deleted.ForceRemoveEdgehas noMinVersiongate inFeatures.json, so nothing prevented this on Windows 10.Fixed by:
finallyblock so no fakeMicrosoftEdge.exeis left inSystemAppswhen the uninstall fails (previously it was left behind permanently when the uninstall registry key was missing)UninstallStringdirectly viaStart-Processinstead of interpolating the raw registry value into acmd.exe /ccommand lineTest plan
Scripts/Run-Tests.ps1)🤖 Generated with Claude Code
Summary by CodeRabbit
Scripts/Get.ps1against launcher TOCTOU attacks.%ProgramData%\Win11Debloatstaging directory.%TEMP%\Win11Debloat.finally.Format-EmbeddedLiteral.