Skip to content

Conversation

@pichlermarc
Copy link
Member

@pichlermarc pichlermarc commented Mar 12, 2024

Which problem is this PR solving?

When passing an options object like { headers: { foo: undefined } } to the http/json and http/protobuf exporters, the headers will not be validated and that will cause

req = request(options, (res: http.IncomingMessage) => {

to throw, as we're passing an undefined header value. While this alone will not cause an application crash, trying to access req at

will cause the end-user's application to crash within typically 10s after the first export (when using defaults).

This PR fixes this by sanitizing these headers first using parseHeaders().

Fixes #4541

Type of change

  • Bug fix (non-breaking change which fixes an issue)

@pichlermarc pichlermarc added bug Something isn't working priority:p1 Bugs which cause problems in end-user applications such as crashes, data inconsistencies, etc pkg:exporter-trace-otlp-http pkg:exporter-trace-otlp-proto pkg:exporter-metrics-otlp-http pkg:exporter-metrics-otlp-proto labels Mar 12, 2024
@pichlermarc pichlermarc force-pushed the fix/exporter-headers branch from 60f7ebd to 02dc846 Compare March 12, 2024 13:03
@codecov
Copy link

codecov bot commented Mar 12, 2024

Codecov Report

Merging #4540 (418b9e3) into main (1b4999f) will decrease coverage by 0.13%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4540      +/-   ##
==========================================
- Coverage   92.83%   92.71%   -0.13%     
==========================================
  Files         328      327       -1     
  Lines        9481     9455      -26     
  Branches     2031     2031              
==========================================
- Hits         8802     8766      -36     
- Misses        679      689      +10     
Files Coverage Δ
...ogs-otlp-http/src/platform/node/OTLPLogExporter.ts 100.00% <100.00%> (ø)
...gs-otlp-proto/src/platform/node/OTLPLogExporter.ts 100.00% <100.00%> (ø)
...e-otlp-http/src/platform/node/OTLPTraceExporter.ts 100.00% <100.00%> (ø)
...-otlp-proto/src/platform/node/OTLPTraceExporter.ts 100.00% <100.00%> (ø)
...porter-metrics-otlp-grpc/src/OTLPMetricExporter.ts 100.00% <100.00%> (ø)
...-otlp-http/src/platform/node/OTLPMetricExporter.ts 100.00% <100.00%> (ø)
...orter-metrics-otlp-proto/src/OTLPMetricExporter.ts 100.00% <100.00%> (ø)
...perimental/packages/otlp-exporter-base/src/util.ts 93.44% <100.00%> (ø)

... and 2 files with indirect coverage changes

@pichlermarc pichlermarc force-pushed the fix/exporter-headers branch from 11d923d to 85d3d4c Compare March 12, 2024 14:05
@pichlermarc pichlermarc marked this pull request as ready for review March 13, 2024 13:14
@pichlermarc pichlermarc requested a review from a team March 13, 2024 13:14
@pichlermarc pichlermarc merged commit 3a426e8 into open-telemetry:main Mar 14, 2024
@pichlermarc pichlermarc deleted the fix/exporter-headers branch March 14, 2024 14:21
Zirak pushed a commit to Zirak/opentelemetry-js that referenced this pull request Sep 14, 2024
…ndefined' (open-telemetry#4540)

* fix(exporters): use parseHeaders() to ensure header-values are not 'undefined'

* chore: changelog

* fixup! fix(exporters): use parseHeaders() to ensure header-values are not 'undefined'
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working pkg:exporter-metrics-otlp-http pkg:exporter-metrics-otlp-proto pkg:exporter-trace-otlp-http pkg:exporter-trace-otlp-proto priority:p1 Bugs which cause problems in end-user applications such as crashes, data inconsistencies, etc

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[otlp-exporters] passing an undefined header values crashes end-user apps

2 participants