fix(otlp-exporter-base): limit Node.js HTTP transport response body to 4 MB#6552
fix(otlp-exporter-base): limit Node.js HTTP transport response body to 4 MB#6552trentm merged 5 commits intoopen-telemetry:mainfrom
Conversation
trentm
left a comment
There was a problem hiding this comment.
Thanks for picking this up!
| // a no-op. res.on('error') does not fire because destroy() is called | ||
| // without an error argument. | ||
| if (res.statusCode && res.statusCode <= 299) { | ||
| resolve({ status: 'success' }); |
There was a problem hiding this comment.
If the limit is exceeded, the client MUST treat the response as a not-retryable error.
Why have a resolve({ status: 'success' }); code path here?
There was a problem hiding this comment.
Good point... I initially kept the 2xx path as success thinking the export was already accepted, but re-reading it, I think you're right. I will update to return failure for all oversized responses regardless of HTTP status.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6552 +/- ##
=======================================
Coverage 95.73% 95.74%
=======================================
Files 369 371 +2
Lines 12497 12523 +26
Branches 2959 2963 +4
=======================================
+ Hits 11964 11990 +26
Misses 533 533
🚀 New features to boost your workflow:
|
|
Should we hold this until the proto PR merges or no? Looks like not everyone did |
|
@dyladan I am fine with merging this. Since it's an internal change to the exporter, we can change the behavior again if the spec changes. |
|
Thanks! |
|
Thank you for your contribution @kartikgola! 🎉 We would like to hear from you about your experience contributing to OpenTelemetry by taking a few minutes to fill out this survey. |
Which problem is this PR solving?
Node.js HTTP transport in
otlp-exporter-baseaccumulates response body chunks into aBuffer[]without any size limit. A misconfigured/misbehaving server could send an arbitrarily large response, causing the exporter to buffer it all in memory.This implements the 4 MB response body limit also implemented by this opentelemetry-proto PR based on https://cwe.mitre.org/data/definitions/789.html.
Fixes #6539
Short description of the changes
In sendWithHttp(), the res.on('data') handler now tracks cumulative response size. If it exceeds 4MB (
MAX_RESPONSE_BODY_SIZE), the stream is destroyed and the promise is resolved based on the HTTP status code:The resolve() is called before res.destroy() so the subsequent ECONNRESET on req.on('error') becomes a no-op (since promise is already settled).
Note: The fetch transport (
otlp-exporter-base/src/transport/fetch-transport.ts) used by browser exporter has not been changed because it never reads the response body so the vulnerability doesn't apply there. If required, we could have added something likeresponse.body?.cancel().catch(() => {});but it has no impact since it never reads the body.Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Three new unit tests added in http-exporter-transport.test.ts. Each spins up a real http.Server that writes oversized responses:
All 146 existing node.js tests also pass.
Checklist: