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

[webview_flutter] [url_launcher] Handle Multiwindows in WebViews #2991

Merged
merged 25 commits into from Sep 21, 2020

Conversation

bparrishMines
Copy link
Contributor

@bparrishMines bparrishMines commented Sep 2, 2020

Related Issues

Internal bug: b/159892679

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • I read and followed the Flutter Style Guide.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
  • I updated CHANGELOG.md to add a description of the change.
  • I signed the CLA.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  • No, this is not a breaking change.

@bparrishMines bparrishMines requested review from blasten, amirh and cyanglaz Sep 2, 2020
@Override
public boolean onCreateWindow(
final WebView view, boolean isDialog, boolean isUserGesture, Message resultMsg) {
final WebViewClient webViewClient =
Copy link
Contributor

@amirh amirh Sep 2, 2020

Choose a reason for hiding this comment

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

Not sure I understand this part, it seems like we're creating a new WebViewClient type and delegate some of the calls to the existing flutterWebViewClient, do we not want all functionality implemented by FlutterWebViewClient to work for the new WebView as well? (e.g should we not just use FlutterWebViewClient as the client for the new webview?)

Copy link
Contributor Author

@bparrishMines bparrishMines Sep 8, 2020

Choose a reason for hiding this comment

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

I assumed that we only wanted the new WebView to filter out non https/http calls and load any secure url. I'm not sure what else we would need it to do? Im assuming you're talking about returning WebResourceErrors and onPageStarted/onPageFinished callbacks. Won't our FlutterWebViewClient receive these after we call loadUrl?

public boolean shouldOverrideUrlLoading(
@NonNull WebView view, @NonNull WebResourceRequest request) {
final String url = request.getUrl().toString();
if (isSecure(url)) {
Copy link
Contributor

@amirh amirh Sep 2, 2020

Choose a reason for hiding this comment

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

Not sure I follow the logic here, can you comment on why we only delegate calls for http and https URLs?

Copy link

@blasten blasten Sep 3, 2020

Choose a reason for hiding this comment

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

return true;
}

private boolean isSecure(String url) {
Copy link
Contributor

@amirh amirh Sep 2, 2020

Choose a reason for hiding this comment

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

I was a little confused by the name isSecure, my initial guess when looking on the call site would have been "is it https"? though I'm not sure even why we're checking on this condition here.

final WebView view, boolean isDialog, boolean isUserGesture, Message resultMsg) {
final WebViewClient webViewClient =
new WebViewClient() {
@TargetApi(Build.VERSION_CODES.LOLLIPOP)
Copy link

@blasten blasten Sep 3, 2020

Choose a reason for hiding this comment

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

what API needs this particular version?

Copy link
Contributor Author

@bparrishMines bparrishMines Sep 8, 2020

Choose a reason for hiding this comment

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

Copy link

@blasten blasten Sep 10, 2020

Choose a reason for hiding this comment

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

understood

}

private boolean isSecure(String url) {
return url.startsWith("https://") || url.startsWith("http://");
Copy link

@blasten blasten Sep 3, 2020

Choose a reason for hiding this comment

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

I would suggest we rename this as: isJavaScriptScheme(String url) and check for url.startsWith("javascript:"). Link to https://tools.ietf.org/html/draft-hoehrmann-javascript-scheme-03 would be useful too.

Copy link
Contributor Author

@bparrishMines bparrishMines Sep 8, 2020

Choose a reason for hiding this comment

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

I would only do this after we get an ok from the internal team. This method is currently just following their suggestion.

@Override
public boolean shouldOverrideUrlLoading(WebView view, String url) {
if (isSecure(url)) {
flutterWebViewClient.shouldOverrideUrlLoading(FlutterWebView.this.webView, url);
Copy link
Contributor

@amirh amirh Sep 3, 2020

Choose a reason for hiding this comment

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

Does invoking shouldOVerrideUrlLoading has a side effect of loading the URL? It seems like our implementation of shouldOVerrideUrlLoading introduces such a side effect on older webview versions only if a navigation delegate is set. Does this work when no navigation delegate is set?

Copy link
Contributor Author

@bparrishMines bparrishMines Sep 10, 2020

Choose a reason for hiding this comment

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

I added a test, so it does still work without a navigation delegate.

@bparrishMines
Copy link
Contributor Author

bparrishMines commented Sep 10, 2020

@amirh @blasten is this ready to land?

@@ -828,6 +828,40 @@ void main() {
final String currentUrl = await controller.currentUrl();
expect(currentUrl, 'about:blank');
});

testWidgets(
'can open new window and go back',
Copy link

@blasten blasten Sep 10, 2020

Choose a reason for hiding this comment

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

this one is good, but what about a test case like the one in the internal doc?

Copy link
Contributor Author

@bparrishMines bparrishMines Sep 11, 2020

Choose a reason for hiding this comment

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

I'm not sure how to do a test like that. I would need to load a link that tries to open a window and test to see if javascript ran?

Copy link

@blasten blasten left a comment

LGTM + nit about adding test case

<!DOCTYPE html><html>
<head><title>Resize test</title>
<script>
setTimeout(function() {
Copy link

@blasten blasten Sep 11, 2020

Choose a reason for hiding this comment

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

this document needs to be inside the <iframe>

}, 0);
</script>
</head>
<body onload="onLoad();" bgColor="blue">
Copy link

@blasten blasten Sep 12, 2020

Choose a reason for hiding this comment

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

onLoad isn't defined. Actually, you might be able to remove the setTimeout and just define:

function onLoad() {
  window.open('javascript:var elem = document.createElement("p");elem.innerHTML = "<b>Executed JS in parent origin: "+window.location.origin+"</b>"; document.body.append(elem);alert("XSS in doc.domain: "+document.domain+", win.origin: "+window.location.origin)');
}

final WebViewController controller = await controllerCompleter.future;
final String result = await controller.evaluateJavascript(
'document.querySelector("p") && document.querySelector("p").textContent');
expect(result, isEmpty);
Copy link

@blasten blasten Sep 12, 2020

Choose a reason for hiding this comment

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

This might be racy.

How do we know that the code inside the iframe has run at this point? One solution is to add a onLoad handler to iframe and set some global state once the event has fired. e.g. window.iframeLoaded = true. Then, at this point check that window.iframeLoaded is true and document.querySelector("p") && document.querySelector("p").textContent is empty.

@bparrishMines bparrishMines requested a review from mklim as a code owner Sep 17, 2020
@bparrishMines bparrishMines changed the title [webview_flutter] [url_launcher] Handle Multiwindows in WebViews [WIP][webview_flutter] [url_launcher] Handle Multiwindows in WebViews Sep 17, 2020

final WebViewController controller = await controllerCompleter.future;
final String result = await controller.evaluateJavascript(
'iframeLoaded && document.querySelector("p") && document.querySelector("p").textContent',
Copy link

@blasten blasten Sep 17, 2020

Choose a reason for hiding this comment

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

I was thinking something like this:

expect(controller.evaluateJavascript('iframeLoaded'), completion('true'));

expect(controller.evaluateJavascript('document.querySelector("p") && document.querySelector("p").textContent'), isEmpty);

Copy link
Contributor Author

@bparrishMines bparrishMines Sep 18, 2020

Choose a reason for hiding this comment

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

Actually, when iframeLoaded was false, it returned 'false', but I understand what you mean. done

@bparrishMines bparrishMines changed the title [WIP][webview_flutter] [url_launcher] Handle Multiwindows in WebViews [webview_flutter] [url_launcher] Handle Multiwindows in WebViews Sep 18, 2020
@bparrishMines bparrishMines removed the WIP label Sep 18, 2020
(WidgetTester tester) async {
final String openWindowTest = '''
<!DOCTYPE html><html>
<head><title>Resize test</title>
Copy link

@blasten blasten Sep 18, 2020

Choose a reason for hiding this comment

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

uber nit: resize test -> XSS test

Copy link

@blasten blasten left a comment

LGTM! Thanks for the test.

@bparrishMines bparrishMines merged commit e80865a into flutter:master Sep 21, 2020
2 checks passed
@bparrishMines bparrishMines deleted the eat_js_webview branch Sep 21, 2020
danielroek pushed a commit to Baseflow/flutter-plugins that referenced this pull request Oct 1, 2020
jorgefspereira pushed a commit to jorgefspereira/plugins_flutter that referenced this pull request Oct 10, 2020
FlutterSu pushed a commit to FlutterSu/flutter-plugins that referenced this pull request Nov 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants