Hand-shipped project icons may embed data:image rasters

[ghackett]

What

Restores the icons the sidebar stopped showing for repos whose project-icon.svg wraps a raster in an SVG <image> with a base64 data: URI (e.g. podcast-hacker and standup-timer). The active-content gate introduced with the icon generator in #195 refused every href that wasn't a local #fragment, which silently rejected those previously working files.

The rule change

• On-disk icons (usable_icon_bytes): an href/xlink:href may now also carry an inline data:image/png URI. Projects only appear in the sidebar once they're trusted, so an embedded raster in a repo's own icon is the project shipping its own artwork, not a foreign fetch. The carve-out is deliberately narrow: only the href branch, only PNG (subtype terminated by ; or ,) — data:image/svg+xml stays refused (it's XML whose base64 payload could smuggle the script/handler content the plain-text checks can't see), other raster codecs stay off the list until an icon actually needs one, and CSS url(data:...), every other data: media type, data URIs hidden behind leading whitespace, scripts, event handlers, @import, and all external URLs remain refused as before.
• Generated icons: a new projecticons.usable_generated_icon_bytes keeps the original strict rule — everything above plus no data: hrefs at all. icongen.extract_svg now vets replies with it, and the design brief gains an explicit "pure vector art only: no data: URIs, no embedded raster images" hard requirement, so what the gate enforces is also what the model is asked for.

Tests

• test_projecticons: inline data:image/png hrefs (both href and xlink:href) are accepted; data:image/svg+xml (base64 and percent-encoded), other raster codecs (jpeg, webp), lookalike subtypes (image/png-evil), data:text/html, whitespace-prefixed data URIs, and url(data:...) are refused; all the existing active-content rejections still hold.
• test_icongen: extract_svg refuses every data: href — including the exact data:image/png payload the on-disk gate now accepts, pinning the two gates' divergence.

Verified against the real files: both repos' PNG-wrapped icons pass the on-disk gate and would still be refused if a generation run produced them. Full local suite: 36 passed.

🤖 Generated with Claude Code

https://claude.ai/code/session_012BwbGQzyQQX3Gpj63DmX4W

[ghackett]

@claude review

[claude]

Claude finished @ghackett's task in 2m 33s —— View job

---

Claude finished the review

Todo list

• Gather context (diff, changed files)
• Understand the request (code review)
• Review collins/projecticons.py changes
• Review collins/icongen.py changes
• Review test changes
• Post review feedback

---

Summary

The split between usable_icon_bytes (on-disk, now allows data:image/* hrefs) and usable_generated_icon_bytes (strict, no data: hrefs at all) is a clean way to keep the generator's output pure-vector while relaxing the rule only for already-trusted, hand-shipped icons. Docstrings, the _SVG_ACTIVE_CONTENT/_SVG_DATA_HREF comments, and the design-brief addition in icongen.py all stay in sync with the code, and the tests exercise both the accept and reject paths (href/xlink:href, data:text/html, whitespace-prefixed data URIs, url(data:...)) plus pin the divergence between the two gates.

One finding worth a look

collins/projecticons.py:34 — the carve-out is keyed off the MIME-type prefix data:image/, not off "raster":

rb"\b(?:xlink:)?href\s*=\s*[\"'](?!#|data:image/)"

image/svg+xml also starts with data:image/, so an on-disk icon can embed another SVG document via <image href="data:image/svg+xml;base64,...">. Unlike PNG/JPEG/GIF/WebP, image/svg+xml is XML that can itself carry <script>/event-handler content — and because that payload is base64 (or percent-encoded) inside the data URI, the literal <\s*script check in the same regex never sees it as text, so it doesn't get caught. That reopens exactly the class of risk this gate exists to close ("Anything that could run ... is refused outright"), for the one MIME subtype where "embedding a raster" and "embedding active content" aren't the same thing. The PR description and docstrings frame the carve-out as being about "raster" artwork specifically, but the implementation doesn't actually exclude svg+xml.

None of the new tests cover this subtype (they use image/png and text/html), so nothing currently pins the behavior either way.

Suggested fix: narrow the lookahead to actual raster subtypes instead of the whole image/ prefix, e.g. data:image/(?:png|jpe?g|gif|webp|bmp|x-icon) (mirrored in the comment above the regex), and add a test that an on-disk icon with a data:image/svg+xml href is still refused.

Fix this →

No other correctness, readability, or test-coverage issues found in icongen.py, projecticons.py, or the test files.

[ghackett]

Addressed the data:image/svg+xml finding in a50763e: the carve-out is now an enumerated raster allowlist — data:image/(?:png|jpe?g|gif|webp|bmp|x-icon) with the subtype terminated by ; or , — so a nested SVG data URI (base64 or percent-encoded) is refused again, along with lookalike subtypes like image/png-evil. New tests pin all of those, and both repos' real PNG-wrapped icons still pass. Full suite: 36 passed locally.