Bug 109308 – Web Inspector: Add command for selecting files for file input element

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Ken Kania]

We'd like to support choosing files for a file input element for automation. Proposal:

DOM.setFileInputFiles
nodeId: <NodeId>
files: Array<string>

For the chromium port, we'd have to allow the renderer to access these files in the browser process.

[Ken Kania]

Created attachment 187334 [details]
Patch

[Pavel Feldman]

Comment on attachment 187334 [details]
Patch

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

> Source/WebCore/inspector/Inspector.json:2106
> +                "name": "setFileInputFiles",

I am not sure this fits DOM domain well, but I have no better idea.

> LayoutTests/inspector-protocol/dom-focus.html:-6
> -window.addEventListener("load", function() {

Please move these in a separate change.

[Ken Kania]

Created attachment 187613 [details]
Patch

[Joseph Pecoraro]

Neat. Is this intended to be protocol only (automating interaction with a page through the inspector protocol), or will there be UI for this in the frontend?

Setting arbitrary file paths on an <input type="file"> through the protocol sounds like it could be dangerous. I'm thinking of remotely inspecting a mobile device where often the file system is obscured to users. Take iOS for instance, setting arbitrary paths through <input type="file"> is not possible, but if exposed through the protocol here, it would be. Maybe it should just be up to the port to protect against this?

[Ken Kania]

(In reply to comment #4)
> Neat. Is this intended to be protocol only (automating interaction with a page through the inspector protocol), or will there be UI for this in the frontend?
> 
> Setting arbitrary file paths on an <input type="file"> through the protocol sounds like it could be dangerous. I'm thinking of remotely inspecting a mobile device where often the file system is obscured to users. Take iOS for instance, setting arbitrary paths through <input type="file"> is not possible, but if exposed through the protocol here, it would be. Maybe it should just be up to the port to protect against this?

My intention is only for automation. I think handling the validity/security of the file upload could be done at the port level. Is this feasible for iOS with the current design?

[Joseph Pecoraro]

(In reply to comment #5)
> (In reply to comment #4)
> > Neat. Is this intended to be protocol only (automating interaction with a page through the inspector protocol), or will there be UI for this in the frontend?
> > 
> > Setting arbitrary file paths on an <input type="file"> through the protocol sounds like it could be dangerous. I'm thinking of remotely inspecting a mobile device where often the file system is obscured to users. Take iOS for instance, setting arbitrary paths through <input type="file"> is not possible, but if exposed through the protocol here, it would be. Maybe it should just be up to the port to protect against this?
> 
> My intention is only for automation. I think handling the validity/security of the file upload could be done at the port level. Is this feasible for iOS with the current design?

That sounds reasonable. The port should be security conscious if it needs to be.

However, it would still be nice to opt out of this if needed. This could be done in the same way "canClearBrowserCache" and friends are done. An InspectorClient method, like:

    virtual bool canSetFileInputFiles() { return false; }

And an optional protocol method to see if the feature is enabled. If not a protocol method, then an error string with your new method to let the that the front-end, or automated front-end, to know whether or not the feature is supported. This would let the front-end know immediately that things are not supported, instead of failing later attempting the upload.

[Joseph Pecoraro]

> If not a protocol method, then an error string with your new method to let the that the front-end, or automated front-end, to know whether or not the feature is supported.

Correction: If not a protocol method, then an error result when your new protocol method is called. This would let the front-end, or automated front-end, know whether or not the feature is supported. [...]

Sorry for my poor grammar!

[Ken Kania]

Created attachment 187711 [details]
Patch

[Build Bot]

Comment on attachment 187711 [details]
Patch

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

New failing tests:
inspector-protocol/dom/setFileInputFiles.html

[Yury Semikhatsky]

Comment on attachment 187711 [details]
Patch

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

> LayoutTests/inspector-protocol/dom/setFileInputFiles.html:6
> +window.addEventListener("load", onLoad);

To make it clear that onLoad is executed before runtTest I'd put runtTest() call at the end of onLoad and set onLoad as onload handler for <body>

> LayoutTests/inspector-protocol/dom/setFileInputFiles.html:24
> +    function abortOnError(msg)

WebKit style guide discourages using abbreviations.

[Yury Semikhatsky]

(In reply to comment #6)
> However, it would still be nice to opt out of this if needed. This could be done in the same way "canClearBrowserCache" and friends are done. An InspectorClient method, like:
> 
>     virtual bool canSetFileInputFiles() { return false; }
> 
Why not opt-in by default an have some ports to opt-out when needed?

[Timothy Hatcher]

(In reply to comment #11)
> (In reply to comment #6)
> > However, it would still be nice to opt out of this if needed. This could be done in the same way "canClearBrowserCache" and friends are done. An InspectorClient method, like:
> > 
> >     virtual bool canSetFileInputFiles() { return false; }
> > 
> Why not opt-in by default an have some ports to opt-out when needed?

I think opt-in makes more sense. This feature is only needed for automation, and that is not the focus for most ports. And as Joe mentioned this can be a security concern that would just be sitting there waiting to be exploited when it isn't needed by the port.

[Ken Kania]

Created attachment 187887 [details]
Patch

[Ken Kania]

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

Addressed comments. Made default be to not allow setFileInputFiles. Enabled for chromium and added chromium-specific expectation.

[WebKit Review Bot]

Comment on attachment 187887 [details]
Patch

Clearing flags on attachment: 187887

Committed r143574: <http://trac.webkit.org/changeset/143574>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.