Windows port — Phase 2: privilege elevation#8
Conversation
…ardening, elevated process)
…T stub for Phase 3
…acOS after pkexec_helper split)
![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: b02e4e4d19
ℹ️ 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".
| let windir = std::env::var_os("WINDIR") | ||
| .map(std::path::PathBuf::from) | ||
| .unwrap_or_else(|| std::path::PathBuf::from("C:\\Windows")); | ||
| let zones = Zones(vec![windir.join("Temp")]); |
There was a problem hiding this comment.
On Windows, core_trash::validate canonicalizes each candidate's parent before checking starts_with, and std::fs::canonicalize returns a verbatim path such as \\?\C:\Windows\Temp. This zone is built from %WINDIR% as a normal C:\Windows\Temp, so even real %WINDIR%\Temp entries won't match and the elevated executor will return refusal reports instead of deleting them. Canonicalize/normalize the zone before constructing Zones.
Useful? React with 👍 / 👎.
| let tmp = std::env::temp_dir(); | ||
| let plan_path = tmp.join(format!("fyd-apply-{token}-plan.json")); | ||
| let report_path = tmp.join(format!("fyd-apply-{token}-report.json")); |
There was a problem hiding this comment.
When UAC elevation is approved with a different administrator account, std::env::temp_dir() here points at the elevated account's %TEMP%, while pkexec_helper wrote the staged plan under the unelevated user's %TEMP%. In that standard-user Windows context the child exits before reading the plan and the parent reports no helper output, so privileged cleanup does not work. Pass the staged path or use a shared location with the right ACLs instead of deriving it separately.
Useful? React with 👍 / 👎.
| Err(refusal) => refusal, | ||
| }; | ||
| let json = serde_json::to_string(&report).unwrap_or_default(); | ||
| if std::fs::write(&report_path, json).is_err() { |
There was a problem hiding this comment.
In the same-account UAC flow, this writes the elevated report into the unelevated user's writable %TEMP% using only the parent PID as the filename. A local process for that user can predict the path during the UAC prompt and replace it with a reparse point/link before the elevated child writes, turning the helper into an admin-context file overwrite. Use a non-predictable, securely created IPC file or an admin-owned location/handle instead of creating the report path from user-writable temp state.
Useful? React with 👍 / 👎.
| let token = std::process::id().to_string(); | ||
| let tmp = std::env::temp_dir(); | ||
| let plan_path = tmp.join(format!("fyd-apply-{token}-plan.json")); | ||
| let report_path = tmp.join(format!("fyd-apply-{token}-report.json")); |
There was a problem hiding this comment.
Because the token is just the process id, every overlapping root cleanup from the same Tauri process uses the same fyd-apply-<pid>-plan/report.json files. Tauri commands run on the blocking pool, so two execute requests can race: one request can overwrite the other's staged plan or report, causing the elevated child to delete the wrong batch or the parent to read the wrong result. Generate a unique nonce/tempfile for each invocation instead of reusing the process id.
Useful? React with 👍 / 👎.
| let windir = std::env::var_os("WINDIR") | ||
| .map(std::path::PathBuf::from) | ||
| .unwrap_or_else(|| std::path::PathBuf::from("C:\\Windows")); | ||
| let zones = Zones(vec![windir.join("Temp")]); |
There was a problem hiding this comment.
When the app is launched from an environment that overrides WINDIR, the elevated allowlist is no longer hard-coded: Microsoft documents that Start-Process inherits the current process environment by default (https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.management/start-process), and this child then trusts that inherited value to choose the root zone. That lets the caller steer admin deletion to a spoofed <WINDIR>\Temp tree instead of the real Windows temp; query the Windows directory from a trusted OS API or fixed system value before validating.
Useful? React with 👍 / 👎.
…-review) - apply_elevated: derive the root zone from a hard-coded, canonicalized C:\Windows\Temp instead of %WINDIR% — env vars propagate across same-user UAC elevation (attacker-influenceable) and an env-derived zone is an admin-delete EoP. Canonicalizing also fixes the \?\-verbatim mismatch that made privileged cleanup a silent no-op (zone never matched validate()'s canonical paths). - execute.rs: launch PowerShell by absolute System32 path (anti PATH-poisoning). - main.rs: --apply always terminates (missing token rejected by digit guard), never falls through to launching the elevated GUI. - core-trash: #[cfg(windows)] regression test for canonical zone matching, run on real Windows via a new 'cargo test -p core-trash' CI step (clippy is compile-only).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa0ffcb24d
ℹ️ 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".
| # zone-canonicalization regression guard for the elevated cleanup path. The | ||
| # clippy gate above is compile-only and cannot catch this runtime bug. | ||
| - name: test core-trash (Windows runtime) | ||
| run: cargo test -p core-trash |
There was a problem hiding this comment.
This new Windows step runs the entire core-trash test suite, but several in-zone tests still build zones with Zones(vec![dir.path().to_path_buf()]) (for example crates/core-trash/src/lib.rs:199-204 and 293-312). On Windows, validate() canonicalizes the candidate parent before starts_with, and the new regression comment documents that non-canonical zones do not match the resulting \\?\ paths, so these existing tests fail on windows-latest as soon as this job is enabled. Canonicalize the fixture zones or narrow the Windows test before adding this CI gate.
Useful? React with 👍 / 👎.
| // admin-delete EoP. Canonicalize the trusted constant so the zone matches | ||
| // validate()'s canonical (\\?\-verbatim) candidate paths; otherwise | ||
| // starts_with() never matches and every item is silently refused (no-op). | ||
| let zone_base = std::path::Path::new("C:\\Windows\\Temp"); |
There was a problem hiding this comment.
On Windows installations where the system directory is not C:\Windows, the scanner still emits the privileged root from %WINDIR%\Temp (crates/core-services/src/temp.rs:56-61), but the elevated executor only allows this hard-coded C:\Windows\Temp. In that environment every real system-temp candidate such as D:\Windows\Temp is refused by execute_root_plan, making privileged cleanup a no-op; resolve the Windows directory from a trusted OS source and use the same root for scanning and applying.
Useful? React with 👍 / 👎.
The Windows cargo-test step (added with the security fix) revealed that ALL core-trash zone checks fail on Windows: validate() canonicalizes candidates to \?\-verbatim paths, but every Zones is built from plain paths, so starts_with() never matched — refusing ALL deletions (user cleanup from Phase 1 AND the new privileged cleanup). Root fix: use dunce::canonicalize (strips the verbatim prefix on Windows; delegates to fs::canonicalize on Linux/macOS). apply_elevated now uses a plain hard-coded C:\Windows\Temp zone (matches dunce candidates; stays EoP-safe — not env-derived). Also made validate_rejects_outside_zone cross-platform (was /etc/passwd, Unix-only).
…idates on Windows + macOS /tmp)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02a5c61a70
ℹ️ 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".
|
|
||
| // Absolute path, not a PATH lookup: a PATH-poisoned powershell.exe could | ||
| // control the RunAs target shown in the UAC prompt. Fixed on Win10/11. | ||
| let status = Command::new("C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe") |
There was a problem hiding this comment.
On Windows installations where the OS is not on C:, this absolute C:\Windows\...\powershell.exe path does not exist, so every privileged cleanup request fails before UAC can launch the elevated --apply child. The allowlist has a separate hard-coded root issue, but this launch path independently breaks elevation even before validation; resolve PowerShell from a trusted system directory/API rather than assuming the system drive is C:.
Useful? React with 👍 / 👎.
1 participant
Phase 2 of the Windows 10/11 port (builds on Phases 0-1). Plan:
docs/superpowers/plans/2026-06-26-windows-port-phase-2-elevation.md.Goal: privileged cleanup (root-owned items, e.g.
%WINDIR%\Temp) works on Windows via a UAC-elevated headless self-relaunch — one UAC prompt per batch, the WebView never runs elevated.Changes
core_trash::execute_root_plan— shared validate-all-or-nothing + execute;privhelpernow uses it (single source of truth for the allowlist).--applyexecutor (headless::apply_elevated, cfg-gated): reads the staged plan, re-validates against hard-coded%WINDIR%\Temp, deletes, writes the report. Headless — dispatched before the Tauri builder.execute.rsWindowspkexec_helper: elevate viapowershell Start-Process -Verb RunAs -Waitself-relaunch; only the numeric parent PID is passed (both sides derive%TEMP%\fyd-apply-<pid>-{plan,report}.json).pkexec_smartWindows stub (Phase 3).Security: background review flagged a path-traversal on the
--applytoken → added a digit-only guard before path construction (elevated process never trusts its input); the PowerShell IPC is injection-proof (numeric PID only). Nounsafe, nowindowscrate this phase. No Linux/macOS behavior change (privhelper refactor preserves its stdout/exit-code contract).