Bug 161943 - input.type cannot be set to "file" after being set to another type
Summary: input.type cannot be set to "file" after being set to another type
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: WebExposed
Depends on:
Blocks:
 
Reported: 2016-09-13 19:39 PDT by Chris Dumez
Modified: 2016-09-14 14:56 PDT (History)
7 users (show)

See Also:


Attachments
Patch (33.87 KB, patch)
2016-09-13 21:16 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews121 for ios-simulator-elcapitan-wk2 (deleted)
2016-09-14 01:00 PDT, Build Bot
no flags Details
Patch (51.68 KB, patch)
2016-09-14 08:56 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2016-09-13 19:39:34 PDT
input.type cannot be set to "file" after being set to another type. This behavior does not match the HTML specification or the behavior of Firefox and Chrome.
Comment 1 Chris Dumez 2016-09-13 21:16:28 PDT
Created attachment 288767 [details]
Patch
Comment 2 Build Bot 2016-09-14 01:00:11 PDT
Comment on attachment 288767 [details]
Patch

Attachment 288767 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2070544

New failing tests:
imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/type-change-state.html
imported/w3c/web-platform-tests/html/dom/reflection-forms.html
Comment 3 Build Bot 2016-09-14 01:00:15 PDT
Created attachment 288780 [details]
Archive of layout-test-results from ews121 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 4 Chris Dumez 2016-09-14 08:56:49 PDT
Created attachment 288823 [details]
Patch
Comment 5 WebKit Commit Bot 2016-09-14 09:44:10 PDT
Comment on attachment 288823 [details]
Patch

Rejecting attachment 288823 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 288823, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
ayoutTests/imported/w3c/ChangeLog
error: Error building trees

Failed to run "['git', 'commit', '--all', '-F', '-']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

You have both LayoutTests/imported/w3c and LayoutTests/imported/w3c/ChangeLog
You have both LayoutTests/imported/w3c and LayoutTests/imported/w3c/ChangeLog
error: Error building trees

Failed to run "['git', 'commit', '--all', '-F', '-']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit
Updating OpenSource
Current branch master is up to date.

Full output: http://webkit-queues.webkit.org/results/2072941
Comment 6 Chris Dumez 2016-09-14 09:58:14 PDT
Comment on attachment 288823 [details]
Patch

Clearing flags on attachment: 288823

Committed r205912: <http://trac.webkit.org/changeset/205912>
Comment 7 Chris Dumez 2016-09-14 09:58:19 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Darin Adler 2016-09-14 10:05:37 PDT
This was originally done for security reasons. I want to be sure there is no way to get a file input into a state where it points to an arbitrary file because it is first set into a state while it has another type.
Comment 9 Chris Dumez 2016-09-14 10:07:12 PDT
(In reply to comment #8)
> This was originally done for security reasons. I want to be sure there is no
> way to get a file input into a state where it points to an arbitrary file
> because it is first set into a state while it has another type.

The comment was saying this was security reasons but also indicated that this would not work in WebKit anyway and was there for compatibility reasons. Also note that I added a layout test that covers the case that was mentioned in the original comment.
Comment 10 Darin Adler 2016-09-14 12:00:57 PDT
Sounds like you have the bases covered. I would also look at the various functions and try to think about whether there's any restrictions we put on file elements once their type is set that could somehow be bypassed by this.
Comment 11 Chris Dumez 2016-09-14 14:56:13 PDT
(In reply to comment #10)
> Sounds like you have the bases covered. I would also look at the various
> functions and try to think about whether there's any restrictions we put on
> file elements once their type is set that could somehow be bypassed by this.

FileInputType does not allow setting the value to anything else than the empty string:
bool FileInputType::canSetValue(const String& value)
{
    // For security reasons, we don't allow setting the filename, but we do allow clearing it.
    // The HTML5 spec (as of the 10/24/08 working draft) says that the value attribute isn't
    // applicable to the file upload control at all, but for now we are keeping this behavior
    // to avoid breaking existing websites that may be relying on this.
    return value.isEmpty();
}

In any case, the value attribute actually does not matter when the input type is "file" because what we actually rely on is FileInputType::m_fileList which is set by FileInputType::setFiles(), which is set by FileInputType::filesChosen() which is called by the FileChooser.
We don't construct m_fileList from the value ever. Whe requesting the value of a FileInputType, we use: "C:\\fakepath\\" + m_fileList->item(0)->name().