Summary: | Web Automation: consider file extensions in the "accept" attribute when deciding if a file can be uploaded | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Blaze Burg <bburg> | ||||||||
Component: | WebKit2 | Assignee: | Blaze Burg <bburg> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | bburg, buildbot, cgarcia, darin, ggaren, joepeck, mcatanzaro, rniwa, thorton, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Blaze Burg
2017-08-02 11:45:23 PDT
Created attachment 316983 [details]
Patch
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 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
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 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
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. (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. Committed r220222: <http://trac.webkit.org/changeset/220222> 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? 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) |