Conversation
|
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)
@Sneezry<https://github.com/Sneezry> requested your review on: #1118<#1118> add favicon support.
—
Reply to this email directly, view it on GitHub<#1118 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AGUAT3RAWUFSBGFHDIRN4KDX7Y3N7AVCNFSM6AAAAAA6DMQTWKVHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJQGY3TINZTGEYDQNQ>.
You are receiving this because your review was requested.Message ID: ***@***.***>
|
|
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. |
|
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. |
|
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. |
|
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. |
|
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 |
chrome://favicon/xxx is the pointer, and it requires a new permission to access to. |
|
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. |
Where should the extension download the favicons? If it downloads from chrome://favicon/xxx, this PR meets your expectation. If it downloads from |
|
Ah! understood, my extension has <all_urls> 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. |
|
For MV2, we can access to the favicons with this URL: For MV3, we can access to the favicons with this URL: |
|
Have not forgotten about this. Next available time to work on this is Wednesday . |
Co-authored-by: Brendan <mymindstorm@evermiss.net>
Co-authored-by: Brendan <mymindstorm@evermiss.net>
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…lict) The original favicon PR (Authenticator-Extension#1118) targeted an MV2 base and had been open ~2 years; dev is now MV3 and the branch no longer merges. This re-implements the feature against current dev conventions and fixes the original bugs: - Built for MV3: uses the favicon permission + the chrome.runtime.getURL ('/_favicon/') endpoint instead of the dead MV2 chrome://favicon/ URL. Only the 'favicon' optional permission is requested (on toggle); the invalid MV2 'chrome://favicon/' host permission and CSP scheme are dropped. The extension's own _favicon resource is already covered by img-src 'self', so no CSP relaxation is needed. - EntryComponent.shouldShowFavicon is a proper reactive computed returning !isFirefox && !isSafari && menu.showFavicon (uses browser.ts constants), fixing the original && short-circuit bug that discarded the guard. - Wires showFavicon into the new UserSettings model (interface, BooleanOption, isBooleanOption) and the menu store, so the setting persists and syncs. - Favicons are fetched lazily per entry at render time; nothing bundled or pre-downloaded. medal.svg is a UI placeholder glyph, not a site favicon. Maintainer constraints honored: no mandatory permissions added, no pre-installed or pre-downloaded favicons.
5 participants
This PR adds favicon support.
chrome://faviconpermission 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.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.