refactor(oauth-config): use shared authorization attempts - #14158
refactor(oauth-config): use shared authorization attempts#14158wenxi-onyx wants to merge 1 commit into
Conversation
GitHub Actions
Inactive
Greptile SummaryThis PR moves OAuth-config authorization state into the shared one-time authorization-attempt store and adds owner binding, PKCE, configuration fingerprinting, and safe return-path validation.
Confidence Score: 4/5The PR should not merge until OAuth configurations for providers without PKCE support remain usable. The generic OAuth-config path now sends PKCE parameters to every provider, while related flows require an explicit provider capability before doing so. Files Needing Attention: backend/onyx/server/features/oauth_config/api.py Important Files Changed
Sequence DiagramsequenceDiagram
participant U as User
participant API as OAuth Config API
participant Cache as Authorization Attempt Store
participant P as OAuth Provider
participant DB as Database
U->>API: POST /oauth-config/initiate
API->>Cache: Store owner, config fingerprint, return path, verifier
API-->>U: Authorization URL with state and PKCE challenge
U->>P: Authorize
P-->>U: Callback with code and state
U->>API: POST /oauth-config/callback
API->>Cache: Consume owner-bound attempt
API->>API: Verify configuration fingerprint
API->>P: Exchange code and PKCE verifier
P-->>API: Access token
API->>DB: Store user OAuth token
API-->>U: Return safe redirect path
Prompt To Fix All With AI### Issue 1
backend/onyx/server/features/oauth_config/api.py:275-282
**Unconditional PKCE breaks provider compatibility**
If an existing OAuth configuration uses a provider that does not support PKCE or rejects unknown protocol parameters, this path always sends a `code_challenge` and later a `code_verifier`, causing authorization or token exchange to fail without storing a token. The other provider-agnostic OAuth flows gate PKCE behind an explicit capability flag.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "refactor(oauth-config): use shared autho..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| code_verifier, code_challenge = generate_pkce_pair() | ||
| state = generate_authorization_state() | ||
|
|
||
| # Build authorization URL | ||
| redirect_uri = f"{WEB_DOMAIN}/oauth-config/callback" | ||
| authorization_url = OAuthTokenManager.build_authorization_url( | ||
| oauth_config, redirect_uri, state | ||
| oauth_config, | ||
| _oauth_callback_url(), | ||
| state, | ||
| code_challenge=code_challenge, |
There was a problem hiding this comment.
If an existing OAuth configuration uses a provider that does not support PKCE or rejects unknown protocol parameters, this path always sends a code_challenge and later a code_verifier, causing authorization or token exchange to fail without storing a token. The other provider-agnostic OAuth flows gate PKCE behind an explicit capability flag.
Knowledge Base Used: Restore the previous OAuth login flow
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/onyx/server/features/oauth_config/api.py
Line: 275-282
Comment:
**Unconditional PKCE breaks provider compatibility**
If an existing OAuth configuration uses a provider that does not support PKCE or rejects unknown protocol parameters, this path always sends a `code_challenge` and later a `code_verifier`, causing authorization or token exchange to fail without storing a token. The other provider-agnostic OAuth flows gate PKCE behind an explicit capability flag.
**Knowledge Base Used:** [Restore the previous OAuth login flow](https://app.greptile.com/onyx/-/custom-context/knowledge-base/onyx-dot-app/onyx/-/reverts/revert_7593-20260120-fastapi-users-oauth-csrf-010bc36.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
GitHub Actions
Error
|
Full-stack Preview (frontend + backend)
Sign in with GitHub as an |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8115ace314
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| redirect_uri=request.return_path, | ||
| additional_data={"oauth_config_id": request.oauth_config_id}, | ||
| ) | ||
| _validate_additional_authorization_params(oauth_config) |
There was a problem hiding this comment.
When an OAuth config supplies the RFC 8707 resource value through additional_params, this validation now rejects initiation. conflicting_authorization_params() reserves resource, but this flow never supplies a replacement to build_authorization_url(). Such configs worked before this change and now have no supported way to send the required resource value.
Useful? React with 👍 / 👎.
| additional_data={"oauth_config_id": request.oauth_config_id}, | ||
| ) | ||
| _validate_additional_authorization_params(oauth_config) | ||
| code_verifier, code_challenge = generate_pkce_pair() |
There was a problem hiding this comment.
When a configured provider does not support PKCE, this unconditional generation makes every OAuth-config flow send an S256 challenge and verifier. Other repository OAuth flows check a supports_pkce capability because some providers reject these fields. OAuthConfig has no equivalent setting, so previously working non-PKCE integrations can no longer complete authorization.
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 4 files
Confidence score: 3/5
backend/onyx/server/features/oauth_config/api.pysends PKCE parameters to every configured OAuth provider, which can cause providers that reject or lack PKCE support to fail during authorization or token exchange; gate generation and exchange on an explicit provider capability.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/onyx/server/features/oauth_config/api.py">
<violation number="1" location="backend/onyx/server/features/oauth_config/api.py:275">
P1: Gate PKCE generation and exchange on an explicit provider capability. This currently sends `code_challenge` to every configured provider and later sends `code_verifier`, so providers that reject or do not support PKCE cannot complete OAuth.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| additional_data={"oauth_config_id": request.oauth_config_id}, | ||
| ) | ||
| _validate_additional_authorization_params(oauth_config) | ||
| code_verifier, code_challenge = generate_pkce_pair() |
There was a problem hiding this comment.
P1: Gate PKCE generation and exchange on an explicit provider capability. This currently sends code_challenge to every configured provider and later sends code_verifier, so providers that reject or do not support PKCE cannot complete OAuth.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/onyx/server/features/oauth_config/api.py, line 275:
<comment>Gate PKCE generation and exchange on an explicit provider capability. This currently sends `code_challenge` to every configured provider and later sends `code_verifier`, so providers that reject or do not support PKCE cannot complete OAuth.</comment>
<file context>
@@ -215,18 +271,25 @@ def initiate_oauth_flow(
- additional_data={"oauth_config_id": request.oauth_config_id},
- )
+ _validate_additional_authorization_params(oauth_config)
+ code_verifier, code_challenge = generate_pkce_pair()
+ state = generate_authorization_state()
</file context>
8115ace to
0224ea8
Compare
GitHub Actions
Inactive
GitHub Actions
Error
0224ea8 to
2d8f3d4
Compare
GitHub Actions
Inactive
GitHub Actions
Inactive
🖼️ Visual Regression Report
|
1 participant
Description
OAuth-protected actions previously reused the federated-connector state format. That coupling did not provide the same owner-bound, atomic callback lifecycle used by the other OAuth connection flows and left PKCE and pending-configuration validation outside the action flow.
This change migrates OAuth actions to the shared authorization-attempt store. The feature declares its namespace and payload type once and reuses the base implementation for tenant-aware cache resolution, TTL, fingerprint validation, PKCE-verifier constraints, protected authorization parameters, and safe return paths. Initiation creates an owner-bound attempt with a server-side verifier and configuration fingerprint; the callback atomically claims it, rejects changed configuration, exchanges the code with PKCE, and persists the token through the existing OAuthConfig path.
OAuthConfig lookup, permissions, fingerprint inputs, token persistence, and action behavior remain feature-owned, so this completes the lifecycle migration without introducing a universal provider abstraction.
How Has This Been Tested?
Additional Options