Merge branch 'pgx/s2' into feat/permission-groups-coverage

Author: waleedlatif1

.agents/skills/validate-permission-group-item/SKILL.md

@@ -106,8 +106,7 @@ For an allowlist the three states must be tested separately — `null` permits e
 bun run check:permission-group-enforcement
 bun run check:application-graph
 bun run check:capability-subject
-cd apps/sim && bun run type-check
-cd apps/sim && bunx vitest run lib/permission-groups
+cd apps/sim && bun run type-check && bunx vitest run lib/permission-groups
 ```
 
 All three are inside `check:audits`, which derives its list from the `check:*` scripts in `package.json` — a new audit is opted *out* deliberately. Read the output, not the exit codes. Reference success lines (counts grow):

apps/sim/app/api/logs/stats/route.test.ts

@@ -80,6 +80,20 @@ describe('GET /api/logs/stats', () => {
     expect(mocks.readLogStatsBounds).toHaveBeenCalled()
   })
 
+  /**
+   * The refusal needs both conditions, so an unfiltered read can never be
+   * refused and the config lookup — which re-reads workspace and
+   * organization/group state — is pure cost on the dashboard's common path.
+   */
+  it('does not consult the group for an unfiltered read', async () => {
+    resolveGroupConfigMock.mockResolvedValue({ hideCostInfo: true })
+
+    const response = await GET(makeRequest())
+
+    expect(response.status).toBe(200)
+    expect(resolveGroupConfigMock).not.toHaveBeenCalled()
+  })
+
   it('answers the same cost-filtered read when no group withholds spend', async () => {
     const response = await GET(makeRequest('&costOperator=%3E&costValue=0.5'))
 

apps/sim/app/api/logs/stats/route.ts

@@ -71,12 +71,10 @@ export const GET = withRouteHandler(async (request: NextRequest) => {
        * workspace access check above has already passed, so the caller is a
        * member learning about their own group.
        */
-      const hideCostInfo = await isWorkspaceCapabilityWithheld(
-        userId,
-        params.workspaceId,
-        'logs.cost'
-      )
-      if (hideCostInfo && logQuerySelectsCost(params)) {
+      if (
+        logQuerySelectsCost(params) &&
+        (await isWorkspaceCapabilityWithheld(userId, params.workspaceId, 'logs.cost'))
+      ) {
         return capabilityRefusalResponse('logs.cost')
       }
 

apps/sim/app/api/v1/tables/[tableId]/route.test.ts

@@ -19,12 +19,14 @@ const {
   mockGetTableById,
   mockGetUserEntityPermissions,
   mockPerformDeleteTable,
+  mockResolveWorkspaceRequestActor,
 } = vi.hoisted(() => ({
   mockCheckRateLimit: vi.fn(),
   mockCheckWorkspaceScope: vi.fn(),
   mockGetTableById: vi.fn(),
   mockGetUserEntityPermissions: vi.fn(),
   mockPerformDeleteTable: vi.fn(),
+  mockResolveWorkspaceRequestActor: vi.fn(),
 }))
 
 vi.mock('@/app/api/v1/middleware', () => ({
@@ -41,6 +43,12 @@ vi.mock('@/app/api/v1/middleware', () => ({
     rateLimit.keyType === 'personal'
       ? { kind: 'user', userId: rateLimit.userId }
       : { kind: 'workspace_api_key', keyCreatorUserId: rateLimit.userId },
+  /**
+   * Mirrors the real resolver: a workspace key names no human, so the billed
+   * account stands in as the explicit system actor; anything else keeps its
+   * owner.
+   */
+  resolveWorkspaceRequestActor: mockResolveWorkspaceRequestActor,
 }))
 
 vi.mock('@/lib/table', () => ({
@@ -92,6 +100,7 @@ describe('DELETE /api/v1/tables/[tableId] — orchestration failure projection',
     vi.clearAllMocks()
     mockCheckRateLimit.mockResolvedValue({ allowed: true, userId: 'user-1', keyType: 'personal' })
     mockCheckWorkspaceScope.mockResolvedValue(null)
+    mockResolveWorkspaceRequestActor.mockResolvedValue('user-1')
     mockGetTableById.mockResolvedValue({
       id: TABLE_ID,
       name: 'Table',

apps/sim/app/api/v1/tables/[tableId]/route.ts

@@ -17,6 +17,7 @@ import {
   checkRateLimit,
   checkWorkspaceScope,
   createRateLimitResponse,
+  resolveWorkspaceRequestActor,
   tableAccessPrincipal,
 } from '@/app/api/v1/middleware'
 
@@ -111,7 +112,6 @@ export const DELETE = withRouteHandler(async (request: NextRequest, context: Tab
       return createRateLimitResponse(rateLimit)
     }
 
-    const userId = rateLimit.userId!
     const parsed = await parseRequest(v1DeleteTableContract, request, context, {
       validationErrorResponse: (error) => {
         const hasInvalidTableId = error.issues.some((issue) => issue.path.includes('tableId'))
@@ -133,14 +133,30 @@ export const DELETE = withRouteHandler(async (request: NextRequest, context: Tab
     const scopeError = await checkWorkspaceScope(rateLimit, workspaceId, 'write')
     if (scopeError) return scopeError
 
+    /**
+     * A workspace key names no human, so its creator must not be attributed the
+     * deletion in audit and analytics. The shared resolver substitutes the
+     * explicit system actor for a workspace key and keeps the owner for a
+     * personal one, exactly as the row routes on this table already do.
+     */
+    const actorUserId = await resolveWorkspaceRequestActor(rateLimit, workspaceId)
+    if (!actorUserId) {
+      throw new Error(`Unable to resolve system actor for workspace ${workspaceId}`)
+    }
+
     const result = await checkAccess(tableId, tableAccessPrincipal(rateLimit), 'write')
     if (!result.ok) return accessError(result, requestId, tableId)
 
     if (result.table.workspaceId !== workspaceId) {
       return NextResponse.json({ error: 'Invalid workspace ID' }, { status: 400 })
     }
 
-    const outcome = await performDeleteTable({ table: result.table, userId, requestId, request })
+    const outcome = await performDeleteTable({
+      table: result.table,
+      userId: actorUserId,
+      requestId,
+      request,
+    })
     if (!outcome.success) {
       return orchestrationOutcomeErrorResponse(outcome, 'Failed to delete table')
     }

apps/sim/app/api/workspaces/route.ts

@@ -10,6 +10,7 @@ import { parseRequest } from '@/lib/api/server'
 import { getSession } from '@/lib/auth'
 import { getActiveOrganizationId } from '@/lib/auth/session-response'
 import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
+import { capabilityRefusalResponse } from '@/lib/permission-groups/capability-response'
 import { captureServerEvent } from '@/lib/posthog/server'
 import { createWorkspace } from '@/lib/workspaces/create'
 import { listWorkspacesForViewer } from '@/lib/workspaces/list'
@@ -183,7 +184,7 @@ export const POST = withRouteHandler(async (req: NextRequest) => {
     return NextResponse.json({ workspace: newWorkspace })
   } catch (error) {
     if (error instanceof WorkspaceCreationCapabilityWithheldError) {
-      return NextResponse.json({ error: error.message }, { status: 403 })
+      return capabilityRefusalResponse('workspace.create')
     }
     if (error instanceof WorkspaceCreationContextChangedError) {
       return NextResponse.json(

apps/sim/background/workflow-column-execution.ts

@@ -627,16 +627,18 @@ async function runWorkflowAndWriteTerminal(
             rowId,
             workspaceId,
             /**
-             * The person who asked, not who pays. For a system-triggered cell
-             * the billing attribution names the workspace's billing owner, and
-             * running a member's tool denylist against a bystander is wrong in
-             * both directions: it fails cells nobody meant to govern, and it
-             * skips the denylist for the person who actually triggered one.
-             * `null` means no per-tool gate applies, which is the documented
-             * behavior for an actorless run — stated, because the field is
-             * required precisely so it cannot be skipped by omission.
+             * The person who asked, not who pays. `triggeredByUserId` is an
+             * attribution: for a workspace-API-key run it names the workspace's
+             * billing owner, and running that bystander's tool denylist against
+             * an actorless request is wrong in both directions — it fails cells
+             * nobody meant to govern, and it skips the denylist for the person
+             * who actually triggered one. The governed subject is carried
+             * separately from the dispatch. `null` means no per-tool gate
+             * applies, which is the documented behavior for an actorless run —
+             * stated, because the field is required precisely so it cannot be
+             * skipped by omission.
              */
-            userId: payload.triggeredByUserId ?? null,
+            userId: payload.capabilityGovernedUserId ?? null,
             signal: attemptSignal,
             resolvedSecretTraceRegistry: enrichmentRegistry,
           })

apps/sim/ee/access-control/utils/permission-check.test.ts

@@ -445,6 +445,26 @@ describe('validateBlockType', () => {
       await validateBlockType('user-123', 'workspace-1', 'slack')
     })
 
+    /**
+     * Registry keys are lowercase, so a mixed-case block type must be folded
+     * *before* the successor lookup. Resolving first makes `getBlock('Slack')`
+     * miss, the successor answer `Slack`, and the comparison fall back to
+     * `slack` — refusing a block the allowlist permits as `slack_v2`.
+     */
+    it('resolves a superseded block supplied with different casing', async () => {
+      setEnterpriseOrgWorkspace()
+      mockGetBlock.mockImplementation((type: string) =>
+        type === 'slack'
+          ? { hideFromToolbar: true, sunset: { status: 'legacy', replacedBy: 'slack_v2' } }
+          : type === 'slack_v2'
+            ? {}
+            : undefined
+      )
+      queueGroupResolution([{ config: { allowedIntegrations: ['slack_v2'] } }])
+
+      await validateBlockType('user-123', 'workspace-1', 'Slack')
+    })
+
     it('still rejects a block absent from a mixed-case stored allowlist', async () => {
       setEnterpriseOrgWorkspace()
       queueGroupResolution([{ config: { allowedIntegrations: ['Slack'] } }])

apps/sim/ee/access-control/utils/permission-check.ts

@@ -270,8 +270,16 @@ function assertBlockTypeAllowed(
    * current block covers every retired version of the same integration. The
    * editor only offers current ids, so without this an admin could not deny a
    * legacy block even knowing it existed.
+   *
+   * Lowercased *before* resolving, not after: registry keys are lowercase, so
+   * `getBlock('Slack')` misses and the successor lookup answers `Slack` — which
+   * then compares as `slack` against an allowlist holding `slack_v2` and
+   * refuses a block both policies allow. `blockType` reaches here from
+   * persisted workflow state and from an agent block's `tool.type`, neither of
+   * which is case-normalized upstream. `toAccessControlAllowlist` normalizes
+   * the policy side the same way.
    */
-  const allowlistType = resolveAccessControlBlockType(blockType).toLowerCase()
+  const allowlistType = resolveAccessControlBlockType(blockType.toLowerCase())
 
   if (!toAccessControlAllowlist(config.allowedIntegrations)?.has(allowlistType)) {
     const envAllowlist = toAccessControlAllowlist(getAllowedIntegrationsFromEnv())

apps/sim/hooks/use-permission-config.ts

@@ -15,9 +15,9 @@ import {
   resolveIntegrationAvailabilityStateForVisibility,
 } from '@/lib/integrations/availability'
 import {
+  intersectAccessControlAllowlists,
   isBlockTypeAccessControlExempt,
   resolveAccessControlBlockType,
-  toAccessControlAllowlist,
 } from '@/lib/permission-groups/block-access'
 import {
   DEFAULT_PERMISSION_GROUP_CONFIG,
@@ -96,10 +96,20 @@ export function usePermissionConfig(): PermissionConfigResult {
    * Both sides of the membership test are judged as the current block, so a
    * policy naming a retired id — `ALLOWED_INTEGRATIONS=slack` — still permits
    * the successor the editor offers.
+   *
+   * Each policy is canonicalized *before* the two are intersected, not after.
+   * `intersectIntegrationAllowlists` case-folds but does not successor-resolve,
+   * so a group naming `slack` and an env allowlist naming `slack_v2` intersect
+   * to nothing textually — hiding an integration both policies allow. Resolving
+   * first puts them in one vocabulary, and the intersection is then exact.
    */
   const allowedAccessControlTypes = useMemo(
-    () => toAccessControlAllowlist(mergedAllowedIntegrations),
-    [mergedAllowedIntegrations]
+    () =>
+      intersectAccessControlAllowlists(
+        config.allowedIntegrations,
+        envAllowlistData?.allowedIntegrations ?? null
+      ),
+    [config.allowedIntegrations, envAllowlistData]
   )
 
   const integrationAvailability = useMemo(() => {