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

Optional support of secret_key #508

Closed
wants to merge 3 commits into from
Closed

Conversation

seagullua
Copy link

We are using the Firefox JS engine to run the game on the iOS (cocos2d-js). The game is written in JavaScript and 100% share code with WEB version. The game runs in Spidermonkey which is much in common as Firefox is with some differences (for example we have an own implementation of XMLHTTPRequest).

The raven-js works well but I receive the error Missing required attribute in authentication header: sentry_secret.

I have read some discussions and the validation on the sentry server is done via Origin header. But in such case the JavaScript is running outside browser and URL means nothing in such environment.

I added a feature to set the sentry_secret as an option. In such environment, it is safe as the secret will be bundled inside the application.

As a bonus, Cordova and Phonegap application has same issues, after this pull request they will also support raven-js.

@mattrobenolt
Copy link
Contributor

Hey @seagullua, I think we should solve this generically the same way that we do for React Native. See: https://github.com/getsentry/raven-js/blob/master/plugins/react-native.js#L47-L52

In theory, we could do the same thing for all of the environments, no?

I also have 0 experience with these implementations, but I've been trying really hard to provide no support for secret key inside raven-js because then it exposes everyone else from attempting to use it incorrectly. If we remove these checks, it's likely that people will start embedding private keys into their browser JS. I'd rather encompass this behavior into an opt-in approach for mobile apps and whatnot some how. React Native is what we've been experimenting with at the moment.

@seagullua
Copy link
Author

I think that the best case would be not to parse secret from the DSN but provide it as an optional parameter in the options array. In such case, the check for secret will remain, but for such case as described it will be possible to use the secret.

@benvinegar
Copy link
Contributor

Yeah, I agree with @mattrobenolt that we can't verbatim accept a DSN w/ a secret key.

Opting in could be an acceptable solution. So, I'm thinking, we change the warning message to link to somewhere in the docs, i.e.:

throw new RavenConfigError('Do not specify your private key in the DSN. See: https://docs.getsentry.com/hosted/clients/javascript/config/#secret-key');

This links to a subheading in docs/config.rst that explains:

  • why Raven.js does not allow a secret key by default
  • scenarios when a secrey key could be acceptable (e.g. Electron, React Native, other things)
  • how to disable the secret key protection, e.g. by specifying allowSecretKey: true (or something else)

Thoughts @mattrobenolt?

@mattrobenolt
Copy link
Contributor

I'm ok with this. I think a flag to explicitly opt into private key would be a good middle ground. 👍

@benvinegar
Copy link
Contributor

Implemented in #525

@benvinegar benvinegar closed this Mar 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants