fix(http): gate reqwest proxy env on NODE_USE_ENV_PROXY to match Node#5889
fix(http): gate reqwest proxy env on NODE_USE_ENV_PROXY to match Node#5889jdalton wants to merge 1 commit into
Conversation
perry's node:http/fetch bindings use reqwest, which honors HTTP(S)_PROXY/NO_PROXY by default, so perry routed through the proxy unconditionally. Node's built-in fetch/http ignore those env vars unless NODE_USE_ENV_PROXY=1 (Node 24+). Gate the client builders on that flag (default .no_proxy(), honor env proxy only when set) so perry-ext-http matches Node. Adds a pure unit test for the parsing. Signed-off-by: jdalton <john.david.dalton@gmail.com>
📝 WalkthroughWalkthroughThis PR centralizes Node-conformant proxy handling for reqwest clients in perry-ext-http. A new predicate and helper functions determine whether to disable standard proxy environment variables, apply this policy across the shared HTTP client, per-agent clients, and TLS client, and change build-failure fallbacks to a new default_client() function. ChangesProxy Policy Centralization
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant ClientBuilder as reqwest::ClientBuilder
participant PolicyFn as apply_node_proxy_policy
participant EnvVar as NODE_USE_ENV_PROXY
participant Client as reqwest::Client
Caller->>ClientBuilder: builder() (+ base config)
Caller->>PolicyFn: apply_node_proxy_policy(builder)
PolicyFn->>EnvVar: read env value
alt value == "1"
PolicyFn-->>Caller: builder (standard proxy env used)
else not "1"
PolicyFn->>ClientBuilder: no_proxy()
PolicyFn-->>Caller: builder (proxy env disabled)
end
Caller->>ClientBuilder: build()
alt build succeeds
ClientBuilder-->>Caller: Client
else build fails
Caller->>Caller: default_client()
Caller-->>Client: fallback Client
end
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@crates/perry-ext-http/src/lib.rs`:
- Around line 263-269: The fallback in default_client currently returns
reqwest::Client::new(), which bypasses the Node-conformant proxy policy applied
by apply_node_proxy_policy. Update the error path in default_client so it
rebuilds a client from the same builder with the same proxy settings, or
otherwise fails closed instead of constructing a fresh client that can re-enable
env proxy handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3881cadb-7bd3-4f9b-aecc-9f14f8d06763
📒 Files selected for processing (3)
crates/perry-ext-http/src/agent.rscrates/perry-ext-http/src/lib.rscrates/perry-ext-http/src/tls_client.rs
| /// A default reqwest client with the Node-conformant proxy policy applied. | ||
| /// Used as the fallback when a customized builder fails to build. | ||
| pub(crate) fn default_client() -> reqwest::Client { | ||
| apply_node_proxy_policy(reqwest::Client::builder()) | ||
| .build() | ||
| .unwrap_or_else(|_| reqwest::Client::new()) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant file and nearby helper to understand fallback behavior.
FILE="crates/perry-ext-http/src/lib.rs"
echo "== Outline =="
ast-grep outline "$FILE" --view expanded || true
echo
echo "== Relevant lines =="
sed -n '1,340p' "$FILE" | cat -n | sed -n '1,340p'
echo
echo "== Search for proxy policy helper usage =="
rg -n "apply_node_proxy_policy|default_client|no_proxy|proxy" crates/perry-ext-http/src/lib.rs crates/perry-ext-http -g '!target' || trueRepository: PerryTS/perry
Length of output: 30069
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read only the relevant section if the file is large.
FILE="crates/perry-ext-http/src/lib.rs"
wc -l "$FILE"
sed -n '220,320p' "$FILE" | cat -nRepository: PerryTS/perry
Length of output: 5108
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the helper and any local tests/examples describing the intended proxy behavior.
rg -n "fn apply_node_proxy_policy|apply_node_proxy_policy|NO_PROXY|HTTP_PROXY|HTTPS_PROXY|Client::new\(" crates/perry-ext-http -g '!target' || true
echo
echo "== Cargo metadata for reqwest =="
rg -n '^reqwest\s*=|^name\s*=\s*"reqwest"|^version\s*=\s*"' Cargo.lock crates/perry-ext-http/Cargo.toml Cargo.toml -g '!target' || trueRepository: PerryTS/perry
Length of output: 36239
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="crates/perry-ext-http/src/lib.rs"
# Show the helper and default_client with surrounding context.
sed -n '1,340p' "$FILE" | nl -ba | sed -n '1,340p'Repository: PerryTS/perry
Length of output: 191
🌐 Web query:
reqwest Client::new default proxy env vars no_proxy documentation
💡 Result:
Reqwest clients created via Client::new() (or the default ClientBuilder) automatically detect and use system proxy settings from environment variables [1][2]. When system proxy support is enabled (which is the default), the following environment variables are checked: 1. Proxy Configuration: HTTP_PROXY, http_proxy, HTTPS_PROXY, https_proxy, ALL_PROXY, and all_proxy [1][2]. 2. Exclusion List (no_proxy): The library checks NO_PROXY first, and if that is not set, it checks no_proxy [3][4][5]. The no_proxy settings follow these rules: - Entries must be comma-separated [4][5]. - An entry of * matches all hostnames [4][5]. - Domain names (e.g., example.com or .example.com) match that domain and all subdomains [4][5]. - IP addresses and subnet masks (e.g., 192.168.1.0/24) are supported [4][5]. If you wish to disable this automatic system proxy behavior, you can call .no_proxy() on the ClientBuilder [6][1]. Note that ClientBuilder::no_proxy() clears all proxies and stops automatic system proxy detection entirely; to configure specific proxy exclusions while still using a proxy, you should use the Proxy::no_proxy() method on your specific proxy configuration [6][7].
Citations:
- 1: https://docs.rs/reqwest/latest/reqwest/
- 2: https://deepwiki.com/seanmonstar/reqwest/4.1-proxy-support
- 3: https://docs.rs/reqwest/latest/reqwest/struct.NoProxy.html
- 4: https://github.com/seanmonstar/reqwest/blob/04a216fc/src/proxy.rs
- 5: https://docs.rs/reqwest/latest/src/reqwest/proxy.rs.html
- 6: https://docs.rs/reqwest/latest/src/reqwest/blocking/client.rs.html
- 7: https://docs.rs/reqwest_wasi/latest/reqwest/struct.ClientBuilder.html
Preserve the proxy policy in the fallback. reqwest::Client::new() re-enables reqwest’s default proxy env handling, so this error path can still honor HTTP_PROXY/HTTPS_PROXY/NO_PROXY and diverge from the Node-conformant .no_proxy() policy. Rebuild the same builder here or fail closed instead of returning that client.
🤖 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 `@crates/perry-ext-http/src/lib.rs` around lines 263 - 269, The fallback in
default_client currently returns reqwest::Client::new(), which bypasses the
Node-conformant proxy policy applied by apply_node_proxy_policy. Update the
error path in default_client so it rebuilds a client from the same builder with
the same proxy settings, or otherwise fails closed instead of constructing a
fresh client that can re-enable env proxy handling.
1 participant
Problem
perry's
node:http/node:https/fetchbindings build their clients withreqwest::Client::builder()/Client::new()and never call.no_proxy(). reqwest honorsHTTP_PROXY/HTTPS_PROXY/ALL_PROXY/NO_PROXYby default, so perry routes through the proxy unconditionally.Real Node's built-in
fetch(undici) andnode:http/node:httpsignore those env vars unless--use-env-proxy/NODE_USE_ENV_PROXY=1is set (added in Node 24). Verified on Node v24.10.0 — without the flag,fetch/http.getconnect directly; with it, they route throughHTTP_PROXY.So a program that does
fetch(url)withHTTP_PROXYin the environment behaves differently on perry (uses the proxy) than on Node (ignores it) — a conformance divergence.Fix
Gate the proxy-env behavior on
NODE_USE_ENV_PROXY, matching Node's opt-in. Client builders inperry-ext-httpnow route through a smallapply_node_proxy_policy(builder)helper that calls.no_proxy()by default and only leaves reqwest's env-proxy on whenNODE_USE_ENV_PROXY=1. Applied at the three client-construction sites: the sharedHTTP_CLIENT, per-agent clients (agent.rs), and per-request TLS clients (tls_client.rs), plus a policy-applieddefault_client()fallback.Scope
perry-ext-httponly — thenode:http/https/fetchbindings, where Node conformance matters.perry-ext-axiosand theauditCLI also use reqwest; left as follow-ups since they aren'tnode:*module surfaces.Test
Pure unit test (
proxy_policy_tests) covering the parsing — enabled iff the value is exactly"1", matching Node'sNODE_*boolean-env convention.cargo test -p perry-ext-httppasses.Notes
NODE_USE_ENV_PROXYcoversfetch/http/https, not raw sockets — same as Node.Summary by CodeRabbit