sentry-javascript icon indicating copy to clipboard operation
sentry-javascript copied to clipboard

Chrome Extension using `@sentry/browser` gets rejected

Open SpaceK33z opened this issue 1 year ago • 1 comments

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

  1. Submit the extension to the Chrome Webstore
  2. 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.

SpaceK33z avatar Oct 17 '24 11:10 SpaceK33z

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

chargome avatar Oct 17 '24 15:10 chargome

@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.

SpaceK33z avatar Oct 21 '24 12:10 SpaceK33z

@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?

chargome avatar Oct 21 '24 12:10 chargome

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).

SpaceK33z avatar Oct 22 '24 15:10 SpaceK33z

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.

s1gr1d avatar Oct 23 '24 07:10 s1gr1d

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.

SpaceK33z avatar Oct 23 '24 10:10 SpaceK33z

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.

s1gr1d avatar Oct 23 '24 12:10 s1gr1d

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

SpaceK33z avatar Oct 23 '24 13:10 SpaceK33z

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

s1gr1d avatar Oct 23 '24 14:10 s1gr1d

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 😰

JensAstrup avatar Nov 03 '24 02:11 JensAstrup

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?

s1gr1d avatar Nov 04 '24 09:11 s1gr1d

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

JensAstrup avatar Nov 08 '24 08:11 JensAstrup

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you remove the label Waiting for: Community, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

getsantry[bot] avatar Nov 30 '24 08:11 getsantry[bot]

For us it worked fine with @sentry/browser, when we imported all dependencies granularly, like:

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

const sentryClient = new BrowserClient({
  integrations: [
    browserApiErrorsIntegration(),
    breadcrumbsIntegration(),
    globalHandlersIntegration(),
  ],
  ...
});

or

import { captureException } from '@sentry/browser';

❌ Unfortunately, this approach did not work with @sentry/react. CDN links were still included into the final bundle and it's sourcemap also. We tried to build in development and production modes. In both cases the result is the same, it did not help. So for us, the temporary solution is to clean up the CDN links after the building process (we are using vite):

    plugins: [
      react(),
      copy({
        targets: [
          {
            src: resolve(PATHS.dist, 'APP_NAME.js'),
            dest: PATHS.dist,
            transform(content) {
              return content
                .toString()
                .replace(/https:\/\/browser\.sentry-cdn\.com/g, '');
            },
          },
          {
            src: resolve(PATHS.dist, 'APP_NAME.js.map'),
            dest: PATHS.dist,
            transform(content) {
              return content
                .toString()
                .replace(/https:\/\/browser\.sentry-cdn\.com/g, '');
            },
          },
        ],
        hook: 'writeBundle',
      }),
   ...

FedorT22 avatar Dec 11 '24 08:12 FedorT22

I have a feeling this is mostly related to bundler and tree-shaking setup. All modern bundlers are able to tree-shake out unused APIs, like lazyloadIntegration. It shouldn't even matter if you're importing a namespace (import * as Sentry ...) or you only use named imports import {...} from ....

Likewise, I can't see a reason as to why @sentry/react would behave differently from @sentry/browser. The only exception is that the React SDK depends on the Browser SDK. lazyloadIntegration is defined in the Browser SDK and only re-exported from the React SDK. Meaning perhaps tree shaking isn't configured correctly for transitive dependencies?

We'd still need a minimal reproduction to investigate this further. Thanks!

Lms24 avatar Dec 12 '24 10:12 Lms24

I've faced this same issue when importing from the @sentry/browser package. Replacing wildcard imports by more specific imports didn't work for me, but I've managed to get a review approval by following the same approach @FedorT22 suggests in https://github.com/getsentry/sentry-javascript/issues/14010#issuecomment-2534815544.

In my project we're using the Rollup bundler, so I've bounced of the input plugin rollup-plugin-modify to strip the Sentry CDN URL:

const rollup = require('rollup');
const modify = require('rollup-plugin-modify');

rollup
  .rollup({
    input: 'src/main/background/index.ts',
    plugins: modify({
      find: 'https://browser.sentry-cdn.com',
      replace: '',
    }),
  })
  .then((bundle) => {
    return bundle.write({
      // ...
    });
  })

piloudu avatar Dec 17 '24 08:12 piloudu

Glad you found a workaround!

However, in general I'd still argue that there must be a tree-shaking related problem in build setups where lazyloadIntegration is included. That is as long as you don't use feedbackIntegration.

My comment from above still stands:

We'd still need a minimal reproduction to investigate this further. Thanks!

Lms24 avatar Dec 17 '24 08:12 Lms24