Skip to content

Commit 2d86b7b

Browse files
committed
fix(security): address PR review comments and harden deepsec fixes
- fix(env): replace jsonb operators with transaction+FOR UPDATE read-modify-write - PUT: uses db.transaction + SELECT FOR UPDATE + JS merge to avoid lost-update race - DELETE: same pattern; fixes variable scope bug where current was referenced outside tx - removes broken || and - jsonb operators that fail on json-typed column - fix(ssh): trim truncated output consistently with non-truncated path - fix(gmail): remove redundant resolveOAuthAccountId call - adds credentialType field to CredentialAccessResult - authorizeCredentialUse now returns credentialType in all success paths - gmail/labels route uses authz.credentialType and authz.resolvedCredentialId directly - fix(supabase): centralize table identifier validation - adds validateDatabaseIdentifier() to input-validation.ts - all 8 supabase tools use the shared util instead of inline regex
1 parent 7b6e43d commit 2d86b7b

13 files changed

Lines changed: 126 additions & 87 deletions

File tree

apps/sim/app/api/tools/gmail/labels/route.ts

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@ import { getScopesForService } from '@/lib/oauth/utils'
1010
import {
1111
getServiceAccountToken,
1212
refreshAccessTokenIfNeeded,
13-
resolveOAuthAccountId,
1413
ServiceAccountTokenError,
1514
} from '@/app/api/auth/oauth/utils'
1615
export const dynamic = 'force-dynamic'
@@ -44,20 +43,15 @@ export const GET = withRouteHandler(async (request: NextRequest) => {
4443
credentialId,
4544
requireWorkflowIdForInternal: false,
4645
})
47-
if (!authz.ok || !authz.credentialOwnerUserId) {
46+
if (!authz.ok || !authz.credentialOwnerUserId || !authz.resolvedCredentialId) {
4847
return NextResponse.json({ error: authz.error || 'Unauthorized' }, { status: 403 })
4948
}
5049

51-
const resolved = await resolveOAuthAccountId(credentialId)
52-
if (!resolved) {
53-
return NextResponse.json({ error: 'Credential not found' }, { status: 404 })
54-
}
55-
5650
let accessToken: string | null = null
5751

58-
if (resolved.credentialType === 'service_account' && resolved.credentialId) {
52+
if (authz.credentialType === 'service_account') {
5953
accessToken = await getServiceAccountToken(
60-
resolved.credentialId,
54+
authz.resolvedCredentialId,
6155
getScopesForService('gmail'),
6256
impersonateEmail
6357
)

apps/sim/app/api/tools/ssh/utils.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -197,10 +197,10 @@ export function executeSSHCommand(client: Client, command: string): Promise<SSHC
197197
stream.on('close', (code: number) => {
198198
resolve({
199199
stdout: stdoutTruncated
200-
? `${stdout}\n[output truncated: exceeded 16MB limit]`
200+
? `${stdout.trim()}\n[output truncated: exceeded 16MB limit]`
201201
: stdout.trim(),
202202
stderr: stderrTruncated
203-
? `${stderr}\n[stderr truncated: exceeded 16MB limit]`
203+
? `${stderr.trim()}\n[stderr truncated: exceeded 16MB limit]`
204204
: stderr.trim(),
205205
exitCode: code ?? -1,
206206
})

apps/sim/app/api/workspaces/[id]/environment/route.ts

Lines changed: 65 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -96,41 +96,47 @@ export const PUT = withRouteHandler(
9696
if (!parsed.success) return parsed.response
9797
const { variables } = parsed.data.body
9898

99-
// Read existing encrypted ws vars
100-
const existingRows = await db
101-
.select()
102-
.from(workspaceEnvironment)
103-
.where(eq(workspaceEnvironment.workspaceId, workspaceId))
104-
.limit(1)
105-
106-
const existingEncrypted: Record<string, string> = (existingRows[0]?.variables as any) || {}
107-
108-
// Encrypt incoming
10999
const encryptedIncoming = await Promise.all(
110100
Object.entries(variables).map(async ([key, value]) => {
111101
const { encrypted } = await encryptSecret(value)
112102
return [key, encrypted] as const
113103
})
114104
).then((entries) => Object.fromEntries(entries))
115105

116-
await db
117-
.insert(workspaceEnvironment)
118-
.values({
119-
id: generateId(),
120-
workspaceId,
121-
variables: encryptedIncoming,
122-
createdAt: new Date(),
123-
updatedAt: new Date(),
124-
})
125-
.onConflictDoUpdate({
126-
target: [workspaceEnvironment.workspaceId],
127-
set: {
128-
variables: sql`${workspaceEnvironment.variables} || EXCLUDED.variables`,
106+
const { existingEncrypted, merged } = await db.transaction(async (tx) => {
107+
await tx.execute(
108+
sql`SELECT id FROM workspace_environment WHERE workspace_id = ${workspaceId} FOR UPDATE`
109+
)
110+
111+
const [existingRow] = await tx
112+
.select()
113+
.from(workspaceEnvironment)
114+
.where(eq(workspaceEnvironment.workspaceId, workspaceId))
115+
.limit(1)
116+
117+
const existing = ((existingRow?.variables as Record<string, string>) ?? {}) as Record<
118+
string,
119+
string
120+
>
121+
const mergedVars = { ...existing, ...encryptedIncoming }
122+
123+
await tx
124+
.insert(workspaceEnvironment)
125+
.values({
126+
id: generateId(),
127+
workspaceId,
128+
variables: mergedVars,
129+
createdAt: new Date(),
129130
updatedAt: new Date(),
130-
},
131-
})
131+
})
132+
.onConflictDoUpdate({
133+
target: [workspaceEnvironment.workspaceId],
134+
set: { variables: mergedVars, updatedAt: new Date() },
135+
})
136+
137+
return { existingEncrypted: existing, merged: mergedVars }
138+
})
132139

133-
const merged = { ...existingEncrypted, ...encryptedIncoming }
134140
const newKeys = Object.keys(variables).filter((k) => !(k in existingEncrypted))
135141
await createWorkspaceEnvCredentials({ workspaceId, newKeys, actingUserId: userId })
136142

@@ -184,36 +190,43 @@ export const DELETE = withRouteHandler(
184190
if (!parsed.success) return parsed.response
185191
const { keys } = parsed.data.body
186192

187-
const wsRows = await db
188-
.select()
189-
.from(workspaceEnvironment)
190-
.where(eq(workspaceEnvironment.workspaceId, workspaceId))
191-
.limit(1)
192-
193-
const current: Record<string, string> = (wsRows[0]?.variables as any) || {}
194-
let changed = false
195-
for (const k of keys) {
196-
if (k in current) {
197-
delete current[k]
198-
changed = true
193+
const result = await db.transaction(async (tx) => {
194+
await tx.execute(
195+
sql`SELECT id FROM workspace_environment WHERE workspace_id = ${workspaceId} FOR UPDATE`
196+
)
197+
198+
const [existingRow] = await tx
199+
.select()
200+
.from(workspaceEnvironment)
201+
.where(eq(workspaceEnvironment.workspaceId, workspaceId))
202+
.limit(1)
203+
204+
if (!existingRow) return null
205+
206+
const current: Record<string, string> =
207+
(existingRow.variables as Record<string, string>) ?? {}
208+
let modified = false
209+
for (const k of keys) {
210+
if (k in current) {
211+
delete current[k]
212+
modified = true
213+
}
199214
}
200-
}
201215

202-
if (!changed) {
216+
if (!modified) return null
217+
218+
await tx
219+
.update(workspaceEnvironment)
220+
.set({ variables: current, updatedAt: new Date() })
221+
.where(eq(workspaceEnvironment.workspaceId, workspaceId))
222+
223+
return { remainingKeysCount: Object.keys(current).length }
224+
})
225+
226+
if (!result) {
203227
return NextResponse.json({ success: true })
204228
}
205229

206-
await db
207-
.update(workspaceEnvironment)
208-
.set({
209-
variables: sql`${workspaceEnvironment.variables} - ARRAY[${sql.join(
210-
keys.map((k) => sql`${k}`),
211-
sql`, `
212-
)}]::text[]`,
213-
updatedAt: new Date(),
214-
})
215-
.where(eq(workspaceEnvironment.workspaceId, workspaceId))
216-
217230
await deleteWorkspaceEnvCredentials({ workspaceId, removedKeys: keys })
218231

219232
recordAudit({
@@ -227,7 +240,7 @@ export const DELETE = withRouteHandler(
227240
description: `Removed ${keys.length} workspace environment variable(s)`,
228241
metadata: {
229242
removedKeys: keys,
230-
remainingKeysCount: Object.keys(current).length,
243+
remainingKeysCount: result.remainingKeysCount,
231244
},
232245
request,
233246
})

apps/sim/lib/auth/credential-access.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ export interface CredentialAccessResult {
1313
credentialOwnerUserId?: string
1414
workspaceId?: string
1515
resolvedCredentialId?: string
16+
credentialType?: string
1617
}
1718

1819
/**
@@ -114,6 +115,7 @@ export async function authorizeCredentialUse(
114115
credentialOwnerUserId: actingUserId,
115116
workspaceId: platformCredential.workspaceId,
116117
resolvedCredentialId: platformCredential.id,
118+
credentialType: 'service_account',
117119
}
118120
}
119121

@@ -182,6 +184,7 @@ export async function authorizeCredentialUse(
182184
credentialOwnerUserId: accountRow.userId,
183185
workspaceId: platformCredential.workspaceId,
184186
resolvedCredentialId: platformCredential.accountId,
187+
credentialType: 'oauth',
185188
}
186189
}
187190

@@ -252,6 +255,7 @@ export async function authorizeCredentialUse(
252255
credentialOwnerUserId: accountRow.userId,
253256
workspaceId: workflowContext.workspaceId,
254257
resolvedCredentialId: workspaceCredential.accountId,
258+
credentialType: 'oauth',
255259
}
256260
}
257261

@@ -279,5 +283,6 @@ export async function authorizeCredentialUse(
279283
requesterUserId: auth.userId,
280284
credentialOwnerUserId: legacyAccount.userId,
281285
resolvedCredentialId: credentialId,
286+
credentialType: 'oauth',
282287
}
283288
}

apps/sim/lib/core/security/input-validation.ts

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1593,3 +1593,30 @@ export function validateWorkdayTenantUrl(
15931593

15941594
return { isValid: true, sanitized: url as string }
15951595
}
1596+
1597+
/**
1598+
* Validates a database identifier (table or column name) to prevent SQL injection.
1599+
*
1600+
* Accepts only identifiers that start with a letter or underscore and contain
1601+
* only letters, digits, and underscores — the safe subset of SQL identifiers.
1602+
*
1603+
* @param value - The identifier to validate
1604+
* @param paramName - Name of the parameter for error messages (e.g. 'table', 'column')
1605+
* @returns ValidationResult with isValid flag and optional error message
1606+
*/
1607+
export function validateDatabaseIdentifier(
1608+
value: unknown,
1609+
paramName = 'identifier'
1610+
): ValidationResult {
1611+
if (typeof value !== 'string' || value.length === 0) {
1612+
return { isValid: false, error: `${paramName} is required` }
1613+
}
1614+
if (!/^[A-Za-z_][A-Za-z0-9_]*$/.test(value)) {
1615+
logger.warn('Invalid database identifier', { paramName, value: value.substring(0, 100) })
1616+
return {
1617+
isValid: false,
1618+
error: `Invalid ${paramName}: must start with a letter or underscore and contain only letters, digits, and underscores`,
1619+
}
1620+
}
1621+
return { isValid: true, sanitized: value }
1622+
}

apps/sim/tools/supabase/count.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import { validateDatabaseIdentifier } from '@/lib/core/security/input-validation'
12
import type { SupabaseCountParams, SupabaseCountResponse } from '@/tools/supabase/types'
23
import { supabaseBaseUrl } from '@/tools/supabase/utils'
34
import type { ToolConfig } from '@/tools/types'
@@ -50,9 +51,8 @@ export const countTool: ToolConfig<SupabaseCountParams, SupabaseCountResponse> =
5051

5152
request: {
5253
url: (params) => {
53-
if (!/^[A-Za-z_][A-Za-z0-9_]*$/.test(params.table)) {
54-
throw new Error('Invalid table name: must contain only letters, digits, and underscores')
55-
}
54+
const tableValidation = validateDatabaseIdentifier(params.table, 'table')
55+
if (!tableValidation.isValid) throw new Error(tableValidation.error)
5656
let url = `${supabaseBaseUrl(params.projectId)}/rest/v1/${encodeURIComponent(params.table)}?select=*`
5757

5858
if (params.filter?.trim()) {

apps/sim/tools/supabase/delete.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import { validateDatabaseIdentifier } from '@/lib/core/security/input-validation'
12
import type { SupabaseDeleteParams, SupabaseDeleteResponse } from '@/tools/supabase/types'
23
import { supabaseBaseUrl } from '@/tools/supabase/utils'
34
import type { ToolConfig } from '@/tools/types'
@@ -44,9 +45,8 @@ export const deleteTool: ToolConfig<SupabaseDeleteParams, SupabaseDeleteResponse
4445

4546
request: {
4647
url: (params) => {
47-
if (!/^[A-Za-z_][A-Za-z0-9_]*$/.test(params.table)) {
48-
throw new Error('Invalid table name: must contain only letters, digits, and underscores')
49-
}
48+
const tableValidation = validateDatabaseIdentifier(params.table, 'table')
49+
if (!tableValidation.isValid) throw new Error(tableValidation.error)
5050

5151
let url = `${supabaseBaseUrl(params.projectId)}/rest/v1/${encodeURIComponent(params.table)}?select=*`
5252

apps/sim/tools/supabase/get_row.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import { validateDatabaseIdentifier } from '@/lib/core/security/input-validation'
12
import type { SupabaseGetRowParams, SupabaseGetRowResponse } from '@/tools/supabase/types'
23
import { supabaseBaseUrl } from '@/tools/supabase/utils'
34
import type { ToolConfig } from '@/tools/types'
@@ -50,9 +51,8 @@ export const getRowTool: ToolConfig<SupabaseGetRowParams, SupabaseGetRowResponse
5051

5152
request: {
5253
url: (params) => {
53-
if (!/^[A-Za-z_][A-Za-z0-9_]*$/.test(params.table)) {
54-
throw new Error('Invalid table name: must contain only letters, digits, and underscores')
55-
}
54+
const tableValidation = validateDatabaseIdentifier(params.table, 'table')
55+
if (!tableValidation.isValid) throw new Error(tableValidation.error)
5656

5757
const selectColumns = params.select?.trim() || '*'
5858
let url = `${supabaseBaseUrl(params.projectId)}/rest/v1/${encodeURIComponent(params.table)}?select=${encodeURIComponent(selectColumns)}`

apps/sim/tools/supabase/insert.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import { validateDatabaseIdentifier } from '@/lib/core/security/input-validation'
12
import type { SupabaseInsertParams, SupabaseInsertResponse } from '@/tools/supabase/types'
23
import { supabaseBaseUrl } from '@/tools/supabase/utils'
34
import type { ToolConfig } from '@/tools/types'
@@ -44,9 +45,8 @@ export const insertTool: ToolConfig<SupabaseInsertParams, SupabaseInsertResponse
4445

4546
request: {
4647
url: (params) => {
47-
if (!/^[A-Za-z_][A-Za-z0-9_]*$/.test(params.table)) {
48-
throw new Error('Invalid table name: must contain only letters, digits, and underscores')
49-
}
48+
const tableValidation = validateDatabaseIdentifier(params.table, 'table')
49+
if (!tableValidation.isValid) throw new Error(tableValidation.error)
5050
return `${supabaseBaseUrl(params.projectId)}/rest/v1/${encodeURIComponent(params.table)}?select=*`
5151
},
5252
method: 'POST',

apps/sim/tools/supabase/query.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import { validateDatabaseIdentifier } from '@/lib/core/security/input-validation'
12
import type { SupabaseQueryParams, SupabaseQueryResponse } from '@/tools/supabase/types'
23
import { supabaseBaseUrl } from '@/tools/supabase/utils'
34
import type { ToolConfig } from '@/tools/types'
@@ -68,9 +69,8 @@ export const queryTool: ToolConfig<SupabaseQueryParams, SupabaseQueryResponse> =
6869

6970
request: {
7071
url: (params) => {
71-
if (!/^[A-Za-z_][A-Za-z0-9_]*$/.test(params.table)) {
72-
throw new Error('Invalid table name: must contain only letters, digits, and underscores')
73-
}
72+
const tableValidation = validateDatabaseIdentifier(params.table, 'table')
73+
if (!tableValidation.isValid) throw new Error(tableValidation.error)
7474
const selectColumns = params.select?.trim() || '*'
7575
let url = `${supabaseBaseUrl(params.projectId)}/rest/v1/${encodeURIComponent(params.table)}?select=${encodeURIComponent(selectColumns)}`
7676

0 commit comments

Comments
 (0)