From FilePath::AppendAndResolveRelative() to file_util::AbsolutePath().

The existing file_util::AbsolutePath() function was already doing what we needed to do in the ExtensionResource class.

BUG= http://crbug.com/25681 & http://crbug.com/25131


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=30149

[MAD, 2009-10-26 20:21:24]

This also fixes http://code.google.com/p/chromium/issues/detail?id=25131

Thanks!

BYE
MAD

[Mark Mentovai, 2009-10-26 20:46:42]

http://codereview.chromium.org/334028/diff/1/3
File base/file_path.h (right):

http://codereview.chromium.org/334028/diff/1/3#newcode181
Line 181: // Returns a copy of this file path without any '.' nor '..' by
resolving the
OK, again, this isn't something that can be done without asking the filesystem
for help, and we can't ask the filesystem for help in FilePath because blocking
operations are forbidden.

[MAD, 2009-10-26 20:53:20]

http://codereview.chromium.org/334028/diff/1/3
File base/file_path.h (right):

http://codereview.chromium.org/334028/diff/1/3#newcode181
Line 181: // Returns a copy of this file path without any '.' nor '..' by
resolving the
On 2009/10/26 20:46:42, Mark Mentovai wrote:
> OK, again, this isn't something that can be done without asking the filesystem
> for help, and we can't ask the filesystem for help in FilePath because
blocking
> operations are forbidden.

I don't understand. Where do we ask the File System for help here?

[Mark Mentovai, 2009-10-26 21:09:06]

I'm saying that we can't resolve .. without asking the filesystem.

Symbolic links.

mad@chromium.org wrote:
> http://codereview.chromium.org/334028/diff/1/3
> File base/file_path.h (right):
>
> http://codereview.chromium.org/334028/diff/1/3#newcode181
> Line 181: // Returns a copy of this file path without any '.' nor '..'
> by resolving the
> On 2009/10/26 20:46:42, Mark Mentovai wrote:
>>
>> OK, again, this isn't something that can be done without asking the
>
> filesystem
>>
>> for help, and we can't ask the filesystem for help in FilePath because
>
> blocking
>>
>> operations are forbidden.
>
> I don't understand. Where do we ask the File System for help here?
>
> http://codereview.chromium.org/334028

[MAD, 2009-10-26 23:40:52]

OK, how about this then?

Thanks!

BYE
MAD

[mad-corp, 2009-10-26 23:50:18]

Wait, I went too quickly...

Still working on it...

A simpler solution is on the way...

BYE
MAD

On Mon, Oct 26, 2009 at 7:40 PM, <mad@chromium.org> wrote:

> OK, how about this then?
>
> Thanks!
>
> BYE
> MAD
>
>
>
> http://codereview.chromium.org/334028
>

[Mark Mentovai, 2009-10-27 00:01:15]

If you really need to resolve . and .. and have it do the right thing with
symbolic links, you'll probably want to use realpath (POSIX) and _wfullpath
(Windows).  At least on POSIX, it consults the filesystem and properly resolves
symbolic links.  realpath is not a well thought-out API because it uses a
PATH_MAX-sized buffer, but PATH_MAX isn't actually the maximum pathname length,
but it's the best that we can do for now.

[mad-corp, 2009-10-27 00:25:47]

I know, I saw that this is what is used in file_util::AbsolutePath(), but
unfortunately, I don't see the same behavior on Windows and Linux... On
Windows, the actual path doesn't need to exist for the function to work, and
it won't resolve above a \\server name (e.g., \\server\..\a\.. will come
back as \\server\..)... Whereas the POSIX version (that I only tried on
linux) will never return a '.' or a '..' in the path, and will fail with the
errno identifying "No such file or directory"...

So I was tempted to leave this as is and just name it properly to identify
that is only walks the path to remove the '.' and '..' without resolving
symbolic links... But it kind of sucks, right?

Otherwise, we might as well accept the differences on Windows and POSIX and
let the ExtensionResource use file_util::AbsolutePath() and it will also
take care of validating the existence of the path (which is what is needed
anyway) but still validate the file existence afterward since on Windows it
is not validated by file_util::AbsolutePath()...

And live with the fact that we may endup with a \\server\.. after
calling file_util::AbsolutePath(), which should be OK, since we
call file_util::PathExists() afterward anyway... Right?

Any other opinion on this?

So, the simpler solution I was talking about is to simply remove the new
ResolveRelative method and use file_util::AbsolutePath() instead.... OK?

Thanks!

BYE
MAD

On Mon, Oct 26, 2009 at 8:01 PM, <mark@chromium.org> wrote:

> If you really need to resolve . and .. and have it do the right thing with
> symbolic links, you'll probably want to use realpath (POSIX) and _wfullpath
> (Windows).  At least on POSIX, it consults the filesystem and properly
> resolves
> symbolic links.  realpath is not a well thought-out API because it uses a
> PATH_MAX-sized buffer, but PATH_MAX isn't actually the maximum pathname
> length,
> but it's the best that we can do for now.
>
>
> http://codereview.chromium.org/334028
>

[MAD, 2009-10-27 00:42:57]

So here is the simple solution...

I didn't finish a full rebuild and running the unit tests yet, but since I
mainly removed code, I'm confident enough to send it to you guys (and the try
servers :-)...

I'll update again if something goes astray...

Thanks again for you patience on this one...

BYE
MAD

[Mark Mentovai, 2009-10-27 01:26:06]

LGTM, although it didn't have a clean try run - looks like that wasn't your
fault.

Update the description, which no longer applies to what you're actually doing
here.

[mad-corp, 2009-10-27 01:37:06]

Thanks, I had updated the description, but noticed that the title didn't
update...
You were talking about the title, right?

BYE
MAD

On Mon, Oct 26, 2009 at 9:26 PM, <mark@chromium.org> wrote:

> LGTM, although it didn't have a clean try run - looks like that wasn't your
> fault.
>
> Update the description, which no longer applies to what you're actually
> doing
> here.
>
>
> http://codereview.chromium.org/334028
>

[Aaron Boodman, 2009-10-27 19:05:44]

Ok, I think I have sorted this out with Marc offline.

I agree with Mark that we should not be trying to do path parsing or
comparison ourselves, but instead use the filesystem APIs. I would
like to remove all the file URL shenanigans completely, and instead
just naively stuff the path component of whatever URL comes into the
extension system into an ExtensionResource, and then leave it up to
the filesystem to sort out. At the end we just compare the absolute
paths and make sure the path being requested is a child of the
extension root.

This change looks like it implements this idea:
http://codereview.chromium.org/334028/diff/22/26

The only hiccup is the issue with missing resources. The desire here
is to have an experience similar to web development. If you specify an
invalid resource the page still loads, it just has broken references
to those resources. To get this effect, I've asked Marc to try
returning an empty FilePath in case the platform canonicalization API
fails. I think that this will result in the right effect.

===

Separately, we don't really need all the file URL crap anymore. This
was just trying to do compare the paths safely before we had a place
to do it on the file thread.

- a

[Mark Mentovai, 2009-10-27 19:37:39]

Sounds workable.  Sounds right.  Thanks for taking a look.

Mark

Aaron Boodman wrote:
> Ok, I think I have sorted this out with Marc offline.
>
> I agree with Mark that we should not be trying to do path parsing or
> comparison ourselves, but instead use the filesystem APIs. I would
> like to remove all the file URL shenanigans completely, and instead
> just naively stuff the path component of whatever URL comes into the
> extension system into an ExtensionResource, and then leave it up to
> the filesystem to sort out. At the end we just compare the absolute
> paths and make sure the path being requested is a child of the
> extension root.
>
> This change looks like it implements this idea:
> http://codereview.chromium.org/334028/diff/22/26
>
> The only hiccup is the issue with missing resources. The desire here
> is to have an experience similar to web development. If you specify an
> invalid resource the page still loads, it just has broken references
> to those resources. To get this effect, I've asked Marc to try
> returning an empty FilePath in case the platform canonicalization API
> fails. I think that this will result in the right effect.
>
> ===
>
> Separately, we don't really need all the file URL crap anymore. This
> was just trying to do compare the paths safely before we had a place
> to do it on the file thread.