fix(integrations): source OAuth initiator userId from auth context#2720
fix(integrations): source OAuth initiator userId from auth context#2720
Conversation
The frontend hook posted `userId: ''` with a comment that the API would fill it in from the auth context, but the controller was reading userId straight from the request body, so every IntegrationOAuthState row was written with an empty userId. This was harmless until #2712 added a defense-in-depth session check on the OAuth callback that compares `session.user.id` to `oauthState.userId`. Because the stored value was always empty, every OAuth flow (GitHub, GCP, AWS, Rippling, etc.) now redirects with `error=session_mismatch` and "OAuth flow can only be completed by the user who initiated it." The spec did not catch this because the callback tests mocked `oauthState` directly with a non-empty userId rather than exercising start -> callback end-to-end. Resolve userId via the @userid() decorator instead of trusting the request body, drop it from StartOAuthDto, and stop sending it from the client. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
1 issue found across 3 files
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="apps/api/src/integration-platform/controllers/oauth.controller.ts">
<violation number="1" location="apps/api/src/integration-platform/controllers/oauth.controller.ts:92">
P2: `@UserId()` on this hybrid-auth route introduces a 500 failure path for API-key callers. Handle non-session auth explicitly (or reject it with a proper HTTP auth exception) instead of relying on a decorator that throws a generic `Error`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Address cubic-dev-ai review feedback. With HybridAuthGuard alone, an API-key or service-token caller would reach the @userid() decorator, which throws a plain Error (turning into a generic 500) when no session user is present. Add SessionOnlyGuard between HybridAuthGuard and PermissionGuard so non-session auth is rejected with a clean 403 and a clear message. The OAuth callback already requires a real session (see checkSessionMatchesState), so non-session callers could never have completed the flow anyway — this just moves the rejection upfront. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Addressed @cubic-dev-ai review feedback in The concern: The Fix: Added @UseGuards(HybridAuthGuard, SessionOnlyGuard, PermissionGuard)Note: this is purely a UX/error-reporting improvement — non-session callers could never complete the OAuth flow anyway because the callback requires a real session via Tests still passing 19/19. |
@tofikwest This is a great fix. Adding By placing it after This effectively resolves the UX and monitoring concern I raised. For more details on how guards and decorators interact in this codebase, you can check the API authentication docs. |
|
🎉 This PR is included in version 3.39.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Summary
error=session_mismatch&error_description=OAuth flow can only be completed by the user who initiated it.userId: ''with a comment claiming the API will fill it in from the auth context, butoauth.controller.tsreadsuserIdstraight from the request body — so everyIntegrationOAuthStaterow is written with an empty string.session.user.idtooauthState.userId. Empty string never matches a real user ID → every flow fails.userIdvia the@UserId()decorator, drop the field fromStartOAuthDto, and stop sending it from the client. The defense-in-depth check from fix(security): address pentest findings — admin escalation, secrets RBAC, oauth session check #2712 is preserved and now actually works as intended.Why the spec didn't catch it
The new callback tests in #2712 mock
oauthStatedirectly withuserId: 'user_1'rather than exercisingstartOAuth → callbackend-to-end, so the empty-userId write path was never exposed.Scope
Strictly in scope. Verified:
IntegrationOAuthState.userIdis read in only two places (the new check atoauth.controller.ts:475and a warning log at:289) — both will continue working with the real userId.oauth-state.repository.ts) only queries bystate(the random token), never byuserId.comp-private,apps/portal, orapps/framework-editor./v1/integrations/oauth/start: the frontend hook, which is updated here.Deploy-order safety
userId: ''in the body is ignored, decorator wins.deleteExpired().Test plan
npx jest src/integration-platform/controllers/oauth.controller.spec.ts— 19/19 passingnpx turbo run typecheck --filter=@trycompai/api— no new errors introduced (177 pre-existing in unrelated files, identical count before/after)npx turbo run typecheck --filter=@trycompai/app— no new errors introduced🤖 Generated with Claude Code