Skip to content

Commit 2d20245

Browse files
committed
fix(api): answer an unresolvable workspace actor and a denied workspace creation with controlled bodies
resolveWorkspaceRequestActor returns null for a reachable request about an unreachable workspace: an authenticated workspace key whose workspace has been archived has no billed account to stand in as its system actor. All five v1 table call sites threw on that, which the routes' catch-all reported as a generic 500. requireWorkspaceRequestActor projects it onto the 400 those routes already use for a workspace mismatch, from one place rather than five copies. POST /api/workspaces refused a permission-group denial with two different bodies. The revocation race the insert detects carried details.code PERMISSION_GROUP_CAPABILITY_BLOCKED; the far more common preflight denial answered a bare { error }, so a client keying off the code saw the capability refusal only in the rarer case. Both now render through capabilityRefusalResponse.
1 parent 6e1b053 commit 2d20245

8 files changed

Lines changed: 210 additions & 35 deletions

File tree

apps/sim/app/api/v1/middleware.ts

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -532,6 +532,30 @@ export async function resolveWorkspaceRequestActor(
532532
return rateLimit.userId ?? null
533533
}
534534

535+
/**
536+
* {@link resolveWorkspaceRequestActor} as a route-ready result.
537+
*
538+
* The resolver answers `null` for a real, reachable request: an authenticated
539+
* workspace key whose workspace has since been archived or deleted has no
540+
* billed account to stand in as its system actor. Every call site used to
541+
* `throw` on that, which the route's catch-all turned into a generic 500 — an
542+
* unreachable workspace reported as a server fault. It is the same condition
543+
* the routes already report as a 400 `Invalid workspace ID` when the addressed
544+
* table belongs to another workspace, so it is reported the same way, from one
545+
* place, rather than five copies of a throw.
546+
*/
547+
export async function requireWorkspaceRequestActor(
548+
rateLimit: RateLimitResult,
549+
workspaceId: string
550+
): Promise<{ ok: true; actorUserId: string } | { ok: false; response: NextResponse }> {
551+
const actorUserId = await resolveWorkspaceRequestActor(rateLimit, workspaceId)
552+
if (actorUserId) return { ok: true, actorUserId }
553+
return {
554+
ok: false,
555+
response: NextResponse.json({ error: 'Invalid workspace ID' }, { status: 400 }),
556+
}
557+
}
558+
535559
/**
536560
* v1 wrapper: renders {@link resolveWorkspaceAccess} as the v1 `{ error }` body.
537561
* Returns null on success, NextResponse on failure.

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

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,9 +46,20 @@ vi.mock('@/app/api/v1/middleware', () => ({
4646
/**
4747
* Mirrors the real resolver: a workspace key names no human, so the billed
4848
* account stands in as the explicit system actor; anything else keeps its
49-
* owner.
49+
* owner. The route reads it through `requireWorkspaceRequestActor`, which
50+
* projects an unresolvable actor onto a 400 instead of throwing, so the mock
51+
* reproduces that projection rather than only the raw resolver.
5052
*/
5153
resolveWorkspaceRequestActor: mockResolveWorkspaceRequestActor,
54+
requireWorkspaceRequestActor: async (rateLimit: unknown, workspaceId: string) => {
55+
const actorUserId = await mockResolveWorkspaceRequestActor(rateLimit, workspaceId)
56+
return actorUserId
57+
? { ok: true, actorUserId }
58+
: {
59+
ok: false,
60+
response: NextResponse.json({ error: 'Invalid workspace ID' }, { status: 400 }),
61+
}
62+
},
5263
}))
5364

5465
vi.mock('@/lib/table', () => ({
@@ -109,6 +120,23 @@ describe('DELETE /api/v1/tables/[tableId] — orchestration failure projection',
109120
mockGetUserEntityPermissions.mockResolvedValue('admin')
110121
})
111122

123+
/**
124+
* A workspace key whose workspace has since been archived resolves no billed
125+
* account, so there is no system actor to attribute the deletion to. That is
126+
* a reachable request about an unreachable workspace, not a server fault: it
127+
* used to `throw`, and the catch-all reported it as a 500.
128+
*/
129+
it('reports an unresolvable workspace actor as a 400, not a 500', async () => {
130+
mockCheckRateLimit.mockResolvedValue({ allowed: true, userId: 'user-1', keyType: 'workspace' })
131+
mockResolveWorkspaceRequestActor.mockResolvedValue(null)
132+
133+
const response = await DELETE(makeRequest(), makeContext())
134+
135+
expect(response.status).toBe(400)
136+
expect(await response.json()).toEqual({ error: 'Invalid workspace ID' })
137+
expect(mockPerformDeleteTable).not.toHaveBeenCalled()
138+
})
139+
112140
it('renders an unclassified internal failure as a fixed generic message', async () => {
113141
mockPerformDeleteTable.mockResolvedValue({
114142
success: false,

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

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ import {
1717
checkRateLimit,
1818
checkWorkspaceScope,
1919
createRateLimitResponse,
20-
resolveWorkspaceRequestActor,
20+
requireWorkspaceRequestActor,
2121
tableAccessPrincipal,
2222
} from '@/app/api/v1/middleware'
2323

@@ -137,12 +137,14 @@ export const DELETE = withRouteHandler(async (request: NextRequest, context: Tab
137137
* A workspace key names no human, so its creator must not be attributed the
138138
* deletion in audit and analytics. The shared resolver substitutes the
139139
* explicit system actor for a workspace key and keeps the owner for a
140-
* personal one, exactly as the row routes on this table already do.
140+
* personal one, exactly as the row routes on this table already do. An
141+
* archived or deleted workspace has no billed account to stand in, which is
142+
* a controlled 400 rather than an uncaught throw the catch-all would report
143+
* as a 500.
141144
*/
142-
const actorUserId = await resolveWorkspaceRequestActor(rateLimit, workspaceId)
143-
if (!actorUserId) {
144-
throw new Error(`Unable to resolve system actor for workspace ${workspaceId}`)
145-
}
145+
const actor = await requireWorkspaceRequestActor(rateLimit, workspaceId)
146+
if (!actor.ok) return actor.response
147+
const actorUserId = actor.actorUserId
146148

147149
const result = await checkAccess(tableId, tableAccessPrincipal(rateLimit), 'write')
148150
if (!result.ok) return accessError(result, requestId, tableId)

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

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ import {
2929
checkRateLimit,
3030
checkWorkspaceScope,
3131
createRateLimitResponse,
32-
resolveWorkspaceRequestActor,
32+
requireWorkspaceRequestActor,
3333
tableAccessPrincipal,
3434
v1ValidationErrorResponse,
3535
v1ValidationErrorResponseFromError,
@@ -134,10 +134,9 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: RowR
134134

135135
const scopeError = await checkWorkspaceScope(rateLimit, validated.workspaceId, 'write')
136136
if (scopeError) return scopeError
137-
const actorUserId = await resolveWorkspaceRequestActor(rateLimit, validated.workspaceId)
138-
if (!actorUserId) {
139-
throw new Error(`Unable to resolve system actor for workspace ${validated.workspaceId}`)
140-
}
137+
const actor = await requireWorkspaceRequestActor(rateLimit, validated.workspaceId)
138+
if (!actor.ok) return actor.response
139+
const actorUserId = actor.actorUserId
141140

142141
const result = await checkAccess(tableId, tableAccessPrincipal(rateLimit), 'write')
143142
if (!result.ok) return accessError(result, requestId, tableId)

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

Lines changed: 10 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ import {
4343
checkRateLimit,
4444
checkWorkspaceScope,
4545
createRateLimitResponse,
46-
resolveWorkspaceRequestActor,
46+
requireWorkspaceRequestActor,
4747
tableAccessPrincipal,
4848
v1ValidationErrorResponse,
4949
v1ValidationErrorResponseFromError,
@@ -239,15 +239,9 @@ export const POST = withRouteHandler(
239239
const batchValidated = parsed.data.body
240240
const scopeError = await checkWorkspaceScope(rateLimit, batchValidated.workspaceId, 'write')
241241
if (scopeError) return scopeError
242-
const actorUserId = await resolveWorkspaceRequestActor(
243-
rateLimit,
244-
batchValidated.workspaceId
245-
)
246-
if (!actorUserId) {
247-
throw new Error(
248-
`Unable to resolve system actor for workspace ${batchValidated.workspaceId}`
249-
)
250-
}
242+
const batchActor = await requireWorkspaceRequestActor(rateLimit, batchValidated.workspaceId)
243+
if (!batchActor.ok) return batchActor.response
244+
const actorUserId = batchActor.actorUserId
251245
return handleBatchInsert(
252246
requestId,
253247
tableId,
@@ -261,10 +255,9 @@ export const POST = withRouteHandler(
261255

262256
const scopeError = await checkWorkspaceScope(rateLimit, validated.workspaceId, 'write')
263257
if (scopeError) return scopeError
264-
const actorUserId = await resolveWorkspaceRequestActor(rateLimit, validated.workspaceId)
265-
if (!actorUserId) {
266-
throw new Error(`Unable to resolve system actor for workspace ${validated.workspaceId}`)
267-
}
258+
const actor = await requireWorkspaceRequestActor(rateLimit, validated.workspaceId)
259+
if (!actor.ok) return actor.response
260+
const actorUserId = actor.actorUserId
268261

269262
const accessResult = await checkAccess(tableId, tableAccessPrincipal(rateLimit), 'write')
270263
if (!accessResult.ok) return accessError(accessResult, requestId, tableId)
@@ -344,10 +337,9 @@ export const PUT = withRouteHandler(async (request: NextRequest, context: TableR
344337

345338
const scopeError = await checkWorkspaceScope(rateLimit, validated.workspaceId, 'write')
346339
if (scopeError) return scopeError
347-
const actorUserId = await resolveWorkspaceRequestActor(rateLimit, validated.workspaceId)
348-
if (!actorUserId) {
349-
throw new Error(`Unable to resolve system actor for workspace ${validated.workspaceId}`)
350-
}
340+
const actor = await requireWorkspaceRequestActor(rateLimit, validated.workspaceId)
341+
if (!actor.ok) return actor.response
342+
const actorUserId = actor.actorUserId
351343

352344
const accessResult = await checkAccess(tableId, tableAccessPrincipal(rateLimit), 'write')
353345
if (!accessResult.ok) return accessError(accessResult, requestId, tableId)

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

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ import {
2020
checkRateLimit,
2121
checkWorkspaceScope,
2222
createRateLimitResponse,
23-
resolveWorkspaceRequestActor,
23+
requireWorkspaceRequestActor,
2424
tableAccessPrincipal,
2525
v1ValidationErrorResponse,
2626
v1ValidationErrorResponseFromError,
@@ -54,10 +54,9 @@ export const POST = withRouteHandler(async (request: NextRequest, context: Upser
5454

5555
const scopeError = await checkWorkspaceScope(rateLimit, validated.workspaceId, 'write')
5656
if (scopeError) return scopeError
57-
const actorUserId = await resolveWorkspaceRequestActor(rateLimit, validated.workspaceId)
58-
if (!actorUserId) {
59-
throw new Error(`Unable to resolve system actor for workspace ${validated.workspaceId}`)
60-
}
57+
const actor = await requireWorkspaceRequestActor(rateLimit, validated.workspaceId)
58+
if (!actor.ok) return actor.response
59+
const actorUserId = actor.actorUserId
6160

6261
const result = await checkAccess(tableId, tableAccessPrincipal(rateLimit), 'write')
6362
if (!result.ok) return accessError(result, requestId, tableId)
Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,119 @@
1+
/**
2+
* @vitest-environment node
3+
*
4+
* POST /api/workspaces refuses a workspace-creation-denied group at two
5+
* moments: the preflight policy read, and the revocation race the insert
6+
* detects. Both are the same decision, so both must produce the same body —
7+
* the preflight one used to answer a bare `{ error }` with no
8+
* `details.code`, so a client keying off the code saw the capability refusal
9+
* only in the rarer case.
10+
*/
11+
import { createMockRequest } from '@sim/testing'
12+
import { beforeEach, describe, expect, it, vi } from 'vitest'
13+
14+
const { mockGetSession, mockGetWorkspaceCreationPolicy, mockCreateWorkspace } = vi.hoisted(() => ({
15+
mockGetSession: vi.fn(),
16+
mockGetWorkspaceCreationPolicy: vi.fn(),
17+
mockCreateWorkspace: vi.fn(),
18+
}))
19+
20+
vi.mock('@/lib/auth', () => ({
21+
auth: { api: { getSession: vi.fn() } },
22+
getSession: mockGetSession,
23+
}))
24+
25+
vi.mock('@/lib/auth/session-response', () => ({
26+
getActiveOrganizationId: () => null,
27+
}))
28+
29+
vi.mock('@/lib/workspaces/create', () => ({
30+
createWorkspace: mockCreateWorkspace,
31+
}))
32+
33+
vi.mock('@/lib/workspaces/list', () => ({
34+
listWorkspacesForViewer: vi.fn(),
35+
}))
36+
37+
vi.mock('@/lib/posthog/server', () => ({
38+
captureServerEvent: vi.fn(),
39+
}))
40+
41+
vi.mock('@sim/audit', () => ({
42+
recordAudit: vi.fn(),
43+
AuditAction: { WORKSPACE_CREATED: 'workspace.created' },
44+
AuditResourceType: { WORKSPACE: 'workspace' },
45+
}))
46+
47+
vi.mock('@/lib/workspaces/policy', async () => {
48+
class WorkspaceCreationCapabilityWithheldError extends Error {}
49+
class WorkspaceCreationContextChangedError extends Error {}
50+
return {
51+
getWorkspaceCreationPolicy: mockGetWorkspaceCreationPolicy,
52+
WorkspaceCreationCapabilityWithheldError,
53+
WorkspaceCreationContextChangedError,
54+
}
55+
})
56+
57+
import { WorkspaceCreationCapabilityWithheldError } from '@/lib/workspaces/policy'
58+
import { POST } from '@/app/api/workspaces/route'
59+
60+
function createRequest() {
61+
return createMockRequest('POST', { name: 'New workspace' })
62+
}
63+
64+
const deniedPolicy = {
65+
canCreate: false,
66+
status: 403,
67+
reason: 'Your permission group does not allow creating workspaces.',
68+
blockedReasonCode: 'permission-group-denied',
69+
}
70+
71+
describe('POST /api/workspaces capability refusal', () => {
72+
beforeEach(() => {
73+
vi.clearAllMocks()
74+
mockGetSession.mockResolvedValue({
75+
user: { id: 'user-1', name: 'A', email: 'a@example.com' },
76+
})
77+
})
78+
79+
it('answers the preflight denial with the capability refusal envelope', async () => {
80+
mockGetWorkspaceCreationPolicy.mockResolvedValue(deniedPolicy)
81+
82+
const response = await POST(createRequest())
83+
84+
expect(response.status).toBe(403)
85+
expect(await response.json()).toMatchObject({
86+
details: { code: 'PERMISSION_GROUP_CAPABILITY_BLOCKED' },
87+
})
88+
expect(mockCreateWorkspace).not.toHaveBeenCalled()
89+
})
90+
91+
it('answers the revocation race with the same envelope', async () => {
92+
mockGetWorkspaceCreationPolicy.mockResolvedValue({ canCreate: true, status: 200 })
93+
mockCreateWorkspace.mockRejectedValue(new WorkspaceCreationCapabilityWithheldError())
94+
95+
const response = await POST(createRequest())
96+
97+
expect(response.status).toBe(403)
98+
expect(await response.json()).toMatchObject({
99+
details: { code: 'PERMISSION_GROUP_CAPABILITY_BLOCKED' },
100+
})
101+
})
102+
103+
/** A non-capability block keeps its own reason and status. */
104+
it('leaves an unrelated policy refusal alone', async () => {
105+
mockGetWorkspaceCreationPolicy.mockResolvedValue({
106+
canCreate: false,
107+
status: 402,
108+
reason: 'Your organization subscription is inactive.',
109+
blockedReasonCode: 'organization-subscription-inactive',
110+
})
111+
112+
const response = await POST(createRequest())
113+
114+
expect(response.status).toBe(402)
115+
const body = await response.json()
116+
expect(body.error).toBe('Your organization subscription is inactive.')
117+
expect(body.details).toBeUndefined()
118+
})
119+
})

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -130,6 +130,18 @@ export const POST = withRouteHandler(async (req: NextRequest) => {
130130
})
131131

132132
if (!creationPolicy.canCreate) {
133+
/**
134+
* The preflight refusal and the revocation-race refusal are the same
135+
* decision reached at two moments, so they must be the same body. Without
136+
* this branch the common path — the group already denied `workspace.create`
137+
* when the policy was read — answered a bare `{ error }`, while only the
138+
* race the `catch` below handles carried
139+
* `details.code: PERMISSION_GROUP_CAPABILITY_BLOCKED`. A client that keys
140+
* off the code then saw the capability refusal in the rarer case only.
141+
*/
142+
if (creationPolicy.blockedReasonCode === 'permission-group-denied') {
143+
return capabilityRefusalResponse('workspace.create')
144+
}
133145
return NextResponse.json(
134146
{ error: creationPolicy.reason || 'Workspace creation is not available.' },
135147
{ status: creationPolicy.status }

0 commit comments

Comments
 (0)