Oh, also, while we're looking at this: the default shouldOverrideUrlLoading doesn't appear to specialcase about: or chrome: URLs and tries to resolve those via intents as well. We probably should treat these as internal and always return false?

> I'd expect the navigation to simply proceed inside the WebView (i.e. it should return false). Is there a reason why we decided to do it this way, or is this just an oversight?

Came from this CL: https://codereview.chromium.org/1180223004/diff/40001/android_webview/java/src/org/chromium/android_webview/AwContentsClient.java

I don't think anyone considered the return value very carefully in the CL, and I agree with all your suggestions here. I guess the next bugcop can write a CL to actually do this :p

I'll take this up

A couple of points below:

chrome:// URLS

 - chrome:// URLs never make it this far. Catching them here would be redundant. Clicking a chrome:// URL is a NOOP (doesn't even send the intent).

Should we leave non-gesture navigation as-is?

 - We actually have a test (see link below) which depends on this behavior--perhaps we should consider this part of the WebView API?
 - This change might be unsafe. JavaScript can navigate to 'http://redirects-to-intent.com', which redirects to a new URL to cause the intent. Because it's a redirect, we allow the intent to be triggered. This is essentially the original exploit: JS code causing intents.
 - See the test: https://cs.chromium.org/chromium/src/android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldOverrideUrlLoadingTest.java?l=937-944

These concerns are minor, since most apps override the default behavior anyway by setting a WebViewClient. But I thought I'd bring them up.

I don't think there's any issue with treating "about:" URLs as internal, it's just kind of a special case.

>  - This change might be unsafe. JavaScript can navigate to 'http://redirects-to-intent.com', which redirects to a new URL to cause the intent. Because it's a redirect, we allow the intent to be triggered. This is essentially the original exploit: JS code causing intents.

Hmm intersting... so the current behavior is js is not allowed to do any navigation or fire intents (without user gesture). But redirects are allowed to fire intents. I think maybe redirects shouldn't be universally allowed to fire intents either, that it has to be either a browser-initiated navigation, or a renderer initiated navigation with a user intent. Then we can allow js to navigate (but not fire intents).

Anyway, you can leave this bit out of your CL for now, and probably wait until torne to come back to discuss this more.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/570a5854119d55591e3a02ac8a471a07039bf467

commit 570a5854119d55591e3a02ac8a471a07039bf467
Author: ntfschr <ntfschr@chromium.org>
Date: Wed Apr 05 03:24:56 2017

WebView: change default shouldOverrideUrlLoading

This CL changes the default shouldOverrideUrlLoading() for 'about:'
URLs. WebView will always navigate to these internally, instead of
creating intents if no WebViewClient is set.

This also refactors Java URL constants for about* URLs into the
content layer.

BUG=655149
TEST=testNullContentsClientOpenAboutUrlInWebView

Review-Url: https://codereview.chromium.org/2780403002
Cr-Commit-Position: refs/heads/master@{#461966}

[modify] https://crrev.com/570a5854119d55591e3a02ac8a471a07039bf467/android_webview/java/src/org/chromium/android_webview/AwContentsClient.java
[modify] https://crrev.com/570a5854119d55591e3a02ac8a471a07039bf467/android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldOverrideUrlLoadingTest.java
[modify] https://crrev.com/570a5854119d55591e3a02ac8a471a07039bf467/chrome/android/java/src/org/chromium/chrome/browser/IntentHandler.java
[modify] https://crrev.com/570a5854119d55591e3a02ac8a471a07039bf467/chrome/android/java/src/org/chromium/chrome/browser/UrlConstants.java
[modify] https://crrev.com/570a5854119d55591e3a02ac8a471a07039bf467/chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java
[modify] https://crrev.com/570a5854119d55591e3a02ac8a471a07039bf467/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java
[modify] https://crrev.com/570a5854119d55591e3a02ac8a471a07039bf467/chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java
[modify] https://crrev.com/570a5854119d55591e3a02ac8a471a07039bf467/chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java
[modify] https://crrev.com/570a5854119d55591e3a02ac8a471a07039bf467/chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java
[modify] https://crrev.com/570a5854119d55591e3a02ac8a471a07039bf467/chrome/android/java/src/org/chromium/chrome/browser/util/UrlUtilities.java
[modify] https://crrev.com/570a5854119d55591e3a02ac8a471a07039bf467/chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabTabPersistenceIntegrationTest.java
[modify] https://crrev.com/570a5854119d55591e3a02ac8a471a07039bf467/chrome/android/javatests/src/org/chromium/chrome/browser/ntp/RecentTabsPageTest.java
[modify] https://crrev.com/570a5854119d55591e3a02ac8a471a07039bf467/content/public/android/BUILD.gn
[add] https://crrev.com/570a5854119d55591e3a02ac8a471a07039bf467/content/public/android/java/src/org/chromium/content_public/common/ContentUrlConstants.java

Torne, do you agree that JS-based navigation should be kept as-is (see my comment on safety in c#4), or do you still think it's worth it to change?

Ping torne@, do you think we still need to change JS-based navigation or should we keep as-is (see c#4)?

OK, so this was really confusing. I think my initial proposal here was based on a misunderstanding of the current code. I don't think I realised that sendBrowsingIntent gets invoked for all navigations (including to regular web URLs) and therefore the current code is actually blocking *all* non-gesture-initiated navigations entirely.

I don't think we want it to work that way because that breaks the web, but I also don't think that was the problem I was thinking of when I filed this bug :)


So almost no app actually uses this code path (virtually everybody has a WebViewClient), but a lot of apps have in the past copied various versions of Android/WebView's default handling code here to use as the basis for their own shouldInterceptRequest implementation, and so the idea was that this should be a "good" model implementation for them to copy.

Just changing the return value to false isn't safe, though, as you note, because of the exception for redirects, so this is harder than I assumed :)

Ideally we should carry the user gesture flag across redirects, and use that instead of having a special case for redirects, but I'm not sure how hard that is. I think it's worth trying, though, because if we can't do that correctly then apps won't be able to either and the effort to expose the user gesture flag was wasted :/

> Ideally we should carry the user gesture flag across redirects, and use that instead of having a special case for redirects

Yup, this sounds like the correct approach to me. I'll see if I can try implementing this once feature work slows down.

it's probably not easy.. apparently url overloading is a sync IPC from renderer (!!), but renderer is not the source of truth for navigations, and with plznavigate, more (all?) that logic is moving to browser

Doing this correctly probably involves re-implementing url overriding again :/

Opening this up for any webview team member to finish. The remaining piece is to propagate the user gesture flag across redirects.

I think this is low priority, since most apps don't directly use the default shouldOverrideUrlLoading.

This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Still desirable - we have a bunch of other bugs about possible ways to reimplement shouldOverrideUrlLoading that may be related, also.