Skip to content

Commit d0519c1

Browse files
authored
fix(security): supabase rpc path validation, ssh stream byte cap, storage quota coverage (#4605)
* fix(security): supabase rpc path validation, ssh stream byte cap, storage quota coverage * fix(security): scope execution log writes to owning workflow; add env-var workspace membership guard Closes two cross-tenant vulnerabilities: 1. Workflow log cross-tenant write (route.ts + logging-session.ts): - Route: SELECT before creating LoggingSession to verify executionId belongs to the claimed workflowId; reject with 404 if owned by a different workflow. - LoggingSession: add workflow_id to all UPDATE/SELECT WHERE clauses (raw SQL marker queries, flushAccumulatedCost, loadExistingCost) so writes are a no-op if executionId was somehow injected. 2. Env-var workspace membership guard (environment/utils.ts): - getPersonalAndWorkspaceEnv now calls checkWorkspaceAccess when workspaceId is provided; throws if the userId is not a member, preventing any future caller from reading another workspace's decrypted secrets without explicit membership verification at the call site. * fix(security): remove fileSize > 0 quota bypass gate; exempt logs context from quota * chore: remove extraneous inline comments * fix(security): scope markExecutionAsFailed UPDATE by workflowId; thread workflowId through HITL callers * fix(security): add personal credential ownership check in sharepoint site route; scope markExecutionAsFailed by workflowId * fix: remove logs from user-accessible upload contexts; restore distinct biome .next glob * fix(sharepoint): migrate site route to authorizeCredentialUse The previous fix only checked userId equality for personal credentials and workspace membership (via getUserEntityPermissions) for workspace credentials. authorizeCredentialUse additionally enforces credentialMember access for workspace-scoped credentials, matching the standard pattern used by all other tool selector routes. * fix(logging): make workflowId required in markExecutionAsFailed Making workflowId optional left a footgun — future callers could silently omit it and the WHERE clause would degrade to executionId-only, losing the cross-tenant scoping guarantee. All callers already supply workflowId, so making it required (with string | undefined for the middle params to keep call sites unchanged) closes the gap without touching any caller. * test(security): add tests for cross-tenant log guard, quota bypass fix, and workflowId scoping - log/route.test.ts: verifies cross-tenant executionId guard returns 404 when the execution belongs to a different workflow, and passes for same workflow or fresh executions - multipart/route.test.ts: verifies fileSize:0 no longer bypasses quota check and that the logs context is rejected at the endpoint level - logging-session.test.ts: verifies markExecutionAsFailed scopes by both executionId and workflowId, and that the instance method forwards workflowId * fix(lint): move IconComponent outside ToolInput to fix noNestedComponentDefinitions * fix(logging): scope completeWithCancellation and completeWithPause reads by workflowId Both SELECT queries that check execution status before writing a terminal result were only filtering on executionId. Adds workflowId to the WHERE clause so all seven reads and writes in LoggingSession consistently scope by (workflowId, executionId).
1 parent 11fa96c commit d0519c1

14 files changed

Lines changed: 360 additions & 76 deletions

File tree

apps/sim/app/api/files/multipart/route.test.ts

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,15 @@ vi.mock('@/lib/uploads/providers/blob/client', () => ({
5353

5454
vi.mock('@/lib/workspaces/permissions/utils', () => permissionsMock)
5555

56+
const { mockCheckStorageQuota, mockInitiateS3MultipartUpload } = vi.hoisted(() => ({
57+
mockCheckStorageQuota: vi.fn(),
58+
mockInitiateS3MultipartUpload: vi.fn(),
59+
}))
60+
61+
vi.mock('@/lib/billing/storage', () => ({
62+
checkStorageQuota: mockCheckStorageQuota,
63+
}))
64+
5665
import { POST } from '@/app/api/files/multipart/route'
5766

5867
const tokenPayload = {
@@ -200,3 +209,69 @@ describe('POST /api/files/multipart action=complete', () => {
200209
expect(mockCompleteS3MultipartUpload).toHaveBeenCalledTimes(2)
201210
})
202211
})
212+
213+
describe('POST /api/files/multipart action=initiate quota enforcement', () => {
214+
const makeInitiateRequest = (body: unknown) =>
215+
new NextRequest('http://localhost/api/files/multipart?action=initiate', {
216+
method: 'POST',
217+
headers: { 'Content-Type': 'application/json' },
218+
body: JSON.stringify(body),
219+
})
220+
221+
beforeEach(() => {
222+
vi.clearAllMocks()
223+
authMockFns.mockGetSession.mockResolvedValue({ user: { id: 'user-1' } })
224+
permissionsMockFns.mockGetUserEntityPermissions.mockResolvedValue('write')
225+
mockIsUsingCloudStorage.mockReturnValue(true)
226+
mockGetStorageProvider.mockReturnValue('s3')
227+
mockGetStorageConfig.mockReturnValue({ bucket: 'b', region: 'r' })
228+
mockSignUploadToken.mockReturnValue('signed-token')
229+
mockCheckStorageQuota.mockResolvedValue({ allowed: true })
230+
mockInitiateS3MultipartUpload.mockResolvedValue({ uploadId: 'up-1', key: 'k/file.bin' })
231+
})
232+
233+
it('blocks upload when fileSize: 0 exceeds quota', async () => {
234+
mockCheckStorageQuota.mockResolvedValue({ allowed: false, error: 'Storage limit exceeded' })
235+
236+
const res = await makeInitiateRequest({
237+
fileName: 'file.bin',
238+
contentType: 'application/octet-stream',
239+
fileSize: 0,
240+
workspaceId: 'ws-1',
241+
context: 'knowledge-base',
242+
})
243+
244+
const response = await POST(res)
245+
expect(response.status).toBe(413)
246+
const body = await response.json()
247+
expect(body.error).toContain('Storage limit exceeded')
248+
})
249+
250+
it('does not check quota for quota-exempt contexts (og-images)', async () => {
251+
const res = await makeInitiateRequest({
252+
fileName: 'img.png',
253+
contentType: 'image/png',
254+
fileSize: 99999,
255+
workspaceId: 'ws-1',
256+
context: 'og-images',
257+
})
258+
259+
const response = await POST(res)
260+
expect(mockCheckStorageQuota).not.toHaveBeenCalled()
261+
})
262+
263+
it('rejects logs context — not allowed via the multipart endpoint', async () => {
264+
const res = await makeInitiateRequest({
265+
fileName: 'exec.log',
266+
contentType: 'text/plain',
267+
fileSize: 1000,
268+
workspaceId: 'ws-1',
269+
context: 'logs',
270+
})
271+
272+
const response = await POST(res)
273+
expect(response.status).toBe(400)
274+
const body = await response.json()
275+
expect(body.error).toMatch(/invalid storage context/i)
276+
})
277+
})

apps/sim/app/api/files/multipart/route.ts

Lines changed: 15 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ import {
2222
type UploadTokenPayload,
2323
verifyUploadToken,
2424
} from '@/lib/uploads/core/upload-token'
25-
import type { StorageConfig } from '@/lib/uploads/shared/types'
25+
import { QUOTA_EXEMPT_STORAGE_CONTEXTS, type StorageConfig } from '@/lib/uploads/shared/types'
2626
import { getUserEntityPermissions } from '@/lib/workspaces/permissions/utils'
2727

2828
const logger = createLogger('MultipartUploadAPI')
@@ -36,7 +36,6 @@ const ALLOWED_UPLOAD_CONTEXTS = new Set<StorageContext>([
3636
'workspace',
3737
'profile-pictures',
3838
'og-images',
39-
'logs',
4039
'workspace-logos',
4140
])
4241

@@ -135,6 +134,20 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
135134

136135
const config = getStorageConfig(storageContext)
137136

137+
if (
138+
!QUOTA_EXEMPT_STORAGE_CONTEXTS.has(context as StorageContext) &&
139+
typeof fileSize === 'number'
140+
) {
141+
const { checkStorageQuota } = await import('@/lib/billing/storage')
142+
const quotaCheck = await checkStorageQuota(userId, fileSize)
143+
if (!quotaCheck.allowed) {
144+
return NextResponse.json(
145+
{ error: quotaCheck.error || 'Storage limit exceeded' },
146+
{ status: 413 }
147+
)
148+
}
149+
}
150+
138151
let customKey: string | undefined
139152
if (context === 'workspace' || context === 'mothership') {
140153
const { MAX_WORKSPACE_FILE_SIZE } = await import('@/lib/uploads/shared/types')
@@ -149,15 +162,6 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
149162
'@/lib/uploads/contexts/workspace/workspace-file-manager'
150163
)
151164
customKey = generateWorkspaceFileKey(workspaceId, fileName)
152-
153-
const { checkStorageQuota } = await import('@/lib/billing/storage')
154-
const quotaCheck = await checkStorageQuota(userId, fileSize)
155-
if (!quotaCheck.allowed) {
156-
return NextResponse.json(
157-
{ error: quotaCheck.error || 'Storage limit exceeded' },
158-
{ status: 413 }
159-
)
160-
}
161165
} else if (context === 'execution') {
162166
const workflowId = (data as { workflowId?: unknown }).workflowId
163167
const executionId = (data as { executionId?: unknown }).executionId

apps/sim/app/api/tools/sharepoint/site/route.ts

Lines changed: 7 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,12 @@
1-
import { db } from '@sim/db'
2-
import { account } from '@sim/db/schema'
31
import { createLogger } from '@sim/logger'
42
import { generateId } from '@sim/utils/id'
5-
import { eq } from 'drizzle-orm'
63
import { type NextRequest, NextResponse } from 'next/server'
74
import { sharepointSiteQuerySchema } from '@/lib/api/contracts/selectors/sharepoint'
85
import { getValidationErrorMessage } from '@/lib/api/server'
9-
import { getSession } from '@/lib/auth'
6+
import { authorizeCredentialUse } from '@/lib/auth/credential-access'
107
import { validateMicrosoftGraphId } from '@/lib/core/security/input-validation'
118
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
12-
import { refreshAccessTokenIfNeeded, resolveOAuthAccountId } from '@/app/api/auth/oauth/utils'
9+
import { refreshAccessTokenIfNeeded } from '@/app/api/auth/oauth/utils'
1310

1411
export const dynamic = 'force-dynamic'
1512

@@ -19,11 +16,6 @@ export const GET = withRouteHandler(async (request: NextRequest) => {
1916
const requestId = generateId().slice(0, 8)
2017

2118
try {
22-
const session = await getSession()
23-
if (!session?.user?.id) {
24-
return NextResponse.json({ error: 'User not authenticated' }, { status: 401 })
25-
}
26-
2719
const { searchParams } = new URL(request.url)
2820
const validation = sharepointSiteQuerySchema.safeParse({
2921
credentialId: searchParams.get('credentialId') ?? '',
@@ -42,37 +34,14 @@ export const GET = withRouteHandler(async (request: NextRequest) => {
4234
return NextResponse.json({ error: siteIdValidation.error }, { status: 400 })
4335
}
4436

45-
const resolved = await resolveOAuthAccountId(credentialId)
46-
if (!resolved) {
47-
return NextResponse.json({ error: 'Credential not found' }, { status: 404 })
37+
const authz = await authorizeCredentialUse(request, { credentialId })
38+
if (!authz.ok || !authz.credentialOwnerUserId || !authz.resolvedCredentialId) {
39+
return NextResponse.json({ error: authz.error || 'Unauthorized' }, { status: 403 })
4840
}
4941

50-
if (resolved.workspaceId) {
51-
const { getUserEntityPermissions } = await import('@/lib/workspaces/permissions/utils')
52-
const perm = await getUserEntityPermissions(
53-
session.user.id,
54-
'workspace',
55-
resolved.workspaceId
56-
)
57-
if (perm === null) {
58-
return NextResponse.json({ error: 'Forbidden' }, { status: 403 })
59-
}
60-
}
61-
62-
const credentials = await db
63-
.select()
64-
.from(account)
65-
.where(eq(account.id, resolved.accountId))
66-
.limit(1)
67-
if (!credentials.length) {
68-
return NextResponse.json({ error: 'Credential not found' }, { status: 404 })
69-
}
70-
71-
const accountRow = credentials[0]
72-
7342
const accessToken = await refreshAccessTokenIfNeeded(
74-
resolved.accountId,
75-
accountRow.userId,
43+
authz.resolvedCredentialId,
44+
authz.credentialOwnerUserId,
7645
requestId
7746
)
7847
if (!accessToken) {

apps/sim/app/api/tools/ssh/read-file-content/route.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,9 +73,16 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
7373

7474
const content = await new Promise<string>((resolve, reject) => {
7575
const chunks: Buffer[] = []
76+
let totalBytes = 0
7677
const readStream = sftp.createReadStream(filePath)
7778

7879
readStream.on('data', (chunk: Buffer) => {
80+
totalBytes += chunk.length
81+
if (totalBytes > maxBytes) {
82+
readStream.destroy()
83+
reject(new Error(`File exceeds maximum allowed size of ${params.maxSize}MB`))
84+
return
85+
}
7986
chunks.push(chunk)
8087
})
8188

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,108 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import { authMockFns, dbChainMock, dbChainMockFns, resetDbChainMock } from '@sim/testing'
5+
import { NextRequest } from 'next/server'
6+
import { beforeEach, describe, expect, it, vi } from 'vitest'
7+
8+
// Override global db mock with the configurable chain mock
9+
vi.mock('@sim/db', () => dbChainMock)
10+
11+
const { mockValidateWorkflowAccess, mockGetWorkspaceBilledAccountUserId } = vi.hoisted(() => ({
12+
mockValidateWorkflowAccess: vi.fn(),
13+
mockGetWorkspaceBilledAccountUserId: vi.fn(),
14+
}))
15+
16+
vi.mock('@/app/api/workflows/middleware', () => ({
17+
validateWorkflowAccess: mockValidateWorkflowAccess,
18+
}))
19+
20+
vi.mock('@/lib/workspaces/utils', () => ({
21+
getWorkspaceBilledAccountUserId: mockGetWorkspaceBilledAccountUserId,
22+
}))
23+
24+
vi.mock('@/lib/logs/execution/logging-session', () => ({
25+
LoggingSession: vi.fn().mockImplementation(() => ({
26+
start: vi.fn().mockResolvedValue(undefined),
27+
markAsFailed: vi.fn().mockResolvedValue(undefined),
28+
safeCompleteWithError: vi.fn().mockResolvedValue(undefined),
29+
safeComplete: vi.fn().mockResolvedValue(undefined),
30+
})),
31+
}))
32+
33+
vi.mock('@/lib/logs/execution/trace-spans/trace-spans', () => ({
34+
buildTraceSpans: vi.fn().mockReturnValue([]),
35+
}))
36+
37+
import { POST } from './route'
38+
39+
const makeRequest = (workflowId: string, body: unknown) =>
40+
new NextRequest(`http://localhost/api/workflows/${workflowId}/log`, {
41+
method: 'POST',
42+
headers: { 'Content-Type': 'application/json' },
43+
body: JSON.stringify(body),
44+
})
45+
46+
const validResult = { success: true, output: { value: 42 } }
47+
48+
describe('POST /api/workflows/[id]/log cross-tenant guard', () => {
49+
const OWNER_WORKFLOW_ID = 'wf-owner'
50+
const ATTACKER_WORKFLOW_ID = 'wf-attacker'
51+
const VICTIM_EXECUTION_ID = 'exec-victim-uuid'
52+
53+
beforeEach(() => {
54+
vi.clearAllMocks()
55+
resetDbChainMock()
56+
authMockFns.mockGetSession.mockResolvedValue({ user: { id: 'user-1' } })
57+
mockValidateWorkflowAccess.mockResolvedValue({ error: null })
58+
mockGetWorkspaceBilledAccountUserId.mockResolvedValue('user-1')
59+
// Default: no existing log (fresh execution)
60+
dbChainMockFns.limit.mockResolvedValue([])
61+
})
62+
63+
it('returns 404 when executionId belongs to a different workflow', async () => {
64+
dbChainMockFns.limit.mockResolvedValueOnce([{ workflowId: OWNER_WORKFLOW_ID }])
65+
66+
const res = await POST(
67+
makeRequest(ATTACKER_WORKFLOW_ID, {
68+
executionId: VICTIM_EXECUTION_ID,
69+
result: validResult,
70+
}),
71+
{ params: Promise.resolve({ id: ATTACKER_WORKFLOW_ID }) }
72+
)
73+
74+
expect(res.status).toBe(404)
75+
const body = await res.json()
76+
expect(body.error).toBe('Execution not found')
77+
})
78+
79+
it('proceeds when executionId belongs to the same workflow', async () => {
80+
dbChainMockFns.limit.mockResolvedValueOnce([{ workflowId: OWNER_WORKFLOW_ID }])
81+
82+
const res = await POST(
83+
makeRequest(OWNER_WORKFLOW_ID, {
84+
executionId: VICTIM_EXECUTION_ID,
85+
result: validResult,
86+
}),
87+
{ params: Promise.resolve({ id: OWNER_WORKFLOW_ID }) }
88+
)
89+
90+
expect(res.status).not.toBe(404)
91+
expect(res.status).not.toBe(403)
92+
})
93+
94+
it('proceeds when executionId has no existing log row (fresh execution)', async () => {
95+
dbChainMockFns.limit.mockResolvedValueOnce([])
96+
97+
const res = await POST(
98+
makeRequest(OWNER_WORKFLOW_ID, {
99+
executionId: 'brand-new-execution-id',
100+
result: validResult,
101+
}),
102+
{ params: Promise.resolve({ id: OWNER_WORKFLOW_ID }) }
103+
)
104+
105+
expect(res.status).not.toBe(404)
106+
expect(res.status).not.toBe(403)
107+
})
108+
})

apps/sim/app/api/workflows/[id]/log/route.ts

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,7 @@
1+
import { db } from '@sim/db'
2+
import { workflowExecutionLogs } from '@sim/db/schema'
13
import { createLogger } from '@sim/logger'
4+
import { eq } from 'drizzle-orm'
25
import type { NextRequest } from 'next/server'
36
import { workflowLogContract } from '@/lib/api/contracts/workflows'
47
import { parseRequest } from '@/lib/api/server'
@@ -40,6 +43,19 @@ export const POST = withRouteHandler(
4043
return createErrorResponse('executionId is required when logging results', 400)
4144
}
4245

46+
const [existingLog] = await db
47+
.select({ workflowId: workflowExecutionLogs.workflowId })
48+
.from(workflowExecutionLogs)
49+
.where(eq(workflowExecutionLogs.executionId, executionId))
50+
.limit(1)
51+
52+
if (existingLog && existingLog.workflowId !== id) {
53+
logger.warn(
54+
`[${requestId}] executionId ${executionId} belongs to workflow ${existingLog.workflowId}, not ${id}`
55+
)
56+
return createErrorResponse('Execution not found', 404)
57+
}
58+
4359
logger.info(`[${requestId}] Persisting execution result for workflow: ${id}`, {
4460
executionId,
4561
success: result.success,

0 commit comments

Comments
 (0)