Skip to content
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

Tilde ~ sometimes not quoted #9

Open
Ordoviz opened this issue Jul 24, 2023 · 7 comments
Open

Tilde ~ sometimes not quoted #9

Ordoviz opened this issue Jul 24, 2023 · 7 comments

Comments

@Ordoviz
Copy link

Ordoviz commented Jul 24, 2023

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:

return String(s).replace(/([A-Za-z]:)?([#!"$&'()*,:;<=>?@[\\\]^`{|}])/g, '$1\\$2');

This quirk was showcased last week in a AmateursCTF challenge.

@ljharb
Copy link
Owner

ljharb commented Jul 24, 2023

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

@Ordoviz
Copy link
Author

Ordoviz commented Jul 25, 2023

Which shells besides bash support that?

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

@ljharb
Copy link
Owner

ljharb commented Jul 25, 2023

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

@Ordoviz
Copy link
Author

Ordoviz commented Jul 26, 2023

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
Copy link
Owner

ljharb commented Jul 26, 2023

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
Copy link
Author

Ordoviz commented Jul 27, 2023

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 ~.

shell-quote/quote.js

Lines 11 to 14 in da8a3ab

if ((/["'\s]/).test(s)) {
return '"' + s.replace(/(["\\$`!])/g, '\\$1') + '"';
}
return String(s).replace(/([A-Za-z]:)?([#!"$&'()*,:;<=>?@[\\\]^`{|}])/g, '$1\\$2');

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
Copy link
Owner

ljharb commented Jul 27, 2023

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?

drok added a commit to Mergesium/node-shell-quote that referenced this issue Nov 5, 2023
In many shells, ~ is a shorthand for ${HOME}, and there are other
expansion as well ~+N and ~-N expand to directories from the history.

The ~ is always quoted at the beginning of a word, or following `=` or
`:`

Fixes #2 and ljharb#9

Author-Rebase-Consent: https://No-rebase.github.io
drok added a commit to Mergesium/node-shell-quote that referenced this issue Nov 5, 2023
In many shells, ~ is a shorthand for ${HOME}, and there are other
expansion as well ~+N and ~-N expand to directories from the history.

The ~ is always quoted at the beginning of a word, or following `=` or
`:`

Fixes #4 and ljharb#9

Author-Rebase-Consent: https://No-rebase.github.io
drok added a commit to Mergesium/node-shell-quote that referenced this issue Nov 5, 2023
In many shells, ~ is a shorthand for ${HOME}, and there are other
expansion as well ~+N and ~-N expand to directories from the history.

The ~ is always quoted at the beginning of a word, or following `=` or
`:`

Fixes #4 and ljharb#9

Author-Rebase-Consent: https://No-rebase.github.io
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants