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

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

Merged
merged 2 commits into from
Mar 28, 2022

Conversation

seratch
Copy link
Member

@seratch seratch commented Mar 26, 2022

Summary

This pull request resolves #1335 by migrating to the new handleInstallPath() method, which is newly added in @slack/[email protected].

Requirements (place an x in each [ ])

@seratch seratch added bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented enhancement M-T: A feature request for new functionality labels Mar 26, 2022
@seratch seratch added this to the 3.11.0 milestone Mar 26, 2022
@seratch seratch requested review from filmaj and srajiang March 26, 2022 04:55
@seratch seratch self-assigned this Mar 26, 2022
@codecov
Copy link

codecov bot commented Mar 26, 2022

Codecov Report

Merging #1391 (2b17cfb) into main (5e19ee1) will decrease coverage by 0.16%.
The diff coverage is 73.68%.

❗ Current head 2b17cfb differs from pull request most recent head 89e6539. Consider uploading reports for the commit 89e6539 to get more accurate results

@@            Coverage Diff             @@
##             main    #1391      +/-   ##
==========================================
- Coverage   77.13%   76.97%   -0.17%     
==========================================
  Files          18       17       -1     
  Lines        1478     1446      -32     
  Branches      434      426       -8     
==========================================
- Hits         1140     1113      -27     
+ Misses        239      232       -7     
- Partials       99      101       +2     
Impacted Files Coverage Δ
src/receivers/ExpressReceiver.ts 69.27% <57.14%> (+1.69%) ⬆️
src/receivers/HTTPReceiver.ts 56.60% <60.00%> (-3.16%) ⬇️
src/receivers/SocketModeReceiver.ts 87.01% <100.00%> (-0.63%) ⬇️
src/receivers/render-html-for-install-path.ts

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5e19ee1...89e6539. Read the comment docs.

Copy link
Contributor

@filmaj filmaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, left a couple of comments/questions.

src/receivers/ExpressReceiver.ts Show resolved Hide resolved
defaultRenderHtmlForInstallPath;
res.send(renderHtml(url));
}
await installer.handleInstallPath(req, res, installPathOptions, installUrlOptions);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👨‍🍳 💋

src/receivers/SocketModeReceiver.ts Show resolved Hide resolved
Copy link
Member

@mwbrooks mwbrooks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Always love seeing PRs with more code removal than additions! I think this refactor also makes Bolt's source easier to read.

@@ -60,6 +35,6 @@ receiver.router.get('/secret-page', (req, res) => {
});

(async () => {
await app.start(8080);
await app.start(3000);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌🏻

@seratch
Copy link
Member Author

seratch commented Mar 28, 2022

Thanks for the reviews!

@seratch seratch merged commit 0ed7874 into slackapi:main Mar 28, 2022
@seratch seratch deleted the issue-1335-oauth branch March 28, 2022 22:23
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 enhancement M-T: A feature request for new functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proper use of state parameter for the OAuth CSRF protection
3 participants