diff --git a/apps/readest-app/src/__tests__/api/storage-upload-traversal.test.ts b/apps/readest-app/src/__tests__/api/storage-upload-traversal.test.ts new file mode 100644 index 00000000..25e08a9c --- /dev/null +++ b/apps/readest-app/src/__tests__/api/storage-upload-traversal.test.ts @@ -0,0 +1,100 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import type { NextApiRequest, NextApiResponse } from 'next'; + +// GHSA-mfmj-2frf-vhgw: a cross-tenant write via `fileName` path traversal. +// The handler must reject a traversing `fileName` before it ever asks the +// storage backend for a presigned PUT URL. + +const validateUserAndTokenMock = vi.fn(); +const getUploadSignedUrlMock = vi.fn(); +const getDownloadSignedUrlMock = vi.fn(); +const createSupabaseAdminClientMock = vi.fn(); + +vi.mock('@/utils/cors', () => ({ + corsAllMethods: {}, + runMiddleware: vi.fn().mockResolvedValue(undefined), +})); +vi.mock('@/utils/access', () => ({ + validateUserAndToken: (...a: unknown[]) => validateUserAndTokenMock(...a), + getStoragePlanData: vi.fn().mockReturnValue({ usage: 0, quota: 10 ** 12 }), + STORAGE_QUOTA_GRACE_BYTES: 0, +})); +vi.mock('@/utils/object', async (orig) => { + const actual = await orig(); + return { + ...actual, + getUploadSignedUrl: (...a: unknown[]) => getUploadSignedUrlMock(...a), + getDownloadSignedUrl: (...a: unknown[]) => getDownloadSignedUrlMock(...a), + }; +}); +vi.mock('@/utils/supabase', () => ({ + createSupabaseAdminClient: (...a: unknown[]) => createSupabaseAdminClientMock(...a), +})); + +import handler from '@/pages/api/storage/upload'; + +const makeReqRes = (body: Record) => { + const req = { + method: 'POST', + headers: { authorization: 'Bearer attacker' }, + body, + } as unknown as NextApiRequest; + const res = { + status: vi.fn().mockReturnThis(), + json: vi.fn().mockReturnThis(), + } as unknown as NextApiResponse; + return { req, res }; +}; + +beforeEach(() => { + validateUserAndTokenMock.mockReset().mockResolvedValue({ + user: { id: 'attacker-id' }, + token: 'tok', + }); + getUploadSignedUrlMock.mockReset().mockResolvedValue('https://r2/upload'); + getDownloadSignedUrlMock.mockReset().mockResolvedValue('https://r2/download'); + + // Minimal chainable Supabase stub: the no-existing-record lookup then the + // insert, both ending in `.single()`. + const single = vi + .fn() + .mockResolvedValueOnce({ data: null, error: { code: 'PGRST116' } }) + .mockResolvedValueOnce({ data: { file_size: 12345 }, error: null }); + const builder: Record = {}; + for (const m of ['select', 'eq', 'limit', 'insert']) builder[m] = () => builder; + builder['single'] = single; + createSupabaseAdminClientMock.mockReset().mockReturnValue({ from: () => builder }); +}); + +describe('POST /api/storage/upload — fileName traversal guard', () => { + it('rejects a traversing fileName with 400 and never presigns', async () => { + const { req, res } = makeReqRes({ + fileName: '../victim-id/Readest/Book/hash/book.epub', + fileSize: 12345, + }); + await handler(req, res); + expect(res.status).toHaveBeenCalledWith(400); + expect(getUploadSignedUrlMock).not.toHaveBeenCalled(); + }); + + it('rejects a traversing fileName on the temp image branch too', async () => { + const { req, res } = makeReqRes({ + fileName: '../../other/evil.png', + fileSize: 999, + temp: true, + }); + await handler(req, res); + expect(res.status).toHaveBeenCalledWith(400); + expect(getUploadSignedUrlMock).not.toHaveBeenCalled(); + }); + + it('allows a normal book key through to presigning', async () => { + const { req, res } = makeReqRes({ + fileName: 'Readest/Books/hash.epub', + fileSize: 12345, + }); + await handler(req, res); + expect(getUploadSignedUrlMock).toHaveBeenCalled(); + expect(res.status).toHaveBeenCalledWith(200); + }); +}); diff --git a/apps/readest-app/src/__tests__/app/api/opds-proxy-ssrf.test.ts b/apps/readest-app/src/__tests__/app/api/opds-proxy-ssrf.test.ts new file mode 100644 index 00000000..d424d536 --- /dev/null +++ b/apps/readest-app/src/__tests__/app/api/opds-proxy-ssrf.test.ts @@ -0,0 +1,79 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { NextRequest } from 'next/server'; +import { GET } from '@/app/api/opds/proxy/route'; + +// SSRF hardening for the OPDS proxy (GHSA-c7mm-g2j2-98cx / GHSA-5g3f-mq2c-j65v). +// The proxy must refuse internal/loopback/link-local targets, non-http(s) +// schemes, and redirects that hop to an internal address — without ever +// reflecting the upstream body. + +const proxyReq = (target: string) => + new NextRequest( + `https://web.readest.com/api/opds/proxy?url=${encodeURIComponent(target)}&stream=false`, + ); + +let fetchSpy: ReturnType; + +beforeEach(() => { + fetchSpy = vi.fn(); + vi.stubGlobal('fetch', fetchSpy); +}); + +afterEach(() => { + vi.unstubAllGlobals(); + vi.restoreAllMocks(); +}); + +describe('OPDS proxy SSRF guard', () => { + it('blocks the AWS metadata endpoint without fetching', async () => { + const res = await GET(proxyReq('http://169.254.169.254/latest/meta-data/iam/')); + expect(res.status).toBe(400); + expect(fetchSpy).not.toHaveBeenCalled(); + }); + + it('blocks loopback service ports without fetching', async () => { + const res = await GET(proxyReq('http://127.0.0.1:6379/')); + expect(res.status).toBe(400); + expect(fetchSpy).not.toHaveBeenCalled(); + }); + + it('blocks private RFC1918 ranges without fetching', async () => { + const res = await GET(proxyReq('http://10.0.0.10:8080/admin')); + expect(res.status).toBe(400); + expect(fetchSpy).not.toHaveBeenCalled(); + }); + + it('blocks non-http(s) schemes without fetching', async () => { + const res = await GET(proxyReq('file:///etc/passwd')); + expect(res.status).toBe(400); + expect(fetchSpy).not.toHaveBeenCalled(); + }); + + it('blocks a public URL that redirects to an internal address', async () => { + // First (and only) upstream hop returns a 302 pointing at the metadata IP. + fetchSpy.mockResolvedValueOnce( + new Response(null, { + status: 302, + headers: { location: 'http://169.254.169.254/latest/meta-data/' }, + }), + ); + const res = await GET(proxyReq('https://feeds.example.com/redirect')); + expect(res.status).toBe(400); + // The internal hop must never be fetched. + expect(fetchSpy).toHaveBeenCalledTimes(1); + }); + + it('proxies a legitimate public feed', async () => { + fetchSpy.mockResolvedValueOnce( + new Response('', { + status: 200, + headers: { 'Content-Type': 'application/atom+xml' }, + }), + ); + const res = await GET(proxyReq('https://feeds.example.com/catalog.atom')); + expect(res.status).toBe(200); + expect(fetchSpy).toHaveBeenCalledTimes(1); + const body = await res.text(); + expect(body).toContain(' ({ + getStripe: () => ({ + checkout: { sessions: { retrieve: (...a: unknown[]) => retrieveMock(...a) } }, + }), + createOrUpdateSubscription: (...a: unknown[]) => createOrUpdateSubscriptionMock(...a), + createOrUpdatePayment: (...a: unknown[]) => createOrUpdatePaymentMock(...a), +})); +vi.mock('@/utils/access', () => ({ + validateUserAndToken: (...a: unknown[]) => validateUserAndTokenMock(...a), +})); + +import { POST } from '@/app/api/stripe/check/route'; + +const postReq = (sessionId: string) => + new Request('https://web.readest.com/api/stripe/check', { + method: 'POST', + headers: { authorization: 'Bearer caller', 'content-type': 'application/json' }, + body: JSON.stringify({ sessionId }), + }); + +beforeEach(() => { + validateUserAndTokenMock.mockReset().mockResolvedValue({ + user: { id: 'caller-id' }, + token: 'tok', + }); + retrieveMock.mockReset(); + createOrUpdateSubscriptionMock.mockReset().mockResolvedValue(undefined); + createOrUpdatePaymentMock.mockReset().mockResolvedValue(undefined); +}); + +describe('POST /api/stripe/check — session ownership', () => { + it('rejects a paid session owned by a different user and grants nothing', async () => { + retrieveMock.mockResolvedValue({ + payment_status: 'paid', + subscription: 'sub_1', + customer: 'cus_1', + metadata: { userId: 'someone-else' }, + }); + const res = await POST(postReq('cs_live_victim')); + expect(res.status).toBe(403); + expect(createOrUpdateSubscriptionMock).not.toHaveBeenCalled(); + expect(createOrUpdatePaymentMock).not.toHaveBeenCalled(); + }); + + it('rejects a paid session with no userId metadata', async () => { + retrieveMock.mockResolvedValue({ + payment_status: 'paid', + subscription: 'sub_1', + customer: 'cus_1', + metadata: {}, + }); + const res = await POST(postReq('cs_live_orphan')); + expect(res.status).toBe(403); + expect(createOrUpdateSubscriptionMock).not.toHaveBeenCalled(); + }); + + it('binds a paid session that belongs to the caller', async () => { + retrieveMock.mockResolvedValue({ + payment_status: 'paid', + subscription: 'sub_1', + customer: 'cus_1', + metadata: { userId: 'caller-id' }, + }); + const res = await POST(postReq('cs_live_own')); + expect(res.status).toBe(200); + expect(createOrUpdateSubscriptionMock).toHaveBeenCalledWith('caller-id', 'cus_1', 'sub_1'); + }); +}); diff --git a/apps/readest-app/src/__tests__/utils/network-ssrf-hardening.test.ts b/apps/readest-app/src/__tests__/utils/network-ssrf-hardening.test.ts new file mode 100644 index 00000000..8bb393d2 --- /dev/null +++ b/apps/readest-app/src/__tests__/utils/network-ssrf-hardening.test.ts @@ -0,0 +1,57 @@ +import { describe, expect, it } from 'vitest'; +import { isBlockedHost, isLanAddress } from '@/utils/network'; + +// Regression coverage for the secondary finding in GHSA-5g3f-mq2c-j65v: +// the old `isLanAddress` only blocked the literal `127.0.0.1` / `0.0.0.0` and +// missed whole ranges + internal hostname suffixes, so the unauthenticated +// `/api/kosync` proxy could still reach e.g. `http://127.0.0.2:6379/`. +// `isLanAddress` now delegates to the canonical `isBlockedHost`. +describe('isLanAddress — hardened ranges (previously bypassable)', () => { + it('blocks the full 127.0.0.0/8 loopback range, not just 127.0.0.1', () => { + expect(isLanAddress('http://127.0.0.2:6379/')).toBe(true); + expect(isLanAddress('http://127.1.2.3/')).toBe(true); + }); + + it('blocks the full 0.0.0.0/8 range', () => { + expect(isLanAddress('http://0.1.2.3/')).toBe(true); + }); + + it('blocks 198.18.0.0/15 benchmarking and 224.0.0.0/4 multicast/reserved', () => { + expect(isLanAddress('http://198.18.0.1/')).toBe(true); + expect(isLanAddress('http://198.19.255.255/')).toBe(true); + expect(isLanAddress('http://225.0.0.1/')).toBe(true); + expect(isLanAddress('http://240.0.0.1/')).toBe(true); + }); + + it('blocks internal hostname suffixes and bare single-label hosts', () => { + expect(isLanAddress('http://metadata/')).toBe(true); + expect(isLanAddress('http://db.internal/')).toBe(true); + expect(isLanAddress('http://box.lan/')).toBe(true); + expect(isLanAddress('http://host.localhost/')).toBe(true); + }); + + it('blocks IPv4-mapped / hex-mapped IPv6 pointing at private space', () => { + expect(isLanAddress('http://[::ffff:127.0.0.1]/')).toBe(true); + expect(isLanAddress('http://[::ffff:7f00:1]/')).toBe(true); + }); + + it('still allows ordinary public catalogs', () => { + expect(isLanAddress('https://sync.koreader.rocks')).toBe(false); + expect(isLanAddress('https://8.8.8.8')).toBe(false); + expect(isLanAddress('https://opds.example.com/feed')).toBe(false); + }); + + it('treats invalid URLs / octets as non-LAN (URL parse rejects them)', () => { + expect(isLanAddress('http://10.256.0.1')).toBe(false); + expect(isLanAddress('not-a-url')).toBe(false); + expect(isLanAddress('')).toBe(false); + }); +}); + +describe('isBlockedHost is exported from the canonical network helper', () => { + it('matches the SSRF blocklist used by the proxies', () => { + expect(isBlockedHost('169.254.169.254')).toBe(true); + expect(isBlockedHost('127.0.0.2')).toBe(true); + expect(isBlockedHost('example.com')).toBe(false); + }); +}); diff --git a/apps/readest-app/src/__tests__/utils/object-key-name.test.ts b/apps/readest-app/src/__tests__/utils/object-key-name.test.ts new file mode 100644 index 00000000..9f76761c --- /dev/null +++ b/apps/readest-app/src/__tests__/utils/object-key-name.test.ts @@ -0,0 +1,46 @@ +import { describe, it, expect } from 'vitest'; +import { isSafeObjectKeyName } from '@/utils/object'; + +// GHSA-mfmj-2frf-vhgw: the storage object key is built as `${user.id}/${fileName}` +// from a client-controlled `fileName`. The R2 signer interpolates it into +// `new Request(url)`, whose URL parser collapses `../` before signing — so a +// crafted name escapes the caller's `${user.id}/` prefix into another tenant's +// namespace. fileName legitimately contains '/' (Readest/Books/..., Replicas), +// so we reject traversal/absolute/backslash forms rather than separators. +describe('isSafeObjectKeyName', () => { + it('accepts the legitimate book / replica / cover key shapes', () => { + expect(isSafeObjectKeyName('Readest/Books/abc123.epub')).toBe(true); + expect(isSafeObjectKeyName('Readest/Replicas/dict/id-1/data.bin')).toBe(true); + expect(isSafeObjectKeyName('cover.png')).toBe(true); + expect(isSafeObjectKeyName('My Book (2024).epub')).toBe(true); + expect(isSafeObjectKeyName('A&B.epub')).toBe(true); + }); + + it('rejects parent-directory traversal segments', () => { + expect(isSafeObjectKeyName('../victim/Readest/Book/h/book.epub')).toBe(false); + expect(isSafeObjectKeyName('Readest/../../victim/book.epub')).toBe(false); + expect(isSafeObjectKeyName('..')).toBe(false); + expect(isSafeObjectKeyName('a/../b')).toBe(false); + }); + + it('rejects percent-encoded traversal', () => { + expect(isSafeObjectKeyName('%2e%2e/victim/book.epub')).toBe(false); + expect(isSafeObjectKeyName('a/%2e%2e/b')).toBe(false); + }); + + it('rejects absolute paths, backslashes, NUL and empty segments', () => { + expect(isSafeObjectKeyName('/etc/passwd')).toBe(false); + expect(isSafeObjectKeyName('a\\b')).toBe(false); + expect(isSafeObjectKeyName('a\0b')).toBe(false); + expect(isSafeObjectKeyName('a//b')).toBe(false); + expect(isSafeObjectKeyName('a/')).toBe(false); + }); + + it('rejects empty / non-string input', () => { + expect(isSafeObjectKeyName('')).toBe(false); + // @ts-expect-error runtime guard for untrusted req.body values + expect(isSafeObjectKeyName(undefined)).toBe(false); + // @ts-expect-error runtime guard for untrusted req.body values + expect(isSafeObjectKeyName(123)).toBe(false); + }); +}); diff --git a/apps/readest-app/src/app/api/opds/proxy/route.ts b/apps/readest-app/src/app/api/opds/proxy/route.ts index d9e8fa8d..fbd8394d 100644 --- a/apps/readest-app/src/app/api/opds/proxy/route.ts +++ b/apps/readest-app/src/app/api/opds/proxy/route.ts @@ -1,6 +1,42 @@ import { READEST_OPDS_USER_AGENT } from '@/services/constants'; import { NextRequest, NextResponse } from 'next/server'; import { deserializeOPDSCustomHeaders } from '@/app/opds/utils/customHeaders'; +import { isBlockedHost } from '@/utils/network'; + +// Cap redirect hops so the SSRF host check below can re-run on every one. +const MAX_REDIRECTS = 5; + +/** + * Fetch the target while running the SSRF host check on every redirect hop. + * `fetch`'s default `redirect: 'follow'` would let a public URL 302 to an + * internal address (e.g. the cloud metadata endpoint) after our initial host + * check passed, so we follow redirects manually and re-validate each hop. + * Throws `SsrfBlockedError` when a hop targets a blocked host or scheme. + */ +class SsrfBlockedError extends Error {} + +async function fetchFollowingRedirects( + startUrl: string, + init: { method: 'GET' | 'HEAD'; headers: Headers; signal: AbortSignal }, +): Promise { + let currentUrl = startUrl; + for (let hop = 0; hop <= MAX_REDIRECTS; hop++) { + const parsed = new URL(currentUrl); + if (parsed.protocol !== 'http:' && parsed.protocol !== 'https:') { + throw new SsrfBlockedError('Only http(s) URLs are supported'); + } + if (isBlockedHost(parsed.hostname)) { + throw new SsrfBlockedError('This URL is not allowed'); + } + const response = await fetch(currentUrl, { ...init, redirect: 'manual' }); + if (response.status >= 300 && response.status < 400 && response.headers.has('location')) { + currentUrl = new URL(response.headers.get('location')!, currentUrl).toString(); + continue; + } + return response; + } + throw new SsrfBlockedError('Too many redirects'); +} async function handleRequest(request: NextRequest, method: 'GET' | 'HEAD') { // Cloudflare Workers incorrectly decodes %26 to & in the url parameter value, @@ -30,12 +66,23 @@ async function handleRequest(request: NextRequest, method: 'GET' | 'HEAD') { ); } + let parsedUrl: URL; try { - new URL(url); + parsedUrl = new URL(url); } catch { return NextResponse.json({ error: 'Invalid URL format' }, { status: 400 }); } + // SSRF guard: this proxy fetches an arbitrary client-supplied URL server-side, + // so reject non-http(s) schemes and internal/loopback/link-local targets + // before making any request (the redirect loop re-checks every hop). + if (parsedUrl.protocol !== 'http:' && parsedUrl.protocol !== 'https:') { + return NextResponse.json({ error: 'Only http(s) URLs are supported' }, { status: 400 }); + } + if (isBlockedHost(parsedUrl.hostname)) { + return NextResponse.json({ error: 'This URL is not allowed' }, { status: 400 }); + } + try { console.log(`[OPDS Proxy] ${method}: ${url}`); @@ -54,7 +101,7 @@ async function handleRequest(request: NextRequest, method: 'GET' | 'HEAD') { headers.set('Authorization', auth); } - const response = await fetch(url, { + const response = await fetchFollowingRedirects(url, { method, headers, signal: controller.signal, @@ -200,6 +247,10 @@ async function handleRequest(request: NextRequest, method: 'GET' | 'HEAD') { } catch (error) { console.error('[OPDS Proxy] Error:', error); + if (error instanceof SsrfBlockedError) { + return NextResponse.json({ error: error.message }, { status: 400 }); + } + if (error instanceof Error) { if (error.name === 'AbortError') { return NextResponse.json( diff --git a/apps/readest-app/src/app/api/stripe/check/route.ts b/apps/readest-app/src/app/api/stripe/check/route.ts index c5645cc8..b53157df 100644 --- a/apps/readest-app/src/app/api/stripe/check/route.ts +++ b/apps/readest-app/src/app/api/stripe/check/route.ts @@ -19,6 +19,15 @@ export async function POST(request: Request) { const stripe = getStripe(); const session = await stripe.checkout.sessions.retrieve(sessionId); + // Bind the entitlement to the session's owner, not the caller. `sessionId` + // is client-supplied and Stripe sessions share a global id space, so without + // this check any authenticated user could replay one paid session id to + // upgrade their own (or many) accounts (GHSA-pv88-3727-j7v8). The checkout + // route stamps `metadata.userId`; the webhook relies on the same field. + if (session.metadata?.['userId'] !== user.id) { + return NextResponse.json({ error: 'Session does not belong to user' }, { status: 403 }); + } + const customerId = session.customer as string; if (session.payment_status === 'paid' && session.subscription) { await createOrUpdateSubscription(user.id, customerId, session.subscription as string); diff --git a/apps/readest-app/src/pages/api/send/fetch-url.ts b/apps/readest-app/src/pages/api/send/fetch-url.ts index 80240b0d..9b7b3d80 100644 --- a/apps/readest-app/src/pages/api/send/fetch-url.ts +++ b/apps/readest-app/src/pages/api/send/fetch-url.ts @@ -1,73 +1,16 @@ import type { NextApiRequest, NextApiResponse } from 'next'; import { corsAllMethods, runMiddleware } from '@/utils/cors'; import { validateUserAndToken } from '@/utils/access'; +import { isBlockedHost } from '@/utils/network'; const FETCH_TIMEOUT_MS = 10_000; const MAX_HTML_BYTES = 5 * 1024 * 1024; const MAX_REDIRECTS = 3; -/** Whether a dotted-decimal IPv4 address falls in a private / reserved range. */ -function isBlockedV4(a: number, b: number, c: number, _d: number): boolean { - if (a === 0 || a === 10 || a === 127) return true; // this-network, private, loopback - if (a === 169 && b === 254) return true; // link-local - if (a === 172 && b >= 16 && b <= 31) return true; // private - if (a === 192 && b === 168) return true; // private - if (a === 100 && b >= 64 && b <= 127) return true; // CGNAT - if (a === 192 && b === 0 && c === 0) return true; // IETF protocol assignments - if (a === 198 && (b === 18 || b === 19)) return true; // benchmarking - if (a >= 224) return true; // multicast + reserved - return false; -} - -/** - * Block obviously-internal hosts. A browser cannot fetch arbitrary cross-origin - * pages (CORS), so the web `/send` page routes article URLs through this proxy - * — which means the server makes the request, so an SSRF guard is mandatory. - * The Tauri apps fetch directly and never hit this route. - * - * Note: this is a string check on the URL host. A hostname that DNS-resolves to - * a private address (DNS rebinding) is a documented residual risk — the web - * build runs on the Cloudflare Workers edge, which has no reachable internal - * network or metadata endpoint. - */ -export function isBlockedHost(hostname: string): boolean { - const h = hostname.toLowerCase().replace(/^\[|\]$/g, ''); - if (!h) return true; - if (h === 'localhost' || h.endsWith('.localhost')) return true; - if (h.endsWith('.local') || h.endsWith('.internal') || h.endsWith('.lan')) return true; - // Bare single-label hostnames (e.g. `intranet`, `metadata`) — never public. - if (!h.includes('.') && !h.includes(':')) return true; - - // IPv4 — the WHATWG URL parser already normalized decimal/hex/octal forms. - const v4 = h.match(/^(\d{1,3})\.(\d{1,3})\.(\d{1,3})\.(\d{1,3})$/); - if (v4) { - return isBlockedV4(Number(v4[1]), Number(v4[2]), Number(v4[3]), Number(v4[4])); - } - - // IPv6, including IPv4-mapped / -compatible forms. - if (h.includes(':')) { - if (h === '::' || h === '::1') return true; // unspecified, loopback - if (/^(fc|fd)/.test(h)) return true; // unique-local - if (/^fe[89ab]/.test(h)) return true; // link-local - const mapped = h.match(/(\d{1,3})\.(\d{1,3})\.(\d{1,3})\.(\d{1,3})$/); - if (mapped) { - return isBlockedV4( - Number(mapped[1]), - Number(mapped[2]), - Number(mapped[3]), - Number(mapped[4]), - ); - } - const hexMapped = h.match(/::ffff:([0-9a-f]{1,4}):([0-9a-f]{1,4})$/); - if (hexMapped) { - const hi = parseInt(hexMapped[1] ?? '0', 16); - const lo = parseInt(hexMapped[2] ?? '0', 16); - return isBlockedV4((hi >> 8) & 0xff, hi & 0xff, (lo >> 8) & 0xff, lo & 0xff); - } - return false; - } - return false; -} +// `isBlockedHost` now lives in the shared network helper so every URL-fetching +// route (this `/send` proxy, `/api/opds/proxy`, `/api/kosync`) uses one +// canonical SSRF blocklist. Re-exported here for existing importers/tests. +export { isBlockedHost }; /** GET ?url=... — fetch a remote page's HTML for client-side article extraction. */ export default async function handler(req: NextApiRequest, res: NextApiResponse) { diff --git a/apps/readest-app/src/pages/api/storage/upload.ts b/apps/readest-app/src/pages/api/storage/upload.ts index 2fda5f44..72b8a6a1 100644 --- a/apps/readest-app/src/pages/api/storage/upload.ts +++ b/apps/readest-app/src/pages/api/storage/upload.ts @@ -6,7 +6,7 @@ import { validateUserAndToken, STORAGE_QUOTA_GRACE_BYTES, } from '@/utils/access'; -import { getDownloadSignedUrl, getUploadSignedUrl } from '@/utils/object'; +import { getDownloadSignedUrl, getUploadSignedUrl, isSafeObjectKeyName } from '@/utils/object'; import { READEST_PUBLIC_STORAGE_BASE_URL } from '@/services/constants'; export default async function handler(req: NextApiRequest, res: NextApiResponse) { @@ -22,6 +22,15 @@ export default async function handler(req: NextApiRequest, res: NextApiResponse) } const { fileName, fileSize, bookHash, replicaKind, replicaId, temp = false } = req.body; + + // Reject object-key path traversal before building any key. `fileName` is + // fully client-controlled and is interpolated into `${user.id}/${fileName}`; + // without this an attacker escapes their own prefix into another user's + // namespace (GHSA-mfmj-2frf-vhgw). + if (!isSafeObjectKeyName(fileName)) { + return res.status(400).json({ error: 'Invalid fileName' }); + } + if (temp) { try { const datetime = new Date(); diff --git a/apps/readest-app/src/utils/network.ts b/apps/readest-app/src/utils/network.ts index 3707be73..f9e7f4c6 100644 --- a/apps/readest-app/src/utils/network.ts +++ b/apps/readest-app/src/utils/network.ts @@ -20,62 +20,81 @@ export const isMetered = (): boolean => { return connection.type === 'cellular' || connection.saveData === true; }; -export const isLanAddress = (url: string) => { - try { - const urlObj = new URL(url); - const hostname = urlObj.hostname; +/** Whether a dotted-decimal IPv4 address falls in a private / reserved range. */ +function isBlockedV4(a: number, b: number, c: number, _d: number): boolean { + if (a === 0 || a === 10 || a === 127) return true; // this-network, private, loopback + if (a === 169 && b === 254) return true; // link-local + if (a === 172 && b >= 16 && b <= 31) return true; // private + if (a === 192 && b === 168) return true; // private + if (a === 100 && b >= 64 && b <= 127) return true; // CGNAT / Tailscale + if (a === 192 && b === 0 && c === 0) return true; // IETF protocol assignments + if (a === 198 && (b === 18 || b === 19)) return true; // benchmarking + if (a >= 224) return true; // multicast + reserved + return false; +} - if (hostname === 'localhost' || hostname === '127.0.0.1' || hostname === '0.0.0.0') { - return true; +/** + * Canonical SSRF host blocklist, shared by every server route that fetches a + * client-supplied URL (`/api/opds/proxy`, `/api/kosync`, `/api/send/fetch-url`). + * Returns true for hosts that must never be reached: loopback, private, + * link-local, CGNAT, benchmarking, multicast, internal hostname suffixes, and + * bare single-label names. + * + * Input is a hostname as the WHATWG URL parser serializes it (decimal/hex/octal + * IPv4 already normalized to dotted-quad). This is a string check — a hostname + * that DNS-resolves to a private address (DNS rebinding) is a documented + * residual risk; the hosted web build runs on the Cloudflare Workers edge, + * which has no reachable internal network or metadata endpoint. + */ +export function isBlockedHost(hostname: string): boolean { + const h = hostname.toLowerCase().replace(/^\[|\]$/g, ''); + if (!h) return true; + if (h === 'localhost' || h.endsWith('.localhost')) return true; + if (h.endsWith('.local') || h.endsWith('.internal') || h.endsWith('.lan')) return true; + // Bare single-label hostnames (e.g. `intranet`, `metadata`) — never public. + if (!h.includes('.') && !h.includes(':')) return true; + + // IPv4 — the WHATWG URL parser already normalized decimal/hex/octal forms. + const v4 = h.match(/^(\d{1,3})\.(\d{1,3})\.(\d{1,3})\.(\d{1,3})$/); + if (v4) { + return isBlockedV4(Number(v4[1]), Number(v4[2]), Number(v4[3]), Number(v4[4])); + } + + // IPv6, including IPv4-mapped / -compatible forms. + if (h.includes(':')) { + if (h === '::' || h === '::1') return true; // unspecified, loopback + if (/^(fc|fd)/.test(h)) return true; // unique-local + if (/^fe[89ab]/.test(h)) return true; // link-local + const mapped = h.match(/(\d{1,3})\.(\d{1,3})\.(\d{1,3})\.(\d{1,3})$/); + if (mapped) { + return isBlockedV4( + Number(mapped[1]), + Number(mapped[2]), + Number(mapped[3]), + Number(mapped[4]), + ); } - - // Check for IPv4 private ranges - const ipv4Regex = /^(\d{1,3})\.(\d{1,3})\.(\d{1,3})\.(\d{1,3})$/; - const match = hostname.match(ipv4Regex); - - if (match) { - const [, a, b, c, d] = match.map(Number); - if (a === undefined || b === undefined || c === undefined || d === undefined) { - return false; - } - - // Validate IP format - if (a > 255 || b > 255 || c > 255 || d > 255) { - return false; - } - - // Check private IP ranges: - // 10.0.0.0/8 (10.0.0.0 to 10.255.255.255) - if (a === 10) return true; - - // 172.16.0.0/12 (172.16.0.0 to 172.31.255.255) - if (a === 172 && b >= 16 && b <= 31) return true; - - // 192.168.0.0/16 (192.168.0.0 to 192.168.255.255) - if (a === 192 && b === 168) return true; - - // 169.254.0.0/16 (link-local addresses) - if (a === 169 && b === 254) return true; - - // Tailscale IPv4 range: 100.64.0.0/10 (100.64.0.0 to 100.127.255.255) - if (a === 100 && b >= 64 && b <= 127) return true; + const hexMapped = h.match(/::ffff:([0-9a-f]{1,4}):([0-9a-f]{1,4})$/); + if (hexMapped) { + const hi = parseInt(hexMapped[1] ?? '0', 16); + const lo = parseInt(hexMapped[2] ?? '0', 16); + return isBlockedV4((hi >> 8) & 0xff, hi & 0xff, (lo >> 8) & 0xff, lo & 0xff); } - - // Check for IPv6 private addresses - // URL.hostname wraps IPv6 in brackets, e.g. '[::1]' — strip them - const ipv6 = hostname.startsWith('[') ? hostname.slice(1, -1) : hostname; - if (ipv6.includes(':')) { - if ( - ipv6 === '::1' || - ipv6.startsWith('fe80:') || - ipv6.startsWith('fc00:') || - ipv6.startsWith('fd00:') - ) { - return true; - } - } - return false; + } + return false; +} + +/** + * Whether a URL points at a LAN / internal address. Delegates to the canonical + * {@link isBlockedHost} so the proxy SSRF guard and the LAN-detection callers + * (KOSync direct-vs-proxy routing, OPDS catalog warnings) stay in sync. + * Returns false for unparseable URLs (matching the previous best-effort + * behavior — an invalid host can't be reached anyway). + */ +export const isLanAddress = (url: string): boolean => { + try { + return isBlockedHost(new URL(url).hostname); } catch { return false; } diff --git a/apps/readest-app/src/utils/object.ts b/apps/readest-app/src/utils/object.ts index 6806ccc0..adbdeb3d 100644 --- a/apps/readest-app/src/utils/object.ts +++ b/apps/readest-app/src/utils/object.ts @@ -2,6 +2,38 @@ import { s3Storage } from './s3'; import { r2Storage } from './r2'; import { getStorageType } from './storage'; +/** + * Whether a client-supplied `fileName` is safe to interpolate into a storage + * object key (`${userId}/${fileName}`). The R2 signer builds the key into + * `new Request(url)`, whose WHATWG URL parser collapses `../` segments before + * the request is signed — so an unsanitized name can escape the caller's + * `${userId}/` prefix and write into another tenant's namespace + * (GHSA-mfmj-2frf-vhgw). + * + * Legitimate names DO contain `/` (e.g. `Readest/Books/.epub`, + * `Readest/Replicas///`), so we reject traversal rather than + * separators: no `.`/`..`/empty path segments, no leading slash (absolute), no + * backslash or NUL, checked on both the raw and percent-decoded forms. + */ +export const isSafeObjectKeyName = (fileName: string): boolean => { + if (typeof fileName !== 'string' || fileName.length === 0) return false; + + const forms = [fileName]; + try { + const decoded = decodeURIComponent(fileName); + if (decoded !== fileName) forms.push(decoded); + } catch { + return false; // malformed percent-encoding + } + + for (const form of forms) { + if (form.includes('\\') || form.includes('\0')) return false; + if (form.startsWith('/')) return false; + if (form.split('/').some((seg) => seg === '' || seg === '.' || seg === '..')) return false; + } + return true; +}; + export const getDownloadSignedUrl = async ( fileKey: string, expiresIn: number,