[Regression fix] [Data URI parser] Accept data URI with invalid mediatype data

[Regression fix] [Data URI parser] Accept data URI with invalid mediatype data

r272022 introduced grammar check on the mediatype part in a data URI.
This broke applications that depend on the behavior that our
WebURLLoaderImpl and URLRequestDataJob accept data URIs with
incomplete mediatype such as "data:image;base64,xxx". Accept this kind
of data URI again for backward compatibility.

Instead, fallback to the default mediatype when
ParseMimeTypeWithoutParameter() returns false in DataURL::Parse() to
avoid generating Content-type header with invalid mediatype.

BUG=412479
Committed: https://crrev.com/5469386eaa6bd55fcd5540c570fb535f4d6667e3
Cr-Commit-Position: refs/heads/master@{#297593}

[tyoshino (SeeGerritForStatus), 2014-09-12 23:37:56]

tyoshino@chromium.org changed reviewers:
+ mmenke@chromium.org

[mmenke, 2014-09-15 14:17:50]

I'm not at all familiar with this code.  Could you have someone a bit more
familiar with data URLs sign off on this?  If that ends up being someone who's
not a net owner, I'm happy to sign off afterwards.

[tyoshino (SeeGerritForStatus), 2014-09-15 14:31:15]

tyoshino@chromium.org changed reviewers:
+ asanka@chromium.org, darin@chromium.org

[tyoshino (SeeGerritForStatus), 2014-09-15 14:31:32]

OK. Adding asanka@ and darin@

[tyoshino (SeeGerritForStatus), 2014-09-15 14:34:33]

Before r272022, we were not returning Content-type header to data URI request.
So, we can choose to return no Content-type header when the mediatype portion of
data URI is invalid again.

That said, I guess it's sufficient that we allow token such as
"data:image;base64,xxx".

[tyoshino (SeeGerritForStatus), 2014-09-15 15:53:21]

Even if we conclude that invalid mediatypes should be rejected, I'd like to have
some period of deprecation as there're some apps depending on the current
behavior.

[tyoshino (SeeGerritForStatus), 2014-09-17 05:29:16]

Wrote Blink site test, too: https://codereview.chromium.org/579673002/

[mmenke, 2014-09-17 13:39:15]

mmenke@chromium.org changed reviewers:
- mmenke@chromium.org

[tyoshino (SeeGerritForStatus), 2014-09-18 08:55:21]

Hmm. On the bug, we received another request to make it accept image\png (not
image/png) as we did before.

The only thing I wanted to make sure is to use valid mediatype if we generate
Content-type header. So, I'd like to move ParseMimeTypeWithoutParameter() call
to BuildResponse() method.

[tyoshino (SeeGerritForStatus), 2014-09-18 17:15:43]

Fixed the unittest failed for the patch set 2. Confirmed that data URI with
image\png as mediatype is rendered using the layout test.

Asanka, please take a look. This is a regression fix.

[tyoshino (SeeGerritForStatus), 2014-09-19 16:35:05]

tyoshino@chromium.org changed reviewers:
- darin@chromium.org

[tyoshino (SeeGerritForStatus), 2014-09-22 14:54:47]

On 2014/09/18 17:15:43, tyoshino wrote:
> Fixed the unittest failed for the patch set 2. Confirmed that data URI with
> image\png as mediatype is rendered using the layout test.
> 
> Asanka, please take a look. This is a regression fix.

ping

[asanka, 2014-09-22 20:35:47]

On 2014/09/22 14:54:47, tyoshino wrote:
> On 2014/09/18 17:15:43, tyoshino wrote:
> > Fixed the unittest failed for the patch set 2. Confirmed that data URI with
> > image\png as mediatype is rendered using the layout test.
> > 
> > Asanka, please take a look. This is a regression fix.
> 
> ping

See discussion on bug.

[tyoshino (SeeGerritForStatus), 2014-09-22 22:52:16]

On 2014/09/22 20:35:47, asanka wrote:
> On 2014/09/22 14:54:47, tyoshino wrote:
> > On 2014/09/18 17:15:43, tyoshino wrote:
> > > Fixed the unittest failed for the patch set 2. Confirmed that data URI
with
> > > image\png as mediatype is rendered using the layout test.
> > > 
> > > Asanka, please take a look. This is a regression fix.
> > 
> > ping
> 
> See discussion on bug.

Thanks. Replied on bug.

[asanka, 2014-09-24 00:11:38]

https://codereview.chromium.org/555383003/diff/40001/net/base/data_url.cc
File net/base/data_url.cc (left):

https://codereview.chromium.org/555383003/diff/40001/net/base/data_url.cc#old...
net/base/data_url.cc:71: } else if (!ParseMimeTypeWithoutParameter(*mime_type,
NULL, NULL)) {
As mentioned elsewhere, it's still a bit worrisome that the mime_type is going
to be returned without any validation which places a heavy burden on callers to
do the validation themselves. E.g. someone stuffing the mime type into a set of
headers might inadvertently be open to a header splitting attack. This is partly
why I suggested that we at least treat invalid mediatypes the same as we treat
empty mediatypes.

[tyoshino (SeeGerritForStatus), 2014-09-24 05:43:29]

PTAL

https://codereview.chromium.org/555383003/diff/40001/net/base/data_url.cc
File net/base/data_url.cc (left):

https://codereview.chromium.org/555383003/diff/40001/net/base/data_url.cc#old...
net/base/data_url.cc:71: } else if (!ParseMimeTypeWithoutParameter(*mime_type,
NULL, NULL)) {
On 2014/09/24 00:11:38, asanka wrote:
> As mentioned elsewhere, it's still a bit worrisome that the mime_type is going
> to be returned without any validation which places a heavy burden on callers
to
> do the validation themselves. E.g. someone stuffing the mime type into a set
of
> headers might inadvertently be open to a header splitting attack. This is
partly
> why I suggested that we at least treat invalid mediatypes the same as we treat
> empty mediatypes.

Good point. It's been a while since I added this check. So, there could be some
new code that depends on this check.

OK. I'll make it fall back to the default ("text/plain") when the media type is
invalid instead of returning false.

[tyoshino (SeeGerritForStatus), 2014-09-24 17:12:34]

Updated the comment on DataURL::Parse() as asanka@ suggested on the bug.

[tyoshino (SeeGerritForStatus), 2014-09-29 03:15:01]

ping

[asanka, 2014-09-30 18:35:51]

Sorry about the delay. Just one more compliance requirement and then we should
be good.

https://codereview.chromium.org/555383003/diff/80001/net/base/data_url.cc
File net/base/data_url.cc (right):

https://codereview.chromium.org/555383003/diff/80001/net/base/data_url.cc#new...
net/base/data_url.cc:73: mime_type->assign("text/plain");
If you are setting the mime_type, also set the charset here. We don't want to
accept one but not the other. This will be a slight behavior change for the
empty MIME type case, but is correct according to the spec.

[tyoshino (SeeGerritForStatus), 2014-10-01 01:03:54]

https://codereview.chromium.org/555383003/diff/80001/net/base/data_url.cc
File net/base/data_url.cc (right):

https://codereview.chromium.org/555383003/diff/80001/net/base/data_url.cc#new...
net/base/data_url.cc:73: mime_type->assign("text/plain");
On 2014/09/30 18:35:51, asanka wrote:
> If you are setting the mime_type, also set the charset here. We don't want to
> accept one but not the other. This will be a slight behavior change for the
> empty MIME type case, but is correct according to the spec.

As specified in RFC2397, when the mime type is empty, we still need to parse the
input and use charset parameter in it.

For invalid mime type case, I've added charset->assign("US-ASCII") as you
suggested.

[asanka, 2014-10-01 03:02:05]

lgtm

[tyoshino (SeeGerritForStatus), 2014-10-01 03:07:48]

The CQ bit was checked by tyoshino@chromium.org

[commit-bot: I haz the power, 2014-10-01 03:08:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/555383003/120001

[commit-bot: I haz the power, 2014-10-01 04:01:42]

Committed patchset #7 (id:120001) as 2036de077c45a06dd07a8e1a1d3ab4978ec60611

[commit-bot: I haz the power, 2014-10-01 04:02:24]

Patchset 7 (id:??) landed as
https://crrev.com/5469386eaa6bd55fcd5540c570fb535f4d6667e3
Cr-Commit-Position: refs/heads/master@{#297593}