Verify Slack OAuth State #264
Conversation
Codecov Report
@@ Coverage Diff @@
## master #264 +/- ##
==========================================
+ Coverage 97.27% 97.41% +0.14%
==========================================
Files 76 76
Lines 1101 1122 +21
Branches 127 131 +4
==========================================
+ Hits 1071 1093 +22
+ Misses 28 27 -1
Partials 2 2
Continue to review full report at Codecov.
|
| @@ -1,7 +1,6 @@ | |||
| const { exec } = require('child_process'); | |||
| require('./env'); | |||
| require('./nock'); | |||
| require('./date'); | |||
There was a problem hiding this comment.
Hmm, anything that is using Date.now() should work. I would expect the opposite, where it never expires.
I'm good with whatever works and doesn't break the tests every month.
There was a problem hiding this comment.
For consistency with the GitHub OAuth process, what would you think about using jwt instead of cookies?
https://github.com/github-slack/app/blob/master/lib/github/oauth.js#L43
lib/slack/oauth.js
Outdated
| @@ -1,66 +1,98 @@ | |||
| const slack = require('./client'); | |||
| const cryptoRandomString = require('crypto-random-string'); | |||
There was a problem hiding this comment.
This package is pretty much one line of code. Can we do this manually to avoid unnecessary dependency?
https://github.com/sindresorhus/crypto-random-string/blob/master/index.js#L9
There was a problem hiding this comment.
Removing it 👍
FWIW it was already in our dependency tree, though.
lib/slack/oauth.js
Outdated
|
|
||
| if (!req.query.state) { | ||
| req.log.debug('No state param in callback.'); | ||
| return res.status(400).send('No state param in callback. Please try again.'); |
There was a problem hiding this comment.
What's the proper resolution for a user that ends up in this position? Would it make more sense to redirect to /slack/oauth/login here to re-start the OAuth process?
There was a problem hiding this comment.
Yeah I think that's one potential option. Although I've seen buggy implementation of this end up in infinite loops where the user is constantly redirected, which is of course even more confusing
There was a problem hiding this comment.
I'm gonna give this a shot now and see if it works well :)
lib/slack/oauth.js
Outdated
|
|
||
| if (!req.session.slackOAuthState) { | ||
| req.log.debug('No state in session.'); | ||
| return res.status(400).send('No state in session. Please try again.'); |
lib/slack/oauth.js
Outdated
|
|
||
| if (!matches) { | ||
| req.log.debug('Session state does not match state.'); | ||
| return res.status(400).send('Session state does not match state. Please try again.'); |
| @@ -1,7 +1,6 @@ | |||
| const { exec } = require('child_process'); | |||
| require('./env'); | |||
| require('./nock'); | |||
| require('./date'); | |||
There was a problem hiding this comment.
Hmm, anything that is using Date.now() should work. I would expect the opposite, where it never expires.
I'm good with whatever works and doesn't break the tests every month.
Interesting. So just a random value signed with the jwt representing the state? I have a feeling that might still leave us open to the CSRF attack vector. An attacker could just kick off the oauth flow, copy the value that is signed by our JWT, and then send a victim into a new OAuth flow which we wouldn't be able to detect. Thinking about it the reason why it's secure for GitHub OAuth is because we encode data in the state that's not really guessable: |
…lhelm-test into verify-slack-oauth-state
…rify-slack-oauth-state
This PR implements use of the state parameter in the slack OAuth flow to protect from CSRF attacks. More details about the "why" here
Fixes #257
Fixes #254