Bug 216900 - Data URL image is not rendered if there is whitespace between the mime type and and "base64" string
Summary: Data URL image is not rendered if there is whitespace between the mime type a...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: Safari 14
Hardware: All All
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-09-23 14:45 PDT by Jeffrey
Modified: 2020-10-01 08:58 PDT (History)
6 users (show)

See Also:


Attachments
Patch (24.02 KB, patch)
2020-09-28 14:03 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (27.97 KB, patch)
2020-09-28 17:54 PDT, Said Abou-Hallawa
darin: review+
Details | Formatted Diff | Diff
Patch (28.03 KB, patch)
2020-09-28 21:25 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeffrey 2020-09-23 14:45:04 PDT
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
Comment 1 Radar WebKit Bug Importer 2020-09-27 09:57:21 PDT
<rdar://problem/69659841>
Comment 2 Said Abou-Hallawa 2020-09-28 14:03:37 PDT
Created attachment 409919 [details]
Patch
Comment 3 Darin Adler 2020-09-28 16:33:12 PDT
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.
Comment 4 Said Abou-Hallawa 2020-09-28 17:54:15 PDT
Created attachment 409935 [details]
Patch
Comment 5 Said Abou-Hallawa 2020-09-28 17:59:58 PDT
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.
Comment 6 Darin Adler 2020-09-28 18:24:47 PDT
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.
Comment 7 Darin Adler 2020-09-28 18:32:36 PDT
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.
Comment 8 Said Abou-Hallawa 2020-09-28 21:25:25 PDT
Created attachment 409960 [details]
Patch
Comment 9 Said Abou-Hallawa 2020-09-28 22:27:37 PDT
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().
Comment 10 EWS 2020-09-29 00:44:44 PDT
Committed r267730: <https://trac.webkit.org/changeset/267730>

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