fix(security): close cross-tenant IDOR gaps in OAuth credential and execution auth#4549
fix(security): close cross-tenant IDOR gaps in OAuth credential and execution auth#4549waleedlatif1 wants to merge 7 commits intostagingfrom
Conversation
…orization Routes that called resolveOAuthAccountId followed by a conditional workspace permission check (only run when workspaceId was set) silently skipped all ownership validation on the legacy account-ID fallback path. Any authenticated user could supply a raw account.id to access another tenant's OAuth credentials. - Replace resolveOAuthAccountId + conditional perm check with authorizeCredentialUse in: auth/oauth/wealthbox/item, tools/gmail/label, tools/onedrive/files, tools/onedrive/folder, tools/outlook/folders, tools/wealthbox/item (routes 1, 3-7) - Add authorizeCredentialUse ownership gate before resolveVertexCredential in providers/route.ts (route 2) - Add verifyFileAccess check on the user-supplied file key before downloadFileFromStorage in tools/wordpress/upload (route 8) - Add workflowId param to PauseResumeManager methods (enqueueOrStartResume, beginPausedCancellation, completePausedCancellation, blockQueuedResumesForCancellation, clearPausedCancellationIntent, getPausedCancellationStatus, processQueuedResumes) and filter all pausedExecutions lookups by workflowId so callers cannot act on another tenant's paused execution by supplying a foreign executionId (route 9) - Update all call sites (cancel, resume, poll routes) to pass workflowId
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryHigh Risk Overview Adds an explicit credential-ownership check before resolving Vertex credentials in Hardens HITL pause/resume/cancel by threading Reviewed by Cursor Bugbot for commit 86f3a53. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Greptile SummaryThis PR closes cross-tenant IDOR vulnerabilities in OAuth credential access and paused-execution management by centralizing auth enforcement in
Confidence Score: 5/5Safe to merge — all credential and execution paths now enforce ownership on every code branch, with no identified regressions. All call sites of No files require special attention. The HITL manager and credential-access module are the highest-impact files and both look correct. Important Files Changed
Reviews (5): Last reviewed commit: "fix(security): thread workflowId through..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…ocessQueuedResumes, fix log level - Fail closed in WordPress upload when userFile.key is present but authResult.userId is absent, preventing silent bypass of ownership check via JWT fallback path - Thread workflowId into processQueuedResumes in the async resume error-recovery path and in pause-persistence.ts to close residual cross-tenant gap - Change logger.error to logger.warn for credential access denial in OneDrive folder route to match all other routes in this PR
|
@cursor review |
|
@greptile |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 94bc2e2. Configure here.
…l sites Closes residual cross-tenant IDOR gap where processQueuedResumes was called without a workflowId scope in persistPauseResult, startResumeExecution (success and error paths), and clearPausedCancellationIntent. workflowId was already in scope at each site — this wires it through to the existing optional parameter.
|
Follow-up fix (fd234ac): threaded |
…caught errors - catch (error: any) → catch (error) + toError(error).message in resume and cancel routes - Remove what-not-why inline comments from wordpress upload and onedrive/files routes - Remove redundant debug-only item breakdown log and the file-IDs log in onedrive/files - Trim extraneous DAG-edge comments from updateSnapshotAfterResume in HITL manager
|
Quality pass complete (0d4c9c6):
All 4 original review threads were already resolved before this pass. Lint is clean. |
|
@cursor review |
|
@greptile |
|
@cursor review |
|
@greptile |
All 7 method signatures (processQueuedResumes, enqueueOrStartResume, beginPausedCancellation, completePausedCancellation, blockQueuedResumesForCancellation, clearPausedCancellationIntent, getPausedCancellationStatus) previously accepted workflowId as optional. Every call site already supplies it — making it required closes the vulnerability at the type level so future callers cannot accidentally omit tenant scoping and silently fall back to an unscoped DB query.
…alls and remove dead branches in credential-access
|
@greptile |
|
@cursor review |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 86f3a53. Configure here.
| .where( | ||
| and( | ||
| eq(pausedExecutions.executionId, executionId), | ||
| ...(workflowId ? [eq(pausedExecutions.workflowId, workflowId)] : []), |
There was a problem hiding this comment.
Required workflowId silently skipped when empty string passed
Medium Severity
Several PauseResumeManager methods declare workflowId as a required string parameter but internally use falsy checks (workflowId ? ... : ... or ...(workflowId ? [...] : [])) to conditionally apply the filter. Since empty string "" is falsy in JavaScript, an empty-string workflowId silently degrades to the old executionId-only lookup, bypassing the cross-tenant scoping this PR intends to enforce. The type system won't catch this because "" satisfies string. Methods affected include beginPausedCancellation, completePausedCancellation, blockQueuedResumesForCancellation, clearPausedCancellationIntent, processQueuedResumes, and enqueueOrStartResume.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 86f3a53. Configure here.
1 participant
Summary
gmail/label,onedrive/files,onedrive/folder,outlook/folders,wealthbox/item,auth/oauth/wealthbox/item) calledresolveOAuthAccountIdthen only checked workspace permissions whenworkspaceIdwas set — the legacy account-ID fallback path returned noworkspaceId, silently skipping all ownership validation. Any authenticated user could supply a rawaccount.idto access another tenant's OAuth credentialsauthorizeCredentialUse, which enforces ownership on every credential type including the legacy fallback pathauthorizeCredentialUseownership gate beforeresolveVertexCredentialinproviders/route.ts(Vertex AI credential path had the same gap)verifyFileAccesscheck inwordpress/uploadbeforedownloadFileFromStorage— file key was user-supplied with no ownership verificationworkflowIdfilter to allPauseResumeManagermethods (enqueueOrStartResume,beginPausedCancellation, etc.) — previously looked up paused executions byexecutionIdonly, allowing any authenticated user to cancel or resume another tenant's paused workflow execution; update all call sites (cancel, resume, poll routes)Type of Change
Testing
Tested manually.
authorizeCredentialUsebackwards-compatibility verified: legacy account-ID callers who own their credentials still work (ownership enforced); HITL managerworkflowIdparam is optional so internal self-calls without it are unaffected;verifyFileAccessconfirmed to exist and match usage.Checklist