Skip to content

Commit 028ad7a

Browse files
VirtueMeclaude
andcommitted
fix(iam): prevent nested Fn::Sub when lambda Resource is a Fn::Sub expression (#302)
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 31b3ec3 commit 028ad7a

5 files changed

Lines changed: 137 additions & 2 deletions

File tree

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+
});

fixtures/package-lock.json

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

lib/deploy/stepFunctions/iamStrategies/lambda.js

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,19 @@ function getPermissions(state, { plugin }) {
8585
}];
8686
}
8787

88+
function getVersionedArn(functionArn) {
89+
if (_.has(functionArn, 'Fn::Sub')) {
90+
const sub = functionArn['Fn::Sub'];
91+
if (typeof sub === 'string') {
92+
return { 'Fn::Sub': `${sub}:*` };
93+
}
94+
if (Array.isArray(sub)) {
95+
return { 'Fn::Sub': [`${sub[0]}:*`, sub[1]] };
96+
}
97+
}
98+
return { 'Fn::Sub': ['${functionArn}:*', { functionArn }] };
99+
}
100+
88101
function getFallbackPermissions(state, { plugin }) {
89102
if (isIntrinsic(state.Resource) || !!state.Resource.match(/arn:aws(-[a-z]+)*:lambda/)) {
90103
const trimmedArn = trimAliasFromLambdaArn(state.Resource);
@@ -93,7 +106,7 @@ function getFallbackPermissions(state, { plugin }) {
93106
action: 'lambda:InvokeFunction',
94107
resource: [
95108
functionArn,
96-
{ 'Fn::Sub': ['${functionArn}:*', { functionArn }] },
109+
getVersionedArn(functionArn),
97110
],
98111
}];
99112
}

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

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -307,4 +307,43 @@ describe('lambda strategy — getFallbackPermissions (direct lambda ARN task res
307307
const result = getFallbackPermissions(state, { plugin });
308308
expect(result).to.deep.equal([]);
309309
});
310+
311+
it('should not produce a nested Fn::Sub when Resource is a Fn::Sub expression (issue #302)', () => {
312+
// serverless-pseudo-parameters converts #{AWS::Region} → ${AWS::Region} and wraps
313+
// the whole ARN in Fn::Sub. Putting that object as a variable value inside another
314+
// Fn::Sub array is invalid CloudFormation and causes MalformedPolicyDocument errors.
315+
const state = {
316+
Type: 'Task',
317+
Resource: {
318+
'Fn::Sub': 'arn:${AWS::Partition}:lambda:${AWS::Region}:${AWS::AccountId}:function:my-fn',
319+
},
320+
};
321+
const plugin = makePlugin();
322+
const result = getFallbackPermissions(state, { plugin });
323+
expect(result[0].action).to.equal('lambda:InvokeFunction');
324+
const arnList = result[0].resource;
325+
for (const arn of arnList) {
326+
if (arn && typeof arn === 'object' && Array.isArray(arn['Fn::Sub'])) {
327+
const [, varMap] = arn['Fn::Sub'];
328+
if (varMap) {
329+
for (const val of Object.values(varMap)) {
330+
expect(val, 'variable map value must not be a nested Fn::Sub').to.not.have.property('Fn::Sub');
331+
}
332+
}
333+
}
334+
}
335+
});
336+
337+
it('should generate a valid versioned ARN when Resource is a Fn::Sub string', () => {
338+
const fnSub = 'arn:${AWS::Partition}:lambda:${AWS::Region}:${AWS::AccountId}:function:my-fn';
339+
const state = {
340+
Type: 'Task',
341+
Resource: { 'Fn::Sub': fnSub },
342+
};
343+
const plugin = makePlugin();
344+
const result = getFallbackPermissions(state, { plugin });
345+
const arnList = result[0].resource;
346+
const versionedArn = arnList.find((a) => a && a['Fn::Sub'] === `${fnSub}:*`);
347+
expect(versionedArn, 'versioned ARN should be a simple Fn::Sub string with :* appended').to.not.equal(undefined);
348+
});
310349
});

0 commit comments

Comments
 (0)