Skip to content

fix(openshell): require literal loopback for plaintext gRPC - #6792

Merged
ericksoa merged 2 commits into
chore/openshell-grpc-protocol/aefrom
feat/openshell-grpc-client/ae
Jul 14, 2026
Merged

fix(openshell): require literal loopback for plaintext gRPC#6792
ericksoa merged 2 commits into
chore/openshell-grpc-protocol/aefrom
feat/openshell-grpc-client/ae

Conversation

@ericksoa

@ericksoa ericksoa commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Summary

Hardens the new direct gRPC client so plaintext transport is accepted only for literal loopback IP addresses. DNS hostnames such as localhost must use TLS because their resolution is not fixed by the endpoint string.

This is PR 3 of 26 in the OpenShell gRPC migration stack.

Changes

  • Validate plaintext endpoints with Node's IP parser instead of a hostname or dotted-string heuristic.
  • Accept IPv4 127.0.0.0/8 and IPv6 ::1 literals only.
  • Reject localhost, non-loopback IPs, malformed dotted addresses, and non-HTTP(S) endpoint forms.
  • Update the direct-client fixtures and focused endpoint-safety coverage.

Stack

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)

Quality Gates

  • Tests added or updated for changed behavior
  • Existing tests cover changed behavior — justification:
  • Tests not applicable — justification:
  • Docs updated for user-facing behavior changes
  • Docs not applicable — justification: this slice tightens an internal transport safety check before any product call site selects direct gRPC.
  • Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging)
  • Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: plaintext is restricted to literal kernel-recognized loopback addresses; hostnames and malformed or non-loopback addresses fail closed.
  • Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue:

Verification

  • PR description includes the DCO sign-off declaration and every commit appears as Verified in GitHub
  • Normal pre-commit, commit-msg, and pre-push hooks passed, or npm run check:diff passed when hooks were skipped or unavailable
  • Targeted behavior tests pass for the current change set, or tests are marked not applicable above — focused direct gRPC client tests passed (19 tests); npm run typecheck:cli passed
  • Applicable broad gate passed — npm test for broad runtime/test-harness changes; npm run check for repo-wide validation/coverage changes — targeted and changed-graph validation were used; required CI supplies the broad repository gates
  • Quality Gates section completed with required justifications or waivers
  • No secrets, API keys, or credentials committed
  • 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)

Signed-off-by: Aaron Erickson aerickson@nvidia.com

Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
@coderabbitai

coderabbitai Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: a030f40e-5b3d-447e-b5d1-cccbf6432b46

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/openshell-grpc-client/ae

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

@github-code-quality

github-code-quality Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage remains at 96%, unchanged from the chore/openshell-grpc... branch.

TypeScript / code-coverage/cli

The overall coverage in the feat/openshell-grpc-... branch remains at 79%, unchanged from the chore/openshell-grpc... branch.

Show a code coverage summary of the most impacted files.
File chore/openshell-grpc... 986b883 feat/openshell-grpc-... da90420 +/-
src/lib/state/m...-acquisition.ts 81% 80% -1%
src/lib/adapter...dbox-control.ts 0% 97% +97%
src/lib/adapter...dbox-control.ts 0% 100% +100%

Updated July 13, 2026 23:08 UTC
Code Coverage is in Public Preview. Learn more and provide us with your feedback.

@github-actions

github-actions Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor — Informational

Advisor assessment: Informational / low confidence
Primary next action: Review the warnings below.
Findings: 0 blockers · 1 warning · 0 optional suggestions
Status: Canonical ledger: 0 blocker(s), 1 warning(s), 0 suggestion(s).

E2E guidance

Advisory only: coverage and selector recommendations are non-authoritative. E2E / PR Gate independently computes and dispatches trusted jobs without consuming this output.

Recommended coverage: cloud-onboard, credential-sanitization, security-posture
Recommended selectors: cloud-onboard, credential-sanitization, security-posture

  • cloud-onboard — Selected from the trusted checked-in E2E coverage inventory.

  • credential-sanitization — Selected from the trusted checked-in E2E coverage inventory.

  • security-posture — Selected from the trusted checked-in E2E coverage inventory.

  • cloud-onboard — Selected as a trusted checked-in E2E job.

  • credential-sanitization — Selected as a trusted checked-in E2E job.

  • security-posture — Selected as a trusted checked-in E2E job.

1 optional coverage item · 1 optional selector · 0 new-test recommendations
  • Optional coverage openshell-gateway-auth-contract — Selected from the trusted checked-in E2E coverage inventory.
  • Optional selector openshell-gateway-auth-contract — Selected as a trusted checked-in E2E job.
1 warning · 0 optional suggestions

Warnings

These merit maintainer attention but do not block by themselves.

PRA-1 Warning — Reject malformed dotted hosts before allowing plaintext gRPC

  • Location: src/lib/adapters/openshell/grpc-sandbox-control.ts:76
  • Category: security
  • Problem: The plaintext endpoint guard accepts any hostname matching `127.<1–3 digits>.<1–3 digits>.<1–3 digits>`, including invalid IPv4 text such as `127.999.999.999`. Because this is not a validated loopback address, it can be passed to gRPC name resolution despite the intended restriction that HTTP transport be loopback-only.
  • Impact: A configuration value intended to be constrained to local plaintext control traffic can cause a connection attempt to a non-loopback name, weakening the SSRF/network-policy boundary and risking command dispatch to an unintended gateway.
  • Recommendation: Parse the host as an IP address and accept plaintext only for canonical loopback addresses (or strictly validate each IPv4 octet and the 127/8 range); reject malformed numeric-looking dotted hosts before constructing the gRPC target.
  • Verification: Inspect `parseEndpoint()` with `http://127.999.999.999:8080\` and verify that construction throws rather than returning a target passed to the gRPC client.
  • Test coverage: Add table-driven negative endpoint tests covering `http://127.999.999.999:8080\` and another out-of-range dotted component, asserting `createOpenShellGrpcApi()` rejects both.
  • Evidence: `isLoopback()` at line 76 uses a shape-only regular expression for `127.*.*.*` rather than IP parsing or octet-range validation. `parseEndpoint()` permits HTTP whenever `isLoopback(url.hostname)` returns true (lines 92–94). The new adapter sends sandbox lookup and exec requests to the constructed gRPC target.

Workflow run details

This is an automated, non-authoritative review. Findings are inputs to maintainer adjudication. Warnings and optional suggestions do not require a response or follow-up. A human maintainer makes the final merge decision.

Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
@ericksoa
ericksoa merged commit 022f386 into chore/openshell-grpc-protocol/ae Jul 14, 2026
51 of 53 checks passed
@ericksoa
ericksoa deleted the feat/openshell-grpc-client/ae branch July 14, 2026 04:42
@ericksoa ericksoa changed the title feat(openshell): add direct gRPC sandbox client fix(openshell): require literal loopback for plaintext gRPC Jul 14, 2026
@wscurran wscurran added area: security Security controls, permissions, secrets, or hardening bug-fix PR fixes a bug or regression labels Jul 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: security Security controls, permissions, secrets, or hardening bug-fix PR fixes a bug or regression

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants