Consolidate file path resolution for WebView controls#34620
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://github.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 34620Or
iex "& { $(irm https://github.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 34620" |
There was a problem hiding this comment.
Pull request overview
This PR refactors WebView-related URL/path resolution across HybridWebView and BlazorWebView to use shared helpers, aiming to prevent Path.Combine from dropping the intended root when given separator-prefixed “relative” paths, and to harden against path traversal.
Changes:
- Added shared helpers for request-URI → relative-path resolution (
WebUtils.ResolveRelativePath) and for validating/combining root + relative paths (FileSystemUtils.IsValidRelativePath/Combine). - Updated HybridWebView handlers (Android/iOS/Windows) and BlazorWebView asset file providers/managers (Android/iOS/Windows/Tizen) to use the shared helpers.
- Added device tests and test assets covering relative paths, rooted paths, dot-dot segments, and encoded separators.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Essentials/src/Types/Shared/WebUtils.shared.cs | Adds ResolveRelativePath for validating app-origin-relative request paths. |
| src/Essentials/src/FileSystem/FileSystemUtils.shared.cs | Adds IsValidRelativePath and Combine helper for safe root+relative path combination. |
| src/Essentials/src/Email/Email.windows.cs | Switches attachment path normalization to FileSystemUtils.NormalizePath. |
| src/Core/src/Platform/Android/MauiHybridWebViewClient.cs | Uses shared helpers for HybridWebView Android request path resolution + asset lookup. |
| src/Core/src/Handlers/HybridWebView/HybridWebViewHandler.iOS.cs | Uses shared helpers for iOS scheme-task request path resolution + asset lookup. |
| src/Core/src/Handlers/HybridWebView/HybridWebViewHandler.Windows.cs | Uses shared helpers for Windows request path resolution + asset lookup. |
| src/Controls/tests/DeviceTests/Resources/Raw/HybridTestRoot/urlresolution.html | Adds test page used to fetch URLs and report results to device tests. |
| src/Controls/tests/DeviceTests/Resources/Raw/HybridTestRoot/safe-file.txt | Adds a known “safe” file for positive resolution assertions. |
| src/Controls/tests/DeviceTests/Elements/HybridWebView/HybridWebViewTests_ContentRootResolution.cs | Adds HybridWebView device tests covering path resolution edge cases. |
| src/Controls/src/Core/Controls.Core.csproj | Includes the shared FileSystemUtils.shared.cs source into Controls.Core for reuse. |
| src/BlazorWebView/tests/DeviceTests/Elements/BlazorWebViewTests.ContentRootResolution.cs | Adds BlazorWebView device tests covering path resolution edge cases. |
| src/BlazorWebView/src/Maui/iOS/iOSMauiAssetFileProvider.cs | Uses shared combine helper when serving bundled static assets (iOS). |
| src/BlazorWebView/src/Maui/Windows/WinUIWebViewManager.cs | Uses shared ResolveRelativePath and combine helper for local folder serving (Windows). |
| src/BlazorWebView/src/Maui/Tizen/TizenMauiAssetFileProvider.cs | Uses shared combine helper when serving packaged static assets (Tizen). |
| src/BlazorWebView/src/Maui/Android/AndroidMauiAssetFileProvider.cs | Uses shared combine helper when serving APK assets (Android). |
2b033cf to
e19a666
Compare
Refactored scattered file-path resolution logic into shared utilities (FileSystemUtils.Combine, WebUtils.ResolveRelativePath) to fix edge cases where Path.Combine drops the root directory when a relative path starts with a separator. Added device tests for path resolution. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
e19a666 to
2b1b2b8
Compare
🧪 PR Test EvaluationOverall Verdict: The device tests cover the critical path traversal attack vectors end-to-end, but the new security-critical utility methods (
📊 Expand Full EvaluationPR Test Evaluation ReportPR: #34620 — Consolidate file path resolution for WebView controls Overall VerdictThe device tests provide solid end-to-end coverage of the path traversal scenarios, but the core security utility methods introduced in 1. Fix Coverage — ✅The tests exercise the actual URL-to-file resolution pipeline that was refactored, including the new 2. Edge Cases & Gaps —
|
| Platform | Fix Files | Test Coverage |
|---|---|---|
| Android | AndroidMauiAssetFileProvider.cs, MauiHybridWebViewClient.cs |
✅ Via device tests |
| iOS | iOSMauiAssetFileProvider.cs, HybridWebViewHandler.iOS.cs |
✅ Via device tests |
| Windows | WinUIWebViewManager.cs, HybridWebViewHandler.Windows.cs, Email.windows.cs |
✅ Via device tests |
| MacCatalyst | Shared with iOS | ✅ Via iOS device tests (.ios.cs compiles for both) |
| Tizen | TizenMauiAssetFileProvider.cs |
❌ No Tizen tests |
| Shared/Cross-platform | FileSystemUtils.shared.cs, WebUtils.shared.cs, Email.windows.cs |
The Essentials/src/Email/Email.windows.cs change also uses the new FileSystemUtils.Combine — this is only exercised if Windows email attachment tests exist elsewhere.
8. Assertion Quality — ⚠️
| Test | Assertion | Quality |
|---|---|---|
RelativePaths_ResolveToContent |
Assert.Equal(200, ...) + body length |
✅ Specific |
KnownFile_ReturnsExpectedContent |
Checks actual body content | ✅ Excellent |
RootedPath_DoesNotResolve (HybridWebView) |
Assert.NotEqual(200, ...) |
✅ Clear |
DotDotSegments_DoNotResolveAboveRoot (HybridWebView) |
Assert.NotEqual(200, ...) |
✅ Clear |
Blazor_RootedPath_DoesNotResolve |
`status != 200 | |
Blazor_DotDotSegments_DoNotResolveAboveRoot |
`status != 200 |
9. Fix-Test Alignment — ✅
Fix files and test files align well: BlazorWebViewTests.ContentRootResolution.cs tests the BlazorWebView asset provider changes, and HybridWebViewTests_ContentRootResolution.cs tests the HybridWebView handler changes. Both exercise the shared FileSystemUtils and WebUtils logic through the request pipeline.
Recommendations
-
Add unit tests for
FileSystemUtils.IsValidRelativePathandFileSystemUtils.Combineinsrc/Essentials/test/UnitTests/FileSystem_Tests.cs. These methods are the security foundation of this PR — test them directly with edge cases like empty string, single dot./, null, backslash-only paths, and null byte injection (\0). -
Tighten BlazorWebView negative assertions — replace
Assert.True(result.status != 200 || IsSpaFallback(result))withAssert.NotEqual(200, result.status)where possible, or add a comment explaining why SPA fallback behavior is intentionally acceptable for these paths. As written, the assertion could silently pass even if a traversal attack succeeds but returns HTML. -
Add a Windows case-sensitivity test — verify that
FileSystemUtils.Combinebehaves correctly when root and path have different casing on Windows (e.g.,C:\Root\Contentvsc:\root\content\file.txt), sinceStringComparison.Ordinalis case-sensitive and Windows paths are not.
Warning
⚠️ Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
dc.services.visualstudio.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "dc.services.visualstudio.com"See Network Configuration for more information.
Note
🔒 Integrity filtering filtered 2 items
Integrity filtering activated and filtered the following items during workflow execution.
This happens when a tool call accesses a resource that does not meet the required integrity or secrecy level of the workflow.
- pr:Consolidate file path resolution for WebView controls #34620 (
pull_request_read: Resource 'pr:Consolidate file path resolution for WebView controls #34620' has lower integrity than agent requires. Agent would need to drop integrity tags [approved:all unapproved:all] to trust this resource.) - pr:Consolidate file path resolution for WebView controls #34620 (
pull_request_read: Resource 'pr:Consolidate file path resolution for WebView controls #34620' has lower integrity than agent requires. Agent would need to drop integrity tags [unapproved:all approved:all] to trust this resource.)
🧪 Test evaluation by Evaluate PR Tests
- Fix IsValidRelativePath to check '..' per-segment instead of substring - Fix Combine to preserve relative roots for Android/package asset paths - Fix Combine to use case-insensitive comparison on Windows - Remove unused using directive in WebUtils.shared.cs - Rename test methods and comments for clarity - Add FileSystemUtils and WebUtils unit tests Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
🧪 PR Test EvaluationOverall Verdict: ✅ Tests are adequate The PR adds strong, well-layered test coverage for the path resolution security fix — unit tests cover the logic exhaustively, and device tests validate the fix end-to-end inside real WebView instances. One minor assertion in
📊 Expand Full EvaluationPR Test Evaluation ReportPR: #34620 — Consolidate file path resolution for WebView controls Overall Verdict✅ Tests are adequate The PR provides excellent layered coverage: pure unit tests for the new 1. Fix Coverage — ✅The tests directly exercise the fixed code paths:
2. Edge Cases & Gaps — ✅Covered:
Potential minor gap:
3. Test Type Appropriateness — ✅
No UI tests (Appium) were added, which is correct — the fix is in file-serving infrastructure, not visual UI. 4. Convention Compliance — ✅Automated script found 0 convention issues.
5. Flakiness Risk — ✅ Low
6. Duplicate Coverage — ✅ No duplicatesThe script identified 7. Platform Scope — ✅The fix touches all four MAUI platforms (Android, iOS, MacCatalyst, Windows). Coverage:
The Tizen gap is acceptable; Tizen has no device test infrastructure in this repo. 8. Assertion Quality — ✅Most assertions are well-targeted:
Minor concern: The Blazor device test assertions use 9. Fix-Test Alignment — ✅
Recommendations
Warning
|
…ompat - Revert Tizen file provider changes (Tizen is no longer supported) - Replace OperatingSystem.IsWindows() with OrdinalIgnoreCase comparison to fix CS0117 on netstandard2.0 builds - Add null-forgiving operators for netstandard2.0 nullable analysis Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
🧪 PR Test EvaluationOverall Verdict: ✅ Tests are adequate The PR includes 22 unit tests and 14 device tests with comprehensive coverage of the core fix (path traversal prevention), edge cases, and cross-platform integration. Minor improvements noted below.
📊 Expand Full EvaluationPR Test Evaluation ReportPR: #34620 — Consolidate file path resolution for WebView controls Overall Verdict✅ Tests are adequate The PR has comprehensive unit tests for the new 1. Fix Coverage — ✅The tests directly exercise the two new shared utilities that form the heart of the fix:
The tests would fail if the fix were reverted. 2. Edge Cases & Gaps — ✅Covered:
Missing (minor):
None of these are blocking. 3. Test Type Appropriateness — ✅Current: Unit Tests + Device Tests The new utilities ( 4. Convention Compliance — ✅
5. Flakiness Risk — ✅ Low
6. Duplicate Coverage — ✅ No duplicatesExisting 7. Platform Scope — ✅Fix touches Windows, Android, iOS, and MacCatalyst handlers. Unit tests run on all platforms (no platform-specific code). Device tests cover:
MacCatalyst is covered by the iOS device tests (same 8. Assertion Quality — ✅
9. Fix-Test Alignment — ✅
All fix components have corresponding tests. Recommendations
Warning
|
|
/azp run maui-pr-devicetests |
|
Azure Pipelines successfully started running 3 pipeline(s). |
The Blazor host page is only served for navigation requests (ResourceContext.Document),
not for JavaScript fetch() requests. Calling fetch('') from within a BlazorWebView either
throws (unpackaged Windows) or hangs indefinitely (packaged Windows), causing CI failures.
The host page loading is already verified by the RunTest infrastructure which waits for
the Blazor component to render before running any test code.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
🧪 PR Test EvaluationOverall Verdict: The unit tests are thorough and well-structured. The main gap is that BlazorWebView device tests lack a positive (happy-path) test confirming valid files still resolve correctly after the fix — without it, a regression that accidentally blocks legitimate requests would go undetected.
📊 Expand Full EvaluationPR Test Evaluation ReportPR: #34620 — Consolidate file path resolution for WebView controls Overall VerdictThe unit tests for 1. Fix Coverage — ✅The fix replaces unsafe
The 2. Edge Cases & Gaps —
|
| Platform | Fix files | Device test coverage |
|---|---|---|
| Android | AndroidMauiAssetFileProvider.cs, MauiHybridWebViewClient.cs |
✅ HybridWebView device tests; BlazorWebView device tests |
| iOS | iOSMauiAssetFileProvider.cs, HybridWebViewHandler.iOS.cs |
✅ HybridWebView device tests; BlazorWebView device tests |
| MacCatalyst | iOSMauiAssetFileProvider.cs (.ios.cs compiles for both) |
.ios.cs; no explicit MacCatalyst test case |
| Windows | WinUIWebViewManager.cs, HybridWebViewHandler.Windows.cs, Email.windows.cs |
✅ HybridWebView device tests; BlazorWebView device tests |
MacCatalyst is covered transitively via the .ios.cs file extension (which compiles for both iOS and MacCatalyst) and the Helix CI infra, but there are no explicit MacCatalyst test assertions.
8. Assertion Quality — ⚠️
- Unit tests: ✅ Specific —
Assert.True/False,Assert.Null/NotNull,Assert.Equalwith concrete expected values - HybridWebView device tests: ✅ Specific —
Assert.Equal(200, result.status),Assert.NotEqual(200, result.status),Assert.Contains("content directory", ...)directly tests the known file content - BlazorWebView device tests:
⚠️ Slightly permissive — assertions useresult.status != 200 || IsSpaFallback(result). This is intentional (SPA fallback behavior) but means a traversal attack that returns the host page would pass the test. Consider adding an assertion onresult.bodyPreviewthat it does NOT contain the actual traversed file's content, to harden the negative cases.
9. Fix-Test Alignment — ✅
All changed files map correctly to the tests:
FileSystemUtils.shared.cs→FileSystemUtils_Tests.cs(complete coverage)WebUtils.shared.cs→WebUtils_Tests.cs(good coverage)- HybridWebView handlers (iOS, Android, Windows) →
HybridWebViewTests_ContentRootResolution.cs - BlazorWebView file providers (iOS, Android) and Windows manager →
BlazorWebViewTests.ContentRootResolution.cs Email.windows.cs→ covered byNormalizePathunit test (transitively)
Recommendations
-
Add a positive test for BlazorWebView — add a test case in
BlazorWebViewTests.ContentRootResolution.csthat fetches a known-good static asset (e.g., the Blazor wwwroot index or a test.txtfile) and asserts it returns 200 with expected content. This ensures the fix doesn't accidentally block legitimate requests. -
Harden BlazorWebView negative assertions — in
Blazor_DotDotSegments_DoNotResolveAboveRootetc., consider additionally asserting thatresult.bodyPreviewdoes not contain sensitive content (even ifIsSpaFallbackis true), to guard against a case where traversal returns the host page content instead of the intended blocked response. -
Consider an explicit
Email.windows.csintegration test (low priority) — the change is a minor refactor, but a quick unit test assertingFileSystemUtils.NormalizePathbehaves identically to the old inline method on the specific patterns used by Email attachments would make the refactor provably safe.
Warning
⚠️ Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
dc.services.visualstudio.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "dc.services.visualstudio.com"See Network Configuration for more information.
Note
🔒 Integrity filtering filtered 2 items
Integrity filtering activated and filtered the following items during workflow execution.
This happens when a tool call accesses a resource that does not meet the required integrity or secrecy level of the workflow.
- pr:Consolidate file path resolution for WebView controls #34620 (
pull_request_read: Resource 'pr:Consolidate file path resolution for WebView controls #34620' has lower integrity than agent requires. Agent would need to drop integrity tags [approved:all unapproved:all] to trust this resource.) - pr:Consolidate file path resolution for WebView controls #34620 (
pull_request_read: Resource 'pr:Consolidate file path resolution for WebView controls #34620' has lower integrity than agent requires. Agent would need to drop integrity tags [unapproved:all approved:all] to trust this resource.)
🧪 Test evaluation by Evaluate PR Tests
|
Azure Pipelines successfully started running 1 pipeline(s). |
1 similar comment
|
Azure Pipelines successfully started running 1 pipeline(s). |
Fetches _framework/blazor.webview.js (always available in a BlazorWebView) and asserts status 200 with content. This exercises ResolveRelativePath with a valid multi-segment relative path and ensures the path hardening doesn't accidentally block legitimate requests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
🧪 PR Test EvaluationOverall Verdict: ✅ Tests are adequate This PR adds a well-layered test suite (unit tests for the shared utilities + device tests for real WebView end-to-end behavior) that directly exercises the path traversal and rooted-path security fixes.
📊 Expand Full EvaluationPR Test Evaluation ReportPR: #34620 — Consolidate file path resolution for WebView controls Overall Verdict✅ Tests are adequate The PR takes a strong, layered approach: lightweight xUnit unit tests for the two new shared utilities ( 1. Fix Coverage — ✅The unit tests in The device tests in 2. Edge Cases & Gaps — ✅Covered:
Minor gaps (non-blocking):
3. Test Type Appropriateness — ✅
No UI tests were added (correct — no Appium-level interaction changed). The device tests are the lightest type that can verify actual HTTP response codes from the WebView interceptors. 4. Convention Compliance — ✅No convention issues found by automated script.
5. Flakiness Risk — ✅ Low
6. Duplicate Coverage — ✅ No duplicatesThe existing 7. Platform Scope — ✅The fix touches all 4 platforms (Android, iOS, Windows, MacCatalyst) plus shared utilities.
8. Assertion Quality — ✅
Minor concern: 9. Fix-Test Alignment — ✅
Recommendations
Warning
|
|
Azure Pipelines successfully started running 1 pipeline(s). |
Note
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
Description
Refactored scattered file-path resolution logic in HybridWebView and BlazorWebView into shared utilities (
FileSystemUtils.Combine,WebUtils.ResolveRelativePath) to fix edge cases wherePath.Combinedrops the root directory when a relative path starts with a separator.Changes
FileSystemUtils.shared.cs— AddedIsValidRelativePath()andCombine()methods that validate relative paths and ensure the resolved path stays within the expected root directoryWebUtils.shared.cs— AddedResolveRelativePath()to resolve request URIs against an app origin into validated relative pathsFileSystemUtils.Combine()for path resolutionEmail.windows.cs— Consolidated duplicateNormalizePathintoFileSystemUtils.NormalizePath