New, more validating, parser for URI.

New, more validating, parser for URI.

BUG= https://code.google.com/p/dart/issues/detail?id=16081
R=ajohnsen@google.com, fschneider@google.com, sgjesse@google.com

Committed: https://code.google.com/p/dart/source/detail?r=37346

[kevmoo, 2014-06-10 21:29:09]

DBC

What about validating calls to the other constructors?

default, file, http, https

Is it possible to create an invalid http URI using the default or http(s) ctor?

Could we add tests for these cases?

https://codereview.chromium.org/321543003/diff/20001/sdk/lib/core/uri.dart
File sdk/lib/core/uri.dart (right):

https://codereview.chromium.org/321543003/diff/20001/sdk/lib/core/uri.dart#ne...
sdk/lib/core/uri.dart:121: static Uri parse(String uri) {
Need to update doc comment that this method will now throw with invalid values
for `uri`.

Perhaps we should reference the RFC?

https://codereview.chromium.org/321543003/diff/20001/sdk/lib/core/uri.dart#ne...
sdk/lib/core/uri.dart:234: _fail(uri, start, "Unmatched [ in host name");
quote '['

https://codereview.chromium.org/321543003/diff/20001/sdk/lib/core/uri.dart#ne...
sdk/lib/core/uri.dart:302: _fail(uri, index, "Expected @");
quote '@'

https://codereview.chromium.org/321543003/diff/20001/sdk/lib/core/uri.dart#ne...
sdk/lib/core/uri.dart:346: _fail(uri, index, "Expected /");
quote '/'

https://codereview.chromium.org/321543003/diff/20001/sdk/lib/core/uri.dart#ne...
sdk/lib/core/uri.dart:390: port: port,
nit: might as well keep these params in the original (canonical) order, right?

https://codereview.chromium.org/321543003/diff/20001/sdk/lib/core/uri.dart#ne...
sdk/lib/core/uri.dart:398: static void _fail(uri, index, message) {
Add types to parameters

https://codereview.chromium.org/321543003/diff/20001/sdk/lib/core/uri.dart#ne...
sdk/lib/core/uri.dart:400: message += ": Unexpected end of input.";
Could this be changed so each message reads like a sentence?

"Unexpected character" + "at end of input." etc

https://codereview.chromium.org/321543003/diff/20001/sdk/lib/core/uri.dart#ne...
sdk/lib/core/uri.dart:402: message += ": Unexpected character at position
$index.\n";
ditto for reading like a sentence from above.

https://codereview.chromium.org/321543003/diff/20001/sdk/lib/core/uri.dart#ne...
sdk/lib/core/uri.dart:406: String post = "";
Code comments for what you're doing here? Is this to limit the length of the
error message?

https://codereview.chromium.org/321543003/diff/20001/tests/corelib/uri_test.dart
File tests/corelib/uri_test.dart (right):

https://codereview.chromium.org/321543003/diff/20001/tests/corelib/uri_test.d...
tests/corelib/uri_test.dart:158: // test that all valid characters are accepted.
split valid test into its own test method?

[kevmoo, 2014-06-10 21:32:04]

https://codereview.chromium.org/321543003/diff/20001/tests/corelib/uri_test.dart
File tests/corelib/uri_test.dart (right):

https://codereview.chromium.org/321543003/diff/20001/tests/corelib/uri_test.d...
tests/corelib/uri_test.dart:240: }
Include examples provided in the bug -
https://code.google.com/p/dart/issues/detail?id=16081

Uri.parse("http://www.example.org/red%09ros\u00E9#red");
Uri.parse("http://r\u00E9sum\u00E9.example.org");

Just for sanity and to give a nod to the bug reporter

[Søren Gjesse, 2014-06-11 08:33:15]

LGTM!

https://codereview.chromium.org/321543003/diff/20001/sdk/lib/core/uri.dart
File sdk/lib/core/uri.dart (right):

https://codereview.chromium.org/321543003/diff/20001/sdk/lib/core/uri.dart#ne...
sdk/lib/core/uri.dart:187: /// Index after current char.
Why dart-doc comments here?

https://codereview.chromium.org/321543003/diff/20001/sdk/lib/core/uri.dart#ne...
sdk/lib/core/uri.dart:230: host = uri.substring(start, index);
Are we doing proper IPv6 validation later, or is that not part of the spec?

https://codereview.chromium.org/321543003/diff/20001/sdk/lib/core/uri.dart#ne...
sdk/lib/core/uri.dart:254: portVal = portVal * 10 + (current - _ZERO);
Any limitations on the port value in the spec?

https://codereview.chromium.org/321543003/diff/20001/sdk/lib/core/uri.dart#ne...
sdk/lib/core/uri.dart:376: // Fragment can contain same characters as query.
No number sign in fragment?

[Anders Johnsen, 2014-06-11 10:00:03]

LGTM, but can you run some numbers before landing? I'd like to know if this has
any impact.

https://codereview.chromium.org/321543003/diff/20001/sdk/lib/core/uri.dart
File sdk/lib/core/uri.dart (right):

https://codereview.chromium.org/321543003/diff/20001/sdk/lib/core/uri.dart#ne...
sdk/lib/core/uri.dart:192: int start = 0;
Start is a bit ambiguous. currentSectionStart ?

https://codereview.chromium.org/321543003/diff/20001/sdk/lib/core/uri.dart#ne...
sdk/lib/core/uri.dart:321: if (_isAlpha(current)) {
Do we need 'scheme != null' to prevent double-parsing of scheme?

https://codereview.chromium.org/321543003/diff/20001/sdk/lib/core/uri.dart#ne...
sdk/lib/core/uri.dart:354: while (_isPathChar(current) || current == _PERCENT) {
Will _isPathChar return true for '/' ?

https://codereview.chromium.org/321543003/diff/20001/sdk/lib/core/uri.dart#ne...
sdk/lib/core/uri.dart:388: return new Uri(scheme: scheme,
This constructor does some extra validation. Do we still need that, or should we
add a helper constructor that just assigns the components?

https://codereview.chromium.org/321543003/diff/20001/sdk/lib/core/uri.dart#ne...
sdk/lib/core/uri.dart:1653: static const int _EOI = 0x10000;  // Not a code
unit.
-1?

[Lasse Reichstein Nielsen, 2014-06-12 08:07:32]

Kevin is right. We should detect the errors in the constructors as well - and
since we call a constructor, it might as well be just in the constructors.

I'll drop the parser, and add validation to the constructors instead.

Then we can start looking into whether the parser can be even simpler (it's
basically a tokenizer, not a parser). As long as it doesn't tokenize
incorrectly, it doesn't matter if it allows incorrect characters in each token,
if they are detected later.

https://codereview.chromium.org/321543003/diff/20001/sdk/lib/core/uri.dart
File sdk/lib/core/uri.dart (right):

https://codereview.chromium.org/321543003/diff/20001/sdk/lib/core/uri.dart#ne...
sdk/lib/core/uri.dart:121: static Uri parse(String uri) {
It threw before as well, now it just detects more invalid cases.
So, we always needed to write that.

https://codereview.chromium.org/321543003/diff/20001/sdk/lib/core/uri.dart#ne...
sdk/lib/core/uri.dart:187: /// Index after current char.
Because I assume the editor will display that comment if you hover over the
variable below.

https://codereview.chromium.org/321543003/diff/20001/sdk/lib/core/uri.dart#ne...
sdk/lib/core/uri.dart:192: int start = 0;
.... loooooong name.
But ok.

https://codereview.chromium.org/321543003/diff/20001/sdk/lib/core/uri.dart#ne...
sdk/lib/core/uri.dart:230: host = uri.substring(start, index);
I believe we are doing it later.

I don't know if we allow [v47:arglebargle] syntax.

https://codereview.chromium.org/321543003/diff/20001/sdk/lib/core/uri.dart#ne...
sdk/lib/core/uri.dart:321: if (_isAlpha(current)) {
No, there is no parsing before this point. The code above is a helper function
which is called below.

https://codereview.chromium.org/321543003/diff/20001/sdk/lib/core/uri.dart#ne...
sdk/lib/core/uri.dart:354: while (_isPathChar(current) || current == _PERCENT) {
No.

https://codereview.chromium.org/321543003/diff/20001/sdk/lib/core/uri.dart#ne...
sdk/lib/core/uri.dart:376: // Fragment can contain same characters as query.
No. At most one # in any URL.

https://codereview.chromium.org/321543003/diff/20001/sdk/lib/core/uri.dart#ne...
sdk/lib/core/uri.dart:388: return new Uri(scheme: scheme,
As pointed out elsewhere, the validation needs to be in the constructor anyway,
so I'll drop this parser change, and move the validation to the normalize
functions used in the constructors instead.

https://codereview.chromium.org/321543003/diff/20001/sdk/lib/core/uri.dart#ne...
sdk/lib/core/uri.dart:390: port: port,
ACK.

https://codereview.chromium.org/321543003/diff/20001/sdk/lib/core/uri.dart#ne...
sdk/lib/core/uri.dart:406: String post = "";
Yes, at most 78 characters of the source will be displayed.

I should move this to FormatException. It's probably useful in other cases too
(I'll have to detect newlines then, and report the line and index).

https://codereview.chromium.org/321543003/diff/20001/sdk/lib/core/uri.dart#ne...
sdk/lib/core/uri.dart:1653: static const int _EOI = 0x10000;  // Not a code
unit.
Breaks the _isPathChar function (or requires an extra test) because it currently
starts with (ch < 127) && ...

[Lasse Reichstein Nielsen, 2014-06-12 08:07:34]

Kevin is right. We should detect the errors in the constructors as well - and
since we call a constructor, it might as well be just in the constructors.

I'll drop the parser, and add validation to the constructors instead.

Then we can start looking into whether the parser can be even simpler (it's
basically a tokenizer, not a parser). As long as it doesn't tokenize
incorrectly, it doesn't matter if it allows incorrect characters in each token,
if they are detected later.

[Lasse Reichstein Nielsen, 2014-06-13 07:18:04]

Rewritten, PTAL

[Søren Gjesse, 2014-06-13 10:13:17]

lgtm

https://codereview.chromium.org/321543003/diff/40001/sdk/lib/core/uri.dart
File sdk/lib/core/uri.dart (right):

https://codereview.chromium.org/321543003/diff/40001/sdk/lib/core/uri.dart#ne...
sdk/lib/core/uri.dart:300: _fail(uri, pathEndIndex - 1, "Colon in initial path
segment");
This is when at least one of the characters before the colon is not a valid
scheme character, right?

https://codereview.chromium.org/321543003/diff/40001/sdk/lib/core/uri.dart#ne...
sdk/lib/core/uri.dart:372: static void _fail(String uri, int index, String
message) {
Thank you for the nice error messages!

https://codereview.chromium.org/321543003/diff/40001/sdk/lib/core/uri.dart#ne...
sdk/lib/core/uri.dart:912: * Schemes are converted to lower case. They cannot
contain
Missing end of sentence.

[Lasse Reichstein Nielsen, 2014-06-13 11:53:04]

Florian, Anders, PTAL

https://codereview.chromium.org/321543003/diff/40001/sdk/lib/core/uri.dart
File sdk/lib/core/uri.dart (right):

https://codereview.chromium.org/321543003/diff/40001/sdk/lib/core/uri.dart#ne...
sdk/lib/core/uri.dart:300: _fail(uri, pathEndIndex - 1, "Colon in initial path
segment");
Yes. If there is no scheme and the path isn't absolute, the first path segment
is generated by path-nc, so there is no valid production with a colon here.

It's non-trivial to say what the error is. The stuff up to and including the :
is neither a scheme nor a path-nc.

https://codereview.chromium.org/321543003/diff/40001/sdk/lib/core/uri.dart#ne...
sdk/lib/core/uri.dart:372: static void _fail(String uri, int index, String
message) {
I'm considering if we can put something like this into FormatException.
Something like:

  FormatException.withPosition(message, source, position) 
     = FormatException(generateMessage(...))
where generateMessage makes something like;
  $message at position $position:\n
  ... slice of source ...
          ^

(or even better: make source and position final fields too, and generate the
message on demand),

It should also be able to handle newlines, so it can say:
  $message at line $line position ${position-lineStart};
  ... slice of line ...
          ^

I guess I'll do that at some point. Shouldn't be that bad, and we can user it in
other places too.

https://codereview.chromium.org/321543003/diff/40001/sdk/lib/core/uri.dart#ne...
sdk/lib/core/uri.dart:912: * Schemes are converted to lower case. They cannot
contain
Done.

https://codereview.chromium.org/321543003/diff/80001/runtime/bin/builtin.dart
File runtime/bin/builtin.dart (right):

https://codereview.chromium.org/321543003/diff/80001/runtime/bin/builtin.dart...
runtime/bin/builtin.dart:217: cwd = _enforceTrailingSlash(cwd);
Are these sanitizations still necessary when using Uri.file?

[Lasse Reichstein Nielsen, 2014-06-13 11:53:05]

Florian, Anders, PTAL

[Florian Schneider, 2014-06-13 12:05:24]

runtime/ lgtm.

[Anders Johnsen, 2014-06-13 12:40:28]

lgtm

[Anders Johnsen, 2014-06-13 12:41:38]

https://codereview.chromium.org/321543003/diff/80001/runtime/bin/builtin.dart
File runtime/bin/builtin.dart (right):

https://codereview.chromium.org/321543003/diff/80001/runtime/bin/builtin.dart...
runtime/bin/builtin.dart:217: cwd = _enforceTrailingSlash(cwd);
On 2014/06/13 11:53:04, Lasse Reichstein Nielsen wrote:
> Are these sanitizations still necessary when using Uri.file?

Please try without.

[Søren Gjesse, 2014-06-13 15:25:04]

https://codereview.chromium.org/321543003/diff/80001/runtime/bin/builtin.dart
File runtime/bin/builtin.dart (right):

https://codereview.chromium.org/321543003/diff/80001/runtime/bin/builtin.dart...
runtime/bin/builtin.dart:217: cwd = _enforceTrailingSlash(cwd);
On 2014/06/13 11:53:04, Lasse Reichstein Nielsen wrote:
> Are these sanitizations still necessary when using Uri.file?

I think it is - if this is used with .resolve then without the trailing slash we
don't get the expected behaviour.

[Lasse Reichstein Nielsen, 2014-06-16 07:00:46]

Committed patchset #7 manually as r37346 (presubmit successful).

[Lasse Reichstein Nielsen, 2014-06-19 08:57:43]

This CL was too strict.
Reverted and changing strategy to a more lenient parser that escapes invalid
characters where possible.