Bug 175081 – Web Automation: consider file extensions in the "accept" attribute when deciding if a file can be uploaded

• Component: WebKit2
• Status: RESOLVED FIXED

[Blaze Burg]

Followup to https://bugs.webkit.org/show_bug.cgi?id=174803 now that https://bugs.webkit.org/show_bug.cgi?id=95698 is fixed.

[Blaze Burg]

Created attachment 316983 [details]
Patch

[Build Bot]

Comment on attachment 316983 [details]
Patch

Attachment 316983 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4241503

New failing tests:
imported/w3c/web-platform-tests/fetch/api/cors/cors-no-preflight.any.worker.html
imported/w3c/web-platform-tests/fetch/api/cors/cors-basic.any.html
imported/w3c/web-platform-tests/fetch/api/cors/cors-basic.any.worker.html
imported/w3c/web-platform-tests/fetch/api/cors/cors-origin.any.worker.html
imported/w3c/web-platform-tests/fetch/api/cors/cors-origin.any.html
imported/w3c/web-platform-tests/fetch/api/cors/cors-no-preflight.any.html

[Build Bot]

Created attachment 316991 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 316983 [details]
Patch

Attachment 316983 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4243532

New failing tests:
fast/dom/StyleSheet/detached-sheet-owner-node-link.html

[Build Bot]

Created attachment 317075 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Joseph Pecoraro]

Comment on attachment 316983 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=316983&action=review

r=me, but you may want to tighten up the extension checking.

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:540
>      String extension = filename.substring(filename.reverseFind('.') + 1);

There must be a better way to get a file's extension.

However this does pretty much match what `File::computeNameAndContentType` does it in WebCore/fileapi/File.cpp.

• What if this is a hidden file: ~/.bashrc
• What if this doesn't have an extension? /tmp/foo
• Is the filename here just a name or a path? /tmp/.hidden/foo

I think you will still at the very least want to handle if reverseFind returns WTF::notFound.

[Blaze Burg]

(In reply to Joseph Pecoraro from comment #6)
> Comment on attachment 316983 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=316983&action=review
> 
> r=me, but you may want to tighten up the extension checking.
> 
> > Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:540
> >      String extension = filename.substring(filename.reverseFind('.') + 1);
> 
> There must be a better way to get a file's extension.
> 
> However this does pretty much match what `File::computeNameAndContentType`
> does it in WebCore/fileapi/File.cpp.
> 
> • What if this is a hidden file: ~/.bashrc
> • What if this doesn't have an extension? /tmp/foo
> • Is the filename here just a name or a path? /tmp/.hidden/foo

MIME type inference will fail in all of these cases, so it doesn't really matter.

> I think you will still at the very least want to handle if reverseFind
> returns WTF::notFound.

Yep.

[Blaze Burg]

Committed r220222: <http://trac.webkit.org/changeset/220222>

[Radar WebKit Bug Importer]

<rdar://problem/33707206>

[Darin Adler]

Comment on attachment 316983 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=316983&action=review

>>> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:540
>>>      String extension = filename.substring(filename.reverseFind('.') + 1);
>> 
>> There must be a better way to get a file's extension.
>> 
>> However this does pretty much match what `File::computeNameAndContentType` does it in WebCore/fileapi/File.cpp.
>> 
>> • What if this is a hidden file: ~/.bashrc
>> • What if this doesn't have an extension? /tmp/foo
>> • Is the filename here just a name or a path? /tmp/.hidden/foo
>> 
>> I think you will still at the very least want to handle if reverseFind returns WTF::notFound.
> 
> MIME type inference will fail in all of these cases, so it doesn't really matter.

I think it’s not great style to do this rather than using a helper function that can do it right.

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:541
> +    if (allowedFileExtensions.contains(extension))

Is this meant to be case sensitive? Or ASCII case insensitive?

[Blaze Burg]

Comment on attachment 316983 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=316983&action=review

>>>> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:540
>>>>      String extension = filename.substring(filename.reverseFind('.') + 1);
>>> 
>>> There must be a better way to get a file's extension.
>>> 
>>> However this does pretty much match what `File::computeNameAndContentType` does it in WebCore/fileapi/File.cpp.
>>> 
>>> • What if this is a hidden file: ~/.bashrc
>>> • What if this doesn't have an extension? /tmp/foo
>>> • Is the filename here just a name or a path? /tmp/.hidden/foo
>>> 
>>> I think you will still at the very least want to handle if reverseFind returns WTF::notFound.
>> 
>> MIME type inference will fail in all of these cases, so it doesn't really matter.
> 
> I think it’s not great style to do this rather than using a helper function that can do it right.

The patch I landed handles the notFound case.

>> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:541
>> +    if (allowedFileExtensions.contains(extension))
> 
> Is this meant to be case sensitive? Or ASCII case insensitive?

This is a great point that Joe and I both missed, and it applies to the MIME types as well. I'll fix this in a followup. WebCore does convertToASCIILowercase on the accept values, so we need to do that for the filenames that we get via WebDriver and any MIME types that we infer.

https://html.spec.whatwg.org/multipage/input.html#file-upload-state-(type=file)