fix(security): fail closed for DNS-backed HTTPS endpoints

[jyaunches]

Summary

This PR fixes #4684 by taking the conservative path requested in the PR #4685 security review: DNS-backed HTTPS endpoints now fail closed before NemoClaw hands them to a downstream OpenShell/provider runtime.

Credit to @1PoPTRoN for the original #4685 work that identified and covered the HTTPS DNS-rebinding gap. This PR intentionally preserves the narrow safety outcome while avoiding the contested host-loopback pin-proxy transport until a runtime-aware design is ready.

What changed

• Adds structured endpoint validation metadata in nemoclaw/src/blueprint/ssrf.ts.
• Introduces safeEndpointUrlForDownstream():
  • HTTP DNS-backed endpoints still use the DNS-pinned IP URL.
  • HTTPS IP-literal endpoints remain allowed.
  • DNS-backed HTTPS endpoints fail closed with an actionable error.
• Applies the same fail-closed policy to generic persisted sandbox config URL rewrites.
• Adds unit coverage for blueprint plan/apply and config-set behavior.
• Adds a PR-safe live E2E slice in inference-routing.test.ts proving DNS-backed HTTPS blueprint apply fails before any OpenShell handoff.

Why this shape

PR #4685's host-local HTTPS pin proxy still had unresolved security/design concerns:

• provider traffic may originate outside the NemoClaw host loopback namespace;
• broadening a plaintext unauthenticated proxy would create credential-boundary risk;
• redirects and credential-bearing URL state need a separate policy/design;
• the long-lived helper environment and commit provenance needed cleanup.

This PR therefore fixes the bug by failing closed instead of introducing a partial transport. A follow-up can add the runtime-aware HTTPS pinning transport with real OpenShell/Docker E2E coverage.

Test plan

• npm --prefix nemoclaw test -- --run src/blueprint/ssrf.test.ts src/blueprint/runner.test.ts
• npx vitest run --project integration test/config-set.test.ts --silent=false --reporter=default
• NEMOCLAW_RUN_LIVE_E2E=1 npx vitest run --project e2e-live test/e2e/live/inference-routing.test.ts -t "TC-INF-10" --silent=false --reporter=default
• npm run build:cli
• npm --prefix nemoclaw run build
• npm run typecheck:cli

Fixes #4684

Summary by CodeRabbit

• Bug Fixes
  • Implemented fail-closed validation for DNS-backed HTTPS endpoints: they’re now rejected during endpoint validation, including when rewriting config URLs, preventing downstream runtime handoff.
  • Made endpoint URL handling consistent across overrides and blueprint resolution by applying the same downstream-safe normalization.
  • Enhanced endpoint validation feedback to include protocol/hostname and DNS-resolution details for consistent behavior across flows.
• Tests
  • Strengthened unit and E2E coverage to assert DNS-backed HTTPS rejection and verify that no handoff command is executed on failure.

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Extends endpoint validation metadata, makes downstream URL selection fail closed for DNS-backed HTTPS endpoints, and updates runner, sandbox config, unit, and live tests to match the new behavior.

Changes

DNS-backed HTTPS fail-closed enforcement

Layer / File(s)	Summary
ValidatedEndpoint contract and fail-closed logic nemoclaw/src/blueprint/ssrf.ts, nemoclaw/src/blueprint/ssrf.test.ts	ValidatedEndpoint gains protocol, dnsResolved, resolvedAddress, and resolvedFamily; validateEndpointUrl populates them; safeEndpointUrlForDownstream throws for DNS-backed HTTPS endpoints, with tests covering the richer metadata and URL edge cases.
Blueprint runner uses downstream-safe endpoints nemoclaw/src/blueprint/runner.ts, nemoclaw/src/blueprint/runner.test.ts, test/e2e/live/inference-routing.test.ts, package.json	resolveRunConfig now routes endpoint selection through safeEndpointUrlForDownstream; runner tests update endpoint-override behavior and DNS-backed HTTPS rejection; the live apply test adds a fake OpenShell helper and checks fail-closed execution; execa is added for the new test setup.
Sandbox config rewrite rejects DNS-backed HTTPS URLs src/lib/sandbox/config.ts, test/config-set.test.ts, src/lib/actions/inference-set-endpoint-security.test.ts	rewriteConfigUrlsWithDnsPinning now rejects DNS-backed HTTPS hostname rewrites; tests cover rejection, HTTPS IP-literal passthrough, recursive rewrite expectations, and custom endpoint normalization rejection.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related issues

• #4684: Directly matches the DNS-backed HTTPS fail-closed behavior implemented in SSRF validation and downstream endpoint selection.
• NVIDIA/NemoClaw#6141: Related to HTTPS pinning transport handling; this change implements the conservative fail-closed behavior for DNS-backed HTTPS endpoints.

Possibly related PRs

• NVIDIA/NemoClaw#898: Overlaps the same SSRF validation surface in nemoclaw/src/blueprint/ssrf.ts and related tests.

Suggested reviewers: ericksoa

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main security change in the PR.
Linked Issues check	✅ Passed	Runner, SSRF, config rewrites, and tests all implement fail-closed handling for DNS-backed HTTPS endpoints as required by #4684.
Out of Scope Changes check	✅ Passed	No unrelated changes stand out; the dependency and test additions support the HTTPS fail-closed security fix.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

nemoclaw/src/blueprint/runner.test.ts (1)

574-574: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Redundant mock-call assertions alongside observable outcomes.

toHaveBeenCalledWith on mockedValidateEndpoint locks in an implementation detail (how the endpoint is validated) on top of the already-sufficient observable assertion (plan.inference.endpoint / prior expectation). Per path instructions for test files, prefer relying on observable outcomes through the public boundary over mock-call assertions.

Not blocking — these are additive, not the sole assertion.

Also applies to: 1116-1116

🤖 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/runner.test.ts` at line 574, The test in
runner.test.ts is asserting the internal call to mockedValidateEndpoint, which
duplicates implementation details beyond the observable result. Remove the
toHaveBeenCalledWith expectation in the relevant test cases and keep the
assertions focused on the public outcome, such as plan.inference.endpoint and
any existing observable checks, using runner/plan-related symbols to locate the
affected tests.

Source: Path instructions

🤖 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/runner.test.ts`:
- Line 574: The test in runner.test.ts is asserting the internal call to
mockedValidateEndpoint, which duplicates implementation details beyond the
observable result. Remove the toHaveBeenCalledWith expectation in the relevant
test cases and keep the assertions focused on the public outcome, such as
plan.inference.endpoint and any existing observable checks, using
runner/plan-related symbols to locate the affected tests.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 34f5682e-810d-40a8-90f3-297c7e2cf665

📥 Commits

Reviewing files that changed from the base of the PR and between f6fac71 and 6fe66b2.

📒 Files selected for processing (7)

• nemoclaw/src/blueprint/runner.test.ts
• nemoclaw/src/blueprint/runner.ts
• nemoclaw/src/blueprint/ssrf.test.ts
• nemoclaw/src/blueprint/ssrf.ts
• src/lib/sandbox/config.ts
• test/config-set.test.ts
• test/e2e/live/inference-routing.test.ts

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[coderabbitai]

🧹 Nitpick comments (2)

nemoclaw/src/blueprint/runner.test.ts (2)

566-566: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Coverage gap: actionPlan no longer tests DNS-backed HTTPS override fail-closed behavior.

The diff removes the actionPlan test that verified endpointUrl override handling (previously asserting plan.inference.endpoint and the validateEndpointUrl call), and no replacement was added. Since resolveRunConfig/safeEndpointUrlForDownstream is the shared enforcement point for the fail-closed policy (per nemoclaw/src/blueprint/runner.ts:416-446), this file now only proves the fail-closed behavior for actionApply, not for actionPlan. If actionPlan reaches the same resolveRunConfig path, a DNS-backed HTTPS override would presumably also reject there, but that's currently unverified.

Consider adding back an actionPlan test asserting rejection for DNS-backed HTTPS overrides (mirroring the new actionApply test), to keep both entrypoints covered for this security-critical fail-closed contract.

Also applies to: 1088-1104

🤖 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/runner.test.ts` at line 566, Restore coverage for the
shared fail-closed endpoint enforcement by adding an actionPlan test that
mirrors the actionApply DNS-backed HTTPS override rejection, using the
runner.test.ts actionPlan case and the
resolveRunConfig/safeEndpointUrlForDownstream path to assert the override is
rejected and validateEndpointUrl behavior is exercised. Make sure the new test
specifically covers the DNS-backed HTTPS override scenario so actionPlan is
verified independently, not just actionApply.

---

1088-1104: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Consider asserting no side effects at all, not just no provider execa call.

resolveRunConfig throws before sandbox creation even begins (per nemoclaw/src/blueprint/runner.ts:685-835), so a stronger assertion would additionally confirm no runCmd-based sandbox creation or state-file writes occurred, not only that execa wasn't called with "provider". This would more fully validate the "fails closed before any side effect" claim in the test name.

🤖 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/runner.test.ts` around lines 1088 - 1104, Strengthen
the “fails closed” test in runner.test by asserting no side effects occur before
provider creation: in addition to the existing execa check for "provider",
verify that resolveRunConfig rejects before any sandbox setup or state
persistence happens. Use the existing actionApply, mockedValidateEndpoint, and
mockExeca hooks to assert no runCmd-based sandbox creation and no state-file
writes are triggered when the DNS-backed HTTPS override is rejected.

🤖 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/runner.test.ts`:
- Line 566: Restore coverage for the shared fail-closed endpoint enforcement by
adding an actionPlan test that mirrors the actionApply DNS-backed HTTPS override
rejection, using the runner.test.ts actionPlan case and the
resolveRunConfig/safeEndpointUrlForDownstream path to assert the override is
rejected and validateEndpointUrl behavior is exercised. Make sure the new test
specifically covers the DNS-backed HTTPS override scenario so actionPlan is
verified independently, not just actionApply.
- Around line 1088-1104: Strengthen the “fails closed” test in runner.test by
asserting no side effects occur before provider creation: in addition to the
existing execa check for "provider", verify that resolveRunConfig rejects before
any sandbox setup or state persistence happens. Use the existing actionApply,
mockedValidateEndpoint, and mockExeca hooks to assert no runCmd-based sandbox
creation and no state-file writes are triggered when the DNS-backed HTTPS
override is rejected.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 15540c4f-2f84-47f9-9814-58e7c7eea245

📥 Commits

Reviewing files that changed from the base of the PR and between 6fe66b2 and 0493433.

📒 Files selected for processing (8)

• nemoclaw/src/blueprint/runner.test.ts
• nemoclaw/src/blueprint/runner.ts
• nemoclaw/src/blueprint/ssrf.test.ts
• nemoclaw/src/blueprint/ssrf.ts
• src/lib/actions/inference-set-endpoint-security.test.ts
• src/lib/sandbox/config.ts
• test/config-set.test.ts
• test/e2e/live/inference-routing.test.ts

🚧 Files skipped from review as they are similar to previous changes (5)

• test/config-set.test.ts
• src/lib/sandbox/config.ts
• nemoclaw/src/blueprint/ssrf.ts
• nemoclaw/src/blueprint/ssrf.test.ts
• nemoclaw/src/blueprint/runner.ts

[coderabbitai]

🧹 Nitpick comments (1)

test/e2e/live/inference-routing.test.ts (1)

712-712: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use literal : instead of path.delimiter for PATH construction.

Based on learnings, this repo's test suite establishes the POSIX PATH separator : for building process.env.PATH in tests rather than path.delimiter, since CI only runs on Linux and Windows is limited to already-POSIX-compliant WSL e2e runs.

♻️ Proposed fix

-          PATH: `${fakeBinDir}${path.delimiter}${process.env.PATH ?? ""}`,
+          PATH: `${fakeBinDir}:${process.env.PATH ?? ""}`,

🤖 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 `@test/e2e/live/inference-routing.test.ts` at line 712, The PATH setup in the
e2e test uses path.delimiter, but this suite should hardcode the POSIX separator
instead. Update the PATH construction in inference-routing.test.ts to use a
literal colon when concatenating fakeBinDir with process.env.PATH, keeping the
rest of the test setup unchanged.

Source: Learnings

🤖 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 `@test/e2e/live/inference-routing.test.ts`:
- Line 712: The PATH setup in the e2e test uses path.delimiter, but this suite
should hardcode the POSIX separator instead. Update the PATH construction in
inference-routing.test.ts to use a literal colon when concatenating fakeBinDir
with process.env.PATH, keeping the rest of the test setup unchanged.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 36142bde-05e6-462c-a96b-5bfa8e768e2f

📥 Commits

Reviewing files that changed from the base of the PR and between 0493433 and 371c2ea.

📒 Files selected for processing (8)

• nemoclaw/src/blueprint/runner.test.ts
• nemoclaw/src/blueprint/runner.ts
• nemoclaw/src/blueprint/ssrf.test.ts
• nemoclaw/src/blueprint/ssrf.ts
• src/lib/actions/inference-set-endpoint-security.test.ts
• src/lib/sandbox/config.ts
• test/config-set.test.ts
• test/e2e/live/inference-routing.test.ts

🚧 Files skipped from review as they are similar to previous changes (7)

• src/lib/actions/inference-set-endpoint-security.test.ts
• nemoclaw/src/blueprint/ssrf.test.ts
• src/lib/sandbox/config.ts
• nemoclaw/src/blueprint/runner.ts
• nemoclaw/src/blueprint/ssrf.ts
• test/config-set.test.ts
• nemoclaw/src/blueprint/runner.test.ts

[coderabbitai]

🧹 Nitpick comments (1)

test/e2e/live/inference-routing.test.ts (1)

712-712: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Prefer literal : over path.delimiter for constructing test PATH.

Based on learnings, this repo's test suite prefers the established POSIX PATH separator : when constructing process.env.PATH in tests rather than path.delimiter, since these tests only run on Linux CI runners (Windows is limited to WSL e2e, which is already POSIX-compliant).

♻️ Suggested change

-          PATH: `${fakeBinDir}${path.delimiter}${process.env.PATH ?? ""}`,
+          PATH: `${fakeBinDir}:${process.env.PATH ?? ""}`,

🤖 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 `@test/e2e/live/inference-routing.test.ts` at line 712, The PATH construction
in the e2e test is using path.delimiter, but this suite expects a fixed POSIX
separator. Update the PATH assignment in inference-routing.test.ts to use a
literal colon when prepending fakeBinDir, and keep the change localized to the
test setup that builds process.env.PATH.

Source: Learnings

🤖 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 `@test/e2e/live/inference-routing.test.ts`:
- Line 712: The PATH construction in the e2e test is using path.delimiter, but
this suite expects a fixed POSIX separator. Update the PATH assignment in
inference-routing.test.ts to use a literal colon when prepending fakeBinDir, and
keep the change localized to the test setup that builds process.env.PATH.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: ac0004f1-58dd-47ea-a8f6-35bad364af87

📥 Commits

Reviewing files that changed from the base of the PR and between 371c2ea and 9506fa4.

📒 Files selected for processing (8)

• nemoclaw/src/blueprint/runner.test.ts
• nemoclaw/src/blueprint/runner.ts
• nemoclaw/src/blueprint/ssrf.test.ts
• nemoclaw/src/blueprint/ssrf.ts
• src/lib/actions/inference-set-endpoint-security.test.ts
• src/lib/sandbox/config.ts
• test/config-set.test.ts
• test/e2e/live/inference-routing.test.ts

🚧 Files skipped from review as they are similar to previous changes (7)

• src/lib/sandbox/config.ts
• test/config-set.test.ts
• nemoclaw/src/blueprint/ssrf.test.ts
• nemoclaw/src/blueprint/runner.test.ts
• nemoclaw/src/blueprint/runner.ts
• src/lib/actions/inference-set-endpoint-security.test.ts
• nemoclaw/src/blueprint/ssrf.ts

[wscurran]

✨
Related open PRs:

• #4685 fix(security): bind HTTPS endpoint validation to connection IP

---

Related open issues:

• #4684 DNS rebinding TOCTOU for HTTPS endpoint validation