Skip to content

Commit 9661638

Browse files
icecrasher321icecrasher321
committed
fix(execution): harden compatibility and secret diagnostics
1 parent 1a7c79b commit 9661638

33 files changed

Lines changed: 1351 additions & 253 deletions

‎apps/sim/app/api/cron/cleanup-stale-executions/route.test.ts‎

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
/**
22
* @vitest-environment node
33
*/
4-
import { workflowExecutionLogs } from '@sim/db/schema'
4+
import { asyncJobs, workflowExecutionLogs } from '@sim/db/schema'
55
import { createMockRequest, dbChainMockFns, queueTableRows, resetDbChainMock } from '@sim/testing'
66
import { beforeEach, describe, expect, it, vi } from 'vitest'
77

@@ -82,6 +82,30 @@ describe('stale execution cleanup deadline grace', () => {
8282
}
8383
})
8484

85+
it('reports a configured job duration cap while preserving the generic stale fallback', async () => {
86+
const response = await GET(createRequest())
87+
88+
expect(response.status).toBe(200)
89+
const staleProcessingUpdateIndex = dbChainMockFns.update.mock.calls.findIndex(
90+
([table]) => table === asyncJobs
91+
)
92+
expect(staleProcessingUpdateIndex).toBeGreaterThanOrEqual(0)
93+
94+
const update = dbChainMockFns.set.mock.calls[staleProcessingUpdateIndex]?.[0] as {
95+
error: { toSQL: () => { sql: string; params: unknown[] } }
96+
}
97+
const errorExpression = update.error.toSQL()
98+
99+
expect(errorExpression.sql).toContain("->>'maxDurationSeconds'")
100+
expect(errorExpression.sql).toContain(
101+
"'Job terminated: exceeded configured maximum duration of '"
102+
)
103+
expect(errorExpression.sql).toContain("|| ' seconds'")
104+
expect(errorExpression.params).toContainEqual(
105+
expect.stringMatching(/^Job terminated: stuck in processing for more than \d+ minutes$/)
106+
)
107+
})
108+
85109
it('caps every bulk mutation and returns only scalar export cleanup fields', async () => {
86110
const stateBatch = Array.from({ length: 1000 }, (_, index) => ({ id: `state-${index}` }))
87111
const retentionBatch = Array.from({ length: 2000 }, (_, index) => ({

‎apps/sim/app/api/cron/cleanup-stale-executions/route.ts‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ const logger = createLogger('CleanupStaleExecutions')
2323

2424
const STALE_THRESHOLD_MS = getExecutionReservationTtlMs()
2525
const STALE_THRESHOLD_MINUTES = Math.ceil(STALE_THRESHOLD_MS / 60000)
26+
const GENERIC_STALE_PROCESSING_ERROR = `Job terminated: stuck in processing for more than ${STALE_THRESHOLD_MINUTES} minutes`
2627
const MAX_INT32 = 2_147_483_647
2728
/** Terminal table-jobs older than this are pruned; only the latest job per table is ever read. */
2829
const TABLE_JOB_RETENTION_HOURS = 24
@@ -214,7 +215,13 @@ export const GET = withRouteHandler(async (request: NextRequest) => {
214215
.set({
215216
status: JOB_STATUS.FAILED,
216217
completedAt: new Date(),
217-
error: `Job terminated: stuck in processing for more than ${STALE_THRESHOLD_MINUTES} minutes`,
218+
error: sql<string>`CASE
219+
WHEN jsonb_typeof(${asyncJobs.metadata}->'maxDurationSeconds') = 'number'
220+
THEN 'Job terminated: exceeded configured maximum duration of '
221+
|| (${asyncJobs.metadata}->>'maxDurationSeconds')
222+
|| ' seconds'
223+
ELSE ${GENERIC_STALE_PROCESSING_ERROR}
224+
END`,
218225
updatedAt: new Date(),
219226
})
220227
.where(and(staleProcessingPredicate, inArray(asyncJobs.id, candidates)))

‎apps/sim/app/api/function/execute/route.test.ts‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1447,6 +1447,36 @@ describe('Function Execute API Route', () => {
14471447
expect(Object.values(request.contextVariables)).not.toContain('must-not-bind')
14481448
})
14491449

1450+
it('lowers missing shell placeholders while preserving comments and heredoc delimiters', async () => {
1451+
envFlagsMock.isRemoteSandboxEnabled = true
1452+
const response = await POST(
1453+
createMockRequest(
1454+
'POST',
1455+
{
1456+
code: [
1457+
'# {{COMMENT_ONLY}}',
1458+
'printf \'%s\\n\' "before{{MISSING}}after"',
1459+
"cat <<'{{DELIMITER}}'",
1460+
'literal body',
1461+
'{{DELIMITER}}',
1462+
].join('\n'),
1463+
language: 'shell',
1464+
envVars: { COMMENT_ONLY: 'must-not-bind' },
1465+
},
1466+
{ 'x-sim-request-private-tool-metadata': 'resolved-secret-names-v1' }
1467+
)
1468+
)
1469+
1470+
const [request] = mockExecuteShellInSandbox.mock.calls.at(-1) ?? []
1471+
expect(response.status).toBe(200)
1472+
expect((await response.json()).__resolvedSecretNames).toEqual([])
1473+
expect(request.code).toContain('# {{COMMENT_ONLY}}')
1474+
expect(request.code).toContain('"beforeafter"')
1475+
expect(request.code).toContain("cat <<'{{DELIMITER}}'")
1476+
expect(request.code).toContain('\n{{DELIMITER}}')
1477+
expect(request.code).not.toContain('{{MISSING}}')
1478+
})
1479+
14501480
it.each([
14511481
{
14521482
language: 'javascript',

‎apps/sim/app/api/resume/[workflowId]/[executionId]/[contextId]/route.test.ts‎

Lines changed: 89 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
/**
22
* @vitest-environment node
33
*/
4+
import { loggerMock } from '@sim/testing'
45
import { NextRequest } from 'next/server'
56
import { beforeEach, describe, expect, it, vi } from 'vitest'
67

@@ -11,6 +12,7 @@ const {
1112
mockGetPauseContextDetail,
1213
mockGetPausedExecutionDetail,
1314
mockPreprocessExecution,
15+
mockStartResumeExecution,
1416
mockExecuteResumeJob,
1517
mockShouldExecuteInline,
1618
mockValidateWorkflowAccess,
@@ -21,6 +23,7 @@ const {
2123
mockGetPauseContextDetail: vi.fn(),
2224
mockGetPausedExecutionDetail: vi.fn(),
2325
mockPreprocessExecution: vi.fn(),
26+
mockStartResumeExecution: vi.fn(),
2427
mockExecuteResumeJob: vi.fn(),
2528
mockShouldExecuteInline: vi.fn(() => false),
2629
mockValidateWorkflowAccess: vi.fn(),
@@ -58,12 +61,20 @@ vi.mock('@/lib/workflows/executor/human-in-the-loop-manager', () => ({
5861
getPausedExecutionDetail: mockGetPausedExecutionDetail,
5962
markResumeAttemptFailed: vi.fn(),
6063
processQueuedResumes: vi.fn(),
61-
startResumeExecution: vi.fn(),
64+
startResumeExecution: mockStartResumeExecution,
6265
},
6366
}))
6467

6568
import { GET, POST } from '@/app/api/resume/[workflowId]/[executionId]/[contextId]/route'
6669

70+
const resumeApiLoggerCallIndex = loggerMock.createLogger.mock.calls.findIndex(
71+
([name]) => name === 'WorkflowResumeAPI'
72+
)
73+
const resumeApiLogger = loggerMock.createLogger.mock.results[resumeApiLoggerCallIndex]?.value
74+
if (!resumeApiLogger) {
75+
throw new Error('WorkflowResumeAPI logger mock was not initialized')
76+
}
77+
6778
const WORKFLOW_ID = 'workflow-1'
6879
const EXECUTION_ID = 'execution-1'
6980
const CONTEXT_ID = 'context-1'
@@ -188,6 +199,12 @@ describe('POST /api/resume/[workflowId]/[executionId]/[contextId]', () => {
188199
})
189200
mockEnqueue.mockResolvedValue('resume-job-1')
190201
mockExecuteResumeJob.mockResolvedValue({ success: true })
202+
mockStartResumeExecution.mockResolvedValue({
203+
success: true,
204+
status: 'completed',
205+
output: {},
206+
logs: [],
207+
})
191208
mockShouldExecuteInline.mockReturnValue(false)
192209
})
193210

@@ -329,6 +346,77 @@ describe('POST /api/resume/[workflowId]/[executionId]/[contextId]', () => {
329346
)
330347
})
331348

349+
it('projects rejected resume requests for logs without changing the API error response', async () => {
350+
const secret = 'resume-request-secret-value'
351+
const message = `Resume request exposed ${secret} __var_API_KEY __sim_code_1_binding_0`
352+
const rawError = new Error(message)
353+
mockEnqueueOrStartResume.mockRejectedValueOnce(rawError)
354+
const { request, context } = makeRequest()
355+
356+
const response = await POST(request, context)
357+
358+
expect(response.status).toBe(400)
359+
expect(await response.json()).toEqual({ error: message })
360+
expect(resumeApiLogger.error).toHaveBeenCalledWith('Resume request failed', {
361+
errorType: 'error',
362+
hasStack: true,
363+
})
364+
const loggerPayload = JSON.stringify(resumeApiLogger.error.mock.calls)
365+
expect(loggerPayload).not.toContain(secret)
366+
expect(loggerPayload).not.toContain('__var_')
367+
expect(loggerPayload).not.toContain('__sim_')
368+
expect(rawError.message).toBe(message)
369+
})
370+
371+
it('projects detached resume failures without changing the started response', async () => {
372+
const secret = 'detached-resume-secret-value'
373+
const message = `Detached resume exposed ${secret} __var_API_KEY __sim_code_2_binding_0`
374+
const rawError = new Error(message)
375+
const pausedExecution = createPausedExecution()
376+
mockValidateWorkflowAccess.mockResolvedValueOnce({
377+
workflow: {
378+
id: WORKFLOW_ID,
379+
workspaceId: WORKSPACE_ID,
380+
},
381+
auth: {
382+
success: true,
383+
userId: 'current-session-user',
384+
authType: 'session',
385+
workspaceId: WORKSPACE_ID,
386+
},
387+
})
388+
mockEnqueueOrStartResume.mockResolvedValueOnce({
389+
status: 'starting',
390+
resumeExecutionId: 'resume-attempt-1',
391+
resumeEntryId: 'resume-entry-1',
392+
pausedExecution,
393+
contextId: CONTEXT_ID,
394+
resumeInput: { approved: true },
395+
userId: 'current-session-user',
396+
})
397+
mockStartResumeExecution.mockRejectedValueOnce(rawError)
398+
const { request, context } = makeRequest()
399+
400+
const response = await POST(request, context)
401+
await Promise.resolve()
402+
403+
expect(response.status).toBe(200)
404+
expect(await response.json()).toEqual({
405+
status: 'started',
406+
executionId: 'resume-attempt-1',
407+
message: 'Resume execution started.',
408+
})
409+
expect(resumeApiLogger.error).toHaveBeenCalledWith('Failed to start resume execution', {
410+
errorType: 'error',
411+
hasStack: true,
412+
})
413+
const loggerPayload = JSON.stringify(resumeApiLogger.error.mock.calls)
414+
expect(loggerPayload).not.toContain(secret)
415+
expect(loggerPayload).not.toContain('__var_')
416+
expect(loggerPayload).not.toContain('__sim_')
417+
expect(rawError.message).toBe(message)
418+
})
419+
332420
it.each([
333421
{ statusCode: 402, message: 'Member usage limit reached', retryable: false },
334422
{ statusCode: 429, message: 'Target concurrency full', retryable: true },

‎apps/sim/app/api/resume/[workflowId]/[executionId]/[contextId]/route.ts‎

Lines changed: 17 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ import {
2929
import { validateWorkflowAccess } from '@/app/api/workflows/middleware'
3030
import { executeResumeJob, type ResumeExecutionPayload } from '@/background/resume-execution'
3131
import { ExecutionSnapshot } from '@/executor/execution/snapshot'
32+
import { projectResolvedSecretDiagnosticError } from '@/executor/utils/resolved-secret-content-projection'
3233

3334
const logger = createLogger('WorkflowResumeAPI')
3435

@@ -394,12 +395,14 @@ export const POST = withRouteHandler(
394395
}
395396

396397
PauseResumeManager.startResumeExecution(resumeArgs).catch((error) => {
397-
logger.error('Failed to start resume execution', {
398-
workflowId,
399-
parentExecutionId: executionId,
400-
resumeExecutionId: enqueueResult.resumeExecutionId,
401-
error,
402-
})
398+
logger.error(
399+
'Failed to start resume execution',
400+
projectResolvedSecretDiagnosticError(error, undefined, {
401+
workflowId,
402+
parentExecutionId: executionId,
403+
resumeExecutionId: enqueueResult.resumeExecutionId,
404+
})
405+
)
403406
})
404407

405408
return NextResponse.json({
@@ -408,12 +411,14 @@ export const POST = withRouteHandler(
408411
message: 'Resume execution started.',
409412
})
410413
} catch (error) {
411-
logger.error('Resume request failed', {
412-
workflowId,
413-
executionId,
414-
contextId,
415-
error,
416-
})
414+
logger.error(
415+
'Resume request failed',
416+
projectResolvedSecretDiagnosticError(error, undefined, {
417+
workflowId,
418+
executionId,
419+
contextId,
420+
})
421+
)
417422
const statusCode =
418423
isRecordLike(error) && typeof error.statusCode === 'number' ? error.statusCode : 400
419424
return NextResponse.json(

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

Lines changed: 31 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1598,7 +1598,10 @@ async function handleExecutePost(
15981598
return payloadTooLargeResponse()
15991599
}
16001600

1601-
reqLogger.error(`Non-SSE execution failed: ${errorMessage}`)
1601+
reqLogger.error(
1602+
'Non-SSE execution failed',
1603+
loggingSession.projectDiagnosticError(error, { isTimeout: executionTimedOut })
1604+
)
16021605

16031606
const executionResult = hasExecutionResult(error) ? error.executionResult : undefined
16041607
const status = executionTimedOut ? 408 : getExecutionErrorStatus(error)
@@ -1826,11 +1829,13 @@ async function handleExecutePost(
18261829
// events still reach the active client and the UI doesn't hang on "running".
18271830
// Marking a terminal event delivered-live as published lets finalization close
18281831
// the stream cleanly instead of aborting it with controller.error().
1829-
reqLogger.warn('Event buffer write failed; delivering event over live stream only', {
1830-
eventType: event.type,
1831-
terminal: Boolean(terminalStatus),
1832-
error: toError(e).message,
1833-
})
1832+
reqLogger.warn(
1833+
'Event buffer write failed; delivering event over live stream only',
1834+
loggingSession.projectDiagnosticError(e, {
1835+
eventType: event.type,
1836+
terminal: Boolean(terminalStatus),
1837+
})
1838+
)
18341839
terminalBufferWriteFailed = Boolean(terminalStatus)
18351840
terminalEventPublished ||= Boolean(terminalStatus)
18361841
}
@@ -2095,7 +2100,10 @@ async function handleExecutePost(
20952100
}
20962101
} catch (error) {
20972102
if (!timeoutController.signal.aborted && !isStreamClosed) {
2098-
reqLogger.error('Error streaming block content:', error)
2103+
reqLogger.error(
2104+
'Error streaming block content',
2105+
loggingSession.projectDiagnosticError(error, { blockId })
2106+
)
20992107
}
21002108
} finally {
21012109
unsubscribe()
@@ -2337,7 +2345,10 @@ async function handleExecutePost(
23372345
? getTimeoutErrorMessage(error, timeoutController.timeoutMs)
23382346
: getErrorMessage(error, 'Unknown error')
23392347

2340-
reqLogger.error(`SSE execution failed: ${errorMessage}`, { isTimeout })
2348+
reqLogger.error(
2349+
'SSE execution failed',
2350+
loggingSession.projectDiagnosticError(error, { isTimeout })
2351+
)
23412352

23422353
const executionResult = hasExecutionResult(error) ? error.executionResult : undefined
23432354
let compactErrorLogs: BlockLog[] | undefined
@@ -2355,9 +2366,10 @@ async function handleExecutePost(
23552366
)
23562367
: undefined
23572368
} catch (compactionError) {
2358-
reqLogger.warn('Failed to compact SSE error logs, omitting oversized error details', {
2359-
error: toError(compactionError).message,
2360-
})
2369+
reqLogger.warn(
2370+
'Failed to compact SSE error logs, omitting oversized error details',
2371+
loggingSession.projectDiagnosticError(compactionError)
2372+
)
23612373
}
23622374

23632375
finalMetaStatus = 'error'
@@ -2403,18 +2415,19 @@ async function handleExecutePost(
24032415
}
24042416
} else if (terminalEventPublished) {
24052417
await eventWriter.close().catch((closeError) => {
2406-
reqLogger.warn('Failed to close execution event writer after terminal publish', {
2407-
executionId,
2408-
error: getErrorMessage(closeError),
2409-
})
2418+
reqLogger.warn(
2419+
'Failed to close execution event writer after terminal publish',
2420+
loggingSession.projectDiagnosticError(closeError, { executionId })
2421+
)
24102422
})
24112423
} else {
24122424
try {
24132425
await eventWriter.close()
24142426
} catch (closeError) {
2415-
reqLogger.warn('Failed to close event writer', {
2416-
error: toError(closeError).message,
2417-
})
2427+
reqLogger.warn(
2428+
'Failed to close event writer',
2429+
loggingSession.projectDiagnosticError(closeError, { executionId })
2430+
)
24182431
}
24192432
}
24202433
timeoutController.cleanup()

0 commit comments

Comments
 (0)