RESOLVED FIXED 216900
Data URL image is not rendered if there is whitespace between the mime type and and "base64" string
https://bugs.webkit.org/show_bug.cgi?id=216900
Summary Data URL image is not rendered if there is whitespace between the mime type a...
Jeffrey
Reported 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
Attachments
Patch (24.02 KB, patch)
2020-09-28 14:03 PDT, Said Abou-Hallawa
no flags
Patch (27.97 KB, patch)
2020-09-28 17:54 PDT, Said Abou-Hallawa
darin: review+
Patch (28.03 KB, patch)
2020-09-28 21:25 PDT, Said Abou-Hallawa
no flags
Radar WebKit Bug Importer
Comment 1 2020-09-27 09:57:21 PDT
Said Abou-Hallawa
Comment 2 2020-09-28 14:03:37 PDT
Darin Adler
Comment 3 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.
Said Abou-Hallawa
Comment 4 2020-09-28 17:54:15 PDT
Said Abou-Hallawa
Comment 5 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.
Darin Adler
Comment 6 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.
Darin Adler
Comment 7 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.
Said Abou-Hallawa
Comment 8 2020-09-28 21:25:25 PDT
Said Abou-Hallawa
Comment 9 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().
EWS
Comment 10 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].
Note You need to log in before you can comment on or make changes to this bug.