Remove attributes that contain javascript from MHTML

Remove attributes that contain javascript from MHTML

We remove the following attributes that could contain javascript since they will not work from fully sandboxed MHTML loading:
1) Any event handler attribute
2) Any attribute that can contain a URI will be executed as Javascript
3) Any attribute of SVG elements that can contain Javascript

BUG=669325
TEST=new tests added

Committed: https://crrev.com/52a4d5c0fa88e881003e2f352c4de4f294257529
Cr-Commit-Position: refs/heads/master@{#435811}

[jianli, 2016-11-29 02:00:10]

Description was changed from

==========
Remove attributes that contain javascript from MHTML

BUG=669325
TEST=new tests added
==========

to

==========
Remove attributes that contain javascript from MHTML

We remove the following attributes that could contain javascript since they will
not work from fully sandboxed MHTML loading:
1) Any event handler attribute
2) Any attribute that can contain a URI will be executed as Javascript

BUG=669325
TEST=new tests added
==========

[jianli, 2016-11-29 02:03:48]

jianli@chromium.org changed reviewers:
+ carlosk@chromium.org, dcheng@chromium.org, lukasza@chromium.org

[jianli, 2016-11-29 02:04:24]

The CQ bit was checked by jianli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-29 02:05:08]

Dry run: 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-11-29 04:02:56]

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

[commit-bot: I haz the power, 2016-11-29 04:02:57]

Dry run: This issue passed the CQ dry run.

[Łukasz Anforowicz, 2016-11-29 18:59:55]

Overall change LGTM, but I think we might want to be careful about the new API
of blink::Element.  OTOH, this is something that just probably need a few more
iterations and can be taken care during review by Blink owners (dcheng@? :-).

https://codereview.chromium.org/2531163004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/Element.h (right):

https://codereview.chromium.org/2531163004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Element.h:479: // Returns true is the given
attribute is an event handler.
typo: s/is/if/

https://codereview.chromium.org/2531163004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Element.h:483: static inline bool
isEventHandlerAttribute(const Attribute& attribute) {
Shouldn't this be a method of Attribute?  (Law of Demeter)

I understand that you are just moving old code from Element.cpp, but the move
exposes what used to be an implementation detail of Element.cpp as a public API
of blink::Element class.  So, I think we might want to reconsider where this API
belongs.  WDYT?

https://codereview.chromium.org/2531163004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Element.h:484: return
attribute.name().namespaceURI().isNull() &&
Will this work when attributes are qualified with something like a html4
namespace (i.e. http://www.w3.org/TR/html4/) in html or xhtml?

I see that you are just moving old code from Element.cpp, but I am still curious
how this works in presence of explicit namespaces :-)

https://codereview.chromium.org/2531163004/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebFrameSerializer.cpp (right):

https://codereview.chromium.org/2531163004/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebFrameSerializer.cpp:107: // Do not include
attributes that can contain javascript:
nit: Could you please expand the comment to explain that we do this because
opening a mhtml won't execute any javascript for security reasons, and so we can
save disk space by stripping the unnecessary attributes.

https://codereview.chromium.org/2531163004/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebFrameSerializer.cpp:109: // 2) Any attribute
that can contain a URL will be executed as Javascript.
nit: I am not sure if the 2 comment lines above are useful - they just repeat
what the code below says.  I would consider removing them.  WDYT?

[dcheng, 2016-11-29 21:11:48]

dcheng@chromium.org changed reviewers:
+ tkent@chromium.org

[dcheng, 2016-11-29 21:11:49]

+tkent, would you mind doing the Blink review for this?

[jianli, 2016-11-30 00:46:26]

https://codereview.chromium.org/2531163004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/Element.h (right):

https://codereview.chromium.org/2531163004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Element.h:479: // Returns true is the given
attribute is an event handler.
On 2016/11/29 18:59:55, Łukasz Anforowicz wrote:
> typo: s/is/if/

Done.

https://codereview.chromium.org/2531163004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Element.h:483: static inline bool
isEventHandlerAttribute(const Attribute& attribute) {
On 2016/11/29 18:59:55, Łukasz Anforowicz wrote:
> Shouldn't this be a method of Attribute?  (Law of Demeter)
> 
> I understand that you are just moving old code from Element.cpp, but the move
> exposes what used to be an implementation detail of Element.cpp as a public
API
> of blink::Element class.  So, I think we might want to reconsider where this
API
> belongs.  WDYT?

Found out we have more cases to cover. Indeed the check implemented in
Element::stripScriptingAttributes is the one we need. So I move the checking
logic out to become a separate function.

https://codereview.chromium.org/2531163004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Element.h:484: return
attribute.name().namespaceURI().isNull() &&
On 2016/11/29 18:59:55, Łukasz Anforowicz wrote:
> Will this work when attributes are qualified with something like a html4
> namespace (i.e. http://www.w3.org/TR/html4/) in html or xhtml?
> 
> I see that you are just moving old code from Element.cpp, but I am still
curious
> how this works in presence of explicit namespaces :-)

I am not an expert on this. Could tkent comment on this? Thanks.

https://codereview.chromium.org/2531163004/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebFrameSerializer.cpp (right):

https://codereview.chromium.org/2531163004/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebFrameSerializer.cpp:107: // Do not include
attributes that can contain javascript:
On 2016/11/29 18:59:55, Łukasz Anforowicz wrote:
> nit: Could you please expand the comment to explain that we do this because
> opening a mhtml won't execute any javascript for security reasons, and so we
can
> save disk space by stripping the unnecessary attributes.

Done.

https://codereview.chromium.org/2531163004/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebFrameSerializer.cpp:109: // 2) Any attribute
that can contain a URL will be executed as Javascript.
On 2016/11/29 18:59:55, Łukasz Anforowicz wrote:
> nit: I am not sure if the 2 comment lines above are useful - they just repeat
> what the code below says.  I would consider removing them.  WDYT?

Done.

[jianli, 2016-11-30 00:46:31]

Patchset #2 (id:20001) has been deleted

[jianli, 2016-11-30 00:47:00]

The CQ bit was checked by jianli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-30 00:48:10]

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

[carlosk, 2016-11-30 01:03:16]

Indeed this new version does look better. LGTM with one observation.

https://codereview.chromium.org/2531163004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/WebFrameSerializerTest.cpp (right):

https://codereview.chromium.org/2531163004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/WebFrameSerializerTest.cpp:209:
EXPECT_EQ(std::string::npos, mhtml.find("javascript:"));
I think you should also test a few positive cases here, where attributes are
expected to not be removed. There might be some "edgy" cases you are aware of?

Currently if SimpleMHTMLPartsGenerationDelegate should override
shouldIgnoreAttribute to always return |false| this test would succeed.

[jianli, 2016-11-30 01:35:37]

https://codereview.chromium.org/2531163004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/WebFrameSerializerTest.cpp (right):

https://codereview.chromium.org/2531163004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/WebFrameSerializerTest.cpp:209:
EXPECT_EQ(std::string::npos, mhtml.find("javascript:"));
On 2016/11/30 01:03:16, carlosk wrote:
> I think you should also test a few positive cases here, where attributes are
> expected to not be removed. There might be some "edgy" cases you are aware of?
> 
> Currently if SimpleMHTMLPartsGenerationDelegate should override
> shouldIgnoreAttribute to always return |false| this test would succeed.

Done.

[jianli, 2016-11-30 01:36:04]

The CQ bit was checked by jianli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-30 01:37:13]

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

[jianli, 2016-11-30 02:09:58]

Description was changed from

==========
Remove attributes that contain javascript from MHTML

We remove the following attributes that could contain javascript since they will
not work from fully sandboxed MHTML loading:
1) Any event handler attribute
2) Any attribute that can contain a URI will be executed as Javascript

BUG=669325
TEST=new tests added
==========

to

==========
Remove attributes that contain javascript from MHTML

We remove the following attributes that could contain javascript since they will
not work from fully sandboxed MHTML loading:
1) Any event handler attribute
2) Any attribute that can contain a URI will be executed as Javascript
3) Any attribute of SVG elements that can contain Javascript

BUG=669325
TEST=new tests added
==========

[commit-bot: I haz the power, 2016-11-30 03:04:36]

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

[commit-bot: I haz the power, 2016-11-30 03:04:37]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[tkent, 2016-11-30 08:10:17]

The code change looks ok, however does this mean save-page-as-complete-html and
save-page-as-mhtml have different user-visible behaviors?

https://codereview.chromium.org/2531163004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/Element.h (right):

https://codereview.chromium.org/2531163004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Element.h:484: return
attribute.name().namespaceURI().isNull() &&
On 2016/11/30 at 00:46:26, jianli wrote:
> On 2016/11/29 18:59:55, Łukasz Anforowicz wrote:
> > Will this work when attributes are qualified with something like a html4
> > namespace (i.e. http://www.w3.org/TR/html4/) in html or xhtml?
> > 
> > I see that you are just moving old code from Element.cpp, but I am still
curious
> > how this works in presence of explicit namespaces :-)
> 
> I am not an expert on this. Could tkent comment on this? Thanks.

All of HTML attributes have no namespace.  An attribute without a namespace and
an attribute with a namespace must be handled as different.

https://codereview.chromium.org/2531163004/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/web/tests/data/frameserialization/script_in_attributes.html
(right):

https://codereview.chromium.org/2531163004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/data/frameserialization/script_in_attributes.html:3:
<body onload="foo();">
Please add a test for upper-case attributes.  e.g. <body ONLOAD="..">

[jianli, 2016-12-01 01:52:35]

save-page-as-complete-html and
save-page-as-mhtml are indeed treated differently in loading. The former one
allows script execution while the latter does not allow that. So the visual
experience could be different.

I also added one more custom logic in
MHTMLFrameSerializerDelegate::shouldIgnoreAttribute to allow srcdoc attribute if
it can be written by MHTML serializer logic.

https://codereview.chromium.org/2531163004/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/web/tests/data/frameserialization/script_in_attributes.html
(right):

https://codereview.chromium.org/2531163004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/data/frameserialization/script_in_attributes.html:3:
<body onload="foo();">
On 2016/11/30 08:10:17, tkent wrote:
> Please add a test for upper-case attributes.  e.g. <body ONLOAD="..">

Done.

[jianli, 2016-12-01 01:52:40]

The CQ bit was checked by jianli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 01:52:54]

Dry run: 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-12-01 03:29:10]

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

[commit-bot: I haz the power, 2016-12-01 03:29:10]

Dry run: This issue passed the CQ dry run.

[tkent, 2016-12-01 14:53:10]

I'm not sure if the behavior change is good for users, but anyway the code
change LGTM.

https://codereview.chromium.org/2531163004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/WebFrameSerializerTest.cpp (right):

https://codereview.chromium.org/2531163004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/WebFrameSerializerTest.cpp:206:
EXPECT_EQ(std::string::npos, mhtml.find("onload="));
The return value of String::find() is WTF::kNotFound, not std::string::npos.

[jianli, 2016-12-01 23:03:56]

https://codereview.chromium.org/2531163004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/WebFrameSerializerTest.cpp (right):

https://codereview.chromium.org/2531163004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/WebFrameSerializerTest.cpp:206:
EXPECT_EQ(std::string::npos, mhtml.find("onload="));
On 2016/12/01 14:53:10, tkent wrote:
> The return value of String::find() is WTF::kNotFound, not std::string::npos.

Done.

[jianli, 2016-12-01 23:04:09]

The CQ bit was checked by jianli@chromium.org

[jianli, 2016-12-01 23:04:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
lukasza@chromium.org, carlosk@chromium.org, tkent@chromium.org
Link to the patchset: https://codereview.chromium.org/2531163004/#ps100001
(title: "Address final feedback")

[commit-bot: I haz the power, 2016-12-01 23:04:34]

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

[jianli, 2016-12-01 23:59:24]

On 2016/12/01 14:53:10, tkent wrote:
> I'm not sure if the behavior change is good for users, but anyway the code
> change LGTM.
> 
>
https://codereview.chromium.org/2531163004/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/web/tests/WebFrameSerializerTest.cpp (right):
> 
>
https://codereview.chromium.org/2531163004/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/web/tests/WebFrameSerializerTest.cpp:206:
> EXPECT_EQ(std::string::npos, mhtml.find("onload="));
> The return value of String::find() is WTF::kNotFound, not std::string::npos.

MHTML archive pages are loaded in fully sandbox mode, including script execution
disabling, for security reason. Due to that, we're now also removing those
attributes/elements that do not bring any visual effect since they are not going
to be changed dynamically. So all these changes should be good for users'
privacy and security.

[commit-bot: I haz the power, 2016-12-02 01:38:24]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1480633449304060,
"parent_rev": "a72b17e7716ee48e767d7736d60a1e3e9137953f", "commit_rev":
"74d909b706084ef10c18d883190772053a527b99"}

[commit-bot: I haz the power, 2016-12-02 01:38:50]

Description was changed from

==========
Remove attributes that contain javascript from MHTML

We remove the following attributes that could contain javascript since they will
not work from fully sandboxed MHTML loading:
1) Any event handler attribute
2) Any attribute that can contain a URI will be executed as Javascript
3) Any attribute of SVG elements that can contain Javascript

BUG=669325
TEST=new tests added
==========

to

==========
Remove attributes that contain javascript from MHTML

We remove the following attributes that could contain javascript since they will
not work from fully sandboxed MHTML loading:
1) Any event handler attribute
2) Any attribute that can contain a URI will be executed as Javascript
3) Any attribute of SVG elements that can contain Javascript

BUG=669325
TEST=new tests added
==========

[commit-bot: I haz the power, 2016-12-02 01:38:51]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-12-02 01:42:09]

Description was changed from

==========
Remove attributes that contain javascript from MHTML

We remove the following attributes that could contain javascript since they will
not work from fully sandboxed MHTML loading:
1) Any event handler attribute
2) Any attribute that can contain a URI will be executed as Javascript
3) Any attribute of SVG elements that can contain Javascript

BUG=669325
TEST=new tests added
==========

to

==========
Remove attributes that contain javascript from MHTML

We remove the following attributes that could contain javascript since they will
not work from fully sandboxed MHTML loading:
1) Any event handler attribute
2) Any attribute that can contain a URI will be executed as Javascript
3) Any attribute of SVG elements that can contain Javascript

BUG=669325
TEST=new tests added

Committed: https://crrev.com/52a4d5c0fa88e881003e2f352c4de4f294257529
Cr-Commit-Position: refs/heads/master@{#435811}
==========

[commit-bot: I haz the power, 2016-12-02 01:42:10]

Patchset 5 (id:??) landed as
https://crrev.com/52a4d5c0fa88e881003e2f352c4de4f294257529
Cr-Commit-Position: refs/heads/master@{#435811}