fix(external-apps): harden OAuth authorization attempts - #14147
fix(external-apps): harden OAuth authorization attempts#14147wenxi-onyx wants to merge 1 commit into
Conversation
Greptile SummaryThis PR migrates external-app OAuth state to the shared tenant-aware authorization-attempt store and hardens callbacks with owner binding, atomic one-shot consumption, and configuration fingerprint validation.
Confidence Score: 5/5The PR appears safe to merge with no concrete correctness or security defects identified. The shared attempt store preserves tenant-aware backend selection and atomic owner-bound state handling, while provider-specific PKCE behavior and configuration-change rejection are consistently wired and tested. Important Files Changed
Sequence DiagramsequenceDiagram
participant U as User
participant API as Onyx API
participant Store as Authorization Attempt Store
participant P as OAuth Provider
U->>API: Start external-app OAuth
API->>API: Generate PKCE pair when supported
API->>API: Fingerprint OAuth configuration
API->>Store: Store owner-bound attempt
API-->>U: Authorization URL with state and challenge
U->>P: Authorize application
P-->>U: Redirect with code and state
U->>API: Callback with code and state
API->>Store: Atomically consume owner-bound attempt
API->>API: Validate current configuration fingerprint
API->>P: Exchange code and server-side verifier
P-->>API: OAuth credentials
API->>API: Persist user credentials and refresh skills
API-->>U: Success
Reviews (1): Last reviewed commit: "fix(external-apps): harden OAuth authori..." | Re-trigger Greptile |
There was a problem hiding this comment.
1 issue found across 7 files
Confidence score: 4/5
backend/tests/unit/external_apps/test_oauth_flow.pyusesExternalAppType.LINEAR, whose provider supports PKCE, so these tests do not validate the claimed non-PKCE paths instart_external_app_oauthand the callback; add a fixture or provider configuration withsupports_pkce=Falseto cover that branch.
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/tests/unit/external_apps/test_oauth_flow.py">
<violation number="1" location="backend/tests/unit/external_apps/test_oauth_flow.py:39">
P3: The PR description says these tests exercise non-PKCE provider behavior, but every test here uses ExternalAppType.LINEAR, whose provider has supports_pkce=True. The non-PKCE branch in start_external_app_oauth/callback (no code_challenge/verifier, payload.code_verifier=None) is left untested. If that coverage lives in another file of this PR, note it here; otherwise add a test with a non-PKCE OAuth provider asserting the authorize URL omits code_challenge/code_challenge_method and the callback sends no code_verifier.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| id=7, | ||
| name="Linear", | ||
| enabled=True, | ||
| app_type=ExternalAppType.LINEAR, |
There was a problem hiding this comment.
P3: The PR description says these tests exercise non-PKCE provider behavior, but every test here uses ExternalAppType.LINEAR, whose provider has supports_pkce=True. The non-PKCE branch in start_external_app_oauth/callback (no code_challenge/verifier, payload.code_verifier=None) is left untested. If that coverage lives in another file of this PR, note it here; otherwise add a test with a non-PKCE OAuth provider asserting the authorize URL omits code_challenge/code_challenge_method and the callback sends no code_verifier.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/tests/unit/external_apps/test_oauth_flow.py, line 39:
<comment>The PR description says these tests exercise non-PKCE provider behavior, but every test here uses ExternalAppType.LINEAR, whose provider has supports_pkce=True. The non-PKCE branch in start_external_app_oauth/callback (no code_challenge/verifier, payload.code_verifier=None) is left untested. If that coverage lives in another file of this PR, note it here; otherwise add a test with a non-PKCE OAuth provider asserting the authorize URL omits code_challenge/code_challenge_method and the callback sends no code_verifier.</comment>
<file context>
@@ -0,0 +1,192 @@
+ id=7,
+ name="Linear",
+ enabled=True,
+ app_type=ExternalAppType.LINEAR,
+ organization_credentials=credentials,
+ ),
</file context>
🖼️ Visual Regression Report
|
253567a to
dfafe8e
Compare
|
Full-stack Preview (frontend + backend)
Sign in with GitHub as an |
|
Preview Deployment
|
dfafe8e to
7c6e8b2
Compare
7c6e8b2 to
b140fcf
Compare
Description
Craft external-app OAuth previously maintained its own Redis state record and deleted it only after token exchange. A concurrent callback could therefore reuse the same state, and this flow had a separate lifecycle from the connector and MCP OAuth implementations.
This change moves external apps onto the shared, tenant-aware authorization-attempt store introduced by the base PR. The feature now declares its namespace and payload type once; shared cache resolution, TTL, fingerprint validation, and PKCE-verifier constraints are not reimplemented locally. Each attempt is bound to the initiating user and claimed atomically before the upstream token request. The callback also rejects attempts when the app or OAuth client configuration changed while authorization was pending.
PKCE verifiers remain server-side. GitHub and Linear use the S256 flow, while providers without PKCE support preserve their existing exchange behavior. Provider-specific request construction, fingerprint inputs, token interpretation, credential persistence, and skill updates remain in the external-app domain.
How Has This Been Tested?
Additional Options