255931 - favicons should validate content type before saving data

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Favicons should sniff the content type (using bug 247981 stuff) before saving
the icon to bookmarks.html, to make sure we don't save invalid non-image data as
an icon.

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: favicon-sniffing-0.patch

patch to add sniffing support.

Comment 2

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 156379 [details] [diff] [review]
favicon-sniffing-0.patch

a=[e-mail]

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

in branch/trunk.

Comment 4

[Jeremy M. Dolan]

It looks like your changes to nsBookmarksService::UpdateBookmarkIcon also fixes
the issue of the wrong MIME type being saved in bookmarks.html (eg, text/plain).
See: http://bugzilla.mozilla.org/show_bug.cgi?id=174265#c85

Not sure if a seperate bug on the issue was filed elsewhere. Will try to verify
this soon.

Comment 5

[Jeremy M. Dolan]

With todays builds, I can verify the issue I mentioned previously is now fixed.
When we receive a BMP icon sent as text/plain, it is now listed in the bookmarks
store with the proper type, image/x-icon.

However, I noticed a seperate issue while testing this. When I initially
bookmark a page, the icon isn't saved. Only after first loading the bookmark is
the ICON= attribute added. If we're bookmarking a page that's currently
displayed, the icon data should be available for saving in the initial bookmarks
update. (and if not, refetech it).

Comment 6

[Jungshik Shin]

Comment on attachment 156379 [details] [diff] [review]
favicon-sniffing-0.patch

>Index: src/nsBookmarksService.cpp

>+        PRInt32 len = ((aIconDataLen + 2) / 3) * 4;  

  This is not necessary at all.

>+        nsString dataUri;
>+        dataUri += NS_LITERAL_STRING("data:");
>+        dataUri += NS_ConvertASCIItoUTF16(aMIMEType);
>+        dataUri += NS_LITERAL_STRING(";base64,");
>+        dataUri += NS_ConvertASCIItoUTF16(iconDataBase64, len);

For the trunk, it'd have been better to use 
AppendLiteral and AppendASCII in place of '+= NS_LITERAL_STRING' and '+=
NS_ConvertASCIItoUTF16'. 

For the branch, 'dataUri += NS_ConvertASCIItoUTF16(,)' should have been
replaced by 'AppendASCIItoUTF16(icondDataBase64)'.

Comment 7

[Jungshik Shin]

(In reply to comment #6)

> For the branch, 'dataUri += NS_ConvertASCIItoUTF16(,)' should have been
> replaced by 'AppendASCIItoUTF16(icondDataBase64)'.

  I meant 'AppendASCIItoUTF16(icondDataBase64, dataUri);' (note PL_base64Encode
? returns a null-terminated string, which is why you don't need to calculate
'len'). The same is true of aMimeType. 

Comment 8

[Mike Connor [:mconnor]]

sorry for bugspam, long-overdue mass reassign of ancient QA contact bugs, filter on "beltznerLovesGoats" to get rid of this mass change