fix(cloudformation-diff): fails on CloudFormation intrinsics in unexp…

…ected places (#26791)

The `cloudformation-diff` module was written to parse templates that CDK itself would produce, mostly consisting of concrete values and barely any CloudFormation intrinsics. It would crash when encountering CloudFormation intrinsics in unexpected places (for example, an intrinsic where it expected an array).

Make the parsing more robust, checking the types of various values before we try and access it. Property-based tests generate random templates to make sure we didn't forget any edge cases.

Upgrade `fast-check` to the latest version while we're at it.

Fixes #7413.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

packages/@aws-cdk/cloudformation-diff/lib/diff/maybe-parsed.ts

@@ -0,0 +1,25 @@
+/**
+ * A value that may or may not be parseable
+ */
+export type MaybeParsed<A> = Parsed<A> | UnparseableCfn;
+
+export interface Parsed<A> {
+  readonly type: 'parsed';
+  readonly value: A;
+}
+
+export interface UnparseableCfn {
+  readonly type: 'unparseable';
+  readonly repr: string;
+}
+
+export function mkParsed<A>(value: A): Parsed<A> {
+  return { type: 'parsed', value };
+}
+
+export function mkUnparseable(value: any): UnparseableCfn {
+  return {
+    type: 'unparseable',
+    repr: typeof value === 'string' ? value : JSON.stringify(value),
+  };
+}

packages/@aws-cdk/cloudformation-diff/lib/diff/util.ts

@@ -113,6 +113,12 @@ export function diffKeyedEntities<T>(
   for (const logicalId of unionOf(Object.keys(oldValue || {}), Object.keys(newValue || {}))) {
     const oldElement = oldValue && oldValue[logicalId];
     const newElement = newValue && newValue[logicalId];
+
+    if (oldElement === undefined && newElement === undefined) {
+      // Shouldn't happen in reality, but may happen in tests. Skip.
+      continue;
+    }
+
     result[logicalId] = elementDiff(oldElement, newElement, logicalId);
   }
   return result;

packages/@aws-cdk/cloudformation-diff/lib/iam/iam-changes.ts

@@ -2,6 +2,7 @@ import * as cfnspec from '@aws-cdk/cfnspec';
 import * as chalk from 'chalk';
 import { ManagedPolicyAttachment, ManagedPolicyJson } from './managed-policy';
 import { parseLambdaPermission, parseStatements, Statement, StatementJson } from './statement';
+import { MaybeParsed } from '../diff/maybe-parsed';
 import { PropertyChange, PropertyMap, ResourceChange } from '../diff/types';
 import { DiffableCollection } from '../diffable';
 import { renderIntrinsics } from '../render-intrinsics';
@@ -184,7 +185,7 @@ export class IamChanges {
    * Parse a list of policies on an identity
    */
   private readIdentityPolicies(policies: any, logicalId: string): Statement[] {
-    if (policies === undefined) { return []; }
+    if (policies === undefined || !Array.isArray(policies)) { return []; }
 
     const appliesToPrincipal = 'AWS:${' + logicalId + '}';
 
@@ -276,8 +277,8 @@ function defaultResource(resource: string, statements: Statement[]) {
 }
 
 export interface IamChangesJson {
-  statementAdditions?: StatementJson[];
-  statementRemovals?: StatementJson[];
-  managedPolicyAdditions?: ManagedPolicyJson[];
-  managedPolicyRemovals?: ManagedPolicyJson[];
+  statementAdditions?: Array<MaybeParsed<StatementJson>>;
+  statementRemovals?: Array<MaybeParsed<StatementJson>>;
+  managedPolicyAdditions?: Array<MaybeParsed<ManagedPolicyJson>>;
+  managedPolicyRemovals?: Array<MaybeParsed<ManagedPolicyJson>>;
 }

packages/@aws-cdk/cloudformation-diff/lib/iam/managed-policy.ts

@@ -1,3 +1,5 @@
+import { MaybeParsed, mkParsed } from '../diff/maybe-parsed';
+
 export class ManagedPolicyAttachment {
   public static parseManagedPolicies(identityArn: string, arns: string | string[]): ManagedPolicyAttachment[] {
     return typeof arns === 'string'
@@ -19,8 +21,11 @@ export class ManagedPolicyAttachment {
    *
    * @internal
    */
-  public _toJson(): ManagedPolicyJson {
-    return { identityArn: this.identityArn, managedPolicyArn: this.managedPolicyArn };
+  public _toJson(): MaybeParsed<ManagedPolicyJson> {
+    return mkParsed({
+      identityArn: this.identityArn,
+      managedPolicyArn: this.managedPolicyArn,
+    });
   }
 }
 

packages/@aws-cdk/cloudformation-diff/lib/iam/statement.ts

@@ -1,3 +1,4 @@
+import { MaybeParsed, mkParsed, mkUnparseable } from '../diff/maybe-parsed';
 import { deepRemoveUndefined } from '../util';
 
 // namespace object imports won't work in the bundle for function exports
@@ -94,17 +95,17 @@ export class Statement {
    *
    * @internal
    */
-  public _toJson(): StatementJson {
+  public _toJson(): MaybeParsed<StatementJson> {
     return this.serializedIntrinsic
-      ? this.serializedIntrinsic
-      : deepRemoveUndefined({
+      ? mkUnparseable(this.serializedIntrinsic)
+      : mkParsed(deepRemoveUndefined({
         sid: this.sid,
         effect: this.effect,
         resources: this.resources._toJson(),
         principals: this.principals._toJson(),
         actions: this.actions._toJson(),
         condition: this.condition,
-      });
+      }));
   }
 
   /**

packages/@aws-cdk/cloudformation-diff/lib/network/security-group-changes.ts

@@ -97,12 +97,16 @@ export class SecurityGroupChanges {
   }
 
   private readInlineRules(rules: any, logicalId: string): SecurityGroupRule[] {
-    if (!rules) { return []; }
+    if (!rules || !Array.isArray(rules)) { return []; }
 
     // UnCloudFormation so the parser works in an easier domain
 
     const ref = '${' + logicalId + '.GroupId}';
-    return rules.map((r: any) => new SecurityGroupRule(renderIntrinsics(r), ref));
+    return rules.flatMap((r: any) => {
+      const rendered = renderIntrinsics(r);
+      // SecurityGroupRule is not robust against unparsed objects
+      return typeof rendered === 'object' ? [new SecurityGroupRule(rendered, ref)] : [];
+    });
   }
 
   private readRuleResource(resource: any): SecurityGroupRule[] {

packages/@aws-cdk/cloudformation-diff/lib/network/security-group-rule.ts

@@ -121,8 +121,12 @@ function peerEqual(a?: RulePeer, b?: RulePeer) {
 
 function findFirst<T>(obj: any, keys: string[], fn: (x: string) => T): T | undefined {
   for (const key of keys) {
-    if (key in obj) {
-      return fn(obj[key]);
+    try {
+      if (key in obj) {
+        return fn(obj[key]);
+      }
+    } catch (e) {
+      debugger;
     }
   }
   return undefined;

packages/@aws-cdk/cloudformation-diff/package.json

@@ -35,7 +35,7 @@
     "@aws-cdk/pkglint": "0.0.0",
     "@types/jest": "^29.5.3",
     "@types/string-width": "^4.0.1",
-    "fast-check": "^2.25.0",
+    "fast-check": "^3.12.0",
     "jest": "^29.6.2",
     "ts-jest": "^29.1.1"
   },

packages/@aws-cdk/cloudformation-diff/test/diff-template.test.ts

@@ -1,3 +1,5 @@
+import * as fc from 'fast-check';
+import { arbitraryTemplate } from './test-arbitraries';
 import { diffTemplate, ResourceImpact } from '../lib/diff-template';
 
 const POLICY_DOCUMENT = { foo: 'Bar' }; // Obviously a fake one!
@@ -689,3 +691,13 @@ test('handles a resource changing its Type', () => {
     resourceTypes: { newType: 'AWS::ApiGateway::RestApi', oldType: 'AWS::Serverless::Api' },
   });
 });
+
+test('diffing any two arbitrary templates should not crash', () => {
+  // We're not interested in making sure we find the right differences here -- just
+  // that we're not crashing.
+  fc.assert(fc.property(arbitraryTemplate, arbitraryTemplate, (t1, t2) => {
+    diffTemplate(t1, t2);
+  }), {
+    // path: '1:0:0:0:3:0:1:1:1:1:1:1:1:1:1:1:1:1:1:2:1:1:1',
+  });
+});

packages/@aws-cdk/cloudformation-diff/test/iam/detect-changes.test.ts

@@ -1,4 +1,7 @@
 import { diffTemplate } from '../../lib';
+import { MaybeParsed } from '../../lib/diff/maybe-parsed';
+import { IamChangesJson } from '../../lib/iam/iam-changes';
+import { deepRemoveUndefined } from '../../lib/util';
 import { poldoc, policy, resource, role, template } from '../util';
 
 test('shows new AssumeRolePolicyDocument', () => {
@@ -14,7 +17,7 @@ test('shows new AssumeRolePolicyDocument', () => {
   }));
 
   // THEN
-  expect(diff.iamChanges._toJson()).toEqual({
+  expect(unwrapParsed(diff.iamChanges._toJson())).toEqual({
     statementAdditions: [
       {
         effect: 'Allow',
@@ -41,7 +44,7 @@ test('implicitly knows principal of identity policy for all resource types', ()
     }));
 
     // THEN
-    expect(diff.iamChanges._toJson()).toEqual({
+    expect(unwrapParsed(diff.iamChanges._toJson())).toEqual({
       statementAdditions: [
         {
           effect: 'Allow',
@@ -73,7 +76,7 @@ test('policies on an identity object', () => {
     }));
 
     // THEN
-    expect(diff.iamChanges._toJson()).toEqual({
+    expect(unwrapParsed(diff.iamChanges._toJson())).toEqual({
       statementAdditions: [
         {
           effect: 'Allow',
@@ -86,6 +89,39 @@ test('policies on an identity object', () => {
   }
 });
 
+test('statement is an intrinsic', () => {
+  const diff = diffTemplate({}, template({
+    MyIdentity: resource('AWS::IAM::User', {
+      Policies: [
+        {
+          PolicyName: 'Polly',
+          PolicyDocument: poldoc({
+            'Fn::If': [
+              'SomeCondition',
+              {
+                Effect: 'Allow',
+                Action: 's3:DoThatThing',
+                Resource: '*',
+              },
+              { Ref: 'AWS::NoValue' },
+            ],
+          }),
+        },
+      ],
+    }),
+  }));
+
+  // THEN
+  expect(diff.iamChanges._toJson()).toEqual({
+    statementAdditions: [
+      {
+        type: 'unparseable',
+        repr: '{"Fn::If":["SomeCondition",{"Effect":"Allow","Action":"s3:DoThatThing","Resource":"*"}]}',
+      },
+    ],
+  });
+});
+
 test('if policy is attached to multiple roles all are shown', () => {
   // WHEN
   const diff = diffTemplate({}, template({
@@ -100,7 +136,7 @@ test('if policy is attached to multiple roles all are shown', () => {
   }));
 
   // THEN
-  expect(diff.iamChanges._toJson()).toEqual({
+  expect(unwrapParsed(diff.iamChanges._toJson())).toEqual({
     statementAdditions: [
       {
         effect: 'Allow',
@@ -131,7 +167,7 @@ test('correctly parses Lambda permissions', () => {
   }));
 
   // THEN
-  expect(diff.iamChanges._toJson()).toEqual({
+  expect(unwrapParsed(diff.iamChanges._toJson())).toEqual({
     statementAdditions: [
       {
         effect: 'Allow',
@@ -162,7 +198,7 @@ test('implicitly knows resource of (queue) resource policy even if * given', ()
   }));
 
   // THEN
-  expect(diff.iamChanges._toJson()).toEqual({
+  expect(unwrapParsed(diff.iamChanges._toJson())).toEqual({
     statementAdditions: [
       {
         effect: 'Allow',
@@ -189,7 +225,7 @@ test('finds sole statement removals', () => {
   }), {});
 
   // THEN
-  expect(diff.iamChanges._toJson()).toEqual({
+  expect(unwrapParsed(diff.iamChanges._toJson())).toEqual({
     statementRemovals: [
       {
         effect: 'Allow',
@@ -233,7 +269,7 @@ test('finds one of many statement removals', () => {
     }));
 
   // THEN
-  expect(diff.iamChanges._toJson()).toEqual({
+  expect(unwrapParsed(diff.iamChanges._toJson())).toEqual({
     statementRemovals: [
       {
         effect: 'Allow',
@@ -254,7 +290,7 @@ test('finds policy attachments', () => {
   }));
 
   // THEN
-  expect(diff.iamChanges._toJson()).toEqual({
+  expect(unwrapParsed(diff.iamChanges._toJson())).toEqual({
     managedPolicyAdditions: [
       {
         identityArn: '${SomeRole}',
@@ -279,7 +315,7 @@ test('finds policy removals', () => {
     }));
 
   // THEN
-  expect(diff.iamChanges._toJson()).toEqual({
+  expect(unwrapParsed(diff.iamChanges._toJson())).toEqual({
     managedPolicyRemovals: [
       {
         identityArn: '${SomeRole}',
@@ -314,7 +350,7 @@ test('queuepolicy queue change counts as removal+addition', () => {
   }));
 
   // THEN
-  expect(diff.iamChanges._toJson()).toEqual({
+  expect(unwrapParsed(diff.iamChanges._toJson())).toEqual({
     statementAdditions: [
       {
         effect: 'Allow',
@@ -354,7 +390,7 @@ test('supports Fn::If in the top-level property value of Role', () => {
   }));
 
   // THEN
-  expect(diff.iamChanges._toJson()).toEqual({
+  expect(unwrapParsed(diff.iamChanges._toJson())).toEqual({
     managedPolicyAdditions: [
       {
         identityArn: '${MyRole}',
@@ -420,3 +456,22 @@ test('supports Fn::If in the elements of an array-typed property of Role', () =>
   expect(changedPolicies[resourceColumn]).toContain('{"Fn::If":["SomeCondition",{"PolicyName":"S3","PolicyDocument":{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":"s3:GetObject","Resource":"*"}]}}]}');
   expect(changedPolicies[principalColumn]).toContain('AWS:${MyRole}');
 });
+
+/**
+ * Assume that all types are parsed, and unwrap them
+ */
+function unwrapParsed(chg: IamChangesJson) {
+  return deepRemoveUndefined({
+    managedPolicyAdditions: chg.managedPolicyAdditions?.map(unwrap1),
+    managedPolicyRemovals: chg.managedPolicyRemovals?.map(unwrap1),
+    statementAdditions: chg.statementAdditions?.map(unwrap1),
+    statementRemovals: chg.statementRemovals?.map(unwrap1),
+  });
+
+  function unwrap1<A>(x: MaybeParsed<A>): A {
+    if (x.type !== 'parsed') {
+      throw new Error(`Expected parsed expression, found: "${x.repr}"`);
+    }
+    return x.value;
+  }
+}