-
Notifications
You must be signed in to change notification settings - Fork 2k
[inflight regression] Fix HybridWebView JS↔.NET bridge: Android request interception order and Windows NUL-character handling #36544
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
a4915d4
8d9a6d7
27f6d4a
5b8c1fe
089681c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -98,7 +98,10 @@ public static void MapSendRawMessage(IHybridWebViewHandler handler, IHybridWebVi | |
|
|
||
| private void OnWebMessageReceived(WebView2 sender, CoreWebView2WebMessageReceivedEventArgs args) | ||
| { | ||
| MessageReceived(args.TryGetWebMessageAsString()); | ||
| // The JS transport URL-encodes messages so embedded NUL characters survive WebView2's | ||
| // null-terminated string marshalling (TryGetWebMessageAsString returns an LPWSTR). Decode | ||
| // the payload before dispatching it. | ||
| MessageReceived(Uri.UnescapeDataString(args.TryGetWebMessageAsString())); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
[major] Null Safety — malformed/non-string WebView2 message — |
||
| } | ||
|
|
||
| internal static void MapFlowDirection(IHybridWebViewHandler handler, IHybridWebView hybridWebView) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -69,14 +69,36 @@ public override void OnPageFinished(AWebView? view, string? url) | |
|
|
||
| if (view is not null && request is not null && !string.IsNullOrEmpty(url)) | ||
| { | ||
| // 1. Check if the app wants to modify or override the request | ||
| // 1. Framework-internal bridge requests must be handled by the framework and | ||
| // never exposed to app-level WebResourceRequested interception. See | ||
| // IsFrameworkInternalRequest: each reserved endpoint is bound to BOTH its | ||
| // well-known path and (for the message/invoke channels) the protocol marker | ||
| // header, so the header alone is never a trust boundary. Before JS -> .NET | ||
| // messages were routed over HTTP they were invisible to app interception, and | ||
| // this preserves that invariant. | ||
| if (IsFrameworkInternalRequest(url, request)) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
[major] Cross-Platform Behavioral Consistency — This new |
||
| { | ||
| // A framework-internal request must be handled by the framework. If it | ||
| // cannot be resolved, fail fast with a 400 rather than forwarding it to the | ||
| // app handler. | ||
| return GetResponse(url, request, logger) | ||
| ?? new WebResourceResponse(null, "UTF-8", 400, "Bad Request", null, new MemoryStream()); | ||
| } | ||
|
|
||
| // 2. Check if the app wants to modify or override the request. This path is | ||
| // intentionally left unwrapped: if a user WebResourceRequested handler throws | ||
| // for a legitimate app-origin request, the exception propagates exactly as it | ||
| // did before bridge traffic was routed over HTTP. Only the framework's own | ||
| // .NET dispatch (Handler.MessageReceived in GetResponse) is exception-isolated, | ||
| // because it runs under a JNI stack where an unhandled throw crashes the | ||
| // native WebView thread. | ||
| var response = WebRequestInterceptingWebView.TryInterceptResponseStream(Handler, view, request, url, logger); | ||
| if (response is not null) | ||
| { | ||
| return response; | ||
| } | ||
|
|
||
| // 2. Check if the request is for a local resource | ||
| // 3. Check if the request is for a local resource | ||
| response = GetResponse(url, request, logger); | ||
| if (response is not null) | ||
| { | ||
|
|
@@ -90,22 +112,67 @@ public override void OnPageFinished(AWebView? view, string? url) | |
| return base.ShouldInterceptRequest(view, request); | ||
| } | ||
|
|
||
| // Resolves the app-origin-relative path for a request URL. Returns false when the URL is | ||
| // not under the HybridWebView app origin; returns true otherwise, with relativePath set to | ||
| // the resolved path (which may itself be null if the path could not be resolved). Shared by | ||
| // IsFrameworkInternalRequest and GetResponse to keep the URI parsing in one place. | ||
| static bool TryGetAppRelativePath(string fullUrl, out string? relativePath) | ||
| { | ||
| relativePath = null; | ||
|
|
||
| var requestUri = WebUtils.RemovePossibleQueryString(fullUrl); | ||
| if (new Uri(requestUri) is not Uri uri || !HybridWebViewHandler.AppOriginUri.IsBaseOf(uri)) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| relativePath = WebUtils.ResolveRelativePath(HybridWebViewHandler.AppOriginUri, uri); | ||
| return true; | ||
| } | ||
|
|
||
| // Returns true when the request targets a reserved HybridWebView bridge endpoint and must | ||
| // therefore be handled by the framework instead of being exposed to app-level | ||
| // WebResourceRequested interception. Each endpoint is bound to its well-known path: | ||
| // - the bridge bootstrap script is a plain <script> load with no header, matched by path; | ||
| // - the message/invoke channels must ALSO carry the protocol marker header, because the | ||
| // header name/value are public and a same-origin script could otherwise set it on an | ||
| // arbitrary URL to bypass interception. | ||
| static bool IsFrameworkInternalRequest(string fullUrl, IWebResourceRequest request) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
[moderate] Regression Prevention / Test Coverage — No tests accompany this behavior change (request-interception ordering plus new |
||
| { | ||
| if (!TryGetAppRelativePath(fullUrl, out var relativePath) || relativePath is null) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| if (relativePath == HybridWebViewHandler.HybridWebViewDotJsPath) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| if (relativePath == HybridWebViewHandler.InvokeDotNetPath || | ||
| relativePath == HybridWebViewHandler.SendMessagePath) | ||
| { | ||
| return HybridWebViewHandler.HasExpectedHeaders(request.RequestHeaders); | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| private WebResourceResponse? GetResponse(string fullUrl, IWebResourceRequest request, ILogger? logger) | ||
| { | ||
| if (Handler is null || Handler is IViewHandler ivh && ivh.VirtualView is null) | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| var requestUri = WebUtils.RemovePossibleQueryString(fullUrl); | ||
| if (new Uri(requestUri) is not Uri uri || !HybridWebViewHandler.AppOriginUri.IsBaseOf(uri)) | ||
| if (!TryGetAppRelativePath(fullUrl, out var relativePath)) | ||
| { | ||
| // Not an app-origin request; let it proceed unmodified. | ||
| return null; | ||
| } | ||
|
|
||
| logger?.LogDebug("Request for {Url} will be handled by .NET MAUI.", fullUrl); | ||
|
|
||
| var relativePath = WebUtils.ResolveRelativePath(HybridWebViewHandler.AppOriginUri, uri); | ||
| if (relativePath is null) | ||
| { | ||
| logger?.LogDebug("Request for {Url} resolved to an invalid path.", fullUrl); | ||
|
|
@@ -148,7 +215,13 @@ public override void OnPageFinished(AWebView? view, string? url) | |
| return error; | ||
| } | ||
|
|
||
| // Do not wrap this in a try/catch. MessageReceived raises the app-facing message | ||
| // handlers (e.g. RawMessageReceived); an exception thrown by app code must be allowed | ||
| // to propagate rather than be swallowed, matching how MAUI treats event handlers such | ||
| // as Button.Click. Developers who want to handle these exceptions can catch them in | ||
| // their own handler. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
[critical] Async/Threading Safety — contradictory JNI exception-safety claims — This |
||
| Handler.MessageReceived(messageBody); | ||
|
|
||
| return new WebResourceResponse(null, "UTF-8", 204, "No Content", null, new MemoryStream()); | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,7 +23,10 @@ public MauiHybridWebView(HybridWebViewHandler handler) | |
|
|
||
| public void SendRawMessage(string rawMessage) | ||
| { | ||
| CoreWebView2.PostWebMessageAsString(rawMessage); | ||
| // WebView2's PostWebMessageAsString marshals to a null-terminated LPCWSTR, so any embedded | ||
| // NUL character would truncate the message. URL-encode the payload so it survives; the JS | ||
| // transport decodes it in the WebView2 'message' event listener in hybridwebview.js. | ||
| CoreWebView2.PostWebMessageAsString(Uri.EscapeDataString(rawMessage)); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
[moderate] Performance-Critical Path — |
||
| } | ||
|
|
||
| public async void RunAfterInitialize(Action action) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[moderate] Malformed/non-string WebView2 message handling —
decodeURIComponent(arg.data)assumes everychrome.webview'message'event was posted via the new percent-encoding inMauiHybridWebView.SendRawMessage. But this listener fires for any message posted to thisCoreWebView2(e.g. viaPostWebMessageAsJson, a non-string payload, or any other app/component code sharing the same WebView2), wherearg.datamay not be a string or may not be valid percent-encoding.decodeURIComponentthrowsURIError: malformed URI sequencefor invalid%sequences (and coerces non-stringarg.datavia implicittoString()first, which can also produce invalid sequences); the resulting unhandled exception abortsdispatchHybridWebViewMessagefor that message with no fallback, silently breaking hybrid message dispatch for that event. Consider atypeof arg.data === 'string'guard plus a try/catch around the decode, falling back to the raw value on failure. (Same pattern applies to the generatedHybridWebView.jsat the equivalent line.)