Skip to content

fix: preserve Connection host path when proxying PromQL - #3047

Merged
kodiakhq[bot] merged 19 commits into
hyperdxio:mainfrom
milansanjeev:fix/promql-proxy-preserve-host-path
Sep 4, 2026
Merged

fix: preserve Connection host path when proxying PromQL#3047
kodiakhq[bot] merged 19 commits into
hyperdxio:mainfrom
milansanjeev:fix/promql-proxy-preserve-host-path

Conversation

@milansanjeev

Copy link
Copy Markdown
Contributor

Summary

proxyToPrometheus built the upstream URL with new URL(path, upstreamHost). Every proxied path is absolute (/api/v1/query_range, /api/v1/query, /api/v1/query_exemplars, /api/v1/label/${labelName}/values), so URL()replaces the Connection host pathname instead of appending to it.

That silently drops prefixes such as VictoriaMetrics cluster's required /select/<accountID>/prometheus. Root-mounted Prometheus / Thanos / VM single-node are unchanged.

Fixes#3046

Test plan

  • Unit tests for joinPrometheusUpstreamUrl in packages/api/src/routers/api/__tests__/prometheus.test.ts
    • root-mounted host (with and without trailing slash)
    • VictoriaMetrics cluster tenant prefix (with and without trailing slash)
    • userinfo + existing query params preserved
    • invalid host still throws (proxy 400 path)
  • yarn ci:unit src/routers/api/__tests__/prometheus.test.ts — 46/46 passing

Suggested join

consturl=newURL(upstreamHost);url.pathname=url.pathname.replace(/\/$/,'')+path;

@changeset-bot

changeset-botBot commented Sep 1, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 9b06e71

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
NameType
@hyperdx/apiMinor
@hyperdx/appMinor
@hyperdx/otel-collectorMinor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel

vercelBot commented Sep 1, 2026

Copy link
Copy Markdown

@cursoragent is attempting to deploy a commit to the HyperDX Team on Vercel.

A member of the Team first needs to authorize it.

@github-actionsgithub-actionsBot added external needs-vouch Author needs a maintainer to vouch for them labels Sep 1, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Hi @milansanjeev, thanks for the pull request!

Before we review code from a first-time contributor we ask that a maintainer vouches for you, and you're not on our list yet. This PR stays open — it just isn't in the review queue until someone vouches.

To get vouched, open an issue saying hello and what you're working on:

https://github.com/hyperdxio/hyperdx/issues/new?template=introduce-yourself.md

A maintainer will usually reply within a day or two, and then this PR gets picked up as normal. More detail in our contributing guide.

@milansanjeevmilansanjeev mentioned this pull request Sep 1, 2026
@greptile-apps

greptile-appsBot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR preserves Connection host path prefixes when constructing Prometheus upstream URLs and tightens query-parameter merging and error redaction.

  • Adds a dedicated URL join helper for prefixed Prometheus-compatible endpoints.
  • Ensures request-supplied Prometheus parameters override matching host parameters while retaining host-pinned extension parameters.
  • Adds unit and integration coverage for path prefixes, parameter precedence, zero limits, invalid hosts, and credential redaction.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

FilenameOverview
packages/api/src/routers/api/prometheus.tsIntroduces prefix-preserving upstream URL construction, controlled parameter precedence, safer error display URLs, and explicit zero-limit handling.
packages/api/src/routers/api/tests/prometheus.test.tsAdds focused unit coverage for root and prefixed hosts, trailing slashes, URL components, and invalid schemes.
packages/api/src/routers/api/tests/prometheus.int.test.tsAdds route-level regression coverage for URL prefixes, parameter merging, limit semantics, and error redaction.
.changeset/promql-proxy-preserve-host-path.mdDocuments the corrected prefix behavior and compatibility implications for existing Connection host paths and query parameters.

Sequence Diagram

sequenceDiagram
participant Client
participant API as HyperDX API
participant Upstream as Prometheus-compatible upstream
Client->>API: PromQL request + Connection ID
API->>API: Load Connection host
API->>API: Append API path to host prefix
API->>API: Merge permitted request parameters
API->>Upstream: Fetch preserved-prefix URL
Upstream-->>API: Prometheus response
API-->>Client: Proxied response
Loading

Reviews (21): Last reviewed commit: "Merge branch 'main' into fix/promql-prox..." | Re-trigger Greptile

@github-actions

github-actionsBot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Deep Review

Deep Review

Scope: git diff f0d1cef5 …HEADpackages/api/src/routers/api/prometheus.ts, its two test files, and the changeset. Intent: preserve a Connection host path prefix (e.g. VictoriaMetrics /select/0/prometheus) when proxying PromQL, restrict which request params can override host-pinned ones, and stop host query-string secrets leaking into proxy error bodies.

✅ No critical issues found. The core fix is correct and well covered: joinPrometheusUpstreamUrl preserves the host pathname, guards non-http(s) hosts (restoring the 400 path), strips all trailing slashes, and keeps userinfo on the outgoing fetch while the redacted display target (${url.origin}${url.pathname}) drops both userinfo and the query string. The prior major concern — a host ?authKey= secret leaking into 502/504 bodies — is resolved and pinned by integration tests. The param allowlist change does not regress the HyperDX app, which only ever sends allowlisted keys (query/start/end/step, verified in packages/app/src/api.ts:634).

🟡 P2 — recommended

  • packages/api/src/routers/api/prometheus.ts:230 — The CALLER_SETTABLE_PARAM_KEYS allowlist forwards only vanilla-Prometheus params, so backend-specific query params a direct API caller previously could send to a Thanos or VictoriaMetrics connection (dedup, partial_response, max_source_resolution, nocache, round_digits) are now silently dropped, returning a differently-computed answer with no error rather than a failure.
    • Fix: Invert the rule to forward any request key that is not HyperDX-internal while leaving keys already pinned on the host non-overridable, or reject an unknown param with a 400 instead of dropping it silently.
    • api-contract, previous-comments
🔵 P3 nitpicks (3)
  • packages/api/src/routers/api/prometheus.ts:949 — The comment states limit is "0 or absent when validation passes", but labelLookupRequestQuerySchema accepts any non-negative integer (an existing test sends limit: '25'), so the note wrongly implies String(limit) is always '0'.

    • Fix: Reword to state limit is a non-negative integer or absent, where 0 means unlimited to Prometheus and is forwarded by omission.
  • packages/api/src/routers/api/prometheus.ts:283 — The merge loop re-filters the label-lookup params against the allowlist even though handleLabelLookup already constructed them from a validated schema, so a future validated param silently forwards nothing until the distant Set is also edited.

    • Fix: Apply the allowlist only where raw getParams(req) enters the three untrusted handlers and let proxyToPrometheus forward whatever object it is handed.
  • packages/api/src/routers/api/__tests__/prometheus.int.test.ts:373 — No test drives a genuinely unparseable host carrying credentials through the new 400 branch, so the safety of interpolating err.message (rather than the host) rests on a comment rather than an assertion.

    • Fix: Add a case asserting a malformed host with embedded userinfo yields a 400 whose body contains neither the host nor the secret.

Reviewers (6): correctness, security, api-contract, testing, previous-comments, adversarial.

Testing gaps: No test asserts that a non-allowlisted request param (e.g. dedup) is dropped rather than forwarded, so a future revert of the allowlist restriction would not be caught at the route level.

Comment threadpackages/api/src/routers/api/__tests__/prometheus.test.ts
Comment threadpackages/api/src/routers/api/prometheus.ts
Comment threadpackages/api/src/routers/api/prometheus.ts Outdated
@github-actions

github-actionsBot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

PR Review

2 finding(s): 🔴 0 critical · 🟠 0 major · 🔵 2 minor

2 posted as inline comment(s) on the changed lines.


Severity is the reviewer's own estimate and is used for ordering, not filtering.

@milansanjeev

Copy link
Copy Markdown
ContributorAuthor

Addressed the review comments in 2d4a8d1:

  • Moved joinPrometheusUpstreamUrl above the proxyToPrometheus doc block.
  • Integration test pins the fetch call site for a VictoriaMetrics cluster host (/select/0/prometheus/api/v1/query_range).
  • Changeset notes the behavior change for Connection hosts that included a non-prefix path such as /graph.
  • Unit test for a leading-slash-less path.
  • Request query params no longer overwrite keys already pinned on the Connection host; added an integration collision test.

Comment threadpackages/api/src/routers/api/prometheus.ts Outdated
Comment threadpackages/api/src/routers/api/prometheus.ts Outdated
Comment thread.changeset/promql-proxy-preserve-host-path.md
@jordan-simonovski

Copy link
Copy Markdown
Contributor

/vouch @milansanjeev

@github-actionsgithub-actionsBot mentioned this pull request Sep 1, 2026
@pulpdrew
pulpdrew self-requested a review September 1, 2026 18:58
@pulpdrew

Copy link
Copy Markdown
Contributor

Hey @milansanjeev, thanks for the contribution. Looks like there is a merge conflict. Tf you wouldn't mind fixing that up, I can review after that 👍

new URL(absolutePath, base) replaces the base pathname, so a
VictoriaMetrics cluster host like
http://vmselect:8481/select/0/prometheus dropped /select/0/prometheus
and hit /api/v1/query_range instead.
Join the existing pathname with the Prometheus API path. Covers
query_range, query, query_exemplars, and label values.
Fixeshyperdxio#3046
Move joinPrometheusUpstreamUrl above the proxyToPrometheus doc block,
keep operator-pinned host query params from being overwritten, and pin
the fetch call site with a VictoriaMetrics cluster integration test.
Also document that Connection hosts with a stray UI path (e.g. /graph)
will 404 after this change.
@milansanjeev
milansanjeevforce-pushed the fix/promql-proxy-preserve-host-path branch from 2d4a8d1 to 70b886dCompareSeptember 2, 2026 06:14
@milansanjeev

Copy link
Copy Markdown
ContributorAuthor

@pulpdrew Thank you so much for ack. Merge conflict resolved so please check now.

Comment threadpackages/api/src/routers/api/prometheus.ts
Reject non-http(s) Connection hosts in joinPrometheusUpstreamUrl so a
scheme-less host 400s instead of 502ing. Keep start/end/step under the
proxy so exemplar clamping and chart resolution still win over host-pinned
params, and record both behavior changes in a minor changeset.
@milansanjeev

Copy link
Copy Markdown
ContributorAuthor

Addressed the remaining review notes in 5c2e40e:

  • Scheme-less Connection hosts (prometheus:9090 / localhost:9090) now 400 again instead of 502.
  • Host-pinned query params stay, documented in the changeset; start/end/step are exempt so the exemplar window clamp and chart resolution still win.
  • Changeset bumped from patch to minor.

@pulpdrew one process ask: milansanjeev is already on .github/VOUCHED.td (vouch PR merged) but this PR still has the needs-vouch label. The Vouch workflow only adds that label on open; it never strips it after a later vouch. I cannot remove it from a fork. Could you drop the label when you pick this up?

Comment threadpackages/api/src/routers/api/prometheus.ts Outdated
Comment threadpackages/api/src/routers/api/prometheus.ts Outdated
Comment threadpackages/api/src/routers/api/prometheus.ts Outdated
- joinPrometheusUpstreamUrl: strip ALL trailing slashes from the host
pathname (was only stripping one), so a host saved with a doubled
trailing slash doesn't leave a `//` in the joined path.
- Drop the dead relative-path branch (every call site passes an
absolute path); document the contract instead. Removes the unit
test that only exercised that branch.
- Invert query-param precedence in proxyToPrometheus: every param the
request supplies now always wins over a same-named one pinned on the
Connection host -- including repeatable params like match[], via an
explicit delete before append so host and request values don't end
up coexisting. A host-only param the request never references (e.g.
a VictoriaMetrics extra_label tenant scope) is left untouched. This
replaces the previous PROXY_OWNED_PARAM_KEYS allowlist, which let a
host silently override caller-owned params like query/match[]/limit.
- proxyToPrometheus's invalid-host 400 branch: redact userinfo from
the raw host before interpolating it into the response, and surface
the caught error's own message instead of one generic string, so a
wrong-scheme host and an unparseable host get distinct, actionable
400 bodies.
- Rewrite the host-pinned-param test that asserted the OLD (now
reverted) precedence; add a test for the new override behavior, a
match[]-specific regression test (replace, not append-alongside),
and fix the exemplar-window test that pinned the same `end` value on
both host and request (making the assertion pass regardless of
which side actually won).
- Update the changeset to describe the corrected param precedence.
@milansanjeevmilansanjeev mentioned this pull request Sep 3, 2026
@milansanjeev

Copy link
Copy Markdown
ContributorAuthor

@jordan-simonovski@pulpdrew this PR still shows needs-vouch even though I am already on .github/VOUCHED.td (vouch via #3048 / #3049).

Opened a follow-up intro issue so this can get unblocked: #3070

Could a maintainer drop the needs-vouch label here? The Vouch workflow never removes it after a later vouch, and I cannot change labels on this repo from the fork.

Comment threadpackages/api/src/routers/api/prometheus.ts Outdated
…passwords
The 400 branch's ad-hoc redaction (`replace(/:\/\/[^/@]*@/, '://')`) only
strips credentials when the host contains `://`. That's exactly the case
the http(s) scheme guard exists to catch: a host saved as `user:pw@prom:9090`
(no scheme) parses with scheme `user:` and an opaque path, never contains
`://`, and would echo the raw password straight back into the browser-visible
error body -- the opposite of the redaction's own stated intent.
Extracted a shared `redactHostUserinfo` helper (anchored, optional scheme and
`//`) used by both the 400 branch and the success-path `redactedTarget`,
replacing the URL-object-based approach there with the same string helper so
there's one redaction implementation instead of two that could drift. Added
unit tests for the scheme-less case, the normal case, a no-userinfo host, and
an "@" appearing later in a query value (must not be touched).
…the helper
Only redactHostUserinfo itself was unit-tested; reverting the 400
branch to echo the raw Connection host verbatim (pre-redaction-fix
behavior) would still pass the whole suite. Add an integration case
seeding a scheme-less host with embedded credentials and asserting
the 400 body omits the password while still naming the host.
…irst occurrence
redactHostUserinfo's userinfo-matching group excluded "@" from what it
consumed, so it stopped at the first "@" in the userinfo segment --
a password containing a literal "@" left its suffix in the redacted
output. Match through the last "@" before the first path separator
instead (still bounded by "/" so query-string "@"s are untouched).
Also documents the known, accepted limitation that a scheme-less host
like `user:pw@prom:9090` (which parses with scheme `user:`, ambiguous
with real userinfo) only guarantees the password is stripped, not the
username, and that the changeset should tell operators to trim a
stray host query string too, not just a stray path.
Comment threadpackages/api/src/routers/api/prometheus.ts Outdated
Comment threadpackages/api/src/routers/api/prometheus.ts Outdated
…st; fix limit=0 loss
Three issues from the latest review round:
1. getParams() spreads the entire req.query/req.body with no allowlist, so
the "request always wins" merge loop let a request override ANY
same-named key on the Connection host -- including non-Prometheus keys
like VictoriaMetrics's extra_label, which a host may pin as a
tenant-isolation scope. Restrict the merge to a fixed set of real
Prometheus API params (query, time, start, end, step, match, match[],
limit); anything else the request supplies is now ignored, so a
caller can never un-pin a host-only param just by naming it.
2. The /label/:name/values route dropped `limit=0` before it ever reached
proxyToPrometheus (`limit ? {...} : {}` treats 0 as absent), so a
request explicitly asking for "unlimited" silently lost to a
host-pinned limit. Forward it based on presence (`limit != null`), and
have the merge loop turn a request `limit=0` into deleting any
host-pinned limit rather than sending a literal "0" upstream --
preserving the existing "0 forwards as absent" contract while still
letting the request's 0 clear a host value.
3. The 400 branch's redacted-host echo is fundamentally fragile: a
regex-based redactor can always be wrong for some malformed shape (this
round found one: a "/" preceding credentials defeats the last-fix's
[^/]*@ boundary). Stop echoing the host at all -- err.message alone is
always safe, since it's either this function's own fixed string or
URL's fixed "Invalid URL" (verified against Node, never echoes input).
Rewrote the extra_label test to demonstrate the new (correct) "host-only
param survives" behavior instead of the old "request overrides it"
behavior it was asserting; added tests for query overriding a host-pinned
query, limit=0 clearing a host-pinned limit, and a happy-path fetch-target
assertion that real credentials still reach the actual upstream call (only
the display-redaction path had coverage before).
Comment threadpackages/api/src/routers/api/prometheus.ts Outdated
Comment threadpackages/api/src/routers/api/prometheus.ts Outdated
…t userinfo
Preserving the Connection host's path prefix (this PR's whole point) also
now preserves its query string on the joined URL. A secret pinned there
(e.g. VictoriaMetrics `?authKey=...`) flows into `target` and, since
`redactHostUserinfo` only ever stripped userinfo, would be echoed
verbatim in the 502/504 error bodies shown in the browser -- a new
exposure this PR created (the pre-fix `new URL(path, host)` discarded the
host's query entirely, so this case couldn't previously occur).
Build the displayed target from `url.origin` + `url.pathname` only,
dropping the query/hash outright rather than trying to redact only the
secret-shaped parts of it (there's no way to tell a legitimate host query
value apart from a pinned secret once merged into one URL). `url.origin`
never includes userinfo per the URL spec, which also makes it this
function's only remaining caller -- the 400 branch stopped echoing the
host entirely in the previous commit -- so redactHostUserinfo has no
production call site left. Removed it and its dedicated unit tests rather
than leave unused code around.
Added an integration test asserting a host-query-string secret is absent
from a 502 body (mirrors the existing basic-auth-userinfo 502 test).
- Hoist CALLER_SETTABLE_PARAM_KEYS to module scope instead of
reallocating it on every proxyToPrometheus call.
- Fix a stale test comment referencing the now-removed
redactHostUserinfo helper.
- Add a 504-timeout-branch test mirroring the existing 502 one,
pinning that it also drops both userinfo and a host query-string
secret from the error message (previously only the 502 path had
direct coverage).
CALLER_SETTABLE_PARAM_KEYS omitted timeout and stats -- both real
Prometheus API params on /api/v1/query and /api/v1/query_range. Under
the old denylist a request's ?timeout=5s was forwarded; with the new
allowlist it was silently dropped instead, and the query would run to
this proxy's own PROMETHEUS_PROXY_TIMEOUT_MS. Added both keys and a
regression test forwarding a request timeout that overrides one pinned
on the host.
Also corrected the changeset, which claimed "any param the request
supplies always wins" -- no longer true once the allowlist exists.
…behavior
stats and timeout were added to the allowlist together but only timeout
had a request-overrides-host regression test, so a regression dropping
just stats would still pass. Add the same test shape for stats.
Also make the changeset explicit that a direct API caller's own
non-Prometheus query param (not just a host-pinned one) is now silently
dropped rather than forwarded, since the allowlist applies regardless of
who supplied the param name.
// 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([

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 major — The new param allowlist silently drops Thanos/VictoriaMetrics query params that were forwarded before

CALLER_SETTABLE_PARAM_KEYS covers only vanilla Prometheus params, but the old loop forwarded everything except connectionId/database/table, and Thanos is a first-class target here — ConnectionForm.tsx:280 uses http://thanos-querier:10902 as the host placeholder. A caller that sends dedup=false, partial_response=true or max_source_resolution=5m (or VictoriaMetrics's nocache=1, latency_offset, round_digits) to /v1/prometheus/query_range now has those stripped with no error, and gets a differently-deduplicated answer instead of a failure. If the goal is only to stop a request un-pinning a host-scoped param, invert it: forward any request key that is not HyperDX-internal, and make keys already present on the Connection host non-overridable — that protects extra_label without a hand-maintained list of every backend's params.

// 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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 minor — Allowlist re-filters params the label-values route already built from a validated schema, two sources of truth

/label/:name/values (lines 927-943) constructs its params object explicitly from labelValuesRequestQuerySchema, then proxyToPrometheus filters that object again against a Set defined 650 lines earlier. Adding a newly validated param there (say limit_per_metric) forwards nothing and fails no test until someone remembers to also edit CALLER_SETTABLE_PARAM_KEYS. Apply the allowlist where untrusted params enter — a callerSettableParams(req) wrapper used by the three handlers that pass raw getParams(req) — and let proxyToPrometheus forward what it is given.

...(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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 minor — Comment claims limit can only be 0 or absent, but the schema accepts any non-negative integer

labelValuesRequestQuerySchema uses z.coerce.number().int().nonnegative().optional(), and the existing test forwards limit upstream (prometheus.int.test.ts:880) sends limit: '25'. Reword to "limit is a non-negative integer or absent; 0 means unlimited to Prometheus and must still be forwarded" so nobody later reads this as a guarantee that String(limit) is always '0'.

expect(res.body.error).toContain('prom.example.com');
});

// nosniff is set by router middleware, so the helper's own error bodies —

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 minor — Test comment says the helper's error bodies echo the caller-supplied host, which this PR made false

The 400 branch now interpolates only err.message and never upstreamHost — the test added at line 373 asserts exactly that. Drop the "which echo the caller-supplied host" clause; nosniff on the 400 body is worth pinning on its own.

// 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}`;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 minor — Existing Connections with a stray host path break on upgrade with an undiagnosable 404

A host like http://prom:9090/graph used to work because the absolute API path replaced /graph; it now requests /graph/api/v1/query_range, and the proxy streams upstream's 404 straight through while recordProxyOutcome skips 4xx, so there is no counter and no hint that the Connection host is the cause. The changeset asks users to trim these by hand before upgrading, which self-hosted operators will not see. At minimum log a warning (with the connection id, not the host) when a proxied response is 404 and url.pathname had a non-empty prefix, so the failure is traceable.

@pulpdrewpulpdrew added automerge and removed needs-vouch Author needs a maintainer to vouch for them labels Sep 3, 2026
pulpdrew
pulpdrew previously approved these changes Sep 3, 2026

@pulpdrewpulpdrew left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the contribution. Please fix the lint / formatting error

milansanjeevand others added 2 commits September 4, 2026 08:49
The only real lint failure on this PR -- a multi-line expect() call
that prettier wants collapsed to one line, plus the changeset's prose
wrap. No behavior change.
upstreamHost: string,
path: string,
): URL {
const url = new URL(upstreamHost);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 minor — Preserving the host query string (not just the path) is what creates both documented regressions

Issue #3046 is about the pathname; keeping url.search is incidental to how the join is written (new URL(host) inherits the query by construction). That single side effect is the reason this PR also needs CALLER_SETTABLE_PARAM_KEYS, the delete-then-set merge, and the redactedTarget rewrite — and it is the sole cause of the two behaviour changes the changeset has to warn about (an arbitrary caller param is now silently dropped; a host saved with a stray query string now forwards its keys upstream on every request, including any ?authKey= that then has to be redacted out of the 502/504 bodies). Adding url.search = ''; in joinPrometheusUpstreamUrl keeps the fix to exactly the reported bug, leaves the previous ['connectionId','database','table'] skip loop untouched, and makes the redaction change unnecessary. If host-pinned params (extra_label, authKey) are genuinely wanted, they are a separate feature worth its own PR — and worth surfacing in ConnectionForm rather than depending on whatever a user pasted into the Host field.

// 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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 minor — The param allowlist also filters the route-built params from handleLabelLookup, giving "which params exist" two hand-synced sources of truth

handleLabelLookup (prometheus.ts:943-959) hands proxyToPrometheus a closed object it constructs itself from labelLookupRequestQuerySchemastart/end/limit/match[], all already validated — and proxyToPrometheus then re-filters it against a module-level set that must be kept in sync by hand. A key added to the schema and forwarded there but not added to CALLER_SETTABLE_PARAM_KEYS is dropped with no error and no failing test; upstream just answers a different question. Only the three routes that spread raw getParams(req) (query_range:461, query:594, query_exemplars:768) carry untrusted keys, so apply the filter to those spreads (e.g. a proxyParams(getParams(req)) helper at the call sites) and let route-constructed param objects through untouched. Moot if the host query string is dropped in joinPrometheusUpstreamUrl instead.

@milansanjeev

Copy link
Copy Markdown
ContributorAuthor

@pulpdrew the lint/formatting error is fixed in de4026b (prettier formatting in prometheus.int.test.ts + changeset prose-wrap) — verified locally: prettier --check clean, tsc --noEmit clean, all 48 unit tests passing.

Your approval was auto-dismissed by the push (branch protection dismisses stale approvals on new commits), and it looks like the workflow runs on this commit are also sitting in action_required pending approval to run. Whenever you get a chance, could you re-approve/re-run so CI can confirm green?

@kodiakhq
kodiakhqBot merged commit d9c5c45 into hyperdxio:mainSep 4, 2026
25 of 27 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PromQL proxy drops Connection host path prefix (breaks VictoriaMetrics cluster)

4 participants

@milansanjeev@jordan-simonovski@pulpdrew@cursoragent