fix(security): trust-access oracle + proxied-action org checks (GH-042 follow-ups) - #3578
dennisofficial wants to merge 1 commit into
Conversation
…follow-ups) - POST /v1/trust-access/:org/requests returned three distinguishable responses (already-approved with grant.expiresAt, 400 for a pending request, 201 for a new one) — an email-enumeration oracle. All paths now return one generic message, mirroring the reclaimAccess fix, and grant details are never in the body. - uploadAutomationScript, executeAutomationScript, publishAutomation and restoreVersion checked the session org but not that the forwarded taskId/automationId belongs to it; each now verifies ownership before proxying to the enterprise API. - createVersion now rejects scriptKey values outside the caller's organization S3 prefix (including '..' escapes), so a caller cannot register another tenant's object as an automation version. - Six public trust readers resolved portals without a status filter; all now only resolve published portals (overview, custom links, vendors, favicon, security questionnaire, custom frameworks). - Dropped the status: 'published' literal from the trust-access ensureFriendlyUrl upsert.
|
|
|
@cubic-dev-ai review it |
@dennisofficial I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
2 issues found across 9 files
Confidence score: 2/5
- In
task-automation-actions.ts, org ownership alone allows any authenticated org member to reach restore/publish mutations without task-update permission; guard these mutations through an API endpoint that checks the required permission. - In
task-automation-actions.ts, the forwardedtaskIdis not verified against the automation, so a caller can proxy an in-org automation with a different task ID; validate that the task and automation are bound before proxying.
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/app/src/app/(app)/[orgId]/tasks/[taskId]/automation/[automationId]/actions/task-automation-actions.ts">
<violation number="1" location="apps/app/src/app/(app)/[orgId]/tasks/[taskId]/automation/[automationId]/actions/task-automation-actions.ts:269">
P1: Bind the forwarded `taskId` to the automation before proxying; this check proves only that the automation belongs to the org, so a caller can submit an in-org automation with another task ID and still reach the enterprise action.</violation>
<violation number="2" location="apps/app/src/app/(app)/[orgId]/tasks/[taskId]/automation/[automationId]/actions/task-automation-actions.ts:273">
P1: Any authenticated member of the active org can reach the restore/publish proxy without task-update permission because this check only verifies org ownership. Route these mutations through an API endpoint guarded by `PermissionGuard` and `@RequirePermission('task', 'update')` before proxying.
(Based on your team's feedback about RBAC permission checks.)</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| automationId: data.automationId, | ||
| organizationId: activeOrganizationId, | ||
| }); | ||
| if (!belongsToOrg) { |
There was a problem hiding this comment.
P1: Any authenticated member of the active org can reach the restore/publish proxy without task-update permission because this check only verifies org ownership. Route these mutations through an API endpoint guarded by PermissionGuard and @RequirePermission('task', 'update') before proxying.
(Based on your team's feedback about RBAC permission checks.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/app/src/app/(app)/[orgId]/tasks/[taskId]/automation/[automationId]/actions/task-automation-actions.ts, line 273:
<comment>Any authenticated member of the active org can reach the restore/publish proxy without task-update permission because this check only verifies org ownership. Route these mutations through an API endpoint guarded by `PermissionGuard` and `@RequirePermission('task', 'update')` before proxying.
(Based on your team's feedback about RBAC permission checks.) </comment>
<file context>
@@ -255,6 +266,14 @@ export async function executeAutomationScript(data: {
+ automationId: data.automationId,
+ organizationId: activeOrganizationId,
+ });
+ if (!belongsToOrg) {
+ return { success: false, error: 'Unauthorized' };
+ }
</file context>
| return { success: false, error: 'Unauthorized' }; | ||
| } | ||
|
|
||
| const belongsToOrg = await isAutomationInOrganization({ |
There was a problem hiding this comment.
P1: Bind the forwarded taskId to the automation before proxying; this check proves only that the automation belongs to the org, so a caller can submit an in-org automation with another task ID and still reach the enterprise action.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/app/src/app/(app)/[orgId]/tasks/[taskId]/automation/[automationId]/actions/task-automation-actions.ts, line 269:
<comment>Bind the forwarded `taskId` to the automation before proxying; this check proves only that the automation belongs to the org, so a caller can submit an in-org automation with another task ID and still reach the enterprise action.</comment>
<file context>
@@ -255,6 +266,14 @@ export async function executeAutomationScript(data: {
return { success: false, error: 'Unauthorized' };
}
+ const belongsToOrg = await isAutomationInOrganization({
+ automationId: data.automationId,
+ organizationId: activeOrganizationId,
</file context>
Summary
Follow-up fixes from the security review of #3573 (merged). All changes are minimal, in-place fixes.
POST /v1/trust-access/:org/requestsreturned three distinguishable responses:already_approvedwithgrant.expiresAtfor an active grant, a 400 for a pending request, and a 201 for a new request. All three paths now return one generic message — the same shape the merged reclaim fix uses — and grant details are never in the response body. The fresh access link still goes out by email on the already-approved path.uploadAutomationScript,executeAutomationScript,publishAutomation, andrestoreVersionverified the caller's session org but not that the forwardedtaskId/automationIdbelongs to that org. Each now checks ownership before proxying (automation actions via the existingisAutomationInOrganizationhelper; upload verifies the task's organization directly since it takes noautomationId).CreateVersionDto.scriptKeyprefix check —AutomationsService.createVersionnow rejects script keys outside the caller's${organizationId}/S3 prefix (including..escapes), so a caller with task-update permission cannot register another tenant's object as an automation version.getPublicOverview,getPublicCustomLinks,getPublicVendors,getPublicFavicon,getPublicSecurityQuestionnaireEnabled(viaresolveTrustByFriendlyUrl), andgetPublicCustomFrameworks(viaresolveOrganizationId). All now resolve published portals only; a draft portal behaves exactly like a missing one.ensureFriendlyUrlpublished literal dropped — the trust-accessensureFriendlyUrlupsert no longer setsstatus: 'published'on create.Verification
apps/api:NODE_ENV=test bunx jest src/tasks/automations src/trust-portal— 166 tests pass, including new specs asserting identical generic responses across all threecreateAccessRequestpaths (no id/status/grant leakage), published-only resolution for each public reader, and thescriptKeyprefix/..rejection. Two controller suites fail at module load with a pre-existingDATABASE_URL/TLS guard error — reproduced identically atorigin/main, unrelated to this diff.apps/app:bunx vitest run src/app/(app)/[orgId]/tasks/[taskId]/automation/[automationId]/actions/task-automation-actions.test.ts— 10/10 pass (new spec: cross-org task/automation rejection, missing records, and success paths for all four actions).tsc --noEmitoutput forapps/apiandapps/appis byte-identical to theorigin/mainbaseline (pre-existing errors only, none in touched files).unbound-methoderror class already present atorigin/mainon these spec files (7 baseline, +2 from new tests using the same assertion pattern; lint is not CI-gated).Things to be aware of
Trust.statusschema default is@default(published)(packages/db/prisma/schema/trust.prisma). Dropping the literal fromensureFriendlyUrlremoves the explicit publish-on-create, but a row created there would still default to published. Fully closing that path needs a schema default change todraft— a product decision, since existing behavior elsewhere relies on the default. Not changed here.getAutomationRunStatusresidual gap from fix: P0 auth and tenant-scoping security fixes (GH-036, GH-042, GH-046, GH-049, GH-052, GH-075, GH-102, GH-272) #3573 still stands: enterprise/trigger run ids have no org mapping in this app's database, so that action can only require an authenticated session. Closing it needs the enterprise service to scope runs by org.POST /v1/trust-access/:org/requestsno longer returns the created request'sid/status. No consumer in this repo reads those fields; if an external portal frontend does, it should rely on the email flow instead.Summary by cubic
Closes the remaining security gaps from the GH-42 review: trust-access request responses no longer leak grant details, and proxied automation actions are now scope-checked before forwarding.
Security fixes
POST /v1/trust-access/:org/requestsreturns the same generic message on all three paths (active grant, pending, new) and never includes grant details; the fresh link still goes out by email.uploadAutomationScript,executeAutomationScript,publishAutomation, andrestoreVersionverify the forwarded task/automation belongs to the caller's org before proxying to the enterprise API.AutomationsService.createVersionrejectsscriptKeyvalues outside the caller's${organizationId}/S3 prefix, including..escapes.getPublicOverview,getPublicCustomLinks,getPublicVendors,getPublicFavicon,getPublicSecurityQuestionnaireEnabled,getPublicCustomFrameworks) now resolve only published portals; a draft behaves like a missing one.Behavior changes
createAccessRequestendpoint no longer returns the created request'sidorstatus; consumers should rely on the email flow.status: 'published'literal from the trust-accessensureFriendlyUrlupsert, though the schema default remainspublished.Written for commit 92966a5. Summary will update on new commits.