Skip to content

Commit aa959d4

Browse files
VirtueMeclaude
andcommitted
fix(notifications): merge SNS/SQS policies for shared topics and queues
AWS::SNS::TopicPolicy and AWS::SQS::QueuePolicy are full replacements: when two separate CloudFormation resources target the same topic/queue, the second overwrites the first. When multiple state machines share a notification topic or queue (fan-in), the second machine's policy destroys the first machine's permissions — its notification rules stop working silently. Fix: after compiling all notification resources, group SNS topic policies by topic and SQS queue policies by queue. Same-target policies are merged into a single resource with all statements combined, so CloudFormation sets the policy exactly once with all principals authorised. Adds three unit tests covering: - Two state machines sharing the same SNS topic - Two state machines sharing the same SQS queue - One state machine with the same topic on multiple statuses Updates the notifications integration fixture to include a second state machine sharing the same SNS topic and SQS queue as the first, matching the fan-in scenario described in #275. Closes #275 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent 31b3ec3 commit aa959d4

File tree

3 files changed

+143
-1
lines changed

3 files changed

+143
-1
lines changed

fixtures/notifications/serverless.yml

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,3 +29,22 @@ stepFunctions:
2929
SUCCEEDED:
3030
- sqs:
3131
Fn::GetAtt: [NotificationQueue, Arn]
32+
33+
# Second state machine sharing the same SNS topic and SQS queue (fan-in).
34+
# Reproduces issue #275: without the fix, this machine's TopicPolicy and
35+
# QueuePolicy overwrite the first machine's, leaving it unable to publish.
36+
notificationMachine2:
37+
name: integration-notifications-2-${opt:stage, 'test'}
38+
definition:
39+
StartAt: PassThrough
40+
States:
41+
PassThrough:
42+
Type: Pass
43+
End: true
44+
notifications:
45+
FAILED:
46+
- sns:
47+
Ref: NotificationTopic
48+
SUCCEEDED:
49+
- sqs:
50+
Fn::GetAtt: [NotificationQueue, Arn]

lib/deploy/stepFunctions/compileNotifications.js

Lines changed: 49 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -387,6 +387,54 @@ function validateConfig(serverless, stateMachineName, stateMachineObj, notificat
387387
return true;
388388
}
389389

390+
// Merge AWS::SNS::TopicPolicy and AWS::SQS::QueuePolicy resources that target
391+
// the same topic/queue into a single resource with combined statements.
392+
//
393+
// CloudFormation treats these resource types as full policy replacements: if two
394+
// separate resources target the same topic/queue, the second one overwrites the
395+
// first. When multiple state machines share a notification topic/queue (fan-in),
396+
// this destroys the first machine's permissions. Merging into one resource with
397+
// all statements ensures CloudFormation sets the policy exactly once, correctly.
398+
function mergeResourcePolicies(resourcePairs) {
399+
const snsMap = new Map(); // JSON-serialised Topics array → { logicalId, resource }
400+
const sqsMap = new Map(); // JSON-serialised Queues array → { logicalId, resource }
401+
const result = {};
402+
403+
for (const [logicalId, resource] of resourcePairs) {
404+
if (resource.Type === 'AWS::SNS::TopicPolicy') {
405+
const key = JSON.stringify(resource.Properties.Topics);
406+
if (snsMap.has(key)) {
407+
const incoming = [].concat(resource.Properties.PolicyDocument.Statement);
408+
snsMap.get(key).resource.Properties.PolicyDocument.Statement.push(...incoming);
409+
} else {
410+
const merged = _.cloneDeep(resource);
411+
merged.Properties.PolicyDocument.Statement = [].concat(
412+
merged.Properties.PolicyDocument.Statement,
413+
);
414+
snsMap.set(key, { logicalId, resource: merged });
415+
result[logicalId] = merged;
416+
}
417+
} else if (resource.Type === 'AWS::SQS::QueuePolicy') {
418+
const key = JSON.stringify(resource.Properties.Queues);
419+
if (sqsMap.has(key)) {
420+
const incoming = [].concat(resource.Properties.PolicyDocument.Statement);
421+
sqsMap.get(key).resource.Properties.PolicyDocument.Statement.push(...incoming);
422+
} else {
423+
const merged = _.cloneDeep(resource);
424+
merged.Properties.PolicyDocument.Statement = [].concat(
425+
merged.Properties.PolicyDocument.Statement,
426+
);
427+
sqsMap.set(key, { logicalId, resource: merged });
428+
result[logicalId] = merged;
429+
}
430+
} else {
431+
result[logicalId] = resource;
432+
}
433+
}
434+
435+
return result;
436+
}
437+
390438
module.exports = {
391439
compileNotifications() {
392440
logger.config(this.serverless, this.v3Api);
@@ -412,7 +460,7 @@ module.exports = {
412460

413461
return Array.from(resourcesIterator);
414462
});
415-
const newResources = _.fromPairs(newResourcePairs);
463+
const newResources = mergeResourcePolicies(newResourcePairs);
416464

417465
_.merge(
418466
this.serverless.service.provider.compiledCloudFormationTemplate.Resources,

lib/deploy/stepFunctions/compileNotifications.test.js

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -623,4 +623,79 @@ describe('#compileNotifications', () => {
623623

624624
expect(resources.Beta1NotificationsIamRole.Properties.Path).to.equal('/teamA/');
625625
});
626+
627+
it('should produce a single SNS topic policy when multiple state machines share the same topic', () => {
628+
// Reproduces issue #275: each state machine generates its own AWS::SNS::TopicPolicy
629+
// for the shared topic. CloudFormation applies them sequentially — the second
630+
// overwrites the first, leaving the first machine's notifications unauthorised.
631+
// The fix merges policies for the same topic into a single resource with combined
632+
// statements so CloudFormation only sets the policy once, correctly.
633+
serverless.service.stepFunctions = {
634+
stateMachines: {
635+
machine1: genStateMachineWithTargets('Machine1', [{ sns: 'arn:aws:sns:us-east-1:123:shared-topic' }]),
636+
machine2: genStateMachineWithTargets('Machine2', [{ sns: 'arn:aws:sns:us-east-1:123:shared-topic' }]),
637+
},
638+
};
639+
640+
serverlessStepFunctions.compileNotifications();
641+
const resources = serverlessStepFunctions.serverless.service
642+
.provider.compiledCloudFormationTemplate.Resources;
643+
644+
const snsPolicies = _.values(resources).filter((r) => r.Type === 'AWS::SNS::TopicPolicy');
645+
const policiesForSharedTopic = snsPolicies.filter((p) => _.isEqual(
646+
p.Properties.Topics[0],
647+
'arn:aws:sns:us-east-1:123:shared-topic',
648+
));
649+
650+
expect(policiesForSharedTopic).to.have.lengthOf(1);
651+
652+
// Both machines × 5 statuses = 10 statements in the merged policy
653+
const statements = [].concat(policiesForSharedTopic[0].Properties.PolicyDocument.Statement);
654+
expect(statements.length).to.equal(10);
655+
});
656+
657+
it('should produce a single SQS queue policy when multiple state machines share the same queue', () => {
658+
serverless.service.stepFunctions = {
659+
stateMachines: {
660+
machine1: genStateMachineWithTargets('Machine1', [{ sqs: 'arn:aws:sqs:us-east-1:123:shared-queue' }]),
661+
machine2: genStateMachineWithTargets('Machine2', [{ sqs: 'arn:aws:sqs:us-east-1:123:shared-queue' }]),
662+
},
663+
};
664+
665+
serverlessStepFunctions.compileNotifications();
666+
const resources = serverlessStepFunctions.serverless.service
667+
.provider.compiledCloudFormationTemplate.Resources;
668+
669+
const sqsPolicies = _.values(resources).filter((r) => r.Type === 'AWS::SQS::QueuePolicy');
670+
expect(sqsPolicies).to.have.lengthOf(1);
671+
672+
const statements = [].concat(sqsPolicies[0].Properties.PolicyDocument.Statement);
673+
expect(statements.length).to.equal(10);
674+
});
675+
676+
it('should merge SNS policies when same topic is used for multiple statuses in one state machine', () => {
677+
serverless.service.stepFunctions = {
678+
stateMachines: {
679+
machine1: {
680+
id: 'Machine1',
681+
name: 'Machine1',
682+
definition: { StartAt: 'A', States: { A: { Type: 'Pass', End: true } } },
683+
notifications: {
684+
SUCCEEDED: [{ sns: 'arn:aws:sns:us-east-1:123:shared-topic' }],
685+
FAILED: [{ sns: 'arn:aws:sns:us-east-1:123:shared-topic' }],
686+
},
687+
},
688+
},
689+
};
690+
691+
serverlessStepFunctions.compileNotifications();
692+
const resources = serverlessStepFunctions.serverless.service
693+
.provider.compiledCloudFormationTemplate.Resources;
694+
695+
const snsPolicies = _.values(resources).filter((r) => r.Type === 'AWS::SNS::TopicPolicy');
696+
expect(snsPolicies).to.have.lengthOf(1);
697+
698+
const statements = [].concat(snsPolicies[0].Properties.PolicyDocument.Statement);
699+
expect(statements.length).to.equal(2);
700+
});
626701
});

0 commit comments

Comments
 (0)