Merged
Conversation
* Add WDK Google ID token auth flow * Unify Google WDK auth kinds * Refine WDK Google id token flow * Fix id-token auth key cleanup on signer mismatch * Restore guard error logging * Unify Google WDK signer kind * Fix WDK auth flow cleanup and implicit session metadata
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
|
|
|
@taylanpince is attempting to deploy a commit to the Foundry development Team on Vercel. A member of the Team first needs to authorize it. |
Reviewer's GuideAdds first-class ID token login support (Google, Apple, custom OIDC) to the wallet WDK, refactors handler registration and signer kinds to use canonical login-google/apple keys, wires UI registration for ID token flows, preserves guard error causes, and bumps package versions to 3.0.4 with changelog updates. Sequence diagram for ID token signup flowsequenceDiagram
actor User
participant DappClient
participant Wallets
participant SharedHandlers
participant IdTokenHandler
participant IdentityInstrument
participant DbAuthKeys
User->>DappClient: Clicks sign up with Google (id-token)
DappClient->>Wallets: signUp({ kind: google-id-token, idToken })
Wallets->>SharedHandlers: get(Kinds.LoginGoogle)
SharedHandlers-->>Wallets: IdTokenHandler
Wallets->>IdTokenHandler: completeAuth(idToken)
IdTokenHandler->>IdentityInstrument: nitroCommitVerifier(IdTokenChallenge)
IdentityInstrument-->>IdTokenHandler: commit verified
IdTokenHandler->>IdentityInstrument: nitroCompleteAuth(IdTokenChallenge)
IdentityInstrument-->>IdTokenHandler: { signer, email }
IdTokenHandler->>DbAuthKeys: store auth key for signer
IdTokenHandler-->>Wallets: [signer, { email }]
Wallets-->>DappClient: { signer, extra.signerKind = Kinds.LoginGoogle, loginEmail }
DappClient-->>User: Wallet created and logged in
Class diagram for ID token and auth handlersclassDiagram
class IdentityHandler {
+Identity.IdentityInstrument nitro
+Db.AuthKeys authKeys
+Signatures signatures
+Identity.IdentityType identityType
+WdkEnv env
+getAuthKeySigner(address Address.Address) IdentitySigner
+createAuthKeySigner(authKey Db.AuthKey) IdentitySigner
+clearAuthKeySigner(address string) void
}
class IdTokenHandler {
+string signupKind
+string issuer
+string audience
-PromptIdTokenHandler onPromptIdToken
+IdTokenHandler(signupKind string, issuer string, audience string, nitro Identity.IdentityInstrument, signatures Signatures, authKeys Db.AuthKeys, env WdkEnv)
+kind string
+registerUI(onPromptIdToken PromptIdTokenHandler) function
+unregisterUI() void
+completeAuth(idToken string) IdentitySigner_map
+getSigner() signer_Signers_Signer_email_string
+status(address Address.Address, _imageHash Hex.Hex, request BaseSignatureRequest) SignerUnavailable_Ready_Actionable
-handleAuth(onPromptIdToken PromptIdTokenHandler) signer_Signers_Signer_email_string
}
class AuthCodeHandler {
+string signupKind
+string issuer
+string audience
+kind string
}
class AuthCodePkceHandler {
+string signupKind
+string issuer
+string audience
}
class ManagerOptions {
+IdentityOptions identity
}
class IdentityOptions {
+GoogleIdentityOptions google
+AppleIdentityOptions apple
+CustomIdentityProvider[] customProviders
}
class GoogleIdentityOptions {
+boolean enabled
+string clientId
+string authMethod_authcode_pkce_or_id_token
}
class AppleIdentityOptions {
+boolean enabled
+string clientId
+string authMethod_authcode_or_id_token
}
class CustomIdentityProvider {
+string kind
+string authMethod_id_token_or_authcode_or_authcode_pkce
+string issuer
+string oauthUrl
+string clientId
}
class Manager {
+shared handlers_Map
+identityInstrument Identity.IdentityInstrument
+modules signatures_Signatures
+registerIdTokenUI(onPromptIdToken PromptIdTokenHandler) function
}
class Wallets {
+shared Shared
+signUp(args SignupArgs) Promise
+startSignUpWithRedirect(args StartSignUpWithRedirectArgs) Promise
+completeRedirect(args CompleteRedirectArgs) Promise
}
class SignupArgs {
}
class IdTokenSignupArgs {
+string kind_google_id_token_or_apple_id_token_or_custom_string
+string idToken
}
class Kinds {
<<enumeration>>
+LoginGoogle
+LoginApple
}
IdentityHandler <|-- IdTokenHandler
IdentityHandler <|-- AuthCodeHandler
IdentityHandler <|-- AuthCodePkceHandler
ManagerOptions o-- IdentityOptions
IdentityOptions o-- GoogleIdentityOptions
IdentityOptions o-- AppleIdentityOptions
IdentityOptions o-- CustomIdentityProvider
Manager o-- IdTokenHandler
Manager o-- AuthCodeHandler
Manager o-- AuthCodePkceHandler
SignupArgs <|-- IdTokenSignupArgs
Wallets ..> IdTokenHandler
Wallets ..> AuthCodeHandler
Wallets ..> AuthCodePkceHandler
Kinds ..> IdTokenHandler
Kinds ..> AuthCodeHandler
Kinds ..> Wallets
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly expands the Wallet Development Kit's (WDK) authentication capabilities by integrating ID token login support for various OIDC providers, including Google, Apple, and custom solutions. This allows for more granular control over authentication flows, enabling developers to choose between traditional OAuth 2.0 Authorization Code Flow with PKCE and the new ID token method. Additionally, the changes include an important improvement to error handling in the Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 8 security issues, 4 other issues, and left some high level feedback:
Security issues:
- Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data. (link)
- Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data. (link)
- Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data. (link)
- Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data. (link)
- Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data. (link)
- Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data. (link)
- Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data. (link)
- Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data. (link)
General comments:
- In
IdTokenHandler.handleAuthyou're creating anew Promisewith anasyncexecutor, which can lead to unhandled rejections; consider removing theasyncfrom the executor and treatingonPromptIdToken/respondas plain promise chains instead. - In
IdTokenHandler.status, when handling anid-token-signer-mismatchyou callclearAuthKeySignerwith the returned signer address; double-check that this is the intended key to delete in the auth key store and that you don't inadvertently clear valid auth for identities that might still be in use elsewhere.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `IdTokenHandler.handleAuth` you're creating a `new Promise` with an `async` executor, which can lead to unhandled rejections; consider removing the `async` from the executor and treating `onPromptIdToken`/`respond` as plain promise chains instead.
- In `IdTokenHandler.status`, when handling an `id-token-signer-mismatch` you call `clearAuthKeySigner` with the returned signer address; double-check that this is the intended key to delete in the auth key store and that you don't inadvertently clear valid auth for identities that might still be in use elsewhere.
## Individual Comments
### Comment 1
<location path="packages/wallet/wdk/src/sequence/wallets.ts" line_range="801-807" />
<code_context>
}
async startSignUpWithRedirect(args: StartSignUpWithRedirectArgs) {
- const kind = args.kind.startsWith('custom-') ? args.kind : 'login-' + args.kind
- const handler = this.shared.handlers.get(kind) as AuthCodeHandler
+ const kind = getSignupHandlerKey(args.kind)
+ const handler = this.shared.handlers.get(kind)
if (!handler) {
throw new Error('handler-not-registered')
}
+ if (!(handler instanceof AuthCodeHandler)) {
+ throw new Error('handler-does-not-support-redirect')
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** Redirect sign-up now rejects AuthCodePkceHandler handlers, breaking Google PKCE redirect flows.
The previous version effectively treated the handler as any object with `commitAuth`, so both `AuthCodeHandler` and `AuthCodePkceHandler` (and other compatible handlers) worked. The new `instanceof AuthCodeHandler` guard causes a `AuthCodePkceHandler` registered under `Kinds.LoginGoogle` to fail with `handler-does-not-support-redirect`, breaking Google PKCE sign-up when `authMethod: 'authcode-pkce'`.
Please relax the check to allow both handler types, e.g.:
```ts
const handler = this.shared.handlers.get(kind)
if (!handler) throw new Error('handler-not-registered')
if (!(handler instanceof AuthCodeHandler) && !(handler instanceof AuthCodePkceHandler)) {
throw new Error('handler-does-not-support-redirect')
}
```
Alternatively, check for the required methods (e.g. `commitAuth`/`completeAuth`) instead of relying solely on `instanceof`, so any compatible redirect handler continues to work.
</issue_to_address>
### Comment 2
<location path="packages/wallet/wdk/test/wallets.test.ts" line_range="204-30" />
<code_context>
+ ).rejects.toThrow('handler-does-not-support-id-token')
+ })
+
+ it('Should reject custom ID token signup when the provider uses redirect auth', async () => {
+ manager = newManager({
+ identity: {
+ customProviders: [
+ {
+ kind: 'custom-oidc',
+ authMethod: 'authcode',
+ issuer: 'https://issuer.example.com',
+ oauthUrl: 'https://issuer.example.com/oauth/authorize',
+ clientId: 'test-custom-client-id',
+ },
+ ],
+ },
+ })
+
+ await expect(
</code_context>
<issue_to_address>
**suggestion (testing):** Add a positive-path test for custom ID token providers to complement the rejection test
To fully exercise the new behavior, please also add a positive-path test where a custom provider is configured with `authMethod: 'id-token'` and `signUp({ kind: 'custom-...', idToken })` succeeds, asserting that the wallet is created, the signer kind matches the custom provider, and metadata such as `loginEmail` is propagated correctly.
Suggested implementation:
```typescript
it('Should allow custom ID token signup when the provider uses id-token auth', async () => {
manager = newManager({
identity: {
customProviders: [
{
kind: 'custom-oidc',
authMethod: 'id-token',
issuer: 'https://issuer.example.com',
oauthUrl: 'https://issuer.example.com/oauth/authorize',
clientId: 'test-custom-client-id',
},
],
},
})
const wallet = await manager.wallets.signUp({
kind: 'custom-oidc',
idToken: 'eyJhbGciOiJub25lIn0.eyJlbWFpbCI6ImN1c3RvbS11c2VyQGV4YW1wbGUuY29tIn0.',
noGuard: true,
})
expect(wallet).toBeDefined()
expect(wallet.signer.kind).toBe('custom-oidc')
// Depending on the wallet shape, adjust this assertion to where loginEmail is stored
expect(
// prefer loginEmail if present, otherwise fall back to a generic email field
(wallet as any).metadata?.loginEmail ?? (wallet as any).metadata?.email,
).toBe('custom-user@example.com')
})
it('Should reject apple-id-token signup when Apple is configured for redirect auth', async () => {
manager = newManager({
```
You may need to adjust a few details to match the existing test helpers and types:
1. If the `kind` used for custom providers in signUp is namespaced (e.g. `'custom-oidc:issuer.example.com'` or similar), update the `kind: 'custom-oidc'` argument to whatever value the other custom-provider tests in this file use.
2. The ID token here is a simple, unsigned JWT-like string with an email claim; if your test utilities already expose a helper for generating ID tokens (e.g. `makeIdToken({ email: ... })`), replace the hard-coded string with that helper.
3. Update the expectations to match the actual wallet shape:
- If `wallet.signer` is nested differently (e.g. `wallet.signer.kind` vs `wallet.signer.type` or `wallet.signerInfo.kind`), adjust the assertion accordingly.
- If `loginEmail` is stored somewhere else (e.g. `wallet.loginEmail`, `wallet.profile.email`, or `wallet.metadata.identity.loginEmail`), change the metadata assertion to point to the correct path and drop the `(wallet as any)` cast once you know the correct type.
4. If the test suite uses typed helpers like `createTestManager()` instead of `newManager`, ensure the new test uses the same helper for consistency.
</issue_to_address>
### Comment 3
<location path="packages/wallet/wdk/test/wallets.test.ts" line_range="116-30" />
<code_context>
+ expect(configuration.login[0]!.kind).toBe(Kinds.LoginApple)
+ })
+
+ it('Should register and unregister Google ID token UI callbacks through the manager', async () => {
+ manager = newManager({
+ identity: {
+ google: {
+ enabled: true,
+ clientId: 'test-google-client-id',
+ authMethod: 'id-token',
+ },
+ },
+ })
+
+ const handler = (manager as any).shared.handlers.get(Kinds.LoginGoogle) as IdTokenHandler
</code_context>
<issue_to_address>
**suggestion (testing):** Extend `registerIdTokenUI` tests to cover multiple ID token handlers (Apple/custom) and cleanup
Currently this only verifies wiring for a single Google handler. Since `registerIdTokenUI` iterates over all handlers, please add a test with multiple providers enabled (e.g., Google + Apple, and optionally a custom provider) to assert that:
- each relevant `IdTokenHandler` receives the same callback, and
- the returned `unregister` clears `onPromptIdToken` on all of them.
This will validate behavior in multi-provider setups.
Suggested implementation:
```typescript
manager = newManager({
identity: {
google: {
enabled: true,
clientId: 'test-google-client-id',
authMethod: 'id-token',
},
apple: {
enabled: true,
clientId: 'test-apple-client-id',
authMethod: 'id-token',
},
},
})
const googleHandler = (manager as any).shared.handlers.get(Kinds.LoginGoogle) as IdTokenHandler
const appleHandler = (manager as any).shared.handlers.get(Kinds.LoginApple) as IdTokenHandler
expect(googleHandler).toBeDefined()
expect(appleHandler).toBeDefined()
const callback = vi.fn()
const unregister = manager.registerIdTokenUI(callback)
expect(googleHandler.onPromptIdToken).toBe(callback)
expect(appleHandler.onPromptIdToken).toBe(callback)
unregister()
expect(googleHandler.onPromptIdToken).toBeUndefined()
expect(appleHandler.onPromptIdToken).toBeUndefined()
```
If your codebase supports additional custom ID token providers with their own `Kinds.*` enum entries and `IdTokenHandler`s, you can extend this test further by:
1. Enabling the custom provider in the `identity` configuration of `newManager`.
2. Fetching its handler from `(manager as any).shared.handlers`.
3. Adding it to the shared expectations that `onPromptIdToken` is set to the same callback and cleared by `unregister`.
</issue_to_address>
### Comment 4
<location path="packages/wallet/wdk/src/sequence/handlers/idtoken.ts" line_range="125" />
<code_context>
+ }
+ }
+
+ private handleAuth(
+ onPromptIdToken: PromptIdTokenHandler,
+ ): Promise<{ signer: Signers.Signer & Signers.Witnessable; email: string }> {
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring `handleAuth` to delegate the callback-to-promise wiring to a small helper so the method itself can be a simple async function with a single, linear error path.
The main complexity comes from `handleAuth` using an async promise executor with nested try/catch. You can keep the existing `PromptIdTokenHandler` contract and move the awkward promise wiring into a small helper, then make `handleAuth` a straightforward `async` function with a single error path.
For example:
```ts
// Keep existing type, just allow non-async respond as well if you like.
type RespondFn = (idToken: string) => void | Promise<void>
export type PromptIdTokenHandler = (
kind: 'google-id-token' | 'apple-id-token' | `custom-${string}`,
respond: RespondFn,
) => Promise<void>
// Small helper to isolate the callback → Promise conversion
private waitForIdToken(onPromptIdToken: PromptIdTokenHandler): Promise<string> {
return new Promise<string>((resolve, reject) => {
const respond: RespondFn = async (idToken) => {
// No try/catch here: if this throws, the caller of `respond` sees it
resolve(idToken)
}
// Capture errors from the UI prompt itself
onPromptIdToken(this.signupKind, respond).catch(reject)
})
}
// `handleAuth` becomes linear and easy to reason about
private async handleAuth(
onPromptIdToken: PromptIdTokenHandler,
): Promise<{ signer: Signers.Signer & Signers.Witnessable; email: string }> {
const idToken = await this.waitForIdToken(onPromptIdToken)
const [signer, metadata] = await this.completeAuth(idToken)
return { signer, email: metadata.email || '' }
}
```
This keeps:
- The same external `PromptIdTokenHandler`/`RespondFn` signature.
- All current behavior (UI still calls `onPromptIdToken(kind, respond)` and awaits its own promise).
But it:
- Removes the async executor + nested try/catch from `handleAuth`.
- Centralizes the “callback → Promise” logic and error propagation in one small helper.
- Makes `handleAuth` and callers like `status` much easier to follow.
</issue_to_address>
### Comment 5
<location path="packages/wallet/wdk/test/wallets.test.ts" line_range="98" />
<code_context>
eyJhbGciOiJub25lIn0.eyJleHAiOjQxMDI0NDQ4MDB9.
</code_context>
<issue_to_address>
**security (jwt):** Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
*Source: gitleaks*
</issue_to_address>
### Comment 6
<location path="packages/wallet/wdk/test/wallets.test.ts" line_range="103" />
<code_context>
eyJhbGciOiJub25lIn0.eyJleHAiOjQxMDI0NDQ4MDB9.
</code_context>
<issue_to_address>
**security (jwt):** Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
*Source: gitleaks*
</issue_to_address>
### Comment 7
<location path="packages/wallet/wdk/test/wallets.test.ts" line_range="198" />
<code_context>
eyJhbGciOiJub25lIn0.eyJleHAiOjQxMDI0NDQ4MDB9.
</code_context>
<issue_to_address>
**security (jwt):** Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
*Source: gitleaks*
</issue_to_address>
### Comment 8
<location path="packages/wallet/wdk/test/wallets.test.ts" line_range="56" />
<code_context>
eyJhbGciOiJub25lIn0.eyJleHAiOjQxMDI0NDQ4MDB9.
</code_context>
<issue_to_address>
**security (jwt):** Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
*Source: gitleaks*
</issue_to_address>
### Comment 9
<location path="packages/wallet/wdk/test/wallets.test.ts" line_range="61" />
<code_context>
eyJhbGciOiJub25lIn0.eyJleHAiOjQxMDI0NDQ4MDB9.
</code_context>
<issue_to_address>
**security (jwt):** Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
*Source: gitleaks*
</issue_to_address>
### Comment 10
<location path="packages/wallet/wdk/test/wallets.test.ts" line_range="137" />
<code_context>
eyJhbGciOiJub25lIn0.eyJleHAiOjQxMDI0NDQ4MDB9.
</code_context>
<issue_to_address>
**security (jwt):** Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
*Source: gitleaks*
</issue_to_address>
### Comment 11
<location path="packages/wallet/wdk/test/wallets.test.ts" line_range="161" />
<code_context>
eyJhbGciOiJub25lIn0.eyJleHAiOjQxMDI0NDQ4MDB9.
</code_context>
<issue_to_address>
**security (jwt):** Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
*Source: gitleaks*
</issue_to_address>
### Comment 12
<location path="packages/wallet/wdk/test/idtoken.test.ts" line_range="149" />
<code_context>
eyJhbGciOiJub25lIn0.eyJleHAiOjQxMDI0NDQ4MDB9.
</code_context>
<issue_to_address>
**security (jwt):** Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
*Source: gitleaks*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for ID token-based login across the wallet stack, normalizes Google auth handlers, and updates package versions. The changes include modifications to several packages, adding a new IdTokenHandler, and updating the manager.ts to handle different authentication flows. The code changes look good, but there is an opportunity to improve the error message in packages/services/guard/src/sequence.ts.
Owner
Author
|
@Mergifyio refresh |
1 similar comment
Owner
Author
|
@Mergifyio refresh |
☑️ Command
|
✅ Pull request refreshed |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Add id-token based login support across the wallet stack and normalize Google auth handlers to a canonical signer kind while preserving redirect flows as default.
New Features:
Enhancements:
Build:
Documentation:
Tests: