Skip to content
This repository has been archived by the owner on May 10, 2024. It is now read-only.

Harden favicon loading logic to fix crash when loading certain web pages #3868

Closed
kylehickinson opened this issue Jun 29, 2021 · 1 comment · Fixed by #3874
Closed

Harden favicon loading logic to fix crash when loading certain web pages #3868

kylehickinson opened this issue Jun 29, 2021 · 1 comment · Fixed by #3874

Comments

@kylehickinson
Copy link
Collaborator

kylehickinson commented Jun 29, 2021

Description:

Currently the favicon loaders can load PDF files if the URL response when loading /favicon.ico actually returns pdf data. This is a result of SDWebImage being able to decode it and return an "image" as a result instead of throwing an error.

There are multiple points of hardening that should happen:

  • Update metadata retrieval to append favicon.ico to the base domain and not the URL. In the case of the example PDF, the url ends up being https://static1.squarespace.com/static/57338e2c04426238cff175c6/t/60c9462f924a473dc95dc850/1623803439283/ListaDeVinos...pdf/favicon.ico which is incorrect, it should have been attempting to fetch https://static1.squarespace.com/favicon.ico
  • Update favicon loader to reject PDF's as an acceptable favicon image. (Combination of rejecting Content-Type: application/pdf response headers and possibly file header byte checking)
  • Impose favicon size limitation when inserting into Favicon DB table
  • Purge any Database records of favicons who's sizes exceed 1024x1024 to ensure any users who have already cached a large image or non-image be removed and re-downloaded correctly

Steps to Reproduce

  1. Visit https://static1.squarespace.com/static/57338e2c04426238cff175c6/t/60c9462f924a473dc95dc850/1623803439283/ListaDeVinos...pdf
  2. If it doesn't crash immediately (due to your device having sufficient memory), tap on tab tray button

Actual result: Crashes
Expected result: Shouldn't crash
Reproduces how often: Always
Brave Version: Live (1.27.1)
Device details: Depending on the devices memory limitations it may crash while downloading the favicon or when displaying it

@srirambv
Copy link
Contributor

Verification passed on iPhone XR with iOS 13.5 running 1.29(21.07.20.16)

  • Verified steps from issue description
  • Verified no browser crash when opening the page or opening tab tray
  • Verified on both normal and private tabs

Verification passed on iPhone 7+ with iOS 14.5.1 running 1.29(21.07.20.16)

  • Verified steps from issue description
  • Verified no browser crash when opening the page or opening tab tray
  • Verified on both normal and private tabs

Verification passed on iPad Pro with iOS 14.6 running 1.29(21.07.20.16)

  • Verified steps from issue description
  • Verified no browser crash when opening the page or opening tab tray
  • Verified on both normal and private tabs

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.