Skip to content

refactor(security): extract curl probe policy#4885

Merged
cv merged 3 commits into
mainfrom
fix/security-curl-probe-helper
Jun 6, 2026
Merged

refactor(security): extract curl probe policy#4885
cv merged 3 commits into
mainfrom
fix/security-curl-probe-helper

Conversation

@cv

@cv cv commented Jun 6, 2026

Copy link
Copy Markdown
Collaborator

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

  • 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

  • npx prek run --all-files passes
  • npm test passes
  • Tests added or updated for new or changed behavior
  • 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 (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

@cv cv added the security Potential vulnerability, unsafe behavior, or access risk label Jun 6, 2026
@cv cv self-assigned this Jun 6, 2026
@coderabbitai

coderabbitai Bot commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@cv, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 31 minutes and 35 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 5875c8c5-97cc-4125-bc9f-5bcafd569a32

📥 Commits

Reviewing files that changed from the base of the PR and between 1f0b4c5 and 35a9fed.

📒 Files selected for processing (11)
  • src/lib/actions/sandbox/doctor.ts
  • src/lib/adapters/http/curl-args.ts
  • src/lib/adapters/http/probe.ts
  • src/lib/agent/onboard.ts
  • src/lib/core/wait.ts
  • src/lib/inference/local.ts
  • src/lib/inference/nim.ts
  • src/lib/inference/ollama-runtime-context.ts
  • src/lib/inference/ollama-version.ts
  • src/lib/inference/ollama/model-size.ts
  • src/lib/inference/vllm.ts
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/security-curl-probe-helper

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions

github-actions Bot commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor

Findings: 0 needs attention, 1 worth checking, 0 nice ideas
Since last review: 1 prior item resolved, 0 still apply, 1 new item found

Review findings

🛠️ Needs attention

  • None.

🔎 Worth checking

  • Scope curl redirect support away from authenticated probes (src/lib/adapters/http/curl-args.ts:44): The new shared curl policy treats -L, -sfL, and --location as generally safe flags. That appears intended for the unauthenticated Ollama registry-size probe, but it also relaxes every current and future runCurlProbe caller. If a caller sends an Authorization or Proxy-Authorization header to a user-controlled endpoint and also enables --location, curl may follow redirects to another host and forward sensitive headers, widening the SSRF/credential-leak blast radius.
    • Recommendation: Keep redirect-following out of the generic safe allowlist, or add a narrower redirect mode that is only used by the registry manifest probe. If redirects must remain generally available, reject them whenever sensitive headers are present and add tests for that invariant.
    • Evidence: curl-args.ts adds "-L", "-sfL", and "--location" to CURL_SAFE_FLAG_OPTIONS, while runCurlProbe callers in inference/onboard-probes.ts can include Authorization headers for provider validation. The only changed call site that needs -sfL is probeRegistrySize(), which sends only an Accept header to registry.ollama.ai.

🌱 Nice ideas

  • None.
Consider writing more tests for
  • **Runtime validation** — validateCurlProbeArgs rejects --location when an Authorization or Proxy-Authorization header is present. The change touches runtime/sandbox/inference network probes. Unit coverage exists for the public probe wrappers, but the newly shared helper now affects direct curl callers in doctor, onboard, wait, local inference, NIM, Ollama, and vLLM paths.
  • **Runtime validation** — probeRegistrySize follows registry redirects only through a narrow unauthenticated manifest-fetch path. The change touches runtime/sandbox/inference network probes. Unit coverage exists for the public probe wrappers, but the newly shared helper now affects direct curl callers in doctor, onboard, wait, local inference, NIM, Ollama, and vLLM paths.
  • **Runtime validation** — buildValidatedCurlCommandArgs rejects file-backed --data and --header inputs before direct host-probe callers invoke spawn or runCapture. The change touches runtime/sandbox/inference network probes. Unit coverage exists for the public probe wrappers, but the newly shared helper now affects direct curl callers in doctor, onboard, wait, local inference, NIM, Ollama, and vLLM paths.
  • **Runtime validation** — handleAgentSetup handles an invalid manifest health_probe URL through the curl policy without leaking credentials or crashing outside the setup failure path. The change touches runtime/sandbox/inference network probes. Unit coverage exists for the public probe wrappers, but the newly shared helper now affects direct curl callers in doctor, onboard, wait, local inference, NIM, Ollama, and vLLM paths.
  • **Runtime validation** — Runtime validation confirms the converted loopback health checks still reach Ollama, NIM, vLLM, and agent dashboard endpoints using argv-array curl execution. The change touches runtime/sandbox/inference network probes. Unit coverage exists for the public probe wrappers, but the newly shared helper now affects direct curl callers in doctor, onboard, wait, local inference, NIM, Ollama, and vLLM paths.
  • **Acceptance clause:** This keeps the SSRF/file-read/multi-transfer policy behind a smaller, auditable boundary while preserving existing probe behavior and coverage. — add test evidence or identify existing coverage. The file-read, config-file, URL-scheme, --url, and --next policy checks are centralized in curl-args.ts and existing probe wrapper tests still cover those public behaviors. However, the extracted shared policy also newly allows redirect flags globally, which is broader than the unauthenticated registry-size use case.
  • **Acceptance clause:** Refs chore(security): triage code scanning backlog by risk/reward #3654; follow-up to fix(security): harden P0 code scanning boundaries #4878. — add test evidence or identify existing coverage. No trusted linked issue text or comments were provided in the deterministic context, so issue-specific acceptance clauses could not be mapped.
Since last review details

Current findings:

  • Scope curl redirect support away from authenticated probes (src/lib/adapters/http/curl-args.ts:44): The new shared curl policy treats -L, -sfL, and --location as generally safe flags. That appears intended for the unauthenticated Ollama registry-size probe, but it also relaxes every current and future runCurlProbe caller. If a caller sends an Authorization or Proxy-Authorization header to a user-controlled endpoint and also enables --location, curl may follow redirects to another host and forward sensitive headers, widening the SSRF/credential-leak blast radius.
    • Recommendation: Keep redirect-following out of the generic safe allowlist, or add a narrower redirect mode that is only used by the registry manifest probe. If redirects must remain generally available, reject them whenever sensitive headers are present and add tests for that invariant.
    • Evidence: curl-args.ts adds "-L", "-sfL", and "--location" to CURL_SAFE_FLAG_OPTIONS, while runCurlProbe callers in inference/onboard-probes.ts can include Authorization headers for provider validation. The only changed call site that needs -sfL is probeRegistrySize(), which sends only an Accept header to registry.ollama.ai.

Workflow run details

This is an automated advisory review. A human maintainer must make the final merge decision.

@github-actions

github-actions Bot commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

E2E Advisor Recommendation

Required E2E: inference-routing-e2e, cloud-onboard-e2e, hermes-e2e, gpu-e2e, onboard-inference-smoke-e2e, strict-tool-call-probe-e2e
Optional E2E: cloud-inference-e2e, model-router-provider-routed-inference-e2e, ollama-proxy-e2e, diagnostics-e2e, openclaw-inference-switch-e2e

Auto-dispatched E2E: inference-routing-e2e, cloud-onboard-e2e, hermes-e2e, gpu-e2e via nightly-e2e.yaml at 35a9fed35314911feee091d831333221f14dc299nightly run

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E

  • inference-routing-e2e (medium): Required because HTTP probe behavior and inference routing security changed. This validates gateway inference routing, credential isolation, and error classification through real CLI/sandbox flows.
  • cloud-onboard-e2e (medium): Required because onboarding and provider health checks are affected. This runs the real non-interactive cloud onboarding path and catches regressions in setup, health probing, and initial route configuration.
  • hermes-e2e (medium): Required because src/lib/agent/onboard.ts is the non-OpenClaw agent setup path. Hermes E2E validates agent-specific onboarding, health probing, and live inference after setup.
  • gpu-e2e (high): Required because local Ollama provider readiness, model probing, daemon/version/context helpers, and sandbox local-inference routing are touched. This is the existing real local Ollama GPU user-flow coverage.
  • onboard-inference-smoke-e2e (low): Required as a fast regression guard for onboarding accepting a configured-but-broken inference route. The PR changes HTTP probe plumbing used by setupInference diagnostics and failures.
  • strict-tool-call-probe-e2e (low): Required because local Ollama onboarding validation depends on HTTP probe payload/argument behavior. This hermetic E2E validates the strict Chat Completions tool-call probe without requiring a GPU.

Optional E2E

  • cloud-inference-e2e (medium): Useful adjacent confidence for real cloud inference after the HTTP probe refactor, but cloud-onboard and inference-routing are the more direct merge-blocking checks.
  • model-router-provider-routed-inference-e2e (medium): Useful because provider-routed inference also depends on probe and routing behavior, especially if the PR is suspected to affect model-router-compatible endpoint paths.
  • ollama-proxy-e2e (medium): Useful adjacent local Ollama confidence for the auth proxy chain and real CPU Ollama inference, though the changed files mostly affect callers around the proxy rather than the proxy script itself.
  • diagnostics-e2e (medium): Adjacent coverage for diagnostics/status workflows after a doctor.ts change, but it does not appear to directly assert the doctor Ollama local-service check changed in this PR.
  • openclaw-inference-switch-e2e (medium): Useful to validate a running OpenClaw sandbox can switch inference routes after the probe refactor, but less direct than onboarding and routing tests for this diff.

New E2E recommendations

  • sandbox-diagnostics-doctor (high): Existing E2E coverage only incidentally mentions doctor for messaging; there is no direct E2E assertion that nemoclaw <sandbox> doctor --json reports the Ollama local-service/provider-health checks correctly after validated curl argument changes.
    • Suggested test: Add a doctor local-inference diagnostics E2E that creates or mocks an Ollama-selected sandbox, runs nemoclaw <sandbox> doctor --json, and asserts the Local services/Ollama and Inference provider-health checks report expected ok/fail details without leaking credentials.
  • curl-probe-security (high): The new curl validator protects a security boundary, but existing E2E jobs mostly validate happy-path probes. A regression could permit file-backed headers/data/configs or reject safe generated probes only in real CLI paths.
    • Suggested test: Add an E2E or regression E2E that drives a CLI/provider probe path with unsafe curl-style file references and asserts NemoClaw rejects them before spawning curl, while a normal Authorization/header/data probe still succeeds.
  • vllm-nim-local-provider-health (medium): Changed vLLM and NIM readiness/status probes are not directly represented by an existing named E2E job; current local-provider E2E emphasis is Ollama.
    • Suggested test: Add a local vLLM/NIM provider health smoke E2E with a lightweight mocked or fixture OpenAI-compatible /v1/models endpoint that validates readiness/status probes and served-model adoption behavior.

@github-actions

github-actions Bot commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

E2E Scenario Advisor Recommendation

Required scenario E2E: ubuntu-repo-cloud-openclaw, ubuntu-repo-cloud-hermes, gpu-repo-local-ollama-openclaw
Optional scenario E2E: ubuntu-repo-openai-compatible-openclaw, wsl-repo-cloud-openclaw

Dispatch required scenario E2E:

  • gh workflow run e2e-scenarios.yaml --ref <pr-head-ref> --field scenarios=ubuntu-repo-cloud-openclaw
  • gh workflow run e2e-scenarios.yaml --ref <pr-head-ref> --field scenarios=ubuntu-repo-cloud-hermes
  • gh workflow run e2e-scenarios.yaml --ref <pr-head-ref> --field scenarios=gpu-repo-local-ollama-openclaw

Workflow run

Full scenario advisor summary

E2E Scenario Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required scenario E2E

  • ubuntu-repo-cloud-openclaw: Primary Ubuntu repo scenario exercises the standard OpenClaw cloud onboarding path, gateway/sandbox smoke checks, and cloud inference probes affected by the new shared curl argument validation and changes in HTTP probe/wait/inference helpers.
    • Dispatch: gh workflow run e2e-scenarios.yaml --ref <pr-head-ref> --field scenarios=ubuntu-repo-cloud-openclaw
  • ubuntu-repo-cloud-hermes: The PR changes src/lib/agent/onboard.ts health-probe execution for non-OpenClaw agents; the Hermes scenario is the dispatchable scenario that exercises agent-specific onboarding plus Hermes health validation.
    • Dispatch: gh workflow run e2e-scenarios.yaml --ref <pr-head-ref> --field scenarios=ubuntu-repo-cloud-hermes
  • gpu-repo-local-ollama-openclaw: The PR changes local Ollama health/probe/runtime-context/version/model-size paths and shared curl validation. This GPU scenario is the only routed scenario that exercises local Ollama onboarding, local Ollama inference, and the Ollama auth proxy.
    • Dispatch: gh workflow run e2e-scenarios.yaml --ref <pr-head-ref> --field scenarios=gpu-repo-local-ollama-openclaw

Optional scenario E2E

  • ubuntu-repo-openai-compatible-openclaw: Adjacent provider-onboarding coverage for non-NVIDIA OpenAI-compatible configuration; useful because the shared HTTP/curl validation touches provider probing, but the scenario only runs smoke coverage so it is secondary to the primary cloud and local-Ollama targets.
    • Dispatch: gh workflow run e2e-scenarios.yaml --ref <pr-head-ref> --field scenarios=ubuntu-repo-openai-compatible-openclaw
  • wsl-repo-cloud-openclaw: Optional platform-adjacent coverage for the same OpenClaw cloud onboarding path on WSL. Relevant because local inference and host reachability helpers include WSL-sensitive behavior, but it is a special-runner platform scenario and not the primary exercise of this change.
    • Dispatch: gh workflow run e2e-scenarios.yaml --ref <pr-head-ref> --field scenarios=wsl-repo-cloud-openclaw

Relevant changed files

  • src/lib/actions/sandbox/doctor.ts
  • src/lib/adapters/http/curl-args.ts
  • src/lib/adapters/http/probe.ts
  • src/lib/agent/onboard.ts
  • src/lib/core/wait.ts
  • src/lib/inference/local.ts
  • src/lib/inference/nim.ts
  • src/lib/inference/ollama-runtime-context.ts
  • src/lib/inference/ollama-version.ts
  • src/lib/inference/ollama/model-size.ts
  • src/lib/inference/vllm.ts

@github-actions

github-actions Bot commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

Selective E2E Results — ✅ All requested jobs passed

Run: 27050466714
Target ref: 134d701e1c9114760f7207b867080f01c8e7cd58
Workflow ref: main
Requested jobs: inference-routing-e2e
Summary: 1 passed, 0 failed, 0 skipped

Job Result
inference-routing-e2e ✅ success

@github-actions

github-actions Bot commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

Selective E2E Results — ✅ All requested jobs passed

Run: 27051116486
Target ref: 35a9fed35314911feee091d831333221f14dc299
Workflow ref: main
Requested jobs: inference-routing-e2e,cloud-onboard-e2e,hermes-e2e,gpu-e2e
Summary: 3 passed, 0 failed, 1 skipped

Job Result
cloud-onboard-e2e ✅ success
gpu-e2e ⏭️ skipped
hermes-e2e ✅ success
inference-routing-e2e ✅ success

@cv cv merged commit 7e07724 into main Jun 6, 2026
32 checks passed
@cv cv deleted the fix/security-curl-probe-helper branch June 6, 2026 04:28
@cv cv added the v0.0.61 Release target label Jun 7, 2026
@wscurran wscurran added the refactor PR restructures code without intended behavior change label Jun 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

refactor PR restructures code without intended behavior change security Potential vulnerability, unsafe behavior, or access risk v0.0.61 Release target

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants