fix(favicon): static /favicon.svg respects app's own file before letter-mark - #187
Merged
Conversation
Pins the correct behavior: a static app that ships its own /favicon.svg must have it served by Caddy (file_server via file matcher), NOT replaced by the generated platform letter-mark. Bug: faviconVhostBodyLines() emits /favicon.svg as an unconditional `respond` with the generated mark — no file matcher. Astro sites friends-of-twin-peaks and gregg-brandalise would be clobbered on the next heal cycle. New tests (all RED on 472f8e8): - file matcher present for /favicon.svg - file_server branch in /favicon.svg block (own file served when present) - generated mark fallback still exists (apps without own svg) - node path confirmed correct (/favicon.svg covered, handle_response on 404) - /favicon.ico behavior unchanged (regression guard) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…er-mark Bug: faviconVhostBodyLines() emitted /favicon.svg as an unconditional `respond` with the generated letter-mark, overriding any app-supplied favicon.svg. Astro sites friends-of-twin-peaks and gregg-brandalise both ship their own designed /favicon.svg and would have been clobbered on the next heal cycle. Fix: add @samohost_has_favicon_svg file matcher for /favicon.svg, mirroring the existing @samohost_has_favicon pattern already used for /favicon.ico. When the app ships its own /favicon.svg, file_server serves it; when absent, the generated letter-mark fallback respond fires. The node path (faviconVhostLinesNode) was already correct: it proxies the upstream for both /favicon.ico and /favicon.svg and only substitutes the generated mark via handle_response on 404 — no change needed there. caddy validate: PASS bun test test/favicon.test.ts: 78 pass, 0 fail Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Root-cause analysis
Commit 472f8e8 (PR #185) introduced per-app SVG letter-mark fallback for Caddy vhosts. In
faviconVhostBodyLines()(the STATIC path),/favicon.icowas correctly gated behind a@samohost_has_favicon file /favicon.icomatcher — serving the app's own file when present, else the generated mark./favicon.svgwas NOT gated: it emitted an unconditionalrespondwith the generated letter-mark on every request.Two live Astro client sites ship their own designed
/favicon.svg:friends-of-twin-peaks.samo.team(custom dark-bg mountain-peak SVG path)gregg-brandalise.samo.team(custom dark-bg Cormorant Garamond monogram)Both sites were intact when the heal timer was paused (curl-probed, confirmed). Without this fix, the next heal cycle would clobber both with the platform letter-mark.
The node path (
faviconVhostLinesNode) was already correct: it proxies upstream for both/favicon.icoAND/favicon.svgand only substitutes the generated mark viahandle_responseon upstream 404 — no change needed there.Fix
Added
@samohost_has_favicon_svg file /favicon.svgmatcher infaviconVhostBodyLines(), mirroring the existing@samohost_has_faviconpattern for/favicon.ico. The/favicon.svghandle block now has:handle @samohost_has_favicon_svg { file_server }— own file served when presenthandle { respond "<generated-mark>" 200 }— fallback for apps without own svgTest evidence
RED commit
bbd9842: 2 new regression tests fail (file matcher absent, file_server branch absent in svg block).GREEN commit
d694ee1: all 78 favicon tests pass, 0 new failures introduced.caddy validate: Valid configurationPre-existing failures (4, unrelated — env store / Cloudflare env tests): confirmed present on
mainbefore this branch.Scope of change
src/caddy/favicon.ts— fixfaviconVhostBodyLines(), update JSDoctest/favicon.test.ts— 5 new regression tests (RED then GREEN)Heal timer status
Heal timer is currently PAUSED (stopped + disabled on samo-control-plane). Do NOT re-enable until this PR is merged and deployed.
Resume command after merge + deploy:
Intentionally not done
/favicon.svg(manager runs after heal converges).🤖 Generated with Claude Code