Don't use SVG resource documents with an unrecognized MIME-type

Don't use SVG resource documents with an unrecognized MIME-type

Before parsing/creating the actual document of a DocumentResource, make
sure that the resource in question was actually served as a reasonable
MIME-type - one of:

  image/svg+xml,
  text/xml,
  application/xml or
  application/xhtml+xml

Use the original Content-Type from the HTTP header when possible and
treat empty as invalid (matches Gecko).
This could help mitigate some issues with content sanitation. It seems
to match what Gecko is doing so is hopefully not too web-incompatible.

Move the commonly recurring *Resource::mimeType() helper from subclasses
to the baseclass (Resource) and rename it httpContentType() since that
should be a better match for what it is.

BUG=527514
Committed: https://crrev.com/57642dc1f2d31bcade1c741f0e7a119ca9754537
Cr-Commit-Position: refs/heads/master@{#377560}

[fs, 2016-02-18 17:04:10]

fs@opera.com changed reviewers:
+ japhet@chromium.org, pdr@chromium.org, schenney@chromium.org

[Stephen Chennney, 2016-02-18 17:18:22]

lgtm for the portions I own.

[fs, 2016-02-18 23:16:12]

Description was changed from

==========
Don't load SVG resource documents with an unrecognized MIME-type

Before parsing/creating the actual document of a DocumentResource, make
sure that the resource in question was actually served as a reasonable
MIME-type - one of:

  image/svg+xml,
  text/xml,
  application/xml or
  application/xhtml+xml

Use the original Content-Type from the HTTP header when possible and
treat empty as invalid (matches Gecko).
This could help mitigate some issues with content sanitation. It seems
to match what Gecko is doing so is hopefully not too web-incompatible.

Move the commonly recurring *Resource::mimeType() helper from subclasses
to the baseclass (Resource) and rename it httpContentType() since that
should be a better match for what it is.

BUG=527514
==========

to

==========
Don't use SVG resource documents with an unrecognized MIME-type

Before parsing/creating the actual document of a DocumentResource, make
sure that the resource in question was actually served as a reasonable
MIME-type - one of:

  image/svg+xml,
  text/xml,
  application/xml or
  application/xhtml+xml

Use the original Content-Type from the HTTP header when possible and
treat empty as invalid (matches Gecko).
This could help mitigate some issues with content sanitation. It seems
to match what Gecko is doing so is hopefully not too web-incompatible.

Move the commonly recurring *Resource::mimeType() helper from subclasses
to the baseclass (Resource) and rename it httpContentType() since that
should be a better match for what it is.

BUG=527514
==========

[fs, 2016-02-19 16:41:17]

japhet, could you take a look?

[fs, 2016-02-22 15:10:02]

On 2016/02/19 at 16:41:17, fs wrote:
> japhet, could you take a look?

japhet, ping

[fs, 2016-02-23 11:47:15]

fs@opera.com changed reviewers:
+ mkwst@chromium.org

[fs, 2016-02-23 11:47:15]

Mike, could you PTAL?

[fs, 2016-02-24 12:11:11]

japhet/mkwst, ping-TAL

[Mike West, 2016-02-24 13:23:44]

https://codereview.chromium.org/1706243002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/http/tests/svg/use-contenttype-blocked.html
(right):

https://codereview.chromium.org/1706243002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/svg/use-contenttype-blocked.html:13:
onload="finishTest()" onerror="finishTest()"
Does `onerror` fire?

It seems like you could get a more complete test that would also work in other
browsers if you turned both of these into `testharness.js`-based things with
expectations around the events that fire, and inspection of the expected DOM.

https://codereview.chromium.org/1706243002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/1706243002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/Resource.cpp:324: return
extractMIMETypeFromMediaType(m_response.httpHeaderField(HTTPNames::Content_Type)).lower();
Would you mind adding some unit tests for this method (maybe in
`HTTPParserTest.cpp`)? It doesn't look like we're actually testing the result of
extractMIMETypeFromMediaType anywhere (sorry!).

Also, we should probably call `lower()` on the header value rather than the
result of the extraction so that we don't construct more AtomicStrings than we
need to. I also wonder whether the extraction function should be taking care of
lowering for us.

[fs, 2016-02-24 17:05:00]

https://codereview.chromium.org/1706243002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/http/tests/svg/use-contenttype-blocked.html
(right):

https://codereview.chromium.org/1706243002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/svg/use-contenttype-blocked.html:13:
onload="finishTest()" onerror="finishTest()"
On 2016/02/24 at 13:23:43, Mike West wrote:
> Does `onerror` fire?

Yes...

> It seems like you could get a more complete test that would also work in other
browsers if you turned both of these into `testharness.js`-based things with
expectations around the events that fire, and inspection of the expected DOM.

...but only in Blink (and presumably WebKit) AFAIK. Converted to a testharness
test though. Note though that there's no "browser portable" way to inspect the
DOM (which is why I initially opted for a ref-test)

https://codereview.chromium.org/1706243002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/1706243002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/Resource.cpp:324: return
extractMIMETypeFromMediaType(m_response.httpHeaderField(HTTPNames::Content_Type)).lower();
On 2016/02/24 at 13:23:44, Mike West wrote:
> Would you mind adding some unit tests for this method (maybe in
`HTTPParserTest.cpp`)? It doesn't look like we're actually testing the result of
extractMIMETypeFromMediaType anywhere (sorry!).

Not sure if there was anything in particular you wanted me to test, so I just
did a "monkey read code, monkey write unit-test" routine.

> Also, we should probably call `lower()` on the header value rather than the
result of the extraction so that we don't construct more AtomicStrings than we
need to.

Done. (Since it's "AtomicString in, AtomicString out" I'm not sure it makes much
of a difference though?)

> I also wonder whether the extraction function should be taking care of
lowering for us.

I guess it could make sense - looking at the current users the majority either
does .lower() (before or after) or use equalIgnoringCase. I only see one of the
uses that doesn't (NetworkResourcesData::responseReceived), but it probably
wouldn't hurt there either. I can do that as a follow-up if you like.

[Mike West, 2016-02-25 12:04:14]

LGTM, thank you.

On 2016/02/24 at 17:05:00, fs wrote:
>
https://codereview.chromium.org/1706243002/diff/1/third_party/WebKit/LayoutTe...
> File
third_party/WebKit/LayoutTests/http/tests/svg/use-contenttype-blocked.html
(right):
> 
>
https://codereview.chromium.org/1706243002/diff/1/third_party/WebKit/LayoutTe...
> third_party/WebKit/LayoutTests/http/tests/svg/use-contenttype-blocked.html:13:
onload="finishTest()" onerror="finishTest()"
> On 2016/02/24 at 13:23:43, Mike West wrote:
> > Does `onerror` fire?
> 
> Yes...
> 
> > It seems like you could get a more complete test that would also work in
other browsers if you turned both of these into `testharness.js`-based things
with expectations around the events that fire, and inspection of the expected
DOM.
> 
> ...but only in Blink (and presumably WebKit) AFAIK. Converted to a testharness
test though. Note though that there's no "browser portable" way to inspect the
DOM (which is why I initially opted for a ref-test)

Interesting. Is it supposed to fire?

>
https://codereview.chromium.org/1706243002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/fetch/Resource.cpp (right):
> 
>
https://codereview.chromium.org/1706243002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/fetch/Resource.cpp:324: return
extractMIMETypeFromMediaType(m_response.httpHeaderField(HTTPNames::Content_Type)).lower();
> On 2016/02/24 at 13:23:44, Mike West wrote:
> > Would you mind adding some unit tests for this method (maybe in
`HTTPParserTest.cpp`)? It doesn't look like we're actually testing the result of
extractMIMETypeFromMediaType anywhere (sorry!).
> 
> Not sure if there was anything in particular you wanted me to test, so I just
did a "monkey read code, monkey write unit-test" routine.

Looks great, thanks!


> > Also, we should probably call `lower()` on the header value rather than the
result of the extraction so that we don't construct more AtomicStrings than we
need to.
> 
> Done. (Since it's "AtomicString in, AtomicString out" I'm not sure it makes
much of a difference though?)

:( You're right. I misread ResourceResponse. Either way is fine, it won't make
any difference. Sorry!

> > I also wonder whether the extraction function should be taking care of
lowering for us.
> 
> I guess it could make sense - looking at the current users the majority either
does .lower() (before or after) or use equalIgnoringCase. I only see one of the
uses that doesn't (NetworkResourcesData::responseReceived), but it probably
wouldn't hurt there either. I can do that as a follow-up if you like.

I don't think it's terribly important, but it would be nice to centralize it if
we're relying on it everywhere. Certainly not blocking this CL.

[fs, 2016-02-25 12:27:23]

On 2016/02/25 at 12:04:14, mkwst wrote:
...
> On 2016/02/24 at 17:05:00, fs wrote:
> >
https://codereview.chromium.org/1706243002/diff/1/third_party/WebKit/LayoutTe...
> > File
third_party/WebKit/LayoutTests/http/tests/svg/use-contenttype-blocked.html
(right):
> > 
> >
https://codereview.chromium.org/1706243002/diff/1/third_party/WebKit/LayoutTe...
> >
third_party/WebKit/LayoutTests/http/tests/svg/use-contenttype-blocked.html:13:
onload="finishTest()" onerror="finishTest()"
> > On 2016/02/24 at 13:23:43, Mike West wrote:
> > > Does `onerror` fire?
> > 
> > Yes...
> > 
> > > It seems like you could get a more complete test that would also work in
other browsers if you turned both of these into `testharness.js`-based things
with expectations around the events that fire, and inspection of the expected
DOM.
> > 
> > ...but only in Blink (and presumably WebKit) AFAIK. Converted to a
testharness test though. Note though that there's no "browser portable" way to
inspect the DOM (which is why I initially opted for a ref-test)
> 
> Interesting. Is it supposed to fire?

AFAIK/CR it's not really specified (the spec around external <use> is pretty,
well, purposely vague). It's something Blink/WebKit has done for a while though
I think. It's not obvious if/that it's very useful, but it would need a
removal-dance to get rid of regardless.

[fs, 2016-02-25 12:38:32]

The CQ bit was checked by fs@opera.com

[fs, 2016-02-25 12:38:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
schenney@chromium.org
Link to the patchset: https://codereview.chromium.org/1706243002/#ps20001
(title: "Use testharness; add unit test for extractMIMETypeFromMediaType")

[commit-bot: I haz the power, 2016-02-25 12:39:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1706243002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1706243002/20001

[commit-bot: I haz the power, 2016-02-25 12:47:05]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-02-25 12:48:25]

Description was changed from

==========
Don't use SVG resource documents with an unrecognized MIME-type

Before parsing/creating the actual document of a DocumentResource, make
sure that the resource in question was actually served as a reasonable
MIME-type - one of:

  image/svg+xml,
  text/xml,
  application/xml or
  application/xhtml+xml

Use the original Content-Type from the HTTP header when possible and
treat empty as invalid (matches Gecko).
This could help mitigate some issues with content sanitation. It seems
to match what Gecko is doing so is hopefully not too web-incompatible.

Move the commonly recurring *Resource::mimeType() helper from subclasses
to the baseclass (Resource) and rename it httpContentType() since that
should be a better match for what it is.

BUG=527514
==========

to

==========
Don't use SVG resource documents with an unrecognized MIME-type

Before parsing/creating the actual document of a DocumentResource, make
sure that the resource in question was actually served as a reasonable
MIME-type - one of:

  image/svg+xml,
  text/xml,
  application/xml or
  application/xhtml+xml

Use the original Content-Type from the HTTP header when possible and
treat empty as invalid (matches Gecko).
This could help mitigate some issues with content sanitation. It seems
to match what Gecko is doing so is hopefully not too web-incompatible.

Move the commonly recurring *Resource::mimeType() helper from subclasses
to the baseclass (Resource) and rename it httpContentType() since that
should be a better match for what it is.

BUG=527514

Committed: https://crrev.com/57642dc1f2d31bcade1c741f0e7a119ca9754537
Cr-Commit-Position: refs/heads/master@{#377560}
==========

[commit-bot: I haz the power, 2016-02-25 12:48:27]

Patchset 2 (id:??) landed as
https://crrev.com/57642dc1f2d31bcade1c741f0e7a119ca9754537
Cr-Commit-Position: refs/heads/master@{#377560}