Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

request library fails with Sentry enabled on Node 20 #8552

Closed
3 tasks done
simllll opened this issue Jul 17, 2023 · 19 comments
Closed
3 tasks done

request library fails with Sentry enabled on Node 20 #8552

simllll opened this issue Jul 17, 2023 · 19 comments

Comments

@simllll
Copy link

simllll commented Jul 17, 2023

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: '\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
Copy link
Author

simllll commented Jul 17, 2023

related request/request#3458

@simllll
Copy link
Author

simllll commented Jul 23, 2023

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
Copy link

winnyschuster commented Jul 25, 2023

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
Copy link

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
Copy link
Member

lforst commented Jul 25, 2023

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
Copy link

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 :-(

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 Aug 4, 2023
@lforst
Copy link
Member

lforst commented Aug 4, 2023

Seems like this is a bug in Node.js being tracked here: nodejs/node#48921

They merged a fix here: nodejs/node#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

@getsantry getsantry bot removed the status in GitHub Issues with 👀 Aug 4, 2023
pluris pushed a commit to pluris/node that referenced this issue Aug 6, 2023
Fixes: nodejs#48921
PR-URL: nodejs#48928
Fixes: getsentry/sentry-javascript#8552
Fixes: request/request#3458
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
pluris pushed a commit to pluris/node that referenced this issue Aug 7, 2023
Fixes: nodejs#48921
PR-URL: nodejs#48928
Fixes: getsentry/sentry-javascript#8552
Fixes: request/request#3458
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
@Apollon77
Copy link

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

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 Aug 8, 2023
@lforst
Copy link
Member

lforst commented Aug 8, 2023

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

@getsantry getsantry bot removed the status in GitHub Issues with 👀 Aug 8, 2023
Ceres6 pushed a commit to Ceres6/node that referenced this issue Aug 14, 2023
Fixes: nodejs#48921
PR-URL: nodejs#48928
Fixes: getsentry/sentry-javascript#8552
Fixes: request/request#3458
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this issue Aug 14, 2023
Fixes: nodejs#48921
PR-URL: nodejs#48928
Fixes: getsentry/sentry-javascript#8552
Fixes: request/request#3458
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
UlisesGascon pushed a commit to UlisesGascon/node that referenced this issue Aug 14, 2023
Fixes: nodejs#48921
PR-URL: nodejs#48928
Fixes: getsentry/sentry-javascript#8552
Fixes: request/request#3458
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
RafaelGSS pushed a commit to nodejs/node that referenced this issue Aug 15, 2023
Fixes: #48921
PR-URL: #48928
Fixes: getsentry/sentry-javascript#8552
Fixes: request/request#3458
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
RafaelGSS pushed a commit to RafaelGSS/node that referenced this issue Aug 15, 2023
Fixes: nodejs#48921
PR-URL: nodejs#48928
Fixes: getsentry/sentry-javascript#8552
Fixes: request/request#3458
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
RafaelGSS pushed a commit to nodejs/node that referenced this issue Aug 17, 2023
Fixes: #48921
PR-URL: #48928
Fixes: getsentry/sentry-javascript#8552
Fixes: request/request#3458
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
@winnyschuster
Copy link

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

i just did with node 20.6.0 and that solved the issue

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 Sep 4, 2023
@lforst
Copy link
Member

lforst commented Sep 5, 2023

@winnyschuster Thanks for confirming! Closing this then :)

@Apollon77
Copy link

Thank you all!

aduh95 pushed a commit to aduh95/node that referenced this issue Oct 9, 2023
Fixes: nodejs#48921
PR-URL: nodejs#48928
Fixes: getsentry/sentry-javascript#8552
Fixes: request/request#3458
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
targos pushed a commit to nodejs/node that referenced this issue Nov 27, 2023
Fixes: #48921
PR-URL: #48928
Backport-PR-URL: #50105
Fixes: getsentry/sentry-javascript#8552
Fixes: request/request#3458
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
sercher added a commit to sercher/graaljs that referenced this issue Apr 25, 2024
Fixes: nodejs/node#48921
PR-URL: nodejs/node#48928
Backport-PR-URL: nodejs/node#50105
Fixes: getsentry/sentry-javascript#8552
Fixes: request/request#3458
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
sercher added a commit to sercher/graaljs that referenced this issue Apr 25, 2024
Fixes: nodejs/node#48921
PR-URL: nodejs/node#48928
Backport-PR-URL: nodejs/node#50105
Fixes: getsentry/sentry-javascript#8552
Fixes: request/request#3458
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

9 participants