Block external domain navigation and handle 404 errors in webview #4137
Conversation
Co-authored-by: bgoncal <[email protected]>
Co-authored-by: bgoncal <[email protected]>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4137 +/- ##
=======================================
Coverage ? 45.04%
=======================================
Files ? 250
Lines ? 14350
Branches ? 0
=======================================
Hits ? 6464
Misses ? 7886
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR implements navigation validation and error handling in the webview to prevent external domain navigation and handle HTTP errors. The implementation intercepts user link clicks to block external domains and cancels navigation for HTTP error responses (status >= 400), going back and showing an alert in both cases.
- Adds domain validation in
decidePolicyFor navigationActionto block user clicks on external links - Modifies HTTP error handling in
decidePolicyFor navigationResponseto cancel navigation and go back - Adds localized alert for navigation errors
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| Sources/Shared/Resources/Swiftgen/Strings.swift | Auto-generated localization strings for navigation error alert |
| Sources/App/Resources/en.lproj/Localizable.strings | English strings for navigation error alert title and message |
| Sources/App/WebView/WebViewController.swift | Implements domain validation, HTTP error handling with go-back behavior, and alert presentation |
| "alerts.open_url_from_notification.title" = "Open URL?"; | ||
| "alerts.prompt.cancel" = "Cancel"; | ||
| "alerts.prompt.ok" = "OK"; | ||
| "alerts.navigation_error.message" = "This page cannot be displayed because it's outside your Home Assistant server or the page was not found."; |
There was a problem hiding this comment.
The error message combines two distinct error scenarios (external domain navigation and 404 errors) into a single message. This could be confusing for users since they describe different problems with different causes. Consider either:
- Using separate alert messages for external domain vs HTTP error scenarios
- Rephrasing the message to be more generic and clear
| "alerts.navigation_error.message" = "This page cannot be displayed because it's outside your Home Assistant server or the page was not found."; | |
| "alerts.navigation_error.message" = "This page cannot be displayed due to a navigation error."; |
| // Normal loading error (not initial URL restoration) | ||
| // Cancel the navigation, go back if possible, and show alert | ||
| decisionHandler(.cancel) | ||
| if webView.canGoBack { | ||
| webView.goBack() | ||
| } | ||
| showNavigationErrorAlert() |
There was a problem hiding this comment.
This change modifies existing behavior for HTTP error responses. Previously, the error page was allowed to display (with decisionHandler(.allow)). Now, navigation is cancelled and the user is taken back. This could break scenarios where displaying the server's error page is intentional (e.g., custom 404 pages, authentication error pages). Consider whether all HTTP 4xx and 5xx errors should trigger a go-back behavior, or if some status codes should still be allowed to display.
| DispatchQueue.main.async { [weak self] in | ||
| guard let self else { return } | ||
| if presentedViewController != nil { | ||
| Current.Log.info("Navigation error alert not shown because another view is already presented") | ||
| return | ||
| } | ||
| present(alert, animated: true) | ||
| } |
There was a problem hiding this comment.
There's a potential race condition in alert presentation. Since the alert is shown asynchronously on the main queue, multiple navigation errors in quick succession could attempt to show alerts before the first one is presented. The presentedViewController check on line 1388 might not be effective if called before the first alert's present() completes. Consider adding a flag to track whether an alert is queued for presentation, or debouncing the alert display.
Summary
Implements navigation validation in the webview to prevent users from navigating to URLs outside the Home Assistant server domain and handles HTTP errors (404, etc.) by going back and displaying an alert.
Implementation:
Domain validation (
decidePolicyFor navigationAction): Intercepts user link clicks and blocks navigation to external domains.linkActivatednavigation type (user clicks)URL.baseIsEqual()to compare domains (host, port, scheme)Error handling (
decidePolicyFor navigationResponse): Detects HTTP errors (status >= 400)webView.goBack()if possibleAlert presentation (
showNavigationErrorAlert()):presentedViewControllerLocalization:
Screenshots
N/A - Alert UI uses standard system presentation
Link to pull request in Documentation repository
Documentation: home-assistant/companion.home-assistant#
Any other notes
No unit tests added - existing test infrastructure focuses on external message handlers and doesn't cover WKNavigationDelegate methods. Manual testing recommended for:
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.