Skip to content

fix(dcode): allow local OTLP endpoint env#6533

Closed
HOYALIM wants to merge 1 commit into
NVIDIA:mainfrom
HOYALIM:codex/issue-6466-otel-secret-false-positive
Closed

fix(dcode): allow local OTLP endpoint env#6533
HOYALIM wants to merge 1 commit into
NVIDIA:mainfrom
HOYALIM:codex/issue-6466-otel-secret-false-positive

Conversation

@HOYALIM

@HOYALIM HOYALIM commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Allow canonical local HTTP OTLP collector URLs for OTEL_EXPORTER_OTLP_ENDPOINT and OTEL_EXPORTER_OTLP_TRACES_ENDPOINT in the DCode runtime env guard.
  • Mirror the same narrow allowance in the managed direct-module runtime guard.
  • Keep OTLP headers, remote URLs, userinfo, queries, fragments, non-standard OTLP ports, and other auth/key env names rejected.

Root Cause

The DCode secret guard treated OTLP endpoint env var names as credential context unconditionally, so http://host.openshell.internal:4318 reached the generic name-based refusal path even though the value contained no secret material.

Validation

  • bash -n agents/langchain-deepagents-code/dcode-wrapper.sh
  • python3 -m py_compile agents/langchain-deepagents-code/managed-dcode-runtime.py
  • NEMOCLAW_TEST_TIMEOUT=60000 npx vitest run --project integration test/langchain-deepagents-code-image-credentials.test.ts test/langchain-deepagents-code-direct-module-patch.test.ts
  • npm run build:cli
  • NEMOCLAW_TEST_TIMEOUT=60000 npm run check:diff

DCO

  • DCO verified: commit 5bd589e1237ac7572be06b8ed86c87b103985bc0 includes Signed-off-by: Ho Lim <subhoya@gmail.com>.

Summary by CodeRabbit

  • Bug Fixes

    • Allowed specific local OpenTelemetry endpoint environment variables when they point to loopback-only HTTP URLs on standard OTLP ports.
    • Continued blocking credential-like or non-local endpoint values, with stricter rejection of suspicious URL forms.
    • Improved secret-scanning behavior so approved OTLP endpoint values no longer trigger false positives.
  • Tests

    • Added and updated tests to cover accepted local OTLP endpoints, rejected remote or malformed values, and stronger secret-redaction checks.

The DCode secret guard treated OTLP endpoint variable names as credential
context unconditionally. A local collector URL therefore reached the generic
name-based refusal path even when the value had no secret material.

Allow only canonical HTTP local OTLP endpoint URLs for endpoint variables before
the credential-name fallback runs. OTLP headers, remote URLs, userinfo, queries,
fragments, and other auth/key names remain rejected.

Signed-off-by: Ho Lim <subhoya@gmail.com>
Copilot AI review requested due to automatic review settings July 9, 2026 01:41
@copy-pr-bot

copy-pr-bot Bot commented Jul 9, 2026

Copy link
Copy Markdown

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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@coderabbitai

coderabbitai Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Introduces a security-hardened allowlist mechanism permitting only specific local/loopback OTLP telemetry endpoint environment variables to bypass secret-scanning checks in both dcode-wrapper.sh and managed-dcode-runtime.py, restricted to canonical HTTP URLs, fixed ports, and limited paths. Tests validate accepted and rejected endpoint values.

Changes

Local OTLP endpoint allowlist

Layer / File(s) Summary
Shell wrapper OTLP allowlist and scan integration
agents/langchain-deepagents-code/dcode-wrapper.sh
Adds helpers to identify allowed OTLP endpoint variable names and validate their values as canonical loopback http:// URLs on ports 4317/4318 with restricted paths; wires an early-accept branch into the env scanning loop and updates the security comment.
Python runtime OTLP allowlist config and validator
agents/langchain-deepagents-code/managed-dcode-runtime.py
Adds allowlist constants (env names, hosts, ports, paths) and _is_allowed_local_otlp_endpoint; reworks _assert_safe_environment to reject secret-shaped values first, then allow validated OTLP endpoints, then apply remaining credential heuristics.
Tests for OTLP allowlist and secret rejection
test/langchain-deepagents-code-direct-module-patch.test.ts, test/langchain-deepagents-code-image-credentials.test.ts
Adds parameterized tests confirming allowed local OTLP endpoint values succeed while non-local/credential-shaped values fail with proper exit codes and no value leakage; strengthens existing OPENAI_API_KEY rejection assertions.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Runtime as dcode-wrapper.sh / managed-dcode-runtime.py
  participant EnvVar as Environment Variable
  participant Validator as OTLP Allowlist Validator
  participant Scanner as Secret Scanner

  Runtime->>EnvVar: read name and value
  Runtime->>Validator: is_allowed_local_otlp_endpoint(name, value)
  alt name matches OTLP endpoint set and value is canonical loopback URL
    Validator-->>Runtime: accepted
    Runtime->>Runtime: continue (skip secret checks)
  else not a valid OTLP endpoint
    Validator-->>Runtime: rejected
    Runtime->>Scanner: apply secret/credential-shape checks
    alt secret-shaped value detected
      Scanner-->>Runtime: raise error, refuse to start
    else no secret detected
      Scanner-->>Runtime: allow variable
    end
  end
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main change: allowing local OTLP endpoint environment variables in DCode.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
test/langchain-deepagents-code-direct-module-patch.test.ts (1)

369-380: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Assert the rejected value isn't leaked in stderr.

The rejection assertion only checks that CUSTOM_API_KEY appears in stderr, but doesn't verify the value itself is absent — the same hardening applied to the sibling OTLP-reject tests in test/langchain-deepagents-code-image-credentials.test.ts (e.g. expect(result.stderr).not.toContain(value)).

Suggested addition
     expect(rejected.status).not.toBe(0);
     expect(rejected.stderr).toContain("runtime environment variable CUSTOM_API_KEY");
+    expect(rejected.stderr).not.toContain("http://host.openshell.internal:4318");
   });
🤖 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/langchain-deepagents-code-direct-module-patch.test.ts` around lines 369
- 380, The rejection test only verifies that CUSTOM_API_KEY is mentioned in
stderr, but it should also confirm the secret value is not leaked. Update the
deepagents_code rejection assertion in this test to mirror the hardening used in
the sibling OTLP-reject tests by checking the stderr does not contain the actual
CUSTOM_API_KEY value passed to spawnSync, using the rejected result and its
stderr output.
🤖 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/langchain-deepagents-code-direct-module-patch.test.ts`:
- Around line 369-380: The rejection test only verifies that CUSTOM_API_KEY is
mentioned in stderr, but it should also confirm the secret value is not leaked.
Update the deepagents_code rejection assertion in this test to mirror the
hardening used in the sibling OTLP-reject tests by checking the stderr does not
contain the actual CUSTOM_API_KEY value passed to spawnSync, using the rejected
result and its stderr output.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: ac6801a1-97e4-4d6a-9509-6ce012a3251a

📥 Commits

Reviewing files that changed from the base of the PR and between ce19c7b and 5bd589e.

📒 Files selected for processing (4)
  • agents/langchain-deepagents-code/dcode-wrapper.sh
  • agents/langchain-deepagents-code/managed-dcode-runtime.py
  • test/langchain-deepagents-code-direct-module-patch.test.ts
  • test/langchain-deepagents-code-image-credentials.test.ts

@apurvvkumaria

Copy link
Copy Markdown
Collaborator

Closing as superseded by merged #6538, which already resolved #6466 with the canonical exact-host OTLP validator and broader adversarial/non-leak coverage. This branch now conflicts in all four changed files, and its only unique behavior is a wider localhost/host.docker.internal/host.containers.internal allowlist that does not match the managed collector network policy. The contribution is GitHub Verified and DCO-compliant, but there is no safe unique change to salvage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v0.0.79 Release target

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants