Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion apps/sim/ee/workspace-forking/lib/copy/copy-workflows.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
} from '@/lib/workflows/persistence/remap-internal-ids'
import { saveWorkflowToNormalizedTables } from '@/lib/workflows/persistence/utils'
import type { CanonicalModeOverrides } from '@/lib/workflows/subblocks/visibility'
import { isBlockActingAsTrigger } from '@/lib/workflows/triggers/trigger-utils'
import {
deriveForkBlockId,
type ForkBlockIdResolver,
Expand Down Expand Up @@ -436,7 +437,10 @@ export async function copyWorkflowStateIntoTarget(

// double-cast-allowed: SubBlockState is structurally a SubBlockRecord entry but lacks the open index signature SubBlockRecord declares
const sourceSubBlocks = (block.subBlocks ?? {}) as unknown as SubBlockRecord
const sanitizedSource = sanitizeSubBlocksForDuplicate(sourceSubBlocks)
const sanitizedSource = sanitizeSubBlocksForDuplicate(
sourceSubBlocks,
isBlockActingAsTrigger(block)
)
let subBlocks: SubBlockRecord = sanitizedSource
// Tracks the block's live `canonicalModes` through this pass, so a `tool-input` reindex
// (a dropped custom-tool/MCP entry shifts later tools' array positions) is visible to every
Expand Down
71 changes: 71 additions & 0 deletions apps/sim/lib/webhooks/deploy.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ import {
buildProviderConfig,
resolveTriggerCredentialId,
resolveWebhookConfigForBlock,
validateTriggerWebhookConfigForDeploy,
} from '@/lib/webhooks/deploy'
import { getBlock } from '@/blocks'
import { getTrigger } from '@/triggers'
Expand Down Expand Up @@ -359,3 +360,73 @@ describe('resolveWebhookConfigForBlock — slack_oauth routing', () => {
expect(mockFetchSlackTeamId).not.toHaveBeenCalled()
})
})

describe('validateTriggerWebhookConfigForDeploy — duplicate path guard', () => {
const pathTrigger = trigger([{ id: 'eventType', mode: 'trigger', required: false }])

function triggerBlockWithPath(id: string, name: string, triggerPath: string): BlockState {
return {
id,
name,
type: 'generic_webhook',
enabled: true,
triggerMode: true,
subBlocks: { triggerPath: { value: triggerPath } },
} as unknown as BlockState
}

beforeEach(() => {
;(getBlock as Mock).mockReturnValue({ category: 'triggers' })
;(getTrigger as Mock).mockReturnValue(pathTrigger)
})

/**
* The cross-workflow guards do not cover two blocks in ONE workflow sharing a path:
* findConflictingWebhookPathOwner ignores same-workflow owners and claimWebhookPath's CAS
* accepts a same-workflow re-claim. Without this guard it surfaces as an opaque 500 from the
* `path_deployment_unique` index.
*/
it('rejects two trigger blocks in one workflow sharing a triggerPath', async () => {
const result = await validateTriggerWebhookConfigForDeploy({
'block-a': triggerBlockWithPath('block-a', 'Webhook 1', 'shared-path'),
'block-b': triggerBlockWithPath('block-b', 'Webhook 2', 'shared-path'),
})

expect(result.success).toBe(false)
if (result.success) return
expect(result.error.status).toBe(400)
expect(result.error.message).toContain('shared-path')
expect(result.error.message).toContain('Webhook 1')
expect(result.error.message).toContain('Webhook 2')
})

it('allows distinct paths', async () => {
const result = await validateTriggerWebhookConfigForDeploy({
'block-a': triggerBlockWithPath('block-a', 'Webhook 1', 'path-a'),
'block-b': triggerBlockWithPath('block-b', 'Webhook 2', 'path-b'),
})

expect(result.success).toBe(true)
})

it('allows multiple blocks with no explicit path (each falls back to its block id)', async () => {
const result = await validateTriggerWebhookConfigForDeploy({
'block-a': triggerBlockWithPath('block-a', 'Webhook 1', ''),
'block-b': triggerBlockWithPath('block-b', 'Webhook 2', ''),
})

expect(result.success).toBe(true)
})

it('ignores a disabled block that shares a path', async () => {
const disabled = triggerBlockWithPath('block-b', 'Webhook 2', 'shared-path')
;(disabled as { enabled: boolean }).enabled = false

const result = await validateTriggerWebhookConfigForDeploy({
'block-a': triggerBlockWithPath('block-a', 'Webhook 1', 'shared-path'),
'block-b': disabled,
})

expect(result.success).toBe(true)
})
})
20 changes: 20 additions & 0 deletions apps/sim/lib/webhooks/deploy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,11 +73,31 @@ export async function validateTriggerWebhookConfigForDeploy(
blocks: Record<string, BlockState>
): Promise<TriggerSaveResult> {
const triggerBlocks = Object.values(blocks || {}).filter((b) => b && b.enabled !== false)
const blockNameByPath = new Map<string, string>()

for (const block of triggerBlocks) {
const triggerId = resolveTriggerId(block)
if (!triggerId || !isTriggerValid(triggerId)) continue

// Two blocks in ONE workflow claiming the same path is unreachable through the UI, but the
// cross-workflow guards do not cover it (findConflictingWebhookPathOwner ignores same-workflow
// owners; claimWebhookPath's CAS accepts a same-workflow re-claim). Without this it lands on
// the `path_deployment_unique` index as an opaque 500 instead of an actionable message.
const pathValue = getSubBlockValue(block, 'triggerPath')
if (typeof pathValue === 'string' && pathValue.length > 0) {
const owner = blockNameByPath.get(pathValue)
if (owner) {
return {
success: false,
error: {
message: `Webhook path "${pathValue}" is used by more than one trigger block ("${owner}" and "${block.name}"). Each trigger needs its own path.`,
status: 400,
},
}
}
blockNameByPath.set(pathValue, block.name)
}
Comment thread
waleedlatif1 marked this conversation as resolved.

const triggerDef = getTrigger(triggerId)
const { providerConfig, missingFields } = buildProviderConfig(block, triggerId, triggerDef)

Expand Down
6 changes: 5 additions & 1 deletion apps/sim/lib/workflows/persistence/duplicate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import {
type SubBlockRecord,
sanitizeSubBlocksForDuplicate,
} from '@/lib/workflows/persistence/remap-internal-ids'
import { isBlockActingAsTrigger } from '@/lib/workflows/triggers/trigger-utils'
import { deduplicateWorkflowName } from '@/lib/workflows/utils'
import type { Variable } from '@/stores/variables/types'
import type { LoopConfig, ParallelConfig } from '@/stores/workflows/workflow/types'
Expand Down Expand Up @@ -304,7 +305,10 @@ export async function duplicateWorkflow(
typeof updatedSubBlocks === 'object' &&
!Array.isArray(updatedSubBlocks)
) {
updatedSubBlocks = sanitizeSubBlocksForDuplicate(updatedSubBlocks as SubBlockRecord)
updatedSubBlocks = sanitizeSubBlocksForDuplicate(
updatedSubBlocks as SubBlockRecord,
isBlockActingAsTrigger(block)
)
}
if (
varIdMapping.size > 0 &&
Expand Down
76 changes: 76 additions & 0 deletions apps/sim/lib/workflows/persistence/remap-internal-ids.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
remapConditionIdsInSubBlocks,
remapWorkflowReferencesInSubBlocks,
type SubBlockRecord,
sanitizeSubBlocksForDuplicate,
} from '@/lib/workflows/persistence/remap-internal-ids'

describe('remapWorkflowReferencesInSubBlocks', () => {
Expand Down Expand Up @@ -463,3 +464,78 @@ describe('coerceObjectArray', () => {
expect(coerceObjectArray(42)).toEqual({ array: null, wasString: false })
})
})

describe('sanitizeSubBlocksForDuplicate', () => {
/**
* Regression: the strip was keyed on subblock id alone, so duplicating or forking a workflow
* blanked Discord/Attio/Vercel's REQUIRED user-entered `webhookId` while leaving the
* `webhookToken` it pairs with — a silently half-configured copy.
*/
it('keeps a user-entered webhookId on a block that is not acting as a trigger', () => {
const discord: SubBlockRecord = {
operation: { type: 'dropdown', value: 'discord_execute_webhook' },
webhookId: { type: 'short-input', value: '1234567890' },
webhookToken: { type: 'short-input', value: 'tok_abc' },
content: { type: 'long-input', value: 'hello' },
}

const out = sanitizeSubBlocksForDuplicate(discord, false)

expect(out.webhookId).toEqual({ type: 'short-input', value: '1234567890' })
expect(out.webhookToken).toEqual({ type: 'short-input', value: 'tok_abc' })
expect(out.content).toBeDefined()
expect(out.operation).toBeDefined()
})

it('keeps prefixed keys on a block that is not acting as a trigger', () => {
const out = sanitizeSubBlocksForDuplicate(
{
webhookId_custom: { type: 'short-input', value: 'x' },
triggerConfig: { type: 'short-input', value: { labelIds: ['a'] } },
},
false
)

expect(out.webhookId_custom).toBeDefined()
expect(out.triggerConfig).toBeDefined()
})

it('still strips webhook runtime identity on a real trigger block', () => {
const out = sanitizeSubBlocksForDuplicate(
{
webhookId: { type: 'short-input', value: 'wh_original' },
triggerPath: { type: 'short-input', value: 'deployed-path' },
triggerConfig: { type: 'short-input', value: { labelIds: ['a'] } },
triggerId: { type: 'short-input', value: 'generic_webhook' },
requireAuth: { type: 'switch', value: true },
},
true
)

expect(out.webhookId).toBeUndefined()
expect(out.triggerPath).toBeUndefined()
expect(out.triggerConfig).toBeUndefined()
expect(out.triggerId).toBeUndefined()
expect(out.requireAuth).toBeDefined()
})

it('strips display-only system subblocks regardless of trigger status', () => {
for (const isTrigger of [true, false]) {
const out = sanitizeSubBlocksForDuplicate(
{
webhookUrlDisplay: { type: 'short-input', value: 'https://x' },
samplePayload: { type: 'code', value: '{}' },
scheduleInfo: { type: 'schedule-info', value: null },
triggerCredentials: { type: 'oauth-input', value: 'cred-1' },
},
isTrigger
)

expect(out.webhookUrlDisplay).toBeUndefined()
expect(out.samplePayload).toBeUndefined()
expect(out.scheduleInfo).toBeUndefined()
// triggerCredentials is deliberately preserved so a copy keeps its OAuth connection.
expect(out.triggerCredentials).toBeDefined()
}
})
})
20 changes: 17 additions & 3 deletions apps/sim/lib/workflows/persistence/remap-internal-ids.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,11 +47,25 @@ export function isSystemSubBlockKey(key: string, ids: Set<string> | string[]): b
return idList.some((id) => key === id || key.startsWith(`${id}_`))
}

/** Strip trigger-runtime and non-credential system subblocks for a fresh copy. */
export function sanitizeSubBlocksForDuplicate(subBlocks: SubBlockRecord): SubBlockRecord {
/**
* Strip trigger-runtime and non-credential system subblocks for a fresh copy.
*
* `isActingAsTrigger` is required, not optional, so every call site must decide: the
* {@link TRIGGER_RUNTIME_SUBBLOCK_IDS} strip is only correct for a block that actually owns a
* webhook. Discord, Attio, and Vercel action blocks expose a REQUIRED user-entered `webhookId`
* field, and stripping it left the copy half-configured (`webhookToken` survives while the id it
* pairs with vanishes). Resolve the flag with `isBlockActingAsTrigger(block)`.
*
* The display-only system subblocks are stripped unconditionally: those ids exist only on trigger
* definitions, so the strip is a no-op for anything else.
*/
export function sanitizeSubBlocksForDuplicate(
subBlocks: SubBlockRecord,
isActingAsTrigger: boolean
): SubBlockRecord {
const sanitized: SubBlockRecord = {}
for (const [key, subBlock] of Object.entries(subBlocks)) {
if (isSystemSubBlockKey(key, TRIGGER_RUNTIME_SUBBLOCK_IDS)) continue
if (isActingAsTrigger && isSystemSubBlockKey(key, TRIGGER_RUNTIME_SUBBLOCK_IDS)) continue
if (isSystemSubBlockKey(key, DUPLICATE_STRIPPED_SYSTEM_SUBBLOCK_IDS)) continue
sanitized[key] = subBlock
}
Expand Down
19 changes: 19 additions & 0 deletions apps/sim/lib/workflows/triggers/trigger-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,25 @@ export function getAllTriggerBlocks(): TriggerInfo[] {
})
}

/**
* Whether a block instance is actually acting as a trigger, so its
* {@link TRIGGER_RUNTIME_SUBBLOCK_IDS} values are webhook runtime metadata rather than user
* configuration.
*
* Deliberately narrower than {@link hasTriggerCapability}: several `category: 'tools'` blocks
* declare `triggers.enabled` AND expose a required user-entered `webhookId` action field
* (Discord, Attio, Vercel). Gating on capability would treat that field as runtime metadata and
* silently clear it. Mirrors the deploy-side `resolveTriggerId` gate so both sides agree on which
* blocks own a webhook.
*/
export function isBlockActingAsTrigger(block: {
type: string
triggerMode?: boolean | null
}): boolean {
if (block.triggerMode === true) return true
return getBlock(block.type)?.category === 'triggers'
}

/**
* Check if a block has trigger capability (contains trigger mode subblocks)
*/
Expand Down
Loading
Loading