favicons should validate content type before saving data

RESOLVED FIXED

Status

()

RESOLVED FIXED
15 years ago
12 years ago

People

(Reporter: vlad, Assigned: vlad)

Tracking

1.0 Branch
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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.
Created attachment 156379 [details] [diff] [review]
favicon-sniffing-0.patch

patch to add sniffing support.
Status: NEW → ASSIGNED
Comment on attachment 156379 [details] [diff] [review]
favicon-sniffing-0.patch

a=ben@mozilla.org
Attachment #156379 - Flags: approval-aviary? → approval-aviary+
in branch/trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 4

15 years ago
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

14 years ago
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

14 years ago
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

14 years ago
(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. 


sorry for bugspam, long-overdue mass reassign of ancient QA contact bugs, filter on "beltznerLovesGoats" to get rid of this mass change
QA Contact: mconnor → bookmarks
You need to log in before you can comment on or make changes to this bug.