Repository navigation
fix: refuse Prometheus endpoint responses over 8 MB #3541
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| '@hyperdx/api': patch | ||
| --- | ||
|
|
||
| fix: Refuse Prometheus endpoint responses over 8 MB in MCP PromQL label lookups and queries, instead of buffering them whole |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ import { | |
| } from '@hyperdx/common-utils/dist/clickhouse'; | ||
| import { getCurrentQueryAttribution } from '@hyperdx/common-utils/dist/clickhouse/node'; | ||
| import type { PromqlQuery } from '@hyperdx/common-utils/dist/core/promqlChart'; | ||
| import { parseJSON } from '@hyperdx/common-utils/dist/core/utils'; | ||
| import { SourceKind, type SQLInterval } from '@hyperdx/common-utils/dist/types'; | ||
| import { z } from 'zod'; | ||
|
|
||
|
|
@@ -20,12 +21,28 @@ import { | |
| queryRangeViaTableFunction, | ||
| } from '@/controllers/timeseriesEngine'; | ||
| import { withSpan } from '@/utils/instrumentation'; | ||
| import { | ||
| readBoundedBody, | ||
| ResponseTooLargeError, | ||
| } from '@/utils/readBoundedBody'; | ||
|
|
||
| type PromqlBackend = 'prometheus' | 'clickhouse'; | ||
|
|
||
| const PROMETHEUS_FETCH_TIMEOUT_MS = 30_000; | ||
| const PROMETHEUS_CH_TIMEOUT_MS = 30_000; | ||
| export const PROMETHEUS_MAX_RESOLUTION = 11_000; | ||
| /** | ||
| * Largest Prometheus response body fetchPrometheusData will read into memory. | ||
| * | ||
| * Queries to ClickHouse are already capped by `max_result_rows` | ||
| * (PROMETHEUS_MAX_RESULT_ROWS). An external Prometheus has no equivalent: | ||
| * `/query` and `/query_range` take no row limit, and older servers ignore | ||
| * `limit` on the label APIs. So the only way to bound the memory a response | ||
| * can take is to stop reading at a byte limit. 8 MB fits roughly | ||
| * PROMETHEUS_MAX_RESULT_ROWS label values, so both backends allow about the | ||
| * same amount of data. | ||
| */ | ||
| export const PROMETHEUS_MAX_RESPONSE_BYTES = 8 * 1024 * 1024; | ||
|
|
||
| // ClickHouse serves the Prometheus HTTP API under this prefix on its main HTTP | ||
| // port when the `prometheus_api_v1` handler is configured (26.6+). | ||
|
|
@@ -176,9 +193,20 @@ async function fetchPrometheusData<T extends z.ZodTypeAny>( | |
| ); | ||
| } | ||
|
|
||
| const body = prometheusBodySchema.safeParse( | ||
| await resp.json().catch(() => undefined), | ||
| ); | ||
| let text: string; | ||
| try { | ||
| text = await readBoundedBody(resp, PROMETHEUS_MAX_RESPONSE_BYTES); | ||
| } catch (err) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 minor — A timeout or abort while reading the body is reported as a generic read failure, not as a timeout The fetch Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond |
||
| const tooLargeMessage = `Prometheus at ${redactedTarget} returned more than ${PROMETHEUS_MAX_RESPONSE_BYTES / (1024 * 1024)} MB. Narrow the query or time range.`; | ||
| const genericMessage = `Failed to read the response from Prometheus at ${redactedTarget}`; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 minor — A timeout while reading the body is reported as "Failed to read the response", not as a timeout The 30 s Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond |
||
| throw new PromQLQueryError( | ||
| err instanceof ResponseTooLargeError ? tooLargeMessage : genericMessage, | ||
| 'server', | ||
| ); | ||
| } | ||
| // A body that is not JSON (a proxy's HTML error page, say) is reported by | ||
| // its HTTP status below. | ||
| const body = prometheusBodySchema.safeParse(parseJSON(text)); | ||
| if (!resp.ok || !body.success || body.data.status !== 'success') { | ||
| const isUserError = | ||
| resp.status >= 400 && | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| import { | ||
| readBoundedBody, | ||
| ResponseTooLargeError, | ||
| } from '@/utils/readBoundedBody'; | ||
|
|
||
| const MAX = 4 * 1024; | ||
|
|
||
| const streamOf = ( | ||
| pull: (controller: ReadableStreamDefaultController<Uint8Array>) => void, | ||
| onCancel: () => void, | ||
| ) => | ||
| new ReadableStream<Uint8Array>({ | ||
| pull, | ||
| cancel() { | ||
| onCancel(); | ||
| }, | ||
| }); | ||
|
|
||
| describe('readBoundedBody', () => { | ||
| it('refuses a declared Content-Length over the cap without reading the body', async () => { | ||
| let cancelled = false; | ||
| const body = streamOf( | ||
| c => c.enqueue(new TextEncoder().encode('{')), | ||
| () => (cancelled = true), | ||
| ); | ||
| const resp = new Response(body, { | ||
| headers: { 'content-length': String(MAX + 1) }, | ||
| }); | ||
|
|
||
| await expect(readBoundedBody(resp, MAX)).rejects.toBeInstanceOf( | ||
| ResponseTooLargeError, | ||
| ); | ||
| expect(cancelled).toBe(true); | ||
| expect(resp.bodyUsed).toBe(true); | ||
| }); | ||
|
|
||
| it('stops reading an undeclared-length body at the cap', async () => { | ||
| const chunk = new Uint8Array(1024).fill(0x20); | ||
| let pulled = 0; | ||
| let cancelled = false; | ||
| const body = streamOf( | ||
| c => { | ||
| pulled++; | ||
| c.enqueue(chunk); | ||
| }, | ||
| () => (cancelled = true), | ||
| ); | ||
|
|
||
| await expect( | ||
| readBoundedBody(new Response(body), MAX), | ||
| ).rejects.toBeInstanceOf(ResponseTooLargeError); | ||
| expect(cancelled).toBe(true); | ||
| // One chunk per KB, plus the one that crossed the cap and the stream's | ||
| // read-ahead. | ||
| expect(pulled).toBeLessThanOrEqual(MAX / chunk.byteLength + 2); | ||
| }); | ||
|
|
||
| it('joins a chunked body under the cap into one string', async () => { | ||
| const text = 'héllo wörld'; | ||
| const bytes = new TextEncoder().encode(text); | ||
| const body = new ReadableStream<Uint8Array>({ | ||
| start(c) { | ||
| c.enqueue(bytes.slice(0, 3)); | ||
| c.enqueue(bytes.slice(3)); | ||
| c.close(); | ||
| }, | ||
| }); | ||
|
|
||
| await expect(readBoundedBody(new Response(body), MAX)).resolves.toBe(text); | ||
| }); | ||
|
|
||
| it('returns an empty string for a response with no body', async () => { | ||
| await expect( | ||
| readBoundedBody(new Response(null, { status: 204 }), MAX), | ||
| ).resolves.toBe(''); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| /** Thrown by readBoundedBody when a response is larger than its cap. */ | ||
| export class ResponseTooLargeError extends Error { | ||
| constructor(maxBytes: number) { | ||
| super(`Response body exceeds ${maxBytes} bytes`); | ||
| this.name = 'ResponseTooLargeError'; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Reads a fetch Response body as text without ever holding more than | ||
| * `maxBytes` of it. A declared Content-Length over the cap is refused before | ||
| * any of the body is read; an undeclared (chunked) body is read until the cap | ||
| * and then cancelled. Use it for bodies from hosts we do not control, where | ||
| * `resp.text()` / `resp.json()` would buffer whatever the server sends. | ||
| */ | ||
| export async function readBoundedBody( | ||
| resp: Response, | ||
| maxBytes: number, | ||
| ): Promise<string> { | ||
| const declared = Number(resp.headers.get('content-length')); | ||
| if (declared > maxBytes) { | ||
| await resp.body?.cancel(); | ||
| throw new ResponseTooLargeError(maxBytes); | ||
| } | ||
| if (!resp.body) return ''; | ||
|
|
||
| const reader = resp.body.getReader(); | ||
| const chunks: Uint8Array[] = []; | ||
| let received = 0; | ||
| for (;;) { | ||
| const { done, value } = await reader.read(); | ||
| if (done) break; | ||
| received += value.byteLength; | ||
| if (received > maxBytes) { | ||
| await reader.cancel(); | ||
| throw new ResponseTooLargeError(maxBytes); | ||
| } | ||
| chunks.push(value); | ||
| } | ||
| return Buffer.concat(chunks).toString('utf8'); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟠 major — The 8 MB cap was sized for label lists, but it also applies to query_range matrices and to the ClickHouse HTTP backend
fetchPrometheusDataalso serves/queryand/query_range, forkind: 'prometheus'and forclickhouse-http(around line 607). Theclickhouse-httppath is already bounded bymax_result_rows, which is set inclickhousePrometheusUpstream. Each matrix sample is about 28 bytes of JSON, so 8 MB holds roughly 300k samples. That is only about 300 series × 1,000 points, far underPROMETHEUS_MAX_RESOLUTION(11,000 points per series) × 100k rows. Arate(...) by (pod)tile that works on the table-function backend now fails here with 'Narrow the query'. Fix: give range and instant queries their own larger cap, or skip the byte cap forclickhouse-httpsincemax_result_rowsalready bounds it. Then correct the doc comment's claim that 'both backends allow about the same amount of data'.Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond