fix(evm): validate NFT media/metadata URLs before fetching#1278
Closed
0xApotheosis wants to merge 1 commit into
Closed
fix(evm): validate NFT media/metadata URLs before fetching#12780xApotheosis wants to merge 1 commit into
0xApotheosis wants to merge 1 commit into
Conversation
The token metadata endpoint fetches URLs taken from attacker-controllable NFT metadata (image/animation_url, and on-chain tokenURI() in the blockbook path) with no scheme or destination validation and default redirect-following, which is an unauthenticated SSRF primitive. - Add assertSafeOutboundUrl: allow only http(s), resolve the host, and reject private/loopback/link-local/CGNAT and cloud-metadata destinations. - Validate before fetching and set maxRedirects: 0 so a public host can't 3xx-bounce to an internal one (moralisService live path + blockbookService). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR hardens token and NFT metadata fetches against SSRF attacks. It introduces ChangesSSRF Protection for Metadata Fetches
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@node/coinstacks/common/api/src/utils.ts`:
- Around line 92-99: isPrivateIPv6 currently only recognizes IPv4-mapped IPv6
addresses in dotted-decimal form and ignores hex-form mappings like
::ffff:7f00:1, letting private IPv4 targets slip through; update the
isPrivateIPv6 function to detect IPv4-mapped hex forms (e.g. ::ffff:hhhh:hhhh or
::ffff:hhhh) as well as the dotted-decimal form, decode the final 32 bits from
hex to a dotted IPv4 string, then call isPrivateIPv4(mappedIPv4) as you do for
the dotted form; use a regex that captures both ::ffff:IPv4 and ::ffff:hex[:hex]
variants and convert the hex groups into the four octets before delegating to
isPrivateIPv4.
- Around line 106-126: assertSafeOutboundUrl currently resolves and validates
DNS but the actual HTTP requests in blockbookService (axiosNoRetry.get called
with hexUrl/strUrl) and moralisService (axiosNoRetry.head called with mediaUrl)
still use the original hostname and can be re-resolved at connect time; fix by
binding the validated resolution to the socket: after calling
assertSafeOutboundUrl(resolve) obtain the resolved IP(s) and either (A) replace
the request URL host with the chosen IP literal and set the Host header (and
ensure TLS SNI remains the original hostname for https requests) or (B) supply a
custom http(s).Agent to axiosNoRetry that implements a deterministic lookup()
returning the validated IP address(es) so the same IPs are used at connect time;
apply this change where axiosNoRetry.get(hexUrl/strUrl, { maxRedirects: 0 }) and
axiosNoRetry.head(mediaUrl, { maxRedirects: 0 }) are invoked in blockbookService
and moralisService.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 46753942-3bcf-47f4-9eee-64c996393db9
📒 Files selected for processing (3)
node/coinstacks/common/api/src/evm/blockbookService.tsnode/coinstacks/common/api/src/evm/moralisService.tsnode/coinstacks/common/api/src/utils.ts
Comment on lines
+92
to
+99
| const isPrivateIPv6 = (ip: string): boolean => { | ||
| const v = ip.toLowerCase() | ||
| if (v === '::1' || v === '::') return true // loopback / unspecified | ||
| if (v.startsWith('fe80:')) return true // link-local | ||
| if (v.startsWith('fc') || v.startsWith('fd')) return true // fc00::/7 unique-local | ||
| const mapped = v.match(/^::ffff:(\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3})$/) // IPv4-mapped | ||
| if (mapped) return isPrivateIPv4(mapped[1]) | ||
| return false |
There was a problem hiding this comment.
Reject hex-form IPv4-mapped IPv6 literals.
Line 97 only catches ::ffff:127.0.0.1-style mapped addresses. Valid literals like ::ffff:7f00:1 still decode to loopback/private IPv4 targets, but isPrivateIPv6() currently treats them as public, so this guard can still be bypassed with a direct IPv6 literal or AAAA answer.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@node/coinstacks/common/api/src/utils.ts` around lines 92 - 99, isPrivateIPv6
currently only recognizes IPv4-mapped IPv6 addresses in dotted-decimal form and
ignores hex-form mappings like ::ffff:7f00:1, letting private IPv4 targets slip
through; update the isPrivateIPv6 function to detect IPv4-mapped hex forms (e.g.
::ffff:hhhh:hhhh or ::ffff:hhhh) as well as the dotted-decimal form, decode the
final 32 bits from hex to a dotted IPv4 string, then call
isPrivateIPv4(mappedIPv4) as you do for the dotted form; use a regex that
captures both ::ffff:IPv4 and ::ffff:hex[:hex] variants and convert the hex
groups into the four octets before delegating to isPrivateIPv4.
Comment on lines
+106
to
+126
| export const assertSafeOutboundUrl = async (rawUrl: string): Promise<void> => { | ||
| let url: URL | ||
| try { | ||
| url = new URL(rawUrl) | ||
| } catch { | ||
| throw new ApiError('Bad Request', 400, `invalid outbound url: ${rawUrl}`) | ||
| } | ||
|
|
||
| if (url.protocol !== 'https:' && url.protocol !== 'http:') { | ||
| throw new ApiError('Bad Request', 400, `unsupported outbound url scheme: ${url.protocol}`) | ||
| } | ||
|
|
||
| const host = url.hostname.replace(/^\[|\]$/g, '') // strip brackets from IPv6 literals | ||
|
|
||
| const addresses = isIP(host) ? [host] : (await dns.lookup(host, { all: true })).map((r) => r.address) | ||
|
|
||
| for (const address of addresses) { | ||
| const blocked = isIP(address) === 6 ? isPrivateIPv6(address) : isPrivateIPv4(address) | ||
| if (blocked) throw new ApiError('Bad Request', 400, `blocked outbound url host: ${host}`) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Locate usages of assertSafeOutboundUrl
rg -n "assertSafeOutboundUrl" -S . || true
# 2) Inspect the referenced files
for f in node/coinstacks/common/api/src/evm/blockbookService.ts node/coinstacks/common/api/src/evm/moralisService.ts node/coinstacks/common/api/src/utils.ts; do
echo "----- $f -----"
if [ -f "$f" ]; then
wc -l "$f"
sed -n '1,220p' "$f" | nl -ba | sed -n '1,220p'
else
echo "Missing: $f"
fi
done
# 3) Search for Axios/URL/agent usage near the call sites
for f in node/coinstacks/common/api/src/evm/blockbookService.ts node/coinstacks/common/api/src/evm/moralisService.ts; do
echo "----- Axios-related in $f -----"
rg -n "axios\.|Axios|http(s)?Agent|dns|lookup|hostname|host:|connect|net\.|lookup:" "$f" -S || true
doneRepository: shapeshift/unchained
Length of output: 1319
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show relevant sections around the call sites
for f in node/coinstacks/common/api/src/utils.ts \
node/coinstacks/common/api/src/evm/blockbookService.ts \
node/coinstacks/common/api/src/evm/moralisService.ts; do
echo "===== $f ====="
wc -l "$f"
done
echo "===== blockbookService.ts: around assertSafeOutboundUrl calls ====="
sed -n '760,920p' node/coinstacks/common/api/src/evm/blockbookService.ts | cat -n
echo "===== moralisService.ts: around assertSafeOutboundUrl calls ====="
sed -n '720,860p' node/coinstacks/common/api/src/evm/moralisService.ts | cat -n
echo "===== utils.ts: assertSafeOutboundUrl ====="
sed -n '70,170p' node/coinstacks/common/api/src/utils.ts | cat -n
echo "===== blockbookService.ts: axios creation/config near top ====="
sed -n '1,120p' node/coinstacks/common/api/src/evm/blockbookService.ts | cat -n
echo "===== moralisService.ts: axios creation/config near top ====="
sed -n '1,120p' node/coinstacks/common/api/src/evm/moralisService.ts | cat -n
# Also find axios invocation arguments near those call sites
echo "===== blockbookService.ts: axios usage ====="
rg -n "axios|createAxiosRetry|http[s]?Agent|lookup|hostname|host:|url:" node/coinstacks/common/api/src/evm/blockbookService.ts -S
echo "===== moralisService.ts: axios usage ====="
rg -n "axios|createAxiosRetry|http[s]?Agent|lookup|hostname|host:|url:" node/coinstacks/common/api/src/evm/moralisService.ts -SRepository: shapeshift/unchained
Length of output: 27124
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "httpsAgent|httpAgent|agent:|lookup:|createConnection|dns\.lookup|family:" node/coinstacks/common/api/src/evm/blockbookService.ts node/coinstacks/common/api/src/evm/moralisService.ts node/coinstacks/common/api/src/utils.ts -S || true
# Also scan axiosNoRetry.get/head call option objects for agent-related keys
rg -n "axiosNoRetry\.(get|head)\([^)]*,\s*\{[^}]*\}" node/coinstacks/common/api/src/evm/blockbookService.ts node/coinstacks/common/api/src/evm/moralisService.ts -S || trueRepository: shapeshift/unchained
Length of output: 776
Bind DNS validation to the outbound connection
assertSafeOutboundUrl()performs a DNS lookup and rejects private/loopback destinations, butblockbookServicestill callsaxiosNoRetry.get(hexUrl/strUrl, { maxRedirects: 0 })andaxiosNoRetry.head(mediaUrl, { maxRedirects: 0 })using the original hostname (e.g.,blockbookService.tsaround lines 831/841/857);moralisServicedoes the same formediaUrl(around line 795).- These
axiosNoRetryclients are created with only atimeoutand no customhttp(s)Agent/DNSlookup, so the hostname can be re-resolved at socket connect time (DNS rebinding window).maxRedirects: 0only prevents redirect-based SSRF, not re-DNS. - Enforce the resolved address set at request time (e.g., pin to an IP literal and set
Host/SNI as needed, or use an agent with a deterministic DNS lookup).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@node/coinstacks/common/api/src/utils.ts` around lines 106 - 126,
assertSafeOutboundUrl currently resolves and validates DNS but the actual HTTP
requests in blockbookService (axiosNoRetry.get called with hexUrl/strUrl) and
moralisService (axiosNoRetry.head called with mediaUrl) still use the original
hostname and can be re-resolved at connect time; fix by binding the validated
resolution to the socket: after calling assertSafeOutboundUrl(resolve) obtain
the resolved IP(s) and either (A) replace the request URL host with the chosen
IP literal and set the Host header (and ensure TLS SNI remains the original
hostname for https requests) or (B) supply a custom http(s).Agent to
axiosNoRetry that implements a deterministic lookup() returning the validated IP
address(es) so the same IPs are used at connect time; apply this change where
axiosNoRetry.get(hexUrl/strUrl, { maxRedirects: 0 }) and
axiosNoRetry.head(mediaUrl, { maxRedirects: 0 }) are invoked in blockbookService
and moralisService.
0xApotheosis
changed the title
Member
Author
|
Superseded by #1280 (branch renamed). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
GET /api/v1/metadata/tokeninfers a media type by fetching a URL taken straight from NFT metadata (image/animation_url, and the on-chaintokenURI()in the blockbook path). Those URLs are unvalidated and redirects are followed by default, so the server can end up making requests to unintended hosts.assertSafeOutboundUrlincommon/api/src/utils.ts: allow onlyhttp(s), resolve the host, and reject private/loopback/link-local/CGNAT and metadata-service ranges.maxRedirects: 0.Notes
maxRedirects: 0means a media URL that legitimately redirects now resolves to nomedia.typeinstead of following the hop. That field is already best-effort (errors are swallowed toundefined), so this is a minor, safe degradation.Verification
Didn't run build/tests locally per the repo CLAUDE.md. It's a pre-fetch URL check plus disabling redirects; no change to well-formed public media URLs that respond directly.