-
Notifications
You must be signed in to change notification settings - Fork 48
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
Allow setting variable with a value containing strings #78
Comments
I'm not sure if this modified regex is the correct solution https://regex101.com/r/O9zira/1
|
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 #56 (comment) |
Unfortunately yes we need our script to be cross platform on Windows as well. |
Any traction on this so far? This is a major sticking point for us. Env vars with spaces are a must have for us. 🙁 |
Switched over to
Combining This is what I was attempting to use before:
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:
Double quotes in BASH signify a string with interpolation enabled. It's expected that anything within It might be prudent to use something like |
You would expect that if you set I'm not inclined to merge anything which will increase the maintenance burden of this tool for me. |
@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: #105 tl;dr:
EDIT: Original CommentHere's a simple proof-of-concept: 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)
This results in:
And still successfully errors out for the security issue scenario:
With some super simple changes to the pseudo-code above, we could also preserving newline characters, which Newline characters will be preserved in values - if (!param.match(/^\w+=.+$/)) {
+ if (!param.match(/^\w+=.+$/m)) { Outputs:
|
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
However the current validation regex does not allow for spaces or quotes
The
cross-env
package allows for thisThe text was updated successfully, but these errors were encountered: