Make Uri.parse accept [ and ] in path, query and fragment.

Also accept `#` in fragment.
Fix bug in parsing URI where first character of path needs escaping.
The characters are escaped, but parsing doesn't fail, like it previously
did on misplaced general delimiters in those places

Fixes flutter#34451.

Bug: https://dartbug.com/34451
Change-Id: I99aa39bd2909661802ad9d1bb5dada94d579141b
Reviewed-on: https://dart-review.googlesource.com/74780
Commit-Queue: Lasse R.H. Nielsen <lrn@google.com>
Reviewed-by: Leaf Petersen <leafp@google.com>

CHANGELOG.md

@@ -21,6 +21,8 @@
     succeed.
 *   Exported `Future` and `Stream` from `dart:core`.
 *   Added operators `&`, `|` and `^` to `bool`.
+*   Made `Uri` parsing more permissive about `[` and `]` occurring
+    in the path, query or fragment, and `#` occurring in fragment.
 
 #### `dart:async`
 

pkg/analyzer/test/src/dart/analysis/driver_test.dart

@@ -1770,21 +1770,21 @@ class C {}
 
   test_getResult_invalidUri() async {
     String content = r'''
-import '[invalid uri]';
+import ':[invalid uri]';
 import '[invalid uri]:foo.dart';
 import 'package:aaa/a1.dart';
-import '[invalid uri]';
+import ':[invalid uri]';
 import '[invalid uri]:foo.dart';
 
-export '[invalid uri]';
+export ':[invalid uri]';
 export '[invalid uri]:foo.dart';
 export 'package:aaa/a2.dart';
-export '[invalid uri]';
+export ':[invalid uri]';
 export '[invalid uri]:foo.dart';
 
-part '[invalid uri]';
+part ':[invalid uri]';
 part 'a3.dart';
-part '[invalid uri]';
+part ':[invalid uri]';
 ''';
     addTestFile(content);
 

pkg/analyzer/test/src/dart/analysis/file_state_test.dart

@@ -397,13 +397,13 @@ part of L;
     String a3 = _p('/aaa/lib/a3.dart');
     String content_a1 = r'''
 import 'package:aaa/a1.dart';
-import '[invalid uri]';
+import ':[invalid uri]';
 
 export 'package:aaa/a2.dart';
-export '[invalid uri]';
+export ':[invalid uri]';
 
 part 'a3.dart';
-part '[invalid uri]';
+part ':[invalid uri]';
 ''';
     provider.newFile(a, content_a1);
 

pkg/analyzer/test/src/summary/resynthesize_common.dart

@@ -6028,21 +6028,21 @@ unit: null
     allowMissingFiles = true;
     shouldCompareLibraryElements = false;
     var library = await checkLibrary(r'''
-import '[invalid uri]';
-import '[invalid uri]:foo.dart';
+import ':[invaliduri]';
+import ':[invaliduri]:foo.dart';
 import 'a1.dart';
-import '[invalid uri]';
-import '[invalid uri]:foo.dart';
+import ':[invaliduri]';
+import ':[invaliduri]:foo.dart';
 
-export '[invalid uri]';
-export '[invalid uri]:foo.dart';
+export ':[invaliduri]';
+export ':[invaliduri]:foo.dart';
 export 'a2.dart';
-export '[invalid uri]';
-export '[invalid uri]:foo.dart';
+export ':[invaliduri]';
+export ':[invaliduri]:foo.dart';
 
-part '[invalid uri]';
+part ':[invaliduri]';
 part 'a3.dart';
-part '[invalid uri]';
+part ':[invaliduri]';
 ''');
     checkElementText(library, r'''
 import '<unresolved>';

pkg/analyzer/test/src/summary/summary_common.dart

@@ -5992,7 +5992,7 @@ class B extends A {}
   }
 
   test_export_uri_invalid() {
-    String uriString = '[invalid uri]';
+    String uriString = ':[invalid uri]';
     String libraryText = 'export "$uriString";';
     serializeLibraryText(libraryText);
     expect(unlinkedUnits[0].publicNamespace.exports, hasLength(1));
@@ -7647,7 +7647,7 @@ class D extends p.C {} // Prevent "unused import" warning
   }
 
   test_import_uri_invalid() {
-    String uriString = '[invalid uri]';
+    String uriString = ':[invalid uri]';
     String libraryText = 'import "$uriString";';
     serializeLibraryText(libraryText);
     // Second import is the implicit import of dart:core
@@ -9035,7 +9035,7 @@ part "${'a'}.dart"; // <-part
   }
 
   test_part_uri_invalid() {
-    String uriString = '[invalid uri]';
+    String uriString = ':[invalid uri]';
     String libraryText = 'part "$uriString";';
     serializeLibraryText(libraryText);
     expect(unlinkedUnits[0].publicNamespace.parts, hasLength(1));

sdk/lib/core/uri.dart

@@ -849,11 +849,17 @@ abstract class Uri {
     String scheme;
 
     // Derive some positions that weren't set to normalize the indices.
-    // If pathStart isn't set (it's before scheme end or host start), then
-    // the path is empty.
     if (fragmentStart < queryStart) queryStart = fragmentStart;
-    if (pathStart < hostStart || pathStart <= schemeEnd) {
+    // If pathStart isn't set (it's before scheme end or host start), then
+    // the path is empty, or there is no authority part and the path
+    // starts with a non-simple character.
+    if (pathStart < hostStart) {
+      // There is an authority, but no path. The path would start with `/`
+      // if it was there.
       pathStart = queryStart;
+    } else if (pathStart <= schemeEnd) {
+      // There is a scheme, but no authority.
+      pathStart = schemeEnd + 1;
     }
     // If there is an authority with no port, set the port position
     // to be at the end of the authority (equal to pathStart).
@@ -2110,7 +2116,8 @@ class _Uri implements Uri {
     }
     var result;
     if (path != null) {
-      result = _normalizeOrSubstring(path, start, end, _pathCharOrSlashTable);
+      result = _normalizeOrSubstring(path, start, end, _pathCharOrSlashTable,
+          escapeDelimiters: true);
     } else {
       result = pathSegments
           .map((s) => _uriEncode(_pathCharTable, s, utf8, false))
@@ -2143,7 +2150,8 @@ class _Uri implements Uri {
       if (queryParameters != null) {
         throw new ArgumentError('Both query and queryParameters specified');
       }
-      return _normalizeOrSubstring(query, start, end, _queryCharTable);
+      return _normalizeOrSubstring(query, start, end, _queryCharTable,
+          escapeDelimiters: true);
     }
     if (queryParameters == null) return null;
 
@@ -2175,7 +2183,8 @@ class _Uri implements Uri {
 
   static String _makeFragment(String fragment, int start, int end) {
     if (fragment == null) return null;
-    return _normalizeOrSubstring(fragment, start, end, _queryCharTable);
+    return _normalizeOrSubstring(fragment, start, end, _queryCharTable,
+        escapeDelimiters: true);
   }
 
   /**
@@ -2261,8 +2270,10 @@ class _Uri implements Uri {
    * this methods returns the substring if [component] from [start] to [end].
    */
   static String _normalizeOrSubstring(
-      String component, int start, int end, List<int> charTable) {
-    return _normalize(component, start, end, charTable) ??
+      String component, int start, int end, List<int> charTable,
+      {bool escapeDelimiters = false}) {
+    return _normalize(component, start, end, charTable,
+            escapeDelimiters: escapeDelimiters) ??
         component.substring(start, end);
   }
 

tests/co19_2/co19_2-runtime.status

@@ -2,6 +2,10 @@
 # for details. All rights reserved. Use of this source code is governed by a
 # BSD-style license that can be found in the LICENSE file.
 
+[ $runtime != none ]
+LibTest/core/Uri/hasEmptyPath_A01_t01: RuntimeError
+LibTest/core/Uri/parse_A05_t01: RuntimeError
+
 [ $compiler != dart2js && $runtime != none && !$checked ]
 LibTest/async/Future/catchError_A03_t05: RuntimeError
 

tests/corelib_2/uri_test.dart

@@ -280,19 +280,10 @@ void testInvalidUrls() {
   checkInvalid("s://x@x:x/");
   // At most one port.
   checkInvalid("s://x@x:9:9/");
-  // At most one #.
-  checkInvalid("s://x/x#foo#bar");
   // @ not allowed in scheme.
   checkInvalid("s@://x:9/x?x#x");
   // ] not allowed alone in host.
   checkInvalid("s://xx]/");
-  // ] not allowed anywhere except in host.
-  checkInvalid("s://xx/]");
-  checkInvalid("s://xx/?]");
-  checkInvalid("s://xx/#]");
-  checkInvalid("s:/]");
-  checkInvalid("s:/?]");
-  checkInvalid("s:/#]");
   // IPv6 must be enclosed in [ and ] for Uri.parse.
   // It is allowed un-enclosed as argument to `Uri(host:...)` because we don't
   // need to delimit.
@@ -387,6 +378,18 @@ void testNormalization() {
       "scheme:///#",
       new Uri(scheme: "scheme", host: "", path: "/", query: "", fragment: "")
           .toString());
+
+  // We allow, and escape, general delimiters in paths, queries and fragments.
+  // Allow `[` and `]` in path:
+  Expect.equals("s:/%5B%5D", Uri.parse("s:/[]").toString());
+  Expect.equals("s:%5B%5D", Uri.parse("s:[]").toString());
+  Expect.equals("%5B%5D", Uri.parse("[]").toString());
+  // Allow `[`, `]` and `?` in query (anything after *first* `?`).
+  // The `?` is not escaped.
+  Expect.equals("s://xx/?%5B%5D?", Uri.parse("s://xx/?[]?").toString());
+  // Allow `[`, `]`, `?` and `#` in fragment (anything after *first* `#`).
+  // The `?` is not escaped.
+  Expect.equals("s://xx/#%5B%5D%23?", Uri.parse("s://xx/#[]#?").toString());
 }
 
 void testReplace() {