Closed Bug 255931 Opened 18 years ago Closed 18 years ago

favicons should validate content type before saving data

Categories

(Firefox :: Bookmarks & History, defect)

1.0 Branch
x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: vlad, Assigned: vlad)

Details

Attachments

(1 file)

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.
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
Closed: 18 years ago
Resolution: --- → FIXED
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.
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 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)'.
(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.