Self-contained cloud favicon with ICO and apple-touch-icon fallbacks #62
No reviewers
Labels
No labels
Compat/Breaking
Kind/Bug
Kind/Documentation
Kind/Enhancement
Kind/Feature
Kind/Security
Kind/Testing
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
No milestone
No assignees
3 participants
Notifications
Due date
No due date set.
Reference
marvin8/yunjin!62
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/favicon-raster-fallbacks"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
The favicon was a
<text>element rendering the ☁️ emoji — no artwork of its own, appearance dependent on each visitor's emoji font, and nothing usable for clients without SVG favicon support./favicon.icoredirected to the SVG, which the legacy clients requesting it could not render.This makes the icon self-contained and adds raster fallbacks:
favicon.svgredrawn as explicit vector shapes (overlapping circles + rounded base) in--color-primaryblue#0066cc, transparent background — renders identically everywhere and stays deuteranopia-safefavicon.ico(16/32/48, transparent) generated from the SVG and committed; no build-time ImageMagick dependencyapple-touch-icon.png(180×180, white background — iOS composites transparency to black)base.htmladvertises all three via<link>tags/favicon.iconow serves the ICO file directly instead of redirecting to the SVGTestFaviconextended: raster fallback links in the base template, both static files served with correct content-types (ICO verified by magic bytes), and the route now returns 200 instead of a redirect.No
Release-Notes.mdexists in this repo, so no release-notes update applies.Closes #61
WuMing
Found 3 issue(s). See inline comments below.
@ -1798,0 +1801,4 @@assert response.status_code == 200html = response.data.decode()assert "/static/favicon.ico" in htmlcode [LOW]
This assertion only checks that the path string appears in the HTML. It does not verify that it is used in a correct <link rel="icon" ...> fallback (and similarly for the apple-touch-icon). A stray path or wrong rel could pass. Prefer asserting the full link tag or parsing the HTML.
@ -1807,0 +1818,4 @@assert response.status_code == 200# mimetypes.guess_type maps .ico differently per platform; both are valid.assert response.content_type in ("image/x-icon", "image/vnd.microsoft.icon")assert response.data[:4] == b"\x00\x00\x01\x00"code [LOW]
The docstring claims a multi-size 16/32/48 ICO, but the test only checks the ICO magic header. A single-size or malformed ICO could still pass. Consider parsing the ICO directory entries to verify the expected dimensions/count.
@ -1807,0 +1826,4 @@assert response.status_code == 200assert response.content_type == "image/png"assert response.data[:4] == b"\x89PNG"code [LOW]
The docstring states the PNG is 180x180, but this test only checks the PNG magic bytes. Any sized PNG would pass. If the 180x180 dimension is a requirement, parse the PNG IHDR and assert width/height.
✅ Fixed — favicon fallback tests now verify structure, not just strings
tests/test_web.py— raster link assertions ·tests/test_web.py— apple-touch-icon dimensions ·tests/test_web.py— ICO directory entriesAll three strengthened in commit
18b8e78:<link rel="icon" type="image/x-icon">and<link rel="apple-touch-icon">tags, so a stray path or wrongrelfails.WuMing
Found 1 issue(s). See inline comments below.
@ -1814,0 +1849,4 @@assert response.status_code == 200# mimetype is guessed per platform; both mappings are valid for ICO.assert response.content_type in ("image/x-icon", "image/vnd.microsoft.icon")assert response.data[:4] == b"\x00\x00\x01\x00"code [LOW]
This route test only checks the ICO magic number and content type for /favicon.ico. It would still pass if the route served a different or truncated ICO file with a valid header. Compare the full response body to the static file (e.g., assert response.data == client.get('/static/favicon.ico').data) or at least assert the directory entry count/sizes here so the route is verified to serve the intended asset.
✅ Fixed — route test now compares bytes with the static file
tests/test_web.py— route testFixed in commit
ae6f4ee— the test now fetches/static/favicon.icoin the same client context and assertsresponse.data == static_response.data, so a truncated or different file behind a valid header fails. The magic-bytes and directory checks remain on the static-file test; equality here proves the route serves exactly that validated asset.WuMing
Found 1 issue(s). See inline comments below.
@ -171,3 +181,2 @@}curl -fsSL --connect-timeout 2 --max-time 5 http://localhost:18099/favicon.ico | grep -q '<svg' || {echo "/favicon.ico did not resolve to the SVG icon" >&2ico_magic=$(curl -fsSL --connect-timeout 2 --max-time 5 http://localhost:18099/favicon.ico | head -c 4 | od -An -tx1 | tr -d ' \n')config [MEDIUM]
head -c 4closes the pipe after reading four bytes, so curl is killed with SIGPIPE and exits 23 (Failure writing output to destination). If this smoke-test script runs underset -o pipefail(the usual setup for these workflow snippets), the whole pipeline returns non-zero andset -eaborts the job even though the ICO is served correctly — a spurious CI failure. Conversely, if pipefail is off, a curl error (-f) is silently swallowed andico_magiccomes back empty, producing the misleadingdid not return an ICO file (magic )message. Fetch the bytes into a file/variable first and then inspect them, e.g.curl -fsSL --connect-timeout 2 --max-time 5 -o /tmp/favicon.ico http://localhost:18099/favicon.ico && ico_magic=$(head -c 4 /tmp/favicon.ico | od -An -tx1 | tr -d ' \n'), so the failure mode and the assertion are unambiguous.✅ Fixed — smoke test fetches the ICO to a file before inspection
.forgejo/workflows/ci.yml— smoke testFixed in commit
72b64a0— the smoke test now doescurl -fsSL -o /tmp/favicon-smoke.icoand inspects the file withhead -c 4, so curl failures surface via their own exit code under-e/pipefailand there is no SIGPIPE race.WuMing
Found 3 issue(s). See inline comments below.
@ -133,0 +134,4 @@echo "favicon.ico missing from image" >&2exit 1}test -f "$rootfs/app/src/yunjin/web/static/apple-touch-icon.png" || {config [MEDIUM]
apple-touch-icon.png is only checked for existence in the built image rootfs; unlike favicon.ico, it is never requested over HTTP. If its static route/mount is wrong or the file is not actually served, the smoke test still passes even though base.html advertises it and iOS would receive a 404. Fetch http://localhost:18099/apple-touch-icon.png (or whatever the advertised href is) and assert 200 (and optionally the PNG magic 89504e47).
@ -167,3 +177,2 @@code=$(curl -s -o /dev/null -w '%{http_code}' --connect-timeout 2 --max-time 5 -I http://localhost:18099/favicon.ico)[ "$code" = "302" ] || {echo "/favicon.ico returned ${code}, expected 302" >&2[ "$code" = "200" ] || {config [MEDIUM]
The smoke test only asserts status 200 and ICO magic bytes; it never asserts the response Content-Type. A static file served with the wrong MIME type (e.g. text/plain or text/html) would pass this check, yet browsers and iOS may still ignore the icon. The PR explicitly claims correct content-types are verified, so add a content-type assertion for /favicon.ico (e.g. capture
%{content_type}withcurl -sI -o /dev/null -w '%{content_type}'and requireimage/x-iconorimage/vnd.microsoft.icon).@ -174,0 +182,4 @@# Fetch to a file: piping curl into head -c would SIGPIPE-kill curl# mid-body (exit 23) under pipefail, and -f failures would be# silently swallowed by the pipeline.curl -fsSL --connect-timeout 2 --max-time 5 -o /tmp/favicon-smoke.ico http://localhost:18099/favicon.icoconfig [LOW]
The GET writes to a fixed, predictable path in world-writable /tmp (CWE-377 symlink/race risk) and the file is never cleaned up. Use
mktemp(with a trap/rm) or write into the job workspace instead, so parallel runs cannot collide and no stale artifact is left behind.✅ Fixed — smoke test now asserts content types, requests both icons, uses mktemp
ci.yml— content-type assertion ·ci.yml— apple-touch-icon HTTP check ·ci.yml— mktempAll three addressed in commit
8abb4bd:/favicon.icocontent type is now asserted and must beimage/x-iconorimage/vnd.microsoft.icon./apple-touch-icon.pngis fetched over HTTP and asserted to return a PNG (magic89504e47), matching whatbase.htmladvertises.mktempfiles cleaned up in the existing EXIT trap, replacing the fixed/tmppath.