Skip to content

Commit ec8b988

Browse files
icecrasher321icecrasher321
authored
fix(forks): detect secrets referenced from advanced-mode fields (#6566)
1 parent e31d2a9 commit ec8b988

2 files changed

Lines changed: 182 additions & 3 deletions

File tree

‎apps/sim/ee/workspace-forking/lib/remap/remap-references.test.ts‎

Lines changed: 168 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ import {
2727
clearDependentsOnRemap,
2828
collectClearedDependents,
2929
createForkSubBlockTransform,
30+
type ForkReferenceResolver,
3031
parseNestedDependentKey,
3132
readTargetDraftDependentValue,
3233
remapForkSubBlocks,
@@ -1394,6 +1395,173 @@ describe('canonical mode policy (fork/promote)', () => {
13941395
expect(scan.references.map((ref) => ref.sourceId)).toEqual(['kb-active'])
13951396
})
13961397

1398+
/**
1399+
* `{{ENV}}` detection is gated on EXECUTION, not on ownership - unlike resource ids, which
1400+
* follow the verbatim/user-owned policy above. The shipped shape this protects is a Slack
1401+
* block whose advanced "Channel ID" (`manualChannel`) holds a `{{SECRET}}`: that field is
1402+
* live, so the secret must surface as a mapping entry and gate the sync. Suppressing it made
1403+
* the rewrite and detect halves disagree (`remapEnvInValue` rewrites a manual member's ref
1404+
* unconditionally), so the key could never originate a mapping row and a target missing that
1405+
* secret passed the required-env gate silently.
1406+
*/
1407+
const envPairBlock = () =>
1408+
blockWith([
1409+
{
1410+
id: 'channel',
1411+
title: 'Channel',
1412+
type: 'channel-selector',
1413+
canonicalParamId: 'channel',
1414+
mode: 'basic',
1415+
},
1416+
{
1417+
id: 'manualChannel',
1418+
title: 'Channel ID',
1419+
type: 'short-input',
1420+
canonicalParamId: 'channel',
1421+
mode: 'advanced',
1422+
},
1423+
])
1424+
1425+
const scanEnv = (
1426+
subBlocks: Record<string, unknown>,
1427+
canonicalModes?: Record<string, 'basic' | 'advanced'>,
1428+
resolve: ForkReferenceResolver = () => null
1429+
) => {
1430+
vi.mocked(getBlock).mockReturnValue(envPairBlock())
1431+
return scanWorkflowReferences(
1432+
[{ id: 'b1', name: 'Slack', type: 'slack', subBlocks, canonicalModes }],
1433+
resolve
1434+
)
1435+
}
1436+
1437+
it('detects {{ENV}} in an ACTIVE advanced member - it executes, so it gates the sync', () => {
1438+
const scan = scanEnv(
1439+
{
1440+
channel: entry('channel', 'channel-selector', ''),
1441+
manualChannel: entry('manualChannel', 'short-input', '{{SLACK_CHANNEL}}'),
1442+
},
1443+
{ channel: 'advanced' }
1444+
)
1445+
expect(scan.references).toEqual([
1446+
expect.objectContaining({
1447+
kind: 'env-var',
1448+
sourceId: 'SLACK_CHANNEL',
1449+
subBlockKey: 'manualChannel',
1450+
required: true,
1451+
}),
1452+
])
1453+
// Unmapped by this resolver, so it is a required blocker rather than a silent pass.
1454+
expect(scan.unmapped.map((ref) => ref.sourceId)).toEqual(['SLACK_CHANNEL'])
1455+
})
1456+
1457+
it('detects it via the value heuristic too (no stored canonicalModes override)', () => {
1458+
const scan = scanEnv({
1459+
channel: entry('channel', 'channel-selector', ''),
1460+
manualChannel: entry('manualChannel', 'short-input', '{{SLACK_CHANNEL}}'),
1461+
})
1462+
expect(scan.references.map((ref) => ref.sourceId)).toEqual(['SLACK_CHANNEL'])
1463+
})
1464+
1465+
it('rewrite and detect agree: a mapped key is both recorded and rewritten', () => {
1466+
vi.mocked(getBlock).mockReturnValue(envPairBlock())
1467+
const resolve: ForkReferenceResolver = (kind, id) =>
1468+
kind === 'env-var' && id === 'SLACK_CHANNEL' ? 'SLACK_CHANNEL_PROD' : null
1469+
const result = remapForkSubBlocks(
1470+
{
1471+
channel: entry('channel', 'channel-selector', ''),
1472+
manualChannel: entry('manualChannel', 'short-input', '{{SLACK_CHANNEL}}'),
1473+
},
1474+
resolve,
1475+
'promote',
1476+
{ blockType: 'slack', canonicalModes: { channel: 'advanced' } }
1477+
)
1478+
expect(result.subBlocks.manualChannel.value).toBe('{{SLACK_CHANNEL_PROD}}')
1479+
expect(result.references.map((ref) => ref.sourceId)).toEqual(['SLACK_CHANNEL'])
1480+
expect(result.unmapped).toEqual([])
1481+
})
1482+
1483+
it('still does NOT detect {{ENV}} in a DORMANT member (it never executes)', () => {
1484+
const scan = scanEnv(
1485+
{
1486+
channel: entry('channel', 'channel-selector', 'C123'),
1487+
manualChannel: entry('manualChannel', 'short-input', '{{SLACK_CHANNEL}}'),
1488+
},
1489+
{ channel: 'basic' }
1490+
)
1491+
expect(scan.references.filter((ref) => ref.kind === 'env-var')).toEqual([])
1492+
})
1493+
1494+
it('still does NOT detect {{ENV}} in a condition-hidden field (it never executes)', () => {
1495+
vi.mocked(getBlock).mockReturnValue(
1496+
blockWith([
1497+
{ id: 'mode', title: 'Mode', type: 'dropdown' },
1498+
{
1499+
id: 'cloudKey',
1500+
title: 'Cloud Key',
1501+
type: 'short-input',
1502+
condition: { field: 'mode', value: 'cloud' },
1503+
},
1504+
])
1505+
)
1506+
const scan = scanWorkflowReferences(
1507+
[
1508+
{
1509+
id: 'b1',
1510+
name: 'Pi',
1511+
type: 'pi',
1512+
subBlocks: {
1513+
mode: entry('mode', 'dropdown', 'local'),
1514+
cloudKey: entry('cloudKey', 'short-input', '{{HIDDEN_SECRET}}'),
1515+
},
1516+
},
1517+
],
1518+
() => null
1519+
)
1520+
expect(scan.references).toEqual([])
1521+
})
1522+
1523+
it('an active manual member keeps its RESOURCE-id escape hatch while its {{ENV}} is detected', () => {
1524+
vi.mocked(getBlock).mockReturnValue(
1525+
blockWith([
1526+
{
1527+
id: 'kbSelector',
1528+
title: 'KB',
1529+
type: 'knowledge-base-selector',
1530+
canonicalParamId: 'knowledgeBaseId',
1531+
mode: 'basic',
1532+
},
1533+
{
1534+
id: 'manualKbId',
1535+
title: 'KB ID',
1536+
type: 'knowledge-base-selector',
1537+
canonicalParamId: 'knowledgeBaseId',
1538+
mode: 'advanced',
1539+
},
1540+
{ id: 'note', title: 'Note', type: 'long-input', dependsOn: ['kbSelector'] },
1541+
])
1542+
)
1543+
const scan = scanWorkflowReferences(
1544+
[
1545+
{
1546+
id: 'b1',
1547+
name: 'KB',
1548+
type: 'knowledge',
1549+
subBlocks: {
1550+
kbSelector: entry('kbSelector', 'knowledge-base-selector', ''),
1551+
manualKbId: entry('manualKbId', 'knowledge-base-selector', 'kb-typed-by-hand'),
1552+
note: entry('note', 'long-input', 'uses {{DEPENDENT_SECRET}}'),
1553+
},
1554+
canonicalModes: { knowledgeBaseId: 'advanced' },
1555+
},
1556+
],
1557+
() => null
1558+
)
1559+
// The hand-typed resource id stays a user-owned escape hatch (unchanged policy)...
1560+
expect(scan.references.filter((ref) => ref.kind === 'knowledge-base')).toEqual([])
1561+
// ...but a live secret under that manual parent still executes, so it is detected.
1562+
expect(scan.references.map((ref) => ref.sourceId)).toEqual(['DEPENDENT_SECRET'])
1563+
})
1564+
13971565
it('nested tool: remaps a canonical-keyed param (and both keys when aliased)', () => {
13981566
const tool = {
13991567
type: 'tblblock',

‎apps/sim/ee/workspace-forking/lib/remap/remap-references.ts‎

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -868,6 +868,16 @@ export function remapForkSubBlocks(
868868
!dormant &&
869869
(gates.isActiveManualMember(subBlockKey) || gates.isManualParentDependent(subBlockKey))
870870
const detectionSkipped = dormant || verbatimManual || gates.isConditionHidden(subBlockKey)
871+
// `{{ENV}}` detection is gated on EXECUTION, not on ownership. A dormant member and a
872+
// condition-hidden field never execute, so their refs must not become sync blockers - but an
873+
// ACTIVE MANUAL member is exactly the value that DOES execute, and its `{{KEY}}` is a live
874+
// secret reference like any other. Sharing `detectionSkipped` here made the two halves
875+
// disagree: `remapEnvInValue` below rewrites a manual member's ref unconditionally, while
876+
// detection suppressed it - so the key could never originate a mapping entry, and a target
877+
// missing that secret silently passed the required-env gate instead of blocking the sync.
878+
// Resource-id detection keeps `verbatimManual` (a hand-typed id stays a user-owned escape
879+
// hatch); only env refs, which are never workspace-scoped ids, are detected here.
880+
const envDetectionSkipped = dormant || gates.isConditionHidden(subBlockKey)
871881
if (dormant && isNonEmptyValue(value)) {
872882
value = ''
873883
}
@@ -970,11 +980,12 @@ export function remapForkSubBlocks(
970980
if (value !== valueBeforeResource) remappedKeys.add(subBlockKey)
971981

972982
// Promote rewrites `{{ENV}}` refs via the resolver; fork preserves them by name. A hidden
973-
// field's ref is rewritten (kept verbatim when unmapped) but not recorded - it never
974-
// executes, so it must not become a required sync blocker.
983+
// (or dormant) field's ref is rewritten (kept verbatim when unmapped) but not recorded - it
984+
// never executes, so it must not become a required sync blocker. An ACTIVE MANUAL member's
985+
// ref IS recorded (see {@link envDetectionSkipped}) - it executes, so it must gate the sync.
975986
if (mode === 'promote') {
976987
value = remapEnvInValue(value, resolve, (sourceId, mapped) => {
977-
if (detectionSkipped) return
988+
if (envDetectionSkipped) return
978989
recordReference(
979990
`env-var:${sourceId}`,
980991
{

0 commit comments

Comments
 (0)