Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cloud Functions: Incorrect/incomplete transformation of escape sequences in .env files #4270

Closed
yellcorp opened this issue Mar 9, 2022 · 1 comment · Fixed by #4271
Closed
Assignees

Comments

@yellcorp
Copy link
Contributor

yellcorp commented Mar 9, 2022

[REQUIRED] Environment info

firebase-tools: 10.2.2

Platform: macOS (11.6.2 Big Sur)

[REQUIRED] Test case

  1. Create a new Node project

    mkdir firebasetoolstest
    cd firebasetoolstest
    npm init # (accept default answers)
  2. Add this package as a devDependency

    npm i -DE firebase-tools@10.2.2
  3. Init a new Firebase project

    npx firebase init
    # Which Firebase features: enable 'Functions'
    # Project setup: Create a new project
    # Please specify a unique project id: (your choice)
    # What would you like to call your project: (your choice)
    # What language would you like to use to write Cloud Functions?: JavaScript
    # Do you want to use ESLint to catch probable bugs and enforce style? N
    # Do you want to install dependencies with npm now? Y
  4. Edit functions/index.js to have the following content:

    const functions = require("firebase-functions");
    
    exports.envParseTest = functions.https.onRequest((request, response) => {
      const text = process.env.ENVTEST_SAMPLE;
      const lines = text.split("\n");
      const lineCount = lines.length;
      const bytes = Array.from(Buffer.from(text));
      response.json({ text, lines, lineCount, bytes });
    });
  5. Create functions/.env with the following content:

    ENVTEST_SAMPLE="foo\nbar\nbaz"
  6. At this point it's sufficient to run the Cloud Functions emulator to reproduce the issue.

    npx firebase emulators:start --only functions

[REQUIRED] Steps to reproduce

Open the indicated URL for the envParseTest function in a browser, or some terminal-based HTTP client.

curl 'http://localhost:5001/$GCLOUD_PROJECT/us-central1/envParseTest'

[REQUIRED] Expected behavior

A response with the content

{"text":"foo\nbar\nbaz","lines":["foo","bar","baz"],"lineCount":3,"bytes":[102,111,111,10,98,97,114,10,98,97,122]}

[REQUIRED] Actual behavior

A response with the content

{"text":"foo\nbar\\nbaz","lines":["foo","bar\\nbaz"],"lineCount":2,"bytes":[102,111,111,10,98,97,114,92,110,98,97,122]}

Note in the "text" property - the first \n from the .env has been correctly interpreted as a newline; the second remains as a literal \ followed by a literal n. This test also outputs a bytes property to show the parse result unambiguously. A newline appears in the array as decimal 10, a literal \ followed by literal n appears as decimal 92,110.

Debug log from firebase emulators:start is attached.
firebase-debug.log

@yellcorp yellcorp added the bug label Mar 9, 2022
@yellcorp yellcorp changed the title Incorrect/incomplete transformation of escape sequences in .env files Cloud Functions: Incorrect/incomplete transformation of escape sequences in .env files Mar 9, 2022
@taeold taeold self-assigned this Mar 9, 2022
taeold pushed a commit that referenced this issue Mar 25, 2022
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.
@taeold
Copy link
Contributor

taeold commented Mar 25, 2022

@yellcorp Thank you again for your wonderful contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants