src: add support for escaping quotes with escape slash in `--env-file` by pluris · Pull Request #50814 · nodejs/node

pluris · 2023-11-19T13:39:12Z

tniessen · 2023-11-19T14:10:42Z

Is this compatible with dotenv? We should not invent a new format. cc @anonrig

anonrig · 2023-11-19T14:46:23Z

test/fixtures/dotenv/valid.env

 RETAIN_INNER_QUOTES={"foo": "bar"}
 RETAIN_INNER_QUOTES_AS_STRING='{"foo": "bar"}'
 RETAIN_INNER_QUOTES_AS_BACKTICKS=`{"foo": "bar's"}`
+RETAIN_INNER_QUOTES_AS_ESCAPE_SLASH=`{\"foo\": \"bar\"}`


Suggested change

RETAIN_INNER_QUOTES_AS_ESCAPE_SLASH=`{\"foo\": \"bar\"}`

RETAIN_INNER_QUOTES_AS_ESCAPE_SLASH="{\"foo\": \"bar\"}"

meyfa · 2023-11-19T14:20:27Z

src/node_dotenv.cc


    if (existing.IsNothing()) {
+      // Remove all '\' characters from value
+      value.erase(std::remove(value.begin(), value.end(), '\\'), value.end());


What about escaping backslashes?

I received comments and changed the implementation to apply only to \".

meyfa · 2023-11-19T14:21:44Z

test/fixtures/dotenv/valid.env

 RETAIN_INNER_QUOTES={"foo": "bar"}
 RETAIN_INNER_QUOTES_AS_STRING='{"foo": "bar"}'
 RETAIN_INNER_QUOTES_AS_BACKTICKS=`{"foo": "bar's"}`
+RETAIN_INNER_QUOTES_AS_ESCAPE_SLASH=`{\"foo\": \"bar\"}`


Maybe add e.g. C:\\Windows\\system32 as a test case?

What's the default behavior of dotenv?

There was a problem with the current implementation, so I modified it and added the test you commented on.

@pluris Thanks for the quick fix! I think there is still an issue. I would expect the test to read:

assert.strictEqual(process.env.PATH_WINDOWS, 'C:\\Windows\\system32');

instead of the current

assert.strictEqual(process.env.PATH_WINDOWS, 'C:\\\\Windows\\\\system32');

That is, escaping a backslash should result in just one backslash... This is also what dotenv does, @anonrig.

bricss · 2023-11-19T15:45:58Z

That thing looks like a foot gun 🔫 to me 🫣

shubham9411 · 2023-11-19T16:22:46Z

src/node_dotenv.cc


    if (existing.IsNothing()) {
+      // Remove all '\' characters from value
+      value.erase(std::remove(value.begin(), value.end(), '\\'), value.end());


Should this be moved to Dotenv::ParseLine ? 🤔

Thank you for your comment.
IMO, Dotenv::ParseLine() is used in Dotenv::ParsePath(), and the current issue does not seem to be about Path.

Sorry, I checked further and found what you said was correct. I applied it.

pluris · 2023-11-19T16:29:10Z

Thank you for the comments.
It seems that the current modification may cause another error.
Hmm... to fix this issue, how about applying it only to \" instead of all backslashes?
I've updated the PR. Please take a look again.

…nv-file`

anonrig · 2023-11-19T18:34:20Z

What happens if you have a value of '\'hello'?

meyfa · 2023-11-19T19:13:36Z

Actually, I think my previous assessment ¹ was mistaken as I used require('dotenv').parse from within JavaScript, and got confused by the backslash count within the strings. Running it again with files on disk, it seems that there is no escaping going on whatsoever. This is also backed up by the dotenv documentation ².

Contents of .env:

INNER_QUOTES=foo'bar"baz`qux

DUPLICATE_INNER_QUOTES1='foo''bar'
DUPLICATE_INNER_QUOTES2="foo""bar"

SINGLE_QUOTED_ESCAPE1='\'foo\'bar\''
SINGLE_QUOTED_ESCAPE2='\"foo\"bar\"'

DOUBLE_QUOTED_ESCAPE1="\'foo\'bar\'"
DOUBLE_QUOTED_ESCAPE2="\"foo\"bar\""

ESCAPED_BACKSLASH=\\foo\\bar\\
SINGLE_ESCAPED_BACKSLASH='\\foo\\bar\\'
DOUBLE_ESCAPED_BACKSLASH="\\foo\\bar\\"

SINGLE_QUOTED_SINGLE_QUOTE='foo'bar'
DOUBLE_QUOTED_DOUBLE_QUOTE="foo"bar"
SINGLE_QUOTED_SINGLE_QUOTE_WITH_BACKSLASH='foo\\'bar'
DOUBLE_QUOTED_DOUBLE_QUOTE_WITH_BACKSLASH="foo\\"bar"

Test:

$ node -p "require('dotenv').config().parsed"
{
  INNER_QUOTES: 'foo\'bar"baz`qux',
  DUPLICATE_INNER_QUOTES1: "foo''bar",
  DUPLICATE_INNER_QUOTES2: 'foo""bar',
  SINGLE_QUOTED_ESCAPE1: "\\'foo\\'bar\\'",
  SINGLE_QUOTED_ESCAPE2: '\\"foo\\"bar\\"',
  DOUBLE_QUOTED_ESCAPE1: "\\'foo\\'bar\\'",
  DOUBLE_QUOTED_ESCAPE2: '\\"foo\\"bar\\"',
  ESCAPED_BACKSLASH: '\\\\foo\\\\bar\\\\',
  SINGLE_ESCAPED_BACKSLASH: '\\\\foo\\\\bar\\\\',
  DOUBLE_ESCAPED_BACKSLASH: '\\\\foo\\\\bar\\\\',
  SINGLE_QUOTED_SINGLE_QUOTE: "foo'bar",
  DOUBLE_QUOTED_DOUBLE_QUOTE: 'foo"bar',
  SINGLE_QUOTED_SINGLE_QUOTE_WITH_BACKSLASH: "foo\\\\'bar",
  DOUBLE_QUOTED_DOUBLE_QUOTE_WITH_BACKSLASH: 'foo\\\\"bar'
}

anonrig · 2023-11-19T19:14:04Z

src/node_dotenv.cc

    auto existing = env->env_vars()->Get(key.data());

    if (existing.IsNothing()) {
+      size_t found = value.find("\\\"");


This needs to be done inside the ParseLine function. Depending on the start character of the value, we need to de-escape it. For ' it is ', or for " it is "

I checked again and you're right.
I applied it as you said. Please review again.

…nv-file`

tniessen

I am not familiar with this feature but my understanding of @meyfa's #50814 (comment) is that this is incompatible with dotenv, which is the only reference for the file format as far as I know (other than some loose relation to the ancient INI file format). If this feature begins to deviate from dotenv (in a potentially breaking manner), then the file format needs to be defined precisely and in some user-facing documentation.

pluris · 2023-11-20T14:05:19Z

@tniessen
I agree in that it has different behavior than dotenv.
I'm having a bit of trouble with that part too. I will close the PR.
Thank you for your comment.

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Nov 19, 2023

pluris force-pushed the fix/dot-env-escape-slash branch 2 times, most recently from c792097 to 3b215fe Compare November 19, 2023 13:40

src: add support for escaping quotes with escape slash in --env-file

09f8165

pluris force-pushed the fix/dot-env-escape-slash branch from 3b215fe to 09f8165 Compare November 19, 2023 13:41

anonrig reviewed Nov 19, 2023

View reviewed changes

meyfa reviewed Nov 19, 2023

View reviewed changes

shubham9411 reviewed Nov 19, 2023

View reviewed changes

fixup! src: add support for escaping quotes with escape slash in `--e…

690fa43

…nv-file`

pluris force-pushed the fix/dot-env-escape-slash branch from a4c9544 to 690fa43 Compare November 19, 2023 16:39

anonrig requested changes Nov 19, 2023

View reviewed changes

fixup! src: add support for escaping quotes with escape slash in `--e…

94ada51

…nv-file`

tniessen requested changes Nov 20, 2023

View reviewed changes

pluris closed this Nov 20, 2023

pluris mentioned this pull request Nov 20, 2023

--env-file does not support escaping quotes #50801

Closed

macrozone mentioned this pull request Jul 31, 2024

--env-file does not support inner quotes (does not behave like dotenv) #54134

Open

	RETAIN_INNER_QUOTES_AS_ESCAPE_SLASH=`{\"foo\": \"bar\"}`
	RETAIN_INNER_QUOTES_AS_ESCAPE_SLASH="{\"foo\": \"bar\"}"

Uh oh!

Conversation

pluris commented Nov 19, 2023

Uh oh!

tniessen commented Nov 19, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bricss commented Nov 19, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pluris commented Nov 19, 2023

Uh oh!

anonrig commented Nov 19, 2023

Uh oh!

meyfa commented Nov 19, 2023

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pluris Nov 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tniessen left a comment

Choose a reason for hiding this comment

Uh oh!

pluris commented Nov 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

pluris Nov 20, 2023 •

edited

Loading