Skip to content
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

Improved non-HTTP(S) link handling behavior #5313

Merged
merged 21 commits into from
Mar 30, 2022

Conversation

MackinnonBuck
Copy link
Member

@MackinnonBuck MackinnonBuck commented Mar 15, 2022

Description of Change

In Blazor MAUI for Android, clicking a link with a scheme other than HTTP or HTTPS would previously sometimes crash the app or display a "Webpage not available" page. This PR resolves the undesirable behavior by making the following changes:

  • If a link's scheme is not HTTP(S), but the scheme has a registered handler, its associated activity will be started when the link is clicked (this works for schemes for installed apps, the intent scheme, etc.).
  • If a link's scheme does not have a registered handler, clicking the link does nothing (rather than displaying the "Webpage not available" page). This is consistent with the behavior on other platforms.
  • If a link uses the intent scheme, but the intent could not be resolved (or the app could not be started) and a browser_fallback_url is specified, attempt to start an activity from an intent created by the fallback URL.

Issues Fixed

Fixes #4897

Also fixed crashing issues when a scheme has no handler. This is consistent with other platforms.
@MackinnonBuck MackinnonBuck requested a review from a team as a code owner March 15, 2022 21:20
@MackinnonBuck
Copy link
Member Author

Before this gets merged, we will probably want to consider whether the behavior I've described in my previous comment is desirable. The S.browser_fallback_url feature is Chrome-specific (not some other standard) so we need to decide if it's worth it to replicate support for it. Removing support for that feature wouldn't infringe on the functionality of "intent"-schemed URIs with registered handlers.

@TanayParikh
Copy link
Contributor

TanayParikh commented Mar 15, 2022

I realize this wasn't covered in the original issue, but what happens if you use links like these on other platforms? Specifically, iOS with custom URL Schemes?

It's also possible on Windows (ex. "do you want to allow example.com to open GitHub), or deep links to the Microsoft Store.

Just understanding if we get crashes on those platforms, similar to what we saw on Android, would be good. As long as it's not a crash, this would probably fall pretty low on our priority list to address though.

@MackinnonBuck
Copy link
Member Author

@TanayParikh I've tested links with various schemes on other platforms, and they don't seem to have any issues. If the scheme is known, the link is handled correctly, and if the scheme is unknown, nothing happens when the link is clicked. This is why I changed the Android implementation to no-op when the scheme doesn't have a handler.

@MackinnonBuck MackinnonBuck requested a review from Eilon March 15, 2022 23:10
Comment on lines 59 to 60
if (callbackArgs.ExternalLinkNavigationPolicy == ExternalLinkNavigationPolicy.OpenInExternalBrowser)
{
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name OpenInExternalBrowser really could mean OpenInExternalApp. Should we consider renaming this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. What do you think - OpenInExternalApp? OpenExternal? OpenExternally? I'm fine with any. cc @TanayParikh in case he has opinions since he did the earlier version of this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name OpenInExternalBrowser really could mean OpenInExternalApp.

Would this be specific to android schemes, or would it apply to iOS custom schemes as well? Or is it handled differently for iOS?

Should we even give an option to open external/internal for intent? What would happen if the user requested OpenInWebview for an intent?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally the same APIs would have as close-to-equivalent-as-we-can behaviors across all platforms. So if we can do it for iOS too, yes.

Should we even give an option to open external/internal for intent? What would happen if the user requested OpenInWebview for an intent?

My guess is that developers would only want to use OpenInExternalBrowser or CancelNavigation for intents. But if they explicitly tell us to InsecureOpenInWebView, would it be OK to just let them? I know no good will likely come of it, so they probably shouldn't do it, but if they really want to then I'm not sure it's our responsibility to stop them.

Copy link
Contributor

@TanayParikh TanayParikh Mar 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally the same APIs would have as close-to-equivalent-as-we-can behaviors across all platforms. So if we can do it for iOS too, yes.

Assuming we're handling iOS/Windows external schemes in the same relative fashion, I think OpenExternally or OpenExternal makes sense.

I know no good will likely come of it, so they probably shouldn't do it, but if they really want to then I'm not sure it's our responsibility to stop them.

Fair enough. Can we just make sure if they do do OpenInWebView with an intent it just no-ops instead of crashing (not sure what the current code path will do with intents directly). Once we're in a better state with logging, we may also wish to add some logging in place for this to alert developers of a potentially un-intended code path being executed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this be specific to android schemes, or would it apply to iOS custom schemes as well? Or is it handled differently for iOS?

iOS will also open links with non-browser apps if a URL scheme is registered with another app (this was something I tested). So even currently, OpenInExternalBrowser doesn't always guarantee that the link will be opened in a browser app.

What would happen if the user requested OpenInWebview for an intent?

IIRC, assuming the scheme is valid, the webview will just display a "Webpage not available" page (but I can double-check this on Monday). Assuming that's the case, I agree that it's probably fine to allow that page to display without enforcing anything ourselves.

request is not null &&
request.IsRedirect &&
request.IsForMainFrame)
if (AppOriginUri.IsBaseOfPage(uri))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why we're using IsBaseOfPage here instead of IsBaseOf. Does it matter whether or not it's a page?

I thought that distinction was only relevant when deciding whether to do fallback routing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess my reasoning was that if we're trying to load a file, it seems like we should give the developer the option to open it in an external app/browser (i.e., we only load URLs in the view without invoking the ExternalNavigationStarting callback if they refer to other pages in the app). I could be making a wrong assumption here so I'm certainly happy to change it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally would have regarded "external" navigation to relate to content or actions that are outside the application, and would intuitively consider static content in wwwroot to be internal (or perhaps routing patterns like /users/{fullname} matching /users/bob.gif - Bob Gif is a possible real name).

Copy link
Contributor

@TanayParikh TanayParikh Mar 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would intuitively consider static content in wwwroot to be internal

Would that work across all content types? Do we have webview internal PDF support for instance across all the platforms?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(or perhaps routing patterns like /users/{fullname} matching /users/bob.gif - Bob Gif is a possible real name).

The developer could still support this if they wanted by adding their own handler for ExternalNavigationStarting and indicating that they want the URL to be opened in the webview (but this does seem like a bit of a hack IMO). Whereas, if we always loaded internal resources in the webview, I don't know that we have a way for the developer to override that behavior to support external navigation.

But there's something else: I think the existing fallback routing logic could still cause problems anyway for URLs ending in .<something>. I wonder if we should do something like re-processing the request as a navigation event if we fail to load an internal resource from the request URL (basically falling back to the host page if our initial guess was wrong). That way, users hopefully won't ever get the weird "There was no content at _____" page.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead, use a button or similar with an @OnClick handler that does something useful for the target file (for example, opening it in an external app using MAUI APIs)

Ah I wasn't aware we had MAUI APIs to force the loading into an external (PDF Reader) app. In that case I agree this is all fine given developers at least have the optionality to open externally should it be something they want to do.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I may have missed something here. Isn't that already our current behavior? Are you suggesting we don't go the route of UrlLoadingStrategy and just keep things as is?

I may not have been clear, my bad. I was suggesting that the current default behavior for loading file-like URLs internal to the base href is fine, but we would give developers the option to override it via UrlLoadingStrategy.

Normally I'd agree, we want to minimize the complexity however possible.

I guess my concerns with this were that:

  1. If we special-case PDF handling, what should we do about other image formats (or other file types that browsers provide a preview for)? Are we going to evaluate their support on each platform and decide what best the default behavior is?
  2. Would we ever change the default behavior if a webview on some platform adopted a sufficient built-in PDF viewer? For example, the Android webview doesn't have support for previewing PDFs (so presumably we would open in an external app), but iOS does support it. So should we open within the webview on iOS but not Android? But then what if Android someday gets its own PDF viewer, will we change the default behavior again? Maybe we decide that for now we always open PDFs externally on both platforms, but what if the webview PDF viewers on both platforms improve to the extent where it's unnecessary to do so?

That said, maybe I'm overthinking this and it's fine to only consider the PDF format for now, and always open it externally on mobile platforms. I'm fine taking that route, just wanted to share some potential concerns 🙂

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, I didn't refresh and missed a bunch of comments before posting, sorry!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MackinnonBuck This is a long thread and I'm no longer quite sure whether this is a clear plan from your perspective, or whether you're still blocked on getting anyone's input. If you're unblocked, great! But if you need anything please let us know.

Copy link
Member Author

@MackinnonBuck MackinnonBuck Mar 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should be unblocked for now, thanks @SteveSandersonMS! I think most of the work is done, the primary remaining TODOs are:

  1. Completing/verifying the new link handling functionality on iOS/Mac Catalyst.
  2. Determining what should be done about pages with URLs that look like file names. I've implemented a solution for WinUI, but I'm not sure if we want to include it in this PR (we would probably need something for the other platforms as well). My current approach is:
    • If the URL looks like it has a file extension, attempt to load that file.
    • If the URL does not have a file extension, or the file could not be found in the previous step, and WebView2 has indicated that this could be an internal navigation request, fall back on the host page.

Hoping to at least get 1. finished soon so this PR can get reviewed/merged by EOD tomorrow.

@SteveSandersonMS
Copy link
Member

This is a great improvement - thanks @MackinnonBuck! I think we'll easily be able to understand this logic years from now.

@MackinnonBuck
Copy link
Member Author

I think the big remaining question here is how we want to handle URLs that look like they point to internal files, but actually refer to pages. Do you have any thoughts on this @SteveSandersonMS @TanayParikh?

My previous comment on this: #5313 (comment)

@Eilon Eilon added the area-blazor Blazor Hybrid / Desktop, BlazorWebView label Mar 22, 2022
Page URLs with patterns that make them look like files are now supported. Still need to decide how to approach this on the other platforms.
@MackinnonBuck
Copy link
Member Author

MackinnonBuck commented Mar 24, 2022

Okay, I think this PR is ready to be reviewed again @SteveSandersonMS @TanayParikh! I'm going to add some thoughts here to provide some context on the changes I made.


I changed the ExternalNavigationStarting event to UrlLoading, so I renamed ExternalLinkNavigationEventArgs and ExternalLinkNavigationPolicy to UrlLoadingEventArgs and UrlLoadingStrategy respectively. And, of course, the event is now invoked for internal links in addition to external links (except for those with a target="_blank"). The default URL handling behavior follows a simple rule: If the link is internal (i.e. it has an internal host), it gets opened in the webview. Otherwise, it gets opened externally. This behavior can be overridden by changing the UrlLoadingEventArgs.UrlLoadingStrategy in the UrlLoading event.

This new rule does introduce a subtle change in behavior from an earlier version of this PR. Rather than doing something like AppOrigin.IsBaseOf(requestUrl) to determine which links count as "internal", the latest version of this PR simply compares the host names between the app origin URI and the request URI. My reasoning behind this is IsBaseOf() will return false if the two URIs have different schemes, and I don't think it really makes sense to open internal links externally if their scheme is not, for example, https (or app on iOS/Mac Catalyst).

The reason I bring this up is that the default behavior now makes it so if you click a link like <a href="ftp://0.0.0.0">FTP Link</a> on Android, then the "Webpage not available" page is displayed in the webview (shown as the motivating example in #4897, the issue that this PR is fixing). While attempting to open the link externally in this specific case results in a no-op rather than an eyesore (assuming there's not an app registered to handle the ftp scheme), I still think it's best to default to always open internal links internally, especially since the behavior can be overridden. Do you think I've reached the correct conclusion here?


Okay, on to the next thing: I've renamed InsecureOpenInWebView to OpenInWebView. It felt weird having the name Insecure in something that could be a default behavior, and it's not really insecure if we're loading an internal URL. I think the XML documentation comments for UrlLoadingEventArgs.UrlLoadingStrategy and UrlLoadingStrategy.OpenInWebView make the risks of opening external links in the webview clear enough, but maybe there's another API design that makes those risks unignorable.


And finally, I've also changed the MAUI WinUI fallback routing behavior to support pages with URLs that appear to have file extensions. How this works was outlined in a previous comment. This makes the behavior between all WebView2-based Blazor Hybrid platforms consistent (since WPF and WinForms already behave correctly for pages with "extensions"). I realize this change could be considered slightly off-topic for this PR, so I'm happy to undo the change if we need more time to think over the approach. I think the main downside with this change is that now WinUI is the only MAUI platform that supports this behavior. I haven't been able to come up with a good strategy for iOS/Mac Catalyst and Android yet (e.g. I don't think overzealously falling back on the host page when we get a 404 on an internal resource is desirable).

@SteveSandersonMS
Copy link
Member

Thanks @MackinnonBuck! This has ended up as a fairly significant change but I think it's good we got to this before GA. Letting developers control this behavior for all navigations is a valuable feature.

Rather than doing something like AppOrigin.IsBaseOf(requestUrl) to determine which links count as "internal", the latest version of this PR simply compares the host names between the app origin URI and the request URI.

I have to admit I don't fully understand the design intent with this. TBH I don't know why anyone would be using the 0.0.0.0 hostname for any practical purpose. Why would someone actually link to ftp://0.0.0.0/ - what do they want to happen? It seems so strange I'm not sure I'd even mind if the app does crash, unless there's some practical reason I'm missing.

If we could conclude that people aren't actually going to link to 0.0.0.0 on any scheme other than the default one used in the app, then I'd perhaps prioritize keeping the rules simple over worrying about what happens with ftp://0.0.0.0. If you're saying that opening internally gives a "page not available" error and opening it externally is a no-op, then both of those sound totally fine to me. I'd more likely go with the more common definition of internal/external (i.e., that the scheme, host, and port must all match, like with document.origin) and not try to special-case anything for same-host-different-scheme.

Again, if I'm missing some known use case for same-host-different-scheme (given the host is always 0.0.0.0), please let me know!

Okay, on to the next thing: I've renamed InsecureOpenInWebView to OpenInWebView

Sounds good.

And finally, I've also changed the MAUI WinUI fallback routing behavior to support pages with URLs that appear to have file extensions.

While I agree the behavior you've set up for WinUI is good in isolation, I do wonder if having an inconsistency across WinUI and Android/iOS might end up becoming a problem in itself. Developer might only test on Windows, then be confused about why a link to /users/bob.gif correctly loads the "Users" page on Windows but gives a 404 on iOS/Android.

I would probably err on the cautious side by going with a least-common-denominator behavior across all MAUI platforms and filing an issue to consider enhancing this in the future. Is the following viable across all?

  • If the URL matches an existing static asset, use that
  • Otherwise, if we know for sure this is not a navigation request (i.e., on WebView2, using the check about eventArgs.ResourceContext), then give a 404
  • Otherwise, fall back on the host page

Then routing to /users/bob.gif works everywhere (if it's a GIF, we serve it, otherwise we use the host page). In the special case of WinUI and <img src="/users/bob.gif"> then we will give a 404 if no such file exists, whereas on Android/iOS we'd give an HTML response which will lead to a broken image, but that's the developer's fault in creating this error.

@MackinnonBuck
Copy link
Member Author

I have to admit I don't fully understand the design intent with this. TBH I don't know why anyone would be using the 0.0.0.0 hostname for any practical purpose. Why would someone actually link to ftp://0.0.0.0/ - what do they want to happen? It seems so strange I'm not sure I'd even mind if the app does crash, unless there's some practical reason I'm missing.

TBH you're probably right that there isn't a practical purpose to this - I thought there might be some reason to use an alternate scheme that I wasn't aware of, but now that you point this out I think it's fine to not handle this case gracefully.

I'd more likely go with the more common definition of internal/external (i.e., that the scheme, host, and port must all match, like with document.origin) and not try to special-case anything for same-host-different-scheme.

Sure, that sounds good. FWIW, the code before this PR also classified "external navigation" as navigating to a link with a different host name (scheme was ignored, for example, so ftp://0.0.0.0/ would have been considered internal). I'm definitely happy to change this, though.

Is the following viable across all? ...

That seems valid to me. I initially thought it might be undesirable to provide a non-404 response for non-navigation requests that also don't match a static asset, but it does seem like that would actually work.

Thanks so much for the feedback, @SteveSandersonMS!

@MackinnonBuck
Copy link
Member Author

Coming back to my comment here:

That seems valid to me. I initially thought it might be undesirable to provide a non-404 response for non-navigation requests that also don't match a static asset, but it does seem like that would actually work.

I'm starting to think this might actually be problematic in some cases. While implementing this, I noticed that if, for example, an internal JavaScript file could not be located, you get these strange syntax errors in the console basically complaining that HTML tags are not valid JavaScript, etc. But also, if it's something like favicon.ico that isn't validated to the same extent as JavaScript, the 404 errors in the console go away, making the problem much less apparent.

@SteveSandersonMS Do you think these problems are enough to hold off on the navigation fallback behavior changes?

@TanayParikh
Copy link
Contributor

cc/ @guardrex we'll have to update https://docs.microsoft.com/en-us/aspnet/core/blazor/hybrid/routing?view=aspnetcore-6.0 based on this new API (after it's finalized/merged). @MackinnonBuck gave a great summary of the changes in this comment.

@SteveSandersonMS
Copy link
Member

@SteveSandersonMS Do you think these problems are enough to hold off on the navigation fallback behavior changes?

On the web, by default we don't do fallback for file-like URLs, so that script.js will give a 404 instead of a weird HTML-returning behavior.

However the limitation with doing the same for MAUI is that, for MAUI, there's no way to customize the routing AFAIK, so there would be no way to have the fallback (and hence deep linking) for /users/bob.smith if you actually wanted it. Am I right about that, that even by setting a custom URL loading event handler, you couldn't customize this on MAUI? Is there some way we could make that customizable, such as by returning a URL loading strategy called FallbackToHostPage or something?

Copy link
Member

@SteveSandersonMS SteveSandersonMS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving assuming this now reflects the final design that you worked out with @javiercn. Thanks for working through all the details on this one!

@MackinnonBuck MackinnonBuck merged commit 2288eea into main Mar 30, 2022
@MackinnonBuck MackinnonBuck deleted the mbuck/non-http-link-handling branch March 30, 2022 16:36
Copy link
Contributor

@TanayParikh TanayParikh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just a doc change needed 😄

@TanayParikh
Copy link
Contributor

image

That timing 😄

@MackinnonBuck
Copy link
Member Author

Oh no! Sorry about that @TanayParikh!

@mattleibow mattleibow added this to the 6.0.300-rc.1 milestone Mar 31, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Dec 22, 2023
@samhouts samhouts added the fixed-in-6.0.300-rc.1 Look for this fix in 6.0.300-rc.1! label Aug 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-blazor Blazor Hybrid / Desktop, BlazorWebView fixed-in-6.0.300-rc.1 Look for this fix in 6.0.300-rc.1!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Determine non-HTTP(S) link handling behavior
6 participants