Not expanding unquoted variables correctly

[pdpwebsecurify]

Consider the following example:

$ export T="c d"
$ node -e 'console.log(JSON.stringify(process.env.T), process.argv)' a b $T

You get the following result:

"c d" [
  '/home/ec2-user/.nvm/versions/node/v17.0.1/bin/node',
  'a',
  'b',
  'c',
  'd'
]

The variable T is expanded in possitional args c and d. If we quote the variable then we get a different result:

$ export T="c d"
$ node -e 'console.log(JSON.stringify(process.env.T), process.argv)' a b "$T"
"c d" [
  '/home/ec2-user/.nvm/versions/node/v17.0.1/bin/node',
  'a',
  'b',
  'c d'
]

However, in the shell-quote library the situation is different. Consider the following example:

const shellQuote = require('shell-quote')

console.log(shellQuote.parse('test a b $T', { T: 'c d' }))

We get the following result:

[ 'test', 'a', 'b', 'c d' ]

Notice that the behaviour is different as in the variable T does not get expanded. In other words, the behaviour is similar to variable T beign quoted, i.e:

const shellQuote = require('shell-quote')

console.log(shellQuote.parse('test a b "$T"', { T: 'c d' })) // [ 'test', 'a', 'b', 'c d' ]

If shell-quote is used to parse string which are used to spown a process with environment variable that are not quoted, it will result in a completely different behaviour than the one expected from the standard shell.

[ljharb]

interesting - this makes sense, that the presence of quotes should matter.

Are there existing tests that would break with this change?

[pdpwebsecurify]

I have no idea. I was planning to use this library but it seems that at the moment it will not be a good fit due to this particular bug.

[ljharb]

I'd love to review a PR that fixes it and adds tests :-)

[ljharb]

I've pushed up a branch with failing tests, and found the place where the unquoted env var is retrieved.

The difficulty is essentially that the current approach inlines env vars amidst parsing - however, an unquoted env var needs to be treated as part of the parse, and at this point in the implementation, we've already tokenized the input - and it's tricky to insert more tokens into the list.

[drok]

Isn't this a matter of porting an existing shell's grammar parser to js?

Here are the interesting bits from bash:

https://git.savannah.gnu.org/cgit/bash.git/tree/parse.y (Yacc grammar for bash)
https://git.savannah.gnu.org/cgit/bash.git/tree/subst.c (The part of the shell that does parameter, command, arithmetic, and globbing substitutions)

These two files account for probably 60% of what bash is.

Supporting ${T} is not that far off from $T, and you'd have to support brace expansion for cases like abc${T}def. Then, what about ${T[1]} or $1 or $PID or $((T*2)) or $(echo $T | sed s/-/_/g) ?

Are there practical applications to handling shell variable expansion in a javascript program? Can we really take shell variables out of the shell?

[ljharb]

There are many more shells than bash, and porting a parser would be extremely brittle since that can change drastically across even a single shell's major versions.

[drok]

There are many more shells than bash, and porting a parser would be extremely brittle since that can change drastically across even a single shell's major versions.

I whole heartedly agree, and I think it's more sensible to remove variable expansion from the library, rather than attempt to fix it. I'm leaning towards removing this feature from my fork.

[ljharb]

We won’t ever be removing any features; breaking changes are toxic to the ecosystem.