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

reactComponentAnnotation creates Bad Requests (400) when fetching from external data source such as Algolia

Open steve-rodri opened this issue 1 year ago • 10 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/react

SDK Version

8.11.0

Framework Version

18.3.1

Link to Sentry event

No response

SDK Setup/Reproduction Example

Sentry.init({
  dsn: "***",
  integrations: [
    Sentry.browserTracingIntegration(),
    Sentry.replayIntegration(),
  ],
  environment: process.env.NODE_ENV || "local",
  // Performance Monitoring
  tracesSampleRate: 1.0, //  Capture 100% of the transactions
  // Set 'tracePropagationTargets' to control for which URLs distributed tracing should be enabled
  tracePropagationTargets: ["localhost", SITE_URL.test, SITE_URL.production],
  // Session Replay
  replaysSessionSampleRate: process.env.NODE_ENV === "production" ? 0.1 : 1.0, // This sets the sample rate at 10%. You may want to change it to 100% while in development and then sample at a lower rate in production.
  replaysOnErrorSampleRate: 1.0, // If you're not already sampling the entire session, change the sample rate to 100% when sampling sessions where errors occur.
})

Steps to Reproduce

Here is my webpack config:

const { sentryWebpackPlugin } = require("@sentry/webpack-plugin")

const { NxAppWebpackPlugin } = require("@nx/webpack/app-plugin")
const { NxReactWebpackPlugin } = require("@nx/react/webpack-plugin")

const { join } = require("path")

module.exports = {
  output: {
    path: join(__dirname, "./dist"),
  },

  devServer: {
    port: 4200,
    historyApiFallback: { index: "/" },
  },

  plugins: [
    new NxAppWebpackPlugin({
      tsConfig: "./tsconfig.app.json",
      compiler: "babel",
      main: "./src/main.tsx",
      index: "./src/index.html",
      baseHref: "/",
      assets: ["./src/favicon.ico", "./src/assets"],
      styles: ["./src/styles.scss"],
      outputHashing: process.env["NODE_ENV"] === "production" ? "all" : "none",
      optimization: process.env["NODE_ENV"] === "production",
    }),
    new NxReactWebpackPlugin({}),
    sentryWebpackPlugin({
      authToken: process.env.SENTRY_AUTH_TOKEN_FRONTEND,
      org: "v-school",
      project: "lms-react-app",
      reactComponentAnnotation: { enabled: true },
    }),
  ],

  devtool: "source-map",
}

Expected Result

When reactComponentAnnotation is enabled I should not have a response from algolia indicating a failed request of 400.

Actual Result

Screenshot 2024-07-01 at 12 48 23 PM Screenshot 2024-07-01 at 12 48 39 PM

steve-rodri avatar Jul 01 '24 16:07 steve-rodri

@0Calories Would you mind taking a look at this if you got the time? Thanks!

lforst avatar Jul 02 '24 07:07 lforst

Tagging @Haroenv - this looks like an issue on our end (prop bleeding?), thought I would expect the Algolia SDK to guard from this.

JonasBa avatar Sep 26 '24 20:09 JonasBa

@JonasBa what do you mean by our end in this scenario? Sentry SDK, Algolia SDK?

smeubank avatar Sep 27 '24 10:09 smeubank

@smeubank I think we generate these annotations, but then they somehow end up in Algolia, which I would expect the Algolia SDK to guard from (unless we are doing something really bad here)

JonasBa avatar Sep 27 '24 10:09 JonasBa

The JS SDK will take any parameter and pass it on to the network. I guess what may happen in this case is you pass an extra prop to Configure, which then gets turned into a search parameter. As we can't know in advance what all possible parameters are, we don't have an allowlist.

Maybe sentry needs some configuration to prevent attaching these props to certain components?

Haroenv avatar Oct 07 '24 08:10 Haroenv

@Haroenv Please see the following not as passing around a hot potato but rather from a purely technical standpoint 😂: I think it would be more reasonable if the Algolia SDK had a denylist. Our plugin runs during build and in some situations, it is semantically impossible (without doing a completely unreasonable amount of computations) to know what component we are attaching attributes to.

cc @0Calories because he owns the annotation plugin.

lforst avatar Oct 07 '24 08:10 lforst

It's possible to solve the problem in InstantSearch indeed (workaround / patch can be here: https://github.com/algolia/instantsearch/pull/6380) but I'm not convinced that every component can take any arbitrary property. Surely there are more components that will break if you pass the component annotations?

Haroenv avatar Oct 07 '24 10:10 Haroenv

@Haroenv You're probably right. The reality is that the vast majority of components in the ecosystem can take arbitrary properties. It is rather uncommon for components to do something with every prop passed. Especially if the props are prefixed with data-.

We're looking into ways of making the plugin more robust - I have a hunch that it may be really hard. Of course not putting blame on anybody here.

lforst avatar Oct 08 '24 08:10 lforst

I'm working on https://github.com/getsentry/sentry-javascript-bundler-plugins/pull/617 which will add a configuration to specify components that should not have annotations applied. Users have brought up incompatibilities with other libraries so I do think it is important to have this option available on Sentry's side as well

0Calories avatar Oct 08 '24 22:10 0Calories

We could even have a list of components as a default, that we know are potentially problematic.

lforst avatar Oct 09 '24 07:10 lforst

fwiw on a next.js project in next.config.js I added:

reactComponentAnnotation: {
    enabled: false,
  },

as it was breaking site-wide search entirely. When possible will re-enable.

paulgrieselhuber avatar Jan 08 '25 11:01 paulgrieselhuber

You now can either disable react component annotation entirely or mark components you want to ignore by setting

reactComponentAnnotation: {
   enabled: true,
   ignoredComponents: ['ComponentAToIgnore', 'ComponentBToIgnore', ..]
}

I'll close this issue, if this still causes problems feel free to file a new issue.

andreiborza avatar Mar 07 '25 14:03 andreiborza

In case it wasn't already extremely clear, just wanted to leave this here for future folks facing this issue like I just did. The fix for me was simply:

reactComponentAnnotation: {
   enabled: true,
   ignoredComponents: ['Configure']
}

mikehwagz avatar Oct 03 '25 13:10 mikehwagz