Add token asset upload flow

[rossgalloway]

Summary

Move the tokenAssets image-upload tooling into the CMS so token editors can prepare and submit logos without leaving the metadata application. Add the token-logo download support from feat/add-token-logos, keep yearn/tokenAssets as the asset source of truth, and expose the new workflow from the tokens list and token detail pages.

This PR is stacked on #142 and should be reviewed after the Next.js migration.

How to review

Start with packages/app/src/routes/TokenAssetUpload.tsx and packages/app/src/lib/tokenAssetUpload.ts for the client workflow. Then review packages/app/src/server/tokenAssets and packages/app/app/api/token-assets for validation, ERC-20 name resolution, and fork-aware GitHub PR creation.

Happy path: open /tokens, select Add token logo, provide a chain/address and SVG, confirm both PNG previews, review the generated PR metadata, and submit with a GitHub-authenticated CMS session. Token detail pages should prefill the upload form.

Test plan

• Automated: bun install --frozen-lockfile
• Automated: bun run --cwd packages/app tslint
• Automated: bun run lint
• Automated: bun run build
• Manual: verified tokens-list navigation and token detail URL prefilling
• Manual: verified SVG upload, 32px/128px PNG generation, multiple assets, and IndexedDB draft restoration
• Manual: verified ERC-20 name lookup against a live Ethereum token
• Manual: verified unauthenticated and malformed upload requests return the expected 401/400 responses
• Manual: create a real tokenAssets PR using a GitHub-authenticated production session

Risk / impact

The upload endpoint uses the signed-in user's GitHub token to create a branch and pull request in yearn/tokenAssets, falling back to the user's fork when direct writes are unavailable. It does not write to the tokenAssets default branch directly. Repository targets and RPC endpoints are configurable through server-only environment variables; rollback is a revert of this PR.

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
cms	Ready	Preview	Jul 31, 2026 10:32pm

[murderteeth]

Summary

Adds a token-asset upload workflow: an Add token logo entry on /tokens, a /tokens/upload page that takes an SVG, renders 32/128px PNGs client-side, drafts to IndexedDB, and opens a PR against yearn/tokenAssets with the signed-in user's GitHub token (fork fallback on 403). Token detail pages gain a logo preview, per-size downloads, and a prefilled link into the form.

Checks on this branch: bun run lint clean (2 pre-existing warnings in untouched files) · tslint clean · bun run build clean · browser-verified happy path (SVG → both PNG previews → correct PR title and path list), prefill works, button correctly scoped to /tokens, no console errors. No package.json changes, so no dependency policy review needed.

Issues

• TokenAssetUpload.tsx:70 (applyUrlParams) — A saved draft is merged with URL params, so one token's artwork can be submitted under another's address. applyUrlParams spreads the restored first item and replaces only chainId/address/name, retaining files, preview, generatePng. Reproduced in-browser: attach an SVG for address A, navigate away, then open /tokens/upload?chain=1&address=<B> (the exact link TokenLogoDownloads builds) — address shows B, A's SVG is still attached, previews render, submit is enabled, and the POST pairs address: 0xbbb… with A's bytes.

  • Change: when any supplied identity component differs from the restored item (compare addresses case-insensitively), build a fresh createTokenAssetItem() and then apply the URL fields — dropping files, preview, name, generatePng.
  • Keep: bare /tokens/upload with no query string must still restore the draft's files and previews; restore must also still happen when the URL identity matches. This is not a removal of draft persistence.
  • Done when: with a draft for A, /tokens/upload?chain=1&address=<B> shows an empty dropzone and disabled submit; bare /tokens/upload still restores A; a matching-identity URL still restores. Cover all three cases with tests.

• TokenAssetUpload.tsx:854 (buildFormData) — Stale manual PNGs are submitted while the UI previews generated ones. item.files.png32 ?? await dataUrlToFile(item.preview.png32 …) always prefers files, but with generatePng true both canSubmit (:103) and the Preview strip read preview. onGeneratePng (:390) only sets the flag and regenerates previews — it never clears files. Reproduced: after toggle-off → manual upload → toggle-on, previews showed the canvas render but the POST carried filename=manual-32.png / manual-128.png.

  • Change: make the selection mode-aware inside buildFormData — use preview.png32/png128 when generatePng is true, files.png32/png128 when false. Prefer this over clearing files in the toggle handler, which would discard the user's manual uploads if they toggle back off.
  • Keep: with the toggle off, files.png32/png128 must still be exactly what is sent, and manual files must survive a toggle round-trip.
  • Done when: a regression test covering off → manual upload → on shows canvas-rendered bytes in the POST, and manual files are still intact and sent after toggling off again.

• upload.ts:60 (readFiles) — SVG content is never validated; only the client-controlled File.type is. PNGs get a signature and 32/128 dimension check; SVG gets nothing, and there is no size cap on any file (PNG dimensions don't bound encoded byte size either). Verified against the running server: a PE executable, an HTML document with <script>alert(document.cookie)</script>, a script-carrying SVG, and a 40 MB payload all passed validation. Compounding this, :107 only checks the Bearer token is non-empty — the token isn't validated against GitHub until getUserLogin at :137, after every file is read and base64-encoded at :116–132, so a junk token reaches the memory work (that is how the 40 MB probe got through).

  • Change: (1) enforce per-file and total byte limits before arrayBuffer(); (2) move token validation ahead of the file-read loop; (3) for the SVG, use a maintained allowlist sanitizer with DTD/entities and external references disabled, rather than a <script>/on*=/javascript: denylist — SVG also carries foreignObject, CSS, external refs, and encoded URLs (disallowing SVG upload entirely is also acceptable if that fits the workflow); (4) cap the item count alongside the size limits, and map GitHub auth failures to a 401 with a safe message rather than echoing error.message verbatim at :154.
  • Keep: validatePng's signature and 32×32 / 128×128 checks must keep returning 400; apply the new size limits before them, not instead of them.
  • Done when: a non-SVG svg_* part returns 400 before any GitHub call, an oversized file or over-limit item count returns 400, an invalid token returns 401 before files are buffered, and valid input still reaches the commit step.

• erc20Name.ts / POST /api/token-assets/erc20-name — Remove this endpoint and its route entirely. It's an unauthenticated, unthrottled relay that drives eth_call through server-configured RPC URLs (potentially paid endpoints in production), and it isn't needed: Yearn runs a public, CORS-enabled RPC proxy at https://rpc.yearn.fi/chain/[chainId] that is suitable for embedding directly in the frontend (verified: answers eth_call with access-control-allow-origin: *).

  • Change: (1) In TokenAssetUpload.tsx's resolveTokenName, replace the fetch('/api/token-assets/erc20-name', …) call with a client-side lookup against https://rpc.yearn.fi/chain/${chainId} — preferably via viem (already a dependency): createPublicClient({ transport: http(url) }) + readContract with the ERC-20 name() ABI, which also replaces the hand-rolled ABI decoding and its bytes32-name / empty-string edge cases. (2) Delete src/server/tokenAssets/erc20Name.ts and app/api/token-assets/erc20-name/route.ts, and drop the RPC_URI_FOR_*/RPC_* env documentation from the README. (3) For the prefill path, pass name in the link TokenLogoDownloads builds — applyUrlParams already accepts it, and the detail page already has token.name from CMS metadata — so the common case needs no RPC call at all.
  • Keep: the same UX contract — resolve on blur only when name is empty, chain is set, and the address passes isEvmAddress; keep the inline "Could not fetch the token name" error state. Do not replace this with a Next.js server action — that recreates the same unauthenticated POST endpoint.
  • Done when: no /api/token-assets/erc20-name route exists, typing a valid chain/address still resolves the name in the form (now via rpc.yearn.fi), the detail-page link prefills the name without any RPC call, and no RPC_* server env vars remain referenced by this feature.

Suggestions

• TokenAssetUpload.tsx:809 (defaultPrMetadata) — the resolved token name never reaches the generated PR, so the tokenAssets reviewer sees a bare hex address. Include each available name alongside chain/address in the PR body (the body is already submitted — no schema change).
• github.ts:141, :209 — the fork fallback selects on String(error).includes('403') / '422', which also matches those digits anywhere in a GitHub response body. Carry the HTTP status on a typed error instead — especially since the fork path is the part of this PR that hasn't been exercised against real GitHub.
• CollectionList.tsx:117 — use a Link to /tokens/upload instead of router.push (prefetch, modifier-click, copyable href); the sibling entry point in Collection.tsx:160 already does.

Verdict

REQUEST_CHANGES — four blocking items. Two are confirmed data-integrity bugs that can silently publish the wrong image to a real address; the third is unvalidated/uncapped upload content; the fourth is an open RPC relay that should be removed in favor of the public rpc.yearn.fi proxy.

Also worth flagging: the PR's own test plan leaves "create a real tokenAssets PR" unchecked, so github.ts — including the fork fallback discussed above — remains unexercised against GitHub by both the author and this review.

---

How This Was Reviewed

This review was conducted using the review-pr skill.

[socket-security]

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff	Package	Supply Chain Security	Vulnerability	Quality	Maintenance	License
	@​types/​sanitize-html@​2.16.1
	sanitize-html@​2.17.6

View full report

[murderteeth]

Summary

Re-review of the revised branch. The four blocking items from the previous round are all fixed and verified — draft/URL identity separation, generated-vs-manual PNG selection, SVG validation with size and item caps, and removal of the unauthenticated RPC endpoint. Lint, typecheck, tests (8/8), and build all pass, and the new sanitize-html dependency is approved (server-only, no client bundle impact).

One new issue was introduced by the SVG fix itself.

Issues

• packages/app/src/server/tokenAssets/upload.ts:149 (sanitizeSvg) — The sanitized bytes are what gets committed, so legitimate artwork is silently rewritten and the request still returns 200. The uploader is never told the published file differs from the one they submitted. Running real logos through the handler:

  Input (inside <svg viewBox="0 0 24 24">)	Committed output
  <circle …/><text x="12" y="17" font-size="14" text-anchor="middle">Y</text>	<text> stripped; the letter leaks out as a bare text node — …</circle>Y</svg>
  <rect width="24" height="24" style="fill:#0657f9;stroke:#000"/>	<rect width="24" height="24"></rect> — all colour lost
  <style>.a{fill:#0657f9}</style><rect class="a" width="24" height="24"/>	<rect width="24" height="24"></rect> — colour lost
  <path fill-rule="evenodd" clip-rule="evenodd" …/>	fill-rule kept, clip-rule dropped

  The output is well-formed XML, so this is silent corruption rather than a parse failure. text and tspan are absent from SVG_ALLOWED_TAGS, and no font or text-positioning attributes appear in SVG_ALLOWED_ATTRIBUTES.

  The sharpest consequence is cross-file: regenerateTokenPngs renders the PNGs from the original uploaded File and buildTokenAssetFormData ships those alongside the original SVG, but only the SVG is sanitized server-side — so logo.svg and logo-32.png / logo-128.png in the same PR can disagree visually, and a PNG diff won't make that obvious to a reviewer.

  • Change: make sanitization fail-loud instead of silent — when the sanitizer drops a tag or attribute, return a 400 naming what was removed so the uploader can fix the file. Detect removals structurally (compare parsed nodes/attributes, or have the transformTags hook record what it deletes); a raw string comparison will not work, because sanitize-html normalizes harmless syntax such as <rect/> into <rect></rect> and would flag every file. Separately, widen the allowlist to cover ordinary logo content: text and tspan as tags, plus x, y, dx, dy, font-size, font-family, font-weight, text-anchor, dominant-baseline, and clip-rule as attributes.
  • Keep: every existing rejection must still hold — the script / onload / evil.test assertions in upload.test.ts, the DTD and entity check, allowedSchemes: [], and the url(#…) / href="#…" internal-reference restriction. In particular do not add style, class, or the <style> element to the allowlist: allowedSchemes does not apply to CSS, so inline CSS would pass through essentially unfiltered, and a CSS-escaped \75 rl(https://evil.test/a) evades the existing /url\s*\(/i check in the transformTags hook. CSS-styled logos should hit the new 400 and be converted to presentation attributes, not be admitted.
  • Done when: a logo containing <text> is committed faithfully; a logo styled with style/class is rejected with a message saying what was removed rather than silently altered; no SVG is ever committed in a form the uploader did not see; and the existing sanitizer tests still pass.

Verdict

REQUEST_CHANGES — a correctness defect in asset publishing, not a security issue.

---

How This Was Reviewed

This review was conducted using the review-pr skill.

Superseded by the re-review of 2026-07-31. All four blocking items raised here have since been fixed and verified, and the chainId allowlist suggestion was withdrawn as incorrect. Please work from the newer review, which has one remaining item.

[murderteeth]

Summary

Re-reviewed at 8b4610a. The blocking item from the previous review — sanitizeSvg silently rewriting legitimate artwork — is resolved.

text and tspan are now allowed along with their positioning and font attributes, clip-rule is allowed, and the new findUnsupportedSvgContent pre-pass rejects with a 400 naming the exact construct rather than quietly altering the file. style, class, and <style> were correctly left out of the allowlist — admitting inline CSS would have reopened the external-reference surface that allowedSchemes: [] closes.

Checks: lint clean · tsc clean · 13/13 tests · build clean.

Verification

I ran these through the handler independently of the test suite:

Case	Before	At 8b4610a
<text>Y</text> ticker logo	letter silently gutted, 200	committed faithfully, attributes intact
clip-rule	silently dropped	preserved
style="fill:…"	silent colour loss, 200	400 … (attributes: style on <rect>)
<style> + class	silent colour loss, 200	400 … (tags: <style>; attributes: class on <rect>)
<script>	silently stripped, 200	400 … (tags: <script>)
onload=	silently stripped, 200	400 … (attributes: onload on <svg>)
fill="url(https://evil.test/a)"	silently stripped, 200	400 … (attributes: fill on <path>)
fill="\75 rl(https://evil.test/a)"	bypassed the url( guard	400 — closed
foreignObject / <image> / <animate>	silently stripped, 200	all 400
DTD / entity declarations	rejected	still rejected
XML prolog, comments	—	dropped harmlessly, logo committed correctly

Verdict

APPROVE

---

How This Was Reviewed

This review was conducted using the review-pr skill.