Bug 216900 – Data URL image is not rendered if there is whitespace between the mime type and and "base64" string

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Jeffrey]

The following does not render on Safari (macOS, iOS), but does on Chrome and Firefox:

<img src="data:image/png; base64, iVBORw0KGgoAAAANSUhEUgAAAAUAAAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO9TXL0Y4OHwAAAABJRU5ErkJggg=="/>

The following renders on Safari:

<img src="data:image/png;base64, iVBORw0KGgoAAAANSUhEUgAAAAUAAAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO9TXL0Y4OHwAAAABJRU5ErkJggg=="/>

<img src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAUAAAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO9TXL0Y4OHwAAAABJRU5ErkJggg=="/>

Codepen: https://codepen.io/JeffreyCA/pen/BaKMNPR

[Radar WebKit Bug Importer]

<rdar://problem/69659841>

[Said Abou-Hallawa]

Created attachment 409919 [details]
Patch

[Darin Adler]

Comment on attachment 409919 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=409919&action=review

> Source/WebCore/platform/network/DataURLDecoder.cpp:72
> +        const char base64String[] = "base64";

This doesn’t need to be a named array constant any more.

> Source/WebCore/platform/network/DataURLDecoder.cpp:83
> +        size_t mediaTypeEnd = header.find(';', 0);

No need for ", 0" here.

> Source/WebCore/platform/network/DataURLDecoder.cpp:86
>          mediaType = mediaType.stripWhiteSpace();

Seems likely this does the wrong thing; this function uses the Unicode definition of white space and also strips internal spaces. Is that really what we want?

> Source/WebCore/platform/network/DataURLDecoder.cpp:95
> +        isBase64 = equalIgnoringASCIICase(formatType, base64String);

Can use equalLettersIgnoringASCIICase(formatType, "base64") here.

[Said Abou-Hallawa]

Created attachment 409935 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 409919 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=409919&action=review

>> Source/WebCore/platform/network/DataURLDecoder.cpp:72
>> +        const char base64String[] = "base64";
> 
> This doesn’t need to be a named array constant any more.

Variable was removed.

>> Source/WebCore/platform/network/DataURLDecoder.cpp:83
>> +        size_t mediaTypeEnd = header.find(';', 0);
> 
> No need for ", 0" here.

", 0" was removed but I had to use reverseFind() because we should look for the last semicolon in the header to find the end of the media type.

>> Source/WebCore/platform/network/DataURLDecoder.cpp:86
>>          mediaType = mediaType.stripWhiteSpace();
> 
> Seems likely this does the wrong thing; this function uses the Unicode definition of white space and also strips internal spaces. Is that really what we want?

I think all we need to do is to remove the leading and the trailing whitespaces. My understanding is stripWhiteSpace() does that. Please advice if my understanding is incorrect.

>> Source/WebCore/platform/network/DataURLDecoder.cpp:95
>> +        isBase64 = equalIgnoringASCIICase(formatType, base64String);
> 
> Can use equalLettersIgnoringASCIICase(formatType, "base64") here.

Done.

[Darin Adler]

Comment on attachment 409919 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=409919&action=review

>>> Source/WebCore/platform/network/DataURLDecoder.cpp:86
>>>          mediaType = mediaType.stripWhiteSpace();
>> 
>> Seems likely this does the wrong thing; this function uses the Unicode definition of white space and also strips internal spaces. Is that really what we want?
> 
> I think all we need to do is to remove the leading and the trailing whitespaces. My understanding is stripWhiteSpace() does that. Please advice if my understanding is incorrect.

It does that, but uses the Unicode definition of whitespace, which includes characters we don’t want to strip. I think we want one of these:

stripLeadingAndTrailingHTMLSpaces
stripLeadingAndTrailingHTTPSpaces

I was wrong to say it "strips internal spaces". I was thinking of "simplifyWhiteSpace". But I was right to highlight that this unnecessarily involves the Unicode definition of whitespace.

[Darin Adler]

Comment on attachment 409935 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=409935&action=review

> Source/WebCore/platform/network/DataURLDecoder.cpp:94
> +        formatType = formatType.stripWhiteSpace();

To show this is wrong, we need tests that involve non-ASCII white space characters such as U+2000, U+0085, or U+00A0. They should not get stripped, but they will get stripped.

[Said Abou-Hallawa]

Created attachment 409960 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 409919 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=409919&action=review

>>>> Source/WebCore/platform/network/DataURLDecoder.cpp:86
>>>>          mediaType = mediaType.stripWhiteSpace();
>>> 
>>> Seems likely this does the wrong thing; this function uses the Unicode definition of white space and also strips internal spaces. Is that really what we want?
>> 
>> I think all we need to do is to remove the leading and the trailing whitespaces. My understanding is stripWhiteSpace() does that. Please advice if my understanding is incorrect.
> 
> It does that, but uses the Unicode definition of whitespace, which includes characters we don’t want to strip. I think we want one of these:
> 
> stripLeadingAndTrailingHTMLSpaces
> stripLeadingAndTrailingHTTPSpaces
> 
> I was wrong to say it "strips internal spaces". I was thinking of "simplifyWhiteSpace". But I was right to highlight that this unnecessarily involves the Unicode definition of whitespace.

I used stripLeadingAndTrailingHTTPSpaces instead of stripWhiteSpace() because I found it is used in ParsedContentType::create().

[EWS]

Committed r267730: <https://trac.webkit.org/changeset/267730>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 409960 [details].