feat(instrumentation-http): add ignoreIncomingPropagationHook config - #6901
feat(instrumentation-http): add ignoreIncomingPropagationHook config#6901matthieusieben wants to merge 1 commit into
Conversation
|
|
|
Welcome, contributor! Thank you for your contribution to opentelemetry-js. Important reminders:
|
a7dcaf4 to
5f4cba5
Compare
…option Signed-off-by: Matthieu Sieben <matthieu.sieben@gmail.com> Assisted-By: Claude Fable 5 (GitHub Copilot)
5f4cba5 to
26565f5
Compare
|
Hi @matthieusieben - thanks for opening this PR for discussion. This is actually an issue that's mentioned in our docs: https://opentelemetry.io/docs/concepts/context-propagation/#security-best-practices Q: why not strip these headers at at something like a reverse proxy so that these never end up in the app at all? I suppose the idea is to don't allow external callers to add trace context/baggage but to allow internal ones. This is the most common way I've seen this issue handled in production deployments. |
|
That sounds like a possible solution but in our deployment (in which we do have a reverse proxy), we allow calls from both internal and public systems. To make matters worse, some internal systems are deployed on dynamically provisioned systems (IPs). Since we do receive credentials from those systems (which are verified at the application level, not in the proxy), it can be useful to be able to fine tune filtering at the application level, based on those credentials and the same verification logic. Also (and this is not the case for us), NodeJS can be run without a reverse proxy, so having the knob here seems like a decent solution for those instances. |
|
For the sake of discussion, I've had Claude create another option that would allow me to achieve the same thing using a custom Propagator: |
Pull request dashboard statusWaiting on reviewers · refreshed 2026-07-29 01:44 UTC Review the latest changes. Status above doesn't look right?
|
Which problem is this PR solving?
instrumentation-httpunconditionally extracts the propagation context (traceparent,tracestate, baggage, …) from every incoming request's headers. For servers exposed to the public internet, this means any unauthenticated client can choose the trace ID that the server span — and the entire downstream fan-out — is recorded under.Concrete consequences we've observed in production:
traceparentco-locates our server spans under a trace ID it controls. Anyone on the internet can group arbitrary request spans under one trace.traceparent; the root span lives in the client's tracing backend, so our backend shows<root span not yet received>for these traces.max_bytes_per_trace), causing the whole trace — including our own spans — to be silently dropped.There is currently no way to keep tracing incoming requests while not honoring their propagation headers.
OTEL_PROPAGATORS=noneis not an option since it also disables outbound injection, breaking internal trace-joining, andignoreIncomingRequestHookdisables tracing of the request entirely.Short description of the changes
Adds an
ignoreIncomingPropagationHookconfig option toHttpInstrumentationConfig:http.IncomingMessagebefore span creation. When it returnstrue,ROOT_CONTEXTis used instead ofpropagation.extract(ROOT_CONTEXT, headers)— the request is still traced normally, but the server span starts a new trace with no remote parent.safeExecuteInTheMiddle, matchingignoreIncomingRequestHook) and extraction proceeds normally — fail-open to today's behavior.Includes unit tests (hook true / false / throwing), README docs, and a changelog entry.
Alternative considered
Rather than an instrumentation-level hook, the propagation API itself could pass the request (carrier + connection metadata) to
TextMapPropagator.extract()instead of only the headers carrier. A propagator could then decide, per request, to return the incomingcontextunchanged (root context) instead of a context linked to the remote parent. That would make the trust boundary expressible once in a custom propagator and reusable across all server instrumentations (http, grpc, fastify, …), instead of requiring each instrumentation to grow its own hook. It is, however, a wider API change (carrier semantics ofTextMapGetter), so this PR takes the narrower instrumentation-scoped approach; happy to pivot if maintainers prefer the propagator-level design.This was made by specifically directing Claude what to do.