diff --git a/.changeset/promql-proxy-preserve-host-path.md b/.changeset/promql-proxy-preserve-host-path.md new file mode 100644 index 0000000000..1d549c25e2 --- /dev/null +++ b/.changeset/promql-proxy-preserve-host-path.md @@ -0,0 +1,37 @@ +--- +'@hyperdx/api': minor +--- + +fix: preserve a Connection host path prefix when proxying PromQL. +`proxyToPrometheus` joined absolute Prometheus paths (`/api/v1/query_range`, +`/api/v1/query`, `/api/v1/query_exemplars`, `/api/v1/label/.../values`) with +`new URL(path, host)`, which replaces the host pathname instead of appending to +it. VictoriaMetrics cluster `vmselect` URLs such as +`http://vmselect:8481/select/0/prometheus` were rewritten to +`/api/v1/query_range` and rejected. The join now keeps the existing pathname. + +This is a behavior change for Connections whose host already included a path +that was never meant as a Prometheus API prefix — for example +`http://prom:9090/graph` copied from the Prometheus UI. That previously happened +to work because the absolute API path replaced `/graph`; requests now go to +`/graph/api/v1/query_range` and will 404. Trim stray paths from existing +Connection hosts before upgrading. Root-mounted hosts (`http://prom:9090` or +`http://prom:9090/`) are unchanged. + +Query parameters on the Connection host are now only a fallback for a fixed set +of real Prometheus API params (`query`, `time`, `start`, `end`, `step`, +`match`/`match[]`, `limit`, `timeout`, `stats`): a request value for one of +these (including repeatable ones such as `match[]`) always wins and replaces a +same-named host value outright, rather than being dropped. Any other host query +key the request never mentions -- for example `?extra_label=namespace%3Dprod` +pinning a VictoriaMetrics tenant scope -- is left as-is and is never overridable +by the request, since a param name outside that fixed set is not forwarded at +all regardless of what the host carries. This also means a host copied with a +stray query string (not just a stray path) now forwards its non-Prometheus keys +upstream as a fallback on every request -- trim those too if they weren't +intended as Prometheus API params. + +This is also a behavior change for a direct API caller (e.g. curl or Terraform) +that previously relied on sending an arbitrary, non-Prometheus query param +through this endpoint: that param is now silently dropped rather than forwarded, +regardless of whether the Connection host carries anything under the same name. diff --git a/packages/api/src/routers/api/__tests__/prometheus.int.test.ts b/packages/api/src/routers/api/__tests__/prometheus.int.test.ts index d79d3685f3..8dc75637d6 100644 --- a/packages/api/src/routers/api/__tests__/prometheus.int.test.ts +++ b/packages/api/src/routers/api/__tests__/prometheus.int.test.ts @@ -254,6 +254,71 @@ describe('prometheus router', () => { expect(res.body.error).not.toContain('s3cr3t'); }); + // Unlike basic-auth userinfo, a secret pinned in the host's own query + // string (e.g. VictoriaMetrics `?authKey=...`) is preserved verbatim on + // the upstream URL by the param merge -- `redactedTarget` must drop the + // whole query string for display, not try to redact only the + // secret-shaped parts of it, or this leaks into the 502 body. + it('does not leak a secret pinned in the host query string into the 502 message', async () => { + const { agent, team } = await getLoggedInAgent(server); + const conn = await seedPrometheusConnection( + team._id, + 'http://prom.example.com?authKey=super-secret-token', + ); + + mockFetch.mockRejectedValueOnce( + Object.assign(new Error('fetch failed'), { + cause: { code: 'ECONNREFUSED' }, + }), + ); + + const res = await agent + .get('/v1/prometheus/query_exemplars') + .query({ + query: 'up', + start: '1700000000', + end: '1700000060', + connectionId: conn._id.toString(), + }) + .expect(502); + + expect(res.body.error).toContain('ECONNREFUSED'); + expect(res.body.error).not.toContain('super-secret-token'); + expect(res.body.error).toContain('prom.example.com'); + }); + + // The 504 timeout branch builds its message from the same `redactedTarget` + // as the 502 branch, but had no test of its own -- pin it separately so a + // regression isolated to this branch (e.g. someone reintroducing the raw + // `target` here specifically) would still be caught. + it('does not leak credentials or a host query-string secret into the 504 message', async () => { + const { agent, team } = await getLoggedInAgent(server); + const conn = await seedPrometheusConnection( + team._id, + 'http://user:s3cr3t@prom.example.com?authKey=super-secret-token', + ); + + mockFetch.mockRejectedValueOnce( + Object.assign(new Error('The operation was aborted'), { + name: 'TimeoutError', + }), + ); + + const res = await agent + .get('/v1/prometheus/query_exemplars') + .query({ + query: 'up', + start: '1700000000', + end: '1700000060', + connectionId: conn._id.toString(), + }) + .expect(504); + + expect(res.body.error).not.toContain('s3cr3t'); + expect(res.body.error).not.toContain('super-secret-token'); + expect(res.body.error).toContain('prom.example.com'); + }); + // nosniff is set by router middleware, so the helper's own error bodies — // which echo the caller-supplied host — carry it too. it('sends nosniff on its own error responses', async () => { @@ -273,6 +338,56 @@ describe('prometheus router', () => { expect(res.headers['x-content-type-options']).toBe('nosniff'); }); + it('returns 400 for a scheme-less connection host instead of 502', async () => { + const { agent, team } = await getLoggedInAgent(server); + const conn = await seedPrometheusConnection(team._id, 'prometheus:9090'); + + const res = await agent + .get('/v1/prometheus/query_range') + .query({ + query: 'up', + start: '1700000000', + end: '1700000060', + step: '15s', + connectionId: conn._id.toString(), + }) + .expect(400); + + expect(res.body).toMatchObject({ + status: 'error', + errorType: 'bad_data', + error: expect.stringContaining('http(s)'), + }); + expect(mockFetch).not.toHaveBeenCalled(); + }); + + // The 400 branch never echoes the Connection host at all (rather than + // trying to redact it), so this pins the actual call site: reverting to + // interpolate the raw host back in would fail the suite. + it('never echoes the connection host (or any credentials in it) in the 400 body', async () => { + const { agent, team } = await getLoggedInAgent(server); + const conn = await seedPrometheusConnection( + team._id, + 'user:secret-password@prometheus:9090', + ); + + const res = await agent + .get('/v1/prometheus/query_range') + .query({ + query: 'up', + start: '1700000000', + end: '1700000060', + step: '15s', + connectionId: conn._id.toString(), + }) + .expect(400); + + expect(res.body.error).not.toContain('secret-password'); + expect(res.body.error).not.toContain('prometheus:9090'); + expect(res.body.error).toContain('http(s)'); + expect(mockFetch).not.toHaveBeenCalled(); + }); + it('proxies to upstream Prometheus when connection isPrometheusEndpoint', async () => { const { agent, team } = await getLoggedInAgent(server); const conn = await seedPrometheusConnection(team._id); @@ -303,6 +418,270 @@ describe('prometheus router', () => { expect(calledUrl).not.toContain('connectionId'); }); + // Pins the call site, not just the helper: reverting + // `url = new URL(path, upstreamHost)` would drop `/select/0/prometheus`. + it('keeps a VictoriaMetrics cluster path prefix on the upstream URL', async () => { + const { agent, team } = await getLoggedInAgent(server); + const conn = await seedPrometheusConnection( + team._id, + 'http://vmselect:8481/select/0/prometheus', + ); + + mockFetch.mockResolvedValueOnce( + fakeUpstreamResponse({ + status: 'success', + data: { resultType: 'matrix', result: [] }, + }), + ); + + await agent + .get('/v1/prometheus/query_range') + .query({ + query: 'up', + start: '1700000000', + end: '1700000060', + step: '15s', + connectionId: conn._id.toString(), + }) + .expect(200); + + expect(mockFetch).toHaveBeenCalledTimes(1); + expect(mockFetch.mock.calls[0][0] as string).toMatch( + /^http:\/\/vmselect:8481\/select\/0\/prometheus\/api\/v1\/query_range/, + ); + }); + + // Every other userinfo test asserts what's redacted for *display* (the + // error bodies below). The actual outgoing `fetch` target must still + // carry real credentials, or authenticated Prometheus connections would + // silently stop authenticating. + it('still sends real credentials to the actual upstream fetch call', async () => { + const { agent, team } = await getLoggedInAgent(server); + const conn = await seedPrometheusConnection( + team._id, + 'http://user:pw@prom.example.com', + ); + + mockFetch.mockResolvedValueOnce( + fakeUpstreamResponse({ + status: 'success', + data: { resultType: 'matrix', result: [] }, + }), + ); + + await agent + .get('/v1/prometheus/query_range') + .query({ + query: 'up', + start: '1700000000', + end: '1700000060', + step: '15s', + connectionId: conn._id.toString(), + }) + .expect(200); + + expect(mockFetch.mock.calls[0][0] as string).toContain('user:pw@'); + }); + + it('leaves a host-pinned param untouched when the request never references it', async () => { + const { agent, team } = await getLoggedInAgent(server); + const conn = await seedPrometheusConnection( + team._id, + 'http://vmselect:8481/select/0/prometheus?extra_label=namespace%3Dprod', + ); + + mockFetch.mockResolvedValueOnce( + fakeUpstreamResponse({ + status: 'success', + data: { resultType: 'matrix', result: [] }, + }), + ); + + await agent + .get('/v1/prometheus/query_range') + .query({ + query: 'up', + start: '1700000000', + end: '1700000060', + step: '15s', + connectionId: conn._id.toString(), + }) + .expect(200); + + const requested = new URL(mockFetch.mock.calls[0][0] as string); + expect(requested.searchParams.get('extra_label')).toBe('namespace=prod'); + expect(requested.searchParams.get('query')).toBe('up'); + }); + + // A host-pinned Prometheus-native param the request DOES supply a value + // for must not silently win: a Connection host could otherwise override + // `query`, `match[]`, or `limit` and the caller would get a wrong answer + // with no error (e.g. a pinned `query=up` making every chart on that + // connection return the same series regardless of what was requested). + it('lets a request query param override a same-named one pinned on the connection host', async () => { + const { agent, team } = await getLoggedInAgent(server); + const conn = await seedPrometheusConnection( + team._id, + 'http://vmselect:8481/select/0/prometheus?query=host_pinned_query', + ); + + mockFetch.mockResolvedValueOnce( + fakeUpstreamResponse({ + status: 'success', + data: { resultType: 'matrix', result: [] }, + }), + ); + + await agent + .get('/v1/prometheus/query_range') + .query({ + query: 'up', + start: '1700000000', + end: '1700000060', + step: '15s', + connectionId: conn._id.toString(), + }) + .expect(200); + + const requested = new URL(mockFetch.mock.calls[0][0] as string); + expect(requested.searchParams.get('query')).toBe('up'); + }); + + // `timeout` and `stats` are real Prometheus API params on + // /api/v1/query(_range) -- omitting them from the allowlist (as an + // earlier version of this fix did) would silently drop a request's + // value rather than forward it, contradicting the "request always wins" + // guarantee for every param this list claims to cover. + it('forwards a request timeout, overriding one pinned on the connection host', async () => { + const { agent, team } = await getLoggedInAgent(server); + const conn = await seedPrometheusConnection( + team._id, + 'http://prom.example.com?timeout=5s', + ); + + mockFetch.mockResolvedValueOnce( + fakeUpstreamResponse({ + status: 'success', + data: { resultType: 'matrix', result: [] }, + }), + ); + + await agent + .get('/v1/prometheus/query_range') + .query({ + query: 'up', + start: '1700000000', + end: '1700000060', + step: '15s', + timeout: '30s', + connectionId: conn._id.toString(), + }) + .expect(200); + + const requested = new URL(mockFetch.mock.calls[0][0] as string); + expect(requested.searchParams.get('timeout')).toBe('30s'); + }); + + // `stats` is `timeout`'s sibling in the allowlist -- covered separately + // so a regression dropping just `stats` (and not `timeout`) would still + // be caught. + it('forwards a request stats value, overriding one pinned on the connection host', async () => { + const { agent, team } = await getLoggedInAgent(server); + const conn = await seedPrometheusConnection( + team._id, + 'http://prom.example.com?stats=all', + ); + + mockFetch.mockResolvedValueOnce( + fakeUpstreamResponse({ + status: 'success', + data: { resultType: 'matrix', result: [] }, + }), + ); + + await agent + .get('/v1/prometheus/query_range') + .query({ + query: 'up', + start: '1700000000', + end: '1700000060', + step: '15s', + stats: 'none', + connectionId: conn._id.toString(), + }) + .expect(200); + + const requested = new URL(mockFetch.mock.calls[0][0] as string); + expect(requested.searchParams.get('stats')).toBe('none'); + }); + + // Unlike a real Prometheus API param, an arbitrary key the request + // happens to also send (here VictoriaMetrics's own `extra_label`, which + // a Connection host may pin as a tenant-isolation scope) must NOT be + // caller-overridable: `getParams` spreads the entire incoming + // query/body with no allowlist, so without restricting the merge to + // real Prometheus params, any request could un-pin a host's scoping + // param just by naming it. + it('does not let a request override a non-Prometheus param pinned on the connection host', async () => { + const { agent, team } = await getLoggedInAgent(server); + const conn = await seedPrometheusConnection( + team._id, + 'http://vmselect:8481/select/0/prometheus?extra_label=namespace%3Dprod', + ); + + mockFetch.mockResolvedValueOnce( + fakeUpstreamResponse({ + status: 'success', + data: { resultType: 'matrix', result: [] }, + }), + ); + + await agent + .get('/v1/prometheus/query_range') + .query({ + query: 'up', + start: '1700000000', + end: '1700000060', + step: '15s', + extra_label: 'namespace=other', + connectionId: conn._id.toString(), + }) + .expect(200); + + const requested = new URL(mockFetch.mock.calls[0][0] as string); + expect(requested.searchParams.get('extra_label')).toBe('namespace=prod'); + expect(requested.searchParams.get('query')).toBe('up'); + }); + + it('lets the request step override a step pinned on the connection host', async () => { + const { agent, team } = await getLoggedInAgent(server); + const conn = await seedPrometheusConnection( + team._id, + 'http://prom.example.com?step=5m', + ); + + mockFetch.mockResolvedValueOnce( + fakeUpstreamResponse({ + status: 'success', + data: { resultType: 'matrix', result: [] }, + }), + ); + + await agent + .get('/v1/prometheus/query_range') + .query({ + query: 'up', + start: '1700000000', + end: '1700000060', + step: '15s', + connectionId: conn._id.toString(), + }) + .expect(200); + + const requested = new URL(mockFetch.mock.calls[0][0] as string); + expect(requested.searchParams.get('step')).toBe('15s'); + }); + it('does NOT proxy to Prometheus when connection is not isPrometheusEndpoint', async () => { const { agent, team } = await getLoggedInAgent(server); const conn = await seedClickHouseConnection(team._id); @@ -537,6 +916,28 @@ describe('prometheus router', () => { expect(requested.searchParams.has('limit')).toBe(false); }); + // `limit: '0'` is falsy, so a naive `limit ? {...} : {}` guard on the + // route drops it before it ever reaches proxyToPrometheus's merge loop -- + // a host-pinned limit would then silently survive even though the + // request explicitly asked for "unlimited". Pin a non-zero limit on the + // host and confirm the request's 0 still clears it (rather than 0 itself + // reaching upstream, which the previous test guards separately). + it('lets a request limit of 0 clear a non-zero limit pinned on the connection host', async () => { + const { agent, team } = await getLoggedInAgent(server); + const conn = await seedPrometheusConnection( + team._id, + 'http://prom.example.com?limit=100', + ); + + await agent + .get('/v1/prometheus/label/job/values') + .query({ connectionId: conn._id.toString(), limit: '0' }) + .expect(200); + + const requested = new URL(String(mockFetch.mock.calls[0][0])); + expect(requested.searchParams.has('limit')).toBe(false); + }); + it('rejects a negative limit', async () => { const { agent, team } = await getLoggedInAgent(server); const conn = await seedPrometheusConnection(team._id); @@ -655,6 +1056,33 @@ describe('prometheus router', () => { expect(requested.searchParams.has('match[]')).toBe(false); }); + // A repeatable param needs its own regression test: a naive fix could + // append the request's values alongside a host-pinned one instead of + // replacing it, silently narrowing what upstream is asked for (e.g. a + // host pinning `match[]` would otherwise make label-value autocomplete + // return the wrong series list). + it('replaces a host-pinned match[] with the request values, not appends alongside them', async () => { + const { agent, team } = await getLoggedInAgent(server); + const conn = await seedPrometheusConnection( + team._id, + 'http://prom.example.com?match%5B%5D=host-pinned-series', + ); + + await agent + .get('/v1/prometheus/label/job/values') + .query({ + connectionId: conn._id.toString(), + match: ['requested-series-1', 'requested-series-2'], + }) + .expect(200); + + const requested = new URL(String(mockFetch.mock.calls[0][0])); + expect(requested.searchParams.getAll('match[]')).toEqual([ + 'requested-series-1', + 'requested-series-2', + ]); + }); + // queryLabelValues' own bounds/limit/fallback behaviour is covered in // controllers/__tests__/timeseriesEngine.int.test.ts. What only the route // can show is that query-string params reach it intact — bounds in unix @@ -1077,6 +1505,39 @@ describe('prometheus router', () => { expect(end - sentStart).toBe(PROMETHEUS_MAX_EXEMPLAR_WINDOW_SEC); }); + it('lets the clamped exemplar window override start/end pinned on the host', async () => { + const { agent, team } = await getLoggedInAgent(server); + const end = 1700000000; + const thirtyDays = 30 * 24 * 60 * 60; + // Pinned an hour earlier than the request's `end` so the assertion + // below can only pass if the request's value actually won -- if the + // host's pinned value were used instead, `end` wouldn't match. + const hostEnd = end - 3600; + const conn = await seedPrometheusConnection( + team._id, + `http://prom.example.com?start=${hostEnd - thirtyDays}&end=${hostEnd}`, + ); + + mockFetch.mockResolvedValueOnce( + fakeUpstreamResponse({ status: 'success', data: [] }), + ); + + await agent + .get('/v1/prometheus/query_exemplars') + .query({ + query: 'up', + start: String(end - thirtyDays), + end: String(end), + connectionId: conn._id.toString(), + }) + .expect(200); + + const requested = new URL(mockFetch.mock.calls[0][0] as string); + const sentStart = Number(requested.searchParams.get('start')); + expect(Number(requested.searchParams.get('end'))).toBe(end); + expect(end - sentStart).toBe(PROMETHEUS_MAX_EXEMPLAR_WINDOW_SEC); + }); + it('returns an empty result for ClickHouse-backed connections (no native exemplar table function)', async () => { const { agent, team } = await getLoggedInAgent(server); const conn = await seedClickHouseConnection(team._id); diff --git a/packages/api/src/routers/api/__tests__/prometheus.test.ts b/packages/api/src/routers/api/__tests__/prometheus.test.ts index 4fa1848083..dbf11461bf 100644 --- a/packages/api/src/routers/api/__tests__/prometheus.test.ts +++ b/packages/api/src/routers/api/__tests__/prometheus.test.ts @@ -12,6 +12,7 @@ import { formatMatrixResponse, formatVectorResponse, isClientDisconnect, + joinPrometheusUpstreamUrl, parseDuration, parseTimestamp, recordProxyOutcome, @@ -257,3 +258,81 @@ describe('isClientDisconnect', () => { expect(isClientDisconnect(undefined)).toBe(false); }); }); + +describe('joinPrometheusUpstreamUrl', () => { + it('keeps a root-mounted Prometheus host working', () => { + expect( + joinPrometheusUpstreamUrl( + 'http://prometheus:9090', + '/api/v1/query_range', + ).toString(), + ).toBe('http://prometheus:9090/api/v1/query_range'); + }); + + it('keeps a trailing slash on a root-mounted host from doubling the path', () => { + expect( + joinPrometheusUpstreamUrl( + 'http://prometheus:9090/', + '/api/v1/query_range', + ).toString(), + ).toBe('http://prometheus:9090/api/v1/query_range'); + }); + + it('preserves a VictoriaMetrics cluster tenant prefix', () => { + expect( + joinPrometheusUpstreamUrl( + 'http://vmselect:8481/select/0/prometheus', + '/api/v1/query_range', + ).toString(), + ).toBe('http://vmselect:8481/select/0/prometheus/api/v1/query_range'); + }); + + it('strips a trailing slash on the tenant prefix before joining', () => { + expect( + joinPrometheusUpstreamUrl( + 'http://vmselect:8481/select/0/prometheus/', + '/api/v1/label/__name__/values', + ).toString(), + ).toBe( + 'http://vmselect:8481/select/0/prometheus/api/v1/label/__name__/values', + ); + }); + + it('preserves userinfo and existing query params on the connection host', () => { + expect( + joinPrometheusUpstreamUrl( + 'http://user:pw@vmselect:8481/select/0/prometheus?extra=1', + '/api/v1/query', + ).toString(), + ).toBe( + 'http://user:pw@vmselect:8481/select/0/prometheus/api/v1/query?extra=1', + ); + }); + + it('strips every trailing slash, not just one, so the join never leaves a double slash', () => { + expect( + joinPrometheusUpstreamUrl( + 'http://prometheus:9090//', + '/api/v1/query_range', + ).toString(), + ).toBe('http://prometheus:9090/api/v1/query_range'); + }); + + it('throws on an invalid connection host, matching the proxy 400 path', () => { + expect(() => + joinPrometheusUpstreamUrl('not-a-url', '/api/v1/query_range'), + ).toThrow(); + }); + + // `new URL('prometheus:9090')` succeeds (scheme `prometheus:`, opaque path). + // Without the http(s) guard the helper would return the host unchanged and + // the proxy would 502 instead of 400. + it('throws on a scheme-less host that URL parses as an opaque path', () => { + expect(() => + joinPrometheusUpstreamUrl('prometheus:9090', '/api/v1/query_range'), + ).toThrow(/http\(s\)/); + expect(() => + joinPrometheusUpstreamUrl('localhost:9090', '/api/v1/query_range'), + ).toThrow(/http\(s\)/); + }); +}); diff --git a/packages/api/src/routers/api/prometheus.ts b/packages/api/src/routers/api/prometheus.ts index 1bd112f393..8ef1b8c77b 100644 --- a/packages/api/src/routers/api/prometheus.ts +++ b/packages/api/src/routers/api/prometheus.ts @@ -185,6 +185,61 @@ export function isClientDisconnect(err: unknown): boolean { ); } +/** + * Join a Connection host with an absolute Prometheus API path. + * + * `new URL('/api/v1/query_range', 'http://host:8481/select/0/prometheus')` + * discards `/select/0/prometheus` because an absolute path replaces the base + * pathname. VictoriaMetrics cluster (and any Prometheus-compatible server + * mounted under a prefix) needs that prefix kept. Host userinfo, query, and + * hash are left untouched. + * + * `path` must be an absolute path (every call site passes a literal starting + * with `/`) -- this is not a general-purpose URL joiner. + * + * @see https://github.com/hyperdxio/hyperdx/issues/3046 + */ +export function joinPrometheusUpstreamUrl( + upstreamHost: string, + path: string, +): URL { + const url = new URL(upstreamHost); + // `new URL('prometheus:9090')` succeeds with an opaque path (`prometheus:` + // scheme). The pathname setter is a no-op there, so without this guard the + // helper would return the host unchanged, `fetch` would fail, and the proxy + // would 502 / increment query_errors for a user misconfiguration. Same check + // as clickhouseProxy.ts. + if (url.protocol !== 'http:' && url.protocol !== 'https:') { + throw new TypeError('Connection host must be http(s)'); + } + // Strip ALL trailing slashes, not just one -- a host saved with a doubled + // trailing slash (e.g. `http://prom:9090//`) would otherwise leave a `//` + // in the joined path, which most servers treat as a distinct (404) path. + const basePath = url.pathname.replace(/\/+$/, ''); + url.pathname = `${basePath}${path}`; + return url; +} + +// Only real Prometheus API params are ever caller-settable in +// proxyToPrometheus's query merge below. `params` there is built upstream by +// spreading the *entire* `req.query`/`req.body` with no allowlist (see +// `getParams`), so without this, a request could supply an arbitrary key -- +// e.g. VictoriaMetrics's `extra_label`, which a Connection host may pin as a +// tenant-isolation scope -- and un-pin or override it, even though no +// legitimate caller ever sends that key. +const CALLER_SETTABLE_PARAM_KEYS = new Set([ + 'query', + 'time', + 'start', + 'end', + 'step', + 'match', + 'match[]', + 'limit', + 'timeout', + 'stats', +]); + // Forwards the response straight from the upstream Prometheus to the // HyperDX client. Returns the HTTP status it wrote, so callers can record an // error metric: this helper handles its own failures by writing 400/502/504 and @@ -203,19 +258,40 @@ async function proxyToPrometheus( ): Promise { let url: URL; try { - url = new URL(path, upstreamHost); - } catch { + url = joinPrometheusUpstreamUrl(upstreamHost, path); + } catch (err) { + // Not echoing `upstreamHost` at all -- a redaction regex on an arbitrary + // (possibly malformed, since this is the failure branch) string can + // always be wrong for some shape (e.g. a `/` preceding the credentials). + // `err.message` is always safe to show: it's either this function's own + // fixed "Connection host must be http(s)", or `URL`'s parse-failure + // message, which is the fixed string "Invalid URL" and never echoes the + // input (verified against Node's URL implementation). res.status(400).json({ status: 'error', errorType: 'bad_data', - error: `Connection host is not a valid URL: ${JSON.stringify(upstreamHost)}`, + error: `Invalid Connection host: ${err instanceof Error ? err.message : String(err)}`, }); return 400; } + // For a key in CALLER_SETTABLE_PARAM_KEYS, the request always wins + // outright, including repeatable ones like `match[]` -- so a Connection + // host can never silently override (or, for `match[]`, narrow) a value + // the caller or this proxy explicitly sets. A host-only param outside + // that set (e.g. VictoriaMetrics's `extra_label`) is left as-is. for (const [k, v] of Object.entries(params)) { - if (['connectionId', 'database', 'table'].includes(k)) continue; + if (!CALLER_SETTABLE_PARAM_KEYS.has(k)) continue; if (v == null) continue; - // A repeatable param (`match[]`) appends every value + // Clear any host-pinned value at this key first: for a repeatable param + // this prevents an `append` from leaving the host's value(s) alongside + // the request's rather than replacing them. This also means a request's + // `limit=0` still clears a host-pinned `limit` (see below) even though + // it isn't itself re-set. + url.searchParams.delete(k); + // Prometheus reads a limit of 0 as "unlimited", the same as omitting the + // key entirely -- forward that by omission too, rather than a literal + // "0", so upstream sees the same request shape it always has. + if (k === 'limit' && v === '0') continue; if (Array.isArray(v)) { for (const item of v) url.searchParams.append(k, item); } else { @@ -224,15 +300,16 @@ async function proxyToPrometheus( } const target = url.toString(); - // A connection host may carry basic-auth credentials (`http://user:pw@host`), - // and the error bodies below are shown in the browser. Strip them for display - // only — `target` itself still needs them to authenticate. - const redactedTarget = (() => { - const safe = new URL(url); - safe.username = ''; - safe.password = ''; - return safe.toString(); - })(); + // A connection host may carry basic-auth credentials (`http://user:pw@host`) + // or a secret pinned in its own query string (e.g. VictoriaMetrics + // `?authKey=...`, preserved by the merge above for any key outside + // `CALLER_SETTABLE_PARAM_KEYS`), and the error bodies below are shown in + // the browser. `url.origin` never includes userinfo (WHATWG URL spec), and + // dropping the query/hash entirely -- rather than trying to redact only the + // secret-shaped parts of it -- means there's no query key to ever miss. + // `target` itself is unaffected and still carries everything needed to + // authenticate. + const redactedTarget = `${url.origin}${url.pathname}`; let upstreamResp: Response; try { @@ -869,7 +946,12 @@ async function handleLabelLookup( { ...(start != null ? { start: String(start) } : {}), ...(end != null ? { end: String(end) } : {}), - ...(limit ? { limit: String(limit) } : {}), + // `limit` is 0 or absent when validation passes (see the schema + // above); 0 means "unlimited" to Prometheus and must still be + // forwarded (`v == null` is the merge loop's own absence check), + // otherwise a request explicitly asking for unlimited results + // silently loses to a host-pinned `limit`. + ...(limit != null ? { limit: String(limit) } : {}), // Restored under the name Prometheus expects ...(match != null ? { 'match[]': match } : {}), },