bruno icon indicating copy to clipboard operation
bruno copied to clipboard

Add state support to PKCE implementation

Open m-schrepel opened this issue 10 months ago • 5 comments

Issue Ref

1003

Description

State is a recommended part of the PKCE Oauth2 spec, but it is required by our identity provider (Okta). This implements support for state in a fairly straight-forward manner. We use the unique session id from the Oauth2 class (which might be the electron store?) and we make that URL safe and pass it along as a query param when the PKCE checkbox is enabled in the Oauth2 authorization code flow.

I implemented one more fix in the oauth2 helper file, also due to okta. The code previously had a conditional that checked for a code URL parameter when in the redirect flow, but okta returned that parameter before the final redirect was reached. That meant I changed the logic to say if we're at the callback url and we see this code query parameter, then execute the rest of the code. This should be backward compatible with anything else since only the PKCE flow uses this function and callback url is a requirement of the PKCE flow.

https://datatracker.ietf.org/doc/html/rfc6749#section-4.1.1 for more information on state

This pull request includes changes to the packages/bruno-electron/src/ipc/network directory that focus on improving the OAuth2 authentication process. The key changes involve enhancing the URL redirect validation in the authorizeUserInWindow function, simplifying the hash generation process in the generateCodeVerifier function, and adding a unique state parameter in the getOAuth2AuthorizationCode function.

URL Redirect Validation:

  • packages/bruno-electron/src/ipc/network/authorize-user-in-window.js: The onWindowRedirect function now checks if the URL includes the callbackUrl before checking for an authorization code. This change ensures that the finalUrl is always the callbackUrl.

Hash Generation:

  • packages/bruno-electron/src/ipc/network/oauth2-helper.js: The generateCodeChallenge function has been renamed to generateUniqueHash and simplified. It now directly creates a SHA-256 hash of the input string and returns it in base64url format. This function is used to generate a unique hash for the codeVerifier and the state parameter.

OAuth2 Authorization Code Retrieval:

  • packages/bruno-electron/src/ipc/network/oauth2-helper.js: The getOAuth2AuthorizationCode function now generates a unique state parameter using the session ID of the collection and includes this in the OAuth2 query parameters. This change adds an extra layer of security to the OAuth2 process by mitigating cross-site request forgery attacks.

Contribution Checklist:

  • [X] The pull request only addresses one issue or adds one feature.
  • [X] The pull request does not introduce any breaking changes
  • [x] I have added screenshots or gifs to help explain the change if applicable.
  • [x] I have read the contribution guidelines.
  • [X] Create an issue and link to the pull request.

Note: Keeping the PR small and focused helps make it easier to review and merge. If you have multiple changes you want to make, please consider submitting them as separate pull requests.

Publishing to New Package Managers

Please see here for more information.

m-schrepel avatar Apr 17 '24 22:04 m-schrepel

+1 Also, relates to #2148 that also fixes the intermediate redirect issue.

pietrygamat avatar May 09 '24 15:05 pietrygamat

+1 Can we get this merged sometime soon?

werge2121 avatar May 14 '24 12:05 werge2121

+1 This needed for us as well, we use Okta too and state param is required there, can this be merged anytime soon

dhananjaykadam avatar May 17 '24 04:05 dhananjaykadam

Shall state param be accepted as parameter from user instead or generating, created PR here any of these two can be merged.

dhananjaykadam avatar May 17 '24 08:05 dhananjaykadam

@lohxt1 Assigning this to you

Lets make it a priority to have this merged early next week.

helloanoop avatar May 22 '24 20:05 helloanoop