fix(node): Strip query and fragment from request URLs without route parameters

[richardsimko]

Fixes #6586 by adding a call to stripUrlQueryAndFragment() according to the discussion here #6586 (comment)

[richardsimko]

I'd love to add some tests for this however I couldn't quite find where the relevant test definitions are?

[Lms24]

Hey @richardsimko thanks for opening this PR!

I just remembered that this part of the SDK isn't tested well... we have a few integration tests in the node-integration-tests package to test a few edge cases but nothing all too solid. I think the change is pretty safe to make though, so I don't see it as a requirement for this PR.

However, I'm a bit more concerned if this will fix your issue.

In the issue, you wrote:

Defining a route using app.get('/route') which is then called with /route?param=foo will cause this issue but defining it using app.get('/route/:someParam') and calling /route/foo?param=foo will work.

I quickly tried to verify this in a minimal repro to reproduce the error without your fix first and couldn't get a transaction with query param when running

curl "localhost:3000/route?param=foo"

Would you mind taking a look at the repro and telling me what I'm missing here? I just want to make sure that this is in fact really solving an issue 😅

[Lms24]

Heads-up: I only assigned myself to this PR because I'm reviewing it feel free to make whatever change you want (if necessary) ;)

[richardsimko]

Thanks for the feedback! It seems I was wrong in my initial assessment, the issue occurs when there is at least 2 handlers loaded on the same route. You can reproduce it by adding:

const handler = (req, res, next) => {
  next();
};

app.use("/route", handler);

to the repo. issue.patch

This makes me think that perhaps this issue should be solved somewhere else and that my PR is only solving the symptom.

[Lms24]

@richardsimko

thanks, I can now also reproduce the error. I think we should still strip the query params but in another place. I stepped through the code and I think the place where things are going wrong is the case where we check if req._hasParams evaluates to false in which case we just take the original URL. This ignors the query params, as you discovered in the issue. So I suggest, we strip it here:

sentry-javascript/packages/tracing-internal/src/node/integrations/express.ts

Line 338 in f359ef3

req._reconstructedRoute = req.originalUrl;

This makes me think that perhaps this issue should be solved somewhere else and that my PR is only solving the symptom.

While strippingfinalRoute is probably more universal, it potentially hides other flaws which we want to fix down the line. So I suggest for now, we strip params in the place I linked above. Would you mind making that change?

[richardsimko]

Sure thing! I moved the call to where you suggested.

[Lms24]

Thanks!