Skip to content

fix: hide HTTP scrape error details - #2332

Open
zeitlinger wants to merge 4 commits into
mainfrom
agent/hide-http-error-details
Open

fix: hide HTTP scrape error details#2332
zeitlinger wants to merge 4 commits into
mainfrom
agent/hide-http-error-details

Conversation

@zeitlinger

@zeitlinger zeitlinger commented Jul 23, 2026

Copy link
Copy Markdown
Member

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 Security Report: Full Stack Trace Disclosure in HTTP Error Responses #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.
END_COMMIT_OVERRIDE

Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
@zeitlinger
zeitlinger marked this pull request as ready for review July 23, 2026 12:28
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
@github-actions

Copy link
Copy Markdown
Contributor

⚠️ API changes detected — maintainer review required

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

  • 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>
"The Prometheus metrics HTTPServer caught an Exception while trying to send "
+ "the metrics response.",
requestHandlerException);
logger.log(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This branch never calls errorHandlingPolicy.report(...) — it logs at SEVERE unconditionally
instead. Two things fall out of that:

The policy leaks. An application that wired errorReporter into its telemetry pipeline
silently misses this entire class of failure. It gets no callback, and the exception lands in
java.util.logging — exactly the destination the policy exists to avoid.

The Java-agent scenario this PR is built for is unfixed here. A collector that throws
deterministically during writer.write(...) reaches handleException with responseSent == true on every single scrape. That's a SEVERE + full stack trace into the host application's
logs every 15s, with no way to turn it off — which is the original complaint, on a different
code path.

Suggested shape (needs a package-private boolean hasErrorReporter() on the policy):

} else {
  // If the exception occurs after response headers have been sent, it's too late to respond
  // with HTTP 500.
  if (errorHandlingPolicy.hasErrorReporter()) {
    reportException(requestHandlerException);
  } else {
    logger.log(
        Level.SEVERE,
        "The Prometheus metrics HTTPServer caught an Exception while trying to send "
            + "the metrics response.",
        requestHandlerException);
  }
}

Always-report-and-log-only-when-unconfigured works too — the requirement is just that a
configured reporter sees it.

There's no test covering this branch today, which is how it slipped through. A test that calls
sendHeadersAndGetBody(200, 0) and then handleException(...) would pin it.

this(new PrometheusScrapeHandler(config, registry), HttpErrorHandlingPolicy.builder().build());
}

MetricsHandler(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This overload is package-private, so only HTTPServer.Builder can reach it — but
MetricsHandler is @StableApi with four public constructors, and its whole reason to exist as
a public HttpHandler is registering it on a user-owned com.sun.net.httpserver.HttpServer
without going through HTTPServer.

For those users this PR is a strict regression rather than a configurable tradeoff: the old
behavior (stack trace to the client) is gone and the new behavior (reporter) is unreachable.
They get a bare 500 telling them to "Configure an HTTP error reporter for details" with no API
to do so.

Same applies to HttpExchangeAdapter's package-private constructor
(HttpExchangeAdapter.java:30), which is the documented seam for wiring
PrometheusScrapeHandler into any HttpExchange-based server.

Both are purely additive and would show up cleanly in the api-diff:

public MetricsHandler(
    PrometheusProperties config,
    PrometheusRegistry registry,
    HttpErrorHandlingPolicy errorHandlingPolicy) { ... }

public HttpExchangeAdapter(HttpExchange httpExchange, HttpErrorHandlingPolicy policy) { ... }

Comment on lines +31 to +33
By default, scrape failures return a generic HTTP 500 response. Exception details are not
included in the response or logged, because the server may run inside an application or a
Java agent with its own diagnostic pipeline.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

"or logged" isn't accurate — the default policy does log, at SEVERE, with a full throwable, in
two paths: HttpExchangeAdapter:120-128 (couldn't write the 500) and :133-137 (headers already
sent). If we don't take the change in my other comment, the second one is routine rather than
exceptional, which makes this sentence actively misleading for the agent use case it's written
for.

Suggested change
By default, scrape failures return a generic HTTP 500 response. Exception details are not
included in the response or logged, because the server may run inside an application or a
Java agent with its own diagnostic pipeline.
By default, scrape failures return a generic HTTP 500 response. Exception details are not
included in the response, and are not logged when the error response can be delivered, because
the server may run inside an application or a Java agent with its own diagnostic pipeline.

(If the response itself can't be delivered, logging is the only remaining signal — worth saying
so explicitly rather than leaving it as a surprise.)

* must be safe to call concurrently. Runtime exceptions thrown by the reporter are isolated
* from HTTP response handling.
*/
public Builder errorReporter(Consumer<Throwable> errorReporter) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Only Exception is ever delivered here — handleException has exactly two overloads
(IOException, RuntimeException) and PrometheusScrapeHandler:99-103 catches exactly those
two. A bare Throwable can't reach this consumer.

Consumer<? super Exception> would be both more honest about that contract and strictly more
permissive: it accepts Consumer<Exception>, Consumer<Throwable>, and Consumer<Object>,
where today someone holding a Consumer<Exception> needs a cast or a wrapper lambda.

Raising it now rather than as a nit because @StableApi on line 17 freezes this signature at
merge. Existing-style callers keep compiling (contravariance covers
Consumer<Throwable> r = ...; policy.errorReporter(r), and erasure is identical), so it's
cheap now and impossible later.

Related, same "frozen at merge" reasoning: a named @FunctionalInterface ScrapeErrorReporter
instead of Consumer would leave room to add a second parameter later (the failing request, a
scrape context). A Consumer can never grow one. Not saying we need that but a thought.


private static final byte[] GENERIC_RESPONSE =
("An internal error occurred while scraping metrics. "
+ "Configure an HTTP error reporter for details.\n")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The out-of-the-box experience for a plain HTTPServer.builder().buildAndStart() in a standalone
app is now: scrape fails → bare 500 → nothing anywhere. This message tells the operator to
configure a reporter, which requires a code change and a redeploy to act on.

The default itself is defensible — agent safety is the harder constraint and it should win. But
the opt-in ergonomics mean everyone writes the same five-line lambda. Worth shipping it:

/** Reports scrape exceptions to {@code java.util.logging} at {@code SEVERE}. */
public static Consumer<Throwable> julReporter() { ... }

so the override is .errorReporter(HttpErrorHandlingPolicy.julReporter()). Makes "we
deliberately don't do this for you" a one-liner to reverse instead of a research task.

private void sendErrorResponse(Exception requestHandlerException) {
if (!responseSent) {
responseSent = true;
reportException(requestHandlerException);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The reporter runs before the 500 is written. The javadoc says it "should return promptly," but a
reporter that blocks — synchronous telemetry export, saturated appender — delays the client's
500, and one that hangs prevents it entirely. Moving this call below the try/catch guarantees
the response goes out first and costs nothing.

If you do move it, the "Original Exception that caused the Prometheus scrape error" log at
:125-128 becomes redundant whenever a reporter is configured — it has already seen the
exception by then. Logging only errorWriterException there would keep that path quiet for
agent users.

Sign up for free to 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.

Security Report: Full Stack Trace Disclosure in HTTP Error Responses

2 participants