Skip to content
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

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

Closed
3 tasks done
redbugz opened this issue Oct 27, 2022 · 38 comments · Fixed by #6079 or #6259
Closed
3 tasks done

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

redbugz opened this issue Oct 27, 2022 · 38 comments · Fixed by #6079 or #6259

Comments

@redbugz
Copy link

redbugz commented Oct 27, 2022

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 #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

@redbugz redbugz changed the title 7.17.1 attaching baggage header to requests 7.17.1 attaching baggage header to third-party requests Oct 27, 2022
@redbugz redbugz changed the title 7.17.1 attaching baggage header to third-party requests 7.17.1 attaching baggage header to third-party requests breaking CORS policies Oct 27, 2022
@lobsterkatie
Copy link
Member

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.

@lobsterkatie lobsterkatie self-assigned this Oct 28, 2022
Lms24 pushed a commit that referenced this issue Oct 28, 2022
In the process of working on #5285, we missed the fact that the first two PRs (#6039 and #6041) were interdependent, in that the former accidentally introduced a bug (#6077) which the latter then inadvertently fixed. This would have been fine, except that we published a release after merging the bug-creating PR but before merging the bug-fixing PR. Whoops.

This patch pulls just the bug-fixing part out of the second PR. It also adds tests to cover the buggy cases, using `it.each` to cover all of the different combinations of outcomes for `shouldCreateSpanForRequest` and `shouldAttachHeaders`. Finally, since I was already in the test file, I reorganized it a little:
- `it('does not create span if shouldCreateSpan returns false')` -> absorbed into the `it.each()`
- `it('does not create span if there is no fetch data in handler data')` -> added header check, became `it('adds neither fetch request spans nor fetch request headers if there is no fetch data in handler data')`
- `it('does not add fetch request spans if tracing is disabled')` and `it('does not add fetch request headers if tracing is disabled` -> combined into `it('adds neither fetch request spans nor fetch request headers if tracing is disabled')`
- `it('adds sentry-trace header to fetch requests')` -> absorbed into the `it.each()`
- Similar changes made to XHR tests

Co-authored-by: Tim Fish <tim@timfish.uk>
@Lms24
Copy link
Member

Lms24 commented Oct 28, 2022

Hi @redbugz,

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

@DaanSterk
Copy link

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

@Lms24
Copy link
Member

Lms24 commented Nov 18, 2022

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

@DaanSterk
Copy link

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
Copy link
Member

Lms24 commented Nov 18, 2022

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
Copy link

"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
Copy link
Member

Lms24 commented Nov 18, 2022

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
Copy link

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

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
Copy link
Member

Lms24 commented Nov 18, 2022

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
Copy link

Archi4400 commented Nov 18, 2022

Привет@Archi4400

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

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

vue 3

Screenshot 2022-11-18 at 18 09 18

@Lms24
Copy link
Member

Lms24 commented Nov 21, 2022

@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
Copy link

Archi4400 commented Nov 21, 2022

@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
Copy link
Member

Lms24 commented Nov 23, 2022

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
Copy link

Archi4400 commented Nov 28, 2022

@Lms24 everything is ok, it works. Thanks!

@reyoucat
Copy link

reyoucat commented Dec 6, 2022

THX U

amcsi added a commit to amcsi/lycee-overture that referenced this issue Jan 3, 2023
@waynebloss
Copy link

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
Copy link

tushar33 commented Jul 6, 2023

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
Copy link
Member

Lms24 commented Jul 6, 2023

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!

@lobsterkatie lobsterkatie removed their assignment Jul 10, 2023
@tushar33
Copy link

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
Copy link

yue4u commented Jul 18, 2023

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

@Lms24
Copy link
Member

Lms24 commented Jul 18, 2023

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

@yue4u
Copy link

yue4u commented Jul 18, 2023

@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
Copy link
Member

Lms24 commented Jul 20, 2023

@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: []
});
  1. 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
Copy link

yue4u commented Jul 20, 2023

@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
Copy link

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
Copy link

diegobejardelaguila commented Jul 21, 2023

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
Copy link

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
Copy link

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
Copy link

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
Copy link

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
Copy link
Member

Lms24 commented Aug 7, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment