Skip to content

fix: sanitize untrusted external metadata links#935

Open
afurm wants to merge 2 commits intosolana-foundation:masterfrom
afurm:af/fix-external-links
Open

fix: sanitize untrusted external metadata links#935
afurm wants to merge 2 commits intosolana-foundation:masterfrom
afurm:af/fix-external-links

Conversation

@afurm
Copy link
Copy Markdown

@afurm afurm commented Apr 9, 2026

Description

Sanitize untrusted external URLs before rendering them as clickable links in account and NFT views.

This change fixes a user-facing security issue where third-party metadata values such as token websites, NFT external_url, token metadata URIs, compressed NFT links, validator config websites, and verified-build repository URLs could be rendered directly into href attributes. Unsafe or malformed values now render as plain text instead of clickable links.

It also fixes a separate bug in the token mint overview where the "Bridged Asset Contract" row incorrectly linked to the bridge contract URL instead of the asset contract URL.

Type of change

  • Bug fix
  • New feature
  • Protocol integration
  • Documentation update
  • Other (please describe):

Screenshots

N/A — no layout or visual design changes.

Testing

  • Added unit tests for the shared external URL sanitizer
  • Added regression tests covering:
    • unsafe javascript: website values not rendering as clickable links
    • bridged asset contract rows using the correct asset contract URL
  • Ran:
    • pnpm exec eslint app/shared/lib/safe-external-url.ts app/shared/lib/__tests__/safe-external-url.spec.ts app/components/account/__tests__/TokenAccountSection.spec.tsx app/components/account/TokenAccountSection.tsx app/components/account/CompressedNftCard.tsx app/components/account/ConfigAccountSection.tsx app/components/common/NFTArt.tsx app/components/account/VerifiedBuildCard.tsx
    • pnpm exec vitest --project specs run app/shared/lib/__tests__/safe-external-url.spec.ts app/components/account/__tests__/TokenAccountSection.spec.tsx

Related Issues

No matching public issue found.
No overlapping open PR found for this fix in the fork or the upstream repository during triage.

Checklist

  • My code follows the project's style guidelines
  • I have added tests that prove my fix/feature works
  • All tests pass locally and in CI
  • I have updated documentation as needed
  • I have run build:info script to update build information
  • CI/CD checks pass
  • I have included screenshots for protocol screens (if applicable)
  • For security-related features, I have included links to related information

Additional Notes

  • This is a defensive hardening change for metadata-driven external links.
  • No protocol-specific behavior was changed beyond preventing unsafe URLs from becoming clickable.
  • Focused local tests passed. Vitest still emits an existing Next.js SWC lockfile warning in the current local environment, but the targeted test run completed successfully.

@vercel
Copy link
Copy Markdown

vercel bot commented Apr 9, 2026

@afurm is attempting to deploy a commit to the Solana Foundation Team on Vercel.

A member of the Team first needs to authorize it.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Apr 9, 2026

Greptile Summary

This PR introduces a getSafeExternalUrl utility that allows only http: and https: protocol URLs, and threads it through six components (CompressedNftCard, ConfigAccountSection, TokenAccountSection, NFTArt, VerifiedBuildCard) to prevent javascript: and other unsafe schemes from being rendered as clickable href values. It also fixes a separate pre-existing bug where the "Bridged Asset Contract" row was linking to the bridge contract URL instead of the asset contract URL. The implementation is clean and the tests adequately cover the primary attack vectors.

Confidence Score: 5/5

Safe to merge — all remaining findings are P2 style suggestions that do not affect correctness or security.

The security fix is correctly implemented: getSafeExternalUrl uses new URL() to parse and then allowlists only http:/https: protocols, which is the correct approach for blocking javascript:, data:, and other dangerous URI schemes. All six affected components have been updated consistently, and the bridged-asset-contract href bug is fixed with a regression test. The only open items are a P2 note about native IPFS/Arweave URIs silently losing the VIEW ORIGINAL link, and a minor suggestion to return url.href instead of the raw trimmed input.

No files require special attention for merging; the P2 note on app/components/common/NFTArt.tsx regarding IPFS/Arweave URIs is worth a follow-up discussion but is not a blocker.

Vulnerabilities

This PR is a direct security hardening change. Prior to this PR, untrusted third-party metadata values (token websites, NFT external_url, validator config websites, verified-build repo URLs, and NFT image source URIs) could be placed verbatim into href attributes, enabling javascript: URI execution and other non-http protocol injection. The new getSafeExternalUrl utility closes that vector by allowlisting only http: and https: protocols via new URL() parsing, which also guards against URL-encoded and obfuscated variants. No new attack surfaces are introduced by this PR. The data: and mailto: protocols are correctly rejected by the tests.

Important Files Changed

Filename Overview
app/shared/lib/safe-external-url.ts New URL-sanitizer utility; correctly validates protocol via new URL() allowlist; returns the trimmed input rather than url.href (intentional, minor)
app/shared/lib/tests/safe-external-url.spec.ts Unit tests cover http/https acceptance, dangerous protocols (javascript:, data:, mailto:), relative paths, empty/undefined inputs, and the type-guard export
app/components/common/NFTArt.tsx Replaces next/link with <a> and gates rendering behind getSafeExternalUrl; NFTs with native ipfs:// or ar:// URIs will silently lose the "VIEW ORIGINAL" link
app/components/account/TokenAccountSection.tsx Adds URL sanitization for token website, bridge contract, asset contract, and NFT external URL; also fixes the bridged-asset-contract href bug
app/components/account/tests/TokenAccountSection.spec.tsx Regression tests verify javascript: website is not rendered as a link, and the bridged-asset row uses the correct asset contract URL
app/components/account/VerifiedBuildCard.tsx Adds URL sanitization for repository URL via getSafeExternalUrl inside the DisplayType.URL switch case
app/components/account/CompressedNftCard.tsx Adds URL sanitization for external_url field; falls back to plain text with a - placeholder when the URL is unsafe or absent
app/components/account/ConfigAccountSection.tsx Adds URL sanitization for validator website; unsafe values shown as plain text

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Untrusted external URL\nfrom metadata] --> B[getSafeExternalUrl]
    B --> C{typeof value\n=== 'string'?}
    C -- No --> D[return null]
    C -- Yes --> E{trimmedValue\nempty?}
    E -- Yes --> D
    E -- No --> F[new URL parse]
    F --> G{Parse\nsucceeds?}
    G -- No --> D
    G -- Yes --> H{protocol is\nhttp: or https:?}
    H -- No --> D
    H -- Yes --> I[return trimmedValue]
    I --> J[Render as <a href=...>]
    D --> K[Render as plain text]
Loading

Reviews (1): Last reviewed commit: "fix unsafe external links" | Re-trigger Greptile

Comment thread app/components/common/NFTArt.tsx
Comment thread app/shared/lib/safe-external-url.ts Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant