getsentry / sentry-javascript

Official Sentry SDKs for JavaScript
https://sentry.io
MIT License
7.88k stars 1.55k forks source link

7.17.1 attaching baggage header to third-party requests breaking CORS policies #6077

Closed redbugz closed 1 year ago

redbugz commented 1 year ago

Is there an existing issue for this?

How do you use Sentry?

Sentry Saas (sentry.io)

Which package are you using?

@sentry/react

SDK Version

7.17.1

Framework Version

React 16.x

Link to Sentry event

No response

Steps to Reproduce

Our app was running fine using sentry/tracing 7.16.0 We deployed new code with sentry/tracing 7.17.1 A bunch of third party requests started failing with CORS errors: blocked by CORS policy: Request header field baggage is not allowed by Access-Control-Allow-Headers in preflight response. We roll back to previous version, errors go away

in our test environment, with Sentry enabled and sentry/tracing at 7.17.1, CORS errors on third party requests to services on other domains that we don't control with Sentry disabled, everything is fine with Sentry/tracing on 7.16.0 and enabled, everything is fine

Expected Result

No CORS errors on third-party requests, no baggage header attached to third party requests

Actual Result

request has been blocked by CORS policy: Request header field baggage is not allowed by Access-Control-Allow-Headers in preflight response.

I have to assume this was caused by https://github.com/getsentry/sentry-javascript/pull/6039 We use default tracing config

        new TracingIntegrations.BrowserTracing(),

Which according to the docs: https://docs.sentry.io/platforms/javascript/performance/instrumentation/automatic-instrumentation/#tracingorigins should only impact calls to localhost and the same domain/origin as the page, but this does not seem to be happening

lobsterkatie commented 1 year ago

Hi, @redbugz.

Thanks for writing in! Yes, you're right about the PR. We broke one larger change into two PRs (that one and one which has yet to merge) and I didn't appreciate that the way they depend on each other means they really needed to be released together. Apologies!

I'll get that merged and we can do another release tomorrow.

Lms24 commented 1 year ago

Hi @redbugz,

we just released version 7.17.2 with a fix for this bug.

DaanSterk commented 1 year ago

Hi, unfortunately I'm still experiencing this issue. I'm on version 7.19.0 for both sentry/tracing and sentry/vue.

Lms24 commented 1 year ago

Hi @DaanSterk, would you mind sharing your Sentry.init config? I'm curious how you're setting your tracePropagationTargets (or tracingOrigins) options.

DaanSterk commented 1 year ago

Sure!

Sentry.init({
    app,
    dsn: "https://mydsn",
    integrations: [
        new BrowserTracing({
            routingInstrumentation: Sentry.vueRouterInstrumentation(router),
            tracingOrigins: ["localhost", "mydomain.nl", "sub.mydomain.nl", /^\//],
        }),
    ],
    environment: process.env.NODE_ENV,
    logErrors: true,
    // Set tracesSampleRate to 1.0 to capture 100%
    // of transactions for performance monitoring.
    // We recommend adjusting this value in production
    tracesSampleRate: 1.0,
})
Lms24 commented 1 year ago

I looked at this comment from the linked issue above and I might have an idea what's going on (assuming your URL requests look similar to the one mentioned in the comment).

I see that localhost appears in the URL query params (....current_domain=http%3A%2F%2Flocalhost&...) which matches your "localhost" string entry in tracingOrigins. Could you try making this string more specific (e.g. "localhost:" or something similar) and report back if it's still a problem?

DaanSterk commented 1 year ago

"localhost" is not part of my query params. In fact, no query params are present at all. Tried to remove "localhost" from tracingOrigins altogether, but the problem persists.

FYI, no issues on localhost, but I get the CORS errors when running the application in the production environment.

Could my nginx configuration be the source of this issue?

Lms24 commented 1 year ago

So while looking at this, I found a bug that could still explain why some headers were attached even if tracingOrigins items didn't match.

Because of how we internally check if we should attach these headers, in case of no match for tracingOrigins, we'd test the default values of tracePropagationTargets (the new option that replaces the now deprecated tracingOrigins). I'm currently writing up a fix for that.

However, based on information, I'm not entirely sure if this is the same cause. Can you confirm that if you test the URL against your tracingOrigins items, nothing would match? And further, would the URL still match the default values (['localhost', /^\//])?

Archi4400 commented 1 year ago

этот комментарий

I have the same problem on dev and prod environments prod CURL error curl 'https://salesiq.zoho.eu/visitor/v2/channels/website?widgetcode=myCode&internal_channel_req=true&last_modified_time=1668593786042&version=V26&browser_language=en&current_domain=https%3A%2F%2Fapp.brighterly.com&pagetitle=Demo%20Lesson%20-%20Brighterly%20Main%20App' \ -H 'sec-ch-ua: "Google Chrome";v="107", "Chromium";v="107", "Not=A?Brand";v="24"' \ -H 'Referer: https://app.brighterly.com/' \ -H 'baggage: sentry-environment=production,sentry-public_key=d707d73dfaac4abb8e97d83185d0f317,sentry-trace_id=649fe24751994bbc839d129faf65e979,sentry-sample_rate=1' \ -H 'sec-ch-ua-mobile: ?1' \ -H 'User-Agent: Mozilla/5.0 (Linux; Android 6.0; Nexus 5 Build/MRA58N) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/107.0.0.0 Mobile Safari/537.36' \ -H 'sentry-trace: 649fe24751994bbc839d129faf65e979-8d6fd8fe807a07e7-1' \ -H 'sec-ch-ua-platform: "Android"' \ --compressed Screenshot 2022-11-18 at 17 56 36

Lms24 commented 1 year ago

Hi @Archi4400

just stressing this again: Please be aware that the entire URL, including query params, is currently matched against whatever you define in tracingOrigins/tracePropagationTargets. If your domain occurs in a query param (like in the example above), headers will still be attached. We can discuss stripping query params in the future but for now, you should adjust your regexes (this is by the way not new behaviour).

On a related note, there still was a bug in our internal shouldAttachHeaders function, which should be fixed with #6238

Archi4400 commented 1 year ago

Привет@Archi4400

просто еще раз подчеркнем: имейте в виду, что весь URL-адрес, включая параметры запроса , в настоящее время сопоставляется с тем, что вы определяете в tracingOrigins/ tracePropagationTargets. Если ваш домен встречается в параметре запроса (как в приведенном выше примере), заголовки все равно будут прикреплены. Мы можем обсудить удаление параметров запроса в будущем, но сейчас вы должны настроить свои регулярные выражения (кстати, это не новое поведение).

В связи с этим в нашей внутренней функции все еще была ошибка shouldAttachHeaders, которая должна быть исправлена ​​​​с помощью # 6238.

vue 3

Screenshot 2022-11-18 at 18 09 18
Lms24 commented 1 year ago

@Archi4400, As I stated above, your "app.brighterly.com" entry in tracePropagationTargets matches your URL:

https://salesiq.zoho.eu/visitor/v2/channels/website?widgetcode=myCode&internal_channel_req=true&last_modified_time=1668593786042&version=V26&browser_language=en&current_domain=https%3A%2F%2 app.brighterly.com&pagetitle=Demo%20Lesson%20-%20Brighterly%20Main%20App

Which is why our SDK attaches headers to this outgoing request.

Archi4400 commented 1 year ago

@Archi4400, As I stated above, your "app.brighterly.com" entry in tracePropagationTargets matches your URL:

Well I have already tried clearing tracePropagationTargets or putting a false value which is definitely not in the request, well it didn't work for me, also changing tracesSampleRate to 0. I seem silly but help me, I don't understand how to solve this

Lms24 commented 1 year ago

@Archi4400 we just released 7.20.1 with the fix I was mentioning further above. Mind giving this a try?

clearing tracePropagationTargets

If you don't want to attach headers anywhere (i.e. turn off distributed tracing), you can just specify an empty array: tracePropagationTargets: [].

also changing tracesSampleRate to 0

traceSampleRate doesn't have any influence on header attachment

Let's see if the bugfix resolves your issue

Archi4400 commented 1 year ago

@Lms24 only works in this case.

Screenshot 2022-11-22 at 10 45 33

since you said no change (not working) with the same error

Screenshot 2022-11-22 at 10 46 36

version

Screenshot 2022-11-22 at 10 48 45
DaanSterk commented 1 year ago

Setting tracePropagationTargets: [] works for me.

However, this disables performance monitoring.

Do I understand correctly that I must allow the "baggage" header in my webserver to get performance monitoring to work?

Archi4400 commented 1 year ago
Screenshot 2022-11-22 at 12 23 47

but tracingOrigins works for me, why is that?

Lms24 commented 1 year ago

Apologies folks, you're right, there's still another bug in our tracePropagationTargets implementation that explains your problems. Currently working on a fix which we'll release ASAP.

Lms24 commented 1 year ago

@DaanSterk

Do I understand correctly that I must allow the "baggage" header in my webserver to get performance monitoring to work?

Performance monitoring (the entire feature) should work without these headers. You need these headers in your outgoing requests if you want to enable distributed tracing (i.e. connect transactions e.g. from your frontend to your backend).

Lms24 commented 1 year ago

Hi @Archi4400 we released 7.21.0 with the fix. The issue was auto-closed after merging it. Let me know if tracePropagationTargets now work for you. Thanks!

Archi4400 commented 1 year ago

@Lms24 everything is ok, it works. Thanks!

reyoucat commented 1 year ago

THX U

ghost commented 1 year ago

I tried disabling traceFetch and traceXHR to fix this. According to the docs for those options it should have.

So what do I have to do to stop Sentry from modifying my requests altogether?

tushar33 commented 1 year ago

I am using version "@sentry/angular": "^7.57.0" with Angular and facing below issue when deployed on server

Reason: header ‘baggage’ is not allowed according to header ‘Access-Control-Allow-Headers’ from CORS preflight response

Lms24 commented 1 year ago

Hi @tushar33 there are a couple of reasons why this could happen and it's hard to debug this without more context. Some hints:

tracePropagationTargets: [/\//, /^(https:\/\/)?someDomain\.com\/api/]

If this doesn't help you figuring out what's going on, please feel free to open a new issue with a minimal reproduction so that we can look into it. Thanks!

tushar33 commented 1 year ago

Hi @tushar33 there are a couple of reasons why this could happen and it's hard to debug this without more context. Some hints:

  • If you take a look at the URL, would it match the strings/regexes in your tracePropagationTargets option?
  • Sometimes, services like Google Analytics make requests with URLs containing your domain (for example) in query parameters. Hence a simple string might match this part of the URL when in fact the host is a different one. In this case, I'd recommend setting the hosts as a regex in tracePropagationTargets, for example:
tracePropagationTargets: [/\//, /^(https:\/\/)?someDomain\.com\/api/]

If this doesn't help you figuring out what's going on, please feel free to open a new issue with a minimal reproduction so that we can look into it. Thanks!

@Lms24 Thanks for help.....my backend is hosted on a different domain so configured backend CORS to allow the sentry-trace and baggage headers.

yue4u commented 1 year ago

FWIW the default tracePropagationTargets will also produce errors when api returns 302, e.g: presigned download url from S3.

Lms24 commented 1 year ago

@yue4u can you elaborate on that please? How does tracePropagationTargets influence your URL?

yue4u commented 1 year ago

@yue4u can you elaborate on that please? How does tracePropagationTargets influence your URL?

I made a reproduction here: https://github.com/lab-yue/sentry-header-reproduction

Lms24 commented 1 year ago

@yue4u thanks for creating a reproduction! I tried it out and basically this is expected behaviour. You can resolve the error you're getting in two ways:

  1. Disable trace propagation. For example, pass an empty array to tracePropagationTargets:

    Sentry.init({
    dsn: "https://examplePublicKey@o0.ingest.sentry.io/0",
    
    integrations: [new Sentry.BrowserTracing()],
    
    // We recommend adjusting this value in production, or using tracesSampler
    // for finer control
    tracesSampleRate: 1.0,
    tracePropagationTargets: []
    });
  2. Allow the sentry-trace and baggage headers in your server CORS config:

    res.header(
    "Access-Control-Allow-Headers",
    "Accept, Authorization, Content-Type, X-Requested-With, Range, baggage, sentry-trace"
    )

I also opened a PR to your repro with both options. Both resolve the error for me. Lmk if this resolved the issue for you.

yue4u commented 1 year ago

@Lms24 Thanks for reviewing the reproduction! Actually I've already figured this out before commenting. Just wanted to leave the comment for someone else searching.

While it can be fixed both ways, I'm still thinking about if it is possible to avoid changing server headers without disabling tracing at all.

  1. In our case the third party server is not directly controlled by us so changing the header is not very straightforward.
  2. Any redirections like this will need client to disable tracing (which is enabled by default).
  3. It will be great if tracing still works on normal (non-CROS) api requests.
diegobejardelaguila commented 1 year ago

Here, trying to integrate sentry with next js 12. It happens to me that when installing sentry it generates this error:

Access to XMLHttpRequest at 'http://localhost:1337/api/auth/local' from origin 'http://localhost:3000' has been blocked by CORS policy: Request header field baggage is not allowed by Access-Control-Allow-Headers in preflight response.

// This file configures the initialization of Sentry on the client.
// The config you add here will be used whenever a users loads a page in their browser.
// https://docs.sentry.io/platforms/javascript/guides/nextjs/

import * as Sentry from "@sentry/nextjs";

Sentry.init({
  dsn: `${process.env.dsnSentry}`,

  // Adjust this value in production, or use tracesSampler for greater control
  tracesSampleRate: 1.0,

  // Setting this option to true will print useful information to the console while you're setting up Sentry.
  debug: false,

  replaysOnErrorSampleRate: 1.0,

  // This sets the sample rate to be 10%. You may want this to be 100% while
  // in development and sample at a lower rate in production
  replaysSessionSampleRate: 0.1,

  // You can remove this option if you're not planning to use the Sentry Session Replay feature:
  integrations: [
    new Sentry.Replay({
      // Additional Replay configuration goes in here, for example:
      maskAllText: true,
      blockAllMedia: true,
    })
  ],
});

now when I add BrowserTracing, it works correctly for me only when logging in but the rest of the endpoints again get the same error cors

integrations: [
    new BrowserTracing({ tracingOrigins: ["*"] }),
    new Sentry.Replay({
      // Additional Replay configuration goes in here, for example:
      maskAllText: true,
      blockAllMedia: true,
    })
  ],
diegobejardelaguila commented 1 year ago

diegobejardelaguila

fixed, the following additional headers should be added in backend:

'baggage', 'sentry-trace'... You must add all the headers that request sentinel or those that exit in error that are being blocked to the backend.

lobermann commented 1 year ago

Just want to let you know that I just experienced the same issue with the @sentry/vue package, and google brought me here. Updating to version 7.61.0 fixed it.

lopsum-me commented 1 year ago

We got this error even with 7.61.1. We fixed it by adding:

new Sentry.BrowserTracing({
      traceFetch: false
})

To prevent fetch calls for being traced (hence adding the headers).

diegobejardelaguila commented 1 year ago

Tenemos este error incluso con 7.61.1. Lo arreglamos agregando:

new Sentry.BrowserTracing({
      traceFetch: false
})

Para evitar fetchque se rastreen las llamadas (de ahí la adición de los encabezados).

Isn't the normal operation supposed to be to track calls?

lopsum-me commented 1 year ago

Tenemos este error incluso con 7.61.1. Lo arreglamos agregando:

new Sentry.BrowserTracing({
      traceFetch: false
})

Para evitar fetchque se rastreen las llamadas (de ahí la adición de los encabezados).

Isn't the normal operation supposed to be to track calls?

I thought the same until I read the type declarations:

/**
     * Flag to disable patching all together for fetch requests.
     *
     * Default: true
     */
    traceFetch: boolean;
Lms24 commented 1 year ago

@lopsum-me can you elaborate why you had to use traceFetch: false? Generally, you should be able to resolve any CORS issues by setting tracePropagationTargets so it only sends the sentry-trace and baggage header to domains that you checked accept these headers.