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.

Sign up for GitHub

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

Jump to bottom

feat: add cross origin resource policy header #426

Merged
merged 1 commit into from
Sep 16, 2021
Merged

feat: add cross origin resource policy header #426

merged 1 commit into from
Sep 16, 2021

Conversation

ajhorst
Copy link
Contributor

@ajhorst ajhorst commented Sep 14, 2021

Summary

Checklist

  • Does your PR title have the correct title format?
  • Does your PR have a breaking change?:

@ajhorst ajhorst requested a review from jooohhn September 14, 2021 23:58
@ajhorst
Copy link
Contributor Author

ajhorst commented Sep 14, 2021

see this customer request: https://discourse.amplitude.com/t/cross-origin-resource-policy-cross-origin-header-enabled-js-sdk/6358/5

@ajhorst ajhorst merged commit 709078f into main Sep 16, 2021
@ajhorst ajhorst deleted the AMP-42465_cross_origin_header branch September 16, 2021 19:56
github-actions bot pushed a commit that referenced this pull request Sep 16, 2021
@amplitude-sdk-bot
chore(release): 8.7.0 [skip ci]
e010d3e 
# [8.7.0](v8.6.0...v8.7.0) (2021-09-16)

### Features

* add cross origin resource policy header ([#426](#426)) ([709078f](709078f))
@ekanth
Copy link

ekanth commented Jan 11, 2022 •
edited
Loading

@ajhorst could you please explain why this header was made default? The customer request you reference above isn't accessible so couldn't get the context on this. This is causing HTTP OPTIONS request to be sent out before the first request to amplitude.

Could this header option be controlled by an option/config?

@ajhorst
Copy link
Contributor Author

ajhorst commented Jan 11, 2022

Hello @ekanth, this was implemented as a security requirement that some users requested. Are you seeing an issue with the Options request being sent?

CC @kevinpagtakhan

@ekanth
Copy link

ekanth commented Jan 11, 2022

We proxy the amplitude requests through a subdomain on our side and this doesn't appear to make sense for that scenario and see a lot of OPTIONS requests after the library upgrade. So, could we make this optional? If default is preferred for most users, could we have a config option to disable/remove this header?

@kevinpagtakhan
Copy link
Contributor

Making cors optional: #489

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

@jooohhn jooohhn Awaiting requested review from jooohhn

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ajhorst @ekanth @kevinpagtakhan