Add crossOrigin option #362
Conversation
| @@ -759,7 +760,14 @@ function makeRequest(data) { | |||
| var img = newImage(), | |||
| src = globalServer + authQueryString + '&sentry_data=' + encodeURIComponent(JSON.stringify(data)); | |||
|
|
|||
| img.crossOrigin = 'anonymous'; | |||
| if ('crossOrigin' in globalOptions) { | |||
There was a problem hiding this comment.
What's the point in checking this? You're setting it to a default in globalOptions already. So it should always exist, correct?
There was a problem hiding this comment.
I'd say either remove this check, or remove the default. I think it makes more sense here to remove the default.
I'd rather let this feature go undocumented for the time being, since it'd make more sense to allow this to be done through a new transport, but that hasn't been fully thought out yet for raven.js.
Sound reasonable?
There was a problem hiding this comment.
Definitely sounds reasonable to have it undocumented. I added this check because I saw that in the testing, there's assumptions that you can override the globalOptions object completely (there were direct assignments to globalOptions). Doing so would remove the default globalOptions.crossOrigin. I was afraid there would be users that relied on this, assuming that all defaults in globalOptions are optional in testing.
But in real-world usage, globalOptions are merged from the config argument, so that shouldn't be an issue. Hence, I can remove this check.
There was a problem hiding this comment.
I added this check because I saw that in the testing, there's assumptions that you can override the globalOptions object completely (there were direct assignments to globalOptions).
Ahh, this is only being done in tests. globalOptions doesn't exist in a production build in the public scope, so there's no way to clobber it.
Automatically detecting whether the crossOrigin option is needed is unreliable as outlined in this SO answer: http://stackoverflow.com/a/9569822/315562
chrisirhc
force-pushed
the
feature/add-cross-origin
branch
from
ff4fe0d
to
ed0a23a
Compare
|
Updated PR based on comments. |
| @@ -17,6 +17,7 @@ var _Raven = window.Raven, | |||
| ignoreUrls: [], | |||
| whitelistUrls: [], | |||
| includePaths: [], | |||
| crossOrigin: 'anonymous', | |||
There was a problem hiding this comment.
Add this also into the flushRavenState() in raven.test.js plz.
|
After my last comment, 👍, unless @benvinegar has a major objection. |
Expected behavior when empty string is given should be that of the 'anonymous' string. Ref: https://developer.mozilla.org/en-US/docs/Web/HTML/CORS_settings_attributes
|
👍 |
|
Oops, I'll just add it into |
|
thx 🍰 ✨ |
|
Ah apologies, I wasn't checking notifications. Thank you!! |
Automatically detecting whether the crossOrigin option is needed is unreliable
as outlined in this SO answer: http://stackoverflow.com/a/9569822/315562
crossOriginattribute aren't sent with expected cookies when making request to same originuse-credentialsfor internal configurations where Sentry is behind a reverse proxy which requires credentialsResolves #344