Self-contained cloud favicon with ICO and apple-touch-icon fallbacks #62

Manually merged
marvin8 merged 10 commits from feat/favicon-raster-fallbacks into main 2026-10-01 10:24:59 +00:00
Collaborator

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.ico redirected to the SVG, which the legacy clients requesting it could not render.

This makes the icon self-contained and adds raster fallbacks:

  • favicon.svg redrawn as explicit vector shapes (overlapping circles + rounded base) in --color-primary blue #0066cc, transparent background — renders identically everywhere and stays deuteranopia-safe
  • favicon.ico (16/32/48, transparent) generated from the SVG and committed; no build-time ImageMagick dependency
  • apple-touch-icon.png (180×180, white background — iOS composites transparency to black)
  • base.html advertises all three via <link> tags
  • /favicon.ico now serves the ICO file directly instead of redirecting to the SVG

TestFavicon extended: 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.md exists in this repo, so no release-notes update applies.

Closes #61

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.ico` redirected to the SVG, which the legacy clients requesting it could not render. This makes the icon self-contained and adds raster fallbacks: - `favicon.svg` redrawn as explicit vector shapes (overlapping circles + rounded base) in `--color-primary` blue `#0066cc`, transparent background — renders identically everywhere and stays deuteranopia-safe - `favicon.ico` (16/32/48, transparent) generated from the SVG and committed; no build-time ImageMagick dependency - `apple-touch-icon.png` (180×180, white background — iOS composites transparency to black) - `base.html` advertises all three via `<link>` tags - `/favicon.ico` now serves the ICO file directly instead of redirecting to the SVG `TestFavicon` extended: 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.md` exists in this repo, so no release-notes update applies. Closes #61
👽 Add ICO and apple-touch-icon raster fallbacks
Some checks failed
/ gitleaks (pull_request) Failing after 1m1s
/ pr-review (pull_request) Failing after 1m13s
/ checks (pull_request) Failing after 1m15s
/ publish-container (pull_request) Has been skipped
/ deploy-docs (pull_request) Has been skipped
/ container (pull_request) Has been skipped
56c08cbd08
🔧 Retrigger CI after transient zhuiri runner failure
Some checks failed
/ gitleaks (pull_request) Failing after 59s
/ checks (pull_request) Failing after 1m6s
/ publish-container (pull_request) Has been skipped
/ deploy-docs (pull_request) Has been skipped
/ container (pull_request) Has been skipped
/ pr-review (pull_request) Failing after 1m11s
1fefeb4d18
🔧 Retrigger CI after upstream Forgejo hosting outage
Some checks failed
/ gitleaks (pull_request) Successful in 18s
/ checks (pull_request) Successful in 2m2s
/ publish-container (pull_request) Has been skipped
/ deploy-docs (pull_request) Has been skipped
/ pr-review (pull_request) Successful in 2m27s
/ container (pull_request) Failing after 1m51s
75300605c4
forgejo-actions left a comment

WuMing

Found 3 issue(s). See inline comments below.

## WuMing Found **3** issue(s). See inline comments below.
@ -1798,0 +1801,4 @@
assert response.status_code == 200
html = response.data.decode()
assert "/static/favicon.ico" in html

code [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.

**code** [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. <!-- wuming:sha256:1e27e644d369b6c01e00b50ea374cbaf006a53e32f69f7edc19cc4b660c55ccc -->
@ -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.

**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. <!-- wuming:sha256:6c8ba4c34e4f48b4b92663c3648461672843763f438a683c7c4ef2238aab40b1 -->
@ -1807,0 +1826,4 @@
assert response.status_code == 200
assert 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.

**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. <!-- wuming:sha256:345323bcf1ed9881c07dc78cb5009b2be338b9e002fa073f72a354f8afd3d4a3 -->
✅ Strengthen favicon fallback assertions per review
Some checks failed
/ gitleaks (pull_request) Successful in 26s
/ checks (pull_request) Successful in 1m56s
/ publish-container (pull_request) Has been skipped
/ deploy-docs (pull_request) Has been skipped
/ pr-review (pull_request) Successful in 2m10s
/ container (pull_request) Failing after 1m32s
18b8e789d6
Author
Collaborator

✅ 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 entries

This assertion only checks that the path string appears in the HTML…

All three strengthened in commit 18b8e78:

  • The base-template test now asserts the complete <link rel="icon" type="image/x-icon"> and <link rel="apple-touch-icon"> tags, so a stray path or wrong rel fails.
  • The apple-touch-icon test parses the PNG IHDR chunk and asserts exactly 180×180.
  • The ICO test parses the directory entries and asserts exactly three images at 16×16, 32×32, and 48×48.
### ✅ Fixed — favicon fallback tests now verify structure, not just strings [`tests/test_web.py` — raster link assertions](https://forge.marvin8.zone/marvin8/yunjin/pulls/62#issuecomment-4058) · [`tests/test_web.py` — apple-touch-icon dimensions](https://forge.marvin8.zone/marvin8/yunjin/pulls/62#issuecomment-4059) · [`tests/test_web.py` — ICO directory entries](https://forge.marvin8.zone/marvin8/yunjin/pulls/62#issuecomment-4060) > This assertion only checks that the path string appears in the HTML… All three strengthened in commit `18b8e78`: - The base-template test now asserts the complete `<link rel="icon" type="image/x-icon">` and `<link rel="apple-touch-icon">` tags, so a stray path or wrong `rel` fails. - The apple-touch-icon test parses the PNG IHDR chunk and asserts exactly 180×180. - The ICO test parses the directory entries and asserts exactly three images at 16×16, 32×32, and 48×48.
🐛 Update container smoke test for direct ICO favicon serving
All checks were successful
/ gitleaks (pull_request) Successful in 16s
/ checks (pull_request) Successful in 2m8s
/ publish-container (pull_request) Has been skipped
/ deploy-docs (pull_request) Has been skipped
/ pr-review (pull_request) Successful in 2m35s
/ container (pull_request) Successful in 1m32s
08c6125c1b
forgejo-actions left a comment

WuMing

Found 1 issue(s). See inline comments below.

## 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.

**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. <!-- wuming:sha256:9afc75b589eabfc4753133beedd46c920a6ea8428099febf8ef24cc2240e56a2 -->
✅ Route test compares favicon.ico bytes to static file
All checks were successful
/ gitleaks (pull_request) Successful in 37s
/ checks (pull_request) Successful in 1m56s
/ publish-container (pull_request) Has been skipped
/ deploy-docs (pull_request) Has been skipped
/ pr-review (pull_request) Successful in 3m1s
/ container (pull_request) Successful in 1m38s
ae6f4ee5cb
Author
Collaborator

✅ Fixed — route test now compares bytes with the static file

tests/test_web.py — route test

This route test only checks the ICO magic number and content type for /favicon.ico…

Fixed in commit ae6f4ee — the test now fetches /static/favicon.ico in the same client context and asserts response.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.

### ✅ Fixed — route test now compares bytes with the static file [`tests/test_web.py` — route test](https://forge.marvin8.zone/marvin8/yunjin/pulls/62#issuecomment-4065) > This route test only checks the ICO magic number and content type for /favicon.ico… Fixed in commit `ae6f4ee` — the test now fetches `/static/favicon.ico` in the same client context and asserts `response.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.
forgejo-actions left a comment

WuMing

Found 1 issue(s). See inline comments below.

## 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" >&2
ico_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 4 closes 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 under set -o pipefail (the usual setup for these workflow snippets), the whole pipeline returns non-zero and set -e aborts 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 and ico_magic comes back empty, producing the misleading did 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.

**config** [MEDIUM] `head -c 4` closes 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 under `set -o pipefail` (the usual setup for these workflow snippets), the whole pipeline returns non-zero and `set -e` aborts 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 and `ico_magic` comes back empty, producing the misleading `did 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. <!-- wuming:sha256:f58be5f3480f10025a566963bfca344bd7a46e2d69cd891b31fc19b1e0f1e607 -->
🐛 Fetch smoke-test ICO to file to avoid SIGPIPE under pipefail
All checks were successful
/ gitleaks (pull_request) Successful in 14s
/ checks (pull_request) Successful in 2m2s
/ publish-container (pull_request) Has been skipped
/ deploy-docs (pull_request) Has been skipped
/ pr-review (pull_request) Successful in 3m3s
/ container (pull_request) Successful in 1m34s
72b64a01d5
Author
Collaborator

✅ Fixed — smoke test fetches the ICO to a file before inspection

.forgejo/workflows/ci.yml — smoke test

head -c 4 closes the pipe after reading four bytes, so curl is killed with SIGPIPE…

Fixed in commit 72b64a0 — the smoke test now does curl -fsSL -o /tmp/favicon-smoke.ico and inspects the file with head -c 4, so curl failures surface via their own exit code under -e/pipefail and there is no SIGPIPE race.

### ✅ Fixed — smoke test fetches the ICO to a file before inspection [`.forgejo/workflows/ci.yml` — smoke test](https://forge.marvin8.zone/marvin8/yunjin/pulls/62#issuecomment-4069) > `head -c 4` closes the pipe after reading four bytes, so curl is killed with SIGPIPE… Fixed in commit `72b64a0` — the smoke test now does `curl -fsSL -o /tmp/favicon-smoke.ico` and inspects the file with `head -c 4`, so curl failures surface via their own exit code under `-e`/`pipefail` and there is no SIGPIPE race.
forgejo-actions left a comment

WuMing

Found 3 issue(s). See inline comments below.

## WuMing Found **3** issue(s). See inline comments below.
@ -133,0 +134,4 @@
echo "favicon.ico missing from image" >&2
exit 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).

**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). <!-- wuming:sha256:dc1aac1bb1c9b6a751702e754512ae768f95589d2e9ec6fb58566ae35b9d0e0b -->
marvin8 marked this conversation as resolved
@ -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} with curl -sI -o /dev/null -w '%{content_type}' and require image/x-icon or image/vnd.microsoft.icon).

**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}` with `curl -sI -o /dev/null -w '%{content_type}'` and require `image/x-icon` or `image/vnd.microsoft.icon`). <!-- wuming:sha256:59df32806a3bff41d9a95e53de42c9f7122a98b9815d139bb8bf523e60455bf2 -->
marvin8 marked this conversation as resolved
@ -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.ico

config [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.

**config** [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. <!-- wuming:sha256:515f8042a127683458aed2ff96295f436d5faafa6e24a8f8d1e9624eac6413fe -->
✅ Smoke test checks icon content types and serves both icons
Some checks failed
/ gitleaks (pull_request) Successful in 23s
/ checks (pull_request) Successful in 1m55s
/ publish-container (pull_request) Has been skipped
/ deploy-docs (pull_request) Has been skipped
/ pr-review (pull_request) Successful in 2m53s
/ container (pull_request) Failing after 1m51s
8abb4bd9c0
Author
Collaborator

✅ 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 — mktemp

The smoke test only asserts status 200 and ICO magic bytes…

All three addressed in commit 8abb4bd:

  • /favicon.ico content type is now asserted and must be image/x-icon or image/vnd.microsoft.icon.
  • /apple-touch-icon.png is fetched over HTTP and asserted to return a PNG (magic 89504e47), matching what base.html advertises.
  • Both downloads go to mktemp files cleaned up in the existing EXIT trap, replacing the fixed /tmp path.
### ✅ Fixed — smoke test now asserts content types, requests both icons, uses mktemp [`ci.yml` — content-type assertion](https://forge.marvin8.zone/marvin8/yunjin/pulls/62#issuecomment-4073) · [`ci.yml` — apple-touch-icon HTTP check](https://forge.marvin8.zone/marvin8/yunjin/pulls/62#issuecomment-4074) · [`ci.yml` — mktemp](https://forge.marvin8.zone/marvin8/yunjin/pulls/62#issuecomment-4075) > The smoke test only asserts status 200 and ICO magic bytes… All three addressed in commit `8abb4bd`: - `/favicon.ico` content type is now asserted and must be `image/x-icon` or `image/vnd.microsoft.icon`. - `/apple-touch-icon.png` is fetched over HTTP and asserted to return a PNG (magic `89504e47`), matching what `base.html` advertises. - Both downloads go to `mktemp` files cleaned up in the existing EXIT trap, replacing the fixed `/tmp` path.
🐛 Smoke test fetches apple-touch-icon from its /static path
All checks were successful
/ gitleaks (pull_request) Successful in 23s
/ pr-review (pull_request) Successful in 1m53s
/ checks (pull_request) Successful in 1m55s
/ publish-container (pull_request) Has been skipped
/ deploy-docs (pull_request) Has been skipped
/ container (pull_request) Successful in 1m39s
b4716317ba
marvin8 approved these changes 2026-10-01 10:24:06 +00:00
marvin8 manually merged commit e47d317186 into main 2026-10-01 10:24:59 +00:00
Sign in to join this conversation.
No reviewers
No milestone
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Reference
marvin8/yunjin!62
No description provided.