Correctly url-decode the path segment in the http_server.

Correctly url-decode the path segment in the http_server.

BUG=
R=lrn@google.com

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

[Lasse Reichstein Nielsen, 2013-07-01 11:51:51]

https://codereview.chromium.org/18333003/diff/1/pkg/http_server/lib/src/virtu...
File pkg/http_server/lib/src/virtual_directory.dart (right):

https://codereview.chromium.org/18333003/diff/1/pkg/http_server/lib/src/virtu...
pkg/http_server/lib/src/virtual_directory.dart:78: var path = new
Path(Uri.decodeComponent(request.uri.path)).canonicalize();
I think this is wrong.
From the URI RFC: "A path consists of a sequence of path segments separated by a
slash ("/") character."
Here you are decoding "%2f" into "/" *before* deciding on where to split the
path segments. I think you do need to find the path-segments first, and then
decode each.

https://codereview.chromium.org/18333003/diff/1/pkg/http_server/test/virtual_...
File pkg/http_server/test/virtual_directory_test.dart (right):

https://codereview.chromium.org/18333003/diff/1/pkg/http_server/test/virtual_...
pkg/http_server/test/virtual_directory_test.dart:538: return
getStatusCode(server.port, '/my%20file')
Test with strings containing:
 %2e%2e ("..")
 %2f  ("/", probably won't work, since the underlying file system cannot ignore
slashes - can your VirtualDirectory handle file names with slashes?)
 %252e%252e  (double encoded "..")

You may want to consider having a helper function for these tests instead of
duplicating code. Too much boilerplate discourages making many tests, and we
want many tests!

[Anders Johnsen, 2013-07-01 14:14:12]

PTAL

https://codereview.chromium.org/18333003/diff/1/pkg/http_server/lib/src/virtu...
File pkg/http_server/lib/src/virtual_directory.dart (right):

https://codereview.chromium.org/18333003/diff/1/pkg/http_server/lib/src/virtu...
pkg/http_server/lib/src/virtual_directory.dart:78: var path = new
Path(Uri.decodeComponent(request.uri.path)).canonicalize();
On 2013/07/01 11:51:51, Lasse Reichstein Nielsen wrote:
> I think this is wrong.
> From the URI RFC: "A path consists of a sequence of path segments separated by
a
> slash ("/") character."
> Here you are decoding "%2f" into "/" *before* deciding on where to split the
> path segments. I think you do need to find the path-segments first, and then
> decode each.

Done.

https://codereview.chromium.org/18333003/diff/1/pkg/http_server/test/virtual_...
File pkg/http_server/test/virtual_directory_test.dart (right):

https://codereview.chromium.org/18333003/diff/1/pkg/http_server/test/virtual_...
pkg/http_server/test/virtual_directory_test.dart:538: return
getStatusCode(server.port, '/my%20file')
On 2013/07/01 11:51:51, Lasse Reichstein Nielsen wrote:
> Test with strings containing:
>  %2e%2e ("..")
>  %2f  ("/", probably won't work, since the underlying file system cannot
ignore
> slashes - can your VirtualDirectory handle file names with slashes?)
>  %252e%252e  (double encoded "..")
> 
> You may want to consider having a helper function for these tests instead of
> duplicating code. Too much boilerplate discourages making many tests, and we
> want many tests!

Done.

[Lasse Reichstein Nielsen, 2013-07-02 07:31:14]

lgtm.

https://codereview.chromium.org/18333003/diff/9001/pkg/http_server/lib/src/vi...
File pkg/http_server/lib/src/virtual_directory.dart (right):

https://codereview.chromium.org/18333003/diff/9001/pkg/http_server/lib/src/vi...
pkg/http_server/lib/src/virtual_directory.dart:104: path = path.canonicalize();
So you canonicalize after each segment is added.
Will a path not containing "." or ".." canonicalize to itself?

https://codereview.chromium.org/18333003/diff/9001/pkg/http_server/lib/src/vi...
pkg/http_server/lib/src/virtual_directory.dart:121: segments.skip(1));
Ick. That's no way to treat an iterable.
Consider doing a new Queue.from(segments) so you have an efficient removeFirst.
Or pass an Iterator intead of an Iterable. 
You are doing layered skips which can end up as (if we don't recognize and
optimizie it) a linear space and quadratic time operation.

https://codereview.chromium.org/18333003/diff/9001/pkg/http_server/test/virtu...
File pkg/http_server/test/virtual_directory_test.dart (right):

https://codereview.chromium.org/18333003/diff/9001/pkg/http_server/test/virtu...
pkg/http_server/test/virtual_directory_test.dart:571: testEncoding('%2f',
HttpStatus.OK, true);
So you are successfully creating a file called "%2f".

Try to make a tests where you send the *encoded* URI containing "/a%2fb/c",
which should have the path segments (after decoding) "a/b" and "c". That is, a
way to test malicious URIs where the sender controls the entire URI and it's not
necessarily well escaped.

[Anders Johnsen, 2013-07-02 11:08:07]

https://codereview.chromium.org/18333003/diff/9001/pkg/http_server/lib/src/vi...
File pkg/http_server/lib/src/virtual_directory.dart (right):

https://codereview.chromium.org/18333003/diff/9001/pkg/http_server/lib/src/vi...
pkg/http_server/lib/src/virtual_directory.dart:104: path = path.canonicalize();
On 2013/07/02 07:31:14, Lasse Reichstein Nielsen wrote:
> So you canonicalize after each segment is added.
> Will a path not containing "." or ".." canonicalize to itself?

Yes, that's the idea.

https://codereview.chromium.org/18333003/diff/9001/pkg/http_server/lib/src/vi...
pkg/http_server/lib/src/virtual_directory.dart:121: segments.skip(1));
On 2013/07/02 07:31:14, Lasse Reichstein Nielsen wrote:
> Ick. That's no way to treat an iterable.
> Consider doing a new Queue.from(segments) so you have an efficient
removeFirst.
> Or pass an Iterator intead of an Iterable. 
> You are doing layered skips which can end up as (if we don't recognize and
> optimizie it) a linear space and quadratic time operation.

Done.

https://codereview.chromium.org/18333003/diff/9001/pkg/http_server/test/virtu...
File pkg/http_server/test/virtual_directory_test.dart (right):

https://codereview.chromium.org/18333003/diff/9001/pkg/http_server/test/virtu...
pkg/http_server/test/virtual_directory_test.dart:571: testEncoding('%2f',
HttpStatus.OK, true);
On 2013/07/02 07:31:14, Lasse Reichstein Nielsen wrote:
> So you are successfully creating a file called "%2f".
> 
> Try to make a tests where you send the *encoded* URI containing "/a%2fb/c",
> which should have the path segments (after decoding) "a/b" and "c". That is, a
> way to test malicious URIs where the sender controls the entire URI and it's
not
> necessarily well escaped.
> 
> 

Done.

[Anders Johnsen, 2013-07-02 11:08:24]

Committed patchset #4 manually as r24677 (presubmit successful).