Add a path for a web page to request the enumeration of a directory. This, t...

Add a path for a web page to request the enumeration of a directory.  This, together with a WebKit change, will allow a drag-and-drop on a Directory Upload control (<input type=file webkitdirectory>) which provides only the path to the renderer, to correctly populate the control as if the user had selected that directory in a file picker.

BUG=58977
TEST=drag-and-drop on directory upload control (with upstream change)
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=81183

[John Gregg, 2011-03-04 19:11:15]

jcivelli: Please review file_select_helper changes.  

Basically I am changing from one directory enumeration following a
directory-picker to allowing multiple directory enumerations.

jam: Please review render_view through tab_contents.

Thanks!

[jam, 2011-03-04 21:35:46]

-I don't see you checking that the renderer supplied directory was known by the
browser to be granted to that specific renderer process.  Otherwise, an
exploited renderer can crawl the user's drive and upload all their files. 
ChildProcessSecurityPolicy will have to be used, and a method like GrantReadFile
(i.e. GrantReadDir) will have to added.  Adding Justin as a security reviewer.

-also, please see
http://dev.chromium.org/developers/design-documents/multi-process-architectur...
.  We don't want to add new features in
TabContents/RenderView/RenderViewObserver.  Instead they should be added in
separate classes.

[John Gregg, 2011-03-04 21:44:16]

On 2011/03/04 21:35:46, John Abd-El-Malek wrote:
> -I don't see you checking that the renderer supplied directory was known by
the
> browser to be granted to that specific renderer process.  Otherwise, an
> exploited renderer can crawl the user's drive and upload all their files. 
> ChildProcessSecurityPolicy will have to be used, and a method like
GrantReadFile
> (i.e. GrantReadDir) will have to added.  Adding Justin as a security reviewer.

Okay, it seems reasonable to me that dragging a folder onto a web page implies
granting permission, so if we are okay with that I will find the appropriate
place in the drag-and-drop code.

> -also, please see
>
http://dev.chromium.org/developers/design-documents/multi-process-architectur...
> .  We don't want to add new features in
> TabContents/RenderView/RenderViewObserver.  Instead they should be added in
> separate classes.

Would you be in favor of moving the existing file-picker stuff out of
TabContents/RenderView?  I could still make a new class but I see this as
related, thus sharing the same FileSelectHelper.

[jam, 2011-03-04 22:01:19]

On Fri, Mar 4, 2011 at 1:44 PM, <johnnyg@chromium.org> wrote:

> On 2011/03/04 21:35:46, John Abd-El-Malek wrote:
>
>> -I don't see you checking that the renderer supplied directory was known
>> by
>>
> the
>
>> browser to be granted to that specific renderer process.  Otherwise, an
>> exploited renderer can crawl the user's drive and upload all their files.
>> ChildProcessSecurityPolicy will have to be used, and a method like
>>
> GrantReadFile
>
>> (i.e. GrantReadDir) will have to added.  Adding Justin as a security
>> reviewer.
>>
>
> Okay, it seems reasonable to me that dragging a folder onto a web page
> implies
> granting permission, so if we are okay with that I will find the
> appropriate
> place in the drag-and-drop code.


Sounds like a plan, but please double check with the security folks.


>
>  -also, please see
>>
>
>
>
http://dev.chromium.org/developers/design-documents/multi-process-architectur...
>
>> .  We don't want to add new features in
>> TabContents/RenderView/RenderViewObserver.  Instead they should be added
>> in
>> separate classes.
>>
>
> Would you be in favor of moving the existing file-picker stuff out of
> TabContents/RenderView?  I could still make a new class but I see this as
> related, thus sharing the same FileSelectHelper.


Yes please, all the related code should move out too.


>
>
> http://codereview.chromium.org/6623015/
>

[jschuh, 2011-03-07 17:07:48]

On 2011/03/04 22:01:19, John Abd-El-Malek wrote:
> On Fri, Mar 4, 2011 at 1:44 PM, <mailto:johnnyg@chromium.org> wrote:
> 
> > On 2011/03/04 21:35:46, John Abd-El-Malek wrote:
> >
> >> -I don't see you checking that the renderer supplied directory was known
> >> by
> >>
> > the
> >
> >> browser to be granted to that specific renderer process.  Otherwise, an
> >> exploited renderer can crawl the user's drive and upload all their files.
> >> ChildProcessSecurityPolicy will have to be used, and a method like
> >>
> > GrantReadFile
> >
> >> (i.e. GrantReadDir) will have to added.  Adding Justin as a security
> >> reviewer.
> >>
> >
> > Okay, it seems reasonable to me that dragging a folder onto a web page
> > implies
> > granting permission, so if we are okay with that I will find the
> > appropriate
> > place in the drag-and-drop code.
> 
> 
> Sounds like a plan, but please double check with the security folks.

Yes, that sounds good (basically the same as the single file case). And thanks,
John, for catching this. It would have pretty much killed the confidentiality
provided by the sandbox.

[Jay Civelli, 2011-03-07 17:27:12]

file_selected.[h, cc] LGTM

http://codereview.chromium.org/6623015/diff/6005/chrome/browser/file_select_h...
File chrome/browser/file_select_helper.h (right):

http://codereview.chromium.org/6623015/diff/6005/chrome/browser/file_select_h...
chrome/browser/file_select_helper.h:83: int error);
Nit: this can fit on the previous line.

[John Gregg, 2011-03-30 00:40:31]

Please take another look for the security policy and refactored IPC
implementation

[John Gregg, 2011-04-04 19:17:14]

ping?

On 2011/03/30 00:40:31, John Gregg wrote:
> Please take another look for the security policy and refactored IPC
> implementation

[mprocopio, 2011-04-06 19:19:44]

Hi reviewers,

Mike here on the Google Docs team. Thanks in advance for your help in reviewing
this CL--we're eagerly awaiting folder Drag and Drop in Chrome!

Please let us know if there is anything we can do; we'll be able to provide
quick turnaround on testing and feedback once in the dev channel.

Thanks!

Mike


On 2011/04/04 19:17:14, John Gregg wrote:
> ping?
> 
> On 2011/03/30 00:40:31, John Gregg wrote:
> > Please take another look for the security policy and refactored IPC
> > implementation

[jam, 2011-04-07 22:13:18]

the multi-proc stuff looks good to me, as well as the security stuff, but best
to have a security person look over it as well before checkin.

[jschuh, 2011-04-07 23:28:04]

I'm still excavating myself from a pile that built up over my vacation last
week. I'll take a look at it tomorrow.

-j


On Thu, Apr 7, 2011 at 3:13 PM, <jam@chromium.org> wrote:

> the multi-proc stuff looks good to me, as well as the security stuff, but
> best
> to have a security person look over it as well before checkin.
>
>
> http://codereview.chromium.org/6623015/
>

[mprocopio, 2011-04-09 01:45:55]

Thanks, everyone!

On 2011/04/07 23:28:04, Justin Schuh wrote:
> I'm still excavating myself from a pile that built up over my vacation last
> week. I'll take a look at it tomorrow.
> 
> -j
> 
> 
> On Thu, Apr 7, 2011 at 3:13 PM, <mailto:jam@chromium.org> wrote:
> 
> > the multi-proc stuff looks good to me, as well as the security stuff, but
> > best
> > to have a security person look over it as well before checkin.
> >
> >
> > http://codereview.chromium.org/6623015/
> >

[jschuh, 2011-04-11 16:12:52]

SGTM on the security end.