fix(core): Only consider ingest endpoint requests when checking isSentryRequestUrl
#17393
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
size-limit report 📦
|
Lms24
commented
Aug 12, 2025
Comment on lines
+104
to
+105
| fetchData: { url: 'https://dsn.ingest.sentry.io/1337?sentry_key=123', method: 'POST' }, | ||
| args: ['https://dsn.ingest.sentry.io/1337?sentry_key=123'], |
There was a problem hiding this comment.
this perfectly illustrates the consequences of this change. Previously, this URL was considered a "Sentry request", now we have to add the query parameter for it to still be considered one. This is likely just because the test was "incomplete" before but it does open up potential for a few false negatives. Thoughts anyone?
We can further refine the URL matching logic as we want
3 tasks
Lms24
force-pushed
the
lms/fix-core-isSentryRequestUrl
branch
from
44ff5ee to
9209fb3
Compare
Lms24
force-pushed
the
lms/fix-core-isSentryRequestUrl
branch
from
9209fb3 to
21aac6b
Compare
chargome
approved these changes
Aug 26, 2025
This was referenced Sep 29, 2025
This was referenced Oct 7, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR makes a change to our
isSentryRequestUrlutility which is used in various parts of the SDKs.The function checks both, the DSN as well as the
tunneloption to determine if a request URL is a URL to Sentry. I would argue, we should only returntruefor requests to Sentry's ingest endpoint. For example, if users make regular requests to the Sentry API from within their app that uses a Sentry SDK, we should not special case that request.Therefore, this PR makes the check for the request URL more specific:
tunnelis not provided, returntrueiff the the url includes the host of the DSN AND if it includes thesentry_keyquery param. This param is mandatory to be sent along, as it's equal to the public key of the DSN .tunnelis provided, the check was already specific enough because the request URL has to match exactly the configured tunnel URL.closes #17385
(^ very likely. We didn't repro this specifically but the
httpClientIntegrationbails out exactly if it hits theisSentryRequestUrlcheck)