Blockable WebView #10772

rseemann · 2016-11-06T19:30:27Z

Motivation

There isn't yet a fully reliable way to block, and handle, navigation to a specific url while using the WebViews. The current iOS method is at best (as mentioned here #6478 by @astreet) an unpredictable way of handling this. Android itself has no way of doing it at all.

Test Plan

A usage example was built in the UIExplorer WebView section. The controlled webview renders the react-native GitHub and blocks any navigations that would take the user to a non github.com urls or /login within github.com. It handles both blockage differently, showing a different component when the login url is identified or taking the users to the browser on the non github case.

facebook-github-bot · 2016-11-06T19:31:14Z

By analyzing the blame information on this pull request, we identified @jacobp100 and @spicyj to be potential reviewers.

jacobp100 · 2016-11-06T19:43:27Z

I’m reviewing a similar PR at #10654. We’ll need to decide what direction this wants to go in, but I would suggest this looks a lot more flexible, and could incorporate the other PR (I think without additional work).

cbrevik · 2016-11-07T08:30:52Z

Yeah, this PR seems achieve the same thing as mine. I was inspired by the same issue (#6478).

The reason I didn't go for a regex-based solution though is because I was under the impression that there was a divergence in regex support between Java and Objective-C. I haven't looked into that in detail, so maybe someone else can comment on that? It would hinder consistency between platform though, and make the behaviour unpredictable.

Other than that it seems pretty similar, and I wouldn't mind closing my PR in favor of this if the regex bit is a non-issue.

rseemann · 2016-11-07T08:47:09Z

I'm no regex expert, but from what I understand all patterns that work for JS should work for the other 2 languages. It seems that JS have less available patterns, but the ones that it does have should work seemingly on Java and Obj-C. So if the regex that is written is "JS friendly" it should work for Java and Obj-C. I have this code, using this library in production at the moment and the checks that we used, which I believe are the major people uses, are checking for some URL, checking for the absence of a word in a URL, checking protocols, and they all proved the same for all platforms.

jacobp100 · 2016-11-07T09:01:48Z

Do you have any source that compares the regular expressions in JS, Android, and iOS? If you’re sure that any JS regular expression will be valid for Android and iOS, it might be worth it in dev mode to check that we can construct RegExps from the strings.

rseemann · 2016-11-07T10:14:37Z

Nothing that neatly done. This statement of mine comes from the time I needed to find regex patterns, specially for the lookahead ones, and saw some of them existing in iOS and Java, but not in JavaScript.

jacobp100 · 2016-11-07T11:16:48Z

I did notice a few minor inconsistencies (\0 and \v). But these are unlikely to affect anything in real code. But if we go down this route, I still think we should check whether it is possible to construct valid JS RegExps and warn otherwise (a check in the render method should suffice).

Alternatively, could it be made simpler by not using RegExps and using simple wildcards instead (like those in Windows Explorer)? https://*, or *login*? It’s better for consistency, but I’m not sure about the flexibility for a user, and not sure if it’ll further complicate the code. Just a thought anyway.

rseemann · 2016-11-07T11:19:43Z

The wildcards wouldn't work for some more complex use cases, where blacklisting and whitelisting can me mixed.

jacobp100 · 2016-11-07T11:24:55Z

Could that be fixed by adding a mode or shouldMatch flag in each policy?

rseemann · 2016-11-07T11:40:35Z

I did that in the first version, but I found that it gets much more complicated then a generic regex.

rseemann · 2016-11-08T10:42:27Z

@jacobp100 should I fix the ci errors then so we can merge this, or we need further approval?

jacobp100 · 2016-11-08T10:49:17Z

I can’t merge it myself, but it can’t be merged with CI errors (if they’re caused by this).

cbrevik · 2016-11-08T11:10:13Z

Does the blocking work on Android for the initially set source URL?
I had to add the check in the setSource method (as well as shouldOverrideUrlLoading) in ReactWebViewManager.java in order to ensure that it was blocked in all cases - and not just redirects.

rseemann · 2016-11-08T11:14:21Z

The initial source is never blocked, not in Android nor in iOS, actually. The main goal is to block navigations made from the webview, not the react code. Since that is something we have control already I supposed we shouldn't block that.

cbrevik · 2016-11-08T12:17:54Z

I'm not sure that's right @rseemann. For RN at least, it seems to be that shouldStartLoadWithRequest is called for every request on iOS, which blocks the initial source. While shouldOverrideUrlLoading on Android is only called for redirects, which would not block the initial source.

rseemann · 2016-11-08T12:42:20Z

That is correct. What I meant is that the way the code was implemented was done in a way that neither iOS nor Android blocks the initial loading. That was done due to the fact that we understand that the setting of the source can be controlled from the react code already, while this policies would be used to block the navigation from the webview.

cbrevik · 2016-11-08T13:06:14Z

Hm, but testing with your example in WebViewExample.js, it does seem like the initial iOS source is blocked if you set it to anything other than github.com?

rseemann · 2016-11-08T13:07:07Z

Is that so? I'll take a look at it then. It shouldn't.

rseemann · 2016-11-08T15:02:19Z

@cbrevik what url are you testing? I tested Facebook's URL and what occur is that facebook navigates - via javascript - to another URL pretty much immediately after the page is loaded, so this is not the initial load. If a URL of a website that does not navigate forward is used, the page loads as expected. Do you have the same behaviour?

cbrevik · 2016-11-08T17:44:44Z

@rseemann For some reason when I tested earlier today it consistently blocked the URL when I went into the WebView-example. But I cannot seem to reproduce that behavior now. I am probably mistaken, sorry for the false alarm!

cbrevik · 2016-11-10T09:56:23Z

ReactAndroid/src/main/java/com/facebook/react/views/webview/ReactWebViewManager.java

        if (url.startsWith("http://") || url.startsWith("https://") ||
            url.startsWith("file://")) {
-          return false;
+              boolean shouldBlock = false;


Looking over this again, I don't think this will solve the issue my PR is attempting to solve. The problem there (#10055) is with custom URL schemes. If you can only block http/https/file requests it will not be sufficient. Same thing with the iOS implementation.

that is true. We could use an even broader approach where the policies would include the schemes as well.

What’s the verdict on this?

I'd say let's push this version as is and we can come up with a more general solution later.

Why? It's just about not making the check specific to certain URL schemes? Since there's an open issue that can be fixed by this PR, I don't see the point of not doing it.

rseemann · 2016-11-10T13:57:13Z

@jacobp100 I have no idea why the tests aren't passing. Can you give me a hand here?

jacobp100 · 2016-11-10T14:18:28Z

When did you last merge master in?

rseemann · 2016-11-10T14:19:55Z

You mean rebase master into my branch? for some days now. I guess I should do it.

jacobp100 · 2016-11-10T14:22:51Z

You could try pinging someone on the Facebook team to see if they know what’s up. Has it been failing in the same way for a few days now?

jacobp100

LGTM

rseemann · 2016-12-12T10:39:21Z

@jacobp100 do you think we're moving forward with this? Are we missing something?

jacobp100 · 2016-12-12T10:50:57Z

I’m not able to merge pull requests. Try cc’ing one of the FB team.

mkonicek · 2016-12-15T19:55:58Z

Examples/UIExplorer/js/WebViewExample.js

+    );
+  }
+
+  customLoginRender() {


This should be called renderCustomLogin, or ideally should be a small component.

mkonicek · 2016-12-15T19:56:26Z

Libraries/Components/WebView/WebView.android.js

+     */
+    navigationBlockingPolicies: PropTypes.arrayOf(PropTypes.shape({
+      currentURL: PropTypes.string,
+      url: PropTypes.string,


Add docs. What is currentURL. What is url?

Seems from the blocking logic that currentURL is the "source" URL, i.e. the URL we're going from. Whilst url is the "request" URL, i.e. the URL we're going to. Maybe the naming should reflect that a bit better.

Also both of them are matched as regex, so the docs should reflect that.

mkonicek · 2016-12-15T19:57:06Z

Libraries/Components/WebView/WebView.android.js

  }
+
+  onNavigationBlocked = (event) => {
+    var {onNavigationBlocked} = this.props;


Use const

it isn't used anywhere else in the class, are you sure?

mkonicek · 2016-12-15T19:58:16Z

Libraries/Components/WebView/WebView.ios.js

+    navigationBlockingPolicies: PropTypes.arrayOf(PropTypes.shape({
+      currentURL: PropTypes.string,
+      navigationType: PropTypes.string,
+      url: PropTypes.string,


Why is there navigationType only on iOS? What are the possible values? Add docs please.

It seems like navigationType is only used in the iOS RCTWebView.h. That field isn't available in ReactWebViewManager on Android.

Though if we need navigationType as a policy at all (for blocking purposes) is unclear to me.

mkonicek · 2016-12-15T19:59:31Z

React/Views/RCTWebView.m

+
+  BOOL isJSNavigation = [request.URL.scheme isEqualToString:RCTJSNavigationScheme];
+  BOOL isTopFrame = [request.URL isEqual:request.mainDocumentURL];
+  BOOL isLoadingSourceURL = ([sourceURL isEqualToString:requestURL] && !_webView.isLoading);


What if sourceURL is nil?

mkonicek · 2016-12-15T19:59:48Z

React/Views/RCTWebView.m

  });

+  NSString *sourceURL = (NSString *)self.source[@"uri"];
+  NSString *requestURL = (request.URL).absoluteString;


Can request be nil?

mkonicek · 2016-12-15T20:00:44Z

React/Views/RCTWebView.m

+        }
+      }
+    }
+  }


This large block of code that deal with blocking should be extracted to a private method for readability, not inlined into shouldStartLoadWithRequest.

This seemed to be the way it is done in this class, and similar ones. E.g.: take a look at the webViewDidFinishLoad implementation. I agree with you about readability, just want to make sure this is the way to go.

mkonicek · 2016-12-15T20:01:06Z

React/Views/RCTWebView.m

+
+          // unverifiable policy rule
+          if (!eventValue) {
+            RCTLogWarn(@"Could not verify webview loading policy named %@. Failing policy with rules %@", key, policy);


WebView is spelled like this.

mkonicek · 2016-12-15T20:01:40Z

React/Views/RCTWebView.m

+  BOOL isTopFrame = [request.URL isEqual:request.mainDocumentURL];
+  BOOL isLoadingSourceURL = ([sourceURL isEqualToString:requestURL] && !_webView.isLoading);
+
+  // skip this for the JS Navigation handler, initial load, iFrames


The comment should explain why skip blocking in these cases.

Comments start with a capital letter.

mkonicek · 2016-12-15T20:02:40Z

React/Views/RCTWebView.m


-    NSMutableDictionary<NSString *, id> *event = [self baseEvent];
-    [event addEntriesFromDictionary: @{
+    NSMutableDictionary<NSString *, id> *_event = [self baseEvent];


Why change this to _event?

Because there is an event variable declared on line 215 and I didn't want to reuse it.

mkonicek · 2016-12-15T20:03:21Z

ReactAndroid/src/main/java/com/facebook/react/views/webview/ReactWebViewManager.java

+          FLog.w(ReactConstants.TAG, "activity not found to handle uri scheme for: " + url, e);
+        }
+        return true;
+      }


Again, all the blocking handling code should be extracted to a private method.

Same explanation as before. If we think this is the way to go, sure, it makes sense.

Please extract to a protected method instead, since that is a bit more extensible.

mkonicek · 2016-12-15T20:03:32Z

ReactAndroid/src/main/java/com/facebook/react/views/webview/ReactWebViewManager.java

+          intent.setFlags(Intent.FLAG_ACTIVITY_NEW_TASK);
+          view.getContext().startActivity(intent);
+        } catch (ActivityNotFoundException e) {
+          FLog.w(ReactConstants.TAG, "activity not found to handle uri scheme for: " + url, e);


Sentences start with a capital letter.

This came with the rebase, but sure, I changed it.

mkonicek · 2016-12-15T20:03:45Z

ReactAndroid/src/main/java/com/facebook/react/views/webview/ReactWebViewManager.java

+            navigationBlockingPolicies = null;
+            return;
+        }
+


remove extra empty line

mkonicek · 2016-12-15T20:03:48Z

ReactAndroid/src/main/java/com/facebook/react/views/webview/ReactWebViewManager.java

+        }
+
+        navigationBlockingPolicies = new ReadableMap[policies.size()];
+


remove extra empty line

mkonicek · 2016-12-15T20:04:03Z

ReactAndroid/src/main/java/com/facebook/react/views/webview/ReactWebViewManager.java

+        for (int i = 0; i < policies.size(); i++) {
+            navigationBlockingPolicies[i] = policies.getMap(i);
+        }
+    }


Indentation is 2 spaces in the Java codebase.

mkonicek · 2016-12-15T20:04:54Z

Sorry about the delay. Understand the intention now. The code needs quite a lot of changes.

cbrevik · 2017-01-06T06:21:05Z

@rseemann Do you need any help with this PR? I've found more use cases for it on my end (in different projects), so I'd love to have this merged before the next January release.

hramos · 2017-01-19T21:26:17Z

Any progress? Seems like this PR needs some changes before it can be reviewed again.

rseemann · 2017-01-20T10:28:09Z

I'll work on this very soon, but any help is appreciated :)

Liroo · 2017-03-14T07:38:28Z

Any news on this PR ? :)

hramos · 2017-07-20T17:17:32Z

This PR appears to be abandoned. Closing for now.

rseemann changed the title ~~Blockable web view~~ Blockable WebView Nov 6, 2016

facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Nov 6, 2016

jacobp100 mentioned this pull request Nov 6, 2016

Add prop + event for blacklisting URL-schemes in WebView #10654

Closed

jacobp100 approved these changes Nov 10, 2016

View reviewed changes

cbrevik reviewed Nov 10, 2016

View reviewed changes

jacobp100 approved these changes Nov 29, 2016

View reviewed changes

mkonicek reviewed Dec 15, 2016

View reviewed changes

mkonicek added GH Review: needs-revision and removed GH Review: review-needed labels Dec 15, 2016

shergin added Android Platform: iOS iOS applications. JavaScript labels May 25, 2017

hramos closed this Jul 20, 2017

		}

		navigationBlockingPolicies = new ReadableMap[policies.size()];

Blockable WebView #10772

Blockable WebView #10772

Uh oh!

Conversation

rseemann commented Nov 6, 2016

Motivation

Test Plan

Uh oh!

facebook-github-bot commented Nov 6, 2016

Uh oh!

jacobp100 commented Nov 6, 2016

Uh oh!

cbrevik commented Nov 7, 2016

Uh oh!

rseemann commented Nov 7, 2016

Uh oh!

jacobp100 commented Nov 7, 2016

Uh oh!

rseemann commented Nov 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jacobp100 commented Nov 7, 2016

Uh oh!

rseemann commented Nov 7, 2016

Uh oh!

jacobp100 commented Nov 7, 2016

Uh oh!

rseemann commented Nov 7, 2016

Uh oh!

rseemann commented Nov 8, 2016

Uh oh!

jacobp100 commented Nov 8, 2016

Uh oh!

cbrevik commented Nov 8, 2016

Uh oh!

rseemann commented Nov 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cbrevik commented Nov 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rseemann commented Nov 8, 2016

Uh oh!

cbrevik commented Nov 8, 2016

Uh oh!

rseemann commented Nov 8, 2016

Uh oh!

rseemann commented Nov 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cbrevik commented Nov 8, 2016

Uh oh!

cbrevik Nov 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rseemann commented Nov 10, 2016

Uh oh!

jacobp100 commented Nov 10, 2016

Uh oh!

rseemann commented Nov 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jacobp100 commented Nov 10, 2016

Uh oh!

jacobp100 left a comment

Choose a reason for hiding this comment

Uh oh!

rseemann commented Dec 12, 2016

Uh oh!

jacobp100 commented Dec 12, 2016

Uh oh!

rseemann commented Nov 7, 2016 •

edited

Loading

rseemann commented Nov 8, 2016 •

edited

Loading

cbrevik commented Nov 8, 2016 •

edited

Loading

rseemann commented Nov 8, 2016 •

edited

Loading

cbrevik Nov 10, 2016 •

edited

Loading

rseemann commented Nov 10, 2016 •

edited

Loading

mkonicek Dec 15, 2016 •

edited

Loading

rseemann Dec 20, 2016 •

edited

Loading