Bug 38711 – DragData::asURL() shouldn't do file validity checks

• Component: Platform
• Status: RESOLVED FIXED

[Daniel Cheng]

The renderer is sandboxed, and the calls to get file metadata will (should) fail. The loader is smart enough to do the right thing. The general consensus seemed to be that letting the renderer poke at the filesystem like this is undesirable.

This is only a problem on Windows today because the Linux/Mac sandboxes allow metadata access today in order to follow symlinks. However, it's probably good to avoid depending on this function if at all possible.

[Daniel Cheng]

Created attachment 55328 [details]
Patch for issue

Once this patch lands, I can land the Chromium-side changes and remove the hacks in ClipboardChromium.

[Jian Li]

Comment on attachment 55328 [details]
Patch for issue

r=me

[Jian Li]

Landed as http://trac.webkit.org/changeset/58963

[WebKit Review Bot]

http://trac.webkit.org/changeset/58963 might have broken Tiger Intel Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/58963
http://trac.webkit.org/changeset/58965

[Daniel Cheng]

Created attachment 55632 [details]
Patch

Just looking for comments for now. Dmitry commented that there might be some rationale for DragData::asURL to try as hard as possible to not return file:// URLs pointing to things that are not files.

Here are my arguments for why DragData::asURL shouldn't bother trying so hard:
1. Checking the file exists doesn't help much, since the loader will end up needing to do the check too.
2. File IO can be expensive. Why do it twice when you can do it once.
3. This code was never hit on Chromium anyway, due to the fact that we always populate the URL field if we see a URL-like thing in the drag (where that URL-like thing is plain text that happens to resemble a URL, a file object, etc).
4. If the renderer is sandboxed (which more platforms are starting to move towards), the calls will fail. This is already true of Chromium on Windows. Might as well let something which is privileged like the loader handle it.

[Daniel Cheng]

(I can't seem to set the status to open or in progress, or something like that.)

[Daniel Cheng]

Created attachment 55633 [details]
Patch

Sorry for the spam. I was working on another change and it snuck into this patch.

[Daniel Cheng]

Created attachment 55867 [details]
Patch

[Daniel Cheng]

I just noticed that the original issue the code was trying to work around was to prevent Safari from handling any directory loads. However, other WebKit clients can handle this request correctly, so I think the correct place to move this logic is into the Safari loaders. I'm going to poke around the loader code and see where the best place to add this is; I'm not really familiar with it though, so any tips would be welcome...

[Daniel Cheng]

Comment on attachment 55867 [details]
Patch

Any comments on the existing patch would be welcome as well, while I work on the loader change.

[Daniel Cheng]

Created attachment 55903 [details]
Patch

[Evan Martin]

Comment on attachment 55903 [details]
Patch

Setting commit-queue minus just so this doesn't get accidentally committed until you've verified it won't break Safari.

[Jian Li]

Comment on attachment 55903 [details]
Patch

Please also ask someone who is familiar with WebNSPasteboardExtras.mm to take a look.

LayoutTests/ChangeLog:3
 +          DragData::asURL() shouldn't do file validity checks
Since you also touch WebNSPasteboardExtras.mm, could you please update the bug title and all ChangeLog to describe the problem in generic way?

LayoutTests/editing/pasteboard/file-input-files-access.html: 
 +  <script src="../../fast/js/resources/js-test-post.js"></script>
Why do we need to remove this?

LayoutTests/editing/pasteboard/script-tests/file-input-files-access.js:112
 +      // It'd be easiest to use try-finally, but it doesn't work.
This comment is somewhat confusing. Can you explain?

LayoutTests/editing/pasteboard/script-tests/file-input-files-access.js:115
 +          // file input here.
Don't need to fold into 2 lines.

WebKit/mac/Misc/WebNSPasteboardExtras.mm:150
 +                  isDirectory)
Don't need to fold into 2 lines.

WebKit/mac/ChangeLog:11
 +          handling, since the Mac loader doesn't work correctly with them. 
I think there is a risk condition here. If the directory does not exist at first, [NSPasteboard _web_bestURL] will return a file URL. Then the directory is created.

[Daniel Cheng]

Created attachment 56174 [details]
Patch

[Daniel Cheng]

(In reply to comment #13)
> (From update of attachment 55903 [details])
> Please also ask someone who is familiar with WebNSPasteboardExtras.mm to take a look.
> 

I've asked [e-mail] to take a look.

> LayoutTests/ChangeLog:3
>  +          DragData::asURL() shouldn't do file validity checks
> Since you also touch WebNSPasteboardExtras.mm, could you please update the bug title and all ChangeLog to describe the problem in generic way?
> 

I updated the ChangeLogs to try to phrase the issue more clearly.

> LayoutTests/editing/pasteboard/file-input-files-access.html: 
>  +  <script src="../../fast/js/resources/js-test-post.js"></script>
> Why do we need to remove this?
> 

This is removed. Since the final drop test triggers a navigation, nothing executes after that. That's why the usual post-test script is manually embedded in onbeforeunload.

> LayoutTests/editing/pasteboard/script-tests/file-input-files-access.js:112
>  +      // It'd be easiest to use try-finally, but it doesn't work.
> This comment is somewhat confusing. Can you explain?
> 

I removed the comment, but originally, I was hoping that I could do:
try {
  // something that triggers navigation
} finally {
  // verify test results
}

But finally blocks do not execute if the UA is navigating away from the page.

> LayoutTests/editing/pasteboard/script-tests/file-input-files-access.js:115
>  +          // file input here.
> Don't need to fold into 2 lines.
> 

Fixed.

> WebKit/mac/Misc/WebNSPasteboardExtras.mm:150
>  +                  isDirectory)
> Don't need to fold into 2 lines.
> 

Fixed.

> WebKit/mac/ChangeLog:11
>  +          handling, since the Mac loader doesn't work correctly with them. 
> I think there is a risk condition here. If the directory does not exist at first, [NSPasteboard _web_bestURL] will return a file URL. Then the directory is created.

The original code has race conditions too. Maybe the file exists but is deleted before the loader processes it, or maybe something is initially a file and then gets deleted and a directory with the same name is created. There's no real easy way to solve this without changing the loader, and I am not comfortable changing the Mac loader.

[Eric Seidel (no email)]

Comment on attachment 55328 [details]
Patch for issue

Cleared Jian Li's review+ from obsolete attachment 55328 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Daniel Cheng]

Darin, can you take a look at the WebNSPasteboardExtras changes?

[Darin Adler]

Comment on attachment 56174 [details]
Patch

> +        // FIXME: maybe it makes more sense to allow multiple files and only use the first one?

We capitalize sentences and fragments in comments, so this should be capitalized.

> +            // FIXME: this check should move to the loader.

And this.

> -            BOOL isDirectory;
> +            BOOL isDirectory = false;

This should be "NO" rather than "false". But also, the change is not needed. That function will always set the BOOL we pass and the code was already fine in this respect.

> -            if([[NSFileManager defaultManager] fileExistsAtPath:file isDirectory:&isDirectory] && !isDirectory){
> -                return [[NSURL fileURLWithPath:file] _webkit_canonicalize];
> -            }
> +            if ([[NSFileManager defaultManager] fileExistsAtPath:file isDirectory:&isDirectory] && isDirectory)
> +                return nil;
> +            // Purposely allow invalid paths to fall through so the loader can handle it.

So the loader can handle *them*, not *it*.

I think this comment is really confusing. It seems fine to just remove all the checking entirely without comment. What I think is needed is a comment about the strange code that tries to find out if a file is a directory. What you say in the change log is closer to what's needed here. The comment needs to explain that we are filtering out directories here because we always did that in the past and for now we end up opening directories in the Finder without the check. The check needs to go away as soon as possible.

I'm quite confused about the "loader" you are referring to. The term "loader" means something specific in WebKit, and somehow I don't think you mean the same thing I do by that word. Maybe you could be more specific about what you mean by "loader".

[Daniel Cheng]

Created attachment 56281 [details]
Patch

[Daniel Cheng]

(In reply to comment #18)
> (From update of attachment 56174 [details])
> > +        // FIXME: maybe it makes more sense to allow multiple files and only use the first one?
> 
> We capitalize sentences and fragments in comments, so this should be capitalized.
> 
Done.

> > +            // FIXME: this check should move to the loader.
> 
> And this.
> 
Done.

> > -            BOOL isDirectory;
> > +            BOOL isDirectory = false;
> 
> This should be "NO" rather than "false". But also, the change is not needed. That function will always set the BOOL we pass and the code was already fine in this respect.
> 
Reverted this line.

> > -            if([[NSFileManager defaultManager] fileExistsAtPath:file isDirectory:&isDirectory] && !isDirectory){
> > -                return [[NSURL fileURLWithPath:file] _webkit_canonicalize];
> > -            }
> > +            if ([[NSFileManager defaultManager] fileExistsAtPath:file isDirectory:&isDirectory] && isDirectory)
> > +                return nil;
> > +            // Purposely allow invalid paths to fall through so the loader can handle it.
> 
> So the loader can handle *them*, not *it*.
> 
> I think this comment is really confusing. It seems fine to just remove all the checking entirely without comment. What I think is needed is a comment about the strange code that tries to find out if a file is a directory. What you say in the change log is closer to what's needed here. The comment needs to explain that we are filtering out directories here because we always did that in the past and for now we end up opening directories in the Finder without the check. The check needs to go away as soon as possible.
> 
> I'm quite confused about the "loader" you are referring to. The term "loader" means something specific in WebKit, and somehow I don't think you mean the same thing I do by that word. Maybe you could be more specific about what you mean by "loader".

I updated the ChangeLog to be more specific about what I was referring to.

Thanks for the quick review.

[Darin Adler]

Comment on attachment 56281 [details]
Patch

>  <p id="description"></p>
>  <div id="console"></div>
>  <script src="script-tests/file-input-files-access.js"></script>
> -<script src="../../fast/js/resources/js-test-post.js"></script>
>  </body>
>  </html>

This edit will be un-done next time someone runs the make-script-test-wrappers script.

[Daniel Cheng]

Created attachment 56284 [details]
Patch

[Daniel Cheng]

> This edit will be un-done next time someone runs the make-script-test-wrappers script.

Reverted.

[Kent Tamura]

Comment on attachment 56284 [details]
Patch

I'll land this manually.

[Kent Tamura]

Comment on attachment 56284 [details]
Patch

Clearing flags on attachment: 56284

Committed r59652: <http://trac.webkit.org/changeset/59652>

[Kent Tamura]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/59652 might have broken SnowLeopard Intel Release (Tests)

[Kent Tamura]

(In reply to comment #25)
> (From update of attachment 56284 [details])
> Clearing flags on attachment: 56284
> 
> Committed r59652: <http://trac.webkit.org/changeset/59652>

Rolled out because it made editing/pasteboard/files-during-page-drags.html failing.

[Daniel Cheng]

Created attachment 56325 [details]
Patch

[Oliver Hunt]

(In reply to comment #29)
> Created an attachment (id=56325) [details]
> Patch

Given darin has been driving reviews for this patch, could we give him time to respond to the most recent iteration?

[Daniel Cheng]

(In reply to comment #30)
> (In reply to comment #29)
> > Created an attachment (id=56325) [details] [details]
> > Patch
> 
> Given darin has been driving reviews for this patch, could we give him time to respond to the most recent iteration?

Yep. I'm going to upload one more patch, which more closely follows the style of the other layout test that uses onbeforeunload in a similar manner. I don't understand the weird behavior that onbeforeunload is exhibiting, so I'm hoping he can shed some light on that as well.

[Daniel Cheng]

Created attachment 56326 [details]
Patch

[Darin Adler]

Comment on attachment 56326 [details]
Patch

> +        // The loader will do the right thing if the filename is invalid for one reason or another.
> +        url = ChromiumBridge::filePathToURL(fileName).string();

I think the reference to "the loader" here is confusing and unnecessary. The comment is more about the old code than it is about the code that remains and could be omitted entirely.

[Daniel Cheng]

Created attachment 56394 [details]
Patch

[Daniel Cheng]

Comment on attachment 56394 [details]
Patch

Ignore this patch. I can't read.

[Daniel Cheng]

Created attachment 56396 [details]
Patch

[Jian Li]

Comment on attachment 56396 [details]
Patch

r=darin,me

Reapproved this patch after dcheng updated the patch to address darin's comment after he approved it.

[Daniel Cheng]

Landed by dimich in http://trac.webkit.org/changeset/59689