Conversation
…e-oauth-login-failures-caused-by-safari-itp
There was a problem hiding this comment.
Pull request overview
This pull request adds PKCE (Proof Key for Code Exchange) support to three OAuth extension packages for improved security in OAuth 2.0 flows. PKCE is an OAuth 2.0 extension designed to prevent authorization code interception attacks, particularly important for public clients. The implementation generates cryptographic challenges on the client side, stores the code verifier securely (in closure/sessionStorage), and performs client-side state verification before making RPC calls to the embedded wallet service.
Changes:
- Added PKCE cryptographic challenge generation using SHA256 with secure random number generation via crypto.getRandomValues
- Implemented client-side state verification for CSRF protection before RPC calls
- Added backward compatibility by conditionally including clientMetadata only when present in the new PKCE path
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 20 comments.
Show a summary per file
| File | Description |
|---|---|
| yarn.lock | Updated workspace dependency entries to include crypto-js and @types/crypto-js for all three OAuth packages |
| packages/@magic-ext/oauth2/package.json | Added crypto-js dependency (not shown in diff but implied by yarn.lock changes) |
| packages/@magic-ext/oauth2/src/crypto.ts | New file implementing PKCE challenge generation with SHA256, base64url encoding, and secure random string generation |
| packages/@magic-ext/oauth2/src/index.ts | Integrated PKCE flow into loginWithRedirect, storing verifier in sessionStorage and performing client-side state verification in getRedirectResult |
| packages/@magic-ext/oauth2/src/types.ts | Added pkceMetadata interface to OAuthRedirectStartResult for transmitting state, redirectUri, appID, and provider |
| packages/@magic-ext/react-native-bare-oauth/package.json | Added crypto-js as dependency and @types/crypto-js as devDependency |
| packages/@magic-ext/react-native-bare-oauth/src/crypto.ts | New file implementing PKCE challenge generation for React Native with Hermes crypto support detection |
| packages/@magic-ext/react-native-bare-oauth/src/index.ts | Integrated PKCE flow into loginWithPopup, storing verifier in closure and performing client-side state verification |
| packages/@magic-ext/react-native-bare-oauth/src/types.ts | Added pkceMetadata interface to OAuthRedirectStartResult with JSDoc-style documentation |
| packages/@magic-ext/react-native-expo-oauth/package.json | Added crypto-js as dependency |
| packages/@magic-ext/react-native-expo-oauth/src/crypto.ts | New file implementing PKCE challenge generation for React Native Expo with Hermes crypto support detection |
| packages/@magic-ext/react-native-expo-oauth/src/index.ts | Integrated PKCE flow into loginWithPopup, storing verifier in closure and performing client-side state verification |
| packages/@magic-ext/react-native-expo-oauth/src/types.ts | Added pkceMetadata interface to OAuthRedirectStartResult with JSDoc-style documentation |
Comments suppressed due to low confidence (1)
packages/@magic-ext/oauth2/src/crypto.ts:31
- The PKCE code verifier is 128 bytes, which produces a string of 128 characters. According to RFC 7636 section 4.1, the code verifier should be between 43 and 128 characters. While 128 characters is within spec, the actual requirement is for 43-128 characters of entropy in the final string. Since bytesToOAuth2CompatibleString uses modulo to map bytes to characters (value % charset.length), this can reduce the effective entropy. For maximum security and adherence to best practices, consider using 43-128 random characters directly or ensuring the byte-to-string conversion maintains the full entropy.
const cryptoChallengeState = createRandomString(128);
const codeVerifier = createRandomString(128);
const codeChallenge = verifierToBase64URL(CryptoJS.SHA256(codeVerifier));
return { codeVerifier, codeChallenge, cryptoChallengeState };
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const clientMetadata = successResult.pkceMetadata | ||
| ? { codeVerifier, ...successResult.pkceMetadata } | ||
| : undefined; |
There was a problem hiding this comment.
The clientMetadata object is constructed by spreading pkceMetadata, which includes the state field. However, if pkceMetadata contains a 'state' property and codeVerifier is also added, the resulting object will have both values. While this appears intentional, the type annotation Record<string, string> is imprecise. Consider creating a more specific interface type that explicitly defines all expected fields (codeVerifier, state, redirectUri, appID, provider) for better type safety and documentation.
| export function createCryptoChallenge(): { codeVerifier: string; codeChallenge: string; cryptoChallengeState: string } { | ||
| const cryptoChallengeState = createRandomString(128); | ||
| const codeVerifier = createRandomString(128); | ||
| const codeChallenge = verifierToBase64URL(CryptoJS.SHA256(codeVerifier)); | ||
| return { codeVerifier, codeChallenge, cryptoChallengeState }; |
There was a problem hiding this comment.
The PKCE code verifier is 128 bytes, which produces a string of 128 characters. According to RFC 7636 section 4.1, the code verifier should be between 43 and 128 characters. While 128 characters is within spec, the actual requirement is for 43-128 characters of entropy in the final string. Since bytesToOAuth2CompatibleString uses modulo to map bytes to characters (value % charset.length), this can reduce the effective entropy. For maximum security and adherence to best practices, consider using 43-128 random characters directly or ensuring the byte-to-string conversion maintains the full entropy.
| if (successResult?.pkceMetadata) { | ||
| // New path: store codeVerifier + all OAuth metadata at the SDK (parent page) level. | ||
| // sessionStorage persists across same-tab redirects but never enters the iframe. | ||
| sessionStorage.setItem( | ||
| PKCE_STORAGE_KEY, | ||
| JSON.stringify({ codeVerifier, ...successResult.pkceMetadata }), | ||
| ); |
There was a problem hiding this comment.
The cryptoChallengeState is generated and sent to the server but it's not clear from the code how it relates to pkceMetadata.state. The state verification compares pkceMetadata.state (returned from server) with the state in the OAuth callback URL, but there's no direct verification that cryptoChallengeState matches pkceMetadata.state. Consider adding a comment explaining that the server is expected to return the cryptoChallengeState as pkceMetadata.state, or add explicit validation that these values match.
| const clientMetadata = successResult.pkceMetadata | ||
| ? { codeVerifier, ...successResult.pkceMetadata } | ||
| : undefined; |
There was a problem hiding this comment.
The clientMetadata object is constructed by spreading pkceMetadata, which includes the state field. However, if pkceMetadata contains a 'state' property and codeVerifier is also added, the resulting object will have both values. While this appears intentional, the type annotation Record<string, string> is imprecise. Consider creating a more specific interface type that explicitly defines all expected fields (codeVerifier, state, redirectUri, appID, provider) for better type safety and documentation.
| // clientMetadata contains { codeVerifier, state, redirectUri, appID, provider }. | ||
| // Forwarding it lets the embedded-wallet verify handler skip its iframe storage entirely. | ||
| // When absent (old embedded-wallet path), the handler falls back to its stored metadata. | ||
| const clientMetadata = stored ? (JSON.parse(stored) as Record<string, string>) : undefined; |
There was a problem hiding this comment.
The clientMetadata object is constructed by spreading pkceMetadata, which includes the state field. However, if pkceMetadata contains a 'state' property and codeVerifier is also added, the resulting object will have both values. While this appears intentional, the type annotation Record<string, string> is imprecise. Consider creating a more specific interface type that explicitly defines all expected fields (codeVerifier, state, redirectUri, appID, provider) for better type safety and documentation.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
2 participants
📦 Pull Request
[Provide a general summary of the pull request here.]
✅ Fixed Issues
🚨 Test instructions
[Describe any additional context required to test the PR/feature/bug fix.]
Please 🚨 ONLY ADD ONE 🚨 of the following labels, failing to do so may lead to adverse versioning of your changes when published:
patch: Bug Fix?minor: New Feature?major: Breaking Change?skip-release: It's unnecessary to publish this change.Special Note
Please avoid adding any of the
Prioritylabels as they conflict with the labels above ☝️📦 Published PR as canary version:
Canary Versions✨ Test out this PR locally via:
npm install @magic-ext/oauth2@15.3.2-canary.1040.22248562051.0 npm install @magic-ext/react-native-bare-oauth@30.3.2-canary.1040.22248562051.0 npm install @magic-ext/react-native-expo-oauth@30.3.2-canary.1040.22248562051.0 # or yarn add @magic-ext/oauth2@15.3.2-canary.1040.22248562051.0 yarn add @magic-ext/react-native-bare-oauth@30.3.2-canary.1040.22248562051.0 yarn add @magic-ext/react-native-expo-oauth@30.3.2-canary.1040.22248562051.0