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

MM-54558 SSO code challenge verification #7587

Closed
wants to merge 3 commits into from
Closed

MM-54558 SSO code challenge verification #7587

wants to merge 3 commits into from

Conversation

enahum
Copy link
Contributor

@enahum enahum commented Oct 6, 2023

Summary

Adds code challenge verification for SSO logins using the web browser.

Applies to OAuth, OpenID and SAML

Ticket Link

https://mattermost.atlassian.net/browse/MM-54558

Server PR: mattermost/mattermost#24734

Release Note

Added code challenge verification for SSO login

@enahum enahum added 2: Dev Review Requires review by a core commiter 3: Security Review Review requested from Security Team labels Oct 6, 2023
@enahum enahum requested review from larkox and esarafianou October 6, 2023 12:36
@amyblais amyblais removed the 2: Dev Review Requires review by a core commiter label Oct 6, 2023
Copy link

@esarafianou esarafianou left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@esarafianou esarafianou requested a review from jupenur October 9, 2023 15:16
app/screens/sso/sso_with_redirect_url.tsx Outdated Show resolved Hide resolved
const chars = 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789';
const charsLenght = chars.length;
while (counter < length) {
result += chars.charAt(Math.floor(Math.random() * charsLenght));
Copy link
Member

Choose a reason for hiding this comment

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

This needs to use a CSPRNG. Math.random is notoriously predictable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Looks reasonable

@enahum enahum requested a review from jupenur October 10, 2023 13:40
@jupenur jupenur added the Do Not Merge Should not be merged until this label is removed label Oct 12, 2023
@enahum
Copy link
Contributor Author

enahum commented Nov 1, 2023

Closing as won't accept

@enahum enahum closed this Nov 1, 2023
@mattermost-build mattermost-build removed the Do Not Merge Should not be merged until this label is removed label Nov 1, 2023
@enahum enahum deleted the MM-54558 branch November 1, 2023 21:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3: Security Review Review requested from Security Team release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants