security: remove dead code fix cmd exec#162
Merged
Conversation
Signed-off-by: Jingxiang Zhang <jingzhang@nvidia.com>
Fix command injection in actively-used SDK packages by replacing `process.WithCommand(fmt.Sprintf(...))` / `bash -c` string interpolation with `exec.CommandContext` using discrete argument arrays: - pkg/disk/findmnt.go: `findmnt --target <target>` args separated - pkg/disk/lsblk.go: `lsblk <flags>` split via strings.Fields - pkg/host/machine_id.go: `dmidecode -t system` args separated - pkg/host/virtualization_environment.go: `sudo dmidecode -s ...` args separated, removing shell interpolation of the binary path - pkg/systemd/systemd.go: `systemd --version` args separated Fix HTTP server hardening in the agent: - internal/server/server.go: add ReadHeaderTimeout, ReadTimeout, WriteTimeout, and IdleTimeout to prevent connection exhaustion; replace wildcard CORS (`*`) with origin echo to block cross-origin reads from arbitrary websites on the same machine - internal/server/handlers_inject_fault.go: wrap request body with http.MaxBytesReader (1 MiB) to bound memory use - internal/enrollment/enrollment.go: wrap response body with io.LimitReader (1 MiB) against oversized responses from a malicious enrollment server - internal/attestation/attestation.go: remove nonce value from debug log fields to prevent attestation challenges appearing in log files Signed-off-by: Rodrigo Sampaio Vaz <rvaz@nvidia.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughLimits request and response sizes, adds HTTP server timeouts, removes nonce values from attestation logs, replaces a custom subprocess helper with direct exec.CommandContext(...) calls across multiple third-party packages, and adds a test that verifies enrollment responses exceeding the new size limit error out. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 503e63ba95
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
third_party/fleet-intelligence-sdk/pkg/host/virtualization_environment.go (1)
48-60: Inconsistency:GetSystemdDetectVirtstill uses shell interpolation.While the PR fixes
GetSystemManufacturer, this function still constructs a bash script withfmt.Sprintfand interpolatesdetectExecPathinto it. AlthoughdetectExecPathcomes fromfile.LocateExecutable()and is likely safe, this pattern is inconsistent with the security hardening applied elsewhere in this PR.Consider refactoring to run the three commands separately using
exec.CommandContextwith discrete arguments, similar to the other changes in this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@third_party/fleet-intelligence-sdk/pkg/host/virtualization_environment.go` around lines 48 - 60, GetSystemdDetectVirt currently builds a bash script via fmt.Sprintf and injects detectExecPath into process.New (the process.WithBashScriptContentsToRun block); refactor it to avoid shell interpolation by invoking the three detection commands separately using exec.CommandContext (or the existing process helper that wraps exec) with explicit argument slices (e.g., detectExecPath with args ["--vm"], ["--container"], and []), run them sequentially, collect/merge their outputs and errors, and replace the process.New call that embeds the inline script so the code uses discrete exec invocations instead of a formatted bash script.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/server/server.go`:
- Around line 474-489: The CORS middleware registered via router.Use currently
echoes any Origin header which effectively allows arbitrary origins; update the
middleware that reads c.Request.Header.Get("Origin") to validate the origin is a
loopback origin (e.g., scheme + host is localhost, 127.0.0.1, or [::1]) or
matches a configured allowlist before setting Access-Control-Allow-Origin,
Access-Control-Allow-Methods, Access-Control-Allow-Headers and Vary headers; if
the origin is not allowed, do not set Access-Control-Allow-Origin (but still
handle OPTIONS with AbortWithStatus(204) as currently done). Ensure the
validation logic is applied in the same middleware function so only validated
origins are reflected.
---
Nitpick comments:
In `@third_party/fleet-intelligence-sdk/pkg/host/virtualization_environment.go`:
- Around line 48-60: GetSystemdDetectVirt currently builds a bash script via
fmt.Sprintf and injects detectExecPath into process.New (the
process.WithBashScriptContentsToRun block); refactor it to avoid shell
interpolation by invoking the three detection commands separately using
exec.CommandContext (or the existing process helper that wraps exec) with
explicit argument slices (e.g., detectExecPath with args ["--vm"],
["--container"], and []), run them sequentially, collect/merge their outputs and
errors, and replace the process.New call that embeds the inline script so the
code uses discrete exec invocations instead of a formatted bash script.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cc2a06fe-d910-4c01-905e-63df97f529b8
📒 Files selected for processing (9)
internal/attestation/attestation.gointernal/enrollment/enrollment.gointernal/server/handlers_inject_fault.gointernal/server/server.gothird_party/fleet-intelligence-sdk/pkg/disk/findmnt.gothird_party/fleet-intelligence-sdk/pkg/disk/lsblk.gothird_party/fleet-intelligence-sdk/pkg/host/machine_id.gothird_party/fleet-intelligence-sdk/pkg/host/virtualization_environment.gothird_party/fleet-intelligence-sdk/pkg/systemd/systemd.go
jingxiang-z
changed the title
Signed-off-by: Jingxiang Zhang <jingzhang@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/enrollment/enrollment.go (1)
80-83: Adjust oversize-path log message for accuracy.On Line 82, logging
"Failed to read enrollment response body"is misleading because the read succeeded and the failure is a size-policy violation. Consider a dedicated message and structured fields for max/actual size.📌 Suggested logging tweak
if len(respBody) > maxEnrollmentResponseSize { err = fmt.Errorf("enrollment response too large (max %d bytes)", maxEnrollmentResponseSize) - log.Logger.Errorw("Failed to read enrollment response body", "error", err) + log.Logger.Errorw("Enrollment response exceeded maximum allowed size", + "error", err, + "max_bytes", maxEnrollmentResponseSize, + "received_bytes", len(respBody)) return "", err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/enrollment/enrollment.go` around lines 80 - 83, The log message for the oversize path is misleading; update the error logging when len(respBody) > maxEnrollmentResponseSize to use a dedicated message like "enrollment response exceeds max allowed size" and call log.Logger.Errorw with structured fields for "max_size" (maxEnrollmentResponseSize) and "actual_size" (len(respBody)) alongside the error (err) so the log accurately reflects a policy-size violation rather than a read failure.internal/server/handlers_test.go (1)
626-677: Add edge-case coverage for loopback origin parsing.Please add explicit cases for IPv6 loopback origin (
http://[::1]:3000) and uppercase localhost (http://LOCALHOST:3000) to prevent regressions inisLoopbackOrigin.Suggested test additions
{ name: "OPTIONS_request_ipv4_loopback_origin", method: "OPTIONS", origin: "http://127.0.0.1:3000", checkHeaders: func(t *testing.T, w *httptest.ResponseRecorder) { assert.Equal(t, "http://127.0.0.1:3000", w.Header().Get("Access-Control-Allow-Origin")) assert.Equal(t, "GET, POST, OPTIONS", w.Header().Get("Access-Control-Allow-Methods")) }, expectedStatus: http.StatusNoContent, }, + { + name: "OPTIONS_request_ipv6_loopback_origin", + method: "OPTIONS", + origin: "http://[::1]:3000", + checkHeaders: func(t *testing.T, w *httptest.ResponseRecorder) { + assert.Equal(t, "http://[::1]:3000", w.Header().Get("Access-Control-Allow-Origin")) + assert.Equal(t, "GET, POST, OPTIONS", w.Header().Get("Access-Control-Allow-Methods")) + }, + expectedStatus: http.StatusNoContent, + }, + { + name: "OPTIONS_request_uppercase_localhost_origin", + method: "OPTIONS", + origin: "http://LOCALHOST:3000", + checkHeaders: func(t *testing.T, w *httptest.ResponseRecorder) { + assert.Equal(t, "http://LOCALHOST:3000", w.Header().Get("Access-Control-Allow-Origin")) + assert.Equal(t, "GET, POST, OPTIONS", w.Header().Get("Access-Control-Allow-Methods")) + }, + expectedStatus: http.StatusNoContent, + }, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/handlers_test.go` around lines 626 - 677, Add two table-driven test cases in the tests slice used for CORS checks to cover IPv6 loopback and uppercase hostname: add an OPTIONS case with origin "http://[::1]:3000" and expectedStatus http.StatusNoContent whose checkHeaders asserts Access-Control-Allow-Origin == "http://[::1]:3000" and Access-Control-Allow-Methods == "GET, POST, OPTIONS"; and add an OPTIONS case with origin "http://LOCALHOST:3000" and expectedStatus http.StatusNoContent whose checkHeaders asserts Access-Control-Allow-Origin == "http://LOCALHOST:3000" and Access-Control-Allow-Methods == "GET, POST, OPTIONS"; these will exercise isLoopbackOrigin and ensure loopback/hostname case-insensitivity is covered alongside the existing IPv4 test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/server/server.go`:
- Around line 507-509: The hostname comparison for loopback currently uses a
case-sensitive check ("if host == \"localhost\" { return true }"); make it
case-insensitive by using a case-insensitive comparison (e.g.,
strings.EqualFold(host, "localhost") or strings.ToLower(host) == "localhost")
and add the necessary import for the strings package; update the condition in
the function that evaluates origins (the block referencing the host variable) so
variants like "LOCALHOST" or "LocalHost" are accepted.
---
Nitpick comments:
In `@internal/enrollment/enrollment.go`:
- Around line 80-83: The log message for the oversize path is misleading; update
the error logging when len(respBody) > maxEnrollmentResponseSize to use a
dedicated message like "enrollment response exceeds max allowed size" and call
log.Logger.Errorw with structured fields for "max_size"
(maxEnrollmentResponseSize) and "actual_size" (len(respBody)) alongside the
error (err) so the log accurately reflects a policy-size violation rather than a
read failure.
In `@internal/server/handlers_test.go`:
- Around line 626-677: Add two table-driven test cases in the tests slice used
for CORS checks to cover IPv6 loopback and uppercase hostname: add an OPTIONS
case with origin "http://[::1]:3000" and expectedStatus http.StatusNoContent
whose checkHeaders asserts Access-Control-Allow-Origin == "http://[::1]:3000"
and Access-Control-Allow-Methods == "GET, POST, OPTIONS"; and add an OPTIONS
case with origin "http://LOCALHOST:3000" and expectedStatus http.StatusNoContent
whose checkHeaders asserts Access-Control-Allow-Origin ==
"http://LOCALHOST:3000" and Access-Control-Allow-Methods == "GET, POST,
OPTIONS"; these will exercise isLoopbackOrigin and ensure loopback/hostname
case-insensitivity is covered alongside the existing IPv4 test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b6f4ba93-9e9a-411f-abcb-3256324dabe8
📒 Files selected for processing (4)
internal/enrollment/enrollment.gointernal/enrollment/enrollment_test.gointernal/server/handlers_test.gointernal/server/server.go
Signed-off-by: Jingxiang Zhang <jingzhang@nvidia.com>
jingxiang-z
approved these changes
Apr 10, 2026
Signed-off-by: Jingxiang Zhang <jingzhang@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
third_party/fleet-intelligence-sdk/pkg/host/virtualization_environment.go (1)
42-45:⚠️ Potential issue | 🟠 MajorDon’t mask missing executable lookup failures as success.
Line 44 and Line 95 return
nilerror when required binaries are missing, so callers cannot distinguish “tool unavailable” from successful detection (notably inthird_party/fleet-intelligence-sdk/pkg/host/init.gowhereerris checked).🔧 Proposed fix
func GetSystemdDetectVirt(ctx context.Context) (VirtualizationEnvironment, error) { detectExecPath, err := file.LocateExecutable("systemd-detect-virt") if err != nil { - return VirtualizationEnvironment{}, nil + return VirtualizationEnvironment{}, fmt.Errorf("locate systemd-detect-virt: %w", err) } @@ func GetSystemManufacturer(ctx context.Context) (string, error) { dmidecodePath, err := file.LocateExecutable("dmidecode") if err != nil { - return "", nil + return "", fmt.Errorf("locate dmidecode: %w", err) }Also applies to: 93-96
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@third_party/fleet-intelligence-sdk/pkg/host/virtualization_environment.go` around lines 42 - 45, The current code masks failures from file.LocateExecutable by returning a nil error (e.g., in the block that sets detectExecPath), which prevents callers from distinguishing “tool missing” vs success; update the code paths that call file.LocateExecutable (the block assigning detectExecPath and the similar block at lines ~93-96) to propagate and return the actual error instead of nil, ensuring functions like the VirtualizationEnvironment constructor (returning VirtualizationEnvironment{}) return the LocateExecutable error so callers can detect missing binaries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@third_party/fleet-intelligence-sdk/pkg/host/virtualization_environment.go`:
- Around line 80-85: The code currently treats any *exec.ExitError (variable
exitErr) as non-fatal; change this so only the documented "no virtualization"
exit code (ExitError.ExitCode() == 1) is treated as success: when errors.As(err,
&exitErr) is true check exitErr.ExitCode() == 1 and return trimmed, nil only in
that case, otherwise propagate the error (return err) so genuine failures
(permission/file issues) are not masked; update the branch around the
errors.As(err, &exitErr) check in virtualization_environment.go accordingly.
---
Outside diff comments:
In `@third_party/fleet-intelligence-sdk/pkg/host/virtualization_environment.go`:
- Around line 42-45: The current code masks failures from file.LocateExecutable
by returning a nil error (e.g., in the block that sets detectExecPath), which
prevents callers from distinguishing “tool missing” vs success; update the code
paths that call file.LocateExecutable (the block assigning detectExecPath and
the similar block at lines ~93-96) to propagate and return the actual error
instead of nil, ensuring functions like the VirtualizationEnvironment
constructor (returning VirtualizationEnvironment{}) return the LocateExecutable
error so callers can detect missing binaries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: add553e9-3916-4810-bb28-9764d6a23655
📒 Files selected for processing (1)
third_party/fleet-intelligence-sdk/pkg/host/virtualization_environment.go
Signed-off-by: Jingxiang Zhang <jingzhang@nvidia.com>
jingxiang-z
added a commit
that referenced
this pull request
Apr 14, 2026
Signed-off-by: Jingxiang Zhang <jingzhang@nvidia.com> Signed-off-by: Rodrigo Sampaio Vaz <rvaz@nvidia.com> Co-authored-by: Jingxiang Zhang <jingzhang@nvidia.com>
jingxiang-z
added a commit
that referenced
this pull request
Apr 14, 2026
Signed-off-by: Jingxiang Zhang <jingzhang@nvidia.com> Signed-off-by: Rodrigo Sampaio Vaz <rvaz@nvidia.com> Co-authored-by: Jingxiang Zhang <jingzhang@nvidia.com>
jingxiang-z
added a commit
that referenced
this pull request
Apr 14, 2026
Signed-off-by: Jingxiang Zhang <jingzhang@nvidia.com> Signed-off-by: Rodrigo Sampaio Vaz <rvaz@nvidia.com> Co-authored-by: Jingxiang Zhang <jingzhang@nvidia.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fix command injection in actively-used SDK packages by replacing
process.WithCommand(fmt.Sprintf(...))/bash -cstringinterpolation with
exec.CommandContextusing discrete argumentarrays:
findmnt --target <target>args separatedlsblk <flags>split via strings.Fieldsdmidecode -t systemargs separatedsudo dmidecode -s ...args separated, removing shell interpolation of the binary path
systemd --versionargs separatedFix HTTP server hardening in the agent:
WriteTimeout, and IdleTimeout to prevent connection exhaustion;
replace wildcard CORS (
*) with origin echo to block cross-originreads from arbitrary websites on the same machine
http.MaxBytesReader (1 MiB) to bound memory use
io.LimitReader (1 MiB) against oversized responses from a
malicious enrollment server
log fields to prevent attestation challenges appearing in log files
Checklist
Summary by CodeRabbit
Bug Fixes
Tests
Refactor