Cloud Functions: Fix env var escaping (#4271)

Fixes #4270 and a little bit more.

The bug results from the use of `String.prototype.replace` with a first (search) argument of type `string`. This is an unintuitive corner of JavaScript that only replaces the *first* occurrence of the search string. [See MDN](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/replace). To replace all occurrences, either the search argument must be expressed as an equivalent regex with the `/g` flag, or [.replaceAll](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/replaceAll) must be used. .replaceAll isn't supported below Node 15, and given this package's dependency on Node >= 12, the regex approach has been used here.

Quick demonstration:
```
Welcome to Node.js v16.13.2.
Type ".help" for more information.
> const aaaa = "aaaa";
undefined
> aaaa.replace("a", "b") // string argument; replaces only the first occurrence
'baaa'
> aaaa.replace(/a/, "b") // regex argument *without* /g flag; replaces only the first occurrence
'baaa'
> aaaa.replace(/a/g, "b") // regex argument *with* /g flag; replaces all occurrences
'bbbb'
> aaaa.replaceAll("a", "b") // replaceAll method in Node >= 15; replaces all occurrences
'bbbb'
```

There's another reason to use a regex approach - running the string through repeated invocations of .replace(All) processes the escapes in an implementation-defined order, whereas the string really should be scanned from start to end in order to correctly convert (for example) `\\n` to `\` `n`. The popular npm package [dotenv](https://www.npmjs.com/package/dotenv) gets tripped up by this as well.

The changes outside `.env` files result from manually searching the repo for uses of `String.prototype.replace` having a non-regex first argument. There are more occurrences than the ones fixed here that probably should be audited, but 1. Not all instances are immediately obvious to me whether they intend to replace *all* occurrences of the search argument 2. I wanted to keep this PR focused.

CHANGELOG.md

@@ -1,2 +1,3 @@
 - Fixes bug where resumable uploads were not setting custom metadata on upload (#3398).
 - Fixes bug where GCS metadataUpdate cloud functions were triggered in incorrect situations (#3398).
+- Fixes bug where quoted escape sequences in .env files were incompletely unescaped. (#4270)

src/functions/env.ts

@@ -58,6 +58,16 @@ const LINE_RE = new RegExp(
   "gms"                      // flags: global, multiline, dotall
 );
 
+const ESCAPE_SEQUENCES_TO_CHARACTERS: Record<string, string> = {
+  "\\n": "\n",
+  "\\r": "\r",
+  "\\t": "\t",
+  "\\v": "\v",
+  "\\\\": "\\",
+  "\\'": "'",
+  '\\"': '"',
+};
+
 interface ParseResult {
   envs: Record<string, string>;
   errors: string[];
@@ -104,10 +114,9 @@ export function parse(data: string): ParseResult {
       // Remove surrounding single/double quotes.
       v = quotesMatch[2];
       if (quotesMatch[1] === '"') {
-        // Unescape newlines and tabs.
-        v = v.replace("\\n", "\n").replace("\\r", "\r").replace("\\t", "\t").replace("\\v", "\v");
-        // Unescape other escapable characters.
-        v = v.replace(/\\([\\'"])/g, "$1");
+        // Substitute escape sequences. The regex passed to replace() must
+        // match every key in ESCAPE_SEQUENCES_TO_CHARACTERS.
+        v = v.replace(/\\[nrtv\\'"]/g, (match) => ESCAPE_SEQUENCES_TO_CHARACTERS[match]);
       }
     }
 

src/functions/runtimeConfigExport.ts

@@ -169,14 +169,19 @@ export function hydrateEnvs(pInfos: ProjectConfigInfo[], prefix: string): string
   return errMsg;
 }
 
+const CHARACTERS_TO_ESCAPE_SEQUENCES: Record<string, string> = {
+  "\n": "\\n",
+  "\r": "\\r",
+  "\t": "\\t",
+  "\v": "\\v",
+  "\\": "\\\\",
+  '"': '\\"',
+  "'": "\\'",
+};
+
 function escape(s: string): string {
-  // Escape newlines and tabs
-  const result = s
-    .replace("\n", "\\n")
-    .replace("\r", "\\r")
-    .replace("\t", "\\t")
-    .replace("\v", "\\v");
-  return result.replace(/(['"])/g, "\\$1");
+  // Escape newlines, tabs, backslashes and quotes
+  return s.replace(/[\n\r\t\v\\"']/g, (ch) => CHARACTERS_TO_ESCAPE_SEQUENCES[ch]);
 }
 
 /**

src/functions/secrets.ts

@@ -34,8 +34,7 @@ export function labels(): Record<string, string> {
 
 function toUpperSnakeCase(key: string): string {
   return key
-    .replace("-", "_")
-    .replace(".", "_")
+    .replace(/[.-]/g, "_")
     .replace(/([a-z])([A-Z])/g, "$1_$2")
     .toUpperCase();
 }

src/test/functions/env.spec.ts

@@ -53,6 +53,43 @@ BAR=bar
         input: 'FOO="foo1\\nfoo2"\nBAR=bar',
         want: { FOO: "foo1\nfoo2", BAR: "bar" },
       },
+      {
+        description: "should parse escape sequences in order, from start to end",
+        input: `BAZ=baz
+ONE_NEWLINE="foo1\\nfoo2"
+ONE_BSLASH_AND_N="foo3\\\\nfoo4"
+ONE_BSLASH_AND_NEWLINE="foo5\\\\\\nfoo6"
+TWO_BSLASHES_AND_N="foo7\\\\\\\\nfoo8"
+BAR=bar`,
+        want: {
+          BAZ: "baz",
+          ONE_NEWLINE: "foo1\nfoo2",
+          ONE_BSLASH_AND_N: "foo3\\nfoo4",
+          ONE_BSLASH_AND_NEWLINE: "foo5\\\nfoo6",
+          TWO_BSLASHES_AND_N: "foo7\\\\nfoo8",
+          BAR: "bar",
+        },
+      },
+      {
+        description: "should parse double quoted with multiple escaped newlines",
+        input: 'FOO="foo1\\nfoo2\\nfoo3"\nBAR=bar',
+        want: { FOO: "foo1\nfoo2\nfoo3", BAR: "bar" },
+      },
+      {
+        description: "should parse double quoted with multiple escaped horizontal tabs",
+        input: 'FOO="foo1\\tfoo2\\tfoo3"\nBAR=bar',
+        want: { FOO: "foo1\tfoo2\tfoo3", BAR: "bar" },
+      },
+      {
+        description: "should parse double quoted with multiple escaped vertical tabs",
+        input: 'FOO="foo1\\vfoo2\\vfoo3"\nBAR=bar',
+        want: { FOO: "foo1\vfoo2\vfoo3", BAR: "bar" },
+      },
+      {
+        description: "should parse double quoted with multiple escaped carriage returns",
+        input: 'FOO="foo1\\rfoo2\\rfoo3"\nBAR=bar',
+        want: { FOO: "foo1\rfoo2\rfoo3", BAR: "bar" },
+      },
       {
         description: "should leave single quotes when double quoted",
         input: `FOO="'foo'"`,

src/test/functions/runtimeConfigExport.spec.ts

@@ -133,11 +133,11 @@ describe("functions-config-export", () => {
 
     it("should preserve newline characters", () => {
       const dotenv = configExport.toDotenvFormat([
-        { origKey: "service.api.url", newKey: "SERVICE_API_URL", value: "hello\nworld" },
+        { origKey: "service.api.url", newKey: "SERVICE_API_URL", value: "hello\nthere\nworld" },
       ]);
       const { envs, errors } = env.parse(dotenv);
       expect(envs).to.be.deep.equal({
-        SERVICE_API_URL: "hello\nworld",
+        SERVICE_API_URL: "hello\nthere\nworld",
       });
       expect(errors).to.be.empty;
     });

src/test/functions/secrets.spec.ts

@@ -27,22 +27,27 @@ describe("functions/secret", () => {
     });
 
     it("returns the original key if it follows convention", async () => {
-      expect(await secrets.ensureValidKey("MY_KEY", options)).to.equal("MY_KEY");
+      expect(await secrets.ensureValidKey("MY_SECRET_KEY", options)).to.equal("MY_SECRET_KEY");
       expect(warnStub).to.not.have.been.called;
     });
 
-    it("returns the transformed key (with warning) if with dashses", async () => {
-      expect(await secrets.ensureValidKey("MY-KEY", options)).to.equal("MY_KEY");
+    it("returns the transformed key (with warning) if with dashes", async () => {
+      expect(await secrets.ensureValidKey("MY-SECRET-KEY", options)).to.equal("MY_SECRET_KEY");
+      expect(warnStub).to.have.been.calledOnce;
+    });
+
+    it("returns the transformed key (with warning) if with periods", async () => {
+      expect(await secrets.ensureValidKey("MY.SECRET.KEY", options)).to.equal("MY_SECRET_KEY");
       expect(warnStub).to.have.been.calledOnce;
     });
 
     it("returns the transformed key (with warning) if with lower cases", async () => {
-      expect(await secrets.ensureValidKey("my_key", options)).to.equal("MY_KEY");
+      expect(await secrets.ensureValidKey("my_secret_key", options)).to.equal("MY_SECRET_KEY");
       expect(warnStub).to.have.been.calledOnce;
     });
 
     it("returns the transformed key (with warning) if camelCased", async () => {
-      expect(await secrets.ensureValidKey("myKey", options)).to.equal("MY_KEY");
+      expect(await secrets.ensureValidKey("mySecretKey", options)).to.equal("MY_SECRET_KEY");
       expect(warnStub).to.have.been.calledOnce;
     });