Observe and transport rejections label requests from their headers - #236
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| // Headers that carry credentials. A rejection observer is a logging and | ||
| // metrics hook, so it never sees them. | ||
| bool IsCredentialHeader(std::string_view name) { | ||
| static constexpr std::array<std::string_view, 3> kCredentials = {"authorization", |
There was a problem hiding this comment.
This denylist does not justify the guarantee above that a logging hook “never sees” credential headers. For example, x-api-key, x-auth-token, cloud-provider signature headers, and application-specific session headers all pass through and can then be logged from RejectedRequest::headers. Please either expose an explicit allowlist/configured projection for rejection labels, or document the returned headers as sensitive and narrow/remove the credential-free guarantee; an incomplete security denylist is likely to make consumers log secrets they believe were filtered.
There was a problem hiding this comment.
Agreed; fixed in 4f1b66c with the allowlist shape. The denylist and RejectedRequest::headers are gone. Options::label_rejection (the same Headers → labels function a service hands Observe) projects the headers inside the transport, and only its result reaches on_rejected as RejectedRequest::labels. Tests: the 413 case sends an authorization header and sees only the labeler's output; the 431 case pins that a throwing labeler costs the labels, not the observation.
Generated by Claude Code
aaylward
changed the title
| // Projects a rejected request's headers onto RejectedRequest::labels, | ||
| // the same function a service hands Observe. Only its result reaches | ||
| // on_rejected; a throwing labeler is logged and labels nothing. | ||
| std::function<Labels(const Headers&)> label_rejection{}; |
There was a problem hiding this comment.
The allowlist projection fixes the credential-exposure issue. One source-compatibility problem remains: Options is a public aggregate, and inserting this member before on_connection_event shifts every later member for positional aggregate initializers. Its default initializer does not preserve those bindings (and existing initializers may now fail on type mismatch). Since the PR claims source compatibility, please append label_rejection at the end of Options instead of inserting it here.
There was a problem hiding this comment.
Fixed in 4d4f206: label_rejection is now the last member of Options, so positional initializers keep their bindings.
Generated by Claude Code
2 participants
What
Closes #235.
Observetakes an optional fifth argument,RequestLabeler(Headers → RequestLabels, a vector of pairs likeMetricLabels). It runs once before dispatch; the result rides onRequestStart::labelsandRequestObservation::labels, including the thrown-handler completion. A throwing labeler is logged and labels nothing.BeastServerTransport::Options::label_rejectiontakes the same function for the transport's own 413/431 rejections.RejectedRequest::labelscarries its output; raw headers (credentials included) never reachon_rejected.Source-compatible: defaulted trailing parameter, new fields default-initialized (
= {}, per #193).Testing
middleware_test: labels on start and completion, on the thrown path, throwing labeler contained.beast_transport_test: a 413 carryingauthorizationyields only the labeler's output; a throwing rejection labeler still reports the 431. Consumer example inexamples/bazel-consumer/access_log_acceptance_test.cc. Each new test failed before its change.bazel test --config=werror //... -- -//benchmarks/...and clang-tidy on the changed sources pass locally.Checklist
bazel test //...passes locally (codegen not touched, gradle not run)