fix(security): bind HTTPS endpoint validation to connection IP#4685
Open
1PoPTRoN wants to merge 2 commits into
Open
fix(security): bind HTTPS endpoint validation to connection IP#46851PoPTRoN wants to merge 2 commits into
1PoPTRoN wants to merge 2 commits into
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds DNS-aware endpoint validation, a local loopback HTTPS pin-proxy that routes DNS-backed HTTPS to validated IPs while preserving Host/SNI, enforces stricter sandbox persistence (reject DNS-backed HTTPS), and wires proxy lifecycle into runner plus tests and docs. ChangesHTTPS DNS Rebinding Mitigation
Sequence Diagram(s)sequenceDiagram
participant Runner as runner.ts
participant Validate as validateEndpointUrl
participant Endpoint as endpointForValidatedEndpoint
participant Proxy as ensureHttpsPinProxyRoutes
participant Inference as Inference Provider
Runner->>Validate: Validate endpoint URL
Validate->>Validate: Resolve DNS (if hostname)
Validate-->>Runner: Return ValidatedEndpoint with resolved IP
Runner->>Endpoint: Map validated endpoint
Endpoint->>Endpoint: Determine if DNS-backed HTTPS
Endpoint-->>Runner: Return endpoint + optional proxy route
Runner->>Proxy: Setup proxy if route generated
Proxy->>Proxy: Spawn local HTTP proxy process
Proxy->>Proxy: Register routes with resolved IPs
Proxy-->>Runner: Proxy ready on loopback
Runner->>Inference: Configure with proxy URL or pinned URL
sequenceDiagram
participant Client as Blueprint/Provider
participant ProxyServer as Local HTTP Proxy
participant Upstream as Upstream HTTPS Endpoint
Client->>ProxyServer: GET /https-pin/<route-id>/...
ProxyServer->>ProxyServer: Load route by id and verify resolved IP
ProxyServer->>Upstream: https.request to validated IP with Host + SNI = original hostname
Upstream->>ProxyServer: TLS handshake + response
ProxyServer->>Client: Stream proxied response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 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 unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR tightens SSRF/DNS-rebinding safety around persisted HTTPS URLs by rejecting DNS-backed HTTPS endpoints in generic config persistence and introducing an HTTPS pinning proxy for blueprint runs that preserves TLS SNI/HTTP Host while pinning the connection IP.
Changes:
- Reject DNS-backed HTTPS URLs during
config setpersistence (while still pinning HTTP and allowing HTTPS IP-literals). - Extend SSRF validation results with protocol/hostname/DNS-resolution metadata and use it to route DNS-backed HTTPS endpoints through a local pinning proxy.
- Add the HTTPS pinning proxy implementation, port configuration, tests, and troubleshooting docs.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/config-set.test.ts | Updates expectations to reject DNS-backed HTTPS persistence and to use IP-literal HTTPS where allowed. |
| src/lib/sandbox/config.ts | Tracks DNS resolution details and rejects DNS-backed HTTPS URLs on persistence with a clearer error path. |
| nemoclaw/src/lib/ports.ts | Adds a dedicated env-configurable port for the HTTPS pin proxy. |
| nemoclaw/src/blueprint/ssrf.ts | Expands validated endpoint output to include DNS/pinning metadata needed by the proxy routing logic. |
| nemoclaw/src/blueprint/ssrf.test.ts | Adds assertions for new SSRF metadata fields and DNS-resolved behavior. |
| nemoclaw/src/blueprint/runner.ts | Routes DNS-backed HTTPS endpoints through the pin proxy and ensures routes exist during apply. |
| nemoclaw/src/blueprint/runner.test.ts | Adds tests for proxy routing behavior and for starting/ensuring proxy routes on apply. |
| nemoclaw/src/blueprint/https-pin-proxy.ts | Introduces the HTTPS pinning proxy server, persistence, routing, and spawn/health logic. |
| nemoclaw/src/blueprint/https-pin-proxy.test.ts | Tests endpoint routing and request option construction (SNI/Host preservation + IP pin). |
| docs/reference/troubleshooting.mdx | Documents the new behavior/error guidance for DNS-backed HTTPS in config persistence. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/lib/sandbox/config.ts (1)
836-838: 💤 Low valuePer coding guidelines, consider running E2E tests for this config persistence change.
This change modifies URL persistence rules in
config set. As per the coding guidelines for this file, changes to config writes affect Docker/VM sandboxes and should be validated with E2E tests.gh workflow run nightly-e2e.yaml --ref <branch> -f jobs=shields-config-e2e🤖 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/sandbox/config.ts` around lines 836 - 838, This change updates URL persistence rules around the "Validate URLs for SSRF" block in src/lib/sandbox/config.ts (affecting the config set path), so run the nightly E2E job for config persistence to validate behavior in Docker/VM sandboxes: execute the GitHub Actions workflow with gh workflow run nightly-e2e.yaml --ref <branch> -f jobs=shields-config-e2e and verify tests for config writes/pass-through of HTTP vs HTTPS URL pinning pass; if failures occur, adjust the config set logic around the URL persistence code until the E2E passes.
🤖 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 `@docs/reference/troubleshooting.mdx`:
- Line 833: The long single source line beginning "Host-side `config set`
validates any HTTP or HTTPS URLs..." should be split so each sentence is on its
own source line; locate the paragraph starting with that text and break it into
separate lines at each sentence boundary, preserving punctuation, inline code
formatting (backticks), and original wording (including the clause about
loopback/private/reserved/internal hosts, DNS resolution and IP pinning, the
HTTPS/Host/SNI note, and the credential-rotation recommendation) to improve diff
readability.
In `@nemoclaw/src/blueprint/https-pin-proxy.ts`:
- Around line 263-284: The upstream HTTPS request created by https.request
(assigned to upstream from buildPinnedHttpsRequestOptions) lacks a timeout and
can hang; add a request timeout (e.g., 30s) by calling
upstream.setTimeout(timeoutMs) and listen for the 'timeout' event to destroy the
upstream socket with an Error, then mirror the existing error handling: if
!res.headersSent call sendError(res, 502, "...timeout...") else
res.destroy(err); ensure you also handle cleanup when upstream emits 'close' or
'error' and keep the existing req.pipe(upstream) flow.
---
Nitpick comments:
In `@src/lib/sandbox/config.ts`:
- Around line 836-838: This change updates URL persistence rules around the
"Validate URLs for SSRF" block in src/lib/sandbox/config.ts (affecting the
config set path), so run the nightly E2E job for config persistence to validate
behavior in Docker/VM sandboxes: execute the GitHub Actions workflow with gh
workflow run nightly-e2e.yaml --ref <branch> -f jobs=shields-config-e2e and
verify tests for config writes/pass-through of HTTP vs HTTPS URL pinning pass;
if failures occur, adjust the config set logic around the URL persistence code
until the E2E passes.
🪄 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: 9b8f9cd5-848f-4c13-b660-61bccddd0106
📒 Files selected for processing (10)
docs/reference/troubleshooting.mdxnemoclaw/src/blueprint/https-pin-proxy.test.tsnemoclaw/src/blueprint/https-pin-proxy.tsnemoclaw/src/blueprint/runner.test.tsnemoclaw/src/blueprint/runner.tsnemoclaw/src/blueprint/ssrf.test.tsnemoclaw/src/blueprint/ssrf.tsnemoclaw/src/lib/ports.tssrc/lib/sandbox/config.tstest/config-set.test.ts
Contributor
|
✨ Thanks for submitting this detailed PR about fixing a DNS rebinding TOCTOU gap for DNS-backed HTTPS endpoints. This proposes a way to enhance the security of NemoClaw's inference endpoints. Related open issues: |
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
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
Fixes a DNS rebinding TOCTOU gap for DNS-backed HTTPS endpoints. NemoClaw now avoids handing the original hostname URL directly to the downstream provider after validation, and instead routes DNS-backed HTTPS endpoints through a local pinned proxy that connects to the validated IP while preserving TLS SNI and the Host header.
Related Issue
Fixes #4684
Changes
Type of Change
Verification
npx prek run --all-filespassesnpm testpassesnpm run docsbuilds without warnings (doc changes only)Additional verification run:
cd nemoclaw && npm run checkcd nemoclaw && npm run buildnpm run typecheck:clinpx vitest run test/config-set.test.ts --config vitest.config.tsSigned-off-by: 1PoPTRoN vrxn.arp1traj@gmail.com
Summary by CodeRabbit
New Features
Documentation
Improvements