Harden scanner/relaunch exec paths against command and argument injection - #49063
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Triage Summary
Security-critical CWE-78 command/argument injection hardening for scanner/relaunch exec paths (Sighthound flagged). Still draft — needs undraft and expedited security review.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
There was a problem hiding this comment.
Pull request overview
Hardens Docker scanner mounts, Grant image inputs, and self-relaunch arguments before process execution.
Changes:
- Adds shared Docker mount validation helpers.
- Validates scanner paths, image references, and Docker executable resolution.
- Adds focused validation tests.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/docker_args_validation.go |
Adds Docker mount validation helpers. |
pkg/cli/docker_args_validation_test.go |
Tests mount validation. |
pkg/cli/grant.go |
Validates Grant execution inputs. |
pkg/cli/grant_test.go |
Tests invalid image references. |
pkg/cli/poutine.go |
Validates and safely constructs mounts. |
pkg/cli/runner_guard.go |
Uses the shared mount helper. |
pkg/cli/upgrade_command.go |
Rejects NUL-containing relaunch arguments. |
pkg/cli/upgrade_command_test.go |
Tests relaunch argument rejection. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 8/8 changed files
- Comments generated: 2
- Review effort level: Medium
| if strings.ContainsAny(imageRef, " \t\r\n\x00") { | ||
| return nil, fmt.Errorf("grant image reference contains invalid whitespace/control characters: %q", imageRef) | ||
| } |
| if err != nil { | ||
| return "", err | ||
| } | ||
| return cleanHostPath + ":" + cleanContainerPath, nil |
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. 🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (5 tests)
Verdict
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Harden scanner/relaunch exec paths
Good overall direction — the shared buildDockerReadonlyFileMount/buildDockerVolumeMount helpers, imageRef validation, and NUL-byte relaunch guard are all solid additions.
One blocking gap: poutine.go still passes bare "docker" to exec.Command in both runPoutineOnDirectory and runPoutineOnFile, while grant.go and runner_guard.go were both updated in this PR to use fileutil.ResolveExecutablePath("docker"). This is an inconsistency that leaves the stated hardening incomplete — see the inline comment for the fix.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 32.4 AIC · ⌖ 6.04 AIC · ⊞ 5.3K
| return fmt.Errorf("invalid docker mount path: %w", err) | ||
| } | ||
| cmd := exec.Command( | ||
| "docker", |
There was a problem hiding this comment.
poutine.go still uses bare "docker" here (and again at line 212), while grant.go and runner_guard.go were both updated in this PR to use fileutil.ResolveExecutablePath("docker").
Using a bare command name relies on PATH resolution at exec time. A malicious entry earlier in PATH could shadow the real Docker binary — the same weakness this PR explicitly hardens in the other two callers.
Fix both call sites (in runPoutineOnDirectory and runPoutineOnFile):
dockerPath, err := fileutil.ResolveExecutablePath("docker")
if err != nil {
return fmt.Errorf("docker command not found: %w", err)
}
cmd := exec.Command(dockerPath, "run", ...)@copilot please address this.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on three focused issues.
📋 Key Themes & Highlights
Key Themes
- Container path colon gap —
validateContainerMountPathcorrectly rejects NUL/CR/LF but misses:, which Docker uses as a field separator in-vargs. A colon in the container path would silently produce a malformed three-part mount string, exactly the class of injection this PR sets out to prevent. - Inconsistent docker executable resolution —
poutine.gostill uses the hard-coded string"docker"inexec.Commandwhilerunner_guard.goandgrant.gowere upgraded tofileutil.ResolveExecutablePath. The hardening is incomplete for the poutine paths. - Fragile grant test —
TestGrantRunOnImageRejectsInvalidImageRefpasses a hard-coded/tmp/policy.yamlthat may not exist on the runner, and skips the empty-string and NUL-byte cases.
Positive Highlights
- ✅ Clean centralisation of Docker mount construction into validated helpers.
- ✅
buildDockerReadonlyFileMountenforces regular-file requirement and:rosuffix correctly. - ✅ NUL-byte guard in
relaunchWithSameArgsis precise and well-tested. - ✅
imageRefTrimSpace + control-char rejection ingrant.gofollows defence-in-depth.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 50.9 AIC · ⌖ 5.34 AIC · ⊞ 7K
Comment /matt to run again
| if containerPath == "" { | ||
| return "", errors.New("container path cannot be empty") | ||
| } | ||
| if strings.ContainsAny(containerPath, "\x00\r\n") { |
There was a problem hiding this comment.
[/diagnosing-bugs] validateContainerMountPath does not reject colon (:) in the container path. A colon in the container-path component would split the assembled host:container string into unexpected Docker mount segments, undermining the injection protection this helper provides.
💡 Suggested fix
Add : to the rejected character set:
if strings.ContainsAny(containerPath, "\x00
:") {
return "", errors.New("container path contains invalid control characters")
}Without this, a path like /workdir:extra would produce hostPath:/workdir:extra, which Docker parses as three-part syntax (host:container:options).
@copilot please address this.
| return fmt.Errorf("invalid docker mount path: %w", err) | ||
| } | ||
| cmd := exec.Command( | ||
| "docker", |
There was a problem hiding this comment.
[/diagnosing-bugs] poutine.go calls exec.Command("docker", ...) (hard-coded string) while runner_guard.go and grant.go both resolve the path via fileutil.ResolveExecutablePath("docker"). This inconsistency means the poutine paths still rely on PATH lookup, which the PR explicitly hardened against in the other callers.
💡 Suggested fix
Resolve the docker executable before constructing the command, as done in runner_guard.go:
dockerPath, err := fileutil.ResolveExecutablePath("docker")
if err != nil {
return fmt.Errorf("docker command not found: %w", err)
}
cmd := exec.Command(dockerPath, "run", ...)Apply the same change in both runPoutineOnDirectory and runPoutineOnFile.
@copilot please address this.
| require.NoError(t, err) | ||
| require.Equal(t, tmpDir+":/workdir", mount) | ||
|
|
||
| _, err = buildDockerVolumeMount("relative/path", "/workdir") |
There was a problem hiding this comment.
[/tdd] The test for buildDockerVolumeMount does not cover a container path containing a colon or control characters, so the gap noted in the validateContainerMountPath check would not be caught by CI even after a fix.
💡 Suggested additional test cases
_, err = buildDockerVolumeMount(tmpDir, "/work:dir")
require.Error(t, err)
_, err = buildDockerVolumeMount(tmpDir, "/work\x00dir")
require.Error(t, err)Similarly, TestBuildDockerReadonlyFileMount should cover an invalid container path (control chars / colon) to ensure the validation is exercised end-to-end through both helpers.
@copilot please address this.
| @@ -73,3 +74,13 @@ func TestGrantPolicyFile(t *testing.T) { | |||
| t.Fatalf("Expected policy file basename %q, got %q", grantPolicyFilename, filepath.Base(policyFile)) | |||
| } | |||
| } | |||
There was a problem hiding this comment.
[/tdd] TestGrantRunOnImageRejectsInvalidImageRef calls grantRunOnImage with a non-existent policy file path (/tmp/policy.yaml). The test may fail or pass for the wrong reason depending on whether that file exists on the runner, and it doesn't test the NUL-byte or empty-string cases either.
💡 Suggested approach
Create a temp policy file in the test and add separate sub-tests for each rejection condition:
func TestGrantRunOnImageRejectsInvalidImageRef(t *testing.T) {
tmp := t.TempDir()
policyFile := filepath.Join(tmp, "policy.yaml")
require.NoError(t, os.WriteFile(policyFile, []byte("policy: true"), 0o644))
_, err := grantRunOnImage("bad image", policyFile, false)
require.ErrorContains(t, err, "invalid whitespace")
_, err = grantRunOnImage("", policyFile, false)
require.ErrorContains(t, err, "cannot be empty")
_, err = grantRunOnImage("img\x00ref", policyFile, false)
require.ErrorContains(t, err, "invalid whitespace")
}@copilot please address this.
There was a problem hiding this comment.
Verdict: Approve with one non-blocking hardening gap
Solid, narrowly-scoped security hardening PR with good test coverage for the new validation helpers. One inconsistency found (non-blocking) — see inline comment.
Details
- New
docker_args_validation.gohelpers (buildDockerVolumeMount,buildDockerReadonlyFileMount,validateContainerMountPath) correctly reject non-absolute paths, control characters, and non-regular files, and are exercised by focused unit tests. grant.goandrunner_guard.gocorrectly resolvedockerviafileutil.ResolveExecutablePathbeforeexec.Command, matching the PR's stated goal.poutine.go's two call sites (runPoutineOnDirectory,runPoutineOnFile) still hardcode the"docker"literal inexec.Commandinstead of using the sameResolveExecutablePathresolution used elsewhere in this PR — an inconsistency in an otherwise-thorough hardening pass (flagged inline).upgrade_command.go's NUL-byte guard on relaunch args is reasonable defense-in-depth, well tested.- Volume mount construction (string concatenation replaced by validated helper) is a genuine improvement over the prior direct string interpolation.
This is not blocking merge since the flagged inconsistency doesn't introduce a new vulnerability (docker is still resolved via the OS's normal PATH lookup semantics through exec.Command, just without the extra ValidateExecutablePath step); it is a consistency/completeness gap worth addressing to match the PR's own security posture.
🔎 Code quality review by PR Code Quality Reviewer · auto · 65.4 AIC · ⌖ 4.7 AIC · ⊞ 7.8K
Comment /review to run again
| return fmt.Errorf("invalid docker mount path: %w", err) | ||
| } | ||
| cmd := exec.Command( | ||
| "docker", |
There was a problem hiding this comment.
Docker path here is still resolved implicitly via PATH (exec.Command("docker", ...)) while grant.go and runner_guard.go in this same PR now use fileutil.ResolveExecutablePath("docker") for equivalent hardening.
💡 Why this matters
The PR's stated purpose is to harden all Docker exec paths against command/argument injection by validating executable resolution (ResolveExecutablePath calls exec.LookPath then ValidateExecutablePath). runPoutineOnDirectory and runPoutineOnFile (line 212) skip this and rely on bare exec.Command("docker", ...), leaving two of the four hardened call sites without the same PATH-resolution validation the PR introduces elsewhere. This is an inconsistent security posture within a single commit whose whole purpose is exec-path hardening — if ValidateExecutablePath catches something meaningful (e.g. a symlink or non-regular-file executable), poutine silently bypasses that check.
dockerPath, err := fileutil.ResolveExecutablePath("docker")
if err != nil {
return fmt.Errorf("docker command not found: %w", err)
}
cmd := exec.Command(
dockerPath,
"run",
...
)Apply the same pattern at line 212 as well.
Addressed in 26b202e. I updated the new validation errors in the hardened Docker/Grant/poutine/runner-guard/relaunch paths to include concrete usage examples so the lint-error-messages job stays above threshold, then reran the local checks. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in a8eed6d. I synced |
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. @copilot please run the Failed checks:
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in 3d0dc4b. I updated the cache-based CLI tests to seed the complete-download marker expected by the current artifact downloader and made |
PR Triage
Rationale: Addresses CWE-78 command/argument injection findings in scanner and self-relaunch exec paths — security-critical. Adds new validation helpers, tests, and an ADR. Currently has a merge conflict (dirty) blocking merge; reviews mixed (approved then changes requested, dismissed). Given security severity, recommend expedited maintainer attention once rebased.
|
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. @copilot please run the
|
Current HEAD |
|
🎉 This pull request is included in a new release. Release: |
Sighthound flagged critical CWE-78 findings across scanner and self-relaunch exec paths where Docker mounts/targets and relaunch arguments required stronger validation guarantees. This change adds explicit path/argument guards so all high-risk exec inputs are validated, normalized, and constrained before process launch.
Docker mount/path hardening (shared helpers)
pkg/cli/docker_args_validation.goto centralize safe Docker-vargument construction.fileutil.ValidateAbsolutePath)Grant scanner input hardening
pkg/cli/grant.gonow:fileutil.ResolveExecutablePath("docker")imageRef(rejects whitespace/control chars, empty input)Poutine and Runner Guard mount hardening
pkg/cli/poutine.goandpkg/cli/runner_guard.gonow build mounts via shared validation helpers instead of string concatenation.poutinealso validatesgitRootthroughfileutil.ValidateAbsolutePathbefore mount construction.Self-relaunch argument safety
pkg/cli/upgrade_command.gonow rejects relaunch arguments containing NUL bytes beforeexec.Command(exe, newArgs...).Focused unit coverage
pkg/cli/docker_args_validation_test.gopkg/cli/grant_test.gopkg/cli/upgrade_command_test.goWarning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.