Repository navigation
Allow user plugins to declare a monochrome SVG icon - #4608
Conversation
Plugins can set icon.svg (inline, at most 8 KB). Only plain vector shapes are accepted; scripts, styles, links, url() references, raster images, foreignObject and DOCTYPE/entities are rejected, and a rejected SVG falls back to the monogram. The SVG renders as a template image in the switcher and on the menu card. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc94052594
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
clawsweeper
Bot
added
clawsweeper:intake:feature
|
Codex review: needs real behavior proof before merge. What this changesThis PR adds optional validated inline SVG icons to user-plugin switcher tabs and menu cards. Example: Acme Meter declares monogram AM, tint #336699, and an inline gauge SVG.
Review scores
ProductKind: Feature · Worth it: Needs a maintainer decision · Fix scope: Complete Merge readiness⛔ Blocked before merge - 3 items remain Keep this PR open for the plugin owner's decision on the public icon contract; the earlier escaped-URL defect is fixed, and no additional actionable code defect was found. Priority: P2 Decision needed
Before merge
FindingsNone. Tests
Agent review detailsHow this fits togetherLocal JavaScript or TypeScript plugin manifests enter the shared parser, which validates optional SVG markup before the app converts it into template images for plugin menu views. flowchart LR
A[Local plugin file] --> B[Manifest parser]
B --> C[SVG allowlist validation]
C --> D[Validated SVG or monogram fallback]
D --> E[Native template image]
E --> F[Switcher and menu card]
Technical reviewBest possible solution: If the plugin owner approves SVG support, retain this bounded shared validation and existing fallback within the current rendering owners. Do we have a high-confidence way to reproduce the issue? This is a feature request: current main intentionally exposes monogram and tint, and source inspection confirms no existing user-plugin SVG path. Is this the best way to solve the issue? The implementation fits the existing shared manifest parser and menu renderers, bounds untrusted markup, and preserves fallback without adding a competing rendering owner. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 6480977b47f6. Provenance checked
TestingProof path: in-process harness. SecurityNone. EvidenceWhat I checked:
Likely related people:
LabelsLabel changes: No label changes. Label justifications:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior. WorkflowClawSweeper edits this one comment on every review. Comment HistoryReviewed October 18, 2026, 2:36 AM ET / 06:36 UTC (Revision 3). |
Rejecting a literal url( was not enough: fill="u\72l(...)" passes it and CSS decodes the escape into a URL function. Attribute values now allow only letters, digits, spaces and .,#%+-() (no CSS escapes, comments, declarations, quotes, colons or slashes), fill/stroke must be a colour keyword, #hex or numeric rgb()/rgba(), and xmlns must be the SVG namespace. Adds the reported bypass plus red-team cases (case and full-width variants, character references, image-set/paint/var, style, use/a/xlink/xml:base, foreign namespaces, stylesheet PI, NUL) and accepted-paint cases.
|
Thanks for the detailed review. Going through the blockers: Escaped URL functions (P2): fixed in 2732769. Attribute values are now limited to letters, digits, spaces and Native resource boundary: two layers now.
probe// For each hostile SVG (BASE = http://127.0.0.1:8765):
let image = NSImage(data: Data(svg.utf8)); image?.isTemplate = true
// draw into a 32×32 NSBitmapImageRep, then keep the run loop alive 3 s for any async loaderOutput: all five decoded (16×16); So icon rendering never reaches the plugin's network path, with or without approval. Real behavior proof: the screenshots render the production menu views in-process with a synthetic plugin. I haven't attached an installed-app capture yet, because a dev build here would load my personal plugin config. If you want that for merge, I'm happy to record one against a clean profile. Maintainer decision: @steipete, this adds an optional @clawsweeper re-review |
|
🦞👀 Re-review progress:
|
clawsweeper
Bot
added
clawsweeper:intake:feature
Rasterize validated inline icons into fixed template bitmaps and reject unbounded canvas dimensions before native decoding. Keep validation in the icon constructor so every caller retains the monogram fallback. Co-authored-by: Shivanath Devinarayanan <1937549+shivanathd@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4fe7af967b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| svg.range(of: "<!DOCTYPE", options: .caseInsensitive) == nil, | ||
| svg.range(of: "<!ENTITY", options: .caseInsensitive) == nil, |
There was a problem hiding this comment.
When an icon declares encoding="UTF-7", < can be written as +ADw-, so an external-DTD payload contains neither literal checked substring; Foundation's XMLParser then decodes it and ProviderPluginIconSVG.isValid returns true for inputs such as +ADw-!DOCTYPE svg SYSTEM .... Because userPluginSVGImage later hands the original bytes to NSImage, this defeats the promised DTD/entity boundary and exposes the native decoder to external-entity/network processing; reject non-UTF-8 declarations or reject DTD/entity declarations from the parser itself.
Useful? React with 👍 / 👎.
# Conflicts: # CHANGELOG.md
|
Merged in ac30c44, thanks @shivanathd! User plugins can now ship a monochrome SVG icon. We kept your allowlist and added a few hardening steps for untrusted SVGs: canvas bounds are validated before decoding and capped, and the icon is rasterized once into a template image with the monogram as fallback. It will ship in the next release. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06b3190728
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if name == "fill" || name == "stroke" { | ||
| let paint = value.trimmingCharacters(in: .whitespaces) | ||
| return paint.range(of: self.paintPattern, options: .regularExpression) != nil | ||
| && paint.range(of: "url", options: .caseInsensitive) == nil |
There was a problem hiding this comment.
url
When a plugin uses the standard SVG color keyword burlywood for fill or stroke, the paint regex accepts it but this substring check rejects it because the name contains url, so the otherwise valid SVG is discarded and the UI shows the monogram instead. The exact paint grammar already prevents url(...), so URL syntax should be rejected without banning substrings inside valid color names.
Useful? React with 👍 / 👎.
…kage (#4653) The 0.78.0 glibc Linux CLI linked libxml2.so.2 again (regression of #1046) because the plugin SVG icon validation from #4608 pulled FoundationXML into Linux builds, so it exited 127 on systems without that library such as Ubuntu 26.04. SVG parsing is now macOS-only (Linux uses the existing monogram fallback), and a new `Scripts/check-linux-cli-linkage.sh` inspects the ELF NEEDED entries and rejects any libxml2 SONAME; Linux CI and both glibc release builds run it. Fixes #4652. Co-authored-by: harjoth <harjoth.khara@gmail.com>
2 participants
Allow user plugins to declare a monochrome SVG icon
Summary
User plugins can only show a 1–3 character monogram on a tint today (
docs/plugins.md: "File/SVG icons are notsupported"), so a plugin can't look like the service it reports on the way built-in providers do. This adds an
optional inline
icon.svgthat the host draws as a template image, exactly like built-in provider icons, in theprovider switcher and on the plugin's menu card. The monogram stays the fallback.
Safety
The SVG is untrusted plugin input, so it is validated with an allowlist (
ProviderPluginIconSVG) before it reachesNSImage:<svg>;svg, g, path, circle, ellipse, rect, line, polyline, polygon, title, desconly;.,#%+-(), so no CSS escapes, comments, declarations, quotes,colons or slashes, and no
urlin any spelling;fill/strokemust be a colour keyword,#hexor numericrgb()/rgba();xmlnsmust be the SVG namespace;<style>,<image>/raster,<foreignObject>, links (href), event handlers, CDATA,processing instructions, DOCTYPE or entities (external entities are never resolved).
An SVG that fails validation is dropped and the monogram is shown; the plugin itself still loads (an invalid icon
is cosmetic, so it doesn't fail the manifest).
Independently of the sanitizer, I checked the native decoder: on macOS 27,
NSImage(data:)decoding and drawing SVGswith
url()paint, an escapedu\72l(),<image href>,<use xlink:href>and<style>@importpointing at a localHTTP server made no requests at all (server log empty). The sanitizer doesn't rely on that.
Changes
CodexBarCore/Plugins/ProviderPluginIconSVG.swift(new): the allowlist validator (FoundationXML on Linux).ProviderPluginManifest.swift:ProviderPluginIcon.svg, parsed fromicon.svg, validated, elsenil.codexbar-plugin.d.ts:icon?: { monogram?; tint?; svg? }with the limits documented.StatusItemController+UserPlugins.swift: switcher icon uses the SVG as a template image; the card header drawsit in white on the tint; monogram unchanged when there's no SVG.
StatusItemController+MenuLocalization.swift: the SVG joins the menu signature so editing it refreshes the menu.docs/plugins.md,CHANGELOG.md.Tests / commands run
swift build --build-tests✔swift test --filter UserProviderPluginTests✔ (28 tests), including the two new ones:plugin svg icon is kept and rendered as a template imageunsafe or oversized svg icons fall back to the monogram(script, style, foreignObject, image href, url(),onload, DOCTYPE/entity, non-svg root, >8 KB, empty), plus the escaped
u\72l(...)bypass from review and red-teamcases: case and full-width variants, character references,
image-set/paint/var,style,use/a/xlink:href/xml:base, foreign namespaces, a stylesheet PI and NUL. Verified red: before the value grammar theescaped and injection cases were accepted.
none,currentColor, names,#hex,rgb()/rgba(), withtransform.make check✔ (0 SwiftLint violations).MenuBarLayoutThroughputTests.swift:82hits the Swift 6.3.3 type-checker time limit; I used anuncommitted local rewrite of that expression to build tests. It is not part of this patch.
Screenshots
Rendered from the production menu views in a test (synthetic "Acme Meter" plugin, no real account data):
icon.svgseticon.svg(monogram fallback)The images live on a separate
pr-proofbranch of my fork, not in this PR. The switcher tab and the card header draw the SVG as a template image; without it the existing monogram is unchanged.🤖 Generated with Claude Code