-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
feat(browser): Prevent initialization in browser extensions #10844
Conversation
8e2fa68
to
1b85c6b
Compare
packages/browser/src/sdk.ts
Outdated
consoleSandbox(() => { | ||
// eslint-disable-next-line no-console | ||
console.error( | ||
'[Sentry] You cannot run Sentry this way in an extension, check: https://docs.sentry.io/platforms/javascript/troubleshooting/#setting-up-sentry-in-shared-environments-eg-browser-extensions', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'[Sentry] You cannot run Sentry this way in an extension, check: https://docs.sentry.io/platforms/javascript/troubleshooting/#setting-up-sentry-in-shared-environments-eg-browser-extensions', | |
'[Sentry] You cannot run Sentry this way in a browser extension, check: https://docs.sentry.io/platforms/javascript/troubleshooting/#setting-up-sentry-in-shared-environments-eg-browser-extensions', |
maybe a bit clearer, wording wise? 🤔
it('should log a browser extension error if executed inside a Chrome extension', () => { | ||
const consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation(() => {}); | ||
|
||
Object.defineProperty(WINDOW, 'chrome', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add an afterEach
block that resets this back to undefined, to ensure we don't leak this into other tests!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this block to reset it and I also added the test for the regular browser environment at last to make sure this fails directly if browser
or chrome
is still part of the globals.
size-limit report 📦
|
1b85c6b
to
6516bc9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very nice! Added two more very small nits, but this is good to go from my POV! great work :)
import * as Sentry from '@sentry/browser'; | ||
|
||
window.Sentry = Sentry; | ||
window.browser = { runtime: { id: 'mock-extension-id' } }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
window.browser = { runtime: { id: 'mock-extension-id' } }; | |
// We mock this here to simulate a browser extension | |
window.browser = { runtime: { id: 'mock-extension-id' } }; |
maybe leave a comment here for our future selves to know why this exists xD
import * as Sentry from '@sentry/browser'; | ||
|
||
window.Sentry = Sentry; | ||
window.chrome = { runtime: { id: 'mock-extension-id' } }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
window.chrome = { runtime: { id: 'mock-extension-id' } }; | |
// We mock this here to simulate a browser extension | |
window.chrome = { runtime: { id: 'mock-extension-id' } }; |
6516bc9
to
8c5ebc0
Compare
Prevents initialization inside chrome.* and browser.* extension environments.
Also refactored init() in browser because of eslint warning about too much complexity.
Fixes #10632