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

fix(toolbar): Include credentials with fetch requests #82108

Merged
merged 1 commit into from
Dec 13, 2024

Conversation

ryan953
Copy link
Member

@ryan953 ryan953 commented Dec 13, 2024

Our fetch requests are cross-domain by design, we're making requests from a customer domain like acme.sentry.io into a an region like us.sentry.io. Therefore we need to include credentials. This is also how the website is configured:

this.credentials = options.credentials ?? 'include';

More docs: https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API/Using_Fetch#including_credentials

Previously we had same-origin which worked because we weren't using the regionUrl. We were instead sending api requests to the customer domain at acme.sentry.io. This meant that api requests were slower than using the regionUrl, but and also we were unable to make requests for api endpoints that don't include an /:organization/ segment.

So using the regionUrl is an improvement, and this PR updates the credentials field to match.

Another thing to consider is that before we were using window.location.origin to make requests. That has since been replaced by

const regionUrl = '{{ region_url|escapejs }}';
so we can trust that we're always sending these credentials off to a domain that the server trusts and told us about.

Related to #81942

@ryan953 ryan953 requested a review from a team December 13, 2024 22:40
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Dec 13, 2024
@ryan953 ryan953 changed the title fix(toolbar): Include credentials wuith fetch reqeusts fix(toolbar): Include credentials with fetch requests Dec 13, 2024
@ryan953 ryan953 requested review from mdtro and markstory December 13, 2024 22:52
@ryan953 ryan953 enabled auto-merge (squash) December 13, 2024 23:06
@ryan953 ryan953 merged commit 796f3bd into master Dec 13, 2024
48 checks passed
@ryan953 ryan953 deleted the ryan953/toolbar-include-creds branch December 13, 2024 23:07
Copy link

codecov bot commented Dec 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #82108      +/-   ##
==========================================
- Coverage   80.37%   80.37%   -0.01%     
==========================================
  Files        7275     7275              
  Lines      321435   321435              
  Branches    20962    20962              
==========================================
- Hits       258361   258355       -6     
- Misses      62663    62669       +6     
  Partials      411      411              

evanh pushed a commit that referenced this pull request Dec 17, 2024
Our fetch requests are cross-domain by design, we're making requests
from a customer domain like `acme.sentry.io` into a an region like
`us.sentry.io`. Therefore we need to include credentials. This is also
how the website is configured:
https://github.com/getsentry/sentry/blob/aa22f5d2373fc19e224bc9cc1fb30c405f05d782/static/app/api.tsx#L338

More docs:
https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API/Using_Fetch#including_credentials

Previously we had `same-origin` which worked because we weren't using
the regionUrl. We were instead sending api requests to the customer
domain at `acme.sentry.io`. This meant that api requests were slower
than using the regionUrl, but and also we were unable to make requests
for api endpoints that don't include an `/:organization/` segment.

So using the regionUrl is an improvement, and this PR updates the
credentials field to match.

Another thing to consider is that before we were using
`window.location.origin` to make requests. That has since been replaced
by
https://github.com/getsentry/sentry/blob/aa22f5d2373fc19e224bc9cc1fb30c405f05d782/src/sentry/templates/sentry/toolbar/iframe.html#L31
so we can trust that we're always sending these credentials off to a
domain that the server trusts and told us about.

Related to #81942
@github-actions github-actions bot locked and limited conversation to collaborators Dec 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants