Allow setting variable with a value containing strings

[longzheng]

The Node.js NODE_OPTIONS environment variable allows setting multiple options in a space-separated list, and sometimes it is necessary to set multiple options.

For example

NODE_OPTIONS='--no-experimental-fetch --trace-warnings'

However the current validation regex does not allow for spaces or quotes

$ dotenv -v NODE_ENV=production -v NODE_OPTIONS='--no-experimental-fetch --trace-warnings' next start
Unexpected argument NODE_OPTIONS=--no-experimental-fetch --trace-warnings. Expected variable in format variable=value

The cross-env package allows for this

cross-env NODE_ENV=production NODE_OPTIONS='--no-experimental-fetch --trace-warnings'

[longzheng]

I'm not sure if this modified regex is the correct solution https://regex101.com/r/O9zira/1

^\w+=[a-zA-Z0-9"=^!?%@_&\-/:;.'"\s]+$

[entropitor]

I'm a bit worried about security issues and I don't have a lot of time to investigate all these regex changes. Are you also using this on Windows? Because otherwise you can use https://github.com/entropitor/dotenv-cli/issues/56#issuecomment-939326517

[longzheng]

Are you also using this on Windows? Because otherwise you can use #56 (comment)

Unfortunately yes we need our script to be cross platform on Windows as well.

[Swivelgames]

Any traction on this so far? This is a major sticking point for us. Env vars with spaces are a must have for us. 🙁

[Swivelgames]

Switched over to @dotenvx/dotenvx for now, but my methods are not cross-platform.

NODE_ENV=production \
NODE_OPTIONS='--no-experimental-fetch --trace-warnings' \
    npx dotenvx run --env-file=.env.production -- \
    next start

Combining @dotenvx/dotenvx with cross-env might be messy, but it would get the job done, at least until this is resolved.

This is what I was attempting to use before:

dotenv -e .env.production -v NODE_ENV=production -v NODE_OPTIONS='--no-experimental-fetch --trace-warnings' next start

---

For what it's worth, I'm struggling to understand the security issue you mentioned. The syntax you referenced, afterall, is a function of the shell interpreter, and would be working as expected in any other CLI application:

node cli.js -v test="$(echo bax; echo foo=bar)" -p test
# outputs bax
node cli.js -v test="$(echo bax; echo foo=bar)" -p foo
# ouputs bar

Double quotes in BASH signify a string with interpolation enabled. It's expected that anything within "$(...)" would not be idempotent. This is actually a strong feature of bash. Preventing that may be considered an anti-pattern, especially since someone executing either of the above would likely be doing so intentionally and expecting it to work.

It might be prudent to use something like yargs, which is presently rigorously mature. Granted, it adds bulk to the package. But if security is a concern, there's no doubting the maturity of the project. And I would definitely see that as a beneficial trade-off, and might ease the pressure of vetting changes like these over security concerns! 🙂

[entropitor]

Preventing that may be considered an anti-pattern, especially since someone executing either of the above would likely be doing so intentionally and expecting it to work.

You would expect that if you set test with -v that it can also set other variables like foo? Which is what is happening above. Especially with newlines, it could be problematic if only part of the value is in the env variable (e.g. with some encryption keys, it's often multiple lines). That's not a security issue but still unexpected results. I'm happy to merge a PR but I think this is getting more complicated than a simple CLI tool and it warrants some tests.

I'm not inclined to merge anything which will increase the maintenance burden of this tool for me.

[Swivelgames]

@entropitor Hahah, Oops! It was a late Friday, and I misunderstood what was being demonstrated. I apologize.

After looking things over, and especially given the fact that the goal is to avoid over-complicating, I think the right direction would actually be to simplify, rather than over-complicate.

I went ahead and implemented the changes I recommended in this PR: https://github.com/entropitor/dotenv-cli/pull/105

tl;dr:

• We can actually simplify things a lot, instead of increasing the complexity of the regex
• We should just treat everything after the first = as the value
• This is compliant with the way that dotenv expects, including the ability to have newline characters in values
• dotenv.parse() expects the body of a .env file, which is why using it to parse the CLI args creates the security hole.

EDIT: Original Comment

Here's a simple proof-of-concept: ```diff function validateCmdVariable (param) { - if (!param.match(/^\w+=[a-zA-Z0-9"=^!?%@_&\-/:;.]+$/)) { + if (!param.match(/^\w+=.+$/)) { console.error('Unexpected argument ' + param + '. Expected variable in format variable=value') process.exit(1) } - return param + // Remember, just a POC + const [key, ...val] = param.split(/=/) + return [key, val.join('=')] } const variables = [] if (argv.v) { @@ -71,8 +72,10 @@ if (argv.v) { variables.push(...argv.v.map(validateCmdVariable)) } } -// This expects a Buffer, because we should be passing the contents of a file here. -const parsedVariables = dotenv.parse(Buffer.from(variables.join('\n'))) + +// variables is now in the form of [ [key, val], [key, val] ], +// so we can plug it straight into `Object.fromEntries()` +const parsedVariables = Object.fromEntries(variables) if (argv.debug) { console.log(paths) ``` 1. We only care about the first equal sign. Any others are part of the value. 2. We don't need `dotenv.parse`. By splitting on the first equal sign, and preserving the others, we're already creating a Key-Value pairs. - Our security concern was coming from passing overloaded variables to `dotenv.parse()` to begin with. - `dotenv.parse()` expects to be given the body of a `.env` file, so it doesn't exactly fit our use-case for using it to parse parameter values either. - We only really need `dotenv` for parsing the `.env` files now. This results in: ``` node cli.js -v foo=bar=baz -v "pew=pew" -v NODE_OPTIONS='--no-experimental-fetch --trace-warnings --loader=ts-node/tsm --loader=esmock' --debug [ '.env' ] { foo: 'bar=baz', pew: 'pew', NODE_OPTIONS: '--no-experimental-fetch --trace-warnings --loader=ts-node/tsm --loader=esmock' } ``` And still successfully errors out for the security issue scenario: ``` node cli.js -v test="$(echo bax; echo foo=bar)" -p test # Outputs Unexpected argument error node cli.js -v test="$(echo bax; echo foo=bar)" -p foo # Outputs Unexpected argument error ``` With some super simple changes to the pseudo-code above, we could also preserving newline characters, which `dotenv` expects support for: Newline characters will be preserved in values ```diff - if (!param.match(/^\w+=.+$/)) { + if (!param.match(/^\w+=.+$/m)) { ``` Outputs: ``` node cli.js -v test="$(echo bax; echo foo=bar)" -p test # outputs `bax\nfoo=bar` node cli.js -v test="$(echo bax; echo foo=bar)" -p foo # ouputs '' ```