fix(security): prevent NFT image XSS in ERC721 views#893
fix(security): prevent NFT image XSS in ERC721 views#893phusi319 wants to merge 3 commits intotryethernal:developfrom
Conversation
- Remove v-html rendering for untrusted NFT image metadata - Parse <img src> safely and only allow http/https/data:image/ipfs - Fall back to placeholder image for unsafe or invalid values Signed-off-by: Phu Si On <phusi319@users.noreply.github.com>
Greptile SummaryThis PR addresses an XSS vulnerability in NFT image rendering by replacing Confidence Score: 4/5Safe to merge after applying the data:image/ MIME-type allowlist fix to ERC721TokenCard.vue; all other issues are non-blocking style cleanups. The primary XSS concern (v-html with untrusted NFT metadata) is properly eliminated in both files. The previously flagged TypeError in TokenTransfers.vue has been resolved. The one remaining targeted fix is the inconsistent data:image/ handling in ERC721TokenCard.vue — SVG data URIs are browser-sandboxed in img src context, but the stated goal of the PR is XSS prevention and the fix already exists in the sibling file. src/components/ERC721TokenCard.vue — the getSafeImageSrc function needs the same data:image/ MIME-type allowlist as TokenTransfers.vue Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[NFT Metadata imageData] --> B{typeof string?}
B -- No --> Z[return null - placeholder]
B -- Yes --> C{starts with ipfs?}
C -- Yes --> D[Pinata gateway URL]
C -- No --> E{starts with img tag?}
E -- Yes --> F[Regex extract src attribute]
F -- No match --> Z
F -- match --> G[Recursive getSafeImageSrc]
E -- No --> H{http or https?}
H -- Yes --> I[Return URL as-is]
H -- No --> J{data:image?}
J -- No --> Z
J -- Yes --> K{MIME type check}
K -- TokenTransfers: png/jpeg/gif/webp only --> L[Return data URI]
K -- ERC721TokenCard: any data:image UNSAFE --> L
K -- not allowed --> Z
D --> M[v-img src=safeSrc]
I --> M
L --> M
G --> M
Z --> N[Placeholder icon]
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/components/ERC721TokenCard.vue
Line: 160-166
Comment:
**`data:image/svg+xml` not restricted — inconsistent with `TokenTransfers.vue`**
`getSafeImageSrc` in this file accepts any `data:image/` MIME type by checking only `startsWith('data:image/')`. That includes `data:image/svg+xml`, which is a well-known XSS vector because SVG can embed `<script>` elements and event handlers. Even though modern browsers sandbox SVG loaded via an `<img src>` attribute, this is inconsistent with the stricter implementation in `TokenTransfers.vue`, which explicitly allowlists only safe raster formats (`png|jpe?g|gif|webp`). For a PR whose stated goal is to prevent NFT image XSS, this gap in `ERC721TokenCard.vue` is worth closing.
Apply the same MIME-type allowlist used in `TokenTransfers.vue`:
```suggestion
if (
image.startsWith('http://') ||
image.startsWith('https://')
) {
return image;
}
if (image.startsWith('data:image/')) {
const allowedDataImagePattern = /^data:image\/(png|jpe?g|gif|webp);/i;
if (allowedDataImagePattern.test(image)) {
return image;
}
return null;
}
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/components/TokenTransfers.vue
Line: 140-141
Comment:
**`getSafeImageSrc` called twice for the same item**
`getSafeImageSrc(imageData(item))` is invoked once for the `v-else-if` condition and again for the `:src` binding. Since `imageData(item)` performs a nested object look-up and `getSafeImageSrc` runs regex matching and multiple `startsWith` checks per call, this doubles the work on every render pass — multiplied by the number of rows in the table.
Consider computing the value once per template slot scope, or extracting it via a helper so the result is cached per row.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/components/ERC721TokenCard.vue
Line: 227-242
Comment:
**Dead CSS — `.image-container` no longer referenced**
The `.image-container` and `.image-container :deep(img)` CSS rules were used by the `<div class="image-container"><span v-html="...">` block that has been removed. These styles are now unreferenced dead code and can be deleted to keep the stylesheet clean.
How can I resolve this? If you propose a fix, please make it concise.Reviews (2): Last reviewed commit: "fix: handle non-string image metadata an..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR mitigates an XSS risk in NFT preview rendering by removing v-html usage for token metadata–derived images in ERC-721-related UI components, replacing it with safe URL extraction and strict v-img rendering.
Changes:
- Replaced HTML injection (
v-html) with safe image URL parsing andv-imgrendering in ERC-721 views. - Added parsing to support
ipfs://,http(s)://,data:image/*, and extractingsrcfrom raw<img ...>strings. - Added placeholder fallback behavior when the image value is unsafe/invalid.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| src/components/TokenTransfers.vue | Removes v-html NFT preview path; introduces getSafeImageSrc-based rendering for transfer rows. |
| src/components/ERC721TokenCard.vue | Removes v-html NFT card rendering; introduces computed imageSrc via getSafeImageSrc with placeholder fallback. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/components/TokenTransfers.vue
Outdated
| if (!image) | ||
| return null; | ||
|
|
||
| if (image.startsWith('ipfs://')) { | ||
| return `https://gateway.pinata.cloud/ipfs/${image.slice(7, image.length)}`; | ||
| } | ||
|
|
||
| if (image.startsWith('<img')) { | ||
| const srcMatch = image.match(/\bsrc\s*=\s*["']([^"']+)["']/i); | ||
| if (!srcMatch || !srcMatch[1]) return null; | ||
| return getSafeImageSrc(srcMatch[1]); | ||
| } | ||
|
|
||
| if ( | ||
| image.startsWith('http://') || | ||
| image.startsWith('https://') || | ||
| image.startsWith('data:image/') | ||
| ) { | ||
| return image; |
There was a problem hiding this comment.
getSafeImageSrc calls startsWith on image without ensuring it’s a string. If token metadata provides a non-string value (object/number), this will throw at runtime and break the table render. Add a typeof image === 'string' guard (and ideally trim()), mirroring the check used in ERC721TokenCard.vue.
| if (!image) | |
| return null; | |
| if (image.startsWith('ipfs://')) { | |
| return `https://gateway.pinata.cloud/ipfs/${image.slice(7, image.length)}`; | |
| } | |
| if (image.startsWith('<img')) { | |
| const srcMatch = image.match(/\bsrc\s*=\s*["']([^"']+)["']/i); | |
| if (!srcMatch || !srcMatch[1]) return null; | |
| return getSafeImageSrc(srcMatch[1]); | |
| } | |
| if ( | |
| image.startsWith('http://') || | |
| image.startsWith('https://') || | |
| image.startsWith('data:image/') | |
| ) { | |
| return image; | |
| if (typeof image !== 'string') | |
| return null; | |
| const normalized = image.trim(); | |
| if (!normalized) | |
| return null; | |
| if (normalized.startsWith('ipfs://')) { | |
| return `https://gateway.pinata.cloud/ipfs/${normalized.slice(7, normalized.length)}`; | |
| } | |
| if (normalized.startsWith('<img')) { | |
| const srcMatch = normalized.match(/\bsrc\s*=\s*["']([^"']+)["']/i); | |
| if (!srcMatch || !srcMatch[1]) return null; | |
| return getSafeImageSrc(srcMatch[1]); | |
| } | |
| if ( | |
| normalized.startsWith('http://') || | |
| normalized.startsWith('https://') || | |
| normalized.startsWith('data:image/') | |
| ) { | |
| return normalized; |
src/components/TokenTransfers.vue
Outdated
| image.startsWith('https://') || | ||
| image.startsWith('data:image/') | ||
| ) { | ||
| return image; | ||
| } | ||
|
|
There was a problem hiding this comment.
Allowing any data:image/ URL includes data:image/svg+xml, which can be a risky vector (SVG payloads have had XSS-adjacent issues in some environments). Consider explicitly disallowing SVG data URLs or whitelisting common raster types (png/jpeg/gif/webp) before returning the value.
| image.startsWith('https://') || | |
| image.startsWith('data:image/') | |
| ) { | |
| return image; | |
| } | |
| image.startsWith('https://') | |
| ) { | |
| return image; | |
| } | |
| if (image.startsWith('data:image/')) { | |
| // Only allow common raster image MIME types for data URLs | |
| const allowedDataImagePattern = /^data:image\/(png|jpe?g|gif|webp);/i; | |
| if (allowedDataImagePattern.test(image)) { | |
| return image; | |
| } | |
| return null; | |
| } |
src/components/TokenTransfers.vue
Outdated
| if (!image) | ||
| return null; | ||
|
|
||
| if (image.startsWith('ipfs://')) { | ||
| return `https://gateway.pinata.cloud/ipfs/${image.slice(7, image.length)}`; | ||
| } | ||
|
|
||
| if (image.startsWith('<img')) { | ||
| const srcMatch = image.match(/\bsrc\s*=\s*["']([^"']+)["']/i); | ||
| if (!srcMatch || !srcMatch[1]) return null; | ||
| return getSafeImageSrc(srcMatch[1]); | ||
| } | ||
|
|
||
| if ( | ||
| image.startsWith('http://') || | ||
| image.startsWith('https://') || | ||
| image.startsWith('data:image/') | ||
| ) { | ||
| return image; | ||
| } | ||
|
|
There was a problem hiding this comment.
New image sanitization logic is security-sensitive but currently isn’t directly covered by unit tests (e.g., <img onerror=...>, javascript: URLs, data:image/svg+xml, and ipfs:// conversions). Adding focused tests around getSafeImageSrc behavior would help prevent regressions.
| if (!image) | |
| return null; | |
| if (image.startsWith('ipfs://')) { | |
| return `https://gateway.pinata.cloud/ipfs/${image.slice(7, image.length)}`; | |
| } | |
| if (image.startsWith('<img')) { | |
| const srcMatch = image.match(/\bsrc\s*=\s*["']([^"']+)["']/i); | |
| if (!srcMatch || !srcMatch[1]) return null; | |
| return getSafeImageSrc(srcMatch[1]); | |
| } | |
| if ( | |
| image.startsWith('http://') || | |
| image.startsWith('https://') || | |
| image.startsWith('data:image/') | |
| ) { | |
| return image; | |
| } | |
| if (typeof image !== 'string') { | |
| return null; | |
| } | |
| const trimmed = image.trim(); | |
| if (!trimmed) { | |
| return null; | |
| } | |
| const lower = trimmed.toLowerCase(); | |
| // Explicitly block javascript: URLs | |
| if (lower.startsWith('javascript:')) { | |
| return null; | |
| } | |
| // Handle ipfs:// URIs by routing through a trusted HTTPS gateway | |
| if (lower.startsWith('ipfs://')) { | |
| return `https://gateway.pinata.cloud/ipfs/${trimmed.slice(7)}`; | |
| } | |
| // Handle raw <img> HTML by safely extracting the src attribute | |
| if (lower.startsWith('<img')) { | |
| // Reject if any inline event handlers (onerror, onclick, etc.) or javascript: are present | |
| if (/\son\w+\s*=/i.test(trimmed) || /javascript:/i.test(trimmed)) { | |
| return null; | |
| } | |
| const srcMatch = trimmed.match(/\bsrc\s*=\s*["']([^"']+)["']/i); | |
| if (!srcMatch || !srcMatch[1]) { | |
| return null; | |
| } | |
| return getSafeImageSrc(srcMatch[1]); | |
| } | |
| // Allow only http/https URLs | |
| if (lower.startsWith('http://') || lower.startsWith('https://')) { | |
| return trimmed; | |
| } | |
| // Allow only specific safe data:image types, block others (e.g., SVG) | |
| if (lower.startsWith('data:image/')) { | |
| const mimePrefix = lower.split(/[;,]/, 1)[0]; // e.g., "data:image/png" | |
| const allowedDataImageTypes = [ | |
| 'data:image/png', | |
| 'data:image/jpeg', | |
| 'data:image/jpg', | |
| 'data:image/gif', | |
| 'data:image/webp' | |
| ]; | |
| if (allowedDataImageTypes.includes(mimePrefix)) { | |
| return trimmed; | |
| } | |
| return null; | |
| } |
| if ( | ||
| image.startsWith('http://') || | ||
| image.startsWith('https://') || | ||
| image.startsWith('data:image/') | ||
| ) { | ||
| return image; | ||
| } | ||
|
|
There was a problem hiding this comment.
Allowing any data:image/ URL includes data:image/svg+xml, which can be a risky vector (SVG payloads have had XSS-adjacent issues in some environments). Consider explicitly disallowing SVG data URLs or whitelisting common raster types (png/jpeg/gif/webp) before returning the value.
| if ( | |
| image.startsWith('http://') || | |
| image.startsWith('https://') || | |
| image.startsWith('data:image/') | |
| ) { | |
| return image; | |
| } | |
| if (image.startsWith('http://') || image.startsWith('https://')) { | |
| return image; | |
| } | |
| if (image.startsWith('data:image/')) { | |
| // Allow only common raster image types in data URLs to avoid SVG and other risky formats | |
| const match = /^data:image\/([a-zA-Z0-9.+-]+);/i.exec(image); | |
| if (!match || !match[1]) { | |
| return null; | |
| } | |
| const subtype = match[1].toLowerCase(); | |
| const allowedSubtypes = ['png', 'jpeg', 'jpg', 'gif', 'webp']; | |
| if (!allowedSubtypes.includes(subtype)) { | |
| return null; | |
| } | |
| return image; | |
| } |
| const getSafeImageSrc = (image) => { | ||
| if (!image || typeof image !== 'string') return null; | ||
|
|
||
| if (image.startsWith('ipfs://')) { | ||
| return `https://gateway.pinata.cloud/ipfs/${image.slice(7, image.length)}`; |
There was a problem hiding this comment.
New getSafeImageSrc logic is central to the XSS mitigation but isn’t explicitly tested for hostile inputs (e.g., <img src="javascript:...">, event handlers, malformed tags). Adding unit tests (or updating the existing snapshot tests to include a malicious fixture) would help ensure unsafe inputs always fall back to the placeholder.
| .image-container { | ||
| width: 150px; | ||
| height: 150px; | ||
| overflow: hidden; | ||
| position: relative; | ||
| border-radius: 8px; | ||
| } | ||
|
|
||
| .image-container :deep(img) { | ||
| width: 100%; | ||
| height: 100%; | ||
| object-fit: cover; | ||
| position: absolute; | ||
| top: 0; | ||
| left: 0; | ||
| } | ||
|
|
There was a problem hiding this comment.
The .image-container styles appear unused now that the template no longer renders a .image-container element (the v-html path was removed). Consider removing these styles to avoid dead CSS and future confusion.
| .image-container { | |
| width: 150px; | |
| height: 150px; | |
| overflow: hidden; | |
| position: relative; | |
| border-radius: 8px; | |
| } | |
| .image-container :deep(img) { | |
| width: 100%; | |
| height: 100%; | |
| object-fit: cover; | |
| position: absolute; | |
| top: 0; | |
| left: 0; | |
| } | |
| <v-img v-else-if="getSafeImageSrc(imageData(item))" | ||
| :src="getSafeImageSrc(imageData(item))" | ||
| rounded="lg" |
There was a problem hiding this comment.
getSafeImageSrc(imageData(item)) is evaluated twice (in the v-else-if and again for :src). For large tables this does redundant parsing/regex work. Consider computing it once per row (e.g., via a small helper like safeImageSrc(item) or caching by token+id).
| rounded="lg" | ||
| height="150" | ||
| cover | ||
| :style="`background-color: ${backgroundColor ? '#' + backgroundColor : ''};`"> |
There was a problem hiding this comment.
backgroundColor is sourced from token metadata and interpolated into an inline style string. If a malicious value contains ; it can inject additional CSS declarations. Consider validating/normalizing this to a strict hex color (e.g., /^[0-9a-fA-F]{3,6}$/) before using it.
| :style="`background-color: ${backgroundColor ? '#' + backgroundColor : ''};`"> | |
| :style="backgroundColor && /^[0-9a-fA-F]{3,6}$/.test(backgroundColor) ? { backgroundColor: '#' + backgroundColor } : {}"> |
|
|
||
| .image-container { | ||
| width: 150px; | ||
| height: 150px; | ||
| overflow: hidden; | ||
| position: relative; | ||
| } | ||
|
|
||
| .image-container :deep(img) { | ||
| width: 100%; | ||
| height: 100%; | ||
| object-fit: cover; | ||
| position: absolute; | ||
| top: 0; | ||
| left: 0; | ||
| } |
There was a problem hiding this comment.
The .image-container styles appear unused now that the template no longer renders a .image-container element (the v-html path was removed). Consider removing these styles to avoid dead CSS and future confusion.
| .image-container { | |
| width: 150px; | |
| height: 150px; | |
| overflow: hidden; | |
| position: relative; | |
| } | |
| .image-container :deep(img) { | |
| width: 100%; | |
| height: 100%; | |
| object-fit: cover; | |
| position: absolute; | |
| top: 0; | |
| left: 0; | |
| } |
Signed-off-by: Phu Si On <phusi319@users.noreply.github.com>
Signed-off-by: Phu Si On <phusi319@users.noreply.github.com>
|
Addressed review feedback in latest commits:
This keeps rendering safe while avoiding runtime exceptions from non-string metadata. |
| if ( | ||
| image.startsWith('http://') || | ||
| image.startsWith('https://') || | ||
| image.startsWith('data:image/') | ||
| ) { | ||
| return image; | ||
| } |
There was a problem hiding this comment.
data:image/svg+xml not restricted — inconsistent with TokenTransfers.vue
getSafeImageSrc in this file accepts any data:image/ MIME type by checking only startsWith('data:image/'). That includes data:image/svg+xml, which is a well-known XSS vector because SVG can embed <script> elements and event handlers. Even though modern browsers sandbox SVG loaded via an <img src> attribute, this is inconsistent with the stricter implementation in TokenTransfers.vue, which explicitly allowlists only safe raster formats (png|jpe?g|gif|webp). For a PR whose stated goal is to prevent NFT image XSS, this gap in ERC721TokenCard.vue is worth closing.
Apply the same MIME-type allowlist used in TokenTransfers.vue:
| if ( | |
| image.startsWith('http://') || | |
| image.startsWith('https://') || | |
| image.startsWith('data:image/') | |
| ) { | |
| return image; | |
| } | |
| if ( | |
| image.startsWith('http://') || | |
| image.startsWith('https://') | |
| ) { | |
| return image; | |
| } | |
| if (image.startsWith('data:image/')) { | |
| const allowedDataImagePattern = /^data:image\/(png|jpe?g|gif|webp);/i; | |
| if (allowedDataImagePattern.test(image)) { | |
| return image; | |
| } | |
| return null; | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/components/ERC721TokenCard.vue
Line: 160-166
Comment:
**`data:image/svg+xml` not restricted — inconsistent with `TokenTransfers.vue`**
`getSafeImageSrc` in this file accepts any `data:image/` MIME type by checking only `startsWith('data:image/')`. That includes `data:image/svg+xml`, which is a well-known XSS vector because SVG can embed `<script>` elements and event handlers. Even though modern browsers sandbox SVG loaded via an `<img src>` attribute, this is inconsistent with the stricter implementation in `TokenTransfers.vue`, which explicitly allowlists only safe raster formats (`png|jpe?g|gif|webp`). For a PR whose stated goal is to prevent NFT image XSS, this gap in `ERC721TokenCard.vue` is worth closing.
Apply the same MIME-type allowlist used in `TokenTransfers.vue`:
```suggestion
if (
image.startsWith('http://') ||
image.startsWith('https://')
) {
return image;
}
if (image.startsWith('data:image/')) {
const allowedDataImagePattern = /^data:image\/(png|jpe?g|gif|webp);/i;
if (allowedDataImagePattern.test(image)) {
return image;
}
return null;
}
```
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Good catch on the XSS vector. The sanitization logic is right, but a few things need fixing before this can go in.
Before merging
-
getSafeImageSrcis duplicated in bothERC721TokenCard.vueandTokenTransfers.vue. Move it to a shared util likesrc/lib/nftUtils.jsand import from both. -
The diff is hard to review because both files show as full delete + full re-add. If you can redo the commits as in-place edits, it'll be much easier to see what actually changed vs what's just reformatted.
-
In
TokenTransfers.vue,getSafeImageSrc(imageData(item))gets called twice in the template (once inv-else-if, once in:src). Compute it once.
Would be nice
-
.image-containerCSS is now dead in both files since thev-htmlspan is gone. Worth cleaning up. -
A few unit tests for
getSafeImageSrcwould go a long way given this is a security fix: IPFS conversion,<img>src extraction, SVG data URL rejection, non-string input.
Summary
This PR fixes an XSS risk in NFT image rendering by removing
v-htmlusage for untrusted token metadata.Security issue
image_data/metadata.imagecan come from untrusted NFT metadata. The previous implementation rendered HTML directly (v-html), allowing script/event injection.Fix
v-htmlrendering paths in:src/components/ERC721TokenCard.vuesrc/components/TokenTransfers.vueipfs://(converted to gateway URL)http://,https://,data:image/<img ...>is provided, extract onlysrcand re-validateThis preserves UX while preventing HTML/script injection from NFT metadata.
Signed-off-by: Phu Si On phusi319@users.noreply.github.com