fix: add CSP headers to metadata proxy to prevent SVG XSS#919
Conversation
|
@askov is attempting to deploy a commit to the Solana Foundation Team on Vercel. A member of the Team first needs to authorize it. |
Greptile SummaryThis PR closes an XSS vulnerability in the metadata proxy ( The fix adds two security headers to all successful proxy responses:
The policy is minimal and well-reasoned: Confidence Score: 5/5This PR is safe to merge — it adds strictly defensive headers with no functional regressions and the prior frame-ancestors concern is already addressed. The change is purely additive (two response headers), targets a real demonstrated XSS vulnerability, uses correct and minimal CSP directives for the use case, and the previously raised frame-ancestors none concern has been incorporated. No P0 or P1 issues remain. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant Browser
participant Proxy as "/api/metadata/proxy"
participant Upstream as "Upstream (IPFS / HTTP)"
Browser->>Proxy: "GET /api/metadata/proxy?uri=..."
Proxy->>Upstream: "fetch(uri)"
Upstream-->>Proxy: "200 OK (SVG / JSON / Binary)"
Proxy-->>Browser: "200 OK with CSP + X-Content-Type-Options headers"
Note over Browser: "SVG opened as top-level document: CSP blocks scripts, frame-ancestors blocks clickjacking"
Note over Browser: "SVG loaded via img tag: scripts never execute anyway"
Reviews (2): Last reviewed commit: "fix: update CSP in metadata proxy for fr..." | Re-trigger Greptile |
|
@greptile-apps review please |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| "default-src 'none'; style-src 'unsafe-inline'; img-src data:; frame-ancestors 'none'", | ||
| 'Content-Type': resourceHeaders.get('content-type') ?? 'application/json; charset=utf-8', | ||
| Etag: resourceHeaders.get('etag') ?? 'no-etag', | ||
| 'X-Content-Type-Options': 'nosniff', |
There was a problem hiding this comment.
Let's maybe extract headers for the security cases into SECURITY_HEADERS const for better observability.
| // Prevent proxied content (e.g. SVG with embedded scripts) from executing | ||
| // anything if the proxy URL is opened directly as a top-level document. | ||
| 'Content-Security-Policy': | ||
| "default-src 'none'; style-src 'unsafe-inline'; img-src data:; frame-ancestors 'none'", |
There was a problem hiding this comment.
| "default-src 'none'; style-src 'unsafe-inline'; img-src data:; frame-ancestors 'none'", | |
| "default-src 'none'; style-src 'unsafe-inline'; img-src data:; frame-ancestors 'none'; sandbox", |
I'd suggest adding sandbox headers as well to isolate the potentially harmful content from knowing where it is rendered
There was a problem hiding this comment.
That is a good one, thanks
| 'Cache-Control': resourceHeaders.get('cache-control') ?? 'no-cache', | ||
| // Prevent proxied content (e.g. SVG with embedded scripts) from executing | ||
| // anything if the proxy URL is opened directly as a top-level document. | ||
| 'Content-Security-Policy': |
There was a problem hiding this comment.
| 'Content-Security-Policy': | |
| 'Content-Disposition': 'attachment', | |
| 'Content-Security-Policy': | |
| "default-src 'none'; frame-ancestors 'none'; sandbox; |
It seems we can add 'Content-Disposition': 'attachment' header to the image and remove style-src and img-src headers. They'll cover cases with rendering for SVG's with malformed data
There was a problem hiding this comment.
I'm not sure it's worth it. All cases seem to be covered already, and most proxies I've encountered don't include 'attachment.' When they do, it can be a bit annoying because it makes debugging the response more difficult.
1c1e9e8 to
cf8b69b
Compare
|
Partially unblocks #853 |
Description
/api/metadata/proxy?uri=...). An attacker-controlled SVG on IPFS, when navigated to directly (not via<img>), executes scripts on the Explorer's origin.Content-Security-Policy: default-src 'none'; style-src 'unsafe-inline'; img-src data:andX-Content-Type-Options: nosniffto all proxy responses, blocking script execution while preserving cosmetic SVGrendering.
Type of change
Security references
cookies/localStorage)
Testing
NEXT_PUBLIC_METADATA_ENABLED=truescript executes (vulnerable state)
<img>tagsRelated Issues
Closes HOO-339
Checklist