Add some more tests around quoting, fix bugs

Author: rix0rrr

packages/@aws-cdk/core/lib/private/cloudformation-lang.ts

@@ -209,7 +209,7 @@ function tokenAwareStringify(root: any, space: number, ctx: IResolveContext) {
     switch (resolvedTypeHint(intrinsic)) {
       case ResolutionTypeHint.STRING:
         pushLiteral('"');
-        pushIntrinsic(quoteInsideIntrinsic(intrinsic));
+        pushIntrinsic(deepQuoteStringLiterals(intrinsic));
         pushLiteral('"');
         break;
 
@@ -368,22 +368,16 @@ function* definedEntries<A extends object>(xs: A): IterableIterator<[string, any
  * Logical IDs and attribute names, which cannot contain quotes anyway. Hence,
  * we can get away not caring about the distinction and just quoting everything.
  */
-function quoteInsideIntrinsic(x: any): any {
-  if (typeof x === 'object' && x != null && Object.keys(x).length === 1) {
-    const key = Object.keys(x)[0];
-    const params = x[key];
-    switch (key) {
-      case 'Fn::If':
-        return { 'Fn::If': [params[0], quoteInsideIntrinsic(params[1]), quoteInsideIntrinsic(params[2])] };
-      case 'Fn::Join':
-        return { 'Fn::Join': [quoteInsideIntrinsic(params[0]), params[1].map(quoteInsideIntrinsic)] };
-      case 'Fn::Sub':
-        if (Array.isArray(params)) {
-          return { 'Fn::Sub': [quoteInsideIntrinsic(params[0]), params[1]] };
-        } else {
-          return { 'Fn::Sub': quoteInsideIntrinsic(params[0]) };
-        }
+function deepQuoteStringLiterals(x: any): any {
+  if (Array.isArray(x)) {
+    return x.map(deepQuoteStringLiterals);
+  }
+  if (typeof x === 'object' && x != null) {
+    const ret: any = {};
+    for (const [key, value] of Object.entries(x)) {
+      ret[deepQuoteStringLiterals(key)] = deepQuoteStringLiterals(value);
     }
+    return ret;
   }
   if (typeof x === 'string') {
     return quoteString(x);

packages/@aws-cdk/core/test/cloudformation-json.test.ts

@@ -1,4 +1,4 @@
-import { App, CfnOutput, Fn, IPostProcessor, IResolvable, IResolveContext, Lazy, Stack, Token } from '../lib';
+import { App, Aws, CfnOutput, Fn, IPostProcessor, IResolvable, IResolveContext, Lazy, Stack, Token } from '../lib';
 import { Intrinsic } from '../lib/private/intrinsic';
 import { evaluateCFN } from './evaluate-cfn';
 
@@ -196,6 +196,35 @@ describe('tokens returning CloudFormation intrinsics', () => {
     expect(evaluateCFN(resolved, context)).toEqual(expected);
   });
 
+  test('embedded string literals are escaped in Fn.sub (implicit references)', () => {
+    // GIVEN
+    const token = Fn.sub('I am in account "${AWS::AccountId}"');
+
+    // WHEN
+    const resolved = stack.resolve(stack.toJsonString({ token }));
+
+    // THEN
+    const context = { 'AWS::AccountId': '1234' };
+    const expected = '{"token":"I am in account \\"1234\\""}';
+    expect(evaluateCFN(resolved, context)).toEqual(expected);
+  });
+
+  test('embedded string literals are escaped in Fn.sub (explicit references)', () => {
+    // GIVEN
+    const token = Fn.sub('I am in account "${Acct}", also wanted to say: ${Also}', {
+      Acct: Aws.ACCOUNT_ID,
+      Also: '"hello world"',
+    });
+
+    // WHEN
+    const resolved = stack.resolve(stack.toJsonString({ token }));
+
+    // THEN
+    const context = { 'AWS::AccountId': '1234' };
+    const expected = '{"token":"I am in account \\"1234\\", also wanted to say: \\"hello world\\""}';
+    expect(evaluateCFN(resolved, context)).toEqual(expected);
+  });
+
   test('Tokens in Tokens are handled correctly', () => {
     // GIVEN
     const bucketName = new Intrinsic({ Ref: 'MyBucket' });
@@ -344,6 +373,26 @@ describe('tokens returning CloudFormation intrinsics', () => {
   });
 });
 
+test('JSON strings nested inside JSON strings have correct quoting', () => {
+  // GIVEN
+  const payload = stack.toJsonString({
+    message: Fn.sub('I am in account "${AWS::AccountId}"'),
+  });
+
+  // WHEN
+  const resolved = stack.resolve(stack.toJsonString({ payload }));
+
+  // THEN
+  const context = { 'AWS::AccountId': '1234' };
+  const expected = '{"payload":"{\\"message\\":\\"I am in account \\\\\\"1234\\\\\\"\\"}"}';
+  const evaluated = evaluateCFN(resolved, context);
+  expect(evaluated).toEqual(expected);
+
+  // Is this even correct? Let's ask JavaScript because I have trouble reading this many backslashes.
+  expect(JSON.parse(JSON.parse(evaluated).payload).message).toEqual('I am in account "1234"');
+});
+
+
 /**
  * Return two Tokens, one of which evaluates to a Token directly, one which evaluates to it lazily
  */

packages/@aws-cdk/core/test/evaluate-cfn.ts

@@ -42,16 +42,8 @@ export function evaluateCFN(object: any, context: {[key: string]: string} = {}):
       return context[key];
     },
 
-    'Fn::Sub'(argument: string | [string, Record<string, string>]) {
-      let template;
-      let placeholders: Record<string, string>;
-      if (Array.isArray(argument)) {
-        template = argument[0];
-        placeholders = evaluate(argument[1]);
-      } else {
-        template = argument;
-        placeholders = context;
-      }
+    'Fn::Sub'(template: string, explicitPlaceholders?: Record<string, string>) {
+      const placeholders = explicitPlaceholders ? evaluate(explicitPlaceholders) : context;
 
       if (typeof template !== 'string') {
         throw new Error('The first argument to {Fn::Sub} must be a string literal (cannot be the result of an expression)');
@@ -79,7 +71,7 @@ export function evaluateCFN(object: any, context: {[key: string]: string} = {}):
 
       const ret: {[key: string]: any} = {};
       for (const key of Object.keys(obj)) {
-        ret[key] = evaluateCFN(obj[key]);
+        ret[key] = evaluate(obj[key]);
       }
       return ret;
     }