diff --git a/package.json b/package.json index 1299d72d5..2aaa826e0 100644 --- a/package.json +++ b/package.json @@ -183,6 +183,7 @@ "file-type": "^21.3.0", "grammy": "^1.39.3", "hono": "4.11.4", + "ipaddr.js": "^2.2.0", "jiti": "^2.6.1", "json5": "^2.2.3", "jszip": "^3.10.1", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index d1c55dd8d..f78d4fb54 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -112,6 +112,9 @@ importers: hono: specifier: 4.11.4 version: 4.11.4 + ipaddr.js: + specifier: ^2.2.0 + version: 2.3.0 jiti: specifier: ^2.6.1 version: 2.6.1 @@ -3863,6 +3866,10 @@ packages: resolution: {integrity: sha512-0KI/607xoxSToH7GjN1FfSbLoU0+btTicjsQSWQlh/hZykN8KpmMf7uYwPW3R+akZ6R/w18ZlXSHBYXiYUPO3g==} engines: {node: '>= 0.10'} + ipaddr.js@2.3.0: + resolution: {integrity: sha512-Zv/pA+ciVFbCSBBjGfaKUya/CcGmUHzTydLMaTwrUUEM2DIEO3iZvueGxmacvmN50fGpGVKeTXpb2LcYQxeVdg==} + engines: {node: '>= 10'} + ipull@3.9.3: resolution: {integrity: sha512-ZMkxaopfwKHwmEuGDYx7giNBdLxbHbRCWcQVA1D2eqE4crUguupfxej6s7UqbidYEwT69dkyumYkY8DPHIxF9g==} engines: {node: '>=18.0.0'} @@ -9936,6 +9943,8 @@ snapshots: ipaddr.js@1.9.1: {} + ipaddr.js@2.3.0: {} + ipull@3.9.3: dependencies: '@tinyhttp/content-disposition': 2.2.2 diff --git a/src/agents/cli-credentials.ts b/src/agents/cli-credentials.ts index 5ca5629ff..bb77e9134 100644 --- a/src/agents/cli-credentials.ts +++ b/src/agents/cli-credentials.ts @@ -1,4 +1,4 @@ -import { execSync } from "node:child_process"; +import { execSync, spawnSync } from "node:child_process"; import { createHash } from "node:crypto"; import fs from "node:fs"; import path from "node:path"; @@ -78,6 +78,37 @@ type ClaudeCliWriteOptions = ClaudeCliFileOptions & { type ExecSyncFn = typeof execSync; +// Secure keychain operations using spawnSync to prevent shell injection +function secureKeychainFind(service: string, account?: string): string | null { + const args = ["find-generic-password", "-s", service]; + if (account) { + args.push("-a", account); + } + args.push("-w"); + const result = spawnSync("security", args, { + encoding: "utf8", + timeout: 5000, + stdio: ["pipe", "pipe", "pipe"], + }); + if (result.status !== 0 || result.error) { + return null; + } + return result.stdout?.trim() ?? null; +} + +function secureKeychainWrite(service: string, account: string, value: string): boolean { + const result = spawnSync( + "security", + ["add-generic-password", "-U", "-s", service, "-a", account, "-w", value], + { + encoding: "utf8", + timeout: 5000, + stdio: ["pipe", "pipe", "pipe"], + }, + ); + return result.status === 0 && !result.error; +} + function resolveClaudeCliCredentialsPath(homeDir?: string) { const baseDir = homeDir ?? resolveUserPath("~"); return path.join(baseDir, CLAUDE_CLI_CREDENTIALS_RELATIVE_PATH); @@ -113,20 +144,22 @@ function readCodexKeychainCredentials(options?: { }): CodexCliCredential | null { const platform = options?.platform ?? process.platform; if (platform !== "darwin") return null; - const execSyncImpl = options?.execSync ?? execSync; + // Note: execSync option preserved for test mocking but secure helper used by default + const _execSyncImpl = options?.execSync; const codexHome = resolveCodexHomePath(); const account = computeCodexKeychainAccount(codexHome); try { - const secret = execSyncImpl( - `security find-generic-password -s "Codex Auth" -a "${account}" -w`, - { - encoding: "utf8", - timeout: 5000, - stdio: ["pipe", "pipe", "pipe"], - }, - ).trim(); + // Use secure helper to prevent shell injection + const secret = _execSyncImpl + ? _execSyncImpl(`security find-generic-password -s "Codex Auth" -a "${account}" -w`, { + encoding: "utf8", + timeout: 5000, + stdio: ["pipe", "pipe", "pipe"], + }).trim() + : secureKeychainFind("Codex Auth", account); + if (!secret) return null; const parsed = JSON.parse(secret) as Record; const tokens = parsed.tokens as Record | undefined; @@ -186,16 +219,19 @@ function readQwenCliCredentials(options?: { homeDir?: string }): QwenCliCredenti }; } -function readClaudeCliKeychainCredentials( - execSyncImpl: ExecSyncFn = execSync, -): ClaudeCliCredential | null { +function readClaudeCliKeychainCredentials(execSyncImpl?: ExecSyncFn): ClaudeCliCredential | null { try { - const result = execSyncImpl( - `security find-generic-password -s "${CLAUDE_CLI_KEYCHAIN_SERVICE}" -w`, - { encoding: "utf8", timeout: 5000, stdio: ["pipe", "pipe", "pipe"] }, - ); + // Use secure helper to prevent shell injection (unless test mock provided) + const result = execSyncImpl + ? execSyncImpl(`security find-generic-password -s "${CLAUDE_CLI_KEYCHAIN_SERVICE}" -w`, { + encoding: "utf8", + timeout: 5000, + stdio: ["pipe", "pipe", "pipe"], + }) + : secureKeychainFind(CLAUDE_CLI_KEYCHAIN_SERVICE); + if (!result) return null; - const data = JSON.parse(result.trim()); + const data = JSON.parse(typeof result === "string" ? result.trim() : result); const claudeOauth = data?.claudeAiOauth; if (!claudeOauth || typeof claudeOauth !== "object") return null; @@ -311,14 +347,20 @@ export function writeClaudeCliKeychainCredentials( newCredentials: OAuthCredentials, options?: { execSync?: ExecSyncFn }, ): boolean { - const execSyncImpl = options?.execSync ?? execSync; + const execSyncImpl = options?.execSync; try { - const existingResult = execSyncImpl( - `security find-generic-password -s "${CLAUDE_CLI_KEYCHAIN_SERVICE}" -w 2>/dev/null`, - { encoding: "utf8", timeout: 5000, stdio: ["pipe", "pipe", "pipe"] }, - ); + // Use secure helper to prevent shell injection (unless test mock provided) + const existingResult = execSyncImpl + ? execSyncImpl( + `security find-generic-password -s "${CLAUDE_CLI_KEYCHAIN_SERVICE}" -w 2>/dev/null`, + { encoding: "utf8", timeout: 5000, stdio: ["pipe", "pipe", "pipe"] }, + ) + : secureKeychainFind(CLAUDE_CLI_KEYCHAIN_SERVICE); + if (!existingResult) return false; - const existingData = JSON.parse(existingResult.trim()); + const existingData = JSON.parse( + typeof existingResult === "string" ? existingResult.trim() : existingResult, + ); const existingOauth = existingData?.claudeAiOauth; if (!existingOauth || typeof existingOauth !== "object") { return false; @@ -333,10 +375,23 @@ export function writeClaudeCliKeychainCredentials( const newValue = JSON.stringify(existingData); - execSyncImpl( - `security add-generic-password -U -s "${CLAUDE_CLI_KEYCHAIN_SERVICE}" -a "${CLAUDE_CLI_KEYCHAIN_ACCOUNT}" -w '${newValue.replace(/'/g, "'\"'\"'")}'`, - { encoding: "utf8", timeout: 5000, stdio: ["pipe", "pipe", "pipe"] }, - ); + // Use secure helper for write (unless test mock provided) + if (execSyncImpl) { + execSyncImpl( + `security add-generic-password -U -s "${CLAUDE_CLI_KEYCHAIN_SERVICE}" -a "${CLAUDE_CLI_KEYCHAIN_ACCOUNT}" -w '${newValue.replace(/'/g, "'\"'\"'")}'`, + { encoding: "utf8", timeout: 5000, stdio: ["pipe", "pipe", "pipe"] }, + ); + } else { + const writeOk = secureKeychainWrite( + CLAUDE_CLI_KEYCHAIN_SERVICE, + CLAUDE_CLI_KEYCHAIN_ACCOUNT, + newValue, + ); + if (!writeOk) { + log.warn("failed to write credentials to claude cli keychain via secure helper"); + return false; + } + } log.info("wrote refreshed credentials to claude cli keychain", { expires: new Date(newCredentials.expires).toISOString(), diff --git a/src/gateway/auth.test.ts b/src/gateway/auth.test.ts index 7e1022124..157715781 100644 --- a/src/gateway/auth.test.ts +++ b/src/gateway/auth.test.ts @@ -1,6 +1,11 @@ -import { describe, expect, it } from "vitest"; +import { describe, expect, it, beforeEach } from "vitest"; -import { authorizeGatewayConnect } from "./auth.js"; +import { + authorizeGatewayConnect, + checkRateLimit, + recordAuthFailure, + resetRateLimiter, +} from "./auth.js"; describe("gateway auth", () => { it("does not throw when req is missing socket", async () => { @@ -100,3 +105,85 @@ describe("gateway auth", () => { expect(res.user).toBe("peter"); }); }); + +describe("rate limiting", () => { + const mockReq = (ip: string) => ({ socket: { remoteAddress: ip } }) as never; + + beforeEach(() => { + resetRateLimiter(); + }); + + it("allows requests initially", () => { + const result = checkRateLimit(mockReq("192.0.2.1")); + expect(result.allowed).toBe(true); + expect(result.retryAfterMs).toBeUndefined(); + }); + + it("allows requests after fewer than 5 failures", () => { + const req = mockReq("192.0.2.2"); + for (let i = 0; i < 4; i++) { + recordAuthFailure(req); + } + const result = checkRateLimit(req); + expect(result.allowed).toBe(true); + }); + + it("blocks requests after 5 failures", () => { + const req = mockReq("192.0.2.3"); + for (let i = 0; i < 5; i++) { + recordAuthFailure(req); + } + const result = checkRateLimit(req); + expect(result.allowed).toBe(false); + expect(result.retryAfterMs).toBeGreaterThan(0); + expect(result.retryAfterMs).toBeLessThanOrEqual(60000); + }); + + it("tracks different IPs independently", () => { + const req1 = mockReq("192.0.2.10"); + const req2 = mockReq("192.0.2.20"); + + // Exhaust rate limit for req1 + for (let i = 0; i < 5; i++) { + recordAuthFailure(req1); + } + + // req1 should be blocked + expect(checkRateLimit(req1).allowed).toBe(false); + + // req2 should still be allowed + expect(checkRateLimit(req2).allowed).toBe(true); + }); + + it("uses 'unknown' key when socket is missing", () => { + const reqWithoutSocket = {} as never; + for (let i = 0; i < 5; i++) { + recordAuthFailure(reqWithoutSocket); + } + expect(checkRateLimit(reqWithoutSocket).allowed).toBe(false); + }); + + it("integrates with authorizeGatewayConnect", async () => { + const req = mockReq("192.0.2.100"); + + // Fail auth 5 times + for (let i = 0; i < 5; i++) { + const res = await authorizeGatewayConnect({ + auth: { mode: "token", token: "secret", allowTailscale: false }, + connectAuth: { token: "wrong" }, + req, + }); + expect(res.ok).toBe(false); + expect(res.reason).toBe("token_mismatch"); + } + + // 6th attempt should be rate limited + const res = await authorizeGatewayConnect({ + auth: { mode: "token", token: "secret", allowTailscale: false }, + connectAuth: { token: "correct" }, // Even correct token should be blocked + req, + }); + expect(res.ok).toBe(false); + expect(res.reason).toBe("rate_limited"); + }); +}); diff --git a/src/gateway/auth.ts b/src/gateway/auth.ts index 1adc367a2..1feb90569 100644 --- a/src/gateway/auth.ts +++ b/src/gateway/auth.ts @@ -3,6 +3,76 @@ import type { IncomingMessage } from "node:http"; import type { GatewayAuthConfig, GatewayTailscaleMode } from "../config/config.js"; import { readTailscaleWhoisIdentity, type TailscaleWhoisIdentity } from "../infra/tailscale.js"; import { isTrustedProxyAddress, parseForwardedForClientIp, resolveGatewayClientIp } from "./net.js"; + +// Rate limiting for brute-force protection +type RateLimitEntry = { + failures: number; + lastAttempt: number; + lockedUntil: number; +}; + +const rateLimitMap = new Map(); +const MAX_FAILURES = 5; +const LOCKOUT_MS = 60_000; // 1 minute +const WINDOW_MS = 300_000; // 5 minutes +const CLEANUP_INTERVAL_MS = 60_000; + +// Cleanup stale entries to prevent memory leak +let cleanupInterval: ReturnType | null = null; +function ensureCleanupInterval(): void { + if (cleanupInterval) return; + cleanupInterval = setInterval(() => { + const now = Date.now(); + for (const [key, entry] of rateLimitMap) { + if (now - entry.lastAttempt > WINDOW_MS) { + rateLimitMap.delete(key); + } + } + }, CLEANUP_INTERVAL_MS); + // Don't keep process alive just for cleanup + cleanupInterval.unref?.(); +} + +function getRateLimitKey(req?: IncomingMessage): string { + // Use socket address, not forwarded headers (spoofable) + return req?.socket?.remoteAddress ?? "unknown"; +} + +export function checkRateLimit(req?: IncomingMessage): { allowed: boolean; retryAfterMs?: number } { + ensureCleanupInterval(); + const key = getRateLimitKey(req); + const now = Date.now(); + const entry = rateLimitMap.get(key); + + if (entry && now < entry.lockedUntil) { + return { allowed: false, retryAfterMs: entry.lockedUntil - now }; + } + + return { allowed: true }; +} + +export function recordAuthFailure(req?: IncomingMessage): void { + ensureCleanupInterval(); + const key = getRateLimitKey(req); + const now = Date.now(); + const entry = rateLimitMap.get(key) ?? { failures: 0, lastAttempt: 0, lockedUntil: 0 }; + + entry.failures += 1; + entry.lastAttempt = now; + + if (entry.failures >= MAX_FAILURES) { + entry.lockedUntil = now + LOCKOUT_MS; + entry.failures = 0; // Reset after lockout + } + + rateLimitMap.set(key, entry); +} + +// Test helper +export function resetRateLimiter(): void { + rateLimitMap.clear(); +} + export type ResolvedGatewayAuthMode = "token" | "password"; export type ResolvedGatewayAuth = { @@ -204,6 +274,13 @@ export async function authorizeGatewayConnect(params: { tailscaleWhois?: TailscaleWhoisLookup; }): Promise { const { auth, connectAuth, req, trustedProxies } = params; + + // Rate limit check - prevent brute-force attacks + const rateLimit = checkRateLimit(req); + if (!rateLimit.allowed) { + return { ok: false, reason: "rate_limited" }; + } + const tailscaleWhois = params.tailscaleWhois ?? readTailscaleWhoisIdentity; const localDirect = isLocalDirectRequest(req, trustedProxies); @@ -219,6 +296,14 @@ export async function authorizeGatewayConnect(params: { user: tailscaleCheck.user.login, }; } + // Record auth failure for Tailscale attempts that had user headers but failed verification + // (don't count missing headers as a "failure" since that's just falling through to token/password) + if ( + tailscaleCheck.reason === "tailscale_user_mismatch" || + tailscaleCheck.reason === "tailscale_whois_failed" + ) { + recordAuthFailure(req); + } } if (auth.mode === "token") { @@ -229,6 +314,7 @@ export async function authorizeGatewayConnect(params: { return { ok: false, reason: "token_missing" }; } if (!safeEqual(connectAuth.token, auth.token)) { + recordAuthFailure(req); return { ok: false, reason: "token_mismatch" }; } return { ok: true, method: "token" }; @@ -243,6 +329,7 @@ export async function authorizeGatewayConnect(params: { return { ok: false, reason: "password_missing" }; } if (!safeEqual(password, auth.password)) { + recordAuthFailure(req); return { ok: false, reason: "password_mismatch" }; } return { ok: true, method: "password" }; diff --git a/src/logging/logger.ts b/src/logging/logger.ts index d1c740212..f018234f6 100644 --- a/src/logging/logger.ts +++ b/src/logging/logger.ts @@ -82,7 +82,29 @@ export function isFileLogLevelEnabled(level: LogLevel): boolean { } function buildLogger(settings: ResolvedSettings): TsLogger { - fs.mkdirSync(path.dirname(settings.file), { recursive: true }); + const logDir = path.dirname(settings.file); + // Create log directory with secure permissions (owner-only access) + try { + if (!fs.existsSync(logDir)) { + fs.mkdirSync(logDir, { recursive: true, mode: 0o700 }); + } else { + // Ensure existing dir has secure permissions + fs.chmodSync(logDir, 0o700); + } + } catch (err) { + // Fall back to default umask if chmod fails (e.g., on Windows) + // Log a warning since permissions may not be secure + if (process.platform !== "win32") { + console.warn( + `[clawdbot] Could not set secure permissions on log directory ${logDir}: ${err}`, + ); + } + try { + fs.mkdirSync(logDir, { recursive: true }); + } catch { + // Directory may already exist, ignore + } + } // Clean up stale rolling logs when using a dated log filename. if (isRollingPath(settings.file)) { pruneOldRollingLogs(path.dirname(settings.file)); diff --git a/src/media/fetch.test.ts b/src/media/fetch.test.ts index 46445b1bb..559c80696 100644 --- a/src/media/fetch.test.ts +++ b/src/media/fetch.test.ts @@ -1,6 +1,6 @@ import { describe, expect, it } from "vitest"; -import { fetchRemoteMedia } from "./fetch.js"; +import { fetchRemoteMedia, _isUrlAllowed } from "./fetch.js"; function makeStream(chunks: Uint8Array[]) { return new ReadableStream({ @@ -44,4 +44,128 @@ describe("fetchRemoteMedia", () => { }), ).rejects.toThrow("exceeds maxBytes"); }); + + it("blocks localhost URLs", async () => { + await expect(fetchRemoteMedia({ url: "http://localhost:8080/file" })).rejects.toThrow( + "URL not allowed", + ); + + await expect(fetchRemoteMedia({ url: "http://localhost/file" })).rejects.toThrow( + "URL not allowed", + ); + }); + + it("blocks private IP ranges", async () => { + // 10.x.x.x + await expect(fetchRemoteMedia({ url: "http://10.0.0.1/file" })).rejects.toThrow( + "URL not allowed", + ); + // 172.16.x.x - 172.31.x.x + await expect(fetchRemoteMedia({ url: "http://172.16.0.1/file" })).rejects.toThrow( + "URL not allowed", + ); + // 192.168.x.x + await expect(fetchRemoteMedia({ url: "http://192.168.1.1/file" })).rejects.toThrow( + "URL not allowed", + ); + }); + + it("blocks loopback addresses", async () => { + await expect(fetchRemoteMedia({ url: "http://127.0.0.1/file" })).rejects.toThrow( + "URL not allowed", + ); + await expect(fetchRemoteMedia({ url: "http://127.0.0.5/file" })).rejects.toThrow( + "URL not allowed", + ); + }); + + it("blocks IPv6 loopback", async () => { + await expect(fetchRemoteMedia({ url: "http://[::1]/file" })).rejects.toThrow("URL not allowed"); + }); + + it("blocks link-local addresses (AWS metadata)", async () => { + await expect( + fetchRemoteMedia({ url: "http://169.254.169.254/latest/meta-data/" }), + ).rejects.toThrow("URL not allowed"); + }); + + it("blocks redirects to private IPs", async () => { + const fetchImpl = async () => + new Response(null, { + status: 302, + headers: { location: "http://127.0.0.1/admin" }, + }); + + await expect( + fetchRemoteMedia({ url: "https://example.com/redirect", fetchImpl }), + ).rejects.toThrow("Redirect blocked"); + }); + + it("limits redirect count", async () => { + let redirectCount = 0; + const fetchImpl = async () => { + redirectCount++; + return new Response(null, { + status: 302, + headers: { location: `https://example.com/redirect${redirectCount}` }, + }); + }; + + await expect(fetchRemoteMedia({ url: "https://example.com/start", fetchImpl })).rejects.toThrow( + "Too many redirects", + ); + }); +}); + +describe("isUrlAllowed (SSRF protection)", () => { + it("allows valid external URLs", () => { + expect(_isUrlAllowed("https://example.com/file.jpg")).toBe(true); + expect(_isUrlAllowed("http://cdn.example.org/image.png")).toBe(true); + expect(_isUrlAllowed("https://8.8.8.8/file")).toBe(true); + }); + + it("blocks non-http protocols", () => { + expect(_isUrlAllowed("file:///etc/passwd")).toBe(false); + expect(_isUrlAllowed("ftp://example.com/file")).toBe(false); + expect(_isUrlAllowed("data:text/plain,hello")).toBe(false); + }); + + it("blocks localhost variations", () => { + expect(_isUrlAllowed("http://localhost/file")).toBe(false); + expect(_isUrlAllowed("http://localhost:3000/api")).toBe(false); + expect(_isUrlAllowed("http://sub.localhost/file")).toBe(false); + }); + + it("blocks loopback IPs", () => { + expect(_isUrlAllowed("http://127.0.0.1/file")).toBe(false); + expect(_isUrlAllowed("http://127.0.0.255/file")).toBe(false); + expect(_isUrlAllowed("http://[::1]/file")).toBe(false); + }); + + it("blocks private IP ranges", () => { + // Class A private + expect(_isUrlAllowed("http://10.0.0.1/file")).toBe(false); + expect(_isUrlAllowed("http://10.255.255.255/file")).toBe(false); + // Class B private + expect(_isUrlAllowed("http://172.16.0.1/file")).toBe(false); + expect(_isUrlAllowed("http://172.31.255.255/file")).toBe(false); + // Class C private + expect(_isUrlAllowed("http://192.168.0.1/file")).toBe(false); + expect(_isUrlAllowed("http://192.168.255.255/file")).toBe(false); + }); + + it("blocks link-local addresses", () => { + expect(_isUrlAllowed("http://169.254.169.254/latest/meta-data/")).toBe(false); + expect(_isUrlAllowed("http://169.254.0.1/file")).toBe(false); + }); + + it("blocks unspecified addresses", () => { + expect(_isUrlAllowed("http://0.0.0.0/file")).toBe(false); + expect(_isUrlAllowed("http://[::]/file")).toBe(false); + }); + + it("rejects invalid URLs", () => { + expect(_isUrlAllowed("not-a-url")).toBe(false); + expect(_isUrlAllowed("")).toBe(false); + }); }); diff --git a/src/media/fetch.ts b/src/media/fetch.ts index 727ab7a5d..970fa1794 100644 --- a/src/media/fetch.ts +++ b/src/media/fetch.ts @@ -1,7 +1,59 @@ import path from "node:path"; +import * as ipaddr from "ipaddr.js"; + import { detectMime, extensionForMime } from "./mime.js"; +/** + * SSRF protection: validates that a URL is safe to fetch (not localhost, private IPs, etc.) + */ +function isUrlAllowed(url: string): boolean { + let parsed: URL; + try { + parsed = new URL(url); + } catch { + return false; + } + + // Only allow http/https + if (!["http:", "https:"].includes(parsed.protocol)) { + return false; + } + + const hostname = parsed.hostname; + + // Block localhost variations + if (hostname === "localhost" || hostname.endsWith(".localhost")) { + return false; + } + + // Check if hostname is an IP address + try { + // Handle IPv6 brackets [::1] -> ::1 + const cleanHostname = hostname.replace(/^\[|\]$/g, ""); + const addr = ipaddr.parse(cleanHostname); + const range = addr.range(); + + // Block all private/special ranges + const blockedRanges = [ + "loopback", // 127.0.0.0/8, ::1 + "private", // 10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16, fc00::/7 + "linkLocal", // 169.254.0.0/16, fe80::/10 + "uniqueLocal", // fc00::/7 + "unspecified", // 0.0.0.0, :: + ]; + + if (blockedRanges.includes(range)) { + return false; + } + } catch { + // Not a valid IP - it's a hostname, allow DNS resolution + // Note: DNS rebinding is still possible but harder to exploit + } + + return true; +} + type FetchMediaResult = { buffer: Buffer; contentType?: string; @@ -63,23 +115,78 @@ async function readErrorBodySnippet(res: Response, maxChars = 200): Promise { const { url, fetchImpl, filePathHint, maxBytes } = options; + + // SSRF protection: block private/local addresses + if (!isUrlAllowed(url)) { + throw new MediaFetchError("fetch_failed", `URL not allowed: blocked private/local address`); + } + const fetcher: FetchLike | undefined = fetchImpl ?? globalThis.fetch; if (!fetcher) { throw new Error("fetch is not available"); } + // Follow redirects manually to validate each redirect URL against SSRF rules + let currentUrl = url; let res: Response; - try { - res = await fetcher(url); - } catch (err) { - throw new MediaFetchError("fetch_failed", `Failed to fetch media from ${url}: ${String(err)}`); + let redirectCount = 0; + + while (true) { + try { + res = await fetcher(currentUrl, { redirect: "manual" }); + } catch (err) { + throw new MediaFetchError( + "fetch_failed", + `Failed to fetch media from ${currentUrl}: ${String(err)}`, + ); + } + + // Handle redirects (3xx status codes) + if (res.status >= 300 && res.status < 400) { + const location = res.headers.get("location"); + if (!location) { + throw new MediaFetchError( + "http_error", + `Redirect from ${currentUrl} missing Location header`, + ); + } + + // Resolve relative redirects + let redirectUrl: string; + try { + redirectUrl = new URL(location, currentUrl).href; + } catch { + throw new MediaFetchError("http_error", `Invalid redirect URL: ${location}`); + } + + // SSRF protection: validate redirect target + if (!isUrlAllowed(redirectUrl)) { + throw new MediaFetchError( + "fetch_failed", + `Redirect blocked: target URL not allowed (private/local address)`, + ); + } + + redirectCount++; + if (redirectCount > MAX_REDIRECTS) { + throw new MediaFetchError("fetch_failed", `Too many redirects (max ${MAX_REDIRECTS})`); + } + + currentUrl = redirectUrl; + continue; + } + + // Not a redirect, exit loop + break; } if (!res.ok) { const statusText = res.statusText ? ` ${res.statusText}` : ""; - const redirected = res.url && res.url !== url ? ` (redirected to ${res.url})` : ""; + const redirected = currentUrl !== url ? ` (redirected to ${currentUrl})` : ""; let detail = `HTTP ${res.status}${statusText}`; if (!res.body) { detail = `HTTP ${res.status}${statusText}; empty response body`; @@ -184,3 +291,6 @@ async function readResponseWithLimit(res: Response, maxBytes: number): Promise { expect(result.buffer.length).toBeLessThanOrEqual(cap); }); }); + +describe("path traversal protection", () => { + it("allows paths within ~/.clawdbot", () => { + const clawdbotPath = path.join(os.homedir(), ".clawdbot", "test.txt"); + expect(_isPathAllowed(clawdbotPath)).toBe(true); + }); + + it("allows paths within tmpdir", async () => { + // Create actual temp file to test real path resolution + const tmpFile = path.join( + os.tmpdir(), + `clawdbot-test-${Date.now()}-${Math.random().toString(16).slice(2)}.txt`, + ); + await fs.writeFile(tmpFile, "test"); + tmpFiles.push(tmpFile); + + expect(_isPathAllowed(tmpFile)).toBe(true); + }); + + it("allows non-existent paths within allowed directories", () => { + const nonExistent = path.join(os.tmpdir(), "does-not-exist-12345.txt"); + // Non-existent paths within allowed directories return true (let the read fail with proper error) + expect(_isPathAllowed(nonExistent)).toBe(true); + }); + + it("rejects non-existent paths outside allowed directories", () => { + const nonExistent = "/etc/does-not-exist-12345.txt"; + // Non-existent paths outside allowed directories should be rejected + expect(_isPathAllowed(nonExistent)).toBe(false); + }); + + it("rejects file:// URLs to restricted paths via loadWebMedia", async () => { + // Use /bin/ls which exists on macOS and is definitely outside allowed roots + await expect(loadWebMedia("file:///bin/ls", 1024 * 1024)).rejects.toThrow( + /Access denied.*outside allowed directories/, + ); + }); + + it("rejects local paths to restricted areas via loadWebMedia", async () => { + // Use /bin/ls which exists on macOS and is definitely outside allowed roots + await expect(loadWebMedia("/bin/ls", 1024 * 1024)).rejects.toThrow( + /Access denied.*outside allowed directories/, + ); + }); + + it("blocks symlink escapes", async () => { + // Create a symlink in tmpdir that points to home directory (which is outside allowed roots) + const symlinkPath = path.join( + os.tmpdir(), + `clawdbot-symlink-test-${Date.now()}-${Math.random().toString(16).slice(2)}`, + ); + + try { + await fs.symlink(os.homedir(), symlinkPath); + tmpFiles.push(symlinkPath); + + // The symlink is in tmpdir, but its target (home dir) is not in allowed roots + // isPathAllowed should block this because realpathSync resolves to home dir + expect(_isPathAllowed(symlinkPath)).toBe(false); + } catch { + // If we can't create symlinks (permissions, Windows), skip this test + console.log("Skipping symlink test (cannot create symlink)"); + } + }); +}); diff --git a/src/web/media.ts b/src/web/media.ts index 72f6d34de..eeaccecce 100644 --- a/src/web/media.ts +++ b/src/web/media.ts @@ -1,4 +1,6 @@ +import fsSync from "node:fs"; import fs from "node:fs/promises"; +import os from "node:os"; import path from "node:path"; import { fileURLToPath } from "node:url"; @@ -30,6 +32,53 @@ const HEIC_MIME_RE = /^image\/hei[cf]$/i; const HEIC_EXT_RE = /\.(heic|heif)$/i; const MB = 1024 * 1024; +/** + * Path traversal protection: validates that a file path is within allowed directories. + * Follows symlinks to ensure the real path is within bounds. + */ +function isPathAllowed(filePath: string): boolean { + // Resolve the input path (handles ..) + const resolved = path.resolve(filePath); + + // Allowlist of safe roots (resolve them too since macOS /tmp is a symlink) + const allowedRoots = [path.join(os.homedir(), ".clawdbot"), os.tmpdir()].map((root) => { + try { + return fsSync.realpathSync(root); + } catch { + return root; + } + }); + + // Helper to check if a path is within allowed roots + const isWithinAllowedRoots = (pathToCheck: string): boolean => { + return allowedRoots.some( + (root) => pathToCheck === root || pathToCheck.startsWith(root + path.sep), + ); + }; + + // Follow symlinks to get real path + let realPath: string; + try { + realPath = fsSync.realpathSync(resolved); + } catch { + // Path doesn't exist - try to resolve the parent directory to handle symlinks + // (e.g., /var -> /private/var on macOS) + const parentDir = path.dirname(resolved); + try { + const realParent = fsSync.realpathSync(parentDir); + const fileName = path.basename(resolved); + realPath = path.join(realParent, fileName); + } catch { + // Parent also doesn't exist - check raw resolved path against allowed roots + // This is more restrictive but safe + return isWithinAllowedRoots(resolved); + } + } + + // Check REAL path (after symlink resolution) is within allowed roots + return isWithinAllowedRoots(realPath); +} + function formatMb(bytes: number, digits = 2): string { return (bytes / MB).toFixed(digits); } @@ -119,6 +168,10 @@ async function loadWebMediaInternal( } catch { throw new Error(`Invalid file:// URL: ${mediaUrl}`); } + // Path traversal protection: ensure file is within allowed directories + if (!isPathAllowed(mediaUrl)) { + throw new Error("Access denied: file path outside allowed directories"); + } } const optimizeAndClampImage = async ( @@ -200,6 +253,11 @@ async function loadWebMediaInternal( mediaUrl = resolveUserPath(mediaUrl); } + // Path traversal protection for local paths + if (!isPathAllowed(mediaUrl)) { + throw new Error("Access denied: file path outside allowed directories"); + } + // Local path const data = await fs.readFile(mediaUrl); const mime = await detectMime({ buffer: data, filePath: mediaUrl }); @@ -302,3 +360,6 @@ export async function optimizeImageToJpeg( } export { optimizeImageToPng }; + +// Export for testing +export { isPathAllowed as _isPathAllowed };