fix: harden metric scrape edge cases by zeitlinger · Pull Request #2297 · prometheus/client_java

zeitlinger · 2026-07-09T13:53:32Z

Summary

This addresses the bug reports from #2282, #2283, #2284, #2285, #2286, and #2287 in one PR.

Fix Buffer stripe indexing so large thread IDs cannot produce negative stripe indexes.
Bound the Buffer spin wait during collection and replay buffered observations before failing the scrape on timeout.
Return a generic HTTPServer error response instead of exposing stack traces to clients.
Avoid blocking the HTTPServer dispatcher on executor saturation by using a bounded queue with the default rejection policy.
Close rejected authenticated request bodies instead of draining them without a size limit.
Bound default query parsing by query length and parameter count, and return HTTP 400 for invalid query parameters.
De-duplicate exact metric-name filters for faster lookup.
Redact invalid configuration values from PrometheusPropertiesException messages.

Fixes #2282
Fixes #2283
Fixes #2284
Fixes #2285
Fixes #2286
Fixes #2287

Validation

mise run lint:fix
./mvnw -pl prometheus-metrics-core,prometheus-metrics-config,prometheus-metrics-exporter-common,prometheus-metrics-exporter-httpserver,prometheus-metrics-model test -Dcoverage.skip=true -Dcheckstyle.skip=true
mise run build

github-actions · 2026-07-09T13:53:53Z

⚠️ API changes detected — maintainer review required

This PR modifies the published API diff for the following module(s):

prometheus-metrics-core
prometheus-metrics-exporter-common
prometheus-metrics-exporter-httpserver

Please review the changes in docs/apidiffs/current_vs_latest/ carefully before approving.

Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>

Copilot

Pull request overview

This PR hardens the metrics scrape path across core buffering, query parsing/filtering, and the standalone HTTPServer exporter to address multiple reported scrape-triggered crashes/DoS vectors and information disclosure.

Changes:

Make Buffer thread striping overflow-safe and bound spin-wait during collection (replaying buffered observations before failing).
Stop exposing stack traces to HTTP clients; return a generic 500 error body while logging server-side.
Bound query parsing (length + parameter count) and de-duplicate metric-name filters for more predictable scrape cost.

Reviewed changes

Copilot reviewed 22 out of 22 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
prometheus-metrics-model/src/main/java/io/prometheus/metrics/model/registry/MetricNameFilter.java	De-duplicates filter inputs and adds fast-path `contains()` checks.
prometheus-metrics-exporter-httpserver/src/test/java/io/prometheus/metrics/exporter/httpserver/HTTPServerTest.java	Updates tests for new default executor + generic error responses.
prometheus-metrics-exporter-httpserver/src/test/java/io/prometheus/metrics/exporter/httpserver/HttpExchangeAdapterTest.java	Verifies generic 500 body and absence of stack traces.
prometheus-metrics-exporter-httpserver/src/main/java/io/prometheus/metrics/exporter/httpserver/HTTPServer.java	Switches default executor to bounded queue and changes unauthorized-body handling.
prometheus-metrics-exporter-httpserver/src/main/java/io/prometheus/metrics/exporter/httpserver/HttpExchangeAdapter.java	Replaces stack-trace responses with a generic error message + server-side logging.
prometheus-metrics-exporter-httpserver/src/main/java/io/prometheus/metrics/exporter/httpserver/BlockingRejectedExecutionHandler.java	Removes the blocking rejection handler to avoid dispatcher thread stalls.
prometheus-metrics-exporter-common/src/test/java/io/prometheus/metrics/exporter/common/PrometheusScrapeHandlerTest.java	Adds tests for rejecting overly-long / overly-many query parameters.
prometheus-metrics-exporter-common/src/main/java/io/prometheus/metrics/exporter/common/PrometheusScrapeHandler.java	Catches invalid query parameter parsing and returns HTTP 400.
prometheus-metrics-exporter-common/src/main/java/io/prometheus/metrics/exporter/common/PrometheusHttpRequest.java	Implements bounded query parsing for `getParameterValues()`.
prometheus-metrics-exporter-common/src/main/java/io/prometheus/metrics/exporter/common/InvalidQueryParameterException.java	Introduces an internal exception for invalid query parsing.
prometheus-metrics-core/src/test/java/io/prometheus/metrics/core/metrics/BufferTest.java	Adds regression tests for stripe indexing and timeout/replay behavior.
prometheus-metrics-core/src/main/java/io/prometheus/metrics/core/metrics/Buffer.java	Uses `floorMod` for striping; adds spin-wait timeout + replay-on-timeout.
prometheus-metrics-config/src/test/java/io/prometheus/metrics/config/UtilTest.java	Updates tests for redacted/escaped invalid-value error messages.
prometheus-metrics-config/src/test/java/io/prometheus/metrics/config/OpenMetrics2PropertiesTest.java	Updates expected messages to quote/escape invalid values.
prometheus-metrics-config/src/test/java/io/prometheus/metrics/config/ExporterPushgatewayPropertiesTest.java	Updates expected messages to quote/escape invalid values.
prometheus-metrics-config/src/test/java/io/prometheus/metrics/config/ExporterPropertiesTest.java	Updates expected messages to quote/escape invalid values.
prometheus-metrics-config/src/main/java/io/prometheus/metrics/config/Util.java	Centralizes invalid-value message formatting with escaping + truncation.
prometheus-metrics-config/src/main/java/io/prometheus/metrics/config/ExporterPushgatewayProperties.java	Uses `Util.invalidValueMessage(...)` to avoid leaking raw config values.
integration-tests/it-exporter/it-exporter-test/src/test/java/io/prometheus/metrics/it/exporter/test/HttpServerIT.java	Validates generic HTTPServer error body in integration tests.
integration-tests/it-exporter/it-exporter-test/src/test/java/io/prometheus/metrics/it/exporter/test/ExporterIT.java	Factors error-body assertions behind an overridable hook.
docs/apidiffs/current_vs_latest/prometheus-metrics-exporter-httpserver.txt	Records API-diff output for exporter-httpserver changes.
docs/apidiffs/current_vs_latest/prometheus-metrics-exporter-common.txt	Records API-diff output for exporter-common changes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>

jaydeluca

Review of the scrape-hardening changes (#2282–#2287). A few questions/suggestions inline — two worth a look (the new throw reaching the observe() path, and escape-vs-redact for config values); the rest are minor/optional.

jaydeluca · 2026-07-17T13:53:56Z

      observeFunction.accept(buffer[i]);
    }
+    if (timedOut) {
+      throw new IllegalStateException("Timed out while waiting for in-flight observations.");


Throwing here is the intended behavior on the scrape path (Histogram.collect → buffer.run), but run() is also invoked from maybeResetOrScaleDown() / maybeScaleDown() (Histogram.java:439, :470), which sit on the application recording path: observe → doObserve → maybeResetOrScaleDown → buffer.run. Before this change observe() could never throw from here. Under sustained contention or a GC pause > 1 s where in-flight fast-path observers don't settle before the deadline, a native-histogram observe() will now throw an undocumented IllegalStateException into user code — a new failure mode close to the crash class #2282 fixes.

On the internal reset/scale-down path, should we log and defer (skip this cycle's scale-down) instead of throwing, and then only surface the timeout on the scrape/collect() path?

Fixed in ec7f83d. Histogram.collect still calls run with timeout failure enabled, while the reset/scale-down paths call it with failOnTimeout=false. A timeout during maintenance now deactivates and drains the generation, then skips that maintenance cycle instead of throwing into observe(). I also added regression coverage for the timeout/replay behavior.

jaydeluca · 2026-07-17T13:53:56Z

    }
  }
+
+  static String invalidValueMessage(String fullKey, String message, String found) {


Escaping control chars (log-injection / CWE-117 defense) and the 100-char cap are good, but the raw value stays in the message. The client-facing leak via HttpExchangeAdapter is closed by the #2283 fix; however the servlet adapters (servlet-jakarta / javax) re-throw to the container, so up to 100 chars of the offending config value can still land in a container error page.

Fixed in ec7f83d. Invalid configuration messages now include only the property key and expected format; the offending raw value is no longer included. This is also applied to the PushGateway credential-related paths, so servlet/container error propagation cannot expose the configured value.

jaydeluca · 2026-07-17T13:53:56Z

+    return (int) Math.floorMod(threadId, stripeCount);
+  }
+
  private void doAppend(double amount) {


observationBuffer still grows unbounded; the cap is now only implicit via the 1 s active window. Reasonable, but a hard MAX_BUFFER_SIZE with drop-on-overflow would make the bound explicit and independent of the timeout value.

Addressed in ec7f83d. The observation generation now has an explicit DEFAULT_MAX_BUFFER_SIZE of 1,000,000, independent of the timeout. Appends wait for space while the generation is active and stop when it is deactivated; I chose bounded blocking rather than drop-on-overflow so recording does not silently lose observations.

jaydeluca · 2026-07-17T13:53:56Z

        Thread.yield();
      }
-      result = createResult.get();
+      result = timedOut ? null : createResult.get();


The spin-wait timeout covers this first loop, but the second wait loop below (while (bufferPos < expectedBufferSize) bufferFilled.await(), ~line 137) has no timeout: a thread stalled between the stripe increment in append() and doAppend() can still block run() there indefinitely. Narrow window, but the "hang forever" class isn't fully eliminated — consider a bounded await there too.

Fixed in ec7f83d. The buffer handoff is now coordinated through generation/phase transitions, so the second unbounded bufferFilled.await() is gone. Appenders that straddle activation/deactivation are accounted for, and there is regression coverage for a stalled appender.

jaydeluca · 2026-07-17T13:53:56Z

+    this(DEFAULT_MAX_SPIN_WAIT_NANOS);
+  }
+
+  Buffer(long maxSpinWaitNanos) {


The 1 s deadline is only reachable via this package-private constructor (tests). Should we expose a knob for this?

The deadline constructor remains package-private and test-only; there is no public configuration knob. The production constructor continues to use the default one-second deadline.

jaydeluca · 2026-07-17T13:53:56Z

      responseSent = true;
+      logger.log(
+          Level.SEVERE,
+          "The Prometheus metrics HTTPServer caught an Exception during scrape.",


This reverses the prior "avoid logging in Java-agent mode" behavior — correct, since the client no longer gets details. But a persistent misconfiguration will emit a SEVERE line on every scrape. Maybe we should add rate-limiting/deduping the log to avoid flooding logs.

Agreed. I left rate limiting/deduplication as a follow-up rather than expanding this change's scope. This PR removes exception details from the client response and logs the exception server-side, but a persistent misconfiguration can still produce one SEVERE entry per failing scrape. I am leaving this thread unresolved.

Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>

zeitlinger · 2026-07-23T10:05:53Z

Closing this combined PR because its long review history makes the individual fixes difficult to evaluate. I’ll replace it with focused PRs for the separate reports, and each PR will call out any discussion that is still ongoing.

zeitlinger · 2026-07-23T10:16:31Z

Replacement draft PRs:

fix: prevent buffer stripe index overflow #2331 — Security Report: Deterministic ArrayIndexOutOfBoundsException via Math.abs(Integer.MIN_VALUE) in prometheus/client_java Buffer Thread Striping #2282: buffer stripe-index overflow
fix: hide HTTP scrape error details #2332 — Security Report: Full Stack Trace Disclosure in HTTP Error Responses #2283: HTTP scrape error disclosure
fix: bound HTTPServer request resources #2333 — # Security Report: Thread Pool Exhaustion via Slow Clients (Slowloris-style DoS) in prometheus/client_java HTTPServer #2284: HTTPServer resource bounds
fix: bound scrape query parameters #2334 — # Security Report: Denial of Service via Unbounded name[] Query Parameter Flooding in prometheus/client_java #2285: scrape query parameter bounds
fix: redact invalid configuration values #2335 — # Security Report: Sensitive Configuration Values Leaked via PrometheusPropertiesException to HTTP Clients #2286: configuration-value redaction
fix: bound observation buffering during collection #2336 — # Security Report: Unbounded Observation Buffer Growth During Stalled Scrape in prometheus/client_java #2287: observation-buffer bounds (stacked on fix: prevent buffer stripe index overflow #2331)

Each PR has an Ongoing discussion section that records the remaining decisions from this PR's review history.

## Summary - bound default query parsing by query length and parameter count - return HTTP 400 for malformed or excessive query parameters - deduplicate exact metric-name filters before lookup This is the focused replacement for the #2285 portion of #2297. Fixes #2285 ## Ongoing discussion None currently. The malformed percent-encoding case raised during the earlier review is covered and returns HTTP 400. ## Validation - `mise run lint:fix` - `mise run build` - `./mvnw test -pl prometheus-metrics-exporter-common,prometheus-metrics-model -Dcoverage.skip=true -Dcheckstyle.skip=true` --------- Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>

## Summary - return a generic HTTP 500 body instead of exposing exception details - include a safe hint explaining how to enable better diagnostics - avoid adding server-side logging by default - let users explicitly opt into either detailed HTTP responses or server-side error reporting - verify the secure default does not expose the exception type or message This is the focused replacement for the #2283 portion of #2297. Fixes #2283 ## Implementation plan 1. Make `HttpErrorHandlingPolicy` builder-based and pass the built policy through the `HTTPServer` builder to the exchange adapter. Response verbosity and error reporting remain orthogonal choices. 2. Make the default policy return a generic HTTP 500 response with a short hint to configure server-side error reporting for diagnostic details. The default must not add a new log entry for the scrape exception. 3. Let callers configure either axis independently: - attach a caller-supplied error reporter while keeping the generic response, so applications and Java agents can route diagnostics to an appropriate sink; and - enable an explicitly unsafe debug response mode that includes exception details in the HTTP body. Avoid “legacy” naming; the API and docs must make the disclosure risk clear. 4. Preserve the existing logging behavior for failures where the error response itself cannot be sent or response headers were already committed, because no useful client response remains possible in those paths. 5. Add focused tests for the secure default, diagnostic hint, independent verbosity/reporter configuration, reporter invocation and isolation, reporter failure handling, and the explicitly unsafe debug-response mode. Add user documentation for the available policies and their security tradeoffs, plus the repository’s release-please changelog entry linking to that documentation. ## Alternatives considered - **Unconditional server-side logging:** rejected because it replaces the response disclosure with a new operational regression: one stack trace per failed scrape, with possible log-ingestion cost and application-logging side effects for unshaded integrations. - **Keep the detailed response as the default:** rejected because it does not remediate #2283 unless every user discovers and enables the secure mode. - **Generic response with no diagnostic guidance:** rejected because it leaves operators with a silent, unexplained HTTP 500 and no discoverable path to better diagnostics. - **Hard-code rate limiting or deduplication in the adapter:** deferred in favor of a reporter abstraction. Correct suppression requires bounded state, concurrency handling, distinct-failure classification, and suppressed-count reporting; callers or a later reusable reporter can implement that policy without coupling it to HTTP response handling. - **Ambient debug/verbosity configuration:** rejected in favor of an explicit builder option on `HttpErrorHandlingPolicy`, so re-enabling unsafe debug responses is deliberate and carries a clear security warning. - **Network controls or authentication documentation alone:** rejected as the primary fix. They remain useful defense in depth but should not substitute for a safe default response. ## Validation - `mise run lint:fix` - `mise run build` - `./mvnw test -pl prometheus-metrics-exporter-httpserver -Dcoverage.skip=true -Dcheckstyle.skip=true` ## Release note Release Please will use this override for the generated changelog and GitHub release notes after a squash merge: BEGIN_COMMIT_OVERRIDE fix(httpserver): make scrape error responses secure and configurable Scrape failures now return a generic HTTP 500 response by default. Applications can configure a server-side error reporter or explicitly enable an unsafe debug response containing exception details. See the [HTTPServer scrape error handling documentation](https://github.com/prometheus/client_java/blob/main/docs/content/exporters/httpserver.md#scrape-error-handling). END_COMMIT_OVERRIDE --------- Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>

## Summary - use a fixed-size default executor with a bounded queue and non-blocking rejection - close rejected authenticated request bodies instead of draining an unbounded body - close rejected exchanges after sending HTTP 403 This is the focused replacement for the #2284 portion of #2297. Fixes #2284 ## Ongoing discussion None currently. Earlier review feedback about preserving default concurrency and closing rejected exchanges is incorporated here. ## Validation - `mise run lint:fix` - `mise run build` - `./mvnw test -pl prometheus-metrics-exporter-httpserver -Dcoverage.skip=true -Dcheckstyle.skip=true` --------- Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>

github-actions Bot added the api-change label Jul 9, 2026

zeitlinger force-pushed the codex/fix-sec-report-bugs branch from 6cc0f6c to 9a7bf86 Compare July 9, 2026 14:03

fix: harden scrape edge cases

0020301

Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>

zeitlinger force-pushed the codex/fix-sec-report-bugs branch from 9a7bf86 to 0020301 Compare July 10, 2026 17:08

zeitlinger marked this pull request as ready for review July 10, 2026 17:24

zeitlinger requested review from dhoard, fstab and jaydeluca as code owners July 10, 2026 17:24

Copilot started work on behalf of zeitlinger July 10, 2026 17:27 View session

fix: update stale prometheus-metrics-exporter-common API diff

92115df

Copilot finished work on behalf of zeitlinger July 10, 2026 17:39

Merge branch 'main' into codex/fix-sec-report-bugs

88d2099

jaydeluca requested a review from Copilot July 13, 2026 12:53

Copilot started reviewing on behalf of jaydeluca July 13, 2026 12:53 View session

Copilot AI reviewed Jul 13, 2026

View reviewed changes

fix: address scrape hardening review

49b71f4

Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>

jaydeluca reviewed Jul 17, 2026

View reviewed changes

zeitlinger added 4 commits July 23, 2026 08:37

fix: harden scrape buffering and redact config errors

30e3a23

Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>

fix: satisfy scrape buffer style checks

ec7f83d

Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>

fix: preserve observations across buffer generations

1a9a50d

Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>

docs: refresh histogram API diff

b45478d

Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>

zeitlinger closed this Jul 23, 2026

Uh oh!

Conversation

zeitlinger commented Jul 9, 2026

Summary

Validation

Uh oh!

github-actions Bot commented Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ API changes detected — maintainer review required

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jaydeluca left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeitlinger commented Jul 23, 2026

Uh oh!

zeitlinger commented Jul 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

github-actions Bot commented Jul 9, 2026 •

edited

Loading