Skip to content

Conversation

@lwin-kyaw
Copy link

@lwin-kyaw lwin-kyaw commented May 11, 2025

Description

Social login via OAuthController

Google
Apple

Social login authentication flow
Screenshot 2025-04-29 at 8 25 46 PM

This PR adds OAuthController to facilitate the social login in the metamask extension.
It does not include the Login UI Flow.

Related issues

Fixes:

Manual testing steps

  1. Go to this page...

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

@github-actions
Copy link

github-actions bot commented May 11, 2025

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@lwin-kyaw lwin-kyaw changed the title Feat/oauth controller feat: oauth controller for Social Logins May 11, 2025
@lwin-kyaw lwin-kyaw changed the base branch from feat/social-flow-header to feat/social-flow-password-hint May 13, 2025 11:41
@chaitanyapotti chaitanyapotti changed the base branch from feat/social-flow-password-hint to feat/social-flow-header May 21, 2025 12:46
Copy link
Member

@chaitanyapotti chaitanyapotti left a comment

Choose a reason for hiding this comment

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

lgtm

@chaitanyapotti chaitanyapotti marked this pull request as ready for review May 23, 2025 04:36
@lwin-kyaw lwin-kyaw changed the title feat: oauth controller for Social Logins feat: added OAuthService for social logins May 26, 2025
@lwin-kyaw lwin-kyaw requested a review from mcmire May 26, 2025 15:54
Copy link

@NicholasEllul NicholasEllul left a comment

Choose a reason for hiding this comment

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

When using the OAuth Authorization Code Flow, the typical pattern involves redirecting the user to a backend server that can securely handle the authorization code and exchange it for an access token. However, in our case, the authorization code is returned directly to the user’s browser. This presents a security risk, as the browser is a much less secure environment and is susceptible to interception by attackers, especially in MitM scenarios.

For client side applications such as desktop, mobile, or single-page web apps, this risk is commonly mitigated through the use of PKCE (Proof Key for Code Exchange). Okta has a nice intro on it here: https://developer.okta.com/docs/concepts/oauth-openid/#authorization-code-flow-with-pkce-flow.

Google makes PKCE mandatory for client side applications like mobile/desktop apps and IMO extensions fit into this category. Docs on this can be found here: https://developers.google.com/identity/protocols/oauth2/native-app#step1-code-verifier

Unfortunately, it looks like apple does not support PKCE, but getting this right for google would at least help us better secure accounts using sign in with Google.

Adding it here would look something like:

  1. Generate the code_verifier and send the hashed version along with the authorization request to google
  2. When we send our authorization code to the web3auth backend, also provide the code_verifier to included when exchanging the auth code for an access token
  3. Google will validate the code matches the hashed version we had previously sent.

With PKCE in place, even if an attacker intercepts the authorization code, they would not be able to exchange it for an access token without also having the original code_verifier, which never leaves the client device.

authConnectionId: process.env.AUTH_CONNECTION_ID,
groupedAuthConnectionId: process.env.GROUPED_AUTH_CONNECTION_ID,
},
webAuthenticator: window.chrome.identity,

Choose a reason for hiding this comment

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

I see this uses a chrome specific API. Do we have the checks in place to ensure this doesn't cause an error on firefox?

Comment on lines +77 to +80
"notifications",
"identity",
"*://appleid.apple.com/*",
"*://*.chromiumapp.org/*"

Choose a reason for hiding this comment

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

Will any of these create a new permission request popup for our users? If so please consult with @Gudahtt prior to merging this. In the past, it was determined that introducing new permissions prompts would drive users to uninstall the extension, and should be avoided unless absolutely necessary.

Comment on lines +90 to +91
"offscreen",
"identity"

Choose a reason for hiding this comment

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

Similarly to the comment above, please confirm whether this will result in a new permission prompt for users.

Comment on lines 45 to 47
JSON.stringify({
client_redirect_back_uri: this.options.redirectUri,
}),

Choose a reason for hiding this comment

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

To prevent CSRF attacks, OAuth2 strongly encourages that state is a unique & unguessable value. Redirect UI here will be something an attacker can predict, thus would be able to exploit.

These kinds of attacks work by having the attacker send a request to the chrome.identity.getRedirectURL() from their malicious site that contains an auth code associated with their own google login. The extension would then assume this was the auth code associated with the user who was logging in, and bind the attackers social login to their wallet. More on this can be read in this document under Flawed CSRF protection

Using a unique and unguessable value stops this kind of attack. That is because if the attacker did attempt to send a request to the chrome.identity.getRedirectURL(), they wouldn't know what the state value would be. As a result, the extension would reject any malicious requests of this kind.

authUrl.searchParams.set('redirect_uri', this.options.redirectUri);
authUrl.searchParams.set('nonce', this.nonce);
authUrl.searchParams.set('prompt', this.prompt);

Choose a reason for hiding this comment

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

We are missing the state parameter here. We should have it be a unique & unguessable value that we can later verify

Choose a reason for hiding this comment

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

A good value could be crypto.randomUUID()

Comment on lines 42 to 47
const redirectUrlFromOAuth = await this.#webAuthenticator.launchWebAuthFlow(
{
interactive: true,
url: loginHandler.getAuthUrl(),
},
);

Choose a reason for hiding this comment

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

Above I added a couple comments about the importance of the state value. In launchWebAuthFlow's optional callback we should validate that the state we get back from the oauth provider matches what we provided at the start of the flow. If the value does not match, we should throw an error.

Comment on lines 119 to 121
#generateNonce(): string {
return Math.random().toString(16).substring(2, 15);
}

Choose a reason for hiding this comment

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

A nonce should be cryptographically random to prevent replay attacks

@lwin-kyaw lwin-kyaw changed the base branch from feat/social-flow-header to feat/metametrics-new-flow June 1, 2025 07:18
crypto.getRandomValues(bytes);
const codeVerifier = Array.from(bytes).join('');

const challengeBuffer = await crypto.subtle.digest(
Copy link
Member

Choose a reason for hiding this comment

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

@lwin-kyaw noble has this hash function support, i think we should try using that

import { sha256 } from '@noble/hashes/sha2';

e.g

#getHashedNonce(): string {
    const hashBytes = sha512(this.nonce);
    return remove0x(bytesToHex(hashBytes));
  }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants