QuickView: Improve security by rendering images inside webview.

QuickView: Improve security by rendering images inside webview.

It addresses security concern raised on https://bugs.chromium.org/p/chromium/issues/detail?id=614228#c21 for images.
Fixes for audio and video will be sent as a separate CL.

BUG=614228, 627698
TEST=manually
third_party/closure_compiler/run_compiler

Committed: https://crrev.com/70d5586287eca66e4b7a3b4811ac091f74b0ebf9
Cr-Commit-Position: refs/heads/master@{#407960}

[oka, 2016-07-15 10:54:05]

Remove log

[oka, 2016-07-15 13:44:36]

Description was changed from

==========
QuickView: Improve security using webview.

BUG=627698
==========

to

==========
QuickView: Improve security by rendering images inside webview.

BUG=627698
==========

[oka, 2016-07-15 13:58:22]

Description was changed from

==========
QuickView: Improve security by rendering images inside webview.

BUG=627698
==========

to

==========
QuickView: Improve security by rendering images inside webview.

BUG=627698
TEST=manually
==========

[oka, 2016-07-15 13:58:22]

oka@chromium.org changed reviewers:
+ fukino@chromium.org

[oka, 2016-07-15 13:58:34]

PTAL

[oka, 2016-07-15 14:22:09]

Closure compiler.

[oka, 2016-07-15 14:22:54]

Description was changed from

==========
QuickView: Improve security by rendering images inside webview.

BUG=627698
TEST=manually
==========

to

==========
QuickView: Improve security by rendering images inside webview.

BUG=627698
TEST=manually
third_party/closure_compiler/run_compiler
==========

[fukino, 2016-07-19 06:48:28]

In a non-trivial CL like this, please add a CL description to explain what is
going to be changed (and why it needs to be changed if applicable).

https://codereview.chromium.org/2140113003/diff/80001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/elements/files_safe_img.js (right):

https://codereview.chromium.org/2140113003/diff/80001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_safe_img.js:5: var
FILE_MANAGER_EXTENSIONS_ID = 'hhaomjibdihmijegdhdafkllkbggdgoj';
Please add a blank line below.

https://codereview.chromium.org/2140113003/diff/80001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_safe_img.js:5: var
FILE_MANAGER_EXTENSIONS_ID = 'hhaomjibdihmijegdhdafkllkbggdgoj';
FILE_MANAGER_EXTENSIONS_ID is not used?

https://codereview.chromium.org/2140113003/diff/80001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_safe_img.js:17: src:
{type: String, observer: 'onSrcChange_', reflectToAttribute: true},
Use following style to be consistent with others.

src: {
  type: String,
  ...
},

https://codereview.chromium.org/2140113003/diff/80001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_safe_img.js:18: webview_:
Object
private member should not be exposed as an element's property.

https://codereview.chromium.org/2140113003/diff/80001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_safe_img.js:28: var w =
document.createElement('webview');
nit: Please use more meaningful variable name, like view or webview.

https://codereview.chromium.org/2140113003/diff/80001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_safe_img.js:34:
'contentload', function() { this.onSrcChange_(); }.bind(this));
this.onSrcChange_.bind(this) is sufficient

https://codereview.chromium.org/2140113003/diff/80001/ui/file_manager/file_ma...
File
ui/file_manager/file_manager/foreground/elements/files_safe_img_webview_content.html
(right):

https://codereview.chromium.org/2140113003/diff/80001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_safe_img_webview_content.html:9:
<link rel="stylesheet" type="text/css"
href="files_safe_img_webview_content.css"></link>
link tag is void element, and end tags must not be specified.
https://www.w3.org/TR/html-markup/syntax.html#void-element

https://codereview.chromium.org/2140113003/diff/80001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_safe_img_webview_content.html:12:
<img id="img" class="content"></img>
ditto

[oka, 2016-07-22 08:20:19]

Address comments.

[oka, 2016-07-22 08:24:15]

PTAL

https://codereview.chromium.org/2140113003/diff/80001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/elements/files_safe_img.js (right):

https://codereview.chromium.org/2140113003/diff/80001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_safe_img.js:5: var
FILE_MANAGER_EXTENSIONS_ID = 'hhaomjibdihmijegdhdafkllkbggdgoj';
On 2016/07/19 06:48:28, fukino wrote:
> Please add a blank line below.

Done.

https://codereview.chromium.org/2140113003/diff/80001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_safe_img.js:5: var
FILE_MANAGER_EXTENSIONS_ID = 'hhaomjibdihmijegdhdafkllkbggdgoj';
On 2016/07/19 06:48:28, fukino wrote:
> FILE_MANAGER_EXTENSIONS_ID is not used?

Used.

https://codereview.chromium.org/2140113003/diff/80001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_safe_img.js:17: src:
{type: String, observer: 'onSrcChange_', reflectToAttribute: true},
On 2016/07/19 06:48:28, fukino wrote:
> Use following style to be consistent with others.
> 
> src: {
>   type: String,
>   ...
> },

Done. It was auto-formated using autofmt.
https://g3doc.corp.google.com/company/editors/vim/plugins/codefmt.md?cl=head

https://codereview.chromium.org/2140113003/diff/80001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_safe_img.js:18: webview_:
Object
On 2016/07/19 06:48:28, fukino wrote:
> private member should not be exposed as an element's property.

Done.

https://codereview.chromium.org/2140113003/diff/80001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_safe_img.js:28: var w =
document.createElement('webview');
On 2016/07/19 06:48:28, fukino wrote:
> nit: Please use more meaningful variable name, like view or webview.

Done.

https://codereview.chromium.org/2140113003/diff/80001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_safe_img.js:34:
'contentload', function() { this.onSrcChange_(); }.bind(this));
On 2016/07/19 06:48:28, fukino wrote:
> this.onSrcChange_.bind(this) is sufficient

Done.

https://codereview.chromium.org/2140113003/diff/80001/ui/file_manager/file_ma...
File
ui/file_manager/file_manager/foreground/elements/files_safe_img_webview_content.html
(right):

https://codereview.chromium.org/2140113003/diff/80001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_safe_img_webview_content.html:9:
<link rel="stylesheet" type="text/css"
href="files_safe_img_webview_content.css"></link>
On 2016/07/19 06:48:28, fukino wrote:
> link tag is void element, and end tags must not be specified.
> https://www.w3.org/TR/html-markup/syntax.html#void-element

Done.

https://codereview.chromium.org/2140113003/diff/80001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_safe_img_webview_content.html:12:
<img id="img" class="content"></img>
On 2016/07/19 06:48:28, fukino wrote:
> ditto

Done.

[fukino, 2016-07-25 14:07:47]

https://codereview.chromium.org/2140113003/diff/100001/ui/file_manager/file_m...
File ui/file_manager/file_manager/foreground/elements/files_safe_img.js (right):

https://codereview.chromium.org/2140113003/diff/100001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/elements/files_safe_img.js:32: var
webview = document.createElement('webview');
question: Why do we have to create the webview element here, instead of having
it in .html file?

https://codereview.chromium.org/2140113003/diff/100001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/elements/files_safe_img.js:56:
//this.webview_ = null;
I guess this line is commented out unexpectedly. (Or, whole this line should be
removed)

https://codereview.chromium.org/2140113003/diff/100001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/elements/files_safe_img.js:58:
addEventListener('message', function(event) {
nit: window.addEventListener

https://codereview.chromium.org/2140113003/diff/100001/ui/file_manager/file_m...
File
ui/file_manager/file_manager/foreground/elements/files_safe_img_webview_content.js
(right):

https://codereview.chromium.org/2140113003/diff/100001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/elements/files_safe_img_webview_content.js:11:
addEventListener('message', function(event) {
nit: window.addEventListener

https://codereview.chromium.org/2140113003/diff/100001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/elements/files_safe_img_webview_content.js:16:
console.error('Unknown origin: ' + event.origin);
We shouldn't set content.src etc in this case.

https://codereview.chromium.org/2140113003/diff/100001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/elements/files_safe_img_webview_content.js:22:
document.oncontextmenu = function() { return false; };
nit: Use document.addEventListener('contextmenu', ... and e.preventDefault().

https://codereview.chromium.org/2140113003/diff/100001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/elements/files_safe_img_webview_content.js:24:
document.onclick = function(e) {
nit: document.addEventListener('click',...

[oka, 2016-07-26 06:18:35]

Description was changed from

==========
QuickView: Improve security by rendering images inside webview.

BUG=627698
TEST=manually
third_party/closure_compiler/run_compiler
==========

to

==========
QuickView: Improve security by rendering images inside webview.

It addresses security concern raised on
https://bugs.chromium.org/p/chromium/issues/detail?id=614228#c21 for images.
Fixes for audio and video will be sent as a separate CL.

BUG=627698
TEST=manually
third_party/closure_compiler/run_compiler
==========

[oka, 2016-07-26 06:18:59]

Description was changed from

==========
QuickView: Improve security by rendering images inside webview.

It addresses security concern raised on
https://bugs.chromium.org/p/chromium/issues/detail?id=614228#c21 for images.
Fixes for audio and video will be sent as a separate CL.

BUG=627698
TEST=manually
third_party/closure_compiler/run_compiler
==========

to

==========
QuickView: Improve security by rendering images inside webview.

It addresses security concern raised on
https://bugs.chromium.org/p/chromium/issues/detail?id=614228#c21 for images.
Fixes for audio and video will be sent as a separate CL.

BUG=627698
TEST=manually
third_party/closure_compiler/run_compiler
==========

[oka, 2016-07-26 06:19:22]

PTAL

https://codereview.chromium.org/2140113003/diff/100001/ui/file_manager/file_m...
File ui/file_manager/file_manager/foreground/elements/files_safe_img.js (right):

https://codereview.chromium.org/2140113003/diff/100001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/elements/files_safe_img.js:32: var
webview = document.createElement('webview');
On 2016/07/25 14:07:46, fukino wrote:
> question: Why do we have to create the webview element here, instead of having
> it in .html file?

We remove webview when Quick View is closed to save resouces.

https://codereview.chromium.org/2140113003/diff/100001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/elements/files_safe_img.js:56:
//this.webview_ = null;
On 2016/07/25 14:07:46, fukino wrote:
> I guess this line is commented out unexpectedly. (Or, whole this line should
be
> removed)

Done.

https://codereview.chromium.org/2140113003/diff/100001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/elements/files_safe_img.js:58:
addEventListener('message', function(event) {
On 2016/07/25 14:07:46, fukino wrote:
> nit: window.addEventListener

Done.

https://codereview.chromium.org/2140113003/diff/100001/ui/file_manager/file_m...
File
ui/file_manager/file_manager/foreground/elements/files_safe_img_webview_content.js
(right):

https://codereview.chromium.org/2140113003/diff/100001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/elements/files_safe_img_webview_content.js:11:
addEventListener('message', function(event) {
On 2016/07/25 14:07:46, fukino wrote:
> nit: window.addEventListener

Done.

https://codereview.chromium.org/2140113003/diff/100001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/elements/files_safe_img_webview_content.js:16:
console.error('Unknown origin: ' + event.origin);
On 2016/07/25 14:07:46, fukino wrote:
> We shouldn't set content.src etc in this case.

Done.

https://codereview.chromium.org/2140113003/diff/100001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/elements/files_safe_img_webview_content.js:22:
document.oncontextmenu = function() { return false; };
On 2016/07/25 14:07:46, fukino wrote:
> nit: Use document.addEventListener('contextmenu', ... and e.preventDefault().

Done.

https://codereview.chromium.org/2140113003/diff/100001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/elements/files_safe_img_webview_content.js:24:
document.onclick = function(e) {
On 2016/07/25 14:07:46, fukino wrote:
> nit: document.addEventListener('click',...

Done.

[fukino, 2016-07-26 06:39:31]

lgtm with nits

https://codereview.chromium.org/2140113003/diff/120001/ui/file_manager/file_m...
File ui/file_manager/file_manager/foreground/elements/files_safe_img.js (right):

https://codereview.chromium.org/2140113003/diff/120001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/elements/files_safe_img.js:30:
this.webview_ = null;
Please add a comment here too, which explains that we remove webview node to
clean up unnecessary processes.

https://codereview.chromium.org/2140113003/diff/120001/ui/file_manager/file_m...
File ui/file_manager/file_manager/foreground/js/quick_view_controller.js
(right):

https://codereview.chromium.org/2140113003/diff/120001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/quick_view_controller.js:254:
}.bind(this));
.bind(this) is unnecessary.

https://codereview.chromium.org/2140113003/diff/120001/ui/file_manager/file_m...
File ui/file_manager/file_manager/manifest.json (right):

https://codereview.chromium.org/2140113003/diff/120001/ui/file_manager/file_m...
ui/file_manager/file_manager/manifest.json:52: "partitions": [{
indent width should be 2 spaces.

[fukino, 2016-07-26 06:39:32]

lgtm with nits

[oka, 2016-07-26 07:54:04]

The CQ bit was checked by oka@chromium.org

[oka, 2016-07-26 07:54:04]

The patchset sent to the CQ was uploaded after l-g-t-m from fukino@chromium.org
Link to the patchset: https://codereview.chromium.org/2140113003/#ps140001
(title: "Address comments.")

[commit-bot: I haz the power, 2016-07-26 07:54:23]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-26 08:00:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-26 08:00:43]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[oka, 2016-07-26 10:08:03]

The CQ bit was checked by oka@chromium.org

[commit-bot: I haz the power, 2016-07-26 10:08:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-26 10:13:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-26 10:13:29]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[oka, 2016-07-26 10:19:02]

oka@chromium.org changed reviewers:
+ thestig@chromium.org

[oka, 2016-07-26 10:19:03]

thestig@
PTAL for resource_ids.

[oka, 2016-07-26 12:28:02]

Description was changed from

==========
QuickView: Improve security by rendering images inside webview.

It addresses security concern raised on
https://bugs.chromium.org/p/chromium/issues/detail?id=614228#c21 for images.
Fixes for audio and video will be sent as a separate CL.

BUG=627698
TEST=manually
third_party/closure_compiler/run_compiler
==========

to

==========
QuickView: Improve security by rendering images inside webview.

It addresses security concern raised on
https://bugs.chromium.org/p/chromium/issues/detail?id=614228#c21 for images.
Fixes for audio and video will be sent as a separate CL.

BUG=614228, 627698
TEST=manually
third_party/closure_compiler/run_compiler
==========

[Lei Zhang, 2016-07-26 17:40:44]

lgtm but next time file_manager needs more resource ids, we'll need to do more
shuffling of the number space.

[Lei Zhang, 2016-07-26 23:13:38]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2016-07-26 23:14:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-26 23:19:08]

Description was changed from

==========
QuickView: Improve security by rendering images inside webview.

It addresses security concern raised on
https://bugs.chromium.org/p/chromium/issues/detail?id=614228#c21 for images.
Fixes for audio and video will be sent as a separate CL.

BUG=614228, 627698
TEST=manually
third_party/closure_compiler/run_compiler
==========

to

==========
QuickView: Improve security by rendering images inside webview.

It addresses security concern raised on
https://bugs.chromium.org/p/chromium/issues/detail?id=614228#c21 for images.
Fixes for audio and video will be sent as a separate CL.

BUG=614228, 627698
TEST=manually
third_party/closure_compiler/run_compiler
==========

[commit-bot: I haz the power, 2016-07-26 23:19:10]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-07-26 23:21:28]

Description was changed from

==========
QuickView: Improve security by rendering images inside webview.

It addresses security concern raised on
https://bugs.chromium.org/p/chromium/issues/detail?id=614228#c21 for images.
Fixes for audio and video will be sent as a separate CL.

BUG=614228, 627698
TEST=manually
third_party/closure_compiler/run_compiler
==========

to

==========
QuickView: Improve security by rendering images inside webview.

It addresses security concern raised on
https://bugs.chromium.org/p/chromium/issues/detail?id=614228#c21 for images.
Fixes for audio and video will be sent as a separate CL.

BUG=614228, 627698
TEST=manually
third_party/closure_compiler/run_compiler

Committed: https://crrev.com/70d5586287eca66e4b7a3b4811ac091f74b0ebf9
Cr-Commit-Position: refs/heads/master@{#407960}
==========

[commit-bot: I haz the power, 2016-07-26 23:21:29]

Patchset 8 (id:??) landed as
https://crrev.com/70d5586287eca66e4b7a3b4811ac091f74b0ebf9
Cr-Commit-Position: refs/heads/master@{#407960}

[oka, 2016-07-27 07:23:34]

On 2016/07/26 17:40:44, Lei Zhang (Very Slow) wrote:
> lgtm but next time file_manager needs more resource ids, we'll need to do more
> shuffling of the number space.

Thank you. Yes, will do. File manager typically consumes 3 (.js, .html, .css)
additional resource IDs for each new custom polymer element.
I am going to shuffle resources ids to make space of say, 30 for our file
manager.