fix(cloudformation-diff): fails on CloudFormation intrinsics in unexpected places

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