From 78bb56383d017ae8d431c27f9b4bf8c2bd57692b Mon Sep 17 00:00:00 2001 From: Anthony Ettinger Date: Mon, 17 Aug 2026 07:16:37 +0000 Subject: [PATCH] fix(scan): stop go-shell-exec-command reporting a fully literal argv MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `exec.Command("cmd", "/c", "ver")` reads the Windows version. It does re-introduce a shell, which is what the rule says, but there is nothing in the argv for anyone to reach — the metacharacters the consequence warns about are metacharacters nobody can supply. gosec's G204 draws the line in the same place: constant arguments are not the finding. This was the entire output of a whole-repository scan of SibtainOcn/Quiesce, a local Windows CLI in Go. One finding, and it was this one. The maintainer declined the scan workflow on the grounds that the classes it targets have no surface in that codebase, and he was right down to the single line. With the guard the repository scans clean, which is the honest result. The guard has to end at the closing paren so that a literal followed by anything else still reports: `"ls " + dir` leaves a `+`, `fmt.Sprintf(…)` leaves an identifier, a bare variable leaves a name. Escaped quotes inside a literal are consumed rather than treated as the end of one, so `"echo \"hi\""` does not fall out of the guard on a technicality. 323 tests pass. Both halves are covered: three shapes that must go quiet and three that must still fire. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/__tests__/false-positives.test.ts | 27 +++++++++++++++++++ packages/scan/src/code-rules.ts | 16 +++++++++++ 2 files changed, 43 insertions(+) diff --git a/packages/scan/src/__tests__/false-positives.test.ts b/packages/scan/src/__tests__/false-positives.test.ts index 844ae0f..d23ef54 100644 --- a/packages/scan/src/__tests__/false-positives.test.ts +++ b/packages/scan/src/__tests__/false-positives.test.ts @@ -349,6 +349,33 @@ describe('a constant HTML assignment that shares its line', () => { }); }); +describe('a Go shell call whose whole argv is literal', () => { + // Verbatim from SibtainOcn/Quiesce, a local Windows CLI. A whole-repository + // scan returned exactly one finding and this was it: a shell invocation + // reading the OS version, with nothing in the argv for anyone to influence. + it('does not flag exec.Command with an all-literal argv', () => { + expect(ruleIds('ui.go', 'out, err := exec.Command("cmd", "/c", "ver").Output()')).toEqual([]); + expect(ruleIds('a.go', 'exec.Command("sh", "-c", "ls -la").Run()')).toEqual([]); + expect(ruleIds('a.go', 'exec.CommandContext(ctx, "bash", "-c", "echo \\"hi\\"").Run()')).toEqual( + [], + ); + }); + + it('still flags one the caller can reach into', () => { + // Concatenation, a formatter and a bare identifier: the three ways the + // literal stops being the whole of it. + expect(ruleIds('a.go', 'exec.Command("sh", "-c", "ping -c 1 "+host).Output()')).toContain( + 'go-shell-exec-command', + ); + expect( + ruleIds('a.go', 'exec.Command("sh", "-c", fmt.Sprintf("ping %s", host)).Output()'), + ).toContain('go-shell-exec-command'); + expect(ruleIds('a.go', 'exec.Command("sh", "-c", command).Output()')).toContain( + 'go-shell-exec-command', + ); + }); +}); + describe('findings this triage deliberately left alone', () => { // Suppressing these would be the scanner talking itself out of real classes. it('still reports a nested quantifier, a lifecycle script and a live HTML sink', () => { diff --git a/packages/scan/src/code-rules.ts b/packages/scan/src/code-rules.ts index 7fd4661..1013642 100644 --- a/packages/scan/src/code-rules.ts +++ b/packages/scan/src/code-rules.ts @@ -481,6 +481,22 @@ export const CODE_RULES: readonly CodeRule[] = [ severity: 'critical', languages: ['go'], pattern: /\bexec\.Command(?:Context)?\s*\(\s*(?:ctx\s*,\s*)?"(?:\/bin\/)?(?:sh|bash|zsh|cmd|powershell)"\s*,\s*"(?:-c|\/c)"/, + // A call whose whole argv is string literals cannot be injected into. + // `exec.Command("cmd", "/c", "ver")` reads the Windows version; there is no + // value in it for an attacker to reach, and the consequence above — that a + // shell will interpret metacharacters — describes metacharacters nobody can + // supply. gosec's G204 draws the line in the same place, and this was the + // single finding a Go project got out of a whole scan before declining the + // offer, which is an expensive way to report nothing. + // + // The guard has to end at the closing paren, so a literal followed by + // anything else still reports: `"ls " + dir` leaves a `+` before the `)`, + // `fmt.Sprintf(…)` leaves an identifier, and a bare variable leaves a name. + // `(?:[^"\\]|\\.)*` rather than `[^"]*` so an escaped quote inside a + // literal — `"echo \"hi\""` — does not end the literal early and drop the + // guard on a line it should have covered. + lineGuard: + /\bexec\.Command(?:Context)?\s*\(\s*(?:ctx\s*,\s*)?"(?:\/bin\/)?(?:sh|bash|zsh|cmd|powershell)"\s*,\s*"(?:-c|\/c)"\s*(?:,\s*"(?:[^"\\]|\\.)*")*\s*,?\s*\)/, }, { id: 'rb-backtick-interpolation',