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

fix(browser): Ensure wrap() only returns functions #13838

Merged
merged 1 commit into from
Oct 1, 2024

Conversation

lforst
Copy link
Member

@lforst lforst commented Oct 1, 2024

__sentry_wrapped__ may be overwritten by libraries, causing subsequent code to crash if the new value is not a function.

@lforst lforst requested a review from chargome October 1, 2024 09:41
Copy link
Contributor

github-actions bot commented Oct 1, 2024

size-limit report 📦

Path Size % Change Change
@sentry/browser 22.63 KB +0.04% +7 B 🔺
@sentry/browser - with treeshaking flags 21.42 KB +0.03% +6 B 🔺
@sentry/browser (incl. Tracing) 34.86 KB +0.02% +6 B 🔺
@sentry/browser (incl. Tracing, Replay) 71.37 KB +0.01% +5 B 🔺
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags 61.8 KB +0.01% +5 B 🔺
@sentry/browser (incl. Tracing, Replay with Canvas) 75.72 KB +0.01% +5 B 🔺
@sentry/browser (incl. Tracing, Replay, Feedback) 88.49 KB +0.01% +7 B 🔺
@sentry/browser (incl. Tracing, Replay, Feedback, metrics) 90.37 KB +0.01% +8 B 🔺
@sentry/browser (incl. metrics) 26.91 KB +0.01% +2 B 🔺
@sentry/browser (incl. Feedback) 39.78 KB +0.01% +4 B 🔺
@sentry/browser (incl. sendFeedback) 27.3 KB +0.02% +4 B 🔺
@sentry/browser (incl. FeedbackAsync) 32.08 KB +0.03% +7 B 🔺
@sentry/react 25.39 KB +0.01% +2 B 🔺
@sentry/react (incl. Tracing) 37.84 KB +0.01% +3 B 🔺
@sentry/vue 26.8 KB +0.02% +4 B 🔺
@sentry/vue (incl. Tracing) 36.76 KB +0.02% +5 B 🔺
@sentry/svelte 22.77 KB +0.02% +4 B 🔺
CDN Bundle 23.95 KB +0.02% +3 B 🔺
CDN Bundle (incl. Tracing) 36.64 KB +0.02% +4 B 🔺
CDN Bundle (incl. Tracing, Replay) 71.13 KB +0.01% +3 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback) 76.44 KB +0.01% +1 B 🔺
CDN Bundle - uncompressed 70.16 KB +0.04% +22 B 🔺
CDN Bundle (incl. Tracing) - uncompressed 108.62 KB +0.02% +22 B 🔺
CDN Bundle (incl. Tracing, Replay) - uncompressed 220.51 KB +0.01% +22 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed 233.73 KB +0.01% +22 B 🔺
@sentry/nextjs (client) 37.8 KB +0.02% +5 B 🔺
@sentry/sveltekit (client) 35.44 KB +0.04% +12 B 🔺
@sentry/node 125.13 KB - -
@sentry/node - without tracing 93.57 KB - -
@sentry/aws-serverless 103.28 KB - -

View base workflow run

@lforst lforst changed the title fix(browser): Ensure wrap only returns functions fix(browser): Ensure wrap() only returns functions Oct 1, 2024
@lforst lforst merged commit 35bdc87 into develop Oct 1, 2024
141 of 144 checks passed
@lforst lforst deleted the lforst-sentry-wrapped-ensure-consistency branch October 1, 2024 09:53
legobeat pushed a commit to legobeat/sentry-javascript that referenced this pull request Oct 3, 2024
@legobeat
Copy link

legobeat commented Oct 3, 2024

@lforst @chargome Is this slated for being backported in a v7 patch release? Would be very helpful as there is currently no stable upgrade path to @sentry/browser v8 for users of @sentry/react-native.

For consideration:

legobeat pushed a commit to legobeat/sentry-javascript that referenced this pull request Oct 3, 2024
@lforst
Copy link
Member Author

lforst commented Oct 4, 2024

@legobeat for sure. Thanks for bringing it up.

lforst added a commit that referenced this pull request Oct 4, 2024
…ort) (#13864)


Co-authored-by: Luca Forstner <luca.forstner@sentry.io>
@henrahmagix
Copy link

@lforst is it possible please for this to also be backported to v6? Or would it be too much work, unnecessary?

Just wondering because it would make our lives easier to not have to upgrade, since v6 comes with a bundle and we don't have bundling setup ourselves just yet =)

@lforst
Copy link
Member Author

lforst commented Oct 7, 2024

@henrahmagix good point! We discussed this internally and concluded that we will not backport the change to v6. Part of the decision was that this was technically a weakness, not a vulnerability. We think v6 is too old to justify any change. I encourage you to upgrade.

I will also note that we have no strict policy for which versions receive security fixes. Generally the latest major will receive fixes, for versions before that we will decide on a case-per-case basis.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants