Fix #1435 Proper use of state parameter for the OAuth CSRF protection #1436

seratch · 2022-02-25T09:50:13Z

Summary

This pull request resolves #1435. Refer to the issue for the context and the reason of the changes.

Requirements (place an `x` in each `[ ]`)

I've read and understood the Contributing Guidelines and have done my best effort to follow them.
I've read and agree to the Code of Conduct.

seratch

comments for reviewers

seratch · 2022-02-25T09:56:49Z

examples/oauth-v2/app.js

+// This works since @slack/oauth@2.5.0 or newer
+/*
+app.get('/slack/install', async (req, res) => {
+  await installer.handleInstallPath(req, res, {


This is the new way. It's better in security but, moreover, it will be much simpler.

seratch · 2022-02-25T09:57:21Z

packages/oauth/package.json

@@ -31,7 +31,7 @@
    "prepare": "npm run build",
    "build": "npm run build:clean && tsc",
    "build:clean": "shx rm -rf ./dist ./coverage ./.nyc_output",
-    "lint": "eslint --ext .ts src",
+    "lint": "eslint --fix --ext .ts src",


Enabled auto-fix for better productivity

seratch · 2022-02-25T09:57:52Z

packages/oauth/src/callback-options.ts

+  let browserUrl = redirectUrl;
+  if (isNotOrgInstall(installation)) {
+    browserUrl = `https://app.slack.com/client/${installation.team.id}`;
+  }
  const htmlResponse = `<html>


Updated the HTML content to align with bolt-python and bolt-java. I'm sure that this one should be better for most people.

seratch · 2022-02-25T09:58:20Z

packages/oauth/src/callback-options.ts

-  res.writeHead(500, { 'Content-Type': 'text/html' });
-  res.end('<html><body><h1>Oops, Something Went Wrong! Please Try Again or Contact the App Owner</h1></body></html>');
+  let httpStatus: number;
+  switch (error.code) {


Adjusted the HTTP status a bit

seratch · 2022-02-25T09:58:45Z

packages/oauth/src/default-render-html-for-install-path.ts

@@ -0,0 +1,18 @@
+export default function defaultRenderHtmlForInstallPath(addToSlackUrl: string): string {


Ported from bolt-js.

seratch · 2022-02-25T10:11:30Z

packages/oauth/src/install-provider.ts

      }
      if (!installOptions) {
        const emptyInstallOptions: InstallURLOptions = { scopes: [] };
        // eslint-disable-next-line no-param-reassign
        installOptions = emptyInstallOptions;
      }

-      const client = new WebClient(undefined, this.clientOptions);


Removed this instance creation and changed the following code to use a singleton object.

seratch · 2022-02-25T10:11:55Z

packages/oauth/src/install-provider.ts

@@ -500,18 +617,14 @@ export class InstallProvider {
      // End: Build the installation object

      // Save installation object to installation store
-      if (installation.isEnterpriseInstall) {


This if/else can be safely removed (again)

seratch · 2022-02-25T10:12:56Z

packages/oauth/src/state-stores/clear-state-store.spec.js

@@ -11,4 +11,17 @@ describe('ClearStateStore', async () => {
    const returnedInstallUrlOptions = await stateStore.verifyStateParam(new Date(), state);
    assert.deepEqual(installUrlOptions, returnedInstallUrlOptions);
  });
+
+  it('should detect old state values', async () => {


I've added the validation this time.

seratch · 2022-02-25T10:13:36Z

packages/oauth/src/state-stores/interface.ts

+
+  /**
+   * The timestamp that the state object was generated.
+   */
  now: Date;


Wanted to change this property name but we cannot do so for backward-compatibility

seratch · 2022-02-25T10:14:33Z

packages/oauth/src/stores/file-store.ts

+        enterprise,
+        team,
+        // user object can include token values
+        user: { id: user.id },


While doing tests this time, I found that this info-level logging can print user tokens

filmaj

Phew, epic work here! Took me a while to review as I had to read RFC 6749 😬 It looks good to me but I would feel better if someone else also took the time to review this too.

packages/oauth/src/install-provider-options.ts

filmaj · 2022-02-25T17:17:52Z

packages/oauth/src/install-provider-options.ts

+  directInstall?: boolean; // default false, disables rendering "Add to Slack" page for /slack/install when true
+
+  /**
+   * The defalt is "v2" ("v1" is for "classic" apps)


Suggested change

* The defalt is "v2" ("v1" is for "classic" apps)

* The default is "v2" ("v1" is for "classic" apps)

What specifically does this auth version refer to? Not the OAuth version, right? Is it the authentication procedure prior to introducing this state parameter verification built in mechanism in this PR?

Is it the authentication procedure prior to introducing this state parameter verification built in mechanism in this PR?

yes, it is. "v2" here means granular bot permissions (GBP), which is the default way to go now. "v1" is the previous permission model and it's now called "classic apps". Check the following resources for more details:

https://medium.com/slack-developer-blog/more-precision-less-restrictions-a3550006f9c3

https://api.slack.com/authentication/migration

https://api.slack.com/authentication/quickstart

OK, thanks for clarifying, good to know. Not sure if it's appropriate to keep these links as breadcrumbs for the comment? Just for future maintainers who may not understand the difference between the versions or even which version this is referring to. Maybe we could have a comment like:

The default is "v2" (a.k.a. granular bot permissions), different from "v1" (a.k.a. "classic apps"). More details here: https://medium.com/slack-developer-blog/more-precision-less-restrictions-a3550006f9c3

@filmaj Sounds good. I've updated the comments on authVersion.

packages/oauth/src/install-provider.spec.js

packages/oauth/src/install-provider.ts

examples/oauth-v2/app.js

packages/oauth/src/install-provider-options.ts

packages/oauth/src/state-stores/interface.ts

packages/oauth/src/install-provider.ts

packages/oauth/src/state-stores/interface.ts

packages/oauth/src/install-provider.ts

seratch

Thanks for your review! @yujinakayama

packages/oauth/src/install-provider.ts

yujinakayama · 2022-02-28T02:42:58Z

packages/oauth/src/state-stores/interface.ts

+   * Generates a valid state parameter value, which can be decoded as a StateObj object
+   * by the verifyStateParam() method. This value may be stored on the server-side with expiration.
+   * The InstallProvider verifies if this value is set in the installer's browser session.
+   */
  generateStateParam: (installOptions: InstallURLOptions, now: Date) => Promise<string>;


Although not directly related to this change, as a bolt-js user using custom state store, I've been wondering if encoding/decoding the entire InstallURLOptions into the state is necessary.

Currently the decoded InstallURLOptions returned by verifyStateParam() is used for the following purposes:

Passing redirect_uri to oauth.v2.access method

Maybe we can use req.url in handleCallback() instead?

Assigning metadata to Installation

Passing entire InstallURLOptions to CallbackOptions.success/failure

Do we really need this in the callbacks when we have Installation?

I feel the essential is only InstallURLOptions.metadata.

Is there any underlying reason for the design?

That said, I understand we cannot immediately change the public API (StateStore and CallbackOptions) until next major version bump.

@yujinakayama Thanks for the comment.

as a bolt-js user using custom state store, I've been wondering if encoding/decoding the entire InstallURLOptions into the state is necessary.

I do understand your point here. Actually, bolt-python and bolt-java (the ones I implemented as the main developer) do not have this requirement at all. This is specific to this Node SDK package and bolt-js.

Maybe we can use req.url in handleCallback() instead?

This is off-topic a bit but req.url in application code does not always return the publicly available full URL (e.g., in the case of apps behind a load balancer). Thus, knowing rediect_uri may be useful in some situations. That being said, this is not a common need. Also, if a developer would like to do this, including the URL as part of state parameter is not the only way to do so.

I feel the essential is only InstallURLOptions.metadata.
Is there any underlying reason for the design?

I've checked the bolt-js interface designer's design document again and found that the main intention of this design seemed to be just to transfer metadata to /slack/oauth_redirect URL (handleCallback() in code). This means that your question here is so reasonable. Going with InstallURLOptions here might be for flexibility to easily handle future enhancements but I cannot tell the exact reason behind that.

That said, I understand we cannot immediately change the public API (StateStore and CallbackOptions) until next major version bump.

Yes, I'm glad to know that we're on the same page on this. In the long run, we may consider adding StateStore v2 that to make 3rd party developers' custom StateStore code much simpler. That being said, having two types of StateStore interface can be even more confusing (particularly for the developers that are new to Slack app development). Also, the improvement in this PR should enable developers to simply go with the default StateStore for the purpose of OAuth CSRF protection.

Considering all the above, my plans for the near future is to provide more flexibility to developers by adding more callbacks in both /slack/install and /slack/oauth_redirect. The metadata in this SDK can be useful for some use cases but if you can set cookies, it should be more flexible and safer. Here is my proposal for it: #1438 If you have any comments on the design and plans, please feel free to write in there.

Applying the improvements in this PR plus having these callbacks in the next release should enable you to get rid of your own StateStore for customization (let me know if I'm missing something here).

Thanks again for your insights here!

Also, the improvement in this PR should enable developers to simply go with the default StateStore for the purpose of OAuth CSRF protection.

Let me correct my comment a bit. I meant that, with the improvement in this PR, the default ClearStateStore will become safe in the acceptable level. However, the ideal implementation would be to use sever-side database to manage active state values and access the server-side data when verifyStateParam() is called. In Python and Java, we provide out-of-the-box solutions for this:

https://github.com/slackapi/python-slack-sdk/tree/v3.15.1/slack_sdk/oauth/state_store

https://github.com/slackapi/java-slack-sdk/tree/v1.20.0/bolt/src/main/java/com/slack/api/bolt/service/builtin

node-slack-sdk, specifically @slack/oauth, can provide something similar in future versions. I will create a new ticket issue for it later.

…otection Co-authored-by: Yuji Nakayama <nkymyj@gmail.com>

seratch · 2022-03-02T01:59:40Z

@filmaj @misscoded @yujinakayama Thanks again for your great inputs here! I think that this PR already had a good amount of feedback. Let me merge it now.

odedpeer · 2022-04-03T05:43:34Z

packages/oauth/src/install-provider.ts

+            this.logger.warn('Enabling legacyStateVerification is not recommended as it does not properly work for OAuth CSRF protection. Please consider migrating from directly using InstallProvider#generateInstallUrl() to InstallProvider#handleInstallPath() for serving the install path.');
+          } else {
+            const stateInBrowserSession: string | undefined = extractCookieValue(req, this.stateCookieName);
+            if (!stateInBrowserSession || (stateInBrowserSession !== stateInQueryString)) {


In my use case the state is stored in a database. I created a custom State Store.

The client is a React app, and the backend is "AWS AppSync to Lambda" configuration, hence, not a nodejs web server.
I do not have nodejs request and response objects, nor access to the browser's cookies.

I am using node-slack-sdk to generate the install URL and to handle the callback, however, handleCallback is executed from a lambda function, without access to a browser.

stateInBrowserSession is always undefined in my case, and fails this check.

I am setting legacyStateVerification to true to avoid this.
I don't think the warning message is required in this case, and I ask you to keep legacyStateVerification since it is not only for for backward-compatibility with v2.4 and olders but also for backends that use custom state stores and do not have access to browser cookies

seratch added bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented pkg:oauth applies to `@slack/oauth-helper` labels Feb 25, 2022

seratch added this to the oauth@2.5.0 milestone Feb 25, 2022

seratch requested review from filmaj, stevengill, misscoded and srajiang February 25, 2022 09:50

seratch self-assigned this Feb 25, 2022

seratch commented Feb 25, 2022

View reviewed changes

seratch requested a review from mwbrooks February 25, 2022 10:15

filmaj approved these changes Feb 25, 2022

View reviewed changes

misscoded approved these changes Feb 25, 2022

View reviewed changes

seratch commented Feb 26, 2022

View reviewed changes

packages/oauth/src/install-provider.ts Outdated Show resolved Hide resolved

seratch commented Feb 26, 2022

View reviewed changes

packages/oauth/src/state-stores/interface.ts Outdated Show resolved Hide resolved

seratch mentioned this pull request Feb 27, 2022

Add more callbacks (beforeRedirection, before/afterInstallation, async success/failure) in InstallProvider #1438

Closed

6 tasks

yujinakayama reviewed Feb 28, 2022

View reviewed changes

packages/oauth/src/state-stores/interface.ts Outdated Show resolved Hide resolved

yujinakayama reviewed Feb 28, 2022

View reviewed changes

packages/oauth/src/install-provider.ts Outdated Show resolved Hide resolved

yujinakayama reviewed Feb 28, 2022

View reviewed changes

packages/oauth/src/install-provider.ts Outdated Show resolved Hide resolved

seratch commented Feb 28, 2022

View reviewed changes

packages/oauth/src/install-provider.ts Outdated Show resolved Hide resolved

yujinakayama reviewed Feb 28, 2022

View reviewed changes

This was referenced Feb 28, 2022

Add built-in StateStore implementations using server-side database #1439

Closed

Fix #1439 Add built-in StateStore implementations using server-side database #1441

Merged

seratch force-pushed the issue-1435-better-oauth branch from affafa2 to 13c89fb Compare March 1, 2022 07:06

Fix slackapi#1435 Proper use of state parameter for the OAuth CSRF pr…

57ae345

…otection Co-authored-by: Yuji Nakayama <nkymyj@gmail.com>

seratch force-pushed the issue-1435-better-oauth branch from 0ad74b2 to 57ae345 Compare March 1, 2022 08:45

seratch mentioned this pull request Mar 1, 2022

Fix #1438 Add more callbacks (beforeRedirection, before/afterInstallation, async success/failure) in InstallProvider #1442

Merged

2 tasks

seratch closed this Mar 2, 2022

seratch reopened this Mar 2, 2022

seratch merged commit fdd2e45 into slackapi:main Mar 2, 2022

seratch deleted the issue-1435-better-oauth branch March 2, 2022 02:00

odedpeer reviewed Apr 3, 2022

View reviewed changes

rockingskier mentioned this pull request Jul 3, 2022

handleInstallPath throws an error when stateVerification is false #1506

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #1435 Proper use of state parameter for the OAuth CSRF protection #1436

Fix #1435 Proper use of state parameter for the OAuth CSRF protection #1436

seratch commented Feb 25, 2022

seratch left a comment

seratch Feb 25, 2022

seratch Feb 25, 2022

seratch Feb 25, 2022

seratch Feb 25, 2022

seratch Feb 25, 2022

seratch Feb 25, 2022

seratch Feb 25, 2022

seratch Feb 25, 2022

seratch Feb 25, 2022

seratch Feb 25, 2022

filmaj left a comment •

edited

filmaj Feb 25, 2022

filmaj Feb 25, 2022

seratch Feb 25, 2022

filmaj Feb 28, 2022

seratch Feb 28, 2022

seratch left a comment

yujinakayama Feb 28, 2022 •

edited

seratch Feb 28, 2022

seratch Feb 28, 2022 •

edited

seratch commented Mar 2, 2022

odedpeer Apr 3, 2022

		@@ -0,0 +1,18 @@
		export default function defaultRenderHtmlForInstallPath(addToSlackUrl: string): string {

	* The defalt is "v2" ("v1" is for "classic" apps)
	* The default is "v2" ("v1" is for "classic" apps)

Fix #1435 Proper use of state parameter for the OAuth CSRF protection #1436

Fix #1435 Proper use of state parameter for the OAuth CSRF protection #1436

Conversation

seratch commented Feb 25, 2022

Summary

Requirements (place an x in each [ ])

seratch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filmaj left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seratch left a comment

Choose a reason for hiding this comment

yujinakayama Feb 28, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seratch Feb 28, 2022 • edited

Choose a reason for hiding this comment

seratch commented Mar 2, 2022

Choose a reason for hiding this comment

Requirements (place an `x` in each `[ ]`)

filmaj left a comment •

edited

yujinakayama Feb 28, 2022 •

edited

seratch Feb 28, 2022 •

edited