Skip to content

fix(proxy): respect upstream_proxy in TLS CONNECT intercept path (#1048) - #1091

Merged
SequeI merged 4 commits into
nolabs-ai:mainfrom
caiocdcs:1048-upstream-proxy-intercept
Jun 17, 2026
Merged

fix(proxy): respect upstream_proxy in TLS CONNECT intercept path (#1048)#1091
SequeI merged 4 commits into
nolabs-ai:mainfrom
caiocdcs:1048-upstream-proxy-intercept

Conversation

@caiocdcs

@caiocdcs caiocdcs commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

The TLS intercept upstream leg (used for credential injection) always connected directly to the target host, ignoring the configured upstream_proxy / external_proxy. In enterprise environments where the target is only reachable through a corporate proxy, this caused the intercepted request to hang or fail.

Issue: #1048

Approach

Two changes in server.rs::handle_connection and tls_intercept::handle::forward_inner_request:

  1. server.rs — before entering the intercept path, check the bypass matcher against the target host. If the host should go through the upstream proxy and auth is not configured, pass the proxy address into InterceptCtx as InterceptUpstreamProxy. This mirrors the existing bypass logic for transparent CONNECT.
  2. handle.rsforward_inner_request selects UpstreamStrategy::ExternalProxy when ctx.upstream_proxy is set, falling back to UpstreamStrategy::Direct. The forward.rs pipeline already knew how to chain through a proxy — the intercept path just never asked it to.

Files changed

  • crates/nono-proxy/src/tls_intercept/handle.rs — Added InterceptUpstreamProxy struct + upstream_proxy field; strategy selection in forward_inner_request; 3 unit tests.
  • crates/nono-proxy/src/tls_intercept/mod.rs — Re-export InterceptUpstreamProxy.
  • crates/nono-proxy/src/server.rs — Bypass & auth gate before InterceptCtx construction.

Compliance

  • I am not prohibited from contributing under this policy
  • An issue already exists (TLS CONNECT interception ignores configured upstream_proxy #1048)
  • I described my intent and approach in the issue discussion
  • I reviewed repository coding and security rules for the affected area
  • I provided required attribution for reused or adapted code
  • I did not use forbidden patterns such as unwrap/expect
  • I used NonoError where required (proxy crate uses ProxyError, not applicable to library NonoError)
  • I validated and canonicalized all relevant paths (no file path changes in this PR)
  • This PR matches the disclosed issue scope

AI-assisted contribution.

@github-actions github-actions Bot added bug Something isn't working nono-proxy size/medium labels Jun 6, 2026
@github-actions

github-actions Bot commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

PR Review Summary

Size

Metric Value
Lines added +149
Lines removed -4
Total changed 153
Classification Medium (50–300 lines)

Affected crates

  • crates/nono-proxydownstream consumers depend on this crate. API or behaviour changes will affect external callers; treat any breaking change with extra scrutiny.

Blast radius — Contained

This PR touches: source code


Updated automatically on each push to this PR.

@gemini-code-assist gemini-code-assist 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.

Code Review

This pull request implements upstream proxy chaining for TLS intercepted connections, allowing the upstream leg to route through a corporate proxy when configured. The reviewer suggests extracting the strategy selection logic into a helper function select_upstream_strategy to eliminate code duplication between the production code and the newly added unit tests.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread crates/nono-proxy/src/tls_intercept/handle.rs
Comment thread crates/nono-proxy/src/tls_intercept/handle.rs Outdated
Comment thread crates/nono-proxy/src/tls_intercept/handle.rs
…abs-ai#1048)

The TLS intercept upstream leg (used for credential injection) always
connected directly to the target host, ignoring the configured
upstream_proxy / external_proxy. In enterprise environments where the
target is only reachable through a corporate proxy, this caused the
intercepted request to hang or fail.

- Added InterceptUpstreamProxy struct and upstream_proxy field to
  InterceptCtx so the intercept code can select
  UpstreamStrategy::ExternalProxy when a proxy is configured.
- server.rs now checks the bypass matcher and passes the resolved
  proxy info into InterceptCtx, mirroring the existing transparent
  CONNECT path.
- Auth-not-implemented check (config.auth.is_some()) fails early
  and consistently with the external proxy handler.
- 3 unit tests for strategy selection (ExternalProxy vs Direct).

Signed-off-by: Caio Silva <caio@cdcs.dev>
@caiocdcs
caiocdcs force-pushed the 1048-upstream-proxy-intercept branch from 41b8574 to c9c7264 Compare June 6, 2026 09:55

@SequeI SequeI left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thank you!

@SequeI
SequeI merged commit b5f8db5 into nolabs-ai:main Jun 17, 2026
13 checks passed
oscarmackjr-twg added a commit to OscarMackJr/nono that referenced this pull request Jun 23, 2026
- build_proxy_config_maps_upstream_proxy_to_external_proxy: verifies that
  build_proxy_config_from_flags maps upstream_proxy to
  ProxyConfig.external_proxy (nolabs-ai#1048/nolabs-ai#1091 / D-01), documenting that the
  fork already delivers this intent via the external_proxy field
- No cherry-pick of upstream nolabs-ai#1048/nolabs-ai#1091 needed; equivalence confirmed

Signed-off-by: Oscar Mack Jr <oscar.mack.jr@gmail.com>
oscarmackjr-twg added a commit to OscarMackJr/nono that referenced this pull request Jun 23, 2026
…e test

Summary of plan 89-01:
- fix: proxy activation predicate now includes !prepared.custom_credentials.is_empty()
  in both ACTIVE and WARN branches (0c08e5d)
- test: D-07 regression tests proxy_activates_with_custom_credentials_only and
  block_net_overrides_custom_credentials_activation (73bd03a)
- test: D-01 equivalence test build_proxy_config_maps_upstream_proxy_to_external_proxy
  confirms cherry-pick of nolabs-ai#1048/nolabs-ai#1091 unnecessary (751c6ca)
- 13/13 proxy_runtime tests green; cross-target verify PARTIAL→CI

Signed-off-by: Oscar Mack Jr <oscar.mack.jr@gmail.com>
oscarmackjr-twg added a commit to OscarMackJr/nono that referenced this pull request Jun 23, 2026
…ergence ledger

Records the four equivalence findings (D-09 nolabs-ai#1077, D-01 nolabs-ai#1048/nolabs-ai#1091, D-02 nolabs-ai#1151,
D-10 nolabs-ai#1132), two won't-sync findings (D-05 nolabs-ai#1192, D-04 nolabs-ai#1199), and the one
deliberate fork-divergence (D-07 nolabs-ai#1197, fix 0c08e5d) with their guard-test fn
names so future syncs expect the Cluster F divergence and never blind-cherry-pick
the tls_intercept/RouteSelection/TlsInterceptIntent hunks (D-11).

Signed-off-by: Oscar Mack Jr <oscar.mack.jr@gmail.com>
klassm pushed a commit to klassm/nono that referenced this pull request Jul 3, 2026
…abs-ai#1048) (nolabs-ai#1091)

The TLS intercept upstream leg (used for credential injection) always
connected directly to the target host, ignoring the configured
upstream_proxy / external_proxy. In enterprise environments where the
target is only reachable through a corporate proxy, this caused the
intercepted request to hang or fail.

- Added InterceptUpstreamProxy struct and upstream_proxy field to
  InterceptCtx so the intercept code can select
  UpstreamStrategy::ExternalProxy when a proxy is configured.
- server.rs now checks the bypass matcher and passes the resolved
  proxy info into InterceptCtx, mirroring the existing transparent
  CONNECT path.
- Auth-not-implemented check (config.auth.is_some()) fails early
  and consistently with the external proxy handler.
- 3 unit tests for strategy selection (ExternalProxy vs Direct).

Signed-off-by: Caio Silva <caio@cdcs.dev>
Co-authored-by: Luke Hinds <lukehinds@gmail.com>
Co-authored-by: Aleks <121458075+SequeI@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 nono-proxy size/medium

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants