-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix(node): Make body capturing more robust #16105
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Conversation
size-limit report 📦
|
|
||
if (bodyByteLength < MAX_BODY_BYTE_LENGTH) { | ||
chunks.push(bufferifiedChunk); | ||
bodyByteLength += bufferifiedChunk.length; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we use length
or byteLength
here? 🤔
} | ||
} catch { | ||
// noop |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add a debug logger message here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is still one small comment open to add a debug log on error case, but already approving this, this is great!
Ah thanks, forgot to push that change! |
I have a hunch that not all frameworks call `req.on('end')` but may only do `req.on('close')` or whatever else and this is why we are not reliably capturing bodies. It should be safe to attach a `req.on('end')` handler, as that doesn't change any semantics AFAIK. Fixes #16090 --------- Co-authored-by: Francesco Gringl-Novy <francesco.novy@sentry.io>
I have a hunch that not all frameworks call
req.on('end')
but may only doreq.on('close')
or whatever else and this is why we are not reliably capturing bodies.It should be safe to attach a
req.on('end')
handler, as that doesn't change any semantics AFAIK.Fixes #16090