Skip to content

[PM-40245] Add google favicon API to icon service lookups#7951

Open
kspearrin wants to merge 3 commits into
mainfrom
googlefavicon
Open

[PM-40245] Add google favicon API to icon service lookups#7951
kspearrin wants to merge 3 commits into
mainfrom
googlefavicon

Conversation

@kspearrin

@kspearrin kspearrin commented Jul 9, 2026

Copy link
Copy Markdown
Member

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-40245

📔 Objective

  • Adds a Google Favicon API check as the first step in the icon-fetching pipeline. When Google has an icon for the domain, we skip the more expensive HTML-scrape-and-scan-for-<link rel="icon"> flow entirely.
  • The check hits https://www.google.com/s2/favicons?domain={domain}&sz=64, which 301-redirects through Google's internal t0.gstatic.com/faviconV2 service and returns a PNG. The existing IconHttpRequest pipeline handles the redirect via its SSRF-protected client.
  • When Google has no icon for the domain it responds 404, which propagates through the existing IconLink.FetchAsync as a null return, dropping cleanly into the existing scrape → /favicon.ico fallback.
  • Gated behind a new IconsSettings.GoogleFaviconEnabled config flag. Enable via IconsSettings__GoogleFaviconEnabled=true env var or the equivalent key in appsettings.json. On by default.

@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the addition of the Google Favicon API as the first step in the icon-fetching pipeline, gated behind the new IconsSettings.GoogleFaviconEnabled flag. Verified SSRF handling (the user-controlled domain is parsed to uri.Host, URL-escaped, and every request/redirect hop is re-validated by the SSRF-protected IconHttpRequest/IUriService pipeline), 404-to-null fallback behavior, DI registration, and test wiring. The earlier concern about the self-hosted default has been addressed: appsettings.SelfHosted.json now sets googleFaviconEnabled: false, keeping the domain-leak-to-Google behavior opt-out for self-host while cloud remains on.

Code Review Details

No unresolved findings. All prior review threads are resolved:

  • The previously flagged self-host default (googleFaviconEnabled true in base config) is now overridden to false in appsettings.SelfHosted.json, matching the maintainer's stated intent.
  • The generic catch (Exception) in GetGoogleFaviconAsync is acceptable here — it is defensive top-level handling for a best-effort optimization that logs and cleanly falls through to the existing scrape → /favicon.ico pipeline, so a Google failure never breaks icon fetching.

@kspearrin kspearrin marked this pull request as draft July 9, 2026 17:50
Comment thread src/Icons/appsettings.json
Comment thread src/Icons/Services/IconFetchingService.cs Dismissed
@codecov

codecov Bot commented Jul 9, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 24.24242% with 25 lines in your changes missing coverage. Please review.
✅ Project coverage is 61.48%. Comparing base (7081654) to head (4a76fd8).
⚠️ Report is 10 commits behind head on main.

Files with missing lines Patch % Lines
src/Icons/Services/IconFetchingService.cs 21.87% 24 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7951      +/-   ##
==========================================
+ Coverage   61.45%   61.48%   +0.03%     
==========================================
  Files        2245     2253       +8     
  Lines       98854    99120     +266     
  Branches     8945     8962      +17     
==========================================
+ Hits        60747    60941     +194     
- Misses      35957    36027      +70     
- Partials     2150     2152       +2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@gbubemismith gbubemismith added t:tech-debt Change Type - Tech debt t:feature Change Type - Feature Development and removed t:tech-debt Change Type - Tech debt labels Jul 10, 2026

@gbubemismith gbubemismith left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good

Comment thread src/Icons/appsettings.json
@kspearrin kspearrin requested a review from gbubemismith July 10, 2026 20:32
@kspearrin kspearrin changed the title Add google favicon API to icon service lookups [PM-40245] Add google favicon API to icon service lookups Jul 10, 2026
@gbubemismith gbubemismith marked this pull request as ready for review July 10, 2026 22:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

t:feature Change Type - Feature Development

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants