Skip to content

Commit 47473c0

Browse files
isaacsisaacs
committed
bin: Do not expose filenames to shell expansion
This resolves GHSA-5j98-mcp5-4vw2, with a minimum of breaking changes for as many users as possible. First, 'shell: true' is only used on the subprocess if set explicitly by the user in the command line, and only if it is not a shell where this can be avoided safely without any reduction in functionality. In this case, a deprecation warning is printed, telling them that it's unsafe, and that it will be removed in a future version. Second, as the only reason for such behavior was to be able to have commands that include positional arguments in the --cmd/-c value, a new option --cmd-arg/-g is added, so that users can pass positional arguments ahead of the file matches, in a way that does not rely on shell expansion. Lastly, as a general quality of life improvement which should keep this entire issue from even mildly inconveniencing most users, when the command contains space or quote characters (and thus, is likely to contain positional arguments), AND the `SHELL` environment variable refers to a shell program with a known way to pass positional arguments to the child process, then we use that technique, again avoiding shell expansion of the resolved file paths (or the user command itself). This applies to sh, ksh, bash, zsh, and fish. This potentially WILL break workflows, and require updating, if they are relying on the automatic shell expansion, in systems other than the known posix shells referenced above. The only likely case that anyone will thus encounter, is running commands on Windows. While there DOES appear to be a way to use a similar trick on Windows, but there are so many more edge cases, I'm not confident I can do so without introducing more bugs (and potentially more security issues). If users find that this breakage is too severe, the fix will be to roll out a subsequent release that turns `--shell` on by default on Windows, if the command contains space or quote characters. Nevertheless, v12 of this library will *not* contain a `--shell` option, and will not run child processes in `shell:true` mode under any circumstances. Note: this was simultaneously and independently reported by two researchers. My sincerest thanks for their time and attention. Reported-by: @Gyde04 "Babajide Emmanuel Fakile" Reported-by: @aisle-research "Pavel Kohout (Aisle Research)" Fix: GHSA-5j98-mcp5-4vw2 Fix: CVE-2025-64756
1 parent bc33fe1 commit 47473c0

File tree

4 files changed

+345
-35
lines changed

4 files changed

+345
-35
lines changed

‎changelog.md‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,17 @@
11
# changeglob
22

3+
## 11.1
4+
5+
[GHSA-5j98-mcp5-4vw2](https://github.com/isaacs/node-glob/security/advisories/GHSA-5j98-mcp5-4vw2)
6+
7+
- Add the `--shell` option for the command line, with a warning
8+
that this is unsafe. (It will be removed in v12.)
9+
- Add the `--cmd-arg`/`-g` as a way to *safely* add positional
10+
arguments to the command provided to the CLI tool.
11+
- Detect commands with space or quote characters on known shells,
12+
and pass positional arguments to them safely, avoiding
13+
`shell:true` execution.
14+
315
## 11.0
416

517
- Drop support for node before v20

‎src/bin.mts‎

Lines changed: 137 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import { foregroundChild } from 'foreground-child'
33
import { existsSync } from 'fs'
44
import { jack } from 'jackspeak'
55
import { loadPackageJson } from 'package-json-from-dist'
6-
import { join } from 'path'
6+
import { basename, join } from 'path'
77
import { globStream } from './index.js'
88

99
const { version } = loadPackageJson(import.meta.url, '../package.json')
@@ -35,6 +35,50 @@ const j = jack({
3535
this pattern`,
3636
},
3737
})
38+
.flag({
39+
shell: {
40+
default: false,
41+
description: `Interpret the command as a shell command by passing it
42+
to the shell, with all matched filesystem paths appended,
43+
**even if this cannot be done safely**.
44+
45+
This is **not** unsafe (and usually unnecessary) when using
46+
the known Unix shells sh, bash, zsh, and fish, as these can
47+
all be executed in such a way as to pass positional
48+
arguments safely.
49+
50+
**Note**: THIS IS UNSAFE IF THE FILE PATHS ARE UNTRUSTED,
51+
because a path like \`'some/path/\\$\\(cmd)'\` will be
52+
executed by the shell.
53+
54+
If you do have positional arguments that you wish to pass to
55+
the command ahead of the glob pattern matches, use the
56+
\`--cmd-arg\`/\`-g\` option instead.
57+
58+
The next major release of glob will fully remove the ability
59+
to use this option unsafely.`,
60+
},
61+
})
62+
.optList({
63+
'cmd-arg': {
64+
short: 'g',
65+
hint: 'arg',
66+
default: [],
67+
description: `Pass the provided values to the supplied command, ahead of
68+
the glob matches.
69+
70+
For example, the command:
71+
72+
glob -c echo -g"hello" -g"world" *.txt
73+
74+
might output:
75+
76+
hello world a.txt b.txt
77+
78+
This is a safer (and future-proof) alternative than putting
79+
positional arguments in the \`-c\`/\`--cmd\` option.`,
80+
},
81+
})
3882
.flag({
3983
all: {
4084
short: 'A',
@@ -227,54 +271,113 @@ const j = jack({
227271

228272
try {
229273
const { positionals, values } = j.parse()
230-
if (values.version) {
274+
const {
275+
cmd,
276+
shell,
277+
all,
278+
default: def,
279+
version: showVersion,
280+
help,
281+
absolute,
282+
cwd,
283+
dot,
284+
285+
'dot-relative': dotRelative,
286+
follow,
287+
ignore,
288+
'match-base': matchBase,
289+
'max-depth': maxDepth,
290+
mark,
291+
nobrace,
292+
nocase,
293+
nodir,
294+
noext,
295+
noglobstar,
296+
platform,
297+
realpath,
298+
root,
299+
stat,
300+
debug,
301+
posix,
302+
'cmd-arg': cmdArg,
303+
} = values
304+
if (showVersion) {
231305
console.log(version)
232306
process.exit(0)
233307
}
234-
if (values.help) {
308+
if (help) {
235309
console.log(j.usage())
236310
process.exit(0)
237311
}
238-
if (positionals.length === 0 && !values.default)
239-
throw 'No patterns provided'
240-
if (positionals.length === 0 && values.default)
241-
positionals.push(values.default)
312+
//const { shell, help } = values
313+
if (positionals.length === 0 && !def) throw 'No patterns provided'
314+
if (positionals.length === 0 && def) positionals.push(def)
242315
const patterns =
243-
values.all ? positionals : positionals.filter(p => !existsSync(p))
316+
all ? positionals : positionals.filter(p => !existsSync(p))
244317
const matches =
245-
values.all ?
246-
[]
247-
: positionals.filter(p => existsSync(p)).map(p => join(p))
318+
all ? [] : positionals.filter(p => existsSync(p)).map(p => join(p))
319+
248320
const stream = globStream(patterns, {
249-
absolute: values.absolute,
250-
cwd: values.cwd,
251-
dot: values.dot,
252-
dotRelative: values['dot-relative'],
253-
follow: values.follow,
254-
ignore: values.ignore,
255-
mark: values.mark,
256-
matchBase: values['match-base'],
257-
maxDepth: values['max-depth'],
258-
nobrace: values.nobrace,
259-
nocase: values.nocase,
260-
nodir: values.nodir,
261-
noext: values.noext,
262-
noglobstar: values.noglobstar,
263-
platform: values.platform as undefined | NodeJS.Platform,
264-
realpath: values.realpath,
265-
root: values.root,
266-
stat: values.stat,
267-
debug: values.debug,
268-
posix: values.posix,
321+
absolute,
322+
cwd,
323+
dot,
324+
dotRelative,
325+
follow,
326+
ignore,
327+
mark,
328+
matchBase,
329+
maxDepth,
330+
nobrace,
331+
nocase,
332+
nodir,
333+
noext,
334+
noglobstar,
335+
platform: platform as undefined | NodeJS.Platform,
336+
realpath,
337+
root,
338+
stat,
339+
debug,
340+
posix,
269341
})
270342

271-
const cmd = values.cmd
272343
if (!cmd) {
273344
matches.forEach(m => console.log(m))
274345
stream.on('data', f => console.log(f))
275346
} else {
276-
stream.on('data', f => matches.push(f))
277-
stream.on('end', () => foregroundChild(cmd, matches, { shell: true }))
347+
cmdArg.push(...matches)
348+
stream.on('data', f => cmdArg.push(f))
349+
// Attempt to support commands that contain spaces and otherwise require
350+
// shell interpretation, but do NOT shell-interpret the arguments, to avoid
351+
// injections via filenames. This affordance can only be done on known Unix
352+
// shells, unfortunately.
353+
//
354+
// 'bash', ['-c', cmd + ' "$@"', 'bash', ...matches]
355+
// 'zsh', ['-c', cmd + ' "$@"', 'zsh', ...matches]
356+
// 'fish', ['-c', cmd + ' "$argv"', ...matches]
357+
const { SHELL = 'unknown' } = process.env
358+
const shellBase = basename(SHELL)
359+
const knownShells = ['sh', 'ksh', 'zsh', 'bash', 'fish']
360+
if (
361+
(shell || /[ "']/.test(cmd)) &&
362+
knownShells.includes(shellBase)
363+
) {
364+
const cmdWithArgs = `${cmd} "\$${shellBase === 'fish' ? 'argv' : '@'}"`
365+
if (shellBase !== 'fish') {
366+
cmdArg.unshift(SHELL)
367+
}
368+
cmdArg.unshift('-c', cmdWithArgs)
369+
stream.on('end', () => foregroundChild(SHELL, cmdArg))
370+
} else {
371+
if (shell) {
372+
process.emitWarning(
373+
'The --shell option is unsafe, and will be removed. To pass ' +
374+
'positional arguments to the subprocess, use -g/--cmd-arg instead.',
375+
'DeprecationWarning',
376+
'GLOB_SHELL',
377+
)
378+
}
379+
stream.on('end', () => foregroundChild(cmd, cmdArg, { shell }))
380+
}
278381
}
279382
} catch (e) {
280383
console.error(j.usage())

‎tap-snapshots/test/bin.ts.test.cjs‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,45 @@ Object {
3131
If no positional arguments are provided, glob will use
3232
this pattern
3333
34+
--shell Interpret the command as a shell command by passing it
35+
to the shell, with all matched filesystem paths
36+
appended,
37+
**even if this cannot be done safely**.
38+
39+
This is **not** unsafe (and usually unnecessary) when
40+
using the known Unix shells sh, bash, zsh, and fish, as
41+
these can all be executed in such a way as to pass
42+
positional arguments safely.
43+
44+
**Note**: THIS IS UNSAFE IF THE FILE PATHS ARE
45+
UNTRUSTED, because a path like \`'some/path/\\\\$\\\\(cmd)'\`
46+
will be executed by the shell.
47+
48+
If you do have positional arguments that you wish to
49+
pass to the command ahead of the glob pattern matches,
50+
use the \`--cmd-arg\`/\`-g\` option instead.
51+
52+
The next major release of glob will fully remove the
53+
ability to use this option unsafely.
54+
55+
-g<arg> --cmd-arg=<arg>
56+
Pass the provided values to the supplied command, ahead
57+
of the glob matches.
58+
59+
For example, the command:
60+
61+
glob -c echo -g"hello" -g"world" *.txt
62+
63+
might output:
64+
65+
hello world a.txt b.txt
66+
67+
This is a safer (and future-proof) alternative than
68+
putting positional arguments in the \`-c\`/\`--cmd\`
69+
option.
70+
71+
Can be set multiple times
72+
3473
-A --all By default, the glob cli command will not expand any
3574
arguments that are an exact match to a file on disk.
3675

0 commit comments

Comments
 (0)