Keep winapp usable with restricted filesystem access - #873
Nikola Metulev (nmetulev) wants to merge 18 commits into
Conversation
Keep operational state independent of cache overrides, preserve coordination, and share the physical root across packaged and unpackaged processes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The target state root lacks local-user ACL hardening and returns a misleading stable error code when path resolution fails.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Moves UI coordination and Sandbox state into a cache-independent user-profile state root.
Changes:
- Adds shared local-path validation and mapped-drive rejection.
- Relocates UI and target state.
- Updates tests and documentation.
File summaries
| File | Description |
|---|---|
WinappDirectoryService.cs |
Resolves and validates shared state paths. |
InteractiveDesktopPaths.cs |
Moves UI coordination state. |
NativeMethods.txt |
Adds drive-type APIs. |
PathSafety.cs |
Rejects mapped network drives. |
TargetStateDirectoryProvider.cs |
Moves Sandbox state. |
WinappDirectoryServiceTests.cs |
Tests profile-state resolution. |
TargetStateDirectoryProviderTests.cs |
Tests target-root behavior. |
InteractiveDesktopStoreTests.cs |
Tests cache-independent UI state. |
InteractiveDesktopPathsHardeningTests.cs |
Tests permission isolation. |
docs/usage.md |
Documents shared runtime state. |
docs/ui-automation.md |
Links UI coordination documentation. |
docs/sandbox-execution.md |
Links Sandbox state documentation. |
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Build Metrics ReportValidation did not pass. Artifacts were uploaded before validation finished; check the workflow run before using them. Binary Sizes
.NET Test Results (TRX reports)Other suites are reflected in the overall validation status above. ❌ 8126 passed, 1 failed, 38 skipped out of 8165 tests in 1386.4s (+292 tests, +97.1s vs. baseline) Test Coverage✅ 86.3% line coverage, 80.9% branch coverage · ✅ +0.3% vs. baseline CLI Startup Time58ms median (x64, Try This BuildInstalls the MSIX for your architecture, replacing any previously installed build. Needs the GitHub CLI — the command offers to install it and sign you in if it is missing. & ([scriptblock]::Create((irm https://raw.githubusercontent.com/microsoft/winappCli/main/scripts/winapp-pr.ps1))) 873Switching between builds often?Put the tool on your PATH once: & ([scriptblock]::Create((irm https://raw.githubusercontent.com/microsoft/winappCli/main/scripts/winapp-pr.ps1))) -AddToPathThen this build is just: winapp-pr 873Run Updated 2026-09-23 02:36:54 UTC · commit |
Nikola Metulev (nmetulev)
changed the title
Add safe invocation-local cache fallback, preserve NuGet configuration and warm read-only packages, allow detached UI observations, and move layout locks beside their protected outputs. Keep required coordination fail-closed and preserve structured diagnostics and clean path output. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Exercise ordinary offline discovery for update notices and separately verify informational commands suppress them. Dispose test writers, protect partially acquired lock collections, and narrow MSStore archive cleanup exceptions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Validate namespace ownership, ancestor permissions and existing artifacts before target state access; create private user/SYSTEM state compatible with Sandbox folder mapping. Reject untrusted state without repairing exposed keys, add sandbox_state_unavailable, and validate cache-relative path invariants. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Authenticate repository-local tool and debugger payloads; avoid new NuGet prerequisites during evaluation; reuse completed fallback packages; preserve scaffold JSON failures; detect inaccessible shared scratch before NuGet retries; and keep stable layout lock files for reliable handoff. Isolate trusted-state test fixtures from shared CI drives. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Preserve real ownership handoff and replay coverage, then exercise a deliberately expired observation instead of assuming another process starts within idle grace. Add scheduler boundary coverage for retained and expired turns. Verify native getter failures enter the retry path even when a slow read exhausts the timeout, and separately require complete constrained-query replay. Cover the slow-read failure deterministically without changing production deadlines or policies. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Zach Teutsch (zateutsch)
left a comment
Zach Teutsch (zateutsch)
left a comment
There was a problem hiding this comment.
🤖 AI-generated review (winappcli pr-review skill) — verify before acting.
Changes required: two new filesystem tests reproducibly fail on ARM64. I also left two non-blocking comments about fallback-cache performance and documentation.
Create junctions with the Windows reparse-point API and hold child-process leases through architecture-neutral native file handles. Cover extended paths and report child exit codes and stderr on readiness failure without weakening security or locking assertions. Qualify command-specific cache locations as defaults and link the canonical restricted-filesystem guidance. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Use File.Replace for existing deployment records only under the store's existing writer lease. Windows overwrite-by-move rejects even delete-sharing readers; generic unleased cache publication retains its prior behavior. Keep revision arbitration and access failures intact. Add deterministic open-reader, read-only destination, and non-delete-sharing protection regressions; retain the concurrent-writer state test. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
ReplaceFile exposes transient exclusive-handle and missing-name windows to new readers. Use FileRenameInfoEx for state publication so delete-sharing readers retain their snapshots and the destination name stays bound to a complete record. Keep revision leases, namespace validation, ACL enforcement, read-only protection and non-delete-sharing locks intact. Leave generic cache publication unchanged. Cover overlapping readers/writers, concurrent publishers and long paths on ARM64 and x64. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The existing single-frame encoder fixture intermittently reaches Finalize with no compressed samples, as recorded on main in #834. Feed one second at 30 fps while retaining short-buffer validation, idempotent completion and nonempty publication checks; also assert staging is consumed only at completion. Validated the targeted suite in Windows Sandbox and verified the retained MP4 contains exactly 30 video samples. No production encoder changes, added skips or swallowed failures. Refs #834 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Six unresolved findings include critical state-trust and lock-compatibility issues plus cache, NuGet, and notification defects.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 3
Open (3)
Nikola Metulev (nmetulev)
added
the
ready-for-review
Share the existing target-state ancestor trust policy with UI coordination. Reject local reparse points and replaceable ancestors before leaf permission repair or artifact access, and never treat an untrusted path as permission to run an observation detached. Cover junctions at each relevant boundary, mutable ancestors, linked state files, unchanged destination ACLs/content and all turn modes. Preserve existing target-state behavior and document the fail-closed recovery path. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Zach Teutsch (zateutsch)
left a comment
Zach Teutsch (zateutsch)
left a comment
There was a problem hiding this comment.
🤖 AI-generated review (winappcli pr-review skill) — verify before acting.
Decision: changes required. The resilience work is well-structured, well-tested, and correctly scoped. But it opens two repo-controllable caches that execute/trust content without the signature checks this PR applies elsewhere, plus a concurrency race that can spuriously fail state reads. Details are in the inline comments — two security gaps (high) and one correctness race (high), plus one optional cleanup.
Not fully exercised: NativeAOT publish and the end-to-end exploit/concurrency repros couldn't run in the review sandbox (missing MSVC toolchain; MTP test filter). Findings are confirmed against the code paths; the managed test build is clean.
|
|
||
| private void VerifyLocalTool(string installDir) | ||
| { | ||
| if (!_cache.IsLocalFallback) |
There was a problem hiding this comment.
Security (high): explicit repo-local cache override skips tool signature verification.
VerifyLocalTool returns immediately unless _cache.IsLocalFallback is true, and that flag is only set when the default global cache fails and the code auto-switches. When a user sets WINAPP_CLI_CACHE_DIRECTORY to a repo-local path — which docs/usage.md:2144 recommends for restricted-FS use (Join-Path (Get-Location) '.winapp\cache') — that path becomes the "global" cache, succeeds, and IsLocalFallback stays false. A pre-planted .\.winapp\cache\tools\msstore\msstore.exe is then launched unauthenticated, even though the identical directory would be signature-checked under automatic fallback.
The documented workaround for restricted filesystem access becomes arbitrary code execution when building an untrusted/cloned repo.
Suggested fix: authenticate whenever the resolved cache is not the user-profile default (any explicit override), or specifically when it resolves under the invocation directory — don't gate authentication on IsLocalFallback.
| ResolvedTriageBinaries? ResolveExisting(DirectoryInfo dir) => | ||
| (BinariesResolverOverride ?? (d => XamlTriageBinaries.ResolveExisting(d, logger)))(dir); | ||
| ResolvedTriageBinaries? ResolveExisting(DirectoryInfo dir) => | ||
| (BinariesResolverOverride ?? (d => XamlTriageBinaries.ResolveExisting(d, logger, _cache.IsLocalFallback)))(dir); |
There was a problem hiding this comment.
Same root cause as the MSStore finding: local-binary verification is gated on _cache.IsLocalFallback, so an explicit WINAPP_CLI_CACHE_DIRECTORY pointed at a repo-local .winapp\cache bypasses the signature check on reused XAML-triage binaries. Whatever fix is applied to VerifyLocalTool should cover this path too.
| // the marker is missing, fall through so the downloader re-extracts and completes the entry. | ||
| if (HasCompletionMarker(packageDir)) | ||
| var downloaded = false; | ||
| if (!HasCompletionMarker(packageDir)) |
There was a problem hiding this comment.
Security (high): NuGet local fallback trusts repo-planted extracted packages without authentication.
A pre-existing .nupkg.metadata completion marker makes this treat the package as fully installed and skip download entirely — so NugetPackageDownloader (where signature policy is enforced) is never called. The fallback packages directory resolves to <invocation>\.winapp\cache\nuget\packages (NugetSourceProvider.GetLocalPackagesDirectory), which a repo controls, and ValidateLocalPath/ValidateLocalTree only reject reparse points, not content authenticity.
With default package storage denied, a repo can plant .winapp\cache\nuget\packages\<sdk-pkg>\<ver>\ containing the marker + nuspec + a substituted cppwinrt.exe; winapp restore accepts it unverified and WorkspaceSetupService later executes the tool from that cache. Unlike the Store/WinDbg caches, this path has no VerifyLocalTool equivalent, and it's the automatic path — no override needed.
Suggested fix: don't treat completion markers in the invocation-local NuGet fallback as authentication — re-verify via the policy-checked downloader, or refuse reuse of pre-existing extracted payloads there.
| { | ||
| var directories = _directoryService ?? new WinappDirectoryService(_currentDirectoryProvider); | ||
| var root = directories.GetLocalCacheDirectory(); | ||
| var path = Path.Combine(root.FullName, "nuget", "packages"); |
There was a problem hiding this comment.
This is the repo-controllable location that makes the NuGet-fallback trust gap (see NugetService.cs) reachable: the fallback packages folder is <invocation>\.winapp\cache\nuget\packages. The reparse-point checks here don't authenticate the contents of packages a repo may have planted under this directory.
| Verify(new FileInfo(item.FullName), user, allowAncestorAccess: false); | ||
| } | ||
| } | ||
| catch (Exception ex) when (ex is FileNotFoundException or DirectoryNotFoundException) |
There was a problem hiding this comment.
Correctness (high): a delete-pending race becomes a spurious "untrusted" failure.
VerifyContents recursively GetAccessControls every file under the target root but catches only FileNotFoundException/DirectoryNotFoundException. During a concurrent AtomicFile replace, a file can be in Windows delete-pending state, where GetAccessControl throws UnauthorizedAccessException (ERROR_ACCESS_DENIED). That isn't caught here, so it surfaces as '<path>' is owned by or grants unsafe access to another user → StateUnavailable.
DeploymentStateStoreTests.Read_ConcurrentPublications_ReturnsCompleteCommittedRecords exercises exactly this (writer doing atomic commits vs reader triggering EnsureTrusted), and it was observed to fail once with an "unauthorized operation" from Verify(...). It presents as an intermittent CI flake and intermittent failure for concurrent winapp processes on the same target/sandbox.
Suggested fix: treat a lost-to-replacement ACL read (delete-pending / access-denied on a vanishing atomic temp) as a transient retry, not an untrusted owner — but scope it to the known atomic temp/publication names so genuine access denials still fail closed. A targeted stress loop of that test would confirm.
|
|
||
| namespace WinApp.Cli.Services; | ||
|
|
||
| internal interface IStorageDiagnostics |
There was a problem hiding this comment.
Non-blocking (optional): IStorageDiagnostics is a single-method, single-implementation interface, and the concrete StorageDiagnostics also has behavior (Complete) not on it. Tests can already substitute output by constructing it with a StringWriter, so the seam isn't buying anything. Per AGENTS.md (DI doesn't require an interface), consider deleting the interface and consuming the concrete type. Not a blocker.
## Description Keep unreleased UI coordination and Windows Sandbox target state in a shared physical user-profile location, rather than LocalAppData or package-redirected storage. UI locks now default to `%USERPROFILE%\.winapp\state\ui`; each Sandbox target defaults to `%USERPROFILE%\.winapp\state\targets\<target-key>`. These paths do not follow `WINAPP_CLI_CACHE_DIRECTORY`. Existing `WINAPP_UI_LOCK_DIRECTORY` and `WINAPP_TARGET_STATE_ROOT` overrides retain precedence. A profile `.winapp` used for state is not mistaken for a project's local cache when the global cache is overridden. Invalid or inaccessible target-state storage reports `sandbox_state_unavailable`, not a stale Sandbox; one troubleshooting row describes recovery. ## Usage Example `winapp ui click Submit -a MyApp` coordinates through `%USERPROFILE%\.winapp\state\ui` by default; commands on `--on sandbox` keep target records under `%USERPROFILE%\.winapp\state\targets\<target-key>`. The exact paths and override precedence are covered by focused tests, not a live Sandbox session. ## Related Issue N/A — intentionally separate from the broader, still-open #873; no changes to that PR. ## Type of Change - 🐛 Bug fix - 🧪 Test update ## Checklist - [x] New tests added for new functionality - [x] Tested locally on Windows (80 focused tests; x64 and arm64 NativeAOT builds; generated docs and npm build) - [ ] Required `build-and-package` CI on latest commit `03a124dc` (in progress; previous head passed) - [ ] Packaged/unpackaged live Sandbox probe (no Sandbox instance running; no newly packaged app installed) ## Screenshots / Demo N/A — nonvisual path relocation. ## Additional Notes The released 0.6.2 behavior has no UI coordination or Sandbox target state contract to migrate. This PR intentionally excludes filesystem fallback, cache relocation, ACL redesign, and changes to #873. No merge or auto-merge requested. ## AI Description <!-- ai-description-start --> _This section is auto-generated by AI when the PR is opened or updated. To opt out, delete this entire section including the marker comments._ <!-- ai-description-end --> --------- Co-authored-by: Nikola Metulev <711864+nmetulev@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Closing in favor of #941, a focused version of this change. #931 already moved UI and Sandbox state under %USERPROFILE%.winapp\state\ with clear errors, so #941 only handles what remained: the first-run notice, the update check, the |
Description
Keep useful winapp operations working when filesystem access outside the current directory is blocked, without bypassing permissions, shared coordination, package-source policy or signature checks.
%USERPROFILE%\.winapp\state, independent of cache settings and package identity. Target state verifies trusted ownership, ancestors and existing artifacts; new private state grants the host user and SYSTEM access for the Sandbox broker..winapp\cache. Reuse readable warm caches, reject escaping links, and keep explicit settings authoritative.NUGET_SCRATCHguidance rather than silently splitting its lock namespace or waiting through long access-denied retries.new --jsonpreparation failures. Keep optional bookkeeping nonfatal and successful-degradation warnings on stderr.Usage Example
Successful fallback warns on stderr (JSON warnings with
--json). Failed commands retain their error contract and nonzero exit code. No unlocked or independently relocated shared coordination.Related Issue
Related to #764 and #767.
Type of Change
Checklist
Screenshots / Demo
N/A — command output, storage and coordination changes.
Additional Notes
Validation
scripts\build-cli.ps1 -SkipTests -SkipMsix -SkipNuGet -SkipNpm: both NativeAOT architectures and documentation generation passed with zero warnings/errors; npm native binaries refreshed.D:\checkout drive; production ancestor checks remain intact.Boundaries
AI Description
Makes optional global storage recoverable while preserving required shared state, authenticated tools, NuGet configuration and script-friendly errors.