Skip to content

Conversation

@t-arn
Copy link
Contributor

@t-arn t-arn commented May 13, 2025

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:

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

t-arn added 4 commits May 13, 2025 07:34
* implented on_navigation_started for android
* extended the webview example
* applied black
Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR; I've flagged a few issues inline, and the test suite is obviously failing on most platforms (although it looks like the reason for the failure is something that won't be an issue once you've addressed one of the comments inline).

url: str | None = None,
content: str | None = None,
user_agent: str | None = None,
on_navigation_starting=None,
Copy link
Member

Choose a reason for hiding this comment

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

This needs a type declaration (if only for documentation purposes)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, added type declaration

allow = False
message = f"Navigation not allowed to: {url}"
dialog = toga.InfoDialog("on_navigation_starting()", message)
asyncio.create_task(self.dialog(dialog))
Copy link
Member

Choose a reason for hiding this comment

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

It might be easier to make on_navigation_starting an async callback, and then await the response.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The example app now has both, a synchronous and an async handler

event = TogaNavigationEvent(webresourcerequest)
allow = self.webview_impl.interface.on_navigation_starting(
webresourcerequest.getUrl().toString()
)
Copy link
Member

Choose a reason for hiding this comment

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

I haven't checked, but this could be a problem if on_navigation_starting is an async callback, because the returned value will be a future.

style=Pack(flex=1),
)
# activate web navigation filtering on supported platforms
if getattr(self.webview._impl, "SUPPORTS_ON_NAVIGATION_STARTING", True):
Copy link
Member

Choose a reason for hiding this comment

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

Good instinct, but it's fine for the example to raise a warning if a feature isn't available.

def winforms_navigation_starting(self, sender, event):
# print(f"winforms_navigation_starting: {event.Uri}")
if self.interface.on_navigation_starting:
allow = self.interface.on_navigation_starting(event.Uri)
Copy link
Member

Choose a reason for hiding this comment

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

As with the Android version, this needs to handle a Future being returned.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not quite sure, how to do this. Is there a similar case in the code base where I could get some inspiration?

@t-arn
Copy link
Contributor Author

t-arn commented May 13, 2025

@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
Copy link
Member

@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 added 2 commits May 26, 2025 08:12
* added type declaration for handler
* added WeakrefCallable wrapper
@t-arn
Copy link
Contributor Author

t-arn commented Jun 11, 2025

@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.

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

I'm not sure I understand the approach you've taken here.

Firstly, I'm not sure why the cached "allowed" and "not allowed" URL lists is required. It seems to undermine the ability to make per-request based decisions on whether a navigation is required, which is the point of having a callback.

Secondly, the general approach of "if async, cancel the request, and force a URL load if the async result succeeds" makes sense, but the way it gets to the actual resolution seems... convoluted.

wrapped_handler takes a function or co-routine, and guarantees that it will be invoked on the event loop. It already has a an existing cleanup method that can be used to attach "on completion" logic. There's no need to have a whole standalone "attach completion callback to result" handler - it can be baked into the cleanup method when the handler is installed.

@t-arn
Copy link
Contributor Author

t-arn commented Jun 12, 2025

@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
Copy link
Contributor Author

t-arn commented Jun 12, 2025

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
Copy link
Member

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
Copy link
Contributor Author

t-arn commented Jun 13, 2025

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

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

This looks a lot better. There's obviously still a lot of debug to clean up, and I've flagged one way to clean up the cleanup handler even more - but the bones of this are lot closer to something I can see being merged.

The one big outstanding question is the need for the staticProxy on WebView, and whether there's any way to make this opt-in if you want navigation callbacks.


def on_navigation_starting_callback(self, widget, result):
try:
url = widget._requested_url
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 pretty sure this wouldn't be needed if this callback method was an inner method inside on_navigation_starting. That way, it could; be a closure over the url argument. See how Window.on_close is implemented - that inner cleanup method is a closure over handler.

@t-arn
Copy link
Contributor Author

t-arn commented Jun 16, 2025

@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
Copy link
Member

@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
Copy link
Contributor Author

t-arn commented Oct 7, 2025

@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
Copy link
Member

@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
Copy link
Contributor Author

t-arn commented Nov 5, 2025

@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
Copy link
Member

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
Copy link
Contributor Author

t-arn commented Nov 7, 2025

@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
Copy link
Member

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
Copy link
Member

mhsmith commented Nov 10, 2025

chaquo/chaquopy#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
Copy link
Contributor Author

t-arn commented Nov 10, 2025

@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 and others added 3 commits November 10, 2025 17:50
…yproject.toml

* improve docstring for on_navigation_starting
* updated app.py to include the latest changes from main
Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

A few comments inline

  1. The documentation for WebView will need to detail the requirements of the Android implementation.
  2. At the risk of engaging in scope creep - the reason Android doesn't support on_webview_load handlers is that we didn't have a static proxy for the client; if we're able to overcome this now as an optional thing, we might as well add that feature while we're at it.
  3. The testbed app will also need the static declaration, and tests to give coverage.

Regarding the tests for dummy; we need to add a "faked" implementation of URL navigation blocking. We don't need to actually load a URL - we just need to track which URL has been loaded; and provide the hooks in the backend to not load if the navigation handler rejects the request. The tests then exercise all the relevant cases - at a first pass, that's:

  • no handler
  • a sync handler that returns True
  • a sync handler that returns False
  • an async handler that returns True
  • an async handler that returns False

There may be some others to cover edge cases of error handling.

return False


# TogaWebClient
Copy link
Member

Choose a reason for hiding this comment

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

What is this line for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's part of my personal coding style (it marks the end of the class code)

I removed the line

from .webview_static_proxy import TogaWebClient

client = TogaWebClient(self)
except BaseException:
Copy link
Member

Choose a reason for hiding this comment

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

This seems like an oddly low-level exception to be raising - is there nothing more specific we can catch?

Copy link
Member

Choose a reason for hiding this comment

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

FYI: Java exceptions can be imported with a from ... import statement and then used in an except statement just like a Python exception.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I replaced it with an explicit NoClassDefFoundError

Comment on lines 37 to 40
msg = "chaquopy.defaultConfig.staticProxy"
msg += '("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"
Copy link
Member

Choose a reason for hiding this comment

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

A multiline-string would be a better option here; see how GTK's webview handles similar long strings.

However, it would also be preferable to only output this warning if the user actually has an on_navigation_starting handler. Outputting the message when the handler is set would make more sense to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, fixed.

if cleanup:
try:
cleanup(interface, result)
cleanup(interface, result, **kwargs)
Copy link
Member

Choose a reason for hiding this comment

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

It occurs to me that we should probably be including *args as well here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, added *args


# If URL is allowed by user interaction or user on_navigation_starting
# handler, the count will be set to 0
self._url_count = 0
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 confused why this is a "count". All the uses are a simple boolean, not "counting" anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, changed to boolean

return await loaded_future

@property
def on_navigation_starting(self):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def on_navigation_starting(self):
def on_navigation_starting(self) -> OnNavigationStartingHandler:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, fixed

Comment on lines 144 to 145
:returns: The function ``callable`` that is called by this navigation event.
Copy link
Member

Choose a reason for hiding this comment

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

We don't need to document the return value; that's part of the type declaration for properties.

Suggested change
:returns: The function ``callable`` that is called by this navigation event.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, fixed

Comment on lines 156 to 158
msg = f"on_navigation_starting.cleanup, url={url}, "
msg += f"result={str(result)}"
print(msg)
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 assuming this is stray debug.

Suggested change
msg = f"on_navigation_starting.cleanup, url={url}, "
msg += f"result={str(result)}"
print(msg)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, and there are a lot more debugging prints. I'll remove them when everything is working as expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now removed all debugging prints

@t-arn
Copy link
Contributor Author

t-arn commented Nov 11, 2025

@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
Copy link
Member

mhsmith commented Nov 11, 2025

Like this:

from java.lang import NoClassDefFoundError

try:
    ...
except NoClassDefFoundError:
    ...

@t-arn
Copy link
Contributor Author

t-arn commented Nov 11, 2025

@mhsmith Thank you, I'll try this

* fixed too long lines
* set asynchronous handler in example app
* adjusted WebView documentation for Android's staticProxy
* added Android staticProxy for testbed application
* removed debugging prints
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants