fix(security): narrow SSRF scope to allow RFC1918 LAN addresses

NOMAD is a LAN appliance — blocking RFC1918 private ranges (10.x,
172.16-31.x, 192.168.x) would prevent users from downloading content
from local network mirrors. Narrowed to only block loopback (localhost,
127.x, 0.0.0.0, ::1) and link-local (169.254.x, fe80::) addresses.
Restored require_tld: false for LAN hostnames without TLDs.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Author: 2 people

admin/app/validators/common.ts

@@ -1,39 +1,39 @@
 import vine from '@vinejs/vine'
 
 /**
- * Checks whether a URL points to a private/internal network address.
- * Used to prevent SSRF — the server should not fetch from localhost,
- * private RFC1918 ranges, link-local, or cloud metadata endpoints.
+ * Checks whether a URL points to a loopback or link-local address.
+ * Used to prevent SSRF — the server should not fetch from localhost
+ * or link-local/metadata endpoints (e.g. cloud instance metadata at 169.254.x.x).
  *
- * Throws an error if the URL is internal/private.
+ * RFC1918 private ranges (10.x, 172.16-31.x, 192.168.x) are intentionally
+ * ALLOWED because NOMAD is a LAN appliance and users may host content
+ * mirrors on their local network.
+ *
+ * Throws an error if the URL is a loopback or link-local address.
  */
 export function assertNotPrivateUrl(urlString: string): void {
   const parsed = new URL(urlString)
   const hostname = parsed.hostname.toLowerCase()
 
-  const privatePatterns = [
+  const blockedPatterns = [
     /^localhost$/,
     /^127\.\d+\.\d+\.\d+$/,
-    /^10\.\d+\.\d+\.\d+$/,
-    /^172\.(1[6-9]|2\d|3[01])\.\d+\.\d+$/,
-    /^192\.168\.\d+\.\d+$/,
-    /^169\.254\.\d+\.\d+$/, // Link-local / cloud metadata
     /^0\.0\.0\.0$/,
+    /^169\.254\.\d+\.\d+$/, // Link-local / cloud metadata
     /^\[::1\]$/,
-    /^\[?fe80:/i,
-    /^\[?fd[0-9a-f]{2}:/i, // Unique local IPv6
+    /^\[?fe80:/i, // IPv6 link-local
   ]
 
-  if (privatePatterns.some((re) => re.test(hostname))) {
-    throw new Error(`Download URL must not point to a private/internal address: ${hostname}`)
+  if (blockedPatterns.some((re) => re.test(hostname))) {
+    throw new Error(`Download URL must not point to a loopback or link-local address: ${hostname}`)
   }
 }
 
 export const remoteDownloadValidator = vine.compile(
   vine.object({
     url: vine
       .string()
-      .url()
+      .url({ require_tld: false }) // Allow LAN URLs (e.g. http://my-nas:8080/file.zim)
       .trim(),
   })
 )
@@ -42,7 +42,7 @@ export const remoteDownloadWithMetadataValidator = vine.compile(
   vine.object({
     url: vine
       .string()
-      .url()
+      .url({ require_tld: false }) // Allow LAN URLs
       .trim(),
     metadata: vine
       .object({
@@ -59,7 +59,7 @@ export const remoteDownloadValidatorOptional = vine.compile(
   vine.object({
     url: vine
       .string()
-      .url()
+      .url({ require_tld: false }) // Allow LAN URLs
       .trim()
       .optional(),
   })
@@ -97,7 +97,7 @@ const resourceUpdateInfoBase = vine.object({
   resource_type: vine.enum(['zim', 'map'] as const),
   installed_version: vine.string().trim(),
   latest_version: vine.string().trim().minLength(1),
-  download_url: vine.string().url().trim(),
+  download_url: vine.string().url({ require_tld: false }).trim(),
 })
 
 export const applyContentUpdateValidator = vine.compile(resourceUpdateInfoBase)

admin/docs/security-audit-v1.md

@@ -109,47 +109,29 @@ if (!fullPath.startsWith(path.resolve(basePath))) {
 
 ### 4. SSRF — Download Endpoints (HIGH)
 
-**File:** `admin/app/validators/common.ts:3-12`
+**File:** `admin/app/validators/common.ts`
 **Endpoints:** `POST /api/zim/download-remote`, `POST /api/maps/download-remote`, `POST /api/maps/download-base-assets`, `POST /api/maps/download-remote-preflight`
 
-The URL validator uses `require_tld: false`, allowing internal/private URLs:
-
-```typescript
-url: vine.string().url({ require_tld: false })
-```
-
-An attacker on the LAN can make NOMAD fetch from:
+The download endpoints accept user-supplied URLs and the server fetches from them. Without validation, an attacker on the LAN (or via CSRF since `shield.ts` disables CSRF protection) could make NOMAD fetch from co-located services:
 - `http://localhost:3306` (MySQL)
 - `http://localhost:6379` (Redis)
 - `http://169.254.169.254/` (cloud metadata — if NOMAD is ever cloud-hosted)
-- Any internal network service
 
-**Fix:** Add a URL validation helper that rejects private/internal IPs:
+**Fix:** Added `assertNotPrivateUrl()` that blocks loopback and link-local addresses before any download is initiated. Called in all download controllers.
+
+**Scope note:** RFC1918 private addresses (10.x, 172.16-31.x, 192.168.x) are intentionally **allowed** because NOMAD is a LAN appliance and users may host content mirrors on their local network. The `require_tld: false` VineJS option is preserved so URLs like `http://my-nas:8080/file.zim` remain valid.
 
 ```typescript
-function isPrivateUrl(urlString: string): boolean {
-  const url = new URL(urlString)
-  const hostname = url.hostname
-  // Block localhost, private ranges, link-local, metadata
-  const blocked = [
-    /^localhost$/i,
-    /^127\./,
-    /^10\./,
-    /^172\.(1[6-9]|2\d|3[01])\./,
-    /^192\.168\./,
-    /^169\.254\./,
-    /^0\./,
-    /^\[::1\]$/,
-    /^\[fe80:/i,
-  ]
-  return blocked.some(re => re.test(hostname))
-}
+const blockedPatterns = [
+  /^localhost$/,
+  /^127\.\d+\.\d+\.\d+$/,
+  /^0\.0\.0\.0$/,
+  /^169\.254\.\d+\.\d+$/,  // Link-local / cloud metadata
+  /^\[::1\]$/,
+  /^\[?fe80:/i,             // IPv6 link-local
+]
 ```
 
-Alternatively, maintain an allowlist of known-good download domains (e.g., `download.kiwix.org`, `github.com`, `api.projectnomad.us`).
-
-**Note:** The `require_tld: false` setting was intentionally added with the comment "Allow local URLs" — but this should only be needed for dev/testing. Consider making it configurable via environment variable, or keeping the allowlist approach for production.
-
 ---
 
 ## FIX AFTER LAUNCH (Medium Priority)
@@ -186,7 +168,7 @@ The `updateSetting` endpoint validates the key against an enum, but `getSetting`
 
 The `download_url` comes directly from the client request body. An attacker can supply any URL and NOMAD will download from it. The URL should be looked up server-side from the content manifest instead.
 
-**Fix:** Validate `download_url` against the cached manifest, or apply the same SSRF protections as finding #4.
+**Fix:** Validate `download_url` against the cached manifest, or apply the same loopback/link-local protections as finding #4 (already applied in this PR).
 
 ---