Skip to content

Commit c0f554f

Browse files
VirtueMeclaude
andauthored
fix(iam): prevent nested Fn::Sub when lambda Resource is a Fn::Sub expression (#302) (#749)
When a Task state's Resource was a Fn::Sub object (produced by plugins like serverless-pseudo-parameters), the versioned ARN (:*) was generated as a Fn::Sub array with the Fn::Sub object as a variable map value — invalid CloudFormation that causes MalformedPolicyDocument errors on deploy. Introduces getVersionedArn() which appends :* directly to the Fn::Sub template string instead of nesting it as a variable value. Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent 6444f28 commit c0f554f

File tree

4 files changed

+136
-1
lines changed

4 files changed

+136
-1
lines changed

fixtures/basic-state-machine/serverless.yml

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,3 +24,13 @@ stepFunctions:
2424
Resource:
2525
Fn::GetAtt: [HelloLambdaFunction, Arn]
2626
End: true
27+
fnSubMachine:
28+
name: integration-basic-fn-sub-${opt:stage, 'test'}
29+
definition:
30+
StartAt: InvokeLambda
31+
States:
32+
InvokeLambda:
33+
Type: Task
34+
Resource:
35+
Fn::Sub: "arn:${AWS::Partition}:lambda:${AWS::Region}:${AWS::AccountId}:function:my-fn"
36+
End: true
Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
'use strict';
2+
3+
const fs = require('node:fs');
4+
const path = require('node:path');
5+
const expect = require('chai').expect;
6+
7+
const templatePath = path.join(__dirname, '.serverless', 'cloudformation-template-update-stack.json');
8+
9+
describe('basic-state-machine fixture — CloudFormation template', () => {
10+
let resources;
11+
12+
before(() => {
13+
const template = JSON.parse(fs.readFileSync(templatePath, 'utf8'));
14+
resources = template.Resources;
15+
});
16+
17+
it('should have an IAM role for each state machine', () => {
18+
const stateMachineRoles = Object.values(resources).filter(
19+
(r) => r.Type === 'AWS::IAM::Role'
20+
&& JSON.stringify(r).includes('states.'),
21+
);
22+
expect(stateMachineRoles).to.have.lengthOf(2);
23+
});
24+
25+
it('should grant lambda:InvokeFunction for a Fn::GetAtt resource reference', () => {
26+
// basicMachine uses Fn::GetAtt: [HelloLambdaFunction, Arn] — the generated
27+
// role should allow invoking that function and its aliases/versions (:*)
28+
const roles = Object.values(resources).filter((r) => r.Type === 'AWS::IAM::Role');
29+
const statements = roles.flatMap((r) => r.Properties.Policies[0].PolicyDocument.Statement);
30+
const lambdaStatement = statements.find((s) => {
31+
const actions = [].concat(s.Action);
32+
return actions.includes('lambda:InvokeFunction');
33+
});
34+
expect(lambdaStatement, 'should have a lambda:InvokeFunction statement').to.not.equal(undefined);
35+
const arnList = [].concat(lambdaStatement.Resource);
36+
const hasGetAtt = arnList.some((a) => a && a['Fn::GetAtt']);
37+
expect(hasGetAtt, 'should reference the Lambda function via Fn::GetAtt').to.equal(true);
38+
});
39+
40+
it('should not produce nested Fn::Sub when Resource is a Fn::Sub expression (issue #302)', () => {
41+
// fnSubMachine uses a Fn::Sub ARN (simulating serverless-pseudo-parameters output).
42+
// The versioned form (:*) must not nest a Fn::Sub inside a Fn::Sub variable map —
43+
// that is invalid CloudFormation and causes MalformedPolicyDocument errors.
44+
const roles = Object.values(resources).filter((r) => r.Type === 'AWS::IAM::Role');
45+
const statements = roles.flatMap((r) => r.Properties.Policies[0].PolicyDocument.Statement);
46+
const allArns = statements
47+
.filter((s) => [].concat(s.Action).includes('lambda:InvokeFunction'))
48+
.flatMap((s) => [].concat(s.Resource));
49+
50+
for (const arn of allArns) {
51+
if (arn && typeof arn === 'object' && Array.isArray(arn['Fn::Sub'])) {
52+
const [, varMap] = arn['Fn::Sub'];
53+
if (varMap) {
54+
for (const val of Object.values(varMap)) {
55+
expect(val, 'Fn::Sub variable map must not contain a nested Fn::Sub').to.not.have.property('Fn::Sub');
56+
}
57+
}
58+
}
59+
}
60+
});
61+
62+
it('should include a valid versioned ARN (:*) for the Fn::Sub resource', () => {
63+
const fnSubTemplate = 'arn:${AWS::Partition}:lambda:${AWS::Region}:${AWS::AccountId}:function:my-fn';
64+
const roles = Object.values(resources).filter((r) => r.Type === 'AWS::IAM::Role');
65+
const statements = roles.flatMap((r) => r.Properties.Policies[0].PolicyDocument.Statement);
66+
const allArns = statements
67+
.filter((s) => [].concat(s.Action).includes('lambda:InvokeFunction'))
68+
.flatMap((s) => [].concat(s.Resource));
69+
70+
const versionedArn = allArns.find((a) => a && a['Fn::Sub'] === `${fnSubTemplate}:*`);
71+
expect(versionedArn, 'versioned ARN should be a Fn::Sub string with :* appended').to.not.equal(undefined);
72+
});
73+
});

lib/deploy/stepFunctions/iamStrategies/lambda.js

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,19 @@ function getPermissions(state, { plugin }) {
9797
}];
9898
}
9999

100+
function getVersionedArn(functionArn) {
101+
if (_.has(functionArn, 'Fn::Sub')) {
102+
const sub = functionArn['Fn::Sub'];
103+
if (typeof sub === 'string') {
104+
return { 'Fn::Sub': `${sub}:*` };
105+
}
106+
if (Array.isArray(sub)) {
107+
return { 'Fn::Sub': [`${sub[0]}:*`, sub[1]] };
108+
}
109+
}
110+
return { 'Fn::Sub': ['${functionArn}:*', { functionArn }] };
111+
}
112+
100113
function getFallbackPermissions(state, { plugin }) {
101114
if (isIntrinsic(state.Resource) || !!state.Resource.match(/arn:aws(-[a-z]+)*:lambda/)) {
102115
const trimmedArn = trimAliasFromLambdaArn(state.Resource);
@@ -117,7 +130,7 @@ function getFallbackPermissions(state, { plugin }) {
117130
action: 'lambda:InvokeFunction',
118131
resource: [
119132
functionArn,
120-
{ 'Fn::Sub': ['${functionArn}:*', { functionArn }] },
133+
getVersionedArn(functionArn),
121134
],
122135
}];
123136
}

lib/deploy/stepFunctions/iamStrategies/lambda.test.js

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -362,4 +362,43 @@ describe('lambda strategy — getFallbackPermissions (direct lambda ARN task res
362362
const result = getFallbackPermissions(state, { plugin });
363363
expect(result).to.deep.equal([]);
364364
});
365+
366+
it('should not produce a nested Fn::Sub when Resource is a Fn::Sub expression (issue #302)', () => {
367+
// serverless-pseudo-parameters converts #{AWS::Region} → ${AWS::Region} and wraps
368+
// the whole ARN in Fn::Sub. Putting that object as a variable value inside another
369+
// Fn::Sub array is invalid CloudFormation and causes MalformedPolicyDocument errors.
370+
const state = {
371+
Type: 'Task',
372+
Resource: {
373+
'Fn::Sub': 'arn:${AWS::Partition}:lambda:${AWS::Region}:${AWS::AccountId}:function:my-fn',
374+
},
375+
};
376+
const plugin = makePlugin();
377+
const result = getFallbackPermissions(state, { plugin });
378+
expect(result[0].action).to.equal('lambda:InvokeFunction');
379+
const arnList = result[0].resource;
380+
for (const arn of arnList) {
381+
if (arn && typeof arn === 'object' && Array.isArray(arn['Fn::Sub'])) {
382+
const [, varMap] = arn['Fn::Sub'];
383+
if (varMap) {
384+
for (const val of Object.values(varMap)) {
385+
expect(val, 'variable map value must not be a nested Fn::Sub').to.not.have.property('Fn::Sub');
386+
}
387+
}
388+
}
389+
}
390+
});
391+
392+
it('should generate a valid versioned ARN when Resource is a Fn::Sub string', () => {
393+
const fnSub = 'arn:${AWS::Partition}:lambda:${AWS::Region}:${AWS::AccountId}:function:my-fn';
394+
const state = {
395+
Type: 'Task',
396+
Resource: { 'Fn::Sub': fnSub },
397+
};
398+
const plugin = makePlugin();
399+
const result = getFallbackPermissions(state, { plugin });
400+
const arnList = result[0].resource;
401+
const versionedArn = arnList.find((a) => a && a['Fn::Sub'] === `${fnSub}:*`);
402+
expect(versionedArn, 'versioned ARN should be a simple Fn::Sub string with :* appended').to.not.equal(undefined);
403+
});
365404
});

0 commit comments

Comments
 (0)