Skip to content

Commit a82d2f1

Browse files
committed
fix(iam): throw error when statement id is invalid (#34819)
Resolves #34819 by throwing error when PolicyStatement Statement id contains characters not allowed by the API. See docs in https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_sid.html
1 parent 1965014 commit a82d2f1

File tree

3 files changed

+103
-0
lines changed

3 files changed

+103
-0
lines changed
Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
import * as iam from 'aws-cdk-lib/aws-iam';
2+
import * as cdk from 'aws-cdk-lib';
3+
import * as integ from '@aws-cdk/integ-tests-alpha';
4+
5+
/**
6+
* Integration test to verify that PolicyStatements with valid alphanumeric SIDs
7+
* can be successfully deployed to AWS.
8+
*
9+
* This test validates that the SID validation logic doesn't interfere with
10+
* actual CloudFormation deployment of valid PolicyStatements.
11+
*/
12+
13+
const app = new cdk.App();
14+
const stack = new cdk.Stack(app, 'PolicyStatementSidTest');
15+
16+
const role = new iam.Role(stack, 'TestRole', {
17+
assumedBy: new iam.ServicePrincipal('lambda.amazonaws.com'),
18+
inlinePolicies: {
19+
TestPolicy: new iam.PolicyDocument({
20+
statements: [
21+
new iam.PolicyStatement({
22+
sid: 'ValidSid123',
23+
effect: iam.Effect.ALLOW,
24+
actions: ['s3:GetObject'],
25+
resources: ['arn:aws:s3:::example-bucket/*'],
26+
}),
27+
new iam.PolicyStatement({
28+
sid: 'ALLCAPS',
29+
effect: iam.Effect.ALLOW,
30+
actions: ['s3:ListBucket'],
31+
resources: ['arn:aws:s3:::example-bucket'],
32+
}),
33+
new iam.PolicyStatement({
34+
sid: '123456',
35+
effect: iam.Effect.ALLOW,
36+
actions: ['logs:CreateLogGroup'],
37+
resources: ['*'],
38+
}),
39+
new iam.PolicyStatement({
40+
sid: 'abc123DEF',
41+
effect: iam.Effect.ALLOW,
42+
actions: ['logs:CreateLogStream'],
43+
resources: ['*'],
44+
}),
45+
// Test statement without SID (should still work)
46+
new iam.PolicyStatement({
47+
effect: iam.Effect.ALLOW,
48+
actions: ['logs:PutLogEvents'],
49+
resources: ['*'],
50+
}),
51+
],
52+
}),
53+
},
54+
});
55+
56+
const managedPolicy = new iam.ManagedPolicy(stack, 'TestManagedPolicy', {
57+
statements: [
58+
new iam.PolicyStatement({
59+
sid: 'ManagedPolicySid1',
60+
effect: iam.Effect.ALLOW,
61+
actions: ['dynamodb:GetItem'],
62+
resources: ['*'],
63+
}),
64+
],
65+
});
66+
67+
role.addManagedPolicy(managedPolicy);
68+
69+
new integ.IntegTest(app, 'PolicyStatementSidIntegTest', {
70+
testCases: [stack],
71+
});

packages/aws-cdk-lib/aws-iam/lib/policy-statement.ts

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,7 @@ export class PolicyStatement {
9292

9393
constructor(props: PolicyStatementProps = {}) {
9494
this._sid = props.sid;
95+
this.validateStatementId(this._sid);
9596
this._effect = props.effect || Effect.ALLOW;
9697

9798
this.addActions(...props.actions || []);
@@ -117,6 +118,7 @@ export class PolicyStatement {
117118
*/
118119
public set sid(sid: string | undefined) {
119120
this.assertNotFrozen('sid');
121+
this.validateStatementId(sid);
120122
this._sid = sid;
121123
}
122124

@@ -232,6 +234,12 @@ export class PolicyStatement {
232234
}
233235
}
234236

237+
private validateStatementId(sid?: string) {
238+
if (sid !== undefined && !cdk.Token.isUnresolved(sid) && !/^[0-9A-Za-z]*$/.test(sid)) {
239+
throw new UnscopedValidationError(`Statement ID (sid) must be alphanumeric. Got '${sid}'. The Sid element supports ASCII uppercase letters (A-Z), lowercase letters (a-z), and numbers (0-9).`);
240+
}
241+
}
242+
235243
private validatePolicyActions(actions: string[]) {
236244
// In case of an unresolved list of actions return early
237245
if (cdk.Token.isUnresolved(actions)) return;

packages/aws-cdk-lib/aws-iam/test/policy-statement.test.ts

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -260,4 +260,28 @@ describe('IAM policy statement', () => {
260260
expect(mod).toThrow(/can no longer be modified/);
261261
}
262262
});
263+
264+
test('accepts valid alphanumeric sid', () => {
265+
const validSids = ['ValidSid123', 'ALLCAPS', '123456', 'abc123DEF'];
266+
267+
for (const sid of validSids) {
268+
expect(() => new PolicyStatement({ sid })).not.toThrow();
269+
}
270+
});
271+
272+
test('throws error when sid contains non-alphanumeric characters', () => {
273+
const invalidSids = ['invalid-sid', 'with space', 'with_underscore', 'with.dot', 'with!special@chars'];
274+
275+
for (const sid of invalidSids) {
276+
expect(() => new PolicyStatement({ sid }))
277+
.toThrow(`Statement ID (sid) must be alphanumeric. Got '${sid}'. The Sid element supports ASCII uppercase letters (A-Z), lowercase letters (a-z), and numbers (0-9).`);
278+
}
279+
});
280+
281+
test('throws error when setting sid to non-alphanumeric value', () => {
282+
const statement = new PolicyStatement();
283+
284+
expect(() => statement.sid = 'invalid-sid')
285+
.toThrow('Statement ID (sid) must be alphanumeric. Got \'invalid-sid\'. The Sid element supports ASCII uppercase letters (A-Z), lowercase letters (a-z), and numbers (0-9).');
286+
});
263287
});

0 commit comments

Comments
 (0)