[url_launcher] Error handling when URL cannot be parsed with Uri.parse #4365
Conversation
|
Thanks for the submission!
Please file an issue describing this problem in detail, with a specific example. The PR as submitted is not correct, since it will always catch the deliberately thrown exception, so we need to better understand what the problem is to figure out how to move forward. |
|
There is an issue with Uri.parse not correctly parsing URLs containing ":" For example, Microsoft Remote Desktop uses a custom app scheme URL like the one below, but Uri.parse throws an FormatException error. Dart recognizes everything after the ":" as a port, but it seems to be giving an error because it's a string. Should I file an issue to dart-lang/sdk instead of submitting this PR? |
It turns out that's not actually a valid URI per RFC 3986, which is what |
| message: 'To use webview or safariVC, you need to pass' | ||
| 'in a web URL. This $urlString is not a web URL.'); | ||
| } | ||
| } catch (e) {} |
There was a problem hiding this comment.
This should only be wrapping parse, not everything (especially not the throw) and should only catch the specific exception it is intended to handle.
When you write the test, please properly encode the space (see https://docs.microsoft.com/en-us/windows-server/remote/remote-desktop-services/clients/remote-desktop-uri for example); as written this is invalid for reasons unrelated to the actual problem with rdp: URLs. |
| try { | ||
| final Uri url = Uri.parse(urlString.trimLeft()); | ||
| isWebURL = url.scheme == 'http' || url.scheme == 'https'; | ||
| } on FormatException catch (_) {} |
There was a problem hiding this comment.
Actually, you can just use Uri.tryParse instead of this, and simplify the code. I forgot to see if there was a try version of this API before my last review comments.
final Uri? url = Uri.tryParse(urlString.trimLeft());
final bool isWebURL = url != null && (url.scheme == 'http' || url.scheme == 'https');
| @@ -281,6 +281,42 @@ void main() { | |||
| await launchResult; | |||
| expect(binding.renderView.automaticSystemUiAdjustment, true); | |||
| }); | |||
|
|
|||
| test('open remote desktop url', () async { | |||
There was a problem hiding this comment.
Let's call this 'open non-parseable URL' so it's clear what the real purpose of this test is.
| isTrue); | ||
| }); | ||
|
|
||
| test('cannot open remote desktop url with forceSafariVC: true', () async { |
There was a problem hiding this comment.
Same here and below; s/remove desktop/non-parseable/
stuartmorgan-g
added
the
waiting for tree to go green
* master: (51 commits) [webview_flutter] Update version number app_facing package (flutter#4375) [webview_flutter] Adjust integration test domains (flutter#4383) Remove some trivial custom analysis options files (flutter#4379) [google_maps_flutter_platfomr_interface] Add Marker drag events (flutter#2653) [flutter_plugin_tools] Improve version check error handling (flutter#4376) [flutter_plugin_tools] Allow overriding breaking change check (flutter#4369) [url_launcher] Error handling when URL cannot be parsed with Uri.parse (flutter#4365) [webview_flutter] Migrate main package to fully federated architecture. (flutter#4366) [google_sign_in] Bump minimum Flutter version and iOS deployment target (flutter#4334) Add false secret lists, and enforce ordering (flutter#4372) [camera_web] Update usage documentation (flutter#4371) [video_player] VTT Support (flutter#2878) Require authors file (flutter#4367) [flutter_plugin_tools] Fix federated safety check (flutter#4368) [webview_flutter] Extract WKWebView implementation into a separate package (flutter#4345) [webview_flutter] Extract Android implementation into a separate package (flutter#4343) [in_app_purchase] Ensure the `introductoryPriceMicros` field is populated correctly. (flutter#4364) [flutter_plugin_tools] Add a federated PR safety check (flutter#4329) [camera] Add web support (flutter#4240) [webview_flutter] Bump minimum Flutter version and iOS deployment target (flutter#4361) ... # Conflicts: # packages/webview_flutter/webview_flutter/lib/platform_interface.dart # packages/webview_flutter/webview_flutter/lib/src/webview_method_channel.dart # packages/webview_flutter/webview_flutter/lib/webview_flutter.dart
Uri.parse cannot parse some app scheme URLs.
Use try-catch to handle errors.
Pre-launch Checklist
dart format.)[shared_preferences]///).If you need help, consider asking for advice on the #hackers-new channel on Discord.