search

Authenticator-Extension / Authenticator

Authenticator generates 2-Step Verification codes in your browser.
https://authenticator.cc
MIT License
3.09k stars 728 forks source link

add favicon support #1118

Open Sneezry opened 8 months ago

Sneezry commented 8 months ago

This PR adds favicon support.

  1. This PR adds favicon support for MV2
  2. Firefox doesn't support to fetch favicon in MV2, and this PR disable this feature for Firefox
  3. When we migrate to MV3, chrome://favicon permission is not needed, and we need to remove it from permission list and CSP.

We read website url from issuer field. The user need to add ::<website-domain> in end of the issuer to show the favicon correctly. If the user doesn't add such an extension in the issuer, a default icon is shown.

image

image

There will be a warning to ask the user to grant extension permission to read the website favicon when enable this feature at the first time.

mymindstorm commented 8 months ago

Just FYI, not going to have time to look at this until around the 28th.

-------- Original message -------- From: Zhe Li @.> Date: 10/17/23 3:01 AM (GMT-06:00) To: Authenticator-Extension/Authenticator @.> Cc: Brendan Early @.>, Review requested @.> Subject: Re: [Authenticator-Extension/Authenticator] add favicon support (PR #1118)

@Sneezryhttps://github.com/Sneezry requested your review on: #1118https://github.com/Authenticator-Extension/Authenticator/pull/1118 add favicon support.

— Reply to this email directly, view it on GitHubhttps://github.com/Authenticator-Extension/Authenticator/pull/1118#event-10674731086, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AGUAT3RAWUFSBGFHDIRN4KDX7Y3N7AVCNFSM6AAAAAA6DMQTWKVHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJQGY3TINZTGEYDQNQ. You are receiving this because your review was requested.Message ID: @.***>

jaredzimmerman commented 8 months ago

Is requesting permission to read favicons absolutely necessary? it seems really unnecessary from a UX perspective, even having a setting at all seems somewhat unneeded.

Sneezry commented 8 months ago

Is requesting permission to read favicons absolutely necessary? it seems really unnecessary from a UX perspective, even having a setting at all seems somewhat unneeded.

I do not want to embed favicons in the extension. That will increase the size of the package. For MV3, favicon permission is required, and chrome://favicon permission is required for MV2.

I add these new permissions as optional permission, only when this feature is enabled the extension will ask the permission.

Because this feature requires new permissions, we must have a new option to let the users disable it.

jaredzimmerman commented 8 months ago

How many 2FA will users have couple 100s max? even high res/svg favicons stored in local storage the size will be trivial.

Sneezry commented 8 months ago

How many 2FA will users have couple 100s max? even high res/svg favicons stored in local storage the size will be trivial.

The question is which favicons should we embed in the package? Google, Microsoft, Twitter, Facebook, GitHub... and what? And we can expect that there will be issues for missing favicons.

"Hey, please add xxx's favicon in the extension..."

We don't want to have such an issue.

jaredzimmerman commented 8 months ago

No, don't pre-install any favicons, get them when the user adds a site for a new code gen and store it in LocalStorage, you don't need to pre-install anything.

Sneezry commented 8 months ago

I got your point. LocalStorage has a 5MB limit, we can't store too much icons in that place. Most favicons are PNGs and ICOs, and if we want to store them in LocalStorage, we need to convert them to Base64 strings first. Again, base64 strings are ~1/3 bigger than the raw binary. Also, it can't be synced.

jaredzimmerman commented 8 months ago

I'm not sure that's accurate, also you don't need to sync the actual assets across installs, just the pointer, after sync on a new machine just have the local extension redownload from the synced pointers per code item. You can just grab a single variant of the favicon rather than trying to store full icos

Sneezry commented 8 months ago

you don't need to sync the actual assets across installs, just the pointer

chrome://favicon/xxx is the pointer, and it requires a new permission to access to.

jaredzimmerman commented 8 months ago

Use the URL matching function you already have to get the TLD for each item, on new install of the extension run the sync and redownload the favicons to that new machine, you're already storing matching URLS for each item.

Sneezry commented 8 months ago

redownload the favicons

Where should the extension download the favicons? If it downloads from chrome://favicon/xxx, this PR meets your expectation. If it downloads from <TLD>/favicon.ico, it will require <all_urls> permission, which is not acceptable.

jaredzimmerman commented 8 months ago

Ah! understood, my extension has so that's what I was comparing to. I understand what you're saying now, is there not metadata in chrome://favicon/xxx that allows you to match against the URL that your using for your site matching function? I'm not super familiar with how chrome stores favicons locally since I haven't needed to use that function.

Sneezry commented 8 months ago

For MV2, we can access to the favicons with this URL: chrome://favicon/<TLD>, for example, chrome://favicon/https://www.google.com. This requires chrome://favicon permssion.

For MV3, we can access to the favicons with this URL: <extension-url>/_favicon?pageUrl=<TLD>&size=16, for example, chrome-extension://bhghoamapcdpbohphigoooaddinpkbai/_favicon?pageUrl=https%3A%2F%2Fwww.google.com&size=16. This requires favicon permission.

mymindstorm commented 8 months ago

Have not forgotten about this. Next available time to work on this is Wednesday .

Paoapi commented 5 months ago

I am haker

Paoapi commented 5 months ago

I am haker

thebigsafari commented 5 months ago

Just FYI, not going to have time to look at this until around the 28th. … -------- Original message -------- From: Zhe Li @.> Date: 10/17/23 3:01 AM (GMT-06:00) To: Authenticator-Extension/Authenticator @.> Cc: Brendan Early @.>, Review requested @.> Subject: Re: [Authenticator-Extension/Authenticator] add favicon support (PR #1118) @Sneezryhttps://github.com/Sneezry requested your review on: #1118<#1118> add favicon support. — Reply to this email directly, view it on GitHub<#1118 (comment)>, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AGUAT3RAWUFSBGFHDIRN4KDX7Y3N7AVCNFSM6AAAAAA6DMQTWKVHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJQGY3TINZTGEYDQNQ. You are receiving this because your review was requested.Message ID: @.***>

Hey mymindstorm, I urgently need your help