fix(security): harden P0 code scanning boundaries#4878
Merged
Conversation
Contributor
|
Wondering what really moved? Review this PR in Change Stack to inspect semantic changes, definitions, and references. 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:
📝 WalkthroughWalkthroughCentralized argv validation and NUL-byte rejection; atomic temp-file writes for state and credential sanitization; symlink-resistant descriptor-based file operations; curl-probe argv validation with trusted-config allowlist; lock-file snapshot inode checks; related tests updated. ChangesSecurity hardening: atomic writes, symlink-safe file ops, and argv validation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
6 tasks
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Auto-dispatched E2E: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
E2E Scenario Advisor RecommendationRequired scenario E2E: Dispatch required scenario E2E:
Full scenario advisor summaryE2E Scenario AdvisorBase: Required scenario E2E
Optional scenario E2E
Relevant changed files
|
Contributor
PR Review AdvisorFindings: 1 needs attention, 14 worth checking, 0 nice ideas Review findings🛠️ Needs attention
🔎 Worth checking
🌱 Nice ideas
Consider writing more tests for
Since last review detailsCurrent findings:
This is an automated advisory review. A human maintainer must make the final merge decision. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/lib/security/credential-filter.ts (1)
45-50: 💤 Low valueRedundant
chmodSyncafter atomic rename creates a minor TOCTOU window.The
writeFileSyncon line 47 already appliesmode: 0o600. The subsequentchmodSyncafterrenameSyncis redundant and introduces a brief window where the file exists at the final path before the chmod completes. While the risk is minimal (initial mode is already correct), removing the extra chmod simplifies the code.♻️ Remove redundant chmod
function writeFileAtomically(filePath: string, contents: string): void { const tmpPath = join(dirname(filePath), `.${basename(filePath)}.${process.pid}.${randomUUID()}.tmp`); writeFileSync(tmpPath, contents, { mode: 0o600 }); renameSync(tmpPath, filePath); - chmodSync(filePath, 0o600); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/security/credential-filter.ts` around lines 45 - 50, The writeFileAtomically function has a redundant chmodSync call after renameSync that creates a small TOCTOU window; remove the final chmodSync(filePath, 0o600) line and rely on writeFileSync(tmpPath, contents, { mode: 0o600 }) plus the atomic renameSync(tmpPath, filePath) to ensure the destination file has correct permissions, leaving tmpPath creation, writeFileSync and renameSync unchanged.nemoclaw/src/blueprint/state.test.ts (1)
179-183: 💤 Low valueTest description is now misleading after
clearStatebehavior change.The test is named "does nothing when no file exists", but
clearStatenow unconditionally writes a blank state file via the atomic write path. The test still passes (no throw), but the description no longer reflects the actual behavior.Consider updating the test name to "creates blank state when no file exists" and optionally asserting that the state file is created.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nemoclaw/src/blueprint/state.test.ts` around lines 179 - 183, Rename and update the test that currently reads "does nothing when no file exists" to reflect the new behavior of clearState: change the description to "creates blank state when no file exists" and add an assertion that the state file is created (or that the atomic write path was invoked) after calling clearState; target the test in state.test.ts that calls clearState() so the test both documents and verifies the new unconditional blank-state write behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/actions/uninstall/plan.ts`:
- Line 34: The openSync call uses fs.constants.O_NOFOLLOW which is undefined on
Windows so symlink protection is lost; change the flag computation in the code
around openSync(shimPath, ...) to only OR in fs.constants.O_NOFOLLOW when it is
defined (e.g. compute flags = fs.constants.O_RDONLY and if (typeof
fs.constants.O_NOFOLLOW !== 'undefined') flags |= fs.constants.O_NOFOLLOW) and
then call openSync(shimPath, flags); apply the same conditional-flag pattern to
the analogous code in credential-filter.ts to ensure consistent, platform-safe
behavior.
In `@src/lib/credentials/store.ts`:
- Line 240: The open call using fs.openSync(filePath, fs.constants.O_RDWR |
fs.constants.O_NOFOLLOW) can silently drop O_NOFOLLOW on Windows and cause
secureUnlink to zero-out a symlink target; update secureUnlink to detect this:
if fs.constants.O_NOFOLLOW is undefined or process.platform === 'win32', use
fs.lstatSync(filePath) first and abort if lstat.isSymbolicLink() (or, on
Windows, skip the secure zeroing and perform a plain unlink instead), otherwise
keep the existing open with O_NOFOLLOW; ensure you reference the existing
fs.openSync call and the secureUnlink function when making the change.
In `@src/lib/security/credential-filter.test.ts`:
- Around line 134-143: The test creates a symlink with symlinkSync to verify
sanitizeConfigFile doesn't follow symlinks, but symlink creation can fail on
Windows; wrap the symlinkSync(...) call in a try/catch and, if it throws (e.g.,
EPERM/EACCES), call it.skip with a clear message so the test is skipped instead
of failing. Ensure you only skip when symlink creation fails (do not swallow
other errors), and keep targetPath, linkPath and tmpDir usage unchanged so the
rest of the test still asserts that the target file content is untouched by
sanitizeConfigFile.
In `@src/lib/security/credential-filter.ts`:
- Around line 30-43: The readRegularFileNoFollow function currently falls back
to bitwise OR with (constants.O_NOFOLLOW ?? 0) which silently follows symlinks
on platforms where O_NOFOLLOW is undefined; instead, detect the absence of
constants.O_NOFOLLOW and fail explicitly or implement a platform-specific guard:
inside readRegularFileNoFollow check if constants.O_NOFOLLOW is undefined and
either throw/log an error indicating symlink protection is unavailable, or
perform an lstatSync(filePath) prior to openSync to ensure the path is not a
symlink (use fs.lstatSync(...).isSymbolicLink()) before proceeding with
openSync/readFileSync; reference the constants.O_NOFOLLOW symbol and the
readRegularFileNoFollow function when making the change.
---
Nitpick comments:
In `@nemoclaw/src/blueprint/state.test.ts`:
- Around line 179-183: Rename and update the test that currently reads "does
nothing when no file exists" to reflect the new behavior of clearState: change
the description to "creates blank state when no file exists" and add an
assertion that the state file is created (or that the atomic write path was
invoked) after calling clearState; target the test in state.test.ts that calls
clearState() so the test both documents and verifies the new unconditional
blank-state write behavior.
In `@src/lib/security/credential-filter.ts`:
- Around line 45-50: The writeFileAtomically function has a redundant chmodSync
call after renameSync that creates a small TOCTOU window; remove the final
chmodSync(filePath, 0o600) line and rely on writeFileSync(tmpPath, contents, {
mode: 0o600 }) plus the atomic renameSync(tmpPath, filePath) to ensure the
destination file has correct permissions, leaving tmpPath creation,
writeFileSync and renameSync unchanged.
🪄 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: Enterprise
Run ID: 195b6fa8-2539-4baa-a494-8e5a0c02565b
📒 Files selected for processing (15)
nemoclaw/src/blueprint/state.test.tsnemoclaw/src/blueprint/state.tssrc/lib/actions/sandbox/gateway-state.tssrc/lib/actions/uninstall/plan.tssrc/lib/actions/uninstall/run-plan.tssrc/lib/adapters/http/probe.test.tssrc/lib/adapters/http/probe.tssrc/lib/credentials/store.tssrc/lib/inference/ollama/proxy.tssrc/lib/runner-argv.test.tssrc/lib/runner.tssrc/lib/security/credential-filter.test.tssrc/lib/security/credential-filter.tssrc/lib/state/onboard-session.test.tssrc/lib/state/onboard-session.ts
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 27046738496
|
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/lib/actions/uninstall/plan.ts (1)
62-65:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGate the descriptor-read path on
O_NOFOLLOWsupport.Node documents POSIX open flags as platform-dependent and recommends checking for their presence;
O_NOFOLLOWis not available on Windows. If this CLI still runs there, thisopenSync()call degrades to a normal open, so the classifier reads through the link instead of taking thelstatSync()fallback. The downstream effect here is misclassification: a shim symlink can look like a regular file and be preserved instead of removed. (nodejs.org)Proposed fix
export function classifyShimPath(shimPath: string, deps: FileSystemDeps = {}): ShimClassification { const lstatSync = deps.lstatSync ?? fs.lstatSync; const openSync = deps.openSync ?? fs.openSync; const fstatSync = deps.fstatSync ?? fs.fstatSync; const readFileSync = deps.readFileSync ?? fs.readFileSync; const closeSync = deps.closeSync ?? fs.closeSync; + + if (typeof fs.constants.O_NOFOLLOW !== "number") { + return classifyShimPathByMetadata(shimPath, lstatSync); + } + try { const fd = openSync( shimPath, - fs.constants.O_RDONLY | fs.constants.O_NOFOLLOW | fs.constants.O_NONBLOCK, + fs.constants.O_RDONLY | fs.constants.O_NOFOLLOW | (fs.constants.O_NONBLOCK ?? 0), );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/actions/uninstall/plan.ts` around lines 62 - 65, The openSync call using fs.constants.O_NOFOLLOW is unsafe on platforms that don't support O_NOFOLLOW (e.g., Windows) so the code path that reads via descriptor can silently follow symlinks; instead detect support for O_NOFOLLOW before using that path: check for the presence of fs.constants.O_NOFOLLOW (or similar) and only build the flags and call openSync(shimPath, flags) when it's defined; if not defined, skip the descriptor-read branch that uses openSync and fall back to the existing lstatSync-based classifier logic (reference shimPath, openSync, fd, O_NOFOLLOW, and lstatSync in your changes).
🧹 Nitpick comments (1)
src/lib/actions/uninstall/run-plan.test.ts (1)
39-60: ⚡ Quick winAssert that the
ELOOPfallback is the branch under test.Right now this still passes if
buildRunPlan()regresses to metadata-only shim classification, because the end result is the same. MakeopenSyncavi.fn()and assert it was called so this test locks in the new descriptor-based fallback path.Proposed tweak
it("builds a plan using host paths and shim classification", () => { + const openSync = vi.fn(() => { + const error = new Error("symlink") as NodeJS.ErrnoException; + error.code = "ELOOP"; + throw error; + }); + const { paths, plan } = buildRunPlan( { assumeYes: true, deleteModels: false, keepOpenShell: false }, { env: { HOME: "/home/test", TMPDIR: "/tmp/test" } as NodeJS.ProcessEnv, fs: { lstatSync: (() => ({ isFile: () => false, isSymbolicLink: () => true })) as never, - openSync: (() => { - const error = new Error("symlink") as NodeJS.ErrnoException; - error.code = "ELOOP"; - throw error; - }) as never, + openSync: openSync as never, }, }, ); + expect(openSync).toHaveBeenCalled(); expect(paths.nemoclawShimPath).toBe("/home/test/.local/bin/nemoclaw");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/actions/uninstall/run-plan.test.ts` around lines 39 - 60, The test for buildRunPlan should ensure the ELOOP fallback path is exercised by making the fs.openSync used in buildRunPlan a spy and asserting it was called; replace the inline throwing openSync stub with a vi.fn() that throws the ELOOP ErrnoException and then add an expectation that that spy was invoked (e.g., expect(openSyncSpy).toHaveBeenCalled()), while keeping the lstatSync stub that reports a symlink and the existing assertions for nemoclawShimPath and the delete-shim action so the test locks in the descriptor-based fallback branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/lib/actions/uninstall/plan.ts`:
- Around line 62-65: The openSync call using fs.constants.O_NOFOLLOW is unsafe
on platforms that don't support O_NOFOLLOW (e.g., Windows) so the code path that
reads via descriptor can silently follow symlinks; instead detect support for
O_NOFOLLOW before using that path: check for the presence of
fs.constants.O_NOFOLLOW (or similar) and only build the flags and call
openSync(shimPath, flags) when it's defined; if not defined, skip the
descriptor-read branch that uses openSync and fall back to the existing
lstatSync-based classifier logic (reference shimPath, openSync, fd, O_NOFOLLOW,
and lstatSync in your changes).
---
Nitpick comments:
In `@src/lib/actions/uninstall/run-plan.test.ts`:
- Around line 39-60: The test for buildRunPlan should ensure the ELOOP fallback
path is exercised by making the fs.openSync used in buildRunPlan a spy and
asserting it was called; replace the inline throwing openSync stub with a
vi.fn() that throws the ELOOP ErrnoException and then add an expectation that
that spy was invoked (e.g., expect(openSyncSpy).toHaveBeenCalled()), while
keeping the lstatSync stub that reports a symlink and the existing assertions
for nemoclawShimPath and the delete-shim action so the test locks in the
descriptor-based fallback branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ffd670b1-3ab4-42ba-b2e5-ca009dc9557b
📒 Files selected for processing (4)
src/lib/actions/uninstall/plan.tssrc/lib/actions/uninstall/run-plan.test.tssrc/lib/actions/uninstall/run-plan.tssrc/lib/adapters/http/probe.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/actions/uninstall/run-plan.ts
- src/lib/adapters/http/probe.ts
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 27047329923
|
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/adapters/http/probe.ts (1)
243-253:⚠️ Potential issue | 🟠 MajorPrepend
-qinbuildCurlProbeSpawnArgs()to prevent implicit per-user curl config from bypassing validation
buildCurlProbeSpawnArgs()currently placesargs(validated) before any curl flags, but curl will still load the default per-user config file (e.g.,~/.curlrc) at startup unless-q/--disableis the first argument. That config can reintroduce request-affecting options (like--config/-K,--data @..., or default headers), undermining thetrustedConfigFilesboundary hardening. Prepend-qin the shared argv builder so every probe mode disables implicit config loading.Suggested fix
function buildCurlProbeSpawnArgs( args: string[], url: string, bodyFile: string, mode: CurlProbeMode, ): string[] { const outputArgs = mode === "json" ? ["-o", bodyFile, "-w", "%{http_code}"] : ["-N", "-o", bodyFile]; const statusArgs = mode === "chat-stream" ? ["-w", "%{http_code}"] : []; - // lgtm[js/file-access-to-http] URL/argv are validated; file-backed config paths must be explicitly trusted. - return [...args, ...outputArgs, ...statusArgs, url]; + // `-q` must be first so curl ignores implicit per-user config files. + // lgtm[js/file-access-to-http] URL/argv are validated; file-backed config paths must be explicitly trusted. + return ["-q", ...args, ...outputArgs, ...statusArgs, url]; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/adapters/http/probe.ts` around lines 243 - 253, The buildCurlProbeSpawnArgs function must ensure curl's per-user config is disabled by prepending the "-q"/--disable flag as the very first argv element; modify buildCurlProbeSpawnArgs so it returns an array that begins with "-q" followed by the validated args, then outputArgs, statusArgs, and url (i.e., ["-q", ...args, ...outputArgs, ...statusArgs, url]) to guarantee user ~/.curlrc cannot override probe options.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/lib/adapters/http/probe.ts`:
- Around line 243-253: The buildCurlProbeSpawnArgs function must ensure curl's
per-user config is disabled by prepending the "-q"/--disable flag as the very
first argv element; modify buildCurlProbeSpawnArgs so it returns an array that
begins with "-q" followed by the validated args, then outputArgs, statusArgs,
and url (i.e., ["-q", ...args, ...outputArgs, ...statusArgs, url]) to guarantee
user ~/.curlrc cannot override probe options.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: bb128ba7-039b-4c75-aeea-8f182e73a797
📒 Files selected for processing (10)
nemoclaw/src/blueprint/state.test.tssrc/lib/actions/uninstall/plan.tssrc/lib/adapters/http/probe.test.tssrc/lib/adapters/http/probe.tssrc/lib/credentials/store.tssrc/lib/inference/health.tssrc/lib/messaging/channels/slack/hooks/credential-validation.test.tssrc/lib/messaging/channels/slack/hooks/credential-validation.tssrc/lib/security/credential-filter.test.tssrc/lib/security/credential-filter.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/adapters/http/probe.test.ts
- src/lib/actions/uninstall/plan.ts
- src/lib/security/credential-filter.ts
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 27048339630
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
nemoclaw/src/blueprint/state.test.ts (1)
190-197: ⚡ Quick winConsider more explicit assertions for clarity.
The optional chaining (
write?.path,rename?.from) is defensive, but the test could be clearer by assertingwriteandrenameare defined before accessing their properties.♻️ Optional: More explicit assertions
+ expect(write).toBeDefined(); + expect(rename).toBeDefined(); + expect(write!.path.startsWith(`${STATE_PATH}.${process.pid}.`)).toBe(true); + expect(write!.path.endsWith(".tmp")).toBe(true); + expect(write!.options).toMatchObject({ mode: 0o600 }); + expect(rename).toEqual({ from: write!.path, to: STATE_PATH }); + expect(store.has(write!.path)).toBe(false); - const write = writes.at(-1); - const rename = renames.at(-1); - expect(write?.path.startsWith(`${STATE_PATH}.${process.pid}.`)).toBe(true); - expect(write?.path.endsWith(".tmp")).toBe(true); - expect(write?.options).toMatchObject({ mode: 0o600 }); - expect(rename).toEqual({ from: write?.path, to: STATE_PATH }); - expect(store.has(write?.path || "")).toBe(false);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nemoclaw/src/blueprint/state.test.ts` around lines 190 - 197, The test uses optional chaining on write and rename (from writes.at(-1) and renames.at(-1)) which hides potential undefined values; add explicit assertions that write and rename are defined before accessing their properties (e.g., expect(write).toBeDefined(); expect(rename).toBeDefined()) and then replace or follow the subsequent expectations that reference write?.path, rename?.from, and store.get(STATE_PATH) with non-optional accesses to make failures clearer; keep existing assertions about STATE_PATH, file mode and JSON lastAction intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@nemoclaw/src/blueprint/state.test.ts`:
- Around line 190-197: The test uses optional chaining on write and rename (from
writes.at(-1) and renames.at(-1)) which hides potential undefined values; add
explicit assertions that write and rename are defined before accessing their
properties (e.g., expect(write).toBeDefined(); expect(rename).toBeDefined()) and
then replace or follow the subsequent expectations that reference write?.path,
rename?.from, and store.get(STATE_PATH) with non-optional accesses to make
failures clearer; keep existing assertions about STATE_PATH, file mode and JSON
lastAction intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: cb7eb27d-a628-4741-b800-973f5a0dd51a
📒 Files selected for processing (3)
nemoclaw/src/blueprint/state.test.tssrc/lib/adapters/http/probe.test.tssrc/lib/adapters/http/probe.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/adapters/http/probe.ts
- src/lib/adapters/http/probe.test.ts
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 27049837542
|
12 tasks
cv
added a commit
that referenced
this pull request
Jun 6, 2026
## Summary Extracts the curl probe validation and spawn-argument construction introduced in #4878 into a dedicated helper module, then reuses that helper from additional host/sandbox probe call sites. This keeps the SSRF/file-read/multi-transfer policy behind a smaller, auditable boundary while preserving existing probe behavior and coverage. ## Related Issue Refs #3654; follow-up to #4878. ## Changes - Added `src/lib/adapters/http/curl-args.ts` for curl probe URL validation, option policy, trusted config handling, and spawn argv construction. - Updated `src/lib/adapters/http/probe.ts` to import the helper instead of carrying the policy inline. - Added `buildValidatedCurlCommandArgs()` for direct curl command callers that need validation without the `runCurlProbe()` output wrapper. - Reused the shared curl policy in host/local probes including wait helpers, doctor Ollama checks, vLLM/NIM readiness, Ollama version/runtime/model-size probes, local Ollama probe command construction, and agent health checks executed through OpenShell. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Verification - [x] `npx prek run --all-files` passes - [ ] `npm test` passes - [ ] Tests added or updated for new or changed behavior - [x] No secrets, API keys, or credentials committed - [ ] Docs updated for user-facing behavior changes - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) `npm test` was attempted, but the existing installer integration test `test/install-preflight.test.ts > warns on Podman but still runs onboarding` fails in this environment because host CDI detection turns the Podman warning path into a missing-CDI issue path before onboarding. Targeted probe tests and `npm run typecheck:cli` pass. --- Signed-off-by: Carlos Villela <cvillela@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
3 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.
Summary
Harden the P0 code-scanning buckets from #3654 by tightening process spawn boundaries, HTTP/file probe boundaries, and filesystem race-prone IO paths. The changes keep runtime behavior intact while adding validation, atomic writes, descriptor-pinned reads, and regression coverage for the high-priority alert classes.
Related Issue
Refs #3654
Changes
shell: falsespawn boundaries and reject NUL bytes before spawning.command -vcheck with direct PATH probing.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesnpm run docsbuilds without warnings (doc changes only)Note:
npm testwas attempted, but the existing installer integration testtest/install-preflight.test.ts > warns on Podman but still runs onboardingfails in this environment because host CDI detection turns the Podman warning path into a missing-CDI issue path before onboarding. Targeted tests andnpx prek run --all-filespass.Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
New Features
Bug Fixes
Tests