Repository navigation
feat(favicons): one server route for every site icon, with parent-domain fallback - #9494
Conversation
… parent-domain fallback Icons were requested straight from Google in the browser, which shows Google's placeholder globe for any host it can't see (a 404 with an image body that an <img> treats as success), so private tenants and internal hosts rendered a generic placeholder. GET /api/favicon asks Google and DuckDuckGo server-side, where the 404 is visible, walks up to the parent domain so a tenant shows its platform's icon, never sends internal hosts or the destination itself anything, and caches results in Redis. Every surface now uses the shared <Favicon> or faviconSrc, and binary internal routes accept a user rate limit.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
There was a problem hiding this comment.
All reported issues were addressed across 20 files
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
[Medium impact] The PR appears safe to merge; no new actionable issues were found. SummarySite icons now use one signed-in server route with provider fallback, parent-domain lookup, and caching.
|
…mpty labels and non-DNS hostnames A miss is an ordinary answer, so it returns 204 (which an img still reports as failed) instead of an error status; binary internal routes may now declare several success statuses and present a no-content response, sharing the JSON builder's status validation. Providers are asked one at a time so a second only learns a host the first had no icon for, a non-200 body is never read, and anything but 404/410 counts as unavailable rather than a cached miss.
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 23 files
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
…ody before one classification
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
…st its own descriptor
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
Summary
Every site icon in the app now comes from one place, and a site Google can't see no longer renders Google's grey placeholder globe.
Why: icons were
<img src="https://www.google.com/s2/favicons?...">in the browser. For a host Google has no icon for (a private tenant such as<tenant>.atlassian.net, an internal host), it answers 404 with a 16px globe image, which the browser treats as a successful load, soonErrornever fired and every such row showed the placeholder. One surface also fetched<site>/favicon.icostraight from the destination.Resolution (
lib/favicon)GET /api/favicon?host=asks Google, then DuckDuckGo, server-side, where the 404 is visible (the approach Bitwarden's and Linkwarden's icon services use); the second provider only learns a host the first had no icon fortldts), sox.gitlab.io→gitlab.ioandx.atlassian.net→atlassian.net, neverioorco.uk; at most 3 parents.arpa) and names DNS can't hold (punctuation that could reshape a provider URL, empty labels) never leave the server; the destination itself is never contactedsniffImageContentTypeplus ICO), never SVG, capped at 256 KB, and servednosniff<img>still reports it as failed and renders the fallback), not an error statusRoute and client
favicons.get,capability: 'none') throughdefineInternalBinaryRoute, with a per-user rate limit. Binary internal routes now accept the sameuserrate-limit policy JSON routes do, and may declare several success statuses (validation shared with the JSON builder) and present a 204<Favicon host fallback>andfaviconSrc(host)replacefaviconUrl(deleted) in chat source icons, browser tab / suggestion / agent icons, table URL cells, and Sim Links; browser-imported icons stay local, with their link fallback restored on errorType of Change
Testing
lib/favicon/{hosts,probe}.test.ts(49), written red-first from the failure modes: internal hosts never offered, tenant → platform walk, never a bare suffix, 3-parent cap, normalization and IDN, URL-reshaping punctuation and empty labels rejected (verified to fail withtldtshostname validation off); 404-with-placeholder, HTML, SVG, empty, and oversized bodies are misses; 403/408/429/5xx are unavailablenosniffand private caching; made-up Atlassian and GitLab tenants get their platform's icon (~0.6 s cold); unknown site, internal hosts, and a URL-injection host answer 204 fast; malformed host 400; repeat lookup served from cache in ~20 ms. Not committed, since it depends on live third-party providerscheck:api-validation:strict,check:source-conventions,check:client-boundary,check:boundaries,check:application-graph,check:permission-group-enforcement,check:route-verbs,check:file-names,check:comment-hygiene,check:utils,check:test-patterns,check:source-textChecklist
test-auditauthoring gate)