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

Proper use of state parameter for the OAuth CSRF protection #1335

Closed
4 of 10 tasks
seratch opened this issue Feb 25, 2022 · 9 comments · Fixed by #1391
Closed
4 of 10 tasks

Proper use of state parameter for the OAuth CSRF protection #1335

seratch opened this issue Feb 25, 2022 · 9 comments · Fixed by #1391
Assignees
Labels
bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented semver:minor
Milestone

Comments

@seratch
Copy link
Member

seratch commented Feb 25, 2022

Description

Refer to slackapi/node-slack-sdk#1435 for details but in the next minor version, we are going to change the internals of the OAuth flow in bolt-js. The improvement won't bring any breaking changes. Newer versions of bolt-js (v3.11 and newer ones) will handle the OAuth flow more properly under the hood.

What type of issue is this? (place an x in one of the [ ])

  • bug
  • enhancement (feature request)
  • question
  • documentation related
  • example code related
  • testing related
  • discussion

Requirements (place an x in each of the [ ])

  • 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.
  • I've searched for any related issues and avoided creating a duplicate issue.
@seratch seratch added bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented semver:minor labels Feb 25, 2022
@seratch seratch added this to the 3.11.0 milestone Feb 25, 2022
@seratch seratch self-assigned this Feb 25, 2022
seratch added a commit to seratch/bolt-js that referenced this issue Mar 26, 2022
@pmezard
Copy link

pmezard commented Mar 31, 2022

Hello,

Not sure this is the right place to discuss it but I just upgraded from 3.10 to 3.11 and the OAuth flow stopped working with:

Error: The state parameter is not for this browser session.
   at new InvalidStateError2 (/node_modules/.pnpm/@slack/[email protected]/node_modules/@slack/oauth/src/errors.ts:28:1)
   at InstallProvider2.<anonymous> (/node_modules/.pnpm/@slack/[email protected]/node_modules/@slack/oauth/src/install-provider.ts:494:21)

Maybe I am not doing it right, I still have to investigate but The improvement won't bring any breaking changes might not be 100% correct.

@seratch
Copy link
Member Author

seratch commented Mar 31, 2022

@pmezard Thanks for writing in.

As long as your OAuth flow starts from /slack/install and the URLs for the OAuth flow are https ones, the new validation should work for you. And, this is the expected way to run the OAuth flow with bolt-js.

If you use some URLs like http://localhost:3000/slack/install, the change may affect because the cookie used for the user state verification has Secure flag. If many other people need http:// URL supports, we may consider adding a new option to enable it. But we've never received the requests in bolt-python and bolt-java.

I hope this was helpful to you.

@pmezard
Copy link

pmezard commented Mar 31, 2022

Still not have time to investigate properly but here is more context:

  • The flow is running in AWS Lambda, with the express receiver. It is not a local setup
  • The start URL is generated "manually" by calling installer.generateInstallUrl, in a different part of the application. We are not passing through /install AFAIK.

I guess I will have to take a look at the change and see if I can adjust our code.

@seratch
Copy link
Member Author

seratch commented Mar 31, 2022

@pmezard Ah I see. This makes sense. The new way requires running handleInstallPath() method instead (it generates the same URL plus sets the state value to the browser cookies too). If you use the built-in OAuth flow URL handling, the /slack/install uses the new method internally.

If you have a certain reason to continue directly using generateInstallUrl right now, you can use installerOptions.legacyStateVerification: true option to switch back to v3.10 behavior.

@pmezard
Copy link

pmezard commented Mar 31, 2022

I am generating the URL that way because I need to inject metadata in the URL. Maybe there is hook in the ExpressReceiver I can use to do that.

@seratch
Copy link
Member Author

seratch commented Mar 31, 2022

bolt-js v3.11 (and its underlying OAuth module v2.5) offers a more flexible way to set additional information through the OAuth flow. You can use beforeRedirection callback in /slack/install and then beforeInstallation callback in /slack/oauth_redirect. Setting 1st party cookies to transfer metadata or whatever may be easier for your use case too.

Refer to the OAuth module's documentation for more details: https://slack.dev/node-slack-sdk/oauth#persisting-data-during-the-oauth-flow

You can pass beforeRedirection function in installPathOptions in bolt-js app. beforeInstalltion is a new addition to callbackOptions.

@xdumaine
Copy link

I've picked up maintenance of a slack app that is also using gneerateInstallUrl and I'm also getting the above error. using installerOptions.legacyStateVerification: true doesn't work, unfortunately. Posting in case anyone else has ideas while I investigate. I'll try to post back once I've solved it.

@xdumaine
Copy link

I was wrong, that option did fix it, when using 3.11.3. I needed to put in in the installerOptions we were providing to new ExpressReceiver. I had first tried adding installerOptions to the new App If i wasn't in the middle of a hackathon I'd take some more time to try to figure out this code

@cjbara
Copy link

cjbara commented Aug 29, 2022

If you use some URLs like http://localhost:3000/slack/install, the change may affect because the cookie used for the user state verification has Secure flag. If many other people need http:// URL supports, we may consider adding a new option to enable it. But we've never received the requests in bolt-python and bolt-java.

@seratch is there a workaround using the callbacks for http installations (specifically for local development)? I would love to use the new verification method but it breaks my local installation flow for testing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented semver:minor
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants