PR Custody

fix: validate enhanced link permission writes

@kaavee315checks n/achecks…codex/propose-fix-for-link-token-permission-vulnerability → master6 files · +370 −65review: @rohanprabhureview: @sohamganatrareview: @sarthakreview: @acsrujanreview: @anshugarg15updated 2d ago

GitHub

▸Description· 1 comment

Motivation

Prevent link-token holders from persisting attacker-controlled permission rows (wildcard/account-wide or toolkit-mismatched) that can bypass Enhanced Control prompts.
Ensure permission writes are only saved when the corresponding link auth submission succeeds, so failed/expired submissions cannot leave orphaned overrides.

Description

Hardened the input schema by updating ConnectedAccountToolPermissionInputSchema to reject wildcard '*' and preformatted override-key syntax containing : so stored rows cannot create account-wide overrides (apps/apollo/src/lib/consumer/dbUtils/connected_account_tool_permissions.ts).
Added server-side validation that checks submitted permission rows belong to the link session toolkit, conform to the auth-config tool policy, and exist in Thermos tool metadata before allowing persistence (validateEnhancedLinkPermissions and helpers in apps/apollo/src/pages/api/v3.1/internal/consumer/enhanced/link/[token].ts).
Changed the enhanced-link handler to submit the link auth input first and only persist validated permission rows after a successful submission, preventing failed submissions from leaving attacker-chosen rows behind (apps/apollo/src/pages/api/v3.1/internal/consumer/enhanced/link/[token].ts).
Small helpers and Zod parsing were added to safely parse and canonicalize Thermos tool responses and the auth-config allowed-tools policy (same file as above).

Testing

Ran formatter: pnpm exec oxfmt 'apps/apollo/src/pages/api/v3.1/internal/consumer/enhanced/link/[token].ts' (succeeded).
Ran linter on the modified route file: cd apps/apollo && pnpm lint -- 'src/pages/api/v3.1/internal/consumer/enhanced/link/[token].ts' (succeeded with warnings addressed).
Attempted type-check: cd apps/apollo && pnpm check-types and pnpm --filter @composio/lib bundle but full typecheck failed in this environment due to unrelated/generated artifacts (Prisma/thermos client and generated types) and existing repo toolchain expectations.
Attempted unit test run for the permission input schema via vitest, but tests were blocked/failed due to missing environment secrets and missing generated Prisma client binaries; these failures are environmental and not regressions introduced by this patch.

Codex Task

@CryogenicPlanet2d ago

Reviewed. The diagnosis is correct and the original two protections are legit:

Schema refine (* / : rejection) — load-bearing, not cosmetic: permission rows flow verbatim into override keys (connected_account_tool_permissions.ts resolve), and the gate honors *:{ca} as an account-wide override (toolRouterV2/features/permissions/keys.ts, resolve.ts). Since the refine sits on the shared input schema, the consumer + dashboard upsert routes are covered too.
validateEnhancedLinkPermissions — closes what the refine can't see on the only token-authenticated writer: well-formed slugs outside the session toolkit, outside the auth-config tool policy, or nonexistent.

Pushed 1f2fc7d on top to fix the ordering hazard. The original reorder (submit → persist) traded a documented-benign failure (orphan rows on a never-active account) for a worse one: if the upsert failed after a live submit, the connection committed ACTIVE with zero rows — and the resolver's legacy fallback treats a no-rows account as account-wide always_allow, silently disabling the gate. The commit makes submit + persist atomic instead:

Threads an optional Prisma.TransactionClient through submitLinkSessionInput → all three branch status writes (updateNonRedirectable / updateRedirectable / executeS2STokenExchangeForExisting) → DB_CONNECTED_ACCOUNT_UPDATE.statusAndData, and into DB_CONNECTED_ACCOUNT_TOOL_PERMISSIONS_UPSERT.manyForConnectedAccountNanoId (skips its own nested $transaction/serializable-retry when handed a caller-owned tx). All params optional — every existing call site unchanged.
Route wraps both in one prisma.$transaction (submitWithPermissionsAtomically); rollback via a typed sentinel converted back to a Result.
S2S persistFailedState deliberately stays on the global client so FAILED markers survive rollback.
Tx timeout 30s / maxWait 5s: the submit can hold external calls (token exchange, optional credential validation, ~10s fetch timeouts each) inside the tx window. Acceptable for this low-volume route, but it's the real cost of the design.

loading diff…

@CryogenicPlanet2d ago

Reviewed. The diagnosis is correct and the original two protections are legit:

Schema refine (* / : rejection) — load-bearing, not cosmetic: permission rows flow verbatim into override keys (connected_account_tool_permissions.ts resolve), and the gate honors *:{ca} as an account-wide override (toolRouterV2/features/permissions/keys.ts, resolve.ts). Since the refine sits on the shared input schema, the consumer + dashboard upsert routes are covered too.
validateEnhancedLinkPermissions — closes what the refine can't see on the only token-authenticated writer: well-formed slugs outside the session toolkit, outside the auth-config tool policy, or nonexistent.

Threads an optional Prisma.TransactionClient through submitLinkSessionInput → all three branch status writes (updateNonRedirectable / updateRedirectable / executeS2STokenExchangeForExisting) → DB_CONNECTED_ACCOUNT_UPDATE.statusAndData, and into DB_CONNECTED_ACCOUNT_TOOL_PERMISSIONS_UPSERT.manyForConnectedAccountNanoId (skips its own nested $transaction/serializable-retry when handed a caller-owned tx). All params optional — every existing call site unchanged.
Route wraps both in one prisma.$transaction (submitWithPermissionsAtomically); rollback via a typed sentinel converted back to a Result.
S2S persistFailedState deliberately stays on the global client so FAILED markers survive rollback.
Tx timeout 30s / maxWait 5s: the submit can hold external calls (token exchange, optional credential validation, ~10s fetch timeouts each) inside the tx window. Acceptable for this low-volume route, but it's the real cost of the design.