Implement Content Security Policy for the File Manager

Implement Content Security Policy for the File Manager

The policy includes default-src:none and explicitly lists what is permitted. 

The only seemingly lax part is "style-src 'unsafe-inline'" which is required because the HTML flattener always inlines all CSS.

BUG=chromium-os:23500
TEST=Photos/videos/music should open normally (both from local folders and from Google Docs).


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=132558

[Vladislav Kaznacheev, 2012-04-12 10:37:58]

Sumit, please review the change to manifest.json

Dmitry, please review the rest of the patch.

Thanks,

  Vlad

[dgozman, 2012-04-12 11:18:14]

The whole thing LGTM.
However, some nits are here.

https://chromiumcodereview.appspot.com/10068007/diff/1/chrome/browser/resourc...
File chrome/browser/resources/file_manager/css/file_manager.css (left):

https://chromiumcodereview.appspot.com/10068007/diff/1/chrome/browser/resourc...
chrome/browser/resources/file_manager/css/file_manager.css:978:
.preview-filename {
Is this not used?

https://chromiumcodereview.appspot.com/10068007/diff/1/chrome/browser/resourc...
File chrome/browser/resources/file_manager/css/file_manager.css (right):

https://chromiumcodereview.appspot.com/10068007/diff/1/chrome/browser/resourc...
chrome/browser/resources/file_manager/css/file_manager.css:1215:
body[type='full-page'] [invisibleif='full-page'],
I'm wondering whether css selectors have something like

body[type=@invisibleif] {
  display: none;
}

https://chromiumcodereview.appspot.com/10068007/diff/1/chrome/browser/resourc...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

https://chromiumcodereview.appspot.com/10068007/diff/1/chrome/browser/resourc...
chrome/browser/resources/file_manager/js/file_manager.js:728:
this.dialogDom_.setAttribute('type', this.dialogType_);
I believe, we have a method initDialogType.

https://chromiumcodereview.appspot.com/10068007/diff/1/chrome/browser/resourc...
chrome/browser/resources/file_manager/js/file_manager.js:730:
this.dialogDom_.setAttribute('ash', true);
true -> 'true' ?

https://chromiumcodereview.appspot.com/10068007/diff/1/chrome/browser/resourc...
chrome/browser/resources/file_manager/js/file_manager.js:3315:
this.deleteEntries(this.selection.entries);
also event.preventDefault and event.stopPropagation.

https://chromiumcodereview.appspot.com/10068007/diff/1/chrome/browser/resourc...
File chrome/browser/resources/file_manager/js/main_scripts.js (right):

https://chromiumcodereview.appspot.com/10068007/diff/1/chrome/browser/resourc...
chrome/browser/resources/file_manager/js/main_scripts.js:6: // parsing errors in
non-flattened mode. The flattener still sees them.
I think, this comments do not break the code, because we have copyright at the
start of each js file?
If so, it would be nice to write about this.

https://chromiumcodereview.appspot.com/10068007/diff/1/chrome/browser/resourc...
File chrome/browser/resources/file_manager/js/metrics.js (right):

https://chromiumcodereview.appspot.com/10068007/diff/1/chrome/browser/resourc...
chrome/browser/resources/file_manager/js/metrics.js:18: document.write('<script
src="js/mock_chrome.js"><\57script>');
Oh, now I see.

https://chromiumcodereview.appspot.com/10068007/diff/1/chrome/browser/resourc...
File chrome/browser/resources/file_manager/main.html (left):

https://chromiumcodereview.appspot.com/10068007/diff/1/chrome/browser/resourc...
chrome/browser/resources/file_manager/main.html:46: document.write('<script
src="js/mock_chrome.js"><\57script>');
I don't see where we include mock_chrome now.

[Vladislav Kaznacheev, 2012-04-12 12:47:31]

https://chromiumcodereview.appspot.com/10068007/diff/1/chrome/browser/resourc...
File chrome/browser/resources/file_manager/css/file_manager.css (left):

https://chromiumcodereview.appspot.com/10068007/diff/1/chrome/browser/resourc...
chrome/browser/resources/file_manager/css/file_manager.css:978:
.preview-filename {
Yes, this is not used since we removed the right preview panel. Note that we are
not going to get that nasty CSS warning about the data: url anymore.
On 2012/04/12 11:18:14, dgozman wrote:
> Is this not used?

https://chromiumcodereview.appspot.com/10068007/diff/1/chrome/browser/resourc...
File chrome/browser/resources/file_manager/css/file_manager.css (right):

https://chromiumcodereview.appspot.com/10068007/diff/1/chrome/browser/resourc...
chrome/browser/resources/file_manager/css/file_manager.css:1215:
body[type='full-page'] [invisibleif='full-page'],
Not that I know of.
On 2012/04/12 11:18:14, dgozman wrote:
> I'm wondering whether css selectors have something like
> 
> body[type=@invisibleif] {
>   display: none;
> }

https://chromiumcodereview.appspot.com/10068007/diff/1/chrome/browser/resourc...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

https://chromiumcodereview.appspot.com/10068007/diff/1/chrome/browser/resourc...
chrome/browser/resources/file_manager/js/file_manager.js:728:
this.dialogDom_.setAttribute('type', this.dialogType_);
On 2012/04/12 11:18:14, dgozman wrote:
> I believe, we have a method initDialogType.

Done.

https://chromiumcodereview.appspot.com/10068007/diff/1/chrome/browser/resourc...
chrome/browser/resources/file_manager/js/file_manager.js:730:
this.dialogDom_.setAttribute('ash', true);
On 2012/04/12 11:18:14, dgozman wrote:
> true -> 'true' ?

Done.

https://chromiumcodereview.appspot.com/10068007/diff/1/chrome/browser/resourc...
chrome/browser/resources/file_manager/js/file_manager.js:3315:
this.deleteEntries(this.selection.entries);
On 2012/04/12 11:18:14, dgozman wrote:
> also event.preventDefault and event.stopPropagation.

Done.

https://chromiumcodereview.appspot.com/10068007/diff/1/chrome/browser/resourc...
File chrome/browser/resources/file_manager/js/main_scripts.js (right):

https://chromiumcodereview.appspot.com/10068007/diff/1/chrome/browser/resourc...
chrome/browser/resources/file_manager/js/main_scripts.js:6: // parsing errors in
non-flattened mode. The flattener still sees them.
Not sure what you mean.
On 2012/04/12 11:18:14, dgozman wrote:
> I think, this comments do not break the code, because we have copyright at the
> start of each js file?
> If so, it would be nice to write about this.

[dgozman, 2012-04-12 12:50:08]

LGTM

https://chromiumcodereview.appspot.com/10068007/diff/1/chrome/browser/resourc...
File chrome/browser/resources/file_manager/js/main_scripts.js (right):

https://chromiumcodereview.appspot.com/10068007/diff/1/chrome/browser/resourc...
chrome/browser/resources/file_manager/js/main_scripts.js:6: // parsing errors in
non-flattened mode. The flattener still sees them.
On 2012/04/12 12:47:32, Vladislav Kaznacheev wrote:
> Not sure what you mean.
> On 2012/04/12 11:18:14, dgozman wrote:
> > I think, this comments do not break the code, because we have copyright at
the
> > start of each js file?
> > If so, it would be nice to write about this.
> 

The following include translates to
//<here goes the first string of included file>
And it would break, if the first string has some code.

[Vladislav Kaznacheev, 2012-04-12 14:07:09]

https://chromiumcodereview.appspot.com/10068007/diff/1/chrome/browser/resourc...
File chrome/browser/resources/file_manager/js/main_scripts.js (right):

https://chromiumcodereview.appspot.com/10068007/diff/1/chrome/browser/resourc...
chrome/browser/resources/file_manager/js/main_scripts.js:6: // parsing errors in
non-flattened mode. The flattener still sees them.
Oh, I see. Added a comment.
On 2012/04/12 12:50:08, dgozman wrote:
> On 2012/04/12 12:47:32, Vladislav Kaznacheev wrote:
> > Not sure what you mean.
> > On 2012/04/12 11:18:14, dgozman wrote:
> > > I think, this comments do not break the code, because we have copyright at
> the
> > > start of each js file?
> > > If so, it would be nice to write about this.
> > 
> 
> The following include translates to
> //<here goes the first string of included file>
> And it would break, if the first string has some code.

[Vladislav Kaznacheev, 2012-04-16 14:27:10]

Hi Jim,

May I ask you to take a look at this patch, specifically at manifest.json?

Thanks a lot,

  Vlad

[jimhebert, 2012-04-16 16:33:24]

manifest.json LGTM! Thanks for doing this.

On 2012/04/16 14:27:10, Vladislav Kaznacheev wrote:
> Hi Jim,
> 
> May I ask you to take a look at this patch, specifically at manifest.json?
> 
> Thanks a lot,
> 
>   Vlad

[commit-bot: I haz the power, 2012-04-17 09:01:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaznacheev@chromium.org/10068007/12001

[commit-bot: I haz the power, 2012-04-17 10:32:09]

Change committed as 132558