feat: @slack/oauth: add support and examples for CSRF mitigation #1013
Conversation
* Updates the version to 1.1.0 * Moves TypeScript dependencies that were in the production dependencies to the devDependencies
If the JWT doesn't expire, it can be used any time. * Adds configuration option to limit the lifetime of the state token * Adds default lifetime of 3 minutes
Adds documentation to the README to describe the default state lifetime, and how to override it.
* Replaces generateInstallUrl with makeInstallUrl, which returns both the url, and the token that was generated * Adds support for passing the token in with the options to callbackHandler, so it can be bound to the device in a cookie, and compared to the token that we received from Slack * Adds support for injecting the web client, so additional test paths can be evaluated * Improves test coverage * Fixes the mock web client responses (appId should be app_id)
Because the generateInstallUrl returned a string, it wasn't extensible, so I had to either (a) introduce a breaking change, or (b) introduce a new function. I chose the latter, and updated the documentation to demonstrate (b) in the examples.
* Adds dependency on `cookie` library * Uses new makeInstallUrl function to get the redirect url, and token * Adds the generated token to a secure, http-only cookie before redirecting to Slack OAuth * Parses the cookie from the headers when Slack redirects back to the app * Ensures the cookie exists before calling handleCallback, so there isn't a wait-for-expiration gap * Passes the token to handleCallback so it can be timingSafeCompared to the JWT that Slack send back in the query string
* Adds dependency on `cookie` library * Uses new makeInstallUrl function to get the redirect url, and token * Adds the generated token to a secure, http-only cookie before redirecting to Slack OAuth * Parses the cookie from the headers when Slack redirects back to the app * Ensures the cookie exists before calling handleCallback, so there isn't a wait-for-expiration gap * Passes the token to handleCallback so it can be timingSafeCompared to the JWT that Slack send back in the query string
My intention was to only send the random byte array as the value for the OAuth state param, and for the JWT to only exist in the cookie. However, that represents a potentially complex and/or breaking change. However, if the same JWT is used as the state param and in the cookie, and adversary who is able to capture the redirect url can easily spoof a synchronized device. So the examples sign JWTs specifically for the device, and provide the synchronizer to the callbackHandler for comparison.
A test breaking change was introduced in master. This changes the test to evaluate the new behavior.
|
|
Codecov Report
@@ Coverage Diff @@
## master #1013 +/- ##
=======================================
Coverage 94.40% 94.40%
=======================================
Files 12 12
Lines 768 768
Branches 173 173
=======================================
Hits 725 725
Misses 14 14
Partials 29 29
Continue to review full report at Codecov.
|
|
This is really nice, along with #1014, thank you for these detailed examples! I'd like to understand the pros/cons of various options for the cookie and the
Is it purely a matter of taste, or are there security benefits to any of these approaches? Also, just to be a little pedantic here - strictly speaking, the OWASP guide on STP says that
and notes that STP is more of a stateful protocol, where the CSRF token should be stored in user session on the server (instead of a cookie). For stateless apps, it's more appropriate to use Encryption, Hash, or Double Submit Cookie TP. The last one seems closest to what we're using here (though we're using HMAC+cookie+state instead of cookie+cookie). In any case, whichever terminology is used, they seem to recommend also encrypting the CSRF cookie to enhance the security of this solution. Any thoughts about the necessity of it? Thanks again! |
Summary
Adds support, and examples for mitigating Cross Site Request Forgery (CSRF) by synchronizing all 3 parties (user, Slack, app) in the OAuth flow. Uses the synchronizer token pattern (STP) and a JWT embedded in a cookie to bind the token to the device.
makeInstallUrlfunction that returns the URL that is produced, and the token that is used in the state parameter, and the synchronizer value that is in the tokenConcerns / Limitations
This leaves responsibility for synchronizing the device to the consumer of this library. Since the library already deals with
req, andres, it might make sense to support device synchronization as part of the library's features. That's a heavier lift, so this PR is intended to support conversation about the best way to promote CSRF mitigation when using this library.Also, the README doesn't discuss CSRF at all. Even though it's addressed in the examples, it might be worth adding content to the README. For instance, we might want to warn against using the same token in the cookie, and the state param, since that makes it easier to spoof a synchronized device.
Requirements (place an
xin each[ ])