Skip to content

fix(network-proxy): recheck network proxy connect targets#19999

Merged
evawong-oai merged 2 commits intomainfrom
codex/viyatb/network-connect-policy
Apr 28, 2026
Merged

fix(network-proxy): recheck network proxy connect targets#19999
evawong-oai merged 2 commits intomainfrom
codex/viyatb/network-connect-policy

Conversation

@viyatb-oai
Copy link
Copy Markdown
Collaborator

@viyatb-oai viyatb-oai commented Apr 28, 2026

Why

The proxy checks the requested host before opening the upstream connection, but DNS can resolve an allowed hostname to a loopback, private, or other non-public address after that first decision. Without a final check on the actual socket target, a request that looks acceptable at the hostname layer can still connect to a local service once resolution completes.

What changed

  • add a shared TCP connector check for direct proxy egress
  • use that path for HTTP, CONNECT, SOCKS5, and MITM upstream connections
  • keep configured upstream proxy hops on the existing proxy path
  • add direct-connector coverage for allowed and rejected local targets

Security impact

Direct proxy egress now rechecks the resolved socket address before connecting, closing the gap between hostname policy evaluation and the final network target.

Verification

  • cargo test -p codex-network-proxy

viyatb-oai and others added 2 commits April 28, 2026 08:02
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
@viyatb-oai viyatb-oai changed the title [codex] Recheck network proxy connect targets fix(network-proxy): recheck network proxy connect targets Apr 28, 2026
@viyatb-oai viyatb-oai marked this pull request as ready for review April 28, 2026 17:55
Copy link
Copy Markdown
Contributor

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e0d08ebf92

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

type Error = BoxError;

async fn connect(&self, addr: SocketAddr) -> Result<TcpStream, Self::Error> {
if !self.policy.allow_local_binding().await? && is_non_public_ip(addr.ip()) {
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.

P1 Badge Preserve explicit local allowlist in target recheck

Rejecting all non-public socket addresses when allow_local_binding is false regresses a supported policy path: NetworkProxyState::host_blocked intentionally permits explicitly allowlisted local literals. With this connector check, allowlisted targets like localhost/10.0.0.1 pass host policy but are still denied at connect time (PermissionDenied), so valid configurations now fail.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor

@evawong-oai evawong-oai left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Contributor

evawong-oai commented Apr 28, 2026

Validation I ran before merge:

  1. Confirmed this maps to the DNS rebinding item in the readiness tracker.
  2. Confirmed the issue is the DNS rebinding TOCTOU where policy checks an allowed host before connect, then the final TCP connect can resolve to loopback or private IP.
  3. Reviewed the new TargetCheckedTcpConnector and confirmed it rejects the final resolved SocketAddr when allow_local_binding is false and the target IP is non public.
  4. Confirmed HTTP CONNECT uses the target checked connector.
  5. Confirmed plain HTTP direct egress uses the target checked connector.
  6. Confirmed SOCKS5 TCP uses the target checked connector.
  7. Confirmed MITM upstream config carries allow_local_binding so MITM egress also uses the same final target policy.
  8. Verified the PR head I reviewed was e0d08eb.
  9. Verified required GitHub checks were green, including Ubuntu build, clippy, and release checks.
  10. Ran cargo test for codex network proxy locally and got 135 passed and 0 failed.
  11. Ran git diff check locally and it passed.
  12. Approved and squash merged. Merge commit is e1ba87c.

Remaining readiness gate: verify the target event build includes the merge commit and rerun the original rbndr.us PoC or an equivalent real Codex managed proxy regression showing the old local target read path fails.

Ship decision: approved and merged as the minimal fix. I updated the readiness row to Closed but Verify.

@evawong-oai evawong-oai merged commit e1ba87c into main Apr 28, 2026
35 of 36 checks passed
@evawong-oai evawong-oai deleted the codex/viyatb/network-connect-policy branch April 28, 2026 19:51
@github-actions github-actions Bot locked and limited conversation to collaborators Apr 28, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants