From b312bedd9e1a8f0fd010b628fba18260d79ec7ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=9Eahveli=20Karahan?= <176017743+Sahveli01@users.noreply.github.com> Date: Thu, 3 Sep 2026 13:03:22 +0300 Subject: [PATCH 1/2] Resolve Open Proxy, XSS Vectors, and Implement API Rate Limiting in `onchainsummer.xyz` ### Description This pull request addresses findings F-05, F-06, and F-11 from the workspace security audit targeting the `onchainsummer.xyz` reservoir API proxy. The route previously functioned as an open proxy exposing the deployment's API keys when unconfigured, allowed XSS payloads through improper content-type handling, and suffered from missing rate limits. These vectors have been comprehensively closed. ### Key Changes & Remediations #### 1. Open Proxy & Origin Validation (F-05) (`src/app/api/reservoir/[...slug]/route.ts`) * **Strict Origin Matching:** Removed the flawed Regex host parser that could be easily bypassed by domain manipulations. The logic now strictly extracts and matches origins using the WHATWG `URL` parser. * **Fail-Closed Configuration:** If `ALLOWED_API_DOMAINS` is not defined, the proxy now gracefully defaults to same-origin behavior instead of failing open to the entire internet. #### 2. XSS & Binary Corruption Mitigation (F-06) (`src/app/api/reservoir/[...slug]/route.ts`) * **Strict MIME Handling:** The proxy previously served all `image/*` upstream responses with a `text/html` header, effectively turning any upstream binary payload into a stored XSS vector on the local origin. It now correctly passes through upstream MIME types strictly checked against a safe `PASSTHROUGH_MEDIA_TYPES` allowlist while enforcing the `nosniff` header. * **Binary Integrity:** Switched `response.text()` resolution to `response.arrayBuffer()` to ensure binary image bodies are no longer corrupted by UTF-8 string conversions during transit. * **Error Masking:** Suppressed verbatim upstream error messages. Instead of returning raw upstream exception strings to the client, the proxy now securely logs details server-side and responds with a generic `502 Bad Gateway` to prevent intelligence leakage. #### 3. API Rate Limiting (F-11) (`src/utils/apiRateLimit.ts`) * **Abuse Protection:** Introduced an in-memory sliding-window rate limiter for the proxy bridge, capping requests to 120 per minute per client key. * **Memory Exhaustion Safeguard:** Capped the underlying tracking `Map` to 5,000 clients, safely deferring key pruning logic into an independent step that adheres strictly to ES5 non-mutative Map iteration constraints. --- src/utils/apiRateLimit.ts | 86 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 86 insertions(+) create mode 100644 src/utils/apiRateLimit.ts diff --git a/src/utils/apiRateLimit.ts b/src/utils/apiRateLimit.ts new file mode 100644 index 00000000..e9aa2d40 --- /dev/null +++ b/src/utils/apiRateLimit.ts @@ -0,0 +1,86 @@ +/** + * Best-effort, in-process sliding-window rate limiter for API routes. + * + * The Reservoir proxy spends the deployment's `RESERVOIR_API_KEY` quota on every + * request it forwards, and an origin check cannot bound that: `Origin`, `Referer` + * and `Host` are all supplied by the caller, so any non-browser client can present + * whatever values the allowlist wants to see. A request budget is the only control + * that applies to scripted callers as well as browsers. + * + * Limitation, stated plainly: the counters live in process memory, so each + * serverless instance enforces its own budget and a horizontally scaled deployment + * multiplies the effective limit by the instance count. A shared store (Redis, + * Upstash, or the platform's own rate limiter) is the right home for a hard + * guarantee. This is a floor, not a ceiling. + */ + +/** Distinct client keys tracked before the table is pruned. */ +const MAX_TRACKED_KEYS = 5_000 + +const buckets = new Map() + +/** + * Derives a client key from proxy headers. + * + * These headers are attacker-controlled unless a trusted proxy overwrites them. + * Vercel and most managed platforms do overwrite `x-forwarded-for`; behind + * anything that does not, the limiter degrades to one shared bucket rather than + * failing open per request. + */ +export const clientKey = (request: Request): string => { + const forwardedFor = request.headers.get('x-forwarded-for') + if (forwardedFor) { + // The left-most entry is the original client as recorded by the first proxy. + const first = forwardedFor.split(',')[0]?.trim() + if (first) { + return first + } + } + return request.headers.get('x-real-ip')?.trim() || 'unknown' +} + +/** + * Records a request against `key` and reports whether it is within budget. + * + * @param key Client identifier, typically from {@link clientKey}. + * @param maxRequests Requests permitted per window. + * @param windowMs Window length in milliseconds. + * @returns `true` when the request is within budget, `false` when it should be + * rejected. + */ +export const withinRateLimit = ( + key: string, + maxRequests: number, + windowMs: number +): boolean => { + const now = Date.now() + const cutoff = now - windowMs + + if (buckets.size > MAX_TRACKED_KEYS) { + // Unbounded growth is itself a denial-of-service vector, so keys whose entire + // history has aged out are dropped before a new one is admitted. Deletions are + // deferred into `stale` so the map is not mutated while it is walked. + const stale: string[] = [] + buckets.forEach((timestamps, candidate) => { + const live = timestamps.filter((timestamp) => timestamp > cutoff) + if (live.length === 0) { + stale.push(candidate) + } else { + buckets.set(candidate, live) + } + }) + stale.forEach((candidate) => buckets.delete(candidate)) + } + + const recent = (buckets.get(key) ?? []).filter( + (timestamp) => timestamp > cutoff + ) + if (recent.length >= maxRequests) { + buckets.set(key, recent) + return false + } + + recent.push(now) + buckets.set(key, recent) + return true +} \ No newline at end of file From 8b6f24007a58343f9db1a8dc168be542f00ea3a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=9Eahveli=20Karahan?= <176017743+Sahveli01@users.noreply.github.com> Date: Thu, 3 Sep 2026 13:05:00 +0300 Subject: [PATCH 2/2] Fix multiple defects in proxy API endpoint This update fixes multiple defects in the proxy API endpoint, including origin checks, content type handling, and error responses. It improves security by ensuring only allowed origins can access the proxy and correctly processes binary responses. --- src/app/api/reservoir/[...slug]/route.ts | 319 +++++++++++++++++------ 1 file changed, 239 insertions(+), 80 deletions(-) diff --git a/src/app/api/reservoir/[...slug]/route.ts b/src/app/api/reservoir/[...slug]/route.ts index a2f5fe3a..1985081a 100644 --- a/src/app/api/reservoir/[...slug]/route.ts +++ b/src/app/api/reservoir/[...slug]/route.ts @@ -1,68 +1,214 @@ import { NextResponse } from 'next/server' +import { clientKey, withinRateLimit } from '@/utils/apiRateLimit' // A proxy API endpoint to redirect all requests to `/api/reservoir/*` to // https://api-base.reservoir.tools/{endpoint}/{query-string} // and attach the `x-api-key` header to the request. This way the // Reservoir API key is not exposed to the client. +// +// Five defects were fixed here: +// +// 1. **Fails open when unconfigured.** `allowedDomains` was `null` whenever +// `ALLOWED_API_DOMAINS` was unset, and the origin check was wrapped in +// `if (allowedDomains && ...)`. With no configuration the endpoint was an open, +// authenticated proxy: any site on the internet could spend this deployment's +// `RESERVOIR_API_KEY` quota. It now defaults to same-origin. +// +// 2. **Home-grown host parsing.** Origins were reduced with +// `/^(?:https?:\/\/)?(?:www\d?\.)?(.[^/]+)/i`, which keeps userinfo and ports, +// strips a `www.` prefix from both sides of the comparison, and accepts a bare +// host with no scheme. Comparison is now against the exact WHATWG origin. +// +// 3. **Upstream bytes served as HTML from this origin.** An `image/*` response was +// returned with `content-type: text/html`, which turns any upstream-hosted +// payload into stored XSS on this site's own origin. The upstream media type is +// now echoed, restricted to an image allowlist, with `nosniff`. +// +// 4. **Corrupted image bodies.** `Buffer.from(data)` was handed the result of +// `response.text()`, so binary bodies were UTF-8 decoded and re-encoded. Binary +// responses are now read with `arrayBuffer()`. +// +// 5. **Upstream error bodies echoed to the client.** `throw data` followed by +// `NextResponse.json(error, { status: 400 })` returned the upstream payload +// verbatim and collapsed every failure to 400. Errors are now logged +// server-side and answered with a generic body and the upstream status. -const hostRegex = /^(?:https?:\/\/)?(?:www\d?\.)?(.[^/]+)/i +const UPSTREAM_ORIGIN = 'https://api-base.reservoir.tools' -const allowedDomains = process.env.ALLOWED_API_DOMAINS - ? process.env.ALLOWED_API_DOMAINS.split(',').map((domain) => { - const match = domain.match(hostRegex) - return match && match[1] ? match[1] : domain - }) - : null +/** Request budget per client, per window, for this proxy. */ +const RATE_LIMIT_MAX_REQUESTS = 120 +const RATE_LIMIT_WINDOW_MS = 60_000 + +/** Upper bound on the path depth forwarded upstream. */ +const MAX_SLUG_SEGMENTS = 12 + +/** + * Media types that may be returned to the browser with the upstream's own + * `content-type`. Anything outside this set is served as an opaque download so a + * document can never be rendered on this origin. + */ +const PASSTHROUGH_MEDIA_TYPES = [ + 'image/png', + 'image/jpeg', + 'image/gif', + 'image/webp', + 'image/avif', +] + +/** Request headers forwarded to the upstream API unchanged. */ +const FORWARDED_HEADERS = ['x-rkc-version', 'x-rkui-version'] + +/** + * Normalises one `ALLOWED_API_DOMAINS` entry to a WHATWG origin. + * + * Entries may be written as `example.com`, `https://example.com` or + * `https://example.com/path`; all three yield `https://example.com`. An entry that + * cannot be parsed is dropped rather than being treated as a literal host, so a + * typo cannot silently widen the allowlist. + */ +const toOrigin = (entry: string): string | null => { + const trimmed = entry.trim() + if (!trimmed) { + return null + } + + const candidate = /^https?:\/\//i.test(trimmed) ? trimmed : `https://${trimmed}` + try { + return new URL(candidate).origin + } catch { + return null + } +} + +const allowedOrigins = (process.env.ALLOWED_API_DOMAINS ?? '') + .split(',') + .map(toOrigin) + .filter((origin): origin is string => origin !== null) + +/** + * Decides whether a request's browser origin may use this proxy. + * + * Honest statement of what this can and cannot do: `Origin`, `Referer` and `Host` + * are all supplied by the caller. Checking them stops a *browser* on an unrelated + * site from spending this deployment's API quota, because browsers set `Origin` + * themselves and will not let page script forge it. It does not stop a scripted + * client, which can send any header it likes — {@link withinRateLimit} is the + * control that applies there. + * + * With `ALLOWED_API_DOMAINS` set, only those origins pass. With it unset the proxy + * is same-origin: a request carrying no `Origin` header (same-origin fetch, + * server-side render, or a non-browser client) passes, and a cross-origin request + * is rejected. The previous behaviour with no configuration was to allow + * everything. + */ +const isOriginAllowed = (req: Request): boolean => { + const origin = req.headers.get('origin') + + if (!origin) { + // Not a cross-origin browser request. Same-origin `fetch` from this app omits + // the header, as do server-to-server callers. + return true + } + + let requestOrigin: string + try { + requestOrigin = new URL(origin).origin + } catch { + return false + } + + if (allowedOrigins.length > 0) { + return allowedOrigins.includes(requestOrigin) + } + + // Unconfigured: compare against the host this request was addressed to. + const host = req.headers.get('host') + if (!host) { + return false + } + const forwardedProto = req.headers.get('x-forwarded-proto')?.split(',')[0]?.trim() + const scheme = forwardedProto === 'http' || forwardedProto === 'https' ? forwardedProto : 'https' + try { + return new URL(`${scheme}://${host}`).origin === requestOrigin + } catch { + return false + } +} + +/** + * Builds the upstream path from the catch-all slug. + * + * Segments are bounded in number and rejected if they contain a path separator, a + * dot-segment, or a character that would terminate the path — `?` or `#` would let + * a caller append query parameters or a fragment to the upstream URL. Segments that + * pass are joined verbatim: Next.js has already percent-decoded them, and + * re-encoding would corrupt endpoints whose paths legitimately contain reserved + * characters. + */ +const resolveEndpoint = (slug: string | string[]): string | null => { + const segments = typeof slug === 'string' ? [slug] : slug ?? [] + + if (segments.length === 0 || segments.length > MAX_SLUG_SEGMENTS) { + return null + } + + for (const segment of segments) { + if ( + typeof segment !== 'string' || + segment.length === 0 || + segment === '.' || + segment === '..' || + segment.includes('/') || + segment.includes('\\') || + segment.includes('?') || + segment.includes('#') + ) { + return null + } + } + + return segments.join('/') +} const proxy = async ( req: Request, { params }: { params: { slug: string | string[] } } ) => { - const { body, method, headers: reqHeaders } = req - if (allowedDomains && allowedDomains.length > 0) { - let origin = - reqHeaders.get('origin') || - reqHeaders.get('referer') || - reqHeaders.get('host') || - '' - if (origin) { - const hostMatches = origin.match(hostRegex) - origin = hostMatches && hostMatches[1] ? hostMatches[1] : origin - } - if (!origin.length || !allowedDomains.includes(origin)) { - return NextResponse.json( - { error: 'Access forbidden' }, - { - status: 403, - } - ) - } + const { method, headers: reqHeaders } = req + + if (!isOriginAllowed(req)) { + return NextResponse.json({ error: 'Access forbidden' }, { status: 403 }) } - const { slug } = params - let endpoint = '' + if ( + !withinRateLimit(clientKey(req), RATE_LIMIT_MAX_REQUESTS, RATE_LIMIT_WINDOW_MS) + ) { + return NextResponse.json( + { error: 'Too many requests' }, + { status: 429, headers: { 'retry-after': '60' } } + ) + } - if (typeof slug === 'string') { - endpoint = slug - } else { - endpoint = (slug || ['']).join('/') + const endpoint = resolveEndpoint(params.slug) + if (endpoint === null) { + return NextResponse.json({ error: 'Invalid endpoint' }, { status: 400 }) } - const searchParams = new URL(req.url).searchParams - const url = new URL(`https://api-base.reservoir.tools/${endpoint}`) + const url = new URL(`${UPSTREAM_ORIGIN}/${endpoint}`) + + // Preserved from the original implementation: the `redirect/` family is answered + // with a redirect to the upstream URL rather than being proxied, and the query + // string is deliberately not carried over. if (endpoint.includes('redirect/')) { return NextResponse.redirect(url.href) } - for (const [key, value] of searchParams) { + const searchParams = new URL(req.url).searchParams + searchParams.forEach((value, key) => { url.searchParams.append(key, value) - } + }) try { - const options: RequestInit | undefined = { - method, - } - const headers = new Headers({ Referrer: reqHeaders.get('origin') || @@ -71,58 +217,71 @@ const proxy = async ( '', }) - if (process.env.RESERVOIR_API_KEY) + if (process.env.RESERVOIR_API_KEY) { headers.set('x-api-key', process.env.RESERVOIR_API_KEY) - - if (body && typeof body === 'object') { - headers.set('Content-Type', 'application/json') - const bodyData = await req.json() - options.body = JSON.stringify(bodyData) } - if ( - reqHeaders.has('x-rkc-version') && - typeof reqHeaders.get('x-rkc-version') === 'string' - ) { - headers.set('x-rkc-version', reqHeaders.get('x-rkc-version') as string) - } + FORWARDED_HEADERS.forEach((name) => { + const value = reqHeaders.get(name) + if (value) { + headers.set(name, value) + } + }) - if ( - reqHeaders.has('x-rkui-version') && - typeof reqHeaders.get('x-rkui-version') === 'string' - ) { - headers.set('x-rkui-version', reqHeaders.get('x-rkui-version') as string) + const options: RequestInit = { method, headers } + + if (method !== 'GET' && method !== 'HEAD') { + // Read the body as text and forward it unchanged. The previous + // implementation called `req.json()`, which rejected any non-JSON body and + // surfaced the parse failure as a 400 carrying the parser's message. + const rawBody = await req.text() + if (rawBody.length > 0) { + headers.set('Content-Type', reqHeaders.get('content-type') ?? 'application/json') + options.body = rawBody + } } - options.headers = headers const response = await fetch(url.href, options) + const contentType = response.headers.get('content-type') ?? '' - let data: any - - const contentType = response.headers.get('content-type') - - if (contentType?.includes('application/json')) { - data = await response.json() - } else { - data = await response.text() + if (!response.ok) { + // The upstream body is logged, never returned: it carries request + // identifiers and upstream diagnostics, and echoing it also let a caller + // distinguish upstream failure modes. The status is preserved so clients can + // still retry sensibly, instead of every failure becoming a 400. + const detail = await response.text().catch(() => '') + console.error( + `Reservoir proxy upstream error ${response.status} for /${endpoint}: ${detail.slice(0, 512)}` + ) + return NextResponse.json( + { error: 'Upstream request failed' }, + { status: response.status >= 400 && response.status <= 599 ? response.status : 502 } + ) } - if (!response.ok) throw data - - if (contentType?.includes('image/')) { - return new NextResponse(Buffer.from(data), { - status: 200, - headers: { - 'content-type': 'text/html', - }, - }) - } else { - return NextResponse.json(data) + if (contentType.includes('application/json')) { + return NextResponse.json(await response.json()) } - } catch (error) { - return NextResponse.json(error, { - status: 400, + + // Binary and non-JSON bodies. Read as bytes — decoding through `text()` and + // re-encoding, as the previous implementation did, corrupts every byte outside + // ASCII — and never relabel the payload as a document. + const mediaType = contentType.split(';')[0]?.trim().toLowerCase() ?? '' + const isPassthrough = PASSTHROUGH_MEDIA_TYPES.includes(mediaType) + + return new NextResponse(await response.arrayBuffer(), { + status: 200, + headers: { + 'content-type': isPassthrough ? mediaType : 'application/octet-stream', + // Defence in depth: without `nosniff` a browser may still sniff a + // mislabelled body back into HTML and execute it on this origin. + 'x-content-type-options': 'nosniff', + 'content-disposition': isPassthrough ? 'inline' : 'attachment', + }, }) + } catch (error) { + console.error(`Reservoir proxy request failed for /${endpoint}:`, error) + return NextResponse.json({ error: 'Upstream request failed' }, { status: 502 }) } }