fix(platform): allow branding favicon and font under CSP via SITE_URL origin (#1964)#2115
fix(platform): allow branding favicon and font under CSP via SITE_URL origin (#1964)#2115larryro wants to merge 1 commit into
Conversation
larryro
left a comment
There was a problem hiding this comment.
Desk Review — PR #2115 / issue #1964 (CSP blocking custom favicon & font)
VERDICT: READY TO MERGE — the favicon CSP block is genuinely fixed, the change keeps the policy strict, CI is green, and the project's tests pass.
CI state
gh pr checks — all required checks PASS (verified just now). Notable: Analyze (js-ts & python), Build platform, Type check, Lint, Format, Knip, Unit, UI, Smoke test, Scan platform, Opengrep, and all 16 Playwright (platform) shards + Playwright (web/docs) — green. Only fork-only jobs (Validate images / Smoke test / Trivy / container tests for fork PRs) show "skipping", which is expected for an in-repo branch. No red, no pending.
Test outcome (actually run)
cd services/platform && bunx vitest --run --project server server.test.ts→ 37 passed.- Both new regression tests run and pass: "CSP allows branding assets from the SITE_URL origin (issue #1964)" and "CSP omits the branding origin when SITE_URL is unset".
bunx vitest --run --project server convex/branding/queries.test.ts→ 27 passed.
What the change does
Adds siteOriginFromUrl() (mirrors the sibling sentryOriginFromDsn()), derives new URL(SITE_URL).origin, and appends it to img-src and font-src in buildContentSecurityPolicy() (services/platform/server.ts). Empty/invalid SITE_URL → null → nothing added.
Findings (Round 1 breadth + Round 2 confirmation)
1. Favicon fix — CORRECT, verified end-to-end (the substantive bug).
buildBrandingImageUrl (convex/branding/file_utils.ts:134) builds favicons as absolute <SITE_URL><BASE_PATH>/branding/images/... URLs → surfaced as faviconLightUrl/faviconDarkUrl (convex/branding/file_actions.ts:144) → set as link.href (app/components/branding/branding-provider.tsx:144). When the document is reached from a host other than SITE_URL, these are cross-origin and img-src 'self' data: blob: blocked them. Adding the SITE_URL origin to img-src correctly unblocks them. url.origin (scheme+host+port, no path) is the right CSP token and matches every path on that origin, so BASE_PATH in the asset URL is irrelevant. Confirmed correct.
2. font-src addition — harmless and strict, but defensive rather than demonstrably load-bearing (non-blocking, documented honestly).
I traced every font path: there is no font served from the SITE_URL origin in this codebase. The branding schema (lib/shared/schemas/branding.ts) has only logoFilename/faviconLightFilename/faviconDarkFilename and ALLOWED_IMAGE_EXTENSIONS is png/svg/jpg/jpeg/webp/ico — no font type. The shipped Inter webfont (@fontsource/inter, relative url(...) in packages/ui/src/globals.css) is served same-origin: Vite builds with base: './' and server.ts:780 injects <base href="${basePath}/"> (a path, not an absolute SITE_URL), so bundled assets resolve to the document's own origin and are already covered by font-src 'self'. Net: the font-src line allow-lists the operator's own origin symmetrically with img-src; it does not weaken the policy (no wildcard, no third-party CDN) and is forward-compatible with a future branding font, but no currently-served font is blocked by it. This satisfies "CSP remains as strict as possible." Note for the record: the issue asked to "confirm the exact failing font URL/origin first" — that confirmation isn't documented, and per this codebase the only SITE_URL-pinned branding asset is the favicon (an image). If an operator's production font was loaded from a genuinely third-party origin, this fix would not (and correctly should not) auto-allow it. Not a merge blocker.
3. Robustness / security — clean. url.origin cannot contain spaces/newlines/;, and an invalid SITE_URL (incl. whitespace) throws → caught → null; no CSP header-injection vector. The CSP is built once in createApp(), not per-request. Confirmed no wildcard/unsafe-* introduced (the new test asserts font-src has no *).
4. Elegance — minor, non-blocking. (a) The local array is named branding though it holds the site origin — brandingOrigins would read better. (b) siteOriginFromUrl uses url.origin while the sibling sentryOriginFromDsn uses ${url.protocol}//${url.host}; both are equivalent, the inconsistency is cosmetic. (c) The CSP policy-overview comment block (server.ts:~326-359) lists Sentry/Figma/canvas-preview exceptions but not this new SITE_URL branding-origin exception — worth a line, but optional.
5. Tests — adequate, with safe untested edges (non-blocking). The two tests pin the set/unset paths and use a directive-extracting helper plus a no-wildcard assertion. Untested but behaviorally safe (all fall through to the same null/unset path): invalid SITE_URL (the console.warn catch), SITE_URL with a non-default port, and SITE_URL with a trailing path. Nice-to-have, not required.
Verdict
READY TO MERGE — the reported favicon CSP violation is correctly and minimally fixed and verified end-to-end; the font-src addition keeps the policy strict and is a safe symmetric mirror; CI is fully green; the project's own tests pass. The items above are optional polish, not blockers.
Summary
Fixes #1964 — custom-branded favicons and fonts were blocked in production by
Content-Security-Policy: img-src/font-src 'self'.Root cause
Branding assets are addressed by
buildBrandingImageUrlas absolute<SITE_URL>/branding/...URLs. When the app is reached from a host that differs from the canonicalSITE_URL(reverse proxy, custom domain, www/apex split), those assets are cross-origin to the document, so'self'no longer matches and the browser blocks them. LocallySITE_URLis typically unset → URLs are relative → same-origin → works, which is why it only surfaced in production.Change
siteOriginFromUrl()(mirrors the existingsentryOriginFromDsn()pattern) to derive the platform's own canonical origin fromSITE_URL.img-src(favicons) andfont-src(branding-served fonts) inbuildContentSecurityPolicy().This is the operator's own origin (never a third-party CDN), so the policy stays strict and needs no third-party data-transfer / GDPR review. When
SITE_URLis unset, nothing is added and'self'continues to cover same-origin assets.Tests
Added two regression tests in
server.test.ts:SITE_URLorigin in bothimg-srcandfont-src, without widening to a wildcard.SITE_URLis unset.All
server.test.ts(37) andconvex/branding/queries.test.ts(27) tests pass; lint clean.Acceptance criteria