fix(opentelemetry_cowboy): prevent telemetry handler crash on invalid HTTP/2 scheme #568
+28
−12
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This to fix the following compilation error on test:
```
$ rebar3 ct
===> Verifying dependencies...
===> Analyzing applications...
===> Compiling opentelemetry
===> Compiling _build/test/lib/opentelemetry/src/otel_span_utils.erl failed
┌─ _build/test/lib/opentelemetry/src/otel_span_utils.erl:
│
92 │ {ParentSpanCtx#span_ctx{span_id=SpanId, hex_span_id=HexSpanId}, ParentSpanId, ParentIsRemote}
│ ╰── field hex_span_id undefined in record span_ctx
┌─ _build/test/lib/opentelemetry/src/otel_span_utils.erl:
│
101 │ hex_trace_id=HexTraceId,
│ ╰── field hex_trace_id undefined in record span_ctx
┌─ _build/test/lib/opentelemetry/src/otel_span_utils.erl:
│
103 │ hex_span_id=HexSpanId,
│ ╰── field hex_span_id undefined in record span_ctx
┌─ _build/test/lib/opentelemetry/src/otel_span_utils.erl:
│
91 │ HexSpanId = otel_utils:encode_hex(<<SpanId:64>>),
│ ╰── Warning: variable 'HexSpanId' is unused
┌─ _build/test/lib/opentelemetry/src/otel_span_utils.erl:
│
97 │ HexTraceId = otel_utils:encode_hex(<<TraceId:128>>),
│ ╰── Warning: variable 'HexTraceId' is unused
┌─ _build/test/lib/opentelemetry/src/otel_span_utils.erl:
│
99 │ HexSpanId = otel_utils:encode_hex(<<SpanId:64>>),
│ ╰── Warning: variable 'HexSpanId' is unused
```
this is necessary as there are new fields got introduced in the the open-telemetry/opentelemetry-erlang#874
… HTTP/2 scheme Cowboy does not validate the `:scheme` pseudo-header per RFC 9113, which states that scheme "is not restricted to http and https" and can be used for non-HTTP schemes via proxies/gateways. This means Cowboy passes through any value from the HTTP/2 HEADERS frame without validation. When a malformed or unexpected scheme value reaches `extract_scheme/2`, the case clause crashes because it only handles `<<"http">>` and `<<"https">>`. This crash causes Erlang's telemetry library to permanently detach the handler for the affected node, resulting in all subsequent requests losing OpenTelemetry tracing until the node is restarted. In production, this manifests as "almost all" requests suddenly missing trace IDs, with the root cause being a single malformed HTTP/2 request that crashed the telemetry handler. The fix introduces a configurable `valid_schemes` option that maps scheme binaries to atoms. Unknown schemes now return `undefined` instead of crashing, which is consistent with how `otel_http:extract_scheme/2` already handles unknown schemes from headers. The default configuration preserves backwards compatibility by only accepting `http` and `https`. Users who need to support additional schemes (e.g., `ws`, `wss`) can extend the map via configuration. Reproduction repository: https://github.com/velimir/scheme_crash Fixes: open-telemetry#567
There was a problem hiding this comment.
Pull request overview
This PR fixes a crash in the OpenTelemetry Cowboy instrumentation when receiving malformed HTTP/2 scheme values. The crash would cause the telemetry handler to permanently detach, resulting in missing trace IDs for all subsequent requests until node restart.
Key Changes:
- Introduced configurable
valid_schemesoption to map scheme binaries to atoms - Refactored
extract_scheme/2to returnundefinedfor unknown schemes instead of crashing - Added test case to verify invalid schemes return
undefined
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
instrumentation/opentelemetry_cowboy/src/opentelemetry_cowboy.erl |
Added valid_schemes configuration option and refactored scheme extraction logic to handle invalid schemes gracefully |
instrumentation/opentelemetry_cowboy/test/opentelemetry_cowboy_SUITE.erl |
Added test case invalid_scheme_returns_undefined to verify behavior with invalid schemes |
instrumentation/opentelemetry_cowboy/rebar.lock |
Updated opentelemetry_api dependency from 1.4.0 to 1.5.0 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+512
to
+527
| invalid_scheme_returns_undefined(_Config) -> | ||
| Opts = #{ | ||
| valid_schemes => #{<<"https">> => https} | ||
| }, | ||
| opentelemetry_cowboy:setup(Opts), | ||
| Port = 62662, | ||
| {ok, {{_Version, 200, _ReasonPhrase}, _Headers, _Body}} = | ||
| httpc:request(get, {"http://localhost:8080/success", []}, [], [{socket_opts, [{port, Port}]}]), | ||
| receive | ||
| {span, #span{name=Name,attributes=Attributes,kind=Kind}} -> | ||
| ?assertEqual('GET', Name), | ||
| ?assertEqual(?SPAN_KIND_SERVER, Kind), | ||
| ?assertEqual(undefined, maps:get(?URL_SCHEME, otel_attributes:map(Attributes))) | ||
| after | ||
| 1000 -> ct:fail(invalid_scheme_returns_undefined) | ||
| end. |
There was a problem hiding this comment.
The test configures valid_schemes to only accept https, then makes an http:// request. While this correctly tests the scenario of an invalid scheme returning undefined, the test doesn't verify that the handler doesn't crash, which was the original bug being fixed. Consider adding a second request after the first to ensure the telemetry handler is still attached and functioning after encountering an invalid scheme.
Author
There was a problem hiding this comment.
I would love to, but making an http2 request via httpc is.. problematic.
Member
|
Why was this closed? |
Author
GitHub automation in a private fork :/ it should've not been closed |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Cowboy does not validate the
:schemepseudo-header per RFC 9113, whichstates that scheme "is not restricted to http and https" and can be used
for non-HTTP schemes via proxies/gateways. This means Cowboy passes through
any value from the HTTP/2 HEADERS frame without validation.
When a malformed or unexpected scheme value reaches
extract_scheme/2,the case clause crashes because it only handles
<<"http">>and<<"https">>. This crash causes Erlang's telemetry library to permanentlydetach the handler for the affected node, resulting in all subsequent
requests losing OpenTelemetry tracing until the node is restarted.
In production, this manifests as "almost all" requests suddenly missing
trace IDs, with the root cause being a single malformed HTTP/2 request
that crashed the telemetry handler.
The fix introduces a configurable
valid_schemesoption that maps schemebinaries to atoms. Unknown schemes now return
undefinedinstead ofcrashing, which is consistent with how
otel_http:extract_scheme/2alreadyhandles unknown schemes from headers.
The default configuration preserves backwards compatibility by only
accepting
httpandhttps. Users who need to support additional schemes(e.g.,
ws,wss) can extend the map via configuration.Fixes: #567