Harden exec.Command inputs for scanner Docker invocations and upgrade re-exec - #49996
Conversation
|
Thanks for working on addressing the security findings from the Sighthound scan! This PR properly follows the agentic workflow process — assigned to a core team member, addressing a valid security issue (#49911), and marked as WIP during development. The approach of collecting remediation steps (path validation, sanitization, allow-listing) across the five critical locations aligns well with the guidance provided. Once the implementation is underway, reviewers will have a clear security focus to evaluate. Keep the PR description updated as you make progress! 🔒
|
… sites Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
exec.Command inputs for scanner Docker invocations and upgrade re-exec
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (76 additions detected). |
There was a problem hiding this comment.
Pull request overview
Hardens Docker scanner and upgrade relaunch arguments against unsafe input.
Changes:
- Adds structured Docker image-reference validation.
- Restricts relaunch flags and rejects unsafe forwarded arguments.
- Adds focused validation tests.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/docker_args_validation.go |
Validates image names, tags, and digests. |
pkg/cli/docker_args_validation_test.go |
Tests accepted and rejected image references. |
pkg/cli/upgrade_command.go |
Hardens upgrade relaunch arguments. |
pkg/cli/upgrade_command_test.go |
Tests relaunch validation failures. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Balanced
| if arg == "" { | ||
| return errors.New("invalid relaunch arguments: argument cannot be empty. Example: compile .github/workflows/example.md") | ||
| } | ||
| if containsControlCharacters(arg) { | ||
| return errors.New("invalid relaunch arguments: argument contains invalid control characters. Example: compile .github/workflows/example.md") | ||
| } |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 87/100 — Excellent
📊 Metrics (9 scenarios across 2 test files)
Verdict
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on two correctness issues and one test hygiene issue.
📋 Key Themes & Highlights
Issues
- Digest minimum length too permissive (
docker_args_validation.go:21) — allows 32-char (MD5-length) digests; OCI production minimum is 64 hex chars (SHA-256). Tighten{32,}→{64,}. - Registry hostname case inconsistency (
docker_args_validation.go:19) — the hostname regex allows uppercase while OCI spec normalises to lowercase, creating a subtle allow/deny mismatch. - Stale test name (
upgrade_command_test.go:149) —TestRelaunchWithSameArgsRejectsNullByteArgumentdescribes NUL bytes but the check now covers all control characters; rename to match the new behaviour.
Positive Highlights
- ✅ Excellent allow-list approach for
relaunchWithSameArgs— enumerated set is simple and auditable - ✅ Structured digest/tag/name parsing (cut + regex) is robust and easy to test
- ✅ Good separation between reject tests (
TestValidateDockerImageRefRejectsUnsafeCharacters) and accept tests (TestValidateDockerImageRefAcceptsCommonReferences) - ✅ Upgrading from NUL-only to full control-character check is a meaningful improvement
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 50.6 AIC · ⌖ 8.91 AIC · ⊞ 7.1K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/cli/docker_args_validation.go:21
[/diagnosing-bugs] dockerImageDigestRef accepts any hex string ≥ 32 chars, which admits MD5-length digests — weaker than OCI/Docker actually requires.
<details>
<summary>💡 Suggested fix</summary>
Tighten the minimum to 64 hex chars (SHA-256 is the production baseline):
dockerImageDigestRef = regexp.MustCompile(`^\[A-Za-z\]\[A-Za-z0-9\]*:\[0-9a-fA-F\]{64,}$`)MD5 (32) and SHA-1 (40) are not used by OCI registries for content addressing; allowing them widens the attack surfac…
pkg/cli/docker_args_validation.go:19
[/diagnosing-bugs] dockerImageNamePattern allows uppercase letters in the registry hostname (e.g. GHCR.IO/...) but Docker normalises registry hostnames to lowercase — this inconsistency could let a case-variant hostname bypass or be rejected unexpectedly.
<details>
<summary>💡 Details</summary>
The pattern currently is:
^(?:[a-zA-Z0-9.-]+(?::[0-9]+)?/)?[a-z0-9]+...
The hostname portion ([a-zA-Z0-9.-]+) permits uppercase while OCI spec and Docker normalise to lowercase. Cons…
pkg/cli/upgrade_command_test.go:149
[/tdd] The test name TestRelaunchWithSameArgsRejectsNullByteArgument no longer matches the implementation — NUL bytes are now rejected as control characters, not specifically as NUL bytes. The name is misleading for future readers.
<details>
<summary>💡 Suggested rename</summary>
func TestRelaunchWithSameArgsRejectsControlCharacterInArgument(t *testing.T) {Clear test names read as specifications; stale names erode trust in the test suite.
</details>
@copilot please address…
pkg/cli/docker_args_validation_test.go:121
[/tdd] The accept-case tests don't cover an image with both a tag and a digest (image:tag@sha256:...) — this is a valid OCI reference form and worth an explicit positive test to confirm the parser handles it.
<details>
<summary>💡 Suggested addition</summary>
"ghcr.io/org/image:1.0@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",Without this case it's easy for a refactor to silently break tag+digest references.
</details>
@copilot please address th…
There was a problem hiding this comment.
Verdict: Request Changes
The hardening direction is sound, but one change introduces a functional regression, and the digest validation is looser than intended.
💡 Themes
- Regression: the new empty-string check in
relaunchWithSameArgsapplies to all forwarded args (os.Args[1:]), not just the appended flag, which can break legitimate re-exec with empty-string arguments. This isn't a security fix (exec.Command never shells out) and should be scoped or removed. - Weak validation:
dockerImageDigestRefaccepts any alphanumeric "algorithm" prefix with 32+ hex chars instead of the real digest algorithm set (sha256/sha512) with exact lengths, undermining the stated goal of tightening the allow-list. - Image name/tag parsing logic for host:port-without-tag refs (e.g.
localhost:5000/image) was manually verified to work correctly, no issue there. - Test coverage for the new regex paths is reasonable but could add explicit host:port-without-tag and uppercase-repo-name cases.
🔎 Code quality review by PR Code Quality Reviewer · auto · 47.8 AIC · ⌖ 8.1 AIC · ⊞ 7.9K
Comment /review to run again
| for _, arg := range newArgs { | ||
| if strings.ContainsRune(arg, '\x00') { | ||
| return errors.New("invalid relaunch arguments: argument contains NUL byte. Example: compile .github/workflows/example.md") | ||
| if arg == "" { |
There was a problem hiding this comment.
Empty-string argument check now applies to every forwarded arg (os.Args[1:]), not just the appended extraFlag, which can turn legitimate empty-string CLI arguments into a hard re-exec failure.
💡 Details
Before this PR, the loop only rejected NUL bytes in newArgs. Now arg == "" unconditionally errors for any element of os.Args[1:], including args the user actually passed (e.g. an explicit empty-string flag value, or an empty positional arg from shell quoting like gh-aw compile ""). This is a functional regression introduced under the guise of hardening: an empty string is not a command-injection vector for exec.Command (which never invokes a shell), so rejecting it adds no security value but breaks previously-working invocations during re-exec after upgrade.
Suggested fix: drop the empty-string check entirely, or scope it to only extraFlag if empty extra flags are truly invalid.
| var ( | ||
| dockerImageNamePattern = regexp.MustCompile(`^(?:[a-zA-Z0-9.-]+(?::[0-9]+)?/)?[a-z0-9]+(?:[._-][a-z0-9]+)*(?:/[a-z0-9]+(?:[._-][a-z0-9]+)*)*$`) | ||
| dockerImageTagPattern = regexp.MustCompile(`^[A-Za-z0-9_][A-Za-z0-9_.-]{0,127}$`) | ||
| dockerImageDigestRef = regexp.MustCompile(`^[A-Za-z][A-Za-z0-9]*:[0-9a-fA-F]{32,}$`) |
There was a problem hiding this comment.
The digest pattern accepts any 32+ hex-char string after any alphanumeric "algorithm" name, which is far looser than real digest algorithms and provides only superficial validation.
💡 Details
dockerImageDigestRef = ^[A-Za-z][A-Za-z0-9]*:[0-9a-fA-F]{32,}$ will accept things like x:00000000000000000000000000000000 as a "valid" digest — there's no allow-list of known algorithms (sha256, sha512) nor exact-length enforcement (sha256 digests are always exactly 64 hex chars). Since this validation is explicitly meant to be a security allow-list (per the PR description), it should assert the real digest shape precisely rather than a loose superset, otherwise malformed/attacker-controlled digest-shaped strings slip through unnoticed.
Suggested fix: ^sha256:[0-9a-fA-F]{64}$|^sha512:[0-9a-fA-F]{128}$ (or whatever algorithm set is actually supported downstream).
There was a problem hiding this comment.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 51.4 AIC · ⌖ 7.45 AIC · ⊞ 5.4K
| func relaunchWithSameArgs(extraFlag string, exeOverride string) error { | ||
| allowedExtraFlags := map[string]struct{}{ | ||
| "--skip-extension-upgrade": {}, | ||
| "--post-upgrade": {}, |
There was a problem hiding this comment.
Dead allowlist entry: --post-upgrade is in the allowlist but has no call sites — only --skip-extension-upgrade is ever passed as extraFlag. If this is reserved for future use, add a comment. Otherwise remove it to keep the security surface minimal.
@copilot please address this.
|
@copilot sous-chef triage: This PR still has unresolved review feedback covering both correctness and compatibility.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in 223a896.
|
PR Triage: #49996
Hardens
|
|
@copilot This PR still has unresolved blocking review feedback. Please address these threads, then run the Unresolved reviews:
|
|
🎉 This pull request is included in a new release. Release: |
Sighthound flagged high-confidence command-injection risk paths where dynamic values flow into
exec.Commandarguments (scanner docker args and upgrade relaunch args). This PR tightens validation/allow-listing at argument construction points to ensure only expected path/image/flag shapes are executable.Docker image reference hardening (
pkg/cli/docker_args_validation.go)validateDockerImageRef:-@digest separatoralgo:hex{32,})[A-Za-z0-9_][A-Za-z0-9_.-]{0,127})Upgrade relaunch argument hardening (
pkg/cli/upgrade_command.go)relaunchWithSameArgsnow allow-listsextraFlagto known internal flags only:--skip-extension-upgrade--post-upgradeFocused regression coverage
pkg/cli/docker_args_validation_test.gopkg/cli/upgrade_command_test.go