From 132670172df55a4e9548af2f5d0ecf2b58befe59 Mon Sep 17 00:00:00 2001 From: Vikhyath Mondreti Date: Tue, 11 Aug 2026 16:04:00 -0700 Subject: [PATCH] fix(forks): detect secrets referenced from advanced-mode fields --- .../lib/remap/remap-references.test.ts | 168 ++++++++++++++++++ .../lib/remap/remap-references.ts | 17 +- 2 files changed, 182 insertions(+), 3 deletions(-) diff --git a/apps/sim/ee/workspace-forking/lib/remap/remap-references.test.ts b/apps/sim/ee/workspace-forking/lib/remap/remap-references.test.ts index 10d16ab6841..26bd1bc0d55 100644 --- a/apps/sim/ee/workspace-forking/lib/remap/remap-references.test.ts +++ b/apps/sim/ee/workspace-forking/lib/remap/remap-references.test.ts @@ -27,6 +27,7 @@ import { clearDependentsOnRemap, collectClearedDependents, createForkSubBlockTransform, + type ForkReferenceResolver, parseNestedDependentKey, readTargetDraftDependentValue, remapForkSubBlocks, @@ -1394,6 +1395,173 @@ describe('canonical mode policy (fork/promote)', () => { expect(scan.references.map((ref) => ref.sourceId)).toEqual(['kb-active']) }) + /** + * `{{ENV}}` detection is gated on EXECUTION, not on ownership - unlike resource ids, which + * follow the verbatim/user-owned policy above. The shipped shape this protects is a Slack + * block whose advanced "Channel ID" (`manualChannel`) holds a `{{SECRET}}`: that field is + * live, so the secret must surface as a mapping entry and gate the sync. Suppressing it made + * the rewrite and detect halves disagree (`remapEnvInValue` rewrites a manual member's ref + * unconditionally), so the key could never originate a mapping row and a target missing that + * secret passed the required-env gate silently. + */ + const envPairBlock = () => + blockWith([ + { + id: 'channel', + title: 'Channel', + type: 'channel-selector', + canonicalParamId: 'channel', + mode: 'basic', + }, + { + id: 'manualChannel', + title: 'Channel ID', + type: 'short-input', + canonicalParamId: 'channel', + mode: 'advanced', + }, + ]) + + const scanEnv = ( + subBlocks: Record, + canonicalModes?: Record, + resolve: ForkReferenceResolver = () => null + ) => { + vi.mocked(getBlock).mockReturnValue(envPairBlock()) + return scanWorkflowReferences( + [{ id: 'b1', name: 'Slack', type: 'slack', subBlocks, canonicalModes }], + resolve + ) + } + + it('detects {{ENV}} in an ACTIVE advanced member - it executes, so it gates the sync', () => { + const scan = scanEnv( + { + channel: entry('channel', 'channel-selector', ''), + manualChannel: entry('manualChannel', 'short-input', '{{SLACK_CHANNEL}}'), + }, + { channel: 'advanced' } + ) + expect(scan.references).toEqual([ + expect.objectContaining({ + kind: 'env-var', + sourceId: 'SLACK_CHANNEL', + subBlockKey: 'manualChannel', + required: true, + }), + ]) + // Unmapped by this resolver, so it is a required blocker rather than a silent pass. + expect(scan.unmapped.map((ref) => ref.sourceId)).toEqual(['SLACK_CHANNEL']) + }) + + it('detects it via the value heuristic too (no stored canonicalModes override)', () => { + const scan = scanEnv({ + channel: entry('channel', 'channel-selector', ''), + manualChannel: entry('manualChannel', 'short-input', '{{SLACK_CHANNEL}}'), + }) + expect(scan.references.map((ref) => ref.sourceId)).toEqual(['SLACK_CHANNEL']) + }) + + it('rewrite and detect agree: a mapped key is both recorded and rewritten', () => { + vi.mocked(getBlock).mockReturnValue(envPairBlock()) + const resolve: ForkReferenceResolver = (kind, id) => + kind === 'env-var' && id === 'SLACK_CHANNEL' ? 'SLACK_CHANNEL_PROD' : null + const result = remapForkSubBlocks( + { + channel: entry('channel', 'channel-selector', ''), + manualChannel: entry('manualChannel', 'short-input', '{{SLACK_CHANNEL}}'), + }, + resolve, + 'promote', + { blockType: 'slack', canonicalModes: { channel: 'advanced' } } + ) + expect(result.subBlocks.manualChannel.value).toBe('{{SLACK_CHANNEL_PROD}}') + expect(result.references.map((ref) => ref.sourceId)).toEqual(['SLACK_CHANNEL']) + expect(result.unmapped).toEqual([]) + }) + + it('still does NOT detect {{ENV}} in a DORMANT member (it never executes)', () => { + const scan = scanEnv( + { + channel: entry('channel', 'channel-selector', 'C123'), + manualChannel: entry('manualChannel', 'short-input', '{{SLACK_CHANNEL}}'), + }, + { channel: 'basic' } + ) + expect(scan.references.filter((ref) => ref.kind === 'env-var')).toEqual([]) + }) + + it('still does NOT detect {{ENV}} in a condition-hidden field (it never executes)', () => { + vi.mocked(getBlock).mockReturnValue( + blockWith([ + { id: 'mode', title: 'Mode', type: 'dropdown' }, + { + id: 'cloudKey', + title: 'Cloud Key', + type: 'short-input', + condition: { field: 'mode', value: 'cloud' }, + }, + ]) + ) + const scan = scanWorkflowReferences( + [ + { + id: 'b1', + name: 'Pi', + type: 'pi', + subBlocks: { + mode: entry('mode', 'dropdown', 'local'), + cloudKey: entry('cloudKey', 'short-input', '{{HIDDEN_SECRET}}'), + }, + }, + ], + () => null + ) + expect(scan.references).toEqual([]) + }) + + it('an active manual member keeps its RESOURCE-id escape hatch while its {{ENV}} is detected', () => { + vi.mocked(getBlock).mockReturnValue( + blockWith([ + { + id: 'kbSelector', + title: 'KB', + type: 'knowledge-base-selector', + canonicalParamId: 'knowledgeBaseId', + mode: 'basic', + }, + { + id: 'manualKbId', + title: 'KB ID', + type: 'knowledge-base-selector', + canonicalParamId: 'knowledgeBaseId', + mode: 'advanced', + }, + { id: 'note', title: 'Note', type: 'long-input', dependsOn: ['kbSelector'] }, + ]) + ) + const scan = scanWorkflowReferences( + [ + { + id: 'b1', + name: 'KB', + type: 'knowledge', + subBlocks: { + kbSelector: entry('kbSelector', 'knowledge-base-selector', ''), + manualKbId: entry('manualKbId', 'knowledge-base-selector', 'kb-typed-by-hand'), + note: entry('note', 'long-input', 'uses {{DEPENDENT_SECRET}}'), + }, + canonicalModes: { knowledgeBaseId: 'advanced' }, + }, + ], + () => null + ) + // The hand-typed resource id stays a user-owned escape hatch (unchanged policy)... + expect(scan.references.filter((ref) => ref.kind === 'knowledge-base')).toEqual([]) + // ...but a live secret under that manual parent still executes, so it is detected. + expect(scan.references.map((ref) => ref.sourceId)).toEqual(['DEPENDENT_SECRET']) + }) + it('nested tool: remaps a canonical-keyed param (and both keys when aliased)', () => { const tool = { type: 'tblblock', diff --git a/apps/sim/ee/workspace-forking/lib/remap/remap-references.ts b/apps/sim/ee/workspace-forking/lib/remap/remap-references.ts index d419669f296..9dfb419f3ac 100644 --- a/apps/sim/ee/workspace-forking/lib/remap/remap-references.ts +++ b/apps/sim/ee/workspace-forking/lib/remap/remap-references.ts @@ -868,6 +868,16 @@ export function remapForkSubBlocks( !dormant && (gates.isActiveManualMember(subBlockKey) || gates.isManualParentDependent(subBlockKey)) const detectionSkipped = dormant || verbatimManual || gates.isConditionHidden(subBlockKey) + // `{{ENV}}` detection is gated on EXECUTION, not on ownership. A dormant member and a + // condition-hidden field never execute, so their refs must not become sync blockers - but an + // ACTIVE MANUAL member is exactly the value that DOES execute, and its `{{KEY}}` is a live + // secret reference like any other. Sharing `detectionSkipped` here made the two halves + // disagree: `remapEnvInValue` below rewrites a manual member's ref unconditionally, while + // detection suppressed it - so the key could never originate a mapping entry, and a target + // missing that secret silently passed the required-env gate instead of blocking the sync. + // Resource-id detection keeps `verbatimManual` (a hand-typed id stays a user-owned escape + // hatch); only env refs, which are never workspace-scoped ids, are detected here. + const envDetectionSkipped = dormant || gates.isConditionHidden(subBlockKey) if (dormant && isNonEmptyValue(value)) { value = '' } @@ -970,11 +980,12 @@ export function remapForkSubBlocks( if (value !== valueBeforeResource) remappedKeys.add(subBlockKey) // Promote rewrites `{{ENV}}` refs via the resolver; fork preserves them by name. A hidden - // field's ref is rewritten (kept verbatim when unmapped) but not recorded - it never - // executes, so it must not become a required sync blocker. + // (or dormant) field's ref is rewritten (kept verbatim when unmapped) but not recorded - it + // never executes, so it must not become a required sync blocker. An ACTIVE MANUAL member's + // ref IS recorded (see {@link envDetectionSkipped}) - it executes, so it must gate the sync. if (mode === 'promote') { value = remapEnvInValue(value, resolve, (sourceId, mapped) => { - if (detectionSkipped) return + if (envDetectionSkipped) return recordReference( `env-var:${sourceId}`, {