feat(llm): Add subscription-based authentication for OpenAI Codex models #1682
Conversation
Implement OAuth PKCE flow for authenticating with OpenAI's ChatGPT service, allowing users with ChatGPT Plus/Pro subscriptions to use Codex models (gpt-5.2-codex, gpt-5.2, gpt-5.1-codex-max, gpt-5.1-codex-mini) without consuming API credits. Key features: - OAuth PKCE flow with local callback server for secure authentication - Credential storage with automatic token refresh - LLM.subscription_login() classmethod for easy access - Support for multiple Codex models via ChatGPT subscription Usage: from openhands.sdk import LLM llm = LLM.subscription_login(model='gpt-5.2-codex') Inspired by opencode's implementation (anomalyco/opencode#7537). Co-authored-by: openhands <openhands@all-hands.dev>
Coverage Report •
|
|||||||||||||||||||||||||||||||||||
- Use authlib for PKCE generation (generate_token, create_s256_code_challenge) - Use aiohttp.web for OAuth callback server (cleaner than raw asyncio) - Add Codex-specific parameters (store=false, instructions) via litellm_extra_body - Add max_output_tokens=None for Codex (not supported by the API) - Reduce code from ~575 lines to ~492 lines This addresses the 'Instructions are not valid' error by properly configuring the Codex API parameters. Co-authored-by: openhands <openhands@all-hands.dev>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
@OpenHands /codereview-roasted and help me fix all the issues and make sure pre-commit run --all-files pass |
|
I'm on it! xingyaoww can track my progress at all-hands.dev |
- Move time import to module level in credentials.py - Replace os.uname() with platform.system()/machine() for cross-platform compatibility - Move urllib.parse.urlencode import to module level in openai.py - Remove extra blank line before __all__ in auth/__init__.py - Apply ruff formatting fixes to llm.py Co-authored-by: openhands <openhands@all-hands.dev>
SummaryI completed the roasted code review and fixed all identified issues. The changes have been pushed to the Code Review (Linus-Style) 🔴 Needs ImprovementCritical Issues Fixed:
Style Fixes: Changes Made (3 files, -6 lines net)
Verification ✅
The PR is now updated at: #1682 |
Refactor the inline subscription transport check to a reusable @Property method on the LLM class. This improves code readability and allows the subscription check to be used elsewhere if needed. Co-authored-by: openhands <openhands@all-hands.dev>
… heuristics - Add _is_subscription private attribute to LLM class (default False) - Update is_subscription property to use the attribute directly - Set _is_subscription=True in OpenAISubscriptionAuth.create_llm() - Update tests to explicitly set _is_subscription for subscription tests - Add example file for subscription login usage This removes the URL-based heuristics and ensures is_subscription only returns True for LLMs created via LLM.subscription_login(). Co-authored-by: openhands <openhands@all-hands.dev>
- Hardcode stream=True, temperature=None, max_output_tokens=None in subscription_login - Skip unsupported parameters in responses_options for subscription mode - Force parameters to None after LLM init to prevent auto-detection from model info Fixes API errors: 'Unsupported parameter: max_output_tokens', 'Unsupported parameter: temperature', 'Stream must be set to true' Co-authored-by: openhands <openhands@all-hands.dev>
|
@OpenHands review diff against main, understand the purpose of this PR, then open a new PR in OpenHands/docs repo to document the new feature added in this PR. The doc should appear under |
|
I'm on it! xingyaoww can track my progress at all-hands.dev |
Add a Literal type alias SupportedVendor = Literal["openai"] for the vendor parameter in subscription_login methods. This provides better type safety and makes it easier to add more vendors in the future. Changes: - Define SupportedVendor type in openhands/sdk/llm/auth/openai.py - Export SupportedVendor from openhands/sdk/llm/auth/__init__.py - Update vendor parameter type in: - LLM.subscription_login() - subscription_login() - subscription_login_async() Co-authored-by: openhands <openhands@all-hands.dev>
…hentication This adds documentation for the new subscription_login() feature that allows users to authenticate with their ChatGPT Plus/Pro subscription to access Codex models without consuming API credits. Key features documented: - OAuth PKCE authentication flow - Credential caching and automatic token refresh - Supported models (gpt-5.2-codex, gpt-5.2, gpt-5.1-codex-max, gpt-5.1-codex-mini) - Configuration options (force_login, open_browser) - Credential storage location Related SDK PR: OpenHands/software-agent-sdk#1682 Co-authored-by: openhands <openhands@all-hands.dev>
- Extract subscription-specific message transformation into dedicated methods (_transform_for_subscription, _inject_system_prefix) - Use list comprehension for message normalization - Consolidate create_llm method in OpenAISubscriptionAuth - Reduce code duplication and improve readability - Fix test assertion to match actual originator header value Co-authored-by: openhands <openhands@all-hands.dev>
SummaryI've reviewed the PR diff for Checklist
Changes MadeNew file:
Updated:
Pull RequestDocumentation PR: OpenHands/docs#267 (draft) |
Move _transform_for_subscription and _inject_system_prefix from LLM class to auth/openai.py as standalone utility functions: - transform_for_subscription() - inject_system_prefix() This improves code organization by keeping subscription-specific logic in the auth module. Co-authored-by: openhands <openhands@all-hands.dev>
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Overall the OAuth PKCE implementation is solid with proper state validation. However, there are several security and platform compatibility issues that should be addressed before merging. See inline comments for details.
|
|
||
| except Exception as e: | ||
| logger.warning(f"Failed to decode JWT: {e}") | ||
| return None |
There was a problem hiding this comment.
🔴 Critical: Decoding JWT without signature verification is a security risk. While you trust the token came from OpenAI's auth server, if an attacker can intercept the OAuth callback (e.g., through DNS spoofing, malicious proxy, or compromised network), they could inject a manipulated token with a forged account ID.
Recommendation: Use a proper JWT library (e.g., PyJWT) to verify the signature before extracting claims, or at minimum validate the token by making an API call to OpenAI to verify it's legitimate. Even though PKCE prevents token theft, it doesn't prevent token manipulation in transit.
| return None | |
| except Exception as e: | |
| logger.warning(f"Failed to decode JWT: {e}") | |
| return None | |
| # TODO: Consider adding JWT signature verification using PyJWT | |
| # to prevent potential token manipulation attacks |
There was a problem hiding this comment.
@OpenHands can you address this concern?
There was a problem hiding this comment.
I'm on it! xingyaoww can track my progress at all-hands.dev
There was a problem hiding this comment.
I've addressed the security concern raised by @all-hands-bot about decoding JWT without signature verification. The changes have been pushed to the PR.
Summary of Changes
Security Fix: The _extract_chatgpt_account_id function now verifies JWT signatures using OpenAI's published JWKS (JSON Web Key Set) before extracting claims. This prevents potential token injection attacks through OAuth callback interception (e.g., DNS spoofing, malicious proxy, or compromised network).
Key Changes:
-
Added
_JWKSCacheclass (openhands-sdk/openhands/sdk/llm/auth/openai.py):- Thread-safe caching of OpenAI's JWKS
- Fetches keys from
https://auth.openai.com/.well-known/jwks.json - 1-hour cache TTL to minimize network calls
-
Updated
_extract_chatgpt_account_idfunction:- Now uses
authlib.jose.jwt.decode()with the JWKS to verify signatures - Validates standard JWT claims (issuer, expiration)
- Returns
Noneif signature verification fails (graceful degradation)
- Now uses
-
Added comprehensive tests (
tests/sdk/llm/auth/test_openai.py):TestJWTVerificationclass with tests for valid JWT, invalid signature, JWKS fetch failure, missing account ID, and expired tokenTestJWKSCacheclass with tests for cache fetching, caching behavior, and cache clearing
Checklist:
- ✅ Security concern addressed - JWT signatures are now verified
- ✅ Uses existing
authlibdependency (no new dependencies added) - ✅ All 22 tests pass
- ✅ Type checks pass (pyright)
- ✅ Changes are minimal and focused on the security fix
- ✅ Pushed to
feat/openai-subscription-authbranch
| creds_file = self._get_credentials_file(credentials.vendor) | ||
| with open(creds_file, "w") as f: | ||
| json.dump(credentials.model_dump(), f, indent=2) |
There was a problem hiding this comment.
🟠 Important: OAuth tokens (access_token and refresh_token) are stored in plaintext JSON. While file permissions help on Unix systems, this is still a security concern. If an attacker gains file system access, they can steal the tokens.
Recommendation: Consider encrypting the tokens at rest using a key derived from system credentials or keyring integration (e.g., keyring package). For now, at minimum document this security limitation in the docstring.
| site = web.TCPSite(runner, "localhost", self._oauth_port) | ||
|
|
||
| try: | ||
| await site.start() |
There was a problem hiding this comment.
🟠 Important: No error handling for port conflicts. If port 1455 is already in use, site.start() will raise an exception and the OAuth flow fails without a helpful message.
Recommendation: Add port conflict handling:
| await site.start() | |
| try: | |
| await site.start() | |
| except OSError as e: | |
| if "address already in use" in str(e).lower(): | |
| raise RuntimeError( | |
| f"OAuth callback server port {self._oauth_port} is already in use. " | |
| f"Please close the conflicting application or specify a different port." | |
| ) from e | |
| raise |
| if not response.is_success: | ||
| raise RuntimeError(f"Token exchange failed: {response.status_code}") | ||
| return response.json() |
There was a problem hiding this comment.
🟡 Suggestion: Include the response body in error messages for better debugging. OAuth providers often return helpful error details in the response body.
| if not response.is_success: | |
| raise RuntimeError(f"Token exchange failed: {response.status_code}") | |
| return response.json() | |
| if not response.is_success: | |
| error_detail = response.text if response.text else "No details" | |
| raise RuntimeError( | |
| f"Token exchange failed: {response.status_code} - {error_detail}" | |
| ) |
| if not response.is_success: | ||
| raise RuntimeError(f"Token refresh failed: {response.status_code}") | ||
| return response.json() |
There was a problem hiding this comment.
🟡 Suggestion: Same as above - include response body in error message.
| if not response.is_success: | |
| raise RuntimeError(f"Token refresh failed: {response.status_code}") | |
| return response.json() | |
| if not response.is_success: | |
| error_detail = response.text if response.text else "No details" | |
| raise RuntimeError( | |
| f"Token refresh failed: {response.status_code} - {error_detail}" | |
| ) |
| ) -> dict[str, Any]: | ||
| """Exchange authorization code for tokens.""" | ||
| async with AsyncClient() as client: | ||
| response = await client.post( |
There was a problem hiding this comment.
🟡 Suggestion: Add an explicit timeout to HTTP requests to prevent hanging indefinitely if the OAuth server is slow or unresponsive.
| response = await client.post( | |
| async with AsyncClient(timeout=30.0) as client: |
| logger = get_logger(__name__) | ||
|
|
||
| # OAuth configuration for OpenAI Codex | ||
| CLIENT_ID = "app_EMoamEEZ73f0CkXaXp7hrann" |
There was a problem hiding this comment.
🟢 Nit: Document that this is a public client ID and why it's safe to hardcode it here. This will help future reviewers understand this is intentional.
| CLIENT_ID = "app_EMoamEEZ73f0CkXaXp7hrann" | |
| # OAuth configuration for OpenAI Codex | |
| # This is a public client ID for OpenAI's OAuth flow (safe to commit) | |
| CLIENT_ID = "app_EMoamEEZ73f0CkXaXp7hrann" |
| @property | ||
| def credentials_dir(self) -> Path: | ||
| """Get the credentials directory, creating it if necessary.""" | ||
| self._credentials_dir.mkdir(parents=True, exist_ok=True) |
There was a problem hiding this comment.
🟡 Suggestion: Also set restrictive permissions on the directory itself, not just the files within it.
| self._credentials_dir.mkdir(parents=True, exist_ok=True) | |
| self._credentials_dir.mkdir(parents=True, exist_ok=True) | |
| # Set directory permissions to owner-only (rwx------) | |
| if os.name != 'nt': | |
| self._credentials_dir.chmod(0o700) | |
| return self._credentials_dir |
Co-authored-by: OpenHands Bot <contact@all-hands.dev>
Co-authored-by: OpenHands Bot <contact@all-hands.dev>
Address security concern about decoding JWT without signature verification. Now verifies JWT signatures using OpenAI's published JWKS before extracting claims, preventing potential token injection attacks through OAuth callback interception. Changes: - Add _JWKSCache class for thread-safe caching of OpenAI's JWKS - Update _extract_chatgpt_account_id to verify JWT signature using authlib - Add comprehensive tests for JWT verification and JWKS caching - Import authlib.jose for JWT/JWKS handling Co-authored-by: openhands <openhands@all-hands.dev>
|
@OpenHands please implement the following for me: (1) a short notice that’s hard to miss, (2) a longer explainer the first time (or behind --help / --verbose), and (3) a clear “continue = accept” moment. Minimal banner (print every time login starts) First-time / verbose disclaimer (print once per machine or per config profile) @OpenHands please fix the failing actions on PR #1682 at branch |
|
I'm on it! xingyaoww can track my progress at all-hands.dev |
- Add minimal banner shown every time login starts with Terms of Use link - Add verbose disclaimer shown on first login or with verbose flag - Add 'continue = accept' confirmation prompt before proceeding - Store consent acknowledgment marker in credentials directory - Handle non-interactive mode: require prior consent or raise error - Fix undefined 'logger' in credentials.py (use warnings.warn instead) - Add comprehensive tests for consent banner system Co-authored-by: openhands <openhands@all-hands.dev>
|
I've successfully implemented the consent banner system for ChatGPT sign-in and fixed the failing pre-commit checks. The changes have been pushed to the Summary of Changes1. Fixed Pre-commit Failure
2. Implemented Consent Banner System (as requested)(1) Minimal banner (shown every time login starts): (2) First-time/verbose disclaimer (shown once per machine or with (3) Clear "continue = accept" moment:
3. New API ParametersAdded to
4. Non-interactive Mode Handling
5. TestsAdded 14 comprehensive tests for the consent banner system covering:
All 36 tests pass and all pre-commit checks pass. |
neubig
left a comment
neubig
left a comment
There was a problem hiding this comment.
Basically LGTM, a few comments!
| expires_in=tokens.get("expires_in", 3600), | ||
| ) | ||
| return updated | ||
| except Exception as e: |
There was a problem hiding this comment.
Maybe it'd be better to not catch this exception and allow it to propagate and handle it appropriately in the app code?
| # Message transformation utilities for subscription mode | ||
| # ========================================================================= | ||
|
|
||
| DEFAULT_SUBSCRIPTION_INSTRUCTIONS = ( |
There was a problem hiding this comment.
The message and the variable name seem to not match very well. Maybe "DEFAULT_SYSTEM_MESSAGE"?
| vendor: SupportedVendor = "openai", | ||
| model: str = "gpt-5.2-codex", |
There was a problem hiding this comment.
I think we should just remove the defaults here.
There was a problem hiding this comment.
One thing I'm a bit worried about is that because all of these are mocked, I don't know if we'll be able to actually detect if any of this breaks. It'd be worth thinking about how we can do integration tests.
There was a problem hiding this comment.
Yeah this is actually tricky.. because you will need to go through an OAuth so it'd be pretty had to do it in CI. OpenHands please remove the useless tests and open an issue to track the need to add integration tests for oauth
|
@OpenHands please address comments from @neubig and also take my follow up comment under it into consideration |
|
I'm on it! xingyaoww can track my progress at all-hands.dev |
- Let exception propagate in refresh_if_needed() instead of catching it (allows app code to handle errors appropriately) - Rename DEFAULT_SUBSCRIPTION_INSTRUCTIONS to DEFAULT_SYSTEM_MESSAGE (better matches the variable's purpose) - Remove default values for vendor and model in subscription_login() (requires explicit specification) - Remove heavily mocked JWT/JWKS tests that can't detect real breakages - Update example to use explicit vendor parameter - Created issue #1806 to track integration test requirements Co-authored-by: openhands <openhands@all-hands.dev>
SummaryI've addressed all the review comments from @neubig and @xingyaoww's follow-up on PR #1682. The changes have been pushed to the Changes Made
Files Modified
Verification
Related
|
|
Looks like there are a few issues preventing this PR from being merged!
If you'd like me to help, just leave a comment, like Feel free to include any additional details that might help me get this PR into a better state. You can manage your notification settings |
Summary
This PR implements OAuth PKCE flow for authenticating with OpenAI's ChatGPT service, allowing users with ChatGPT Plus/Pro subscriptions to use Codex models without consuming API credits.
Key features:
~/.local/share/openhands/auth/)LLM.subscription_login()classmethod for easy accessgpt-5.2-codexgpt-5.2gpt-5.1-codex-maxgpt-5.1-codex-miniUsage:
Implementation inspired by: opencode's implementation
New modules:
openhands/sdk/llm/auth/__init__.py- Auth module exportsopenhands/sdk/llm/auth/credentials.py- Credential storage and retrievalopenhands/sdk/llm/auth/openai.py- OpenAI OAuth PKCE flow implementationChecklist
Note: This feature requires a ChatGPT Plus/Pro subscription to test the actual OAuth flow. The unit tests cover the credential storage, PKCE generation, URL building, and mock token refresh scenarios.
@xingyaoww can click here to continue refining the PR
Agent Server images for this PR
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdknikolaik/python-nodejs:python3.12-nodejs22golang:1.21-bookwormPull (multi-arch manifest)
# Each variant is a multi-arch manifest supporting both amd64 and arm64 docker pull ghcr.io/openhands/agent-server:1bfee76-pythonRun
All tags pushed for this build
About Multi-Architecture Support
1bfee76-python) is a multi-arch manifest supporting both amd64 and arm641bfee76-python-amd64) are also available if needed