-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat: add OAuth authentication support for ChatGPT Plus/Pro accounts #6995
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Add OAuth types and configuration for ChatGPT authentication - Implement PKCE and state validation helpers for secure OAuth flow - Create OAuth callback server to handle authorization codes - Add token exchange functionality to get API keys from OAuth tokens - Implement SecretStorage manager for ChatGPT credentials - Update OpenAI provider to support chatgpt auth mode - Add main authentication manager to orchestrate the OAuth flow This allows users with ChatGPT Plus/Pro subscriptions to authenticate using their existing accounts instead of requiring separate API billing. Compatible with Codex CLI authentication flow. Fixes #6993
| ` | ||
|
|
||
| res.writeHead(400, { "Content-Type": "text/html" }) | ||
| res.end(html) |
Check failure
Code scanning / CodeQL
Reflected cross-site scripting High
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 months ago
To fix this vulnerability, we must ensure that any user-controlled data interpolated into the HTML response is properly escaped for HTML context. The best way to do this in Node.js/TypeScript is to use a well-known library such as escape-html to encode the error message before inserting it into the HTML. This prevents any HTML tags or scripts from being executed in the browser. The fix should be applied in the sendErrorResponse method in src/core/auth/oauth-server.ts, by escaping errorMessage before it is interpolated into the HTML. We will need to import the escape-html library at the top of the file.
-
Copy modified line R7 -
Copy modified line R171 -
Copy modified line R234
| @@ -4,6 +4,7 @@ | ||
| import axios from "axios" | ||
| import { CHATGPT_OAUTH_CONFIG, type OAuthTokens, type OAuthError } from "@roo-code/types" | ||
| import { validateOAuthState, formatOAuthError } from "./oauth-helpers" | ||
| import escapeHtml from "escape-html" | ||
|
|
||
| /** | ||
| * OAuth callback server for handling the authorization code | ||
| @@ -167,6 +168,7 @@ | ||
| * Send error response to browser | ||
| */ | ||
| private sendErrorResponse(res: http.ServerResponse, errorMessage: string): void { | ||
| const safeErrorMessage = escapeHtml(errorMessage) | ||
| const html = ` | ||
| <!DOCTYPE html> | ||
| <html> | ||
| @@ -229,7 +231,7 @@ | ||
| <div class="error-icon"></div> | ||
| <h1>Authentication Failed</h1> | ||
| <p>There was an error during authentication. Please try again.</p> | ||
| <div class="error-message">${errorMessage}</div> | ||
| <div class="error-message">${safeErrorMessage}</div> | ||
| </div> | ||
| </body> | ||
| </html> |
-
Copy modified lines R484-R485
| @@ -481,7 +481,8 @@ | ||
| "web-tree-sitter": "^0.25.6", | ||
| "workerpool": "^9.2.0", | ||
| "yaml": "^2.8.0", | ||
| "zod": "^3.25.61" | ||
| "zod": "^3.25.61", | ||
| "escape-html": "^1.0.3" | ||
| }, | ||
| "devDependencies": { | ||
| "@roo-code/build": "workspace:^", |
| Package | Version | Security advisories |
| escape-html (npm) | 1.0.3 | None |
| }) | ||
| } else { | ||
| // Initialize immediately for regular API key mode | ||
| this.initializeClient(this.options.openAiApiKey ?? "not-provided") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When not using ChatGPT auth mode, the API key defaults to 'not-provided'. Consider explicit error handling instead of using a fallback string.
|
|
||
| const response = await axios.post( | ||
| CHATGPT_OAUTH_CONFIG.tokenUrl, | ||
| new URLSearchParams(tokenExchangeRequest as any), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Casting tokenExchangeRequest to 'any' reduces type safety. Consider refactoring to properly construct URLSearchParams without using 'as any'.
| new URLSearchParams(tokenExchangeRequest as any), | |
| new URLSearchParams(Object.entries(tokenExchangeRequest).map(([k, v]) => [k, String(v)])), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewing my own code because apparently I trust no one, not even myself.
| <div class="error-icon"></div> | ||
| <h1>Authentication Failed</h1> | ||
| <p>There was an error during authentication. Please try again.</p> | ||
| <div class="error-message">${errorMessage}</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚨 Critical Security Issue: This is an XSS vulnerability waiting to happen. The error message is directly interpolated into HTML without escaping. If the error message contains user-controlled data or malicious content, it could execute arbitrary JavaScript.
| <div class="error-message">${errorMessage}</div> | |
| <div class="error-message">${this.escapeHtml(errorMessage)}</div> |
You'll need to add an escapeHtml helper method:
private escapeHtml(text: string): string {
const map: Record<string, string> = {
'&': '&',
'<': '<',
'>': '>',
'"': '"',
"'": '''
};
return text.replace(/[&<>"']/g, m => map[m]);
}|
|
||
| // Initialize the client asynchronously if using ChatGPT auth | ||
| if (this.options.openAiAuthMode === "chatgpt") { | ||
| this.apiKeyPromise = this.getApiKeyFromChatGpt() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this race condition intentional? The async initialization could cause issues if multiple requests come in before the API key is fetched. Consider using a more robust initialization pattern:
| this.apiKeyPromise = this.getApiKeyFromChatGpt() | |
| private clientPromise: Promise<OpenAI> | |
| private client?: OpenAI | |
| constructor(options: ApiHandlerOptions) { | |
| super() | |
| this.options = options | |
| this.clientPromise = this.initializeClientAsync() | |
| } | |
| private async ensureClient(): Promise<OpenAI> { | |
| if (!this.client) { | |
| this.client = await this.clientPromise | |
| } | |
| return this.client | |
| } |
Then use await this.ensureClient() in your methods instead of checking this.apiKeyPromise.
| /** | ||
| * Refresh credentials if needed | ||
| */ | ||
| async refreshCredentials(): Promise<boolean> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The token refresh logic lacks retry mechanisms. What happens if the refresh fails due to a temporary network issue? Consider implementing exponential backoff:
| async refreshCredentials(): Promise<boolean> { | |
| async refreshCredentials(retries = 3): Promise<boolean> { | |
| for (let i = 0; i < retries; i++) { | |
| try { | |
| // ... existing refresh logic | |
| return true | |
| } catch (error: any) { | |
| if (i === retries - 1) { | |
| console.error("Failed to refresh credentials after retries:", error) | |
| return false | |
| } | |
| await new Promise(resolve => setTimeout(resolve, Math.pow(2, i) * 1000)) | |
| } | |
| } | |
| return false | |
| } |
| * OAuth configuration for ChatGPT authentication | ||
| */ | ||
| export const CHATGPT_OAUTH_CONFIG = { | ||
| clientId: "app_EMoamEEZ73f0CkXaXp7hrann", // Codex CLI client ID for compatibility |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using Codex CLI's client ID for compatibility is clever, but what's the fallback plan if OpenAI revokes this ID? Should we consider making this configurable or having a backup client ID?
|
|
||
| // Check if state is not expired (5 minutes timeout) | ||
| const now = Date.now() | ||
| if (now - storedState.timestamp > 5 * 60 * 1000) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is 5 minutes enough buffer time? For users on slow networks or if the OAuth provider is experiencing delays, this might timeout prematurely. Consider increasing to 10-15 minutes:
| if (now - storedState.timestamp > 5 * 60 * 1000) { | |
| // Check if state is not expired (15 minutes timeout) | |
| const now = Date.now() | |
| if (now - storedState.timestamp > 15 * 60 * 1000) { |
| const authJson = JSON.parse(input) | ||
|
|
||
| // Validate structure | ||
| if (!authJson.tokens?.id_token || !authJson.tokens?.refresh_token) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The paste import doesn't validate if the JWT tokens are expired before storing them. Users might paste expired credentials and wonder why it doesn't work:
| if (!authJson.tokens?.id_token || !authJson.tokens?.refresh_token) { | |
| // Validate tokens aren't expired | |
| if (authJson.tokens?.id_token) { | |
| const { isTokenExpired } = await import('./oauth-helpers') | |
| if (isTokenExpired(authJson.tokens.id_token)) { | |
| throw new Error('The provided tokens are expired. Please authenticate with Codex CLI again first.') | |
| } | |
| } |
| * Attempt to redeem complimentary credits for Plus/Pro users | ||
| * This is a best-effort operation and failures are non-fatal | ||
| */ | ||
| export async function redeemComplimentaryCredits(idToken: string): Promise<void> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice implementation of the complimentary credits redemption! The best-effort approach with silent failure is appropriate here since it's a bonus feature.
|
To be implemented by the author of the issue |
This PR implements OAuth authentication for ChatGPT Plus/Pro accounts, allowing users to leverage their existing subscriptions instead of requiring separate API billing.
Summary
This implementation allows developers who already pay for ChatGPT Plus/Pro ($200/mo) to use those entitlements in Roo Code without opening a separate API billing account. It follows the Codex CLI authentication flow for compatibility.
Changes
chatgptauth mode alongside traditional API key modeKey Features
Testing
Related Issue
Fixes #6993
Next Steps
Important
Adds OAuth authentication for ChatGPT Plus/Pro accounts, enabling use of existing subscriptions with secure token management and integration into the OpenAI provider.
chatgpt-auth-manager.ts.ChatGptAuthManagerto handle sign-in, sign-out, and token refresh.chatgpt-credentials-manager.tsmanages credentials in VS Code's SecretStorage.OpenAiHandlerinopenai.tsto supportchatgptauth mode.oauth-server.tshandles OAuth callback and token exchange.oauth-helpers.tsprovides utilities for state generation, PKCE, and token validation.oauth.tsfor OAuth configuration and types.provider-settings.tsto includeopenAiAuthModeoption.This description was created by
for 0948135. You can customize this summary. It will automatically update as commits are pushed.