From 460694e335ca95babb4b49cede31b87ea40b383c Mon Sep 17 00:00:00 2001 From: Maurice Date: Sun, 31 May 2026 15:43:59 +0200 Subject: [PATCH] Re-check SSRF on every redirect hop when resolving short links Replace the one-shot checkSsrf + fetch(redirect:'follow') in the maps and place short-link resolvers with safeFetchFollow, which follows redirects manually and re-runs checkSsrf against the DNS-pinned IP of each hop (max 5). A redirect to an internal/loopback address is now blocked even when the initial URL is public, while legitimate cross-host redirects (goo.gl -> maps.google.com) still resolve. --- server/src/services/mapsService.ts | 32 +++-- server/src/services/placeService.ts | 27 ++++- server/src/utils/ssrfGuard.ts | 79 ++++++++++++ .../tests/unit/services/mapsService.test.ts | 23 +++- server/tests/unit/utils/ssrfGuard.test.ts | 113 +++++++++++++++++- 5 files changed, 254 insertions(+), 20 deletions(-) diff --git a/server/src/services/mapsService.ts b/server/src/services/mapsService.ts index 0d07a617..d6503228 100644 --- a/server/src/services/mapsService.ts +++ b/server/src/services/mapsService.ts @@ -1,6 +1,6 @@ import { db } from '../db/database'; import { decrypt_api_key } from './apiKeyCrypto'; -import { checkSsrf } from '../utils/ssrfGuard'; +import { safeFetchFollow, SsrfBlockedError } from '../utils/ssrfGuard'; import { getAppUrl } from './notifications'; // ── Google API call counter ─────────────────────────────────────────────────── @@ -634,10 +634,10 @@ export async function getPlacePhoto( try { const wiki = await fetchWikimediaPhoto(lat, lng, name); if (wiki) { - // Wikimedia photos: fetch bytes and cache to disk - const ssrf = await checkSsrf(wiki.photoUrl, true); - if (!ssrf.allowed) throw Object.assign(new Error('Photo URL blocked'), { status: 403 }); - const imgRes = await fetch(wiki.photoUrl); + // Wikimedia photos: fetch bytes and cache to disk. Follow redirects + // manually so each hop (the image URL can 3xx to a CDN host) is + // re-validated against the SSRF guard, not just the first URL. + const imgRes = await safeFetchFollow(wiki.photoUrl, undefined, { bypassInternalIpAllowed: true }); if (imgRes.ok) { const bytes = Buffer.from(await imgRes.arrayBuffer()); const cached = await placePhotoCache.put(placeId, bytes, wiki.attribution); @@ -746,13 +746,25 @@ export async function reverseGeocode(lat: string, lng: string, lang?: string): P export async function resolveGoogleMapsUrl(url: string): Promise<{ lat: number; lng: number; name: string | null; address: string | null }> { let resolvedUrl = url; - // Follow redirects for short URLs (goo.gl, maps.app.goo.gl) with SSRF protection + // Follow redirects for short URLs (goo.gl, maps.app.goo.gl) with SSRF protection. + // Redirects are followed manually so every hop is re-checked — a short link + // that 302s to an internal IP is blocked, while a legitimate cross-host + // redirect (goo.gl → maps.google.com) still resolves. const parsed = new URL(url); if (['goo.gl', 'maps.app.goo.gl'].includes(parsed.hostname)) { - const ssrf = await checkSsrf(url, true); - if (!ssrf.allowed) throw Object.assign(new Error('URL blocked by SSRF check'), { status: 403 }); - const redirectRes = await fetch(url, { redirect: 'follow', signal: AbortSignal.timeout(10000) }); - resolvedUrl = redirectRes.url; + try { + const redirectRes = await safeFetchFollow( + url, + { signal: AbortSignal.timeout(10000) }, + { bypassInternalIpAllowed: true }, + ); + resolvedUrl = redirectRes.url; + } catch (err) { + if (err instanceof SsrfBlockedError) { + throw Object.assign(new Error('URL blocked by SSRF check'), { status: 403 }); + } + throw err; + } } // Extract coordinates from Google Maps URL patterns: diff --git a/server/src/services/placeService.ts b/server/src/services/placeService.ts index 2d8402d8..d5dd7810 100644 --- a/server/src/services/placeService.ts +++ b/server/src/services/placeService.ts @@ -2,7 +2,7 @@ import { XMLParser, XMLValidator } from 'fast-xml-parser'; import unzipper from 'unzipper'; import { db, getPlaceWithTags } from '../db/database'; import { loadTagsByPlaceIds } from './queryHelpers'; -import { checkSsrf } from '../utils/ssrfGuard'; +import { checkSsrf, safeFetchFollow, SsrfBlockedError } from '../utils/ssrfGuard'; import { Place } from '../types'; import { buildCategoryNameLookup, @@ -587,10 +587,18 @@ export async function importGoogleList(tripId: string, url: string) { const ssrf = await checkSsrf(url); if (!ssrf.allowed) return { error: 'URL is not allowed', status: 400 }; - // Follow redirects for short URLs (maps.app.goo.gl, goo.gl) + // Follow redirects for short URLs (maps.app.goo.gl, goo.gl). Redirects are + // followed manually so every hop is re-checked against the SSRF guard — a + // short link that 302s to an internal IP is blocked even though the initial + // host is public. if (url.includes('goo.gl') || url.includes('maps.app')) { - const redirectRes = await fetch(url, { redirect: 'follow', signal: AbortSignal.timeout(10000) }); - resolvedUrl = redirectRes.url; + try { + const redirectRes = await safeFetchFollow(url, { signal: AbortSignal.timeout(10000) }); + resolvedUrl = redirectRes.url; + } catch (err) { + if (err instanceof SsrfBlockedError) return { error: 'URL is not allowed', status: 400 }; + throw err; + } } // Pattern: /placelists/list/{ID} @@ -692,11 +700,18 @@ export async function importNaverList( if (!ssrf.allowed) return { error: 'URL is not allowed', status: 400 }; // Resolve naver.me short links to the canonical map.naver.com folder URL. + // Redirects are followed manually so each hop is re-validated against the + // SSRF guard (a short link could otherwise 302 to an internal address). let parsedUrl: URL; try { parsedUrl = new URL(url); } catch { return { error: 'Invalid URL', status: 400 }; } if (parsedUrl.hostname === 'naver.me') { - const redirectRes = await fetch(url, { redirect: 'follow', signal: AbortSignal.timeout(10000) }); - resolvedUrl = redirectRes.url; + try { + const redirectRes = await safeFetchFollow(url, { signal: AbortSignal.timeout(10000) }); + resolvedUrl = redirectRes.url; + } catch (err) { + if (err instanceof SsrfBlockedError) return { error: 'URL is not allowed', status: 400 }; + throw err; + } } const folderMatch = resolvedUrl.match(/favorite\/myPlace\/folder\/([A-Za-z0-9_-]+)/i); diff --git a/server/src/utils/ssrfGuard.ts b/server/src/utils/ssrfGuard.ts index f9b4255f..c7b27e82 100644 --- a/server/src/utils/ssrfGuard.ts +++ b/server/src/utils/ssrfGuard.ts @@ -131,6 +131,85 @@ export async function safeFetch(url: string, init?: RequestInit, options?: SafeF return fetch(url, { ...init, dispatcher } as any); } +export interface SafeFetchFollowOptions extends SafeFetchOptions { + /** Maximum number of redirects to follow before giving up. Defaults to 5. */ + maxRedirects?: number; + /** + * When true, private/internal IPs that ALLOW_INTERNAL_NETWORK would normally + * permit are still blocked (matches `checkSsrf(url, true)`). Loopback and + * link-local are always blocked regardless. Defaults to false. + */ + bypassInternalIpAllowed?: boolean; +} + +/** + * SSRF-safe fetch that follows redirects MANUALLY, re-validating every hop. + * + * `safeFetch()` (and a one-shot `checkSsrf()` + `fetch(redirect:'follow')`) only + * guards the INITIAL URL: a validated public URL can 302-redirect to an internal + * IP that the platform fetch would then follow unchecked (redirect TOCTOU). This + * helper instead requests with `redirect: 'manual'`, and on every 3xx it resolves + * the `Location` header against the current URL, runs `checkSsrf()` on the new + * target, and only then fetches the next hop through a dispatcher pinned to THAT + * hop's resolved IP. Each hop is therefore SSRF-checked + DNS-pinned, while + * legitimate cross-host redirects (e.g. goo.gl → maps.google.com) still resolve + * because the dispatcher is re-pinned per hop rather than locked to the first IP. + * + * The returned Response is the first non-redirect response (or the last redirect + * if the hop limit is reached). `response.url` reflects the final hop so callers + * relying on the resolved URL keep working. + */ +export async function safeFetchFollow( + url: string, + init?: RequestInit, + options?: SafeFetchFollowOptions, +): Promise { + const maxRedirects = options?.maxRedirects ?? 5; + const rejectUnauthorized = options?.rejectUnauthorized ?? true; + const bypassInternalIpAllowed = options?.bypassInternalIpAllowed ?? false; + + let currentUrl = url; + + for (let hop = 0; ; hop++) { + const ssrf = await checkSsrf(currentUrl, bypassInternalIpAllowed); + if (!ssrf.allowed) { + throw new SsrfBlockedError(ssrf.error ?? 'Request blocked by SSRF guard'); + } + + const dispatcher = createPinnedDispatcher(ssrf.resolvedIp!, rejectUnauthorized); + const response = await fetch(currentUrl, { + ...init, + redirect: 'manual', + dispatcher, + } as any); + + // Only a 3xx WITH a Location header is a redirect we follow; anything else + // (2xx/4xx/5xx, or a 3xx with no Location) is the final response. + const status = typeof response.status === 'number' ? response.status : 0; + const isRedirectStatus = status >= 300 && status < 400; + const location = isRedirectStatus ? response.headers?.get('location') ?? null : null; + if (!location) { + return response; + } + + if (hop >= maxRedirects) { + throw new SsrfBlockedError('Too many redirects'); + } + + // Resolve relative redirects against the current URL, then loop to + // re-check + re-pin on the next iteration. Drain the body so the + // connection can be reused/closed. + let nextUrl: string; + try { + nextUrl = new URL(location, currentUrl).toString(); + } catch { + throw new SsrfBlockedError('Invalid redirect location'); + } + void response.body?.cancel().catch(() => {}); + currentUrl = nextUrl; + } +} + /** * Returns an undici Agent whose connect.lookup is pinned to the already-validated * IP. This prevents DNS rebinding (TOCTOU) by ensuring the outbound connection diff --git a/server/tests/unit/services/mapsService.test.ts b/server/tests/unit/services/mapsService.test.ts index f1f7dd8c..bf6f00a4 100644 --- a/server/tests/unit/services/mapsService.test.ts +++ b/server/tests/unit/services/mapsService.test.ts @@ -29,9 +29,26 @@ vi.mock('../../../src/db/database', () => ({ }, })); -vi.mock('../../../src/utils/ssrfGuard', () => ({ - checkSsrf: mockCheckSsrf, -})); +vi.mock('../../../src/utils/ssrfGuard', () => { + class SsrfBlockedError extends Error { + constructor(message: string) { + super(message); + this.name = 'SsrfBlockedError'; + } + } + return { + checkSsrf: mockCheckSsrf, + SsrfBlockedError, + // Mirror the real per-hop helper closely enough for unit tests: run the + // (mocked) SSRF check, then fetch through the (stubbed) global fetch. The + // fetch stubs in these tests already return the final resolved response. + safeFetchFollow: vi.fn(async (url: string, init?: any) => { + const ssrf = await mockCheckSsrf(url); + if (!ssrf.allowed) throw new SsrfBlockedError(ssrf.error ?? 'Request blocked by SSRF guard'); + return (globalThis.fetch as any)(url, init); + }), + }; +}); vi.mock('../../../src/services/apiKeyCrypto', () => ({ decrypt_api_key: (v: string | null) => v, diff --git a/server/tests/unit/utils/ssrfGuard.test.ts b/server/tests/unit/utils/ssrfGuard.test.ts index c7cc674f..691c24c2 100644 --- a/server/tests/unit/utils/ssrfGuard.test.ts +++ b/server/tests/unit/utils/ssrfGuard.test.ts @@ -21,7 +21,7 @@ vi.mock('undici', () => ({ })); import dns from 'dns/promises'; -import { checkSsrf, SsrfBlockedError, safeFetch, createPinnedDispatcher } from '../../../src/utils/ssrfGuard'; +import { checkSsrf, SsrfBlockedError, safeFetch, safeFetchFollow, createPinnedDispatcher } from '../../../src/utils/ssrfGuard'; const mockLookup = vi.mocked(dns.lookup); @@ -215,6 +215,117 @@ describe('safeFetch', () => { }); }); +describe('safeFetchFollow (manual per-hop redirect SSRF)', () => { + afterEach(() => { + vi.unstubAllGlobals(); + vi.unstubAllEnvs(); + mockLookup.mockReset(); + }); + + /** Build a minimal Response-like object for a given hop. */ + function fakeResponse(opts: { status: number; location?: string; url: string; ok?: boolean }) { + return { + status: opts.status, + ok: opts.ok ?? (opts.status >= 200 && opts.status < 300), + url: opts.url, + headers: { get: (h: string) => (h.toLowerCase() === 'location' ? opts.location ?? null : null) }, + body: { cancel: () => Promise.resolve() }, + }; + } + + it('follows a legitimate cross-host redirect (goo.gl -> maps.google.com) to the final response', async () => { + // Both hops resolve to public IPs. + mockLookup.mockResolvedValue({ address: '142.250.0.0', family: 4 }); + const mockFetch = vi.fn() + .mockResolvedValueOnce(fakeResponse({ status: 302, location: 'https://maps.google.com/maps/place/Foo', url: 'https://goo.gl/abc' })) + .mockResolvedValueOnce(fakeResponse({ status: 200, url: 'https://maps.google.com/maps/place/Foo' })); + vi.stubGlobal('fetch', mockFetch); + + const res = await safeFetchFollow('https://goo.gl/abc'); + expect(mockFetch).toHaveBeenCalledTimes(2); + expect(res.status).toBe(200); + expect(res.url).toBe('https://maps.google.com/maps/place/Foo'); + }); + + it('blocks a redirect whose target resolves to an internal IP', async () => { + vi.stubEnv('ALLOW_INTERNAL_NETWORK', 'false'); + // First hop (public) is allowed; the redirect target resolves to a private IP. + mockLookup + .mockResolvedValueOnce({ address: '142.250.0.0', family: 4 }) // goo.gl + .mockResolvedValue({ address: '169.254.169.254', family: 4 }); // redirect → metadata + const mockFetch = vi.fn() + .mockResolvedValueOnce(fakeResponse({ status: 302, location: 'http://169.254.169.254/latest/meta-data/', url: 'https://goo.gl/evil' })); + vi.stubGlobal('fetch', mockFetch); + + await expect(safeFetchFollow('https://goo.gl/evil')).rejects.toThrow(SsrfBlockedError); + // Only the first hop should have been fetched; the internal hop is blocked BEFORE fetch. + expect(mockFetch).toHaveBeenCalledTimes(1); + }); + + it('blocks a redirect to a loopback address even with ALLOW_INTERNAL_NETWORK=true', async () => { + mockLookup + .mockResolvedValueOnce({ address: '142.250.0.0', family: 4 }) + .mockResolvedValue({ address: '127.0.0.1', family: 4 }); + const mockFetch = vi.fn() + .mockResolvedValueOnce(fakeResponse({ status: 301, location: 'http://internal/', url: 'https://goo.gl/x' })); + vi.stubGlobal('fetch', mockFetch); + + await expect(safeFetchFollow('https://goo.gl/x', undefined, { bypassInternalIpAllowed: true })) + .rejects.toThrow(SsrfBlockedError); + expect(mockFetch).toHaveBeenCalledTimes(1); + }); + + it('rejects the initial URL if it is already internal', async () => { + mockLookup.mockResolvedValue({ address: '10.0.0.5', family: 4 }); + const mockFetch = vi.fn(); + vi.stubGlobal('fetch', mockFetch); + await expect(safeFetchFollow('http://intranet.example')).rejects.toThrow(SsrfBlockedError); + expect(mockFetch).not.toHaveBeenCalled(); + }); + + it('returns the response immediately when not a redirect', async () => { + mockLookup.mockResolvedValue({ address: '8.8.8.8', family: 4 }); + const mockFetch = vi.fn().mockResolvedValue(fakeResponse({ status: 200, url: 'https://example.com' })); + vi.stubGlobal('fetch', mockFetch); + const res = await safeFetchFollow('https://example.com'); + expect(mockFetch).toHaveBeenCalledTimes(1); + expect(res.status).toBe(200); + }); + + it('returns a 3xx with no Location header as-is (nothing to follow)', async () => { + mockLookup.mockResolvedValue({ address: '8.8.8.8', family: 4 }); + const mockFetch = vi.fn().mockResolvedValue(fakeResponse({ status: 304, url: 'https://example.com' })); + vi.stubGlobal('fetch', mockFetch); + const res = await safeFetchFollow('https://example.com'); + expect(res.status).toBe(304); + }); + + it('throws after exceeding the max redirect hops', async () => { + mockLookup.mockResolvedValue({ address: '8.8.8.8', family: 4 }); + // Always 302 to a new public host → loops until the hop cap. + let n = 0; + const mockFetch = vi.fn().mockImplementation(() => + Promise.resolve(fakeResponse({ status: 302, location: `https://h${++n}.example.com/`, url: `https://h${n}.example.com/` })), + ); + vi.stubGlobal('fetch', mockFetch); + await expect(safeFetchFollow('https://start.example.com', undefined, { maxRedirects: 2 })) + .rejects.toThrow(SsrfBlockedError); + // initial + 2 allowed redirects = 3 fetches, then the 4th hop is rejected before fetch + expect(mockFetch).toHaveBeenCalledTimes(3); + }); + + it('resolves relative redirect Location against the current URL', async () => { + mockLookup.mockResolvedValue({ address: '8.8.8.8', family: 4 }); + const mockFetch = vi.fn() + .mockResolvedValueOnce(fakeResponse({ status: 302, location: '/resolved/path', url: 'https://example.com/start' })) + .mockResolvedValueOnce(fakeResponse({ status: 200, url: 'https://example.com/resolved/path' })); + vi.stubGlobal('fetch', mockFetch); + await safeFetchFollow('https://example.com/start'); + // Second fetch must target the absolute resolution of the relative Location. + expect(mockFetch.mock.calls[1][0]).toBe('https://example.com/resolved/path'); + }); +}); + describe('createPinnedDispatcher', () => { it('returns an object (Agent instance)', () => { const dispatcher = createPinnedDispatcher('93.184.216.34');