getsentry / sentry-javascript

Official Sentry SDKs for JavaScript
https://sentry.io
MIT License
7.87k stars 1.55k forks source link

`request` library fails with Sentry enabled on Node 20 #8552

Closed simllll closed 1 year ago

simllll commented 1 year ago

Is there an existing issue for this?

How do you use Sentry?

Self-hosted/on-premise

Which SDK are you using?

@sentry/node

SDK Version

7.57.0

Framework Version

No response

Link to Sentry event

No response

SDK Setup

Sentry.init({
    dsn: 'https://0068cfe558e44d329481bc767d6b4e41@sentry.HOSTNAME/12'
});

Steps to Reproduce

node20 + arm64 + request library + sentry

script:

import * as Sentry from '@sentry/node';
import * as request from 'requestretry';

// comment this, and it works, otherwise oyu get error 403
Sentry.init({
        dsn: 'https://0068cfe558e44d329481bc767d6b4e41@test.com/0'
});

const test = async () => {
const result = await request.get({
                                url: 'https://files.test.hokify.com/undefined565c67d6e855735aa2eab974_1510057848572.jpg'
                        });
console.log('RESULT', result);
};

test();

run with ts-node test.ts ~

body returns body: '<?xml version="1.0" encoding="UTF-8"?>\n' + 'AccessDeniedAccess Denied2W0387H91RPD35YJupEwGHmpd6/Em0HdUtLK6jW0tLKNnKKRdO2o/w1pEFWPsSLGxAeym4tVYhZVCO92PmM8bgk9jQQ=', attempts: 1,

but it should actually return the image (buffer) this behaviour is ONLY happening on arm64, not on amd64. it also only happens with node20, node18 works fine too.

Expected Result

the url is public available, and is retrievable like it is without sentry.init()

Actual Result

it throws a 403 error, somehow senty modifies the request

I assume this is a node bug, as it only happen son arm64, but need help to narrow this down what can cause this.

simllll commented 1 year ago

related https://github.com/request/request/issues/3458

simllll commented 1 year ago

This is a quite serious issue, especially since it is happening in a very unexpected way. I can imagine that more people will run into this, and maybe even do not recognise it in the beginning. E.g. our test suite didn't detect any issues as there is no sentry initialised at this time. Anything we can do to support getting this resolved?

winnyschuster commented 1 year ago

i think we are having the very same issue at open source project iobroker. I have to contradict that it is only happening on arm64 systems, i use amd64(debian-bullseye, node 20.5.0, latest sentry 7.60.0) We found only issues where the request() lib is used but i think it has to do with exchange of the node:http module to the sentry-http module once sentry is initialized, so could probably happen elsewhere.

i have nailed it down to sentry somehow cutting the Request URI from (in your example) /undefined565c67d6e855735aa2eab974_1510057848572.jpg to just /. So, your 403 comes from the fact that host https://files.test.hokify.com/ is just not accessible.

i have changed protocol to http in your request and monitored this with wireshark where you can truly see whats going on at the end. see attached pic with 1st line has sentry enabled and 2nd disabled

Hope this helps the team over here to find the issue. Of course i'm willing to do further testing if asked for

SentryWireshark

winnyschuster commented 1 year ago

I assume this is a node bug, as it only happen son arm64, but need help to narrow this down what can cause this.

not an expert but i wonder how could that be node when plain node just does the job and sentry does not in this case? I've tried to debug things but failed except that i could see that node:http is not even touched during the request. And as stated before, it is also happening on amd64 systems

lforst commented 1 year ago

Hey, internally we are a bit stomped as to why this might be happening. We don't have any functionality specific to cpu architecture. However, we are definitely modifying requests by monkeypatching the http module. Generally, we are only modifying the outgoing headers of requests (for distributed tracing) though and not any request paths.

Can we rule out that this is not a bug in the requestretry package?

Apollon77 commented 1 year ago

We also have big issues with this in ioBroker ... I will link the relevant issues here too. request library is still very commonly used in older projects :-(

Apollon77 commented 1 year ago

When you read the nodejs 19/20 changelogs there is some stuff with "headers" in it but also nothing here that should produce such an issue ... maybe your header manipulations somehow interfere here?

mydea commented 1 year ago

Cross linking this issue: https://github.com/getsentry/sentry-javascript/issues/8549 as this may be related

Apollon77 commented 1 year ago

Also: @winnyschuster was able to also repro this on x86_64, so it do not seems to be arm64 based ...

winnyschuster commented 1 year ago

first of all, thanks for the quick reply! Our number of issues is increasing the more users are getting used to node20. though there is an approch to replace all usage of request() with e.g axios(), a lot of third party libraries we depend on are not the newest ones and as such use request().

.... and not any request paths.

to me it looks like, see the wireshark pics above

Can we rule out that this is not a bug in the requestretry package?

i would, though pretty new to all that http protocol stuff. For one, functionality is given at 100% with the plain request()/requestretry() packages. 2nd, when debugging that little script, at this point in request https://github.com/request/request/blob/3c0cddc7c8eb60b470e9519da85896ed7ee0081e/request.js#L751 self.req = self.httpModule.request(reqOptions) the next step in debugging brings me to Line https://github.com/getsentry/sentry-javascript/blob/be788f00e048d692b17df9ff6948e303c2722a81/packages/node/src/integrations/http.ts#L233 (hope this is the right source, looks like, in runtime it is line 174 of node_modules/@sentry/node/cjs/integrations/http.js)

all args that make sense to me are still there, host, port, href, path, method and look as expected. pathName is missing at this point but seem to rember that this is the case in node:http module too

unfortunately i get lost and overwhelmed somewhere around the mentioned lines in code and thought you guys can do this better and faster :-) However, let me know what additional information you may need and i'll try to provide it, only give some hints where and what to look for. If you could give me a hint at which point in code all that stuff goes to the wire, that would probably help a lot

pkerschbaum commented 1 year ago

Cross linking https://github.com/nodejs/node/issues/48921

simllll commented 1 year ago

Cross linking nodejs/node#48921

Great finding, seems to be the root cause. Interesting though that it's still only happening with sentry enabled.

Sonyque commented 1 year ago

G'Day, we experience the same issue with

lforst commented 1 year ago

Seems like this is a bug in Node.js being tracked here: https://github.com/nodejs/node/issues/48921

They merged a fix here: https://github.com/nodejs/node/pull/48928

Seems like it's a matter of time until this is released in the newest Node.js version.

Props to @Zhangdroid for the genius-level debugging and fixing! <3

Apollon77 commented 1 year ago

Did someone verified that this nodejs fix also fixes the issue?

lforst commented 1 year ago

@Apollon77 I'd assume you would have to apply the node patch. The version that should include this isn't out yet.

winnyschuster commented 1 year ago

Did someone verified that this nodejs fix also fixes the issue?

i just did with node 20.6.0 and that solved the issue

lforst commented 1 year ago

@winnyschuster Thanks for confirming! Closing this then :)

Apollon77 commented 1 year ago

Thank you all!