Skip to content

Commit 6fb41ad

Browse files
Apply suggestions from code review
Co-authored-by: Erik Krogh Kristensen <[email protected]>
1 parent f89992e commit 6fb41ad

File tree

3 files changed

+16
-23
lines changed

3 files changed

+16
-23
lines changed

javascript/ql/src/Security/CWE-078/CommandInjection.inc.qhelp

Lines changed: 7 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -10,15 +10,14 @@ allows the user to execute malicious code.</p>
1010
</overview>
1111

1212
<recommendation>
13-
<p>If possible, use APIs that don't run shell commands, and accept command
13+
<p>If possible, use APIs that don't run shell commands and accept command
1414
arguments as an array of strings rather than a single concatenated string. This
1515
is both safer and more portable.</p>
1616

17-
<p>If you are given the arguments as a single string, note that it is not safe
18-
to simply split the string on whitespace, since an argument may contain quoted
19-
whitespace which would cause it to be split into multiple arguments. Instead,
20-
use a library such as <code>shell-quote</code> to parse the string into an array
21-
of arguments.</p>
17+
<p>If given arguments as a single string, avoid simply splitting the string on
18+
whitespace. Arguments may contain quoted whitespace, causing them to split into
19+
multiple arguments. Use a library like <code>shell-quote</code> to parse the string
20+
into an array of arguments instead.</p>
2221

2322
<p>If this approach is not viable, then add code to verify that the user input
2423
string is safe before using it.</p>
@@ -31,12 +30,9 @@ command to count its lines without examining it first.</p>
3130

3231
<sample src="examples/command-injection.js" />
3332

34-
<p>A malicious user can exploit this code to execute arbitrary shell commands by
35-
passing a filename like <code>foo.txt; rm -rf .</code>, which will first count
36-
the lines in <code>foo.txt</code> and then delete all files in the current
37-
directory.</p>
33+
<p>A malicious user can take advantage of this code by executing arbitrary shell commands. For instance, by providing a filename like <code>foo.txt; rm -rf .</code>, the user can first count the lines in <code>foo.txt</code> and subsequently delete all files in the current directory. </p>
3834

39-
<p>To avoid this catastrophic loophole, use an API like
35+
<p>To avoid this catastrophic loophole, use an API such as
4036
<code>child_process.execFileSync</code> that does not spawn a shell by
4137
default:</p>
4238

javascript/ql/src/Security/CWE-078/IndirectCommandInjection.qhelp

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -27,20 +27,18 @@
2727

2828
<p>
2929

30-
If possible, use APIs that don't run shell commands, and accept
30+
If possible, use APIs that don't run shell commands and accept
3131
command arguments as an array of strings rather than a single
3232
concatenated string. This is both safer and more portable.
3333

3434
</p>
3535

3636
<p>
3737

38-
If you are given the arguments as a single string, note that it is
39-
not safe to simply split the string on whitespace, since an argument
40-
may contain quoted whitespace which would cause it to be split into
41-
multiple arguments. Instead, use a library such as
42-
<code>shell-quote</code> to parse the string into an array of
43-
arguments.
38+
<p>If given arguments as a single string, avoid simply splitting the string on
39+
whitespace. Arguments may contain quoted whitespace, causing them to split into
40+
multiple arguments. Use a library like <code>shell-quote</code> to parse the string
41+
into an array of arguments instead.</p>
4442

4543
</p>
4644

javascript/ql/src/Security/CWE-078/UnsafeShellCommandConstruction.qhelp

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -28,11 +28,10 @@
2828

2929
<p>
3030

31-
Note, however, that if you are given the arguments as a single string,
32-
it is not safe to simply split the string on whitespace, since an
33-
argument may contain quoted whitespace which would cause it to be split
34-
into multiple arguments. Instead, use a library such as
35-
<code>shell-quote</code> to parse the string into an array of arguments.
31+
If given arguments as a single string, avoid simply splitting the string on
32+
whitespace. Arguments may contain quoted whitespace, causing them to split into
33+
multiple arguments. Use a library like <code>shell-quote</code> to parse the string
34+
into an array of arguments instead.
3635

3736
</p>
3837

0 commit comments

Comments
 (0)