feat: OAuth2 PKCE auth mode + provider abstraction (env-compatible) #46
Conversation
WalkthroughAdds pluggable, multi-mode Twitter authentication (env OAuth1, OAuth2 PKCE interactive, broker stub), token stores, PKCE helpers and interactive loopback prompt, refactors client to lazy async initialization, updates environment schema, docs, and tests to support mode-based auth and provider-driven flows. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Client App
participant Base as Plugin Base
participant Factory as Auth Factory
participant Provider as Auth Provider
participant Store as Token Store
participant Twitter as Twitter API
App->>Base: Initialize plugin
Base->>Factory: createTwitterAuthProvider(runtime,state)
Factory->>Factory: determine mode (env|oauth|broker)
alt env
Factory->>Provider: instantiate EnvAuthProvider
else oauth
Factory->>Provider: instantiate OAuth2PKCEAuthProvider
else broker
Factory->>Provider: instantiate BrokerAuthProvider
end
Base->>Provider: authenticate(provider)
alt OAuth2 PKCE
Provider->>Store: load()
alt tokens valid
Store-->>Provider: tokens
else
Provider->>App: open/print authorize URL
App->>Twitter: user authorizes -> callback
Provider->>Twitter: POST /oauth2/token (exchange)
Twitter-->>Provider: access_token (+ refresh_token)
Provider->>Store: save(tokens)
end
else env/broker
Provider->>Provider: getAccessToken()
end
Provider-->>Base: authenticated
Base->>Twitter: API calls using provider token
Twitter-->>Base: responses
sequenceDiagram
autonumber
participant Caller as Code
participant Auth as TwitterAuth
participant Lazy as Lazy Init
participant TwitterAPI as TwitterApi
Caller->>Auth: getV2Client()
Auth->>Lazy: is initialized?
alt not initialized
Lazy->>Auth: ensureClientInitialized()
Auth->>Auth: provider.getAccessToken()
Auth->>TwitterAPI: new TwitterApi(credentials)
TwitterAPI-->>Auth: instance
Auth->>Auth: cache instance
else initialized
Note over Lazy: return cached instance
end
Auth-->>Caller: Promise<TwitterApi>
Caller->>TwitterAPI: await and use client
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to data retention organization setting 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (12)
src/services/MessageService.ts (1)
123-123: Optional: Strengthen type safety on the fallback.The fallback
(result?.id as any)uses a type assertion that bypasses type checking. While the current implementation is functional, consider typing the result parameter or using a more specific type guard if the shape ofresultis known.Alternative with explicit undefined handling
- id: (await this.extractResultId(result)) || (result?.id as any), + id: (await this.extractResultId(result)) ?? (result?.id as string | undefined) ?? "",Note: This is only beneficial if you want to be more explicit about the fallback chain and avoid the
anytype.src/client/__tests__/broker-provider.test.ts (1)
4-15: Consider adding a test for the "broker not implemented" path.The current test validates the missing URL case. For completeness, consider adding a test that verifies the "not implemented" error when
TWITTER_BROKER_URLis provided:it("throws not implemented error when TWITTER_BROKER_URL is set", async () => { const runtime: any = { getSetting: vi.fn(() => "https://broker.example.com"), }; const provider = new BrokerAuthProvider(runtime); await expect(provider.getAccessToken()).rejects.toThrow( "broker auth is not implemented yet", ); });src/client/auth.ts (1)
16-22: Use the type guard for consistency.The constructor duplicates the OAuth1 provider check using
(provider as any)cast, while a proper type guardisOAuth1Provideris defined at line 24. Consider reusing it for consistency:🔎 Proposed refactor
constructor(private readonly provider: TwitterAuthProvider) { - // Backward-compatible behavior: legacy OAuth1 provider is considered authenticated immediately, - // matching previous eager client initialization semantics. - if (typeof (provider as any).getOAuth1Credentials === "function") { + if (this.isOAuth1Provider(provider)) { this.authenticated = true; } }src/client/auth-providers/factory.ts (1)
28-29: Consider passingstatetoOAuth2PKCEAuthProviderfor consistency.
EnvAuthProviderreceives bothruntimeandstate(allowing state overrides for credentials), butOAuth2PKCEAuthProvideronly receivesruntime. If a caller providesTWITTER_CLIENT_IDvia thestateobject, it won't be respected in oauth mode.🔎 Suggested fix
case "oauth": - return new OAuth2PKCEAuthProvider(runtime); + return new OAuth2PKCEAuthProvider(runtime, undefined, undefined, undefined, state);This would require updating
OAuth2PKCEAuthProviderconstructor to accept and usestatesimilarly toEnvAuthProvider.src/client/tweets.ts (1)
1113-1130: Consider caching the authenticated user ID to avoid repeated API calls.Both
likeTweetandretweetcallv2client.v2.me()on every invocation to get the current user ID. This results in an extra API call per operation. Consider caching the user ID (perhaps on theTwitterAuthobject or via a memoization pattern) to reduce API overhead.🔎 Example approach
export async function likeTweet( tweetId: string, auth: TwitterAuth, ): Promise<void> { const v2client = await auth.getV2Client(); if (!v2client) { throw new Error("V2 client is not initialized"); } try { + const userId = await auth.getCurrentUserId(); // Cache this in auth await v2client.v2.like( - (await v2client.v2.me()).data.id, // Current user ID + userId, tweetId, ); } catch (error) { throw new Error(`Failed to like tweet: ${error.message}`); } }src/client/auth-providers/types.ts (1)
37-40:TwitterAuthProviderFactoryOptionsinterface appears unused.This interface is defined but not used by
createTwitterAuthProviderinfactory.ts, which takesruntimeandstateas separate parameters. Consider either using this interface or removing it to reduce dead code.#!/bin/bash # Check if TwitterAuthProviderFactoryOptions is used anywhere rg -n "TwitterAuthProviderFactoryOptions" --type=tssrc/client/auth-providers/oauth2-pkce.ts (2)
100-111: Consider improving JSON parse error handling.When
res.json()fails (line 106), it returns an empty object{}, which then gets serialized into the error message on line 109 as{}. This loses the actual response body. Consider reading the response as text first for better error diagnostics.🔎 Suggested approach
- const json = await res.json().catch(() => ({})); - if (!res.ok) { - throw new Error( - `Twitter token exchange failed (${res.status}): ${JSON.stringify(json)}`, - ); - } + const text = await res.text(); + let json: Record<string, unknown>; + try { + json = JSON.parse(text); + } catch { + if (!res.ok) { + throw new Error(`Twitter token exchange failed (${res.status}): ${text}`); + } + throw new Error(`Twitter token exchange returned invalid JSON: ${text.slice(0, 200)}`); + } + if (!res.ok) { + throw new Error( + `Twitter token exchange failed (${res.status}): ${JSON.stringify(json)}`, + ); + }
196-208: Add error handling for URL parsing in fallback flow.If the user pastes an invalid URL (line 200),
new URL(redirected)will throw, resulting in an unclear error message. Consider wrapping in try-catch with a user-friendly message.🔎 Suggested fix
if (!code) { const redirected = await promptForRedirectedUrl( "Paste the FULL redirected URL here (it contains ?code=...&state=...): ", ); - const parsed = new URL(redirected); + let parsed: URL; + try { + parsed = new URL(redirected); + } catch { + throw new Error("Invalid URL pasted. Please paste the complete redirect URL including https://"); + } const parsedCode = parsed.searchParams.get("code");src/client/__tests__/auth.test.ts (1)
48-53: Consider using async/await for consistency.This test uses
.then()chaining while other tests in the same file useasync/await. For consistency and readability, consider:- it("should return the Twitter API v2 client", () => { - return auth.getV2Client().then((client) => { - expect(client).toBe(mockTwitterApi); - }); + it("should return the Twitter API v2 client", async () => { + const client = await auth.getV2Client(); + expect(client).toBe(mockTwitterApi); });src/client/client.ts (1)
725-729: Silent error swallowing may hide misconfiguration.The
.catch(() => false)suppresses all errors fromisLoggedIn(). Since the stated goal is to "surface misconfiguration quickly," consider logging the error before swallowing it:public async authenticate(provider: TwitterAuthProvider): Promise<void> { this.auth = new TwitterAuth(provider); // Force initialization early to surface misconfiguration quickly - await this.auth.isLoggedIn().catch(() => false); + await this.auth.isLoggedIn().catch((err) => { + console.warn("Early auth validation failed:", err); + return false; + }); }Alternatively, if misconfiguration should be surfaced as an error, let exceptions propagate.
package.json (1)
86-110: Consider marking TWITTER_BROKER_URL as sensitive.While the broker URL itself isn't inherently secret, some broker implementations embed API keys or tokens in the URL path/query. Marking it
sensitive: truewould prevent accidental exposure in logs or UI."TWITTER_BROKER_URL": { "type": "string", "description": "Broker URL (required for TWITTER_AUTH_MODE=broker; stub only).", "required": false, - "sensitive": false + "sensitive": true },src/environment.ts (1)
124-132: Consider using schema parsing for mode validation.The manual normalization and
as anycast could be replaced with Zod's built-in enum handling, which would provide better type safety:- const rawMode = - (config as any).TWITTER_AUTH_MODE ?? - (getSetting(runtime, "TWITTER_AUTH_MODE") as any) ?? - "env"; - const normalizedMode = - typeof rawMode === "string" && rawMode.trim() ? rawMode.trim() : "env"; + const rawMode = ( + (config as any).TWITTER_AUTH_MODE ?? + getSetting(runtime, "TWITTER_AUTH_MODE") ?? + "env" + ).toString().trim().toLowerCase() || "env"; + const normalizedMode = ["env", "oauth", "broker"].includes(rawMode) ? rawMode : "env";This ensures case-insensitive matching and explicit validation before the final schema parse.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (32)
.gitignoreREADME.mdpackage.jsonsrc/__tests__/README.mdsrc/__tests__/TESTING_GUIDE.mdsrc/__tests__/e2e/twitter-integration.test.tssrc/__tests__/environment.test.tssrc/base.tssrc/client/__tests__/auth.test.tssrc/client/__tests__/broker-provider.test.tssrc/client/__tests__/oauth2-provider.test.tssrc/client/__tests__/pkce.test.tssrc/client/__tests__/token-store.test.tssrc/client/auth-providers/broker.tssrc/client/auth-providers/env.tssrc/client/auth-providers/factory.tssrc/client/auth-providers/interactive.tssrc/client/auth-providers/oauth2-pkce.tssrc/client/auth-providers/pkce.tssrc/client/auth-providers/token-store.tssrc/client/auth-providers/types.tssrc/client/auth.tssrc/client/client.tssrc/client/profile.tssrc/client/relationships.tssrc/client/search.tssrc/client/tweets.tssrc/environment.tssrc/index.tssrc/services/MessageService.tssrc/services/PostService.tstsup.config.ts
💤 Files with no reviewable changes (1)
- src/tests/e2e/twitter-integration.test.ts
🧰 Additional context used
🧬 Code graph analysis (16)
src/client/__tests__/oauth2-provider.test.ts (2)
src/client/auth-providers/token-store.ts (2)
TokenStore(15-19)StoredOAuth2Tokens(7-13)src/client/auth-providers/oauth2-pkce.ts (1)
OAuth2PKCEAuthProvider(34-242)
src/client/auth-providers/broker.ts (2)
src/client/auth-providers/types.ts (1)
TwitterAuthProvider(12-20)src/utils/settings.ts (1)
getSetting(10-25)
src/client/__tests__/pkce.test.ts (1)
src/client/auth-providers/pkce.ts (2)
base64UrlEncode(3-9)createCodeChallenge(16-19)
src/client/auth-providers/env.ts (2)
src/client/auth-providers/types.ts (2)
TwitterOAuth1Provider(33-35)OAuth1Credentials(22-27)src/utils/settings.ts (1)
getSetting(10-25)
src/client/auth-providers/oauth2-pkce.ts (5)
src/client/auth-providers/token-store.ts (3)
StoredOAuth2Tokens(7-13)TokenStore(15-19)chooseDefaultTokenStore(80-92)src/client/auth-providers/types.ts (1)
TwitterAuthProvider(12-20)src/utils/settings.ts (1)
getSetting(10-25)src/client/auth-providers/pkce.ts (3)
createCodeVerifier(11-14)createCodeChallenge(16-19)createState(21-23)src/client/auth-providers/interactive.ts (2)
waitForLoopbackCallback(39-118)promptForRedirectedUrl(15-37)
src/client/auth.ts (1)
src/client/auth-providers/types.ts (2)
TwitterAuthProvider(12-20)TwitterOAuth1Provider(33-35)
src/client/auth-providers/factory.ts (5)
src/client/auth-providers/types.ts (2)
TwitterAuthMode(3-3)TwitterAuthProvider(12-20)src/utils/settings.ts (1)
getSetting(10-25)src/client/auth-providers/env.ts (1)
EnvAuthProvider(14-58)src/client/auth-providers/oauth2-pkce.ts (1)
OAuth2PKCEAuthProvider(34-242)src/client/auth-providers/broker.ts (1)
BrokerAuthProvider(15-32)
src/client/auth-providers/interactive.ts (1)
src/client/auth-providers/oauth2-pkce.ts (1)
redirectUri(52-56)
src/client/client.ts (2)
src/client/auth-providers/types.ts (1)
TwitterAuthProvider(12-20)src/client/auth.ts (1)
TwitterAuth(8-154)
src/client/__tests__/auth.test.ts (1)
src/client/auth.ts (1)
TwitterAuth(8-154)
src/client/__tests__/token-store.test.ts (1)
src/client/auth-providers/token-store.ts (2)
FileTokenStore(45-78)RuntimeCacheTokenStore(21-43)
src/__tests__/environment.test.ts (1)
src/environment.ts (1)
validateTwitterConfig(119-320)
src/base.ts (2)
src/client/auth-providers/factory.ts (2)
getTwitterAuthMode(14-18)createTwitterAuthProvider(20-33)src/utils/settings.ts (1)
getSetting(10-25)
src/client/__tests__/broker-provider.test.ts (1)
src/client/auth-providers/broker.ts (1)
BrokerAuthProvider(15-32)
src/index.ts (2)
src/utils/settings.ts (1)
getSetting(10-25)src/client/auth-providers/oauth2-pkce.ts (2)
clientId(46-50)redirectUri(52-56)
src/environment.ts (1)
src/utils/settings.ts (1)
getSetting(10-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🔇 Additional comments (53)
src/services/MessageService.ts (2)
16-23: LGTM! Clean fallback chain for rest_id extraction.The nullish coalescing chain correctly handles multiple response shapes and returns undefined when rest_id is not found in any of the expected paths.
25-48: This review comment is based on an incorrect assumption about the result object type.The
resultobject returned fromcreateCreateTweetRequestis not a Fetch APIResponseobject. It's a custom object that mimics a Response shape by returning:{ ok: true, json: async () => result, data: result, }The
json()function is simplyasync () => result—it returns the already-parsed data, not a stream. Calling it multiple times causes no issues. Additionally, downstream code does not attempt to re-read the body from metadata, so there is no functional problem even if the assumption were correct.Likely an incorrect or invalid review comment.
src/services/PostService.ts (2)
15-22: LGTM: Robust rest_id extraction.The optional chaining and nullish coalescing pattern correctly handles varying API response shapes.
31-45: The Response consumption concern is not applicable to this codebase.The
sendTweet()method returns plain JavaScript objects (verified by test mocks and actual return values), not Response objects. The check forresult?.jsonon lines 31-45 is defensive code that would only execute if somehow a Response-like object were passed, which doesn't happen in practice. Additionally,extractTweetId()completes before the result is stored inmetadata.raw(line 94), so even ifjson()were called, it would not affect later use of the stored result.The empty catch block (lines 42-44) is acceptable given the multiple fallback ID extraction paths available above it.
Likely an incorrect or invalid review comment.
tsup.config.ts (1)
15-23: LGTM!The additions correctly externalize the
node:-prefixed built-ins used by the new OAuth PKCE and interactive authentication features.src/__tests__/TESTING_GUIDE.md (1)
5-10: LGTM!Documentation clearly explains the three authentication modes and their current status. The broker mode is appropriately marked as "stub only, not implemented yet."
.gitignore (1)
7-10: LGTM!Good defense-in-depth approach to prevent accidental commits of OAuth token files. The patterns align with the token-store implementation.
src/__tests__/README.md (1)
37-38: LGTM!Clear documentation distinguishing between the env mode (used by E2E tests) and the interactive oauth mode. This helps developers understand which authentication flow is being tested.
src/client/auth-providers/interactive.ts (2)
84-90: State validation may be too permissive.The current logic only rejects when
stateis present but mismatches. If Twitter returns a callback without astateparameter whileexpectedStatewas provided, the callback is accepted. This could weaken CSRF protection if the OAuth provider behavior varies.Verify whether Twitter's OAuth2 flow guarantees the state parameter is always returned when provided in the authorization request. If it does, consider rejecting callbacks missing the state:
- if (state && state !== expectedState) { + if (state !== expectedState) {
39-53: LGTM!Good validation of loopback-only redirect URIs and sensible default port selection to avoid privileged ports.
src/client/search.ts (2)
36-36: LGTM!Correctly updated to await the now-async
getV2Client()method, aligning with the provider-based initialization pattern.
144-144: LGTM!Consistent async client retrieval in
searchProfiles.src/client/auth.ts (3)
28-53: LGTM!Well-structured lazy initialization with proper handling of both OAuth1 and OAuth2 providers. The token comparison at line 47 correctly prevents unnecessary client recreation when the token hasn't changed.
58-64: LGTM!The async conversion of
getV2Client()properly awaits initialization before returning the client.
140-146: LGTM!The logout implementation correctly clears all state including the new
lastAccessTokenand setsloggedOutto prevent reuse after logout.src/client/__tests__/pkce.test.ts (1)
1-19: LGTM!The PKCE helper tests are well-structured and correctly validate:
- URL-safe base64 encoding (no
+,/, or=)- Code challenge generation against the RFC 7636 test vector
src/client/profile.ts (2)
231-231: LGTM!Correctly awaiting the V2 client initialization, consistent with the async authentication provider migration.
284-284: LGTM!Correctly awaiting the V2 client initialization, consistent with the async authentication provider migration.
src/client/__tests__/oauth2-provider.test.ts (1)
1-202: LGTM!Comprehensive test coverage for OAuth2PKCEAuthProvider including:
- Token retrieval and refresh flows
- Error handling for failed refresh/exchange
- Token rotation and persistence
- Reauthentication when refresh token unavailable
The test structure is clean, mocking is appropriate, and assertions verify expected behavior.
src/client/relationships.ts (1)
49-49: LGTM!All five functions correctly await the V2 client initialization, consistent with the async authentication provider migration. Error handling is properly maintained in each function's try-catch block.
Also applies to: 111-111, 175-175, 230-230, 281-281
README.md (1)
11-40: LGTM!Excellent documentation updates that clearly explain:
- Dual authentication modes (OAuth 1.0a vs OAuth 2.0 PKCE)
- Mode-specific credential requirements
- Step-by-step setup for each mode
- Troubleshooting guidance for mode-specific issues
The documentation correctly notes that OAuth 2.0 PKCE doesn't require a client secret in the environment, which aligns with the PKCE security model.
Also applies to: 95-169, 385-396
src/client/__tests__/token-store.test.ts (1)
1-72: LGTM!Well-structured test suite covering:
- FileTokenStore: save/load roundtrip, clear functionality, and corrupted JSON handling
- RuntimeCacheTokenStore: integration with runtime cache
Tests use appropriate isolation (tmpdir for files) and proper cleanup. Mock setup is clean and effective.
src/__tests__/environment.test.ts (3)
25-28: LGTM!Properly added environment variable stubs for the new authentication mode configuration variables.
93-96: LGTM!Updated error message assertion to match the new mode-specific error messaging ("Twitter env auth is selected").
98-140: LGTM!Excellent test coverage for the new authentication modes:
- OAuth mode validation with required fields (CLIENT_ID, REDIRECT_URI)
- OAuth mode failure when credentials are missing
- Broker mode failure when BROKER_URL is missing
Tests follow the existing pattern and properly validate mode-specific requirements.
src/client/auth-providers/broker.ts (1)
1-33: LGTM!Clean broker provider scaffolding that:
- Implements the TwitterAuthProvider interface
- Provides clear error messages for missing configuration and not-yet-implemented functionality
- Documents the intended future contract (broker handles secrets, returns short-lived tokens)
The stub implementation appropriately blocks usage until the broker is implemented, while preserving the provider abstraction.
src/client/auth-providers/pkce.ts (1)
1-23: LGTM! Correct RFC 7636 PKCE implementation.The PKCE utilities correctly implement the specification: secure random generation via
crypto.randomBytes, proper base64url encoding, and SHA-256 for the code challenge. The default byte lengths (32 for verifier → 43 chars, 16 for state) are appropriate.src/client/auth-providers/factory.ts (1)
8-12: LGTM! Clean mode normalization with proper validation.The mode normalization correctly handles case-insensitivity, defaults to "env" for backward compatibility, and provides a clear error message for invalid modes.
src/index.ts (1)
16-63: LGTM! Comprehensive mode-based configuration validation.The initialization properly validates credentials for each auth mode and provides clear logging for users. Warnings for missing credentials (rather than hard errors) allow the plugin to load with limited functionality, which is appropriate for a plugin init phase.
src/client/tweets.ts (1)
298-345: LGTM! Proper async client retrieval with pagination handling.The migration to async
getV2Client()is correctly implemented. The pagination loop with early break whenmaxTweetsis reached is appropriate.src/client/auth-providers/env.ts (1)
14-58: LGTM! Well-structured OAuth 1.0a provider with proper validation.The credential lookup order (state → runtime → env) provides flexibility, and the error handling clearly lists all missing credentials. The implementation correctly satisfies the
TwitterOAuth1Providerinterface.src/base.ts (2)
261-277: Good mode-aware client reuse, but consider adding a fallback key.The reuse key selection based on mode is correct. However, if the corresponding setting is missing (e.g.,
TWITTER_API_KEYfor env mode),reuseKeywill be undefined and client caching will be skipped. This is likely intentional (the auth will fail later anyway), but a debug log here might help troubleshooting.
280-320: LGTM! Robust authentication with exponential backoff.The provider-based authentication with configurable retries and exponential backoff is well-implemented. Error messages are descriptive and include the last error for debugging.
src/client/auth-providers/types.ts (1)
1-35: LGTM! Well-designed provider abstraction.The type hierarchy cleanly separates the common
TwitterAuthProviderinterface from OAuth1-specific concerns viaTwitterOAuth1Provider. The documentation clearly explains the purpose of each interface.src/client/auth-providers/oauth2-pkce.ts (2)
213-241: LGTM! Well-structured token lifecycle management.The
getAccessTokenmethod correctly handles all token states: cached valid tokens, expired tokens with refresh capability, and full re-authentication when needed. TheinteractiveLoginFninjection enables clean testing.
34-44: Good use of dependency injection for testability.The constructor accepts injectable dependencies (
tokenStore,fetchImpl,interactiveLoginFn) with sensible defaults, enabling comprehensive unit testing without network calls or interactive prompts.src/client/__tests__/auth.test.ts (4)
29-39: Test setup correctly reflects new provider-based constructor.The mock provider includes both
getAccessTokenandgetOAuth1Credentials, which matches the backward-compatible OAuth1 path inTwitterAuth. Theas anycast is acceptable here since tests intentionally create a hybrid mock that satisfies both provider types.
41-45: LGTM!Correctly validates lazy initialization—the
TwitterApiconstructor should not be invoked until the first client access.
207-214: LGTM!Correctly tests that
getV2Client()rejects with the expected error message after logout, matching the new Promise-based error path.
217-226: LGTM!The
hasToken()tests correctly verify the synchronous state check. The first test passes because the OAuth1 provider setsauthenticated = trueeagerly in the constructor.src/client/client.ts (4)
16-16: LGTM!Import aligns with the new provider-based authentication architecture.
303-307: LGTM!Properly awaits the async
getV2Client()call after the auth check.
366-370: LGTM!Consistent async client retrieval pattern.
788-798: LGTM!The inline OAuth1 provider construction is correct. The non-null assertions are safe because the guard at lines 782-786 ensures all credentials are present. The
as anycast is acceptable since this creates a backward-compatible hybrid provider.package.json (2)
55-61: LGTM!The
TWITTER_AUTH_MODEparameter is well-documented with clear mode descriptions and sensible default of"env"for backward compatibility.
62-85: LGTM!Credential parameters correctly updated to be conditionally required with clear descriptions indicating which auth mode requires each credential.
src/client/auth-providers/token-store.ts (3)
7-19: LGTM!Clean interface definitions.
StoredOAuth2Tokenscaptures all necessary OAuth2 token fields, andTokenStoreprovides a minimal async API for persistence.
53-64: LGTM!Good defensive validation of loaded tokens. The checks for required fields (
access_tokenas string,expires_atas number) prevent corrupt or malformed data from causing runtime errors.
80-92: LGTM!Good capability detection and graceful fallback. The warning appropriately alerts users about the security implications of file-based token storage.
src/environment.ts (4)
11-28: LGTM!Schema correctly defines the new auth mode enum and OAuth2/broker configuration fields with sensible defaults. The
offline.accessscope inTWITTER_SCOPESdefault enables refresh token support.
275-303: LGTM!Mode-based credential validation is thorough with clear, actionable error messages. The lowercase comparison at line 275 ensures case-insensitive mode matching.
307-317: LGTM!Robust error handling that accommodates different Zod versions (
issuesvserrors) and provides clear aggregated error messages.
359-390: LGTM!Default configuration correctly includes all new auth mode fields with values matching the schema defaults.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional Comments (10)
-
src/client/auth-providers/token-store.ts, line 41 (link)style: casting
null as anyto bypass type checking may cause issues if the cache implementation expects specific types for deletion. Would usingundefinedor a specific cache deletion method be more appropriate here?Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
-
src/client/auth-providers/factory.ts, line 28-29 (link)style: OAuth provider doesn't receive state parameter while env provider does. Is there a reason OAuth2PKCEAuthProvider doesn't need the state parameter that EnvAuthProvider uses?
-
src/client/client.ts, line 788-799 (link)style: The
as anytype assertion bypasses type safety. Consider defining a proper type for the inline provider object or using a factory function.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
-
src/client/auth-providers/oauth2-pkce.ts, line 188 (link)style: Consider validating that the redirect URI is actually a loopback address before attempting to start the callback server
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
-
src/client/auth-providers/oauth2-pkce.ts, line 204 (link)logic: State validation only occurs when parsedState is truthy, but should validate that state parameter is present and matches
Should the validation require the state parameter to be present in the redirect URL?
-
src/__tests__/e2e/twitter-integration.test.ts, line 22-26 (link)style: The skip condition should ideally check for TWITTER_AUTH_MODE=env or absence of the mode, since other auth modes won't use these environment variables. Should the skip condition be updated to account for different auth modes, or is it intentional to only run E2E tests with env-based auth?
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
-
src/client/auth-providers/interactive.ts, line 78-82 (link)logic: Missing server cleanup when code is not present - server remains open
-
src/client/__tests__/auth.test.ts, line 38 (link)style: Using
as anyhere bypasses TypeScript checking. Consider creating a proper test mock that implements the provider interface.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
-
src/client/auth.ts, line 19 (link)style: Type casting to
anybypasses TypeScript safety. Consider adding a proper type guard or extending the TwitterAuthProvider interface to include an optionalgetOAuth1Credentialsmethod.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
-
src/client/auth.ts, line 72-73 (link)style: Silent error swallowing may mask real authentication issues. Consider logging the caught error for debugging purposes.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
32 files reviewed, 10 comments
|
Addressed CodeRabbit findings:
Verified: bun run build + bun run test:coverage |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (4)
src/client/__tests__/token-store.test.tssrc/client/auth-providers/interactive.tssrc/client/auth-providers/token-store.tssrc/services/PostService.ts
✅ Files skipped from review due to trivial changes (1)
- src/client/tests/token-store.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client/auth-providers/token-store.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/client/auth-providers/interactive.ts (1)
src/client/auth-providers/oauth2-pkce.ts (1)
redirectUri(52-56)
🔇 Additional comments (4)
src/client/auth-providers/interactive.ts (3)
1-9: LGTM!Imports and interface structure are appropriate for OAuth callback handling. The
OAuthCallbackResultinterface correctly captures the authorization code and optional state parameter.
11-37: LGTM!The TTY detection and readline-based prompting are correctly implemented. The
finallyblock ensures proper cleanup of the readline interface, and the error message provides clear guidance when interactive setup isn't possible.
92-97: Fix confirmed: Missing code handling now properly closes server and rejects promise.The previous review concern has been addressed. The code now calls
finish(new Error("Missing code"))which ensures the server is closed and the promise is rejected, preventing resource leaks.src/services/PostService.ts (1)
66-80: Well-handled error case—addresses past review feedback.The fail-fast behavior properly addresses the previous concern about timestamp fallbacks. The implementation includes:
- Detailed error logging with sanitized output (8000 char limit prevents log overflow)
- Useful context (inReplyTo, textLength) for debugging
- Clear error message directing developers to logs
|
More CodeRabbit hardening:
Verified: bun run build + bun run test:coverage |
2 participants
Summary
Adds 3-legged "login + approve" Twitter/X auth (OAuth 2.0 Authorization Code + PKCE) to @elizaos/plugin-twitter without shipping any client secret, while keeping full backward compatibility with the existing env-var OAuth 1.0a credentials. Also adds a broker-ready auth mode stub (not functional yet) so a future Eliza-hosted broker can be added without touching tweet logic.
This requires a “Twitter broker” I referenced below which is a future Eliza-hosted service (not implemented in this repo) that would sit between the plugin and Twitter to handle the parts that require a client secret and a user session.
What it is
A separate web service (owned by ElizaOS) that:
What changed in this PR (exact)
Auth modes
1) Legacy env mode (no behavior change)
Set:
2) OAuth2 PKCE mode ("login + approve")
Set:
Optional:
First run prints an authorization URL. After approval it stores tokens and auto-refreshes.
Requires a
Local callback server (required for best UX)
In oauth mode, the plugin starts a small local HTTP server bound to 127.0.0.1 to capture the OAuth redirect at the host/port/path in TWITTER_REDIRECT_URI.
Broker mode (stub; requires Eliza Twitter broker server)
If TWITTER_AUTH_MODE=broker:
Validation
Greptile Summary
Important Files Changed
src/client/auth-providers/oauth2-pkce.tssrc/client/auth.tsgetV2Client()async, added lazy initialization and token refresh detectionsrc/environment.tsTWITTER_AUTH_MODEconfiguration with mode-specific validation for env/oauth/broker authenticationsrc/client/tweets.tssrc/index.tsConfidence score: 4/5
tweets.ts,relationships.ts,profile.ts, andsearch.tsto ensure all callers handle Promise rejections properlySequence Diagram
sequenceDiagram participant User participant Plugin as TwitterPlugin participant Provider as OAuth2PKCEAuthProvider participant TwitterAPI as Twitter/X API participant TokenStore as Token Store participant Browser User->>Plugin: "Initialize with TWITTER_AUTH_MODE=oauth" Plugin->>Provider: "createTwitterAuthProvider(runtime, state)" Provider->>TokenStore: "load()" TokenStore-->>Provider: "null (no existing tokens)" User->>Plugin: "Perform Twitter action (post tweet, etc.)" Plugin->>Provider: "getAccessToken()" Provider->>Provider: "interactiveLogin()" Provider->>Provider: "createCodeVerifier() & createCodeChallenge()" Provider->>Provider: "buildAuthorizeUrl(state, codeChallenge)" Provider->>User: "Print authorization URL" User->>Browser: "Open authorization URL" Browser->>TwitterAPI: "GET /oauth2/authorize with PKCE params" TwitterAPI-->>Browser: "User consent page" User->>Browser: "Approve application" Browser->>Provider: "Redirect with authorization code" Provider->>TwitterAPI: "POST /oauth2/token with code & verifier" TwitterAPI-->>Provider: "access_token + refresh_token + expires_in" Provider->>TokenStore: "save(tokens)" Provider-->>Plugin: "access_token" Plugin->>TwitterAPI: "API call with Bearer token" TwitterAPI-->>Plugin: "API response" Plugin-->>User: "Success"Summary by CodeRabbit
New Features
Documentation
Configuration
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.