WebView URL filtering

[t-arn]

This PR aims to implement URL filtering for WebView on Windows and Android. Possible use cases are:

• preventing users from navigating off the site intended to be handled by your application's WebView
• preventing users from navigating to known malicious web sites

PR Checklist:

• [x] All new features have been tested
• [x] All new features have been documented
• [x] I have read the CONTRIBUTING.md file
• [x] I will abide by the code of conduct

[t-arn]

@freakboy3742 I wonder if it is possible to make the on_navigation_starting handler async when it is being called by a native event. Aren't those native events always synchronous?

[freakboy3742]

@freakboy3742 I wonder if it is possible to make the on_navigation_starting handler async when it is being called by a native event. Aren't those native events always synchronous?

Yes, native event handlers are always synchronous - but Toga events must be able to be defined asynchronously. That's why they return a future; we need to make sure that if a future is returned, we wait for that future.

[t-arn]

@freakboy3742 I implemented support for synchronous and asynchronous on_navigation_starting handlers for Windows. The code still contains a lot of print statements for easier debugging. If you approve of this approach, I'll implement something similar for Android and clean up the code.

[t-arn]

@freakboy3742 The "allowed" and "not allowed" lists are meant to cache the user's decision whether to allow the URL or not. I think, the user will not want to be asked for the same URL again and again.

As for setting the callback method: I didn't realize that wrapped_handler already has the "on completion" functionality. I'll rewrite the code and use the cleanup method.

[t-arn]

We could make it configurable whether the user's decisions should be saved, for example by setting WebView.save_user_url_permissions=True before setting the on_navigation_starting handler. What do you think? Should the default be True or False?

[freakboy3742]

We could make it configurable whether the user's decisions should be saved, for example by setting WebView.save_user_url_permissions=True before setting the on_navigation_starting handler. What do you think? Should the default be True or False?

I see no practical use for that API. A method that returns False for https://foobar.com will always return False for that URL. There's no need to cache it.

If the intention is to allow user interaction on specific URLs, and cache answers - that's a decision the developer can make if that feature makes sense to have in an app. If nothing else, any caching scheme like that would need to have stored user preferences, a user-clearable cache, and more - so having it baked into Toga seems like massive overreach.

[t-arn]

@freakboy3742 I refactored and simplified the Winforms code by using the cleanup method of wrapped_handler

[t-arn]

@freakboy3742 I defined on_navigation_starting cleanup method as an inner method, but the url parameter is always None inside cleanup. Is there something wrong with how I changed the code?

The webview._requested_url is still there and set, but not used anymore. I will remove it when the url parameter is working, but currently I don't see how to make it work without webview._requested_url

[freakboy3742]

@freakboy3742 I defined on_navigation_starting cleanup method as an inner method, but the url parameter is always None inside cleanup. Is there something wrong with how I changed the code?

The webview._requested_url is still there and set, but not used anymore. I will remove it when the url parameter is working, but currently I don't see how to make it work without webview._requested_url

Ah - on closer inspection - the url argument to on_navigation_started will alway be None - because the method is a setter. It's not invoked when the navigation starts; it's invoked when the handler is installed - so url will always be None (and, in fact, there's no way to pass in anything to that argument at all).

So - I suspect what is needed here is a modification to wrapped_handler so that the cleanup method also accepts the *args and **kwargs passed to the event handler (in this case, url). AFAICT, all the existing uses of cleanup don't use any additional kwargs on their events - this would be the first usage on a handler that has arguments in addition to the widget.

[t-arn]

@freakboy3742

So - I suspect what is needed here is a modification to wrapped_handler so that the cleanup method also accepts the *args and **kwargs passed to the event handler (in this case, url)

How would this change help me? The cleanup method is also set when setting the on_navigation_handler. At this point, the url to be checked is not known yet. The url is only known, when the on_navigation_handler is actually called from the native event. Or do I misunderstand something here?

[freakboy3742]

@freakboy3742

So - I suspect what is needed here is a modification to wrapped_handler so that the cleanup method also accepts the *args and **kwargs passed to the event handler (in this case, url)

How would this change help me? The cleanup method is also set when setting the on_navigation_handler. At this point, the url to be checked is not known yet. The url is only known, when the on_navigation_handler is actually called from the native event. Or do I misunderstand something here?

What I'm saying is that wrapped_handler needs to be modified so that the cleanup method is invoked with the same args and kwargs as the actual navigation handler. The URL doesn't need to be known at time of definition - it's an argument to the cleanup handler, populated when the navigation handler completes.

[t-arn]

@freakboy3742 I now modified wrapped_handler and handler_with_cleanup to pass the kwargs to the cleanup method. I am not sure if the change in wrapped_handler is really needed. It's the change in handler_with_cleanup which is important. But for consistency reasons, I also made the change in wrapped_handler.

The URL filtering now works fine with synchronous and asynchronous handlers. The current example code uses the asynchronous handler.

The code still contains a lot of debugging output. If you generally approve of the way taken to implement the URL filtering, I will cleanup the code and finalise the PR.

[freakboy3742]

Thanks for those updates; four things that stood out:

1. We've recently moved out documentation to Markdown; so the changenote now needs to be a .md file, not a .rst file - but I saw a couple of other RST bits in the doc changes (like using double backticks for literals)
2. The docs build is currently breaking; it looks like a spelling issue (I think using WebView rather than webview will fix that?)
3. There's a merge conflict with the webview example
4. The handler cleanup changes will need tests - that probably won't be picked up by coverage, but it's an obvious gap that isn't being tested.

There's also the outstanding question from a past review; the answer to that question will be fairly significant on whether this PR can be landed at all.

[t-arn]

@freakboy3742 Here's the answer to the outstanding question regarding Android: The changes in Android's WebView cause an app crash when the staticProxy is missing in pyproject.toml, even when I use the unmodified example code from main :-(

This error is shown in the log on app start: E/AndroidRuntime: FATAL EXCEPTION: main E/AndroidRuntime: Process: org.beeware.toga.examples.webview, PID: 28905 E/AndroidRuntime: java.lang.RuntimeException: Unable to start activity ComponentInfo{org.beeware.toga.examples.webview/org.beeware.android.MainActivity}: com.chaquo.python.PyException: java.lang.NoClassDefFoundError: toga_android.widgets.webview.TogaWebClient

So, we cannot merge this PR until Toga itself can declare a list of classes that need a static proxy.

How do we proceed? Should I remove the changes in Android and just finalize the PR for winforms? I would then create a new PR with only the Android changes (async handlers are yet to be implemented there) and a feature request for Toga and Briefcase?

[freakboy3742]

So, we cannot merge this PR until Toga itself can declare a list of classes that need a static proxy.

Ok - that's going to be a blocker then. We can't add code to the definition of WebView that prevents an Android app from working at all.

How do we proceed? Should I remove the changes in Android and just finalize the PR for winforms? I would then create a new PR with only the Android changes (async handlers are yet to be implemented there) and a feature request for Toga and Briefcase?

It sounds like we might need to take that approach. Android's WebView is already missing features because of the need for a static proxy (there's no on_load_url callback support); this becomes another feature that needs to be documented in the same way.

Working out how to represent the static_proxy requirements of Android apps so that Briefcase (and other tools) are able to satisfy that requirement is a bigger task - but definitely one that requires a solution.

[mhsmith]

https://github.com/chaquo/chaquopy/issues/881 covers the general issue.

For this PR, I think it should be possible to make the Android feature conditional on the build_gradle_extra_content being present. You didn't include the full stack trace of the exception, but I think it probably arises from the class statement containing the static_proxy call, in which case you can surround that with an exception handler.

[t-arn]

@mhsmith Many thanks for your response! Following your advice, I now moved the class TogaWebClient(static_proxy(WebViewClient)) into a separate module and I import it like this:

class WebView(Widget):
    SUPPORTS_ON_WEBVIEW_LOAD = False

    def create(self):
        self.native = A_WebView(self._native_activity)
        try:
            from .webview_static_proxy import TogaWebClient
            client = TogaWebClient(self)
        except BaseException as ex:
            SUPPORTS_ON_NAVIGATION_STARTING = False
            client = WebViewClient()
            msg  = 'chaquopy.defaultConfig.staticProxy("toga_android.widgets.webview_static_proxy") '
            msg += 'missing in pyproject.toml section "build_gradle_extra_content"\n'
            msg += 'on_navigation_starting handler is therefore not available'
            print(msg)
        # Set a WebViewClient so that new links open in this activity,
        # rather than triggering the phone's web browser.
        self.native.setWebViewClient(client)

This now works with and without the staticProxy setting in pyproject.toml :-) When the setting is missing, following messages are logged on app start:

I/python.stdout: chaquopy.defaultConfig.staticProxy("toga_android.widgets.webview_static_proxy") missing in pyproject.toml section "build_gradle_extra_content"
I/python.stdout: on_navigation_starting handler is therefore not available

But the webview example works (without the URL filtering)

I will now add the async handler for Android as well, clean up the code and add tests for the changed cleanup handler. How do I write such a test for the dummy backend that in fact never fires the on_navigation_starting event??

[t-arn]

@mhsmith What is the minimal Python Exception/Error that needs to be catched to handle Java ClassNotFoundException? I tried AttributeError and that did not catch the Java Exception

[mhsmith]

Like this:

from java.lang import NoClassDefFoundError

try:
    ...
except NoClassDefFoundError:
    ...

[t-arn]

@mhsmith Thank you, I'll try this