ljharb / shell-quote

MIT License
24 stars 10 forks source link

Tilde `~` sometimes not quoted #9

Open Ordoviz opened 1 year ago

Ordoviz commented 1 year ago
import {quote} from "shell-quote";
console.log(quote(['cat', '~/.bashrc']));   // tilde not quoted
console.log(quote(['cat', '~/s p a c e'])); // tilde quoted

The treatment of ~ should be consistent at the very least. I find it surprising that ~ is not quoted since all other special Bash characters are quoted.

I suggest adding ~ to the list of characters to quote in this line: https://github.com/ljharb/shell-quote/blob/da8a3abb4b2754dd9c1d3203142133e253c515a4/quote.js#L14

This quirk was showcased last week in a AmateursCTF challenge.

ljharb commented 1 year ago

~ isn't universally replaced with $HOME, i believe. Which shells besides bash support that?

Ordoviz commented 1 year ago

Which shells besides bash support that?

Zsh, Fish, dash, oilshell, nushell, PowerShell and probably more

ljharb commented 1 year ago

node, however, does not, i think, if you pass it in child_process.exec?

Ordoviz commented 1 year ago

node, however, does not, i think, if you pass it in child_process.exec?

const { exec } = require('node:child_process');
exec('echo ~', (error, stdout, stderr) => {
  console.log(`exec: ${stdout}`);
}); 

const { execFile } = require('node:child_process');
execFile('echo', ['~'], {shell: false}, (error, stdout, stderr) => {
  console.log(`execFile (shell=false): ${stdout}`);
}); 
execFile('echo', ['~'], {shell: true}, (error, stdout, stderr) => {
  console.log(`execFile (shell=true): ${stdout}`);
}); 

Unsurprisingly, the output is

execFile (shell=true): /home/me

execFile (shell=false): ~

exec: /home/me
ljharb commented 1 year ago

ahh, the shell option is the difference :-) i've never passed that, so i've always been confused that ~ doesn't expand.

sounds like this would be a useful thing to quote. If we did it automatically, in what ways could that be a breaking change? (the case above where it quotes is because of the spaces, so it's not inconsistent right now)

Ordoviz commented 1 year ago

in what ways could that be a breaking change?

Looking at the code, we see that ~ is already quoted when the input contains whitespace, a single quote, or a double quote. Otherwise we reach the return String(s).replace line, which does not quote ~.

https://github.com/ljharb/shell-quote/blob/da8a3abb4b2754dd9c1d3203142133e253c515a4/quote.js#L11-L14

Speaking of introducing breaking changes, you could consider quoting the percent sign % as well for good measure, as mentioned on Exploiting CVE-2021-42740.

ljharb commented 1 year ago

Right, but the presence of ~ isn't triggering the quoting, which remains consistent currently - otherwise, you could claim that a is quoted on line 12 but not on line 14, which is true but a useless thing to measure.

The goal is to not introduce breaking changes, since that's the most harmful thing any package can do to the ecosystem.

My question was, if we change it so that ~ (or %) alone can trigger the quoting, what would that break for a user?