Blazor Open Links in Browser with Configurability #4645

TanayParikh · 2022-02-12T01:22:18Z

Opens link in external browser by default instead of within the WebView
Optionally allows opening links in the Blazor WebView by attaching to the ExternalNavigationStarting event and setting the ExternalLinkNavigationPolicy.OpenInWebView.
- Likewise allows cancelling navigation via ExternalLinkNavigationPolicy.CancelNavigation
Always opens _blank target links in external browser

Status:

Fixes: #4357
Fixes: #4338

TanayParikh · 2022-02-12T01:23:45Z

src/BlazorWebView/src/Maui/Android/WebKitWebViewClient.cs

+				if (uri.Host == "0.0.0.0" &&
+					view is not null && 
+					request is not null && 
+					request.IsRedirect && 
+					request.IsForMainFrame)
 				{
 					view.LoadUrl(uri.ToString());
 					return true;
 				}


It's not totally clear to me when/why this code path gets exercised. Could anyone offer insights?

Maybe setting window.location in JS? Just a guess. Much of this code is likely copied from the non-Blazor WebView, so it could be for scenarios we don't really have in Blazor.

src/BlazorWebView/src/Maui/BlazorWebView.cs

Eilon · 2022-02-15T18:40:54Z

src/BlazorWebView/src/Maui/Android/WebKitWebViewClient.cs

+				if (uri.Host == "0.0.0.0" &&
+					view is not null && 
+					request is not null && 
+					request.IsRedirect && 
+					request.IsForMainFrame)
 				{
 					view.LoadUrl(uri.ToString());
 					return true;
 				}


Maybe setting window.location in JS? Just a guess. Much of this code is likely copied from the non-Blazor WebView, so it could be for scenarios we don't really have in Blazor.

src/BlazorWebView/src/Maui/BlazorWebView.cs

src/BlazorWebView/src/Maui/BlazorWebViewHandler.cs

src/BlazorWebView/src/Maui/ExternalLinkMode.cs

src/BlazorWebView/src/Maui/Android/WebKitWebViewClient.cs

* Blazor Windows Open Links in Browser with Configurability Windows portion of #4338 * TryCreate URI

(cherry picked from commit 35f637e)

Microsoft.Maui.sln

SteveSandersonMS · 2022-02-16T13:48:23Z

I'm a bit uncertain about ExternalLinkMode as an API concept. Having it as a WebView-level setting means that for any given webview, either all external links open internally, or none of them do.

The more common requirement, I would think, is allowing specific URL patterns to open internally while others are forced to open externally. Allowing all possible URLs to open internally would be a security issue, even if there are some URLs that you do want to open internally (e.g., you company's "help and support" pages). I was expecting our API to be something more like a virtual method you can override to receive the URL that is being navigated to, and return a flag to say whether (for this specific URL) it should be allowed to open internally.

Apologies if this has already been considered and I've missed some context here!

TanayParikh · 2022-02-16T16:53:18Z

I was expecting our API to be something more like a virtual method you can override to receive the URL that is being navigated to, and return a flag to say whether (for this specific URL) it should be allowed to open internally.

I think this makes sense. @Eilon had also suggested an event based option for us to consider but I was running into some issues with how the different platforms handle navigation at different stages.

I'm leaning towards an implementation which just takes an optional delegate as the arg (instead of ExternalLinkMode). If the delegate is provided then we simply call it when deciding whether or not to open the link within the WebView. How does this sound?

Eilon · 2022-02-16T17:11:12Z

How does the app developer pass in the delegate? Presumably it's an event on the control?

TanayParikh · 2022-02-16T17:19:01Z

How does the app developer pass in the delegate?

During Blazor WebView initialization:

maui/src/Controls/samples/Controls.Sample/Pages/Blazor/BlazorPage.xaml.cs

Lines 32 to 39 in d3f08ea

    
           var bwv = new BlazorWebView 
        
           { 
        
           	// General properties 
        
           	BackgroundColor = Colors.Orange, 
        
           	// BlazorWebView properties 
        
           	HostPage = @"wwwroot/index.html", 
        
           };

is what I had in mind.

Eilon · 2022-02-16T17:20:26Z

@TanayParikh - sure, but using what syntax? An event? Events are the canonical pattern for controls.

TanayParikh · 2022-02-17T02:38:02Z

@TanayParikh - sure, but using what syntax? An event? Events are the canonical pattern for controls.

The issue with doing an EventCallback<ExternalLinkEventArgs> is the InvokeAsync to actually call the callback. This doesn't work with the synchronous event handlers we have for actually working with navigation on the individual platforms (we're unable to await in those contexts).

I was proposing just using a non-event delegate function, granted, it wouldn't be the canonical approach.

SteveSandersonMS · 2022-02-17T11:34:23Z

I was proposing just using a non-event delegate function, granted, it wouldn't be the canonical approach.

Sounds reasonable to me. I agree that EventCallback<T> would be sketchy as it implies the option to make choices asynchronously.

We could do it via a plain C# event in which we pass some new NavigationInfo parameter giving the new URL and exposing an OpenInWebView callback, e.g.:

myBlazorWebView.OnExternalNavigationStarting += (sender, eventArgs) =>
{
    if (ThisLooksLikeATrustworthyUrl(eventArgs.Url))
    {
        eventArgs.OpenInWebView();
    }
};

... but TBH that seems less ergonomic than your regular-delegate proposal. Maybe something like:

// If not set, default to NavigationPolicy.OpenInSystemBrowser
myBlazorWebView.NavigationPolicy = navigationInfo =>
{
    return ThisLooksLikeATrustworthyUrl(navigationInfo.Url) ? NavigationPolicy.OpenInWebView :  NavigationPolicy.OpenInSystemBrowser;
};

We should check whether the new event/callback fires for all navigations (including ones handled by Blazor's client-side router), or only ones that aren't handled by the client-side router. The API name should account for this, e.g., call it ExternalNavigationPolicy or just NavigationPolicy depending on this distinction.

TanayParikh · 2022-02-17T19:20:19Z

Thanks @SteveSandersonMS & @Eilon, I've updated via 3c7c2d0.

We should check whether the new event/callback fires for all navigations (including ones handled by Blazor's client-side router), or only ones that aren't handled by the client-side router.

We're specifically looking at "external" links (defined by any Uri.Host other than 0.0.0.0).

Ex. https://github.com/dotnet/maui/pull/4645/files#diff-491a4b35962629acb0b5b087bac0135dc974d6f395288b07eae1f8e0de608a00R47

src/BlazorWebView/src/Maui/BlazorWebView.cs

src/BlazorWebView/src/Maui/BlazorWebViewHandler.cs

src/BlazorWebView/src/SharedSource/ExternalLinkNavigationInfo.cs

src/BlazorWebView/src/SharedSource/ExternalLinkNavigationPolicy.cs

src/BlazorWebView/src/SharedSource/WebView2WebViewManager.cs

src/BlazorWebView/src/Maui/iOS/BlazorWebViewHandler.iOS.cs

TanayParikh · 2022-02-19T01:06:52Z

when working with controls the only "ergonomic" pattern is to use events. It also won't work in any tooling (e.g. in a property grid, XAML Intellisense, etc.).

Thanks for the clarification @Eilon, I believe bdfabfd should resolve this. Had to do a sort of awkward workaround using an Action<ExternalLinkNavigationInfo> to get around event invocation from the source, but it gets the job done and it works cross platform (verified on Android/MAUI & WPF).

Please ignore the code comments for now, will update them all once you're happy with this design to minimize churn.

Platforms left to do:

Maui
- Windows (just need to re-test)
- iOS
- MacOS Catalyst
WinForms

Eilon · 2022-02-19T05:22:19Z

Thanks for the update, will review soon. I'm out on Monday too but I will try to review before then so you're not blocked.

Eilon

OK, I think this is looking pretty good (just some fairly small comments). This really hinges on whether this actually works, or not, though 😁 I have no reason to think it wouldn't, but, boy oh boy, this PR gets to deal with all six platforms we support, each with its own unique webview interop, event patterns, launcher APIs, etc.

src/BlazorWebView/src/Maui/BlazorWebView.cs

src/BlazorWebView/src/SharedSource/WebView2WebViewManager.cs

src/BlazorWebView/src/Wpf/BlazorWebView.cs

Eilon

A few more comments.

src/BlazorWebView/src/Maui/BlazorWebView.cs

Eilon · 2022-02-23T23:24:14Z

src/BlazorWebView/src/Maui/BlazorWebViewHandler.cs

 using Microsoft.Maui;
 using Microsoft.Maui.Handlers;

 namespace Microsoft.AspNetCore.Components.WebView.Maui
 {
 	public partial class BlazorWebViewHandler
 	{
-		public static PropertyMapper<IBlazorWebView, BlazorWebViewHandler> BlazorWebViewMapper = new(ViewHandler.ViewMapper)
+		private static readonly PropertyMapper<IBlazorWebView, BlazorWebViewHandler> BlazorWebViewMapper = new(ViewMapper)


Was this change to private deliberate? The mappers are supposed to be public so that derived types can re-use them. Examples: https://github.com/dotnet/maui/blob/main/src/Core/src/Handlers/Button/ButtonHandler.cs#L19-L41

I think making it readonly is good, though interestingly in the one I linked they are not.

I logged #4870 to see if they should be readonly across all of MAUI.

updated to public static readonly.

Eilon · 2022-02-23T23:31:49Z

src/BlazorWebView/src/SharedSource/ExternalLinkNavigationEventArgs.cs

+		/// <summary>
+		/// External <see cref="Uri">URI</see> to be navigated to.
+		/// </summary>
+		public Uri Uri { get; set; }


Should this have a setter? I think not?

I don't see any harm in having it? Wanted to support the hypothetical use case of redirecting the navigation request based on the existing Uri.

Doesn't seem necessary because that isn't the point of this. If someone wants a link to go somewhere else, they should just do that to begin with. Also, the current code doesn't even always work that way with a modified event args URL because in at least some cases it uses the original URL and not the one from the event args that may have been modified:

https://github.com/dotnet/maui/pull/4645/files#diff-75c4562a2bc68615b8d28c071b33a112272d1e08e4ba087c461736787ff8f534R199

Eilon · 2022-02-23T23:35:58Z

src/BlazorWebView/src/SharedSource/ExternalLinkNavigationPolicy.cs

+		OpenInExternalBrowser,
+
+		/// <summary>
+		/// Opens anchor tags <![CDATA[<a>]]> in the WebView. This is not recommended.


This still needs some updated comment. Something like:

Suggested change

/// Opens anchor tags <![CDATA[<a>]]> in the WebView. This is not recommended.

/// Opens anchor tags <![CDATA[<a>]]> in the WebView. This is not recommended unless the content of the URL is fully trusted.

@blowdart - any suggestion on how to word this? This is related to the threat model I updated earlier. (We'll later have regular docs too, but I think it's nice to have a reasonable doc comment warning here too.)

src/BlazorWebView/src/Maui/BlazorWebViewHandler.cs

TanayParikh · 2022-02-24T00:34:32Z

Thanks for the review @Eilon. I believe all the feedback should now be addressed, and all 6 platforms should be working. I'll do a quick validation round across all the platforms before final merge.

TanayParikh · 2022-02-24T00:50:30Z

src/BlazorWebView/src/SharedSource/ExternalLinkNavigationEventArgs.cs

+	/// Used to provide information about a link (<![CDATA[<a>]]>) clicked within a Blazor WebView.
+	/// 
+	/// Anchor tags with target="_blank" will always open in the default
+	/// browser and the ExternalNavigationStarting event won't be called.


Can't cref the ExternalNavigationStarting event because this class is in the SharedSource and is used with ExternalNavigationStarting in Maui, WinForms & WPF.

TanayParikh · 2022-02-24T00:57:13Z

We'll need to add some docs for this new functionality (cc/ @guardrex) alongside maybe having this part of the Preview 14 announcement given we changed defaults(?).

Instructions

Register to the ExternalNavigationStarting event and set the ExternalLinkNavigationEventArgs.ExternalLinkNavigationPolicy property to change link handling behavior. The ExternalLinkNavigationPolicy enum allows setting link handling behavior to OpenInExternalBrowser, OpenInWebView and CancelNavigation. You may utilize the ExternalLinkNavigationEventArgs.Uri property to dynamically set link handling behavior.

Please note, external links are opened in the device default browser, by default. Opening external links within the Blazor WebView is not recommended unless the content is fully trusted.

MAUI

Add the event handler to the constructor of the Page where you construct the BlazorWebView:

blazorWebView.ExternalNavigationStarting += (sender, externalLinkNavigationEventArgs) =>
{
	externalLinkNavigationEventArgs.ExternalLinkNavigationPolicy = ExternalLinkNavigationPolicy.OpenInWebView;
};

WPF

Add the ExternalNavigationStarting="Handle_ExternalNavigationStarting" attribute to the BlazorWebView control in your .xaml file:

<blazor:BlazorWebView HostPage="wwwroot\index.html" Services="{StaticResource services}" x:Name="blazorWebView" ExternalNavigationStarting="Handle_ExternalNavigationStarting" >

Add the event handler in your .xaml.cs file:

private void Handle_ExternalNavigationStarting(object sender, ExternalLinkNavigationEventArgs externalLinkNavigationEventArgs)
{
	externalLinkNavigationEventArgs.ExternalLinkNavigationPolicy = ExternalLinkNavigationPolicy.OpenInWebView;
}

Winforms

In the contructor of the Form containing the BlazorWebView control, add the following event registration:

blazorWebView.ExternalNavigationStarting += (sender, externalLinkNavigationEventArgs) =>
{
        externalLinkNavigationEventArgs.ExternalLinkNavigationPolicy = ExternalLinkNavigationPolicy.OpenInWebView;
};

blowdart · 2022-02-24T14:19:57Z

Opens anchor tags <![CDATA[<a>]]> in the WebView. This is not recommended unless the content of the URL is fully trusted.

Would this apply to all hrefs in the webview? That seems horrible, because local references would be trusted somewhat. If it was external links it'd be easier to describe. "Allows navigation to external links in the WebView. When true this setting can introduce security concerns and should not be enabled unless you can ensure all external links are fully trusted."

Also we now have a habit of naming properties like this as InsecureSomethingOrOther to make it even clearer. OpenInExternalBrowser = false is secure, OpenInExternalBrowser = true potentially isn't. Making it an enum rather than a bool with the option "AllowInsecureOpeningOfLinksInsideWebView" would be clearer.

TanayParikh · 2022-02-24T19:16:35Z

Would this apply to all hrefs in the webview? That seems horrible, because local references would be trusted somewhat. If it was external links it'd be easier to describe.

Yes, this configurability is specifically designed for external URLs, defined as URLs with a Uri.Host != "0.0.0.0".

I've updated the enum to InsecureOpenInWebView and also updated the doc comments to better reflect that this is for external links: d4fb97b

Eilon · 2022-02-24T19:27:41Z

src/BlazorWebView/src/SharedSource/ExternalLinkNavigationEventArgs.cs

+		/// <summary>
+		/// External <see cref="Uri">URI</see> to be navigated to.
+		/// </summary>
+		public Uri Uri { get; set; }


Doesn't seem necessary because that isn't the point of this. If someone wants a link to go somewhere else, they should just do that to begin with. Also, the current code doesn't even always work that way with a modified event args URL because in at least some cases it uses the original URL and not the one from the event args that may have been modified:

https://github.com/dotnet/maui/pull/4645/files#diff-75c4562a2bc68615b8d28c071b33a112272d1e08e4ba087c461736787ff8f534R199

src/BlazorWebView/src/SharedSource/ExternalLinkNavigationPolicy.cs

Eilon

I approve this message.

Blazor Android Open Links in Browser with Configurability

53f79a6

TanayParikh requested a review from a team as a code owner February 12, 2022 01:22

TanayParikh commented Feb 12, 2022

View reviewed changes

src/BlazorWebView/src/Maui/BlazorWebView.cs Outdated Show resolved Hide resolved

jsuarezruiz added the area-blazor Blazor Hybrid / Desktop, BlazorWebView label Feb 14, 2022

rmarinho requested a review from Eilon February 14, 2022 14:20

TanayParikh mentioned this pull request Feb 14, 2022

Blazor Windows Open Links in Browser with Configurability #4680

Merged

Eilon requested changes Feb 15, 2022

View reviewed changes

Eilon reviewed Feb 15, 2022

View reviewed changes

src/BlazorWebView/src/Maui/Android/WebKitWebViewClient.cs Outdated Show resolved Hide resolved

TanayParikh and others added 2 commits February 15, 2022 13:45

Blazor Windows Open Links in Browser with Configurability (#4680)

817166a

* Blazor Windows Open Links in Browser with Configurability Windows portion of #4338 * TryCreate URI

PR Feedback

899d90e

(cherry picked from commit 35f637e)

TanayParikh requested a review from Eilon February 16, 2022 00:15

TanayParikh commented Feb 16, 2022

View reviewed changes

Microsoft.Maui.sln Outdated Show resolved Hide resolved

TanayParikh requested review from pranavkm and a team and removed request for pranavkm February 16, 2022 00:17

TanayParikh changed the title ~~Blazor Android Open Links in Browser with Configurability~~ Blazor Android/Windows Open Links in Browser with Configurability Feb 16, 2022

Merge branch 'main' into taparik/androidAnchorTagConfigurability

96a56cb

OnExternalNavigationStarting

3c7c2d0

Pranav Points

f0448be

TanayParikh force-pushed the taparik/androidAnchorTagConfigurability branch from f712ee5 to f0448be Compare February 17, 2022 19:24

Eilon requested changes Feb 18, 2022

View reviewed changes

TanayParikh requested a review from Eilon February 19, 2022 01:15

Eilon requested changes Feb 21, 2022

View reviewed changes

src/BlazorWebView/src/Maui/BlazorWebView.cs Outdated Show resolved Hide resolved

src/BlazorWebView/src/SharedSource/WebView2WebViewManager.cs Outdated Show resolved Hide resolved

src/BlazorWebView/src/Wpf/BlazorWebView.cs Outdated Show resolved Hide resolved

TanayParikh added 2 commits February 23, 2022 09:53

Merge branch 'main' into taparik/androidAnchorTagConfigurability

9022c5c

iOS & Mac Catalyst

174a169

TanayParikh changed the title ~~Blazor Android/Windows Open Links in Browser with Configurability~~ Blazor Open Links in Browser with Configurability Feb 23, 2022

TanayParikh added 2 commits February 23, 2022 13:04

Fix WPF/Winforms Browser Start

8909a19

Winforms ExternalNavigationStarting

f602372

Eilon requested changes Feb 23, 2022

View reviewed changes

PR Feedback

78c7ba0

TanayParikh commented Feb 24, 2022

View reviewed changes

src/BlazorWebView/src/Maui/BlazorWebViewHandler.cs Outdated Show resolved Hide resolved

Remove ordering dependency during property mapping

27be008

TanayParikh requested a review from Eilon February 24, 2022 00:33

TanayParikh commented Feb 24, 2022

View reviewed changes

guardrex mentioned this pull request Feb 24, 2022

Blazor Hybrid tracking dotnet/AspNetCore.Docs#24956

Closed

25 tasks

@blowdart feedback

d4fb97b

TanayParikh mentioned this pull request Feb 24, 2022

Blazor Hybrid routing and nav topic dotnet/AspNetCore.Docs#25130

Merged

Eilon requested changes Feb 24, 2022

View reviewed changes

@Eilon feedback

20b4f7c

Eilon approved these changes Feb 24, 2022

View reviewed changes

TanayParikh mentioned this pull request Feb 24, 2022

Determine non-HTTP(S) link handling behavior #4897

Closed

TanayParikh merged commit 78c1ffe into main Feb 24, 2022

TanayParikh deleted the taparik/androidAnchorTagConfigurability branch February 24, 2022 21:47

github-actions bot locked and limited conversation to collaborators Dec 21, 2023

samhouts added the fixed-in-6.0.200-preview.14.2 Look for this fix in 6.0.200-preview.14.2! label Aug 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Blazor Open Links in Browser with Configurability #4645

Blazor Open Links in Browser with Configurability #4645

TanayParikh commented Feb 12, 2022 •

edited

Loading

TanayParikh Feb 12, 2022

Eilon Feb 15, 2022

Eilon Feb 15, 2022

SteveSandersonMS commented Feb 16, 2022

TanayParikh commented Feb 16, 2022

Eilon commented Feb 16, 2022

TanayParikh commented Feb 16, 2022

Eilon commented Feb 16, 2022

TanayParikh commented Feb 17, 2022

SteveSandersonMS commented Feb 17, 2022 •

edited

Loading

TanayParikh commented Feb 17, 2022 •

edited

Loading

TanayParikh commented Feb 19, 2022 •

edited

Loading

Eilon commented Feb 19, 2022

Eilon left a comment

Eilon left a comment

Eilon Feb 23, 2022

Eilon Feb 23, 2022

Eilon Feb 23, 2022

TanayParikh Feb 24, 2022

Eilon Feb 23, 2022

TanayParikh Feb 24, 2022

Eilon Feb 24, 2022

Eilon Feb 23, 2022

TanayParikh commented Feb 24, 2022

TanayParikh Feb 24, 2022

TanayParikh commented Feb 24, 2022

blowdart commented Feb 24, 2022 •

edited by TanayParikh

Loading

TanayParikh commented Feb 24, 2022

Eilon Feb 24, 2022

Eilon left a comment

	/// Opens anchor tags <![CDATA[<a>]]> in the WebView. This is not recommended.
	/// Opens anchor tags <![CDATA[<a>]]> in the WebView. This is not recommended unless the content of the URL is fully trusted.

Blazor Open Links in Browser with Configurability #4645

Blazor Open Links in Browser with Configurability #4645

Conversation

TanayParikh commented Feb 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveSandersonMS commented Feb 16, 2022

TanayParikh commented Feb 16, 2022

Eilon commented Feb 16, 2022

TanayParikh commented Feb 16, 2022

Eilon commented Feb 16, 2022

TanayParikh commented Feb 17, 2022

SteveSandersonMS commented Feb 17, 2022 • edited Loading

TanayParikh commented Feb 17, 2022 • edited Loading

TanayParikh commented Feb 19, 2022 • edited Loading

Eilon commented Feb 19, 2022

Eilon left a comment

Choose a reason for hiding this comment

Eilon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TanayParikh commented Feb 24, 2022

Choose a reason for hiding this comment

TanayParikh commented Feb 24, 2022

Instructions

MAUI

WPF

Winforms

blowdart commented Feb 24, 2022 • edited by TanayParikh Loading

TanayParikh commented Feb 24, 2022

Choose a reason for hiding this comment

Eilon left a comment

Choose a reason for hiding this comment

TanayParikh commented Feb 12, 2022 •

edited

Loading

SteveSandersonMS commented Feb 17, 2022 •

edited

Loading

TanayParikh commented Feb 17, 2022 •

edited

Loading

TanayParikh commented Feb 19, 2022 •

edited

Loading

blowdart commented Feb 24, 2022 •

edited by TanayParikh

Loading