Chrome Extension using `@sentry/browser` gets rejected

SpaceK33z commented 1 month ago

Is there an existing issue for this?

[x] I have checked for existing issues https://github.com/getsentry/sentry-javascript/issues
[x] I have reviewed the documentation https://docs.sentry.io/
[x] I am using the latest SDK release https://github.com/getsentry/sentry-javascript/releases

How do you use Sentry?

Sentry Saas (sentry.io)

Which SDK are you using?

@sentry/browser

SDK Version

8.34.0

Framework Version

No response

Link to Sentry event

No response

Reproduction Example/SDK Setup

import * as Sentry from '@sentry/browser';

Sentry.init({ dsn: sentryDsn}); // (does not matter)

Use this in a Chrome Extension in the service worker. In the content script we setup Sentry as outlined in these official docs — but just using it in a service worker is enough to trigger this.

Steps to Reproduce

Submit the extension to the Chrome Webstore
Wait

Expected Result

Extension is approved by the reviewers

Actual Result

Extension is rejected by the reviewers

The given reason:

Violating Content:
Code snippet: assets/[snip].js: const o = _b(n), i = k.document.createElement("script"); i.src = o, i.crossOrigin = "anonymous", i.referrerPolicy = "origin", t && i.setAttribute("nonce", t);
function _b(e) { const t = R(), n = t && t.getOptions(), r = n && n.cdnBaseUrl || "https://browser.sentry-cdn.com/"; return new URL(`/${yt}/${e}.min.js`, r).toString()

When looking up this code, it's coming from @sentry/browser and it looks like it's being used twice;

1) to lazy load integrations 2) and to load the "showReportDialog" feature

We use neither of these, yet still get rejected.

Probably not coincidentally, Chrome now is finally actually phasing out MV2 extensions (source), and only with MV3 remote code is not allowed to be executed. My guess is that they only started checking now.

It might be debatable that this is a bug, but I consider it as such because browser extensions seem to be officially supported.

The current work-around that I am using is to use patch-package to remove the code that injects a script tag in the lazyLoadIntegration and showReportDialog functions.

chargome commented 1 month ago

Hey @SpaceK33z, how are you exactly bundling your application? The lazyLoadIntegration for showReportDialog should basically be treeshakable if you're not including it.

SpaceK33z commented 1 month ago

@chargome I am bundling the application with Vite and following the official docs which has this line in it:

const integrations = getDefaultIntegrations({}).filter(
  (defaultIntegration) => {
    return !["BrowserApiErrors", "Breadcrumbs", "GlobalHandlers"].includes(
      defaultIntegration.name,
    );
  },
);

Because of that, it will always import all integrations. The docs should be changed to not teach this, but instead do this:

const integrations = [browserApiErrorsIntegration(), breadcrumbsIntegration(), globalHandlersIntegration()];

This is shorter, more readable, doesn't use "magic" strings, and most importantly enables tree-shaking which fixes this issue.

chargome commented 1 month ago

@SpaceK33z The snippet you provided does the exact opposite (not filtering out the integrations that use global state). So in your case this might work because it get's rid of all integrations but these three. In the docs there is also a line that states:

As a rule of thumb, it's best to avoid using any integrations and to rely on manual capture of errors only in such scenarios.

We could update the docs in this regard by adding a comment directly in the code snippet, wdyt?

SpaceK33z commented 1 month ago

This was definitely a "problem exists between chair and computer" situation; the whole experience was just very confusing because we suddenly got rejected from the Chrome Webstore;

In the end the real issue was that I was doing import * as Sentry from '@sentry/browser'. Doing that makes tree-shaking not work, and to be fair the docs show specific imports and don't use the *.

To prevent other users from having this issue, it could be helpful to add a warning about that to the docs; that importing * from Sentry might lead to a rejection of the webextension (it's not just Chrome, Firefox also doesn't like this).

s1gr1d commented 1 month ago

Bundlers should still be able to tree-shake this. Did you check the emitted build output? There should not be a place that uses lazyLoadIntegration.

SpaceK33z commented 1 month ago

Yes I checked the build output and it's there when using import * as Sentry from '@sentry/browser', and I verified that using specific imports (import { BrowserClient, ... } from '@sentry/core') fix the issue. This is using Vite.

It also makes sense because that's the whole reason we got rejected from the store, because that piece of code is in our bundle output. We got approved again by the store after switching to specific imports.

For me it's fine if this ticket gets closed, but potentially a warning to the docs like I mention above could be helpful for others.

s1gr1d commented 1 month ago

I just set up a Vite project with the vanilla preset and TS, bundled it and could not find lazyLoadIntegration in the build output.

Can you please share a minimal reproduction example of your setup so we can confirm this? It's useful to warn in the docs, but I want to make sure that this is not a problem within a specific setup or Vite preset.

SpaceK33z commented 1 month ago

Can you also not find any reference of document.createElement("script") in the output? Because it's not about lazyLoadIntegration, it's about whether it tries to execute remote code

s1gr1d commented 1 month ago

The only occurrences I found with createElement inside the whole output are iframe, function and link. A reproduction would be very useful here.

JensAstrup commented 3 weeks ago

I'm running into the same issue although I've got a vanilla Typescript Chrome Extension. Following the documentation and going with the approach mentioned above is still resulting in https://browser.sentry-cdn.com appearing in my final bundle.

import { BrowserClient, Scope, breadcrumbsIntegration, browserApiErrorsIntegration, defaultStackParser, globalHandlersIntegration, makeFetchTransport } from '@sentry/browser'

const manifestData = chrome.runtime.getManifest()
const integrations = [browserApiErrorsIntegration(), breadcrumbsIntegration(), globalHandlersIntegration()]
const client = new BrowserClient({
  dsn: process.env.SENTRY_DSN,
  transport: makeFetchTransport,
  stackParser: defaultStackParser,
  integrations,
  release: manifestData.version,
  environment: process.env.NODE_ENV,
})

const scope = new Scope()
scope.setClient(client)
client.init()

export default scope

I've also tried going with integrations: [] and am still seeing the lazyLoadIntegration usage of https://browser.sentry-cdn.com....which does make me feel similarly that this is a problem between the computer and chair. That being said, something about the documentation as it stands needs updating, because following it results in Chrome Extensions that aren't acceptable to use.

The Google Chrome Store is breathing down my back since I had written this off as likely a trivial issue to resolve, and now have only 2 weeks to resolve this so, any help is appreciated 😰

s1gr1d commented 3 weeks ago

Hello, as I only tried it with a Vite project with the vanilla preset and TS and could not reproduce this issue, it might be depending on a specific setup. Can you please share a minimal reproduction so we can debug this?

JensAstrup commented 2 weeks ago

This definitely doesn't count as a minimal reproduction option, but this is for my Shortcut Assistant extension which can be found here. I'll work on getting something minimal going though

getsentry / sentry-javascript