Skip to content

fix(codex): bind update transport trust environment - #7513

Closed
luvs01 wants to merge 2 commits into
lidge-jun:devfrom
luvs01:codex/fix-codex-cli-update-registry-vulnerability
Closed

luvs01 wants to merge 2 commits into
lidge-jun:devfrom
luvs01:codex/fix-codex-cli-update-registry-vulnerability

Conversation

@luvs01

@luvs01 luvs01 commented Oct 22, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Bind Codex CLI update planning to transport settings captured by the Node launcher before Bun loads project dotenv. Registry evidence no longer takes proxy or CA settings from the later ambient environment, while legitimate operator settings remain supported through the actual CLI path.

The launcher carries proxy/CA entries in its existing proof-bound snapshot, removes duplicate child environment entries, validates and freezes the parsed map, and forwards it to npm inspection. Casing, aliases, empty values and absence retain their meaning. Older snapshots do not fall back to ambient settings.

Verification

  • Exact-head hosted CI passed at 60b1be204964d044790e1f5429df384898f89b07.
  • Pinned Bun 1.4.3 official queued runner: boundary/plan tests 65 passed, 3 platform skips; final boundary fixture 12 passed, 2 platform skips, 52 assertions. Other focused files reported 54 passing tests.
  • The published Node launcher fixture exercises Bun dotenv loading, trusted-context parsing, CLI forwarding and all three real resolver calls with mocked npm/process boundaries. It checks early settings, exclusion of dotenv values and output redaction.
  • TypeScript, structure, privacy, changed-file size and whitespace checks passed. Independent review checked production integration, precedence and credential handling.
  • An existing npm invocation passed with an explicit 30-second test timeout after exceeding the default five seconds. Runtime timeouts are unchanged. Whole-tree size checking was unavailable in the sparse local checkout; the full local suite and real proxy/TLS connectivity were not run. Native Windows/macOS jobs skipped by hosted policy are not claimed as passing.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • The transport contract and regression coverage are updated.
  • Security-sensitive trusted-context and credential paths received independent review.

Review readiness

  • Required validation passed with its scope documented.
  • Branch is based on the current dev commit.
  • No correct Codex or CodeRabbit findings remain outstanding at submission.
  • Ready for review, subject to upstream exact-head CI and maintainer review.

Summary by CodeRabbit

  • Bug Fixes
    • Codex CLI update checks now use the proxy and certificate settings captured by the launcher, preventing ambient or dotenv settings from overriding them.
    • Transport settings are validated and kept out of reports and diagnostics. Older snapshots without these settings continue without an ambient fallback.

@coderabbitai

coderabbitai Bot commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

📝 Walkthrough

The launcher now captures allowlisted proxy and CA environment variables in the Codex CLI inspection snapshot. The trusted context validates and freezes those values, then passes them to update inspection and npm target resolution.

Changes: Codex CLI transport environment

Layer / File(s) Summary
Capture and validate transport settings
src/update/codex-cli-update-launch-policy.mjs, src/update/codex-cli-update-launch-policy.d.mts, bin/ocx.mjs, src/cli/launcher-context.ts, tests/cli/cli-codex-cli-update-boundary.test.ts, tests/cli/ocx-launcher-source.test.ts
The launcher captures allowlisted proxy and CA settings and removes them from the inherited inspection environment. The trusted context rejects invalid transport maps, freezes valid maps, and uses an empty map when the field is missing. Tests cover capture, validation, and older snapshots.
Bind settings to update resolution
src/cli/codex-cli-update.ts, src/codex/cli-update-plan.ts, structure/codex-cli-update.md, tests/cli/cli-codex-cli-update-boundary.test.ts, tests/codex-integration/codex-cli-update-plan.test.ts
Update inspection forwards the trusted transport map. Target resolution removes ambient proxy and CA values, then applies captured values when a snapshot is available. Documentation and tests describe and check the resulting environment, including absent settings and dotenv behavior.

Priority: ➖ Normal

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

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant Launcher as bin/ocx.mjs
  participant Context as launcher-context
  participant Update as codex-cli-update
  participant Plan as cli-update-plan
  participant Npm as npm
  Launcher->>Context: inspection snapshot with transportEnv
  Context->>Update: validated transportEnv
  Update->>Plan: inspection environment with transportEnv
  Plan->>Npm: target-resolution environment
Loading
Merge Risk: ⚪ Minimal · up to 60b1b

The inspected update paths preserve the captured transport settings, and no actionable issue is established before merge.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 9 files. (1 skipped: … Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 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 change: binding Codex CLI update transport settings to the trusted launcher environment.
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.
Full details: Docstring Coverage

Explanation

Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 9 files. (1 skipped: 1 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@github-actions

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the bug Something isn't working label Oct 22, 2026
@luvs01
luvs01 marked this pull request as ready for review October 22, 2026 06:13

@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.

🔇 Additional comments: 11
src/update/codex-cli-update-launch-policy.mjs: 1

19-27: LGTM!

src/update/codex-cli-update-launch-policy.d.mts: 1

19-20: LGTM!

bin/ocx.mjs: 1

62-63: LGTM!

Also applies to: 1213-1213, 1217-1217, 1230-1231

src/cli/launcher-context.ts: 1

28-28: LGTM!

Also applies to: 54-62, 109-111, 123-123, 129-129, 144-144

tests/cli/cli-codex-cli-update-boundary.test.ts: 1

3-3: LGTM!

Also applies to: 5-6, 11-11, 58-190

tests/cli/ocx-launcher-source.test.ts: 1

146-147: LGTM!

src/cli/codex-cli-update.ts: 1

224-224: LGTM!

src/codex/cli-update-plan.ts: 1

223-224: LGTM!

Also applies to: 258-261, 271-271, 276-276, 308-308, 361-361, 500-501

structure/codex-cli-update.md: 1

11-19: LGTM!

tests/codex-integration/codex-cli-update-plan.test.ts: 1

477-477: LGTM!

Also applies to: 479-499, 618-619

src/codex/cli-update-plan.ts-266-268: 1

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

⚠️ Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.

Import the transport key allowlist instead of copying it into boundKeys.

CODEX_CLI_TRANSPORT_ENV_KEYS in src/update/codex-cli-update-launch-policy.mjs (Lines 19-21) already defines the allowlist. The launcher uses it to capture and to strip inherited variables. parseTransportEnv in src/cli/launcher-context.ts uses it to validate. This hunk adds a fourth hand-written copy of the same five names.

The copies can drift. If a key is added to the shared list, the launcher captures it and the parser accepts it. codexCliUpdateBoundEnv then neither removes the ambient alias nor copies the bound value. The npm child then runs with a different transport environment than the captured snapshot. No test or gate detects this.

Derive the set from the shared constant.

♻️ Proposed fix
-  const boundKeys = new Set([
-    "path", "pathext", "http_proxy", "https_proxy", "all_proxy", "no_proxy", "node_extra_ca_certs",
-  ]);
+  const boundKeys = new Set<string>(["path", "pathext", ...CODEX_CLI_TRANSPORT_ENV_KEYS]);

Add this import near the top of the file. It is outside the selected range.

import { CODEX_CLI_TRANSPORT_ENV_KEYS } from "../update/codex-cli-update-launch-policy.mjs";

src/cli/launcher-context.ts already imports the same .mjs module from TypeScript, and codex-cli-update-launch-policy.d.mts types it. Confirm that bun scripts/import-cycles.ts still reports no new cycle for src/codex/cli-update-plan.ts.

As per coding guidelines: "Do not duplicate provider facts across independent pickers or seeds." The same single-source principle applies to this shared allowlist.

Source: Coding guidelines


ℹ️ Review info
⚙️ Run configuration
  • Configuration used: Repository: lidge-jun/opencodex/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 0ddc4d2d-45ab-4069-a214-04a4fe4f1e7c
📥 Commits

Reviewing files that changed from the base of the PR and between bf4eb12 and 60b1be2.

📒 Files selected for processing: 10
  • bin/ocx.mjs
  • src/cli/codex-cli-update.ts
  • src/cli/launcher-context.ts
  • src/codex/cli-update-plan.ts
  • src/update/codex-cli-update-launch-policy.d.mts
  • src/update/codex-cli-update-launch-policy.mjs
  • structure/codex-cli-update.md
  • tests/cli/cli-codex-cli-update-boundary.test.ts
  • tests/cli/ocx-launcher-source.test.ts
  • tests/codex-integration/codex-cli-update-plan.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review.

@lidge-jun

Copy link
Copy Markdown
Owner

Carried into #7546 together with #7540, with your authorship kept on the original commits and a Co-authored-by trailer on the follow-up. Both changes rewrite the same launcher snapshot, so they land as one PR. #7546's description records how the conflicts were resolved, the CodeRabbit single-source fix, and two macOS-only test fixes. A maintainer will close this PR.

@lidge-jun

Copy link
Copy Markdown
Owner

Thank you for this work. It has been carried onto current dev in #7546, with your authorship kept as a Co-authored-by trailer, and review continues there. Closing this PR in favour of #7546.

@lidge-jun lidge-jun closed this Oct 22, 2026
lidge-jun added a commit that referenced this pull request Oct 22, 2026
…env transport (#7546)

Carries #7540 and #7513 onto current dev in one change, with the review findings from both PRs resolved.

Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants