fix: add timeout, block svgs, prevent rebinding

Author: danielroe

server/api/registry/image-proxy/index.get.ts

@@ -1,7 +1,13 @@
 import { createError, getQuery, setResponseHeaders, sendStream } from 'h3'
 import { Readable } from 'node:stream'
 import { CACHE_MAX_AGE_ONE_DAY } from '#shared/utils/constants'
-import { isAllowedImageUrl } from '#server/utils/image-proxy'
+import { isAllowedImageUrl, resolveAndValidateHost } from '#server/utils/image-proxy'
+
+/** Fetch timeout in milliseconds to prevent slow-drip resource exhaustion */
+const FETCH_TIMEOUT_MS = 15_000
+
+/** Maximum image size in bytes (10 MB) */
+const MAX_SIZE = 10 * 1024 * 1024
 
 /**
  * Image proxy endpoint to prevent privacy leaks from README images.
@@ -28,14 +34,23 @@ export default defineEventHandler(async event => {
     })
   }
 
-  // Validate URL
+  // Validate URL syntactically
   if (!isAllowedImageUrl(url)) {
     throw createError({
       statusCode: 400,
       message: 'Invalid or disallowed image URL.',
     })
   }
 
+  // Resolve hostname via DNS and validate the resolved IP is not private.
+  // This prevents DNS rebinding attacks where a hostname resolves to a private IP.
+  if (!(await resolveAndValidateHost(url))) {
+    throw createError({
+      statusCode: 400,
+      message: 'Invalid or disallowed image URL.',
+    })
+  }
+
   try {
     const response = await fetch(url, {
       headers: {
@@ -44,6 +59,7 @@ export default defineEventHandler(async event => {
         'Accept': 'image/*',
       },
       redirect: 'follow',
+      signal: AbortSignal.timeout(FETCH_TIMEOUT_MS),
     })
 
     // Validate final URL after any redirects to prevent SSRF bypass
@@ -54,6 +70,14 @@ export default defineEventHandler(async event => {
       })
     }
 
+    // Also validate the resolved IP of the redirect target
+    if (response.url !== url && !(await resolveAndValidateHost(response.url))) {
+      throw createError({
+        statusCode: 400,
+        message: 'Redirect to disallowed URL.',
+      })
+    }
+
     if (!response.ok) {
       throw createError({
         statusCode: response.status === 404 ? 404 : 502,
@@ -63,16 +87,16 @@ export default defineEventHandler(async event => {
 
     const contentType = response.headers.get('content-type') || 'application/octet-stream'
 
-    // Only allow image content types
-    if (!contentType.startsWith('image/')) {
+    // Only allow raster/vector image content types, but block SVG to prevent
+    // embedded JavaScript execution (SVGs can contain <script> tags, event handlers, etc.)
+    if (!contentType.startsWith('image/') || contentType.includes('svg')) {
       throw createError({
         statusCode: 400,
-        message: 'URL does not point to an image.',
+        message: 'URL does not point to an allowed image type.',
       })
     }
 
     // Check Content-Length header if present (may be absent or dishonest)
-    const MAX_SIZE = 10 * 1024 * 1024 // 10 MB
     const contentLength = response.headers.get('content-length')
     if (contentLength && Number.parseInt(contentLength, 10) > MAX_SIZE) {
       throw createError({
@@ -97,13 +121,14 @@ export default defineEventHandler(async event => {
     })
 
     // Stream the response with a size limit to prevent memory exhaustion.
-    // This avoids buffering the entire image into memory before sending.
+    // Uses pipe-based backpressure so the upstream pauses when the consumer is slow.
     let bytesRead = 0
     // eslint-disable-next-line @typescript-eslint/no-explicit-any
     const upstream = Readable.fromWeb(response.body as any)
     const limited = new Readable({
       read() {
-        /* pulling is driven by upstream push */
+        // Resume the upstream when the consumer is ready for more data
+        upstream.resume()
       },
     })
 
@@ -113,7 +138,11 @@ export default defineEventHandler(async event => {
         upstream.destroy()
         limited.destroy(new Error('Image too large'))
       } else {
-        limited.push(chunk)
+        // Respect backpressure: if push() returns false, pause the upstream
+        // until the consumer calls read() again
+        if (!limited.push(chunk)) {
+          upstream.pause()
+        }
       }
     })
     upstream.on('end', () => limited.push(null))

server/utils/image-proxy.ts

@@ -4,6 +4,7 @@
  * Resolves: https://github.com/npmx-dev/npmx.dev/issues/1138
  */
 
+import { lookup } from 'node:dns/promises'
 import ipaddr from 'ipaddr.js'
 
 /** Trusted image domains that don't need proxying (first-party or well-known CDNs) */
@@ -56,6 +57,17 @@ export function isTrustedImageDomain(url: string): boolean {
   )
 }
 
+/**
+ * Check if a resolved IP address is in a private/reserved range.
+ * Uses ipaddr.js for comprehensive IPv4, IPv6, and IPv4-mapped IPv6 range detection.
+ */
+function isPrivateIP(ip: string): boolean {
+  const bare = ip.startsWith('[') && ip.endsWith(']') ? ip.slice(1, -1) : ip
+  if (!ipaddr.isValid(bare)) return false
+  const addr = ipaddr.process(bare)
+  return addr.range() !== 'unicast'
+}
+
 /**
  * Validate that a URL is a valid HTTP(S) image URL suitable for proxying.
  * Blocks private/reserved IPs (SSRF protection) using ipaddr.js for comprehensive
@@ -80,15 +92,41 @@ export function isAllowedImageUrl(url: string): boolean {
   // For IP addresses, use ipaddr.js to check against all reserved ranges
   // (loopback, private RFC 1918, link-local 169.254, IPv6 ULA fc00::/7, etc.)
   // ipaddr.process() also unwraps IPv4-mapped IPv6 (e.g. ::ffff:127.0.0.1 → 127.0.0.1)
+  if (isPrivateIP(hostname)) {
+    return false
+  }
+
+  return true
+}
+
+/**
+ * Resolve the hostname of a URL via DNS and validate that all resolved IPs are
+ * public unicast addresses. This prevents DNS rebinding SSRF attacks where a
+ * hostname passes the initial string-based check but resolves to a private IP.
+ *
+ * Returns true if the hostname resolves to a safe (unicast) IP.
+ * Returns false if any resolved IP is private/reserved, or if resolution fails.
+ */
+export async function resolveAndValidateHost(url: string): Promise<boolean> {
+  const parsed = URL.parse(url)
+  if (!parsed) return false
+
+  const hostname = parsed.hostname.toLowerCase()
+
+  // If it's already an IP literal, skip DNS resolution (already validated by isAllowedImageUrl)
   const bare = hostname.startsWith('[') && hostname.endsWith(']') ? hostname.slice(1, -1) : hostname
   if (ipaddr.isValid(bare)) {
-    const addr = ipaddr.process(bare)
-    if (addr.range() !== 'unicast') {
-      return false
-    }
+    return !isPrivateIP(bare)
   }
 
-  return true
+  try {
+    // Resolve to check all returned IPs
+    const { address } = await lookup(hostname)
+    return !isPrivateIP(address)
+  } catch {
+    // DNS resolution failed — block the request
+    return false
+  }
 }
 
 /**

test/unit/server/utils/image-proxy.spec.ts

@@ -3,6 +3,7 @@ import {
   isTrustedImageDomain,
   isAllowedImageUrl,
   toProxiedImageUrl,
+  resolveAndValidateHost,
 } from '../../../../server/utils/image-proxy'
 
 describe('Image Proxy Utils', () => {
@@ -113,11 +114,49 @@ describe('Image Proxy Utils', () => {
       expect(isAllowedImageUrl('http://[fd12::1]/image.png')).toBe(false)
     })
 
+    it('blocks 0.0.0.0 (unspecified address)', () => {
+      expect(isAllowedImageUrl('http://0.0.0.0/image.png')).toBe(false)
+    })
+
     it('returns false for invalid URLs', () => {
       expect(isAllowedImageUrl('not-a-url')).toBe(false)
     })
   })
 
+  describe('resolveAndValidateHost', () => {
+    it('allows URLs with publicly-resolvable hostnames', async () => {
+      // example.com resolves to a public IP
+      expect(await resolveAndValidateHost('https://example.com/image.png')).toBe(true)
+    })
+
+    it('blocks URLs with hostnames that resolve to loopback', async () => {
+      // localhost resolves to 127.0.0.1
+      expect(await resolveAndValidateHost('http://localhost/image.png')).toBe(false)
+    })
+
+    it('blocks IP literals that are private', async () => {
+      expect(await resolveAndValidateHost('http://127.0.0.1/image.png')).toBe(false)
+      expect(await resolveAndValidateHost('http://10.0.0.1/image.png')).toBe(false)
+    })
+
+    it('allows IP literals that are public', async () => {
+      // 93.184.215.14 is example.com's IP
+      expect(await resolveAndValidateHost('http://93.184.215.14/image.png')).toBe(true)
+    })
+
+    it('blocks hostnames that fail DNS resolution', async () => {
+      expect(
+        await resolveAndValidateHost(
+          'http://this-domain-definitely-does-not-exist.invalid/img.png',
+        ),
+      ).toBe(false)
+    })
+
+    it('returns false for invalid URLs', async () => {
+      expect(await resolveAndValidateHost('not-a-url')).toBe(false)
+    })
+  })
+
   describe('toProxiedImageUrl', () => {
     it('returns trusted URLs as-is', () => {
       const url = 'https://raw.githubusercontent.com/owner/repo/main/image.png'