From b0fd809f05a04eebeb0271c2d0d9549e38fadf55 Mon Sep 17 00:00:00 2001 From: Daniel Schmidt Date: Fri, 29 Oct 2021 11:15:13 +0200 Subject: [PATCH 1/5] fix(lib): escape newlines in terraform functions Previously newlines were added to the synthesised JSON, which JSON only accepts if escaped Closes #1165 --- packages/cdktf/lib/tfExpression.ts | 22 +++++-- packages/cdktf/test/tfExpression.test.ts | 78 ++++++++++++++++++------ 2 files changed, 78 insertions(+), 22 deletions(-) diff --git a/packages/cdktf/lib/tfExpression.ts b/packages/cdktf/lib/tfExpression.ts index 8b2d0518df..4d1d206593 100644 --- a/packages/cdktf/lib/tfExpression.ts +++ b/packages/cdktf/lib/tfExpression.ts @@ -29,6 +29,16 @@ class TFExpression extends Intrinsic implements IResolvable { return resolvedArg; } + /** + * Escape string removes characters from the string that are not allowed in Terraform or JSON + * It must only be used on non-token values + */ + private escapeString(str: string) { + return str + .replace(/\n/g, "\\n") // escape newlines + .replace(/\${/g, "$$${"); // escape ${ to $${ + } + private resolveString(str: string, resolvedArg: any) { const tokenList = Tokenization.reverseString(str); const numberOfTokens = tokenList.tokens.length + tokenList.intrinsic.length; @@ -36,8 +46,8 @@ class TFExpression extends Intrinsic implements IResolvable { // String literal if (numberOfTokens === 0) { return resolvedArg.startsWith('"') && resolvedArg.endsWith('"') - ? resolvedArg - : `"${resolvedArg}"`; + ? this.escapeString(resolvedArg) + : `"${this.escapeString(resolvedArg)}"`; } // Only a token reference @@ -52,10 +62,14 @@ class TFExpression extends Intrinsic implements IResolvable { const rightTokens = Tokenization.reverse(right); const leftValue = - leftTokens.length === 0 ? left : `\${${leftTokens[0]}}`; + leftTokens.length === 0 + ? this.escapeString(left) + : `\${${leftTokens[0]}}`; const rightValue = - rightTokens.length === 0 ? right : `\${${rightTokens[0]}}`; + rightTokens.length === 0 + ? this.escapeString(right) + : `\${${rightTokens[0]}}`; return `${leftValue}${rightValue}`; }, diff --git a/packages/cdktf/test/tfExpression.test.ts b/packages/cdktf/test/tfExpression.test.ts index 0a324af4d7..a62b4b19a7 100644 --- a/packages/cdktf/test/tfExpression.test.ts +++ b/packages/cdktf/test/tfExpression.test.ts @@ -1,3 +1,4 @@ +import { Token } from "../lib"; import { addOperation, andOperation, @@ -16,9 +17,12 @@ import { orOperation, propertyAccess, ref, + call, subOperation, + Expression, } from "../lib/tfExpression"; import { resolve } from "../lib/_tokens"; +const resolveExpression = (expr: Expression) => resolve(null as any, expr); test("can render reference", () => { expect( @@ -28,8 +32,7 @@ test("can render reference", () => { test("propertyAccess renders correctly", () => { expect( - resolve( - null as any, + resolveExpression( propertyAccess(ref("some_resource.my_resource.some_attribute_array"), [ 0, "name", @@ -41,83 +44,122 @@ test("propertyAccess renders correctly", () => { }); test("conditional renders correctly", () => { - expect(resolve(null as any, conditional(true, 1, 0))).toMatchInlineSnapshot( + expect(resolveExpression(conditional(true, 1, 0))).toMatchInlineSnapshot( `"\${true ? 1 : 0}"` ); }); test("notOperation renders correctly", () => { - expect(resolve(null as any, notOperation(true))).toMatchInlineSnapshot( + expect(resolveExpression(notOperation(true))).toMatchInlineSnapshot( `"\${!true}"` ); }); test("negateOperation renders correctly", () => { - expect(resolve(null as any, negateOperation(1))).toMatchInlineSnapshot( + expect(resolveExpression(negateOperation(1))).toMatchInlineSnapshot( `"\${-1}"` ); }); test("mulOperation renders correctly", () => { - expect(resolve(null as any, mulOperation(2, 3))).toMatchInlineSnapshot( + expect(resolveExpression(mulOperation(2, 3))).toMatchInlineSnapshot( `"\${(2 * 3)}"` ); }); test("divOperation renders correctly", () => { - expect(resolve(null as any, divOperation(4, 2))).toMatchInlineSnapshot( + expect(resolveExpression(divOperation(4, 2))).toMatchInlineSnapshot( `"\${(4 / 2)}"` ); }); test("modOperation renders correctly", () => { - expect(resolve(null as any, modOperation(5, 3))).toMatchInlineSnapshot( + expect(resolveExpression(modOperation(5, 3))).toMatchInlineSnapshot( `"\${(5 % 3)}"` ); }); test("addOperation renders correctly", () => { - expect(resolve(null as any, addOperation(1, 1))).toMatchInlineSnapshot( + expect(resolveExpression(addOperation(1, 1))).toMatchInlineSnapshot( `"\${(1 + 1)}"` ); }); test("subOperation renders correctly", () => { - expect(resolve(null as any, subOperation(1, 2))).toMatchInlineSnapshot( + expect(resolveExpression(subOperation(1, 2))).toMatchInlineSnapshot( `"\${(1 - 2)}"` ); }); test("gtOperation renders correctly", () => { - expect(resolve(null as any, gtOperation(1, 2))).toMatchInlineSnapshot( + expect(resolveExpression(gtOperation(1, 2))).toMatchInlineSnapshot( `"\${(1 > 2)}"` ); }); test("gteOperation renders correctly", () => { - expect(resolve(null as any, gteOperation(1, 2))).toMatchInlineSnapshot( + expect(resolveExpression(gteOperation(1, 2))).toMatchInlineSnapshot( `"\${(1 >= 2)}"` ); }); test("ltOperation renders correctly", () => { - expect(resolve(null as any, ltOperation(1, 2))).toMatchInlineSnapshot( + expect(resolveExpression(ltOperation(1, 2))).toMatchInlineSnapshot( `"\${(1 < 2)}"` ); }); test("lteOperation renders correctly", () => { - expect(resolve(null as any, lteOperation(1, 2))).toMatchInlineSnapshot( + expect(resolveExpression(lteOperation(1, 2))).toMatchInlineSnapshot( `"\${(1 <= 2)}"` ); }); test("eqOperation renders correctly", () => { - expect(resolve(null as any, eqOperation(1, 1))).toMatchInlineSnapshot( + expect(resolveExpression(eqOperation(1, 1))).toMatchInlineSnapshot( `"\${(1 == 1)}"` ); }); test("neqOperation renders correctly", () => { - expect(resolve(null as any, neqOperation(1, 2))).toMatchInlineSnapshot( + expect(resolveExpression(neqOperation(1, 2))).toMatchInlineSnapshot( `"\${(1 != 2)}"` ); }); test("andOperation renders correctly", () => { - expect(resolve(null as any, andOperation(true, true))).toMatchInlineSnapshot( + expect(resolveExpression(andOperation(true, true))).toMatchInlineSnapshot( `"\${(true && true)}"` ); }); test("orOperation renders correctly", () => { - expect(resolve(null as any, orOperation(true, true))).toMatchInlineSnapshot( + expect(resolveExpression(orOperation(true, true))).toMatchInlineSnapshot( `"\${(true || true)}"` ); }); + +test("functions escape newlines", () => { + expect( + resolveExpression( + call("length", [ + ` +This +is +a +multi +line +string + `, + ]) + ) + ).toMatchInlineSnapshot( + `"\${length(\\"\\\\nThis\\\\nis\\\\na\\\\nmulti\\\\nline\\\\nstring\\\\n \\")}"` + ); +}); + +test("functions escape terraform reference like strings", () => { + expect(resolveExpression(call("length", [`\${}`]))).toMatchInlineSnapshot( + `"\${length(\\"$\${}\\")}"` + ); +}); + +test("functions don't escape terraform references", () => { + expect( + resolveExpression(call("length", [ref("docker_container.foo.bar")])) + ).toMatchInlineSnapshot(`"\${length(docker_container.foo.bar)}"`); +}); + +test("functions don't escape terraform references that have been tokenized", () => { + expect( + resolveExpression( + call("length", [Token.asString(ref("docker_container.foo.bar"))]) + ) + ).toMatchInlineSnapshot(`"\${length(docker_container.foo.bar)}"`); +}); From d5001a678f7a71f8b2a82b8ad06062d575de3e2e Mon Sep 17 00:00:00 2001 From: Daniel Schmidt Date: Mon, 1 Nov 2021 12:42:16 +0100 Subject: [PATCH 2/5] fix(lib): Escape double quotes in TFExpressions --- packages/cdktf/lib/terraform-functions.ts | 25 +++++++++++++++++- packages/cdktf/lib/tfExpression.ts | 27 +++++++++++++++++--- packages/cdktf/test/functions.test.ts | 31 +++++++++++++++++++++++ packages/cdktf/test/tfExpression.test.ts | 7 +++++ 4 files changed, 86 insertions(+), 4 deletions(-) diff --git a/packages/cdktf/lib/terraform-functions.ts b/packages/cdktf/lib/terraform-functions.ts index 2390f0e795..56c876792e 100644 --- a/packages/cdktf/lib/terraform-functions.ts +++ b/packages/cdktf/lib/terraform-functions.ts @@ -3,6 +3,7 @@ import { call } from "./tfExpression"; import { IResolvable } from "./tokens/resolvable"; import { TokenMap } from "./tokens/private/token-map"; import { TokenString } from "./tokens/private/encoding"; +import { rawString } from "."; // We use branding here to ensure we internally only handle validated values // this allows us to catch usage errors before terraform does in some cases @@ -11,6 +12,12 @@ type TFValueValidator = (value: any) => TFValue; type ExecutableTfFunction = (...args: any[]) => IResolvable; +function hasUnescapedDoubleQuotes(str: string) { + const ret = /\\([\s\S])|(")/.test(str); + console.log("hasUnescapedDoubleQuotes", str, ret); + return ret; +} + // Validators function anyValue(value: any): any { return value; @@ -22,8 +29,15 @@ function mapValue(value: any): any { function stringValue(value: any): any { if (typeof value !== "string" && !Tokenization.isResolvable(value)) { - throw new Error(`${value} is not a valid number nor a token`); + throw new Error(`'${value}' is not a valid string nor a token`); + } + + if (typeof value === "string" && hasUnescapedDoubleQuotes(value)) { + throw new Error( + `'${value}' can not be used as value directly since it has unescaped double quotes in it. To safely use the value please use Fn.rawString on your string.` + ); } + return value; } @@ -1249,4 +1263,13 @@ export class Fn { public static try(expression: any[]) { return asAny(terraformFunction("try", listOf(anyValue))(...expression)); } + + /** + * Use this function to wrap a string and escape it properly for the use in Terraform + * This is only needed in certain scenarios (e.g., if you have unescaped double quotes in the string) + * @param {String} str + */ + public static rawString(str: string): string { + return asString(rawString(str)); + } } diff --git a/packages/cdktf/lib/tfExpression.ts b/packages/cdktf/lib/tfExpression.ts index 4d1d206593..d3cbbedef6 100644 --- a/packages/cdktf/lib/tfExpression.ts +++ b/packages/cdktf/lib/tfExpression.ts @@ -33,8 +33,8 @@ class TFExpression extends Intrinsic implements IResolvable { * Escape string removes characters from the string that are not allowed in Terraform or JSON * It must only be used on non-token values */ - private escapeString(str: string) { - return str + protected escapeString(str: string) { + return str // Escape double quotes .replace(/\n/g, "\\n") // escape newlines .replace(/\${/g, "$$${"); // escape ${ to $${ } @@ -45,7 +45,9 @@ class TFExpression extends Intrinsic implements IResolvable { // String literal if (numberOfTokens === 0) { - return resolvedArg.startsWith('"') && resolvedArg.endsWith('"') + return resolvedArg !== `"` && + resolvedArg.startsWith('"') && + resolvedArg.endsWith('"') ? this.escapeString(resolvedArg) : `"${this.escapeString(resolvedArg)}"`; } @@ -77,6 +79,25 @@ class TFExpression extends Intrinsic implements IResolvable { } } +// A string that represents an input value to be escaped +class RawString extends TFExpression { + constructor(private readonly str: string) { + super(str); + } + + public resolve() { + return `"${this.escapeString(this.str).replace(/\"/g, '\\"')}"`; // eslint-disable-line no-useless-escape + } + + public toString() { + return this.str; + } +} + +export function rawString(str: string): IResolvable { + return new RawString(str); +} + class Reference extends TFExpression { constructor(private identifier: string) { super(identifier); diff --git a/packages/cdktf/test/functions.test.ts b/packages/cdktf/test/functions.test.ts index 05ed4ec7b3..f0bcca41a7 100644 --- a/packages/cdktf/test/functions.test.ts +++ b/packages/cdktf/test/functions.test.ts @@ -407,3 +407,34 @@ test("undefined and null", () => { }" `); }); + +test("throws error on unescaped double quote string inputs", () => { + expect(() => { + const app = Testing.app(); + const stack = new TerraformStack(app, "test"); + new TerraformOutput(stack, "test-output", { + value: Fn.md5(`"`), + }); + Testing.synth(stack); + }).toThrowErrorMatchingInlineSnapshot( + `"'\\"' can not be used as value directly since it has unescaped double quotes in it. To safely use the value please use Fn.rawString on your string."` + ); +}); + +test("throws no error when wrapping unescaped double quotes in Fn.rawString", () => { + const app = Testing.app(); + const stack = new TerraformStack(app, "test"); + new TerraformOutput(stack, "test-output", { + value: Fn.md5(Fn.rawString(`"`)), + }); + + expect(Testing.synth(stack)).toMatchInlineSnapshot(` + "{ + \\"output\\": { + \\"test-output\\": { + \\"value\\": \\"\${md5(\\\\\\"\\\\\\\\\\\\\\"\\\\\\")}\\" + } + } + }" + `); +}); diff --git a/packages/cdktf/test/tfExpression.test.ts b/packages/cdktf/test/tfExpression.test.ts index a62b4b19a7..161a725c26 100644 --- a/packages/cdktf/test/tfExpression.test.ts +++ b/packages/cdktf/test/tfExpression.test.ts @@ -20,6 +20,7 @@ import { call, subOperation, Expression, + rawString, } from "../lib/tfExpression"; import { resolve } from "../lib/_tokens"; const resolveExpression = (expr: Expression) => resolve(null as any, expr); @@ -163,3 +164,9 @@ test("functions don't escape terraform references that have been tokenized", () ) ).toMatchInlineSnapshot(`"\${length(docker_container.foo.bar)}"`); }); + +test("functions escape string markers", () => { + expect( + resolveExpression(call("length", [rawString(`"`)])) + ).toMatchInlineSnapshot(`"\${length(\\"\\\\\\"\\")}"`); +}); From 8811a18afa5aa7ca4228f7862e2d9aea480ccfe3 Mon Sep 17 00:00:00 2001 From: Ansgar Mertens Date: Wed, 10 Nov 2021 11:51:43 +0100 Subject: [PATCH 3/5] fix(lib): only add extra quotes if raw string is an inner expression This might feel counter-intuitive but quotes are not required for an 'outer' expression as the string will get quoted somewhere else already. Only inner expressions need those extra quotes as they would else be treated as raw expressions like e.g. `var.something`. --- packages/cdktf/lib/tfExpression.ts | 3 ++- packages/cdktf/test/functions.test.ts | 24 ++++++++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/packages/cdktf/lib/tfExpression.ts b/packages/cdktf/lib/tfExpression.ts index d3cbbedef6..9f72c0de0f 100644 --- a/packages/cdktf/lib/tfExpression.ts +++ b/packages/cdktf/lib/tfExpression.ts @@ -86,7 +86,8 @@ class RawString extends TFExpression { } public resolve() { - return `"${this.escapeString(this.str).replace(/\"/g, '\\"')}"`; // eslint-disable-line no-useless-escape + const qts = this.isInnerTerraformExpression ? `"` : ``; + return `${qts}${this.escapeString(this.str).replace(/\"/g, '\\"')}${qts}`; // eslint-disable-line no-useless-escape } public toString() { diff --git a/packages/cdktf/test/functions.test.ts b/packages/cdktf/test/functions.test.ts index f0bcca41a7..282b730e0d 100644 --- a/packages/cdktf/test/functions.test.ts +++ b/packages/cdktf/test/functions.test.ts @@ -438,3 +438,27 @@ test("throws no error when wrapping unescaped double quotes in Fn.rawString", () }" `); }); + +test("rawString escapes correctly", () => { + const app = Testing.app(); + const stack = new TerraformStack(app, "test"); + new TerraformLocal(stack, "test", { + default: "abc", + plain: Fn.rawString("abc"), + infn: Fn.base64encode(Fn.rawString("abc")), + quotes: Fn.rawString(`"`), + doublequotes: Fn.rawString(`""`), + template: Fn.rawString("${TEMPLATE}"), + }); + + const str = Testing.synth(stack); + const json = JSON.parse(str); + + const bslsh = `\\`; // a single backslash + expect(json.locals.test).toHaveProperty("default", "abc"); + expect(json.locals.test).toHaveProperty("plain", "abc"); + expect(json.locals.test).toHaveProperty("infn", '${base64encode("abc")}'); + expect(json.locals.test).toHaveProperty("quotes", `${bslsh}"`); + expect(json.locals.test).toHaveProperty("doublequotes", `${bslsh}"${bslsh}"`); + expect(json.locals.test).toHaveProperty("template", "$${TEMPLATE}"); +}); From 99a2a322f495326bea0efe66add334491b437a4b Mon Sep 17 00:00:00 2001 From: Ansgar Mertens Date: Wed, 10 Nov 2021 11:55:22 +0100 Subject: [PATCH 4/5] fix(lib): Also resolve args of arrays (this allows arrays with literal objects as inputs to functions) This has previously only been done for objects but not for arrays. --- packages/cdktf/lib/tfExpression.ts | 4 +++- packages/cdktf/test/functions.test.ts | 28 +++++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/packages/cdktf/lib/tfExpression.ts b/packages/cdktf/lib/tfExpression.ts index 9f72c0de0f..64c52ef6f8 100644 --- a/packages/cdktf/lib/tfExpression.ts +++ b/packages/cdktf/lib/tfExpression.ts @@ -17,7 +17,9 @@ class TFExpression extends Intrinsic implements IResolvable { } if (Array.isArray(resolvedArg)) { - return `[${resolvedArg.join(", ")}]`; + return `[${resolvedArg + .map((_, index) => this.resolveArg(context, arg[index])) + .join(", ")}]`; } if (typeof resolvedArg === "object") { diff --git a/packages/cdktf/test/functions.test.ts b/packages/cdktf/test/functions.test.ts index 282b730e0d..b8e8240f74 100644 --- a/packages/cdktf/test/functions.test.ts +++ b/packages/cdktf/test/functions.test.ts @@ -357,6 +357,34 @@ test("quoted primitives, unquoted functions", () => { `); }); +test("nested objects and arrays as args", () => { + const app = Testing.app(); + const stack = new TerraformStack(app, "test"); + + new TerraformOutput(stack, "test-output", { + value: Fn.jsonencode({ + Statement: [ + { + Action: "sts:AssumeRole", + Effect: "Allow", + Principal: { Service: "lambda.amazonaws.com" }, + }, + ], + Version: "2012-10-17", + }), + }); + + expect(Testing.synth(stack)).toMatchInlineSnapshot(` + "{ + \\"output\\": { + \\"test-output\\": { + \\"value\\": \\"\${jsonencode({Statement = [{Action = \\\\\\"sts:AssumeRole\\\\\\", Effect = \\\\\\"Allow\\\\\\", Principal = {Service = \\\\\\"lambda.amazonaws.com\\\\\\"}}], Version = \\\\\\"2012-10-17\\\\\\"})}\\" + } + } + }" + `); +}); + test("terraform local", () => { const app = Testing.app(); const stack = new TerraformStack(app, "test"); From e73ea829e371d05a41bfda364785d1ed39d77759 Mon Sep 17 00:00:00 2001 From: Ansgar Mertens Date: Wed, 10 Nov 2021 13:32:45 +0100 Subject: [PATCH 5/5] fix(lib): remove console.log statement --- packages/cdktf/lib/terraform-functions.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/cdktf/lib/terraform-functions.ts b/packages/cdktf/lib/terraform-functions.ts index 56c876792e..fabb46a710 100644 --- a/packages/cdktf/lib/terraform-functions.ts +++ b/packages/cdktf/lib/terraform-functions.ts @@ -13,9 +13,7 @@ type TFValueValidator = (value: any) => TFValue; type ExecutableTfFunction = (...args: any[]) => IResolvable; function hasUnescapedDoubleQuotes(str: string) { - const ret = /\\([\s\S])|(")/.test(str); - console.log("hasUnescapedDoubleQuotes", str, ret); - return ret; + return /\\([\s\S])|(")/.test(str); } // Validators