Skip to content

fix(proxy): the proxy notice follows Node's measured rules; tests stop reading the host's proxy env - #221

Merged
PrzemekGalarowicz merged 1 commit into
mainfrom
claude/optimistic-heisenberg-8rw8ke
Sep 25, 2026
Merged

PrzemekGalarowicz merged 1 commit into
mainfrom
claude/optimistic-heisenberg-8rw8ke

Conversation

@PrzemekGalarowicz

Copy link
Copy Markdown
Contributor

What this changes

Plan E of the second batch from the PHARN-01..18 review (findings F11, F27), shipped through /pharn-dev-ship.

F11: the notice was wrong in several cases. Every rule below was measured, not read from docs. The setup: a local proxy that logs CONNECTs, and a client fetching an https URL under env -i, on the official 20.20.2, 21.7.3, 22.20.0, 22.21.0, 22.22.2, 23.11.1, 24.0.0, 24.4.1 and 24.5.0 binaries.

Which variable Node uses. The notice now follows undici's own ?? lookup over the exact spellings: https_proxy ?? HTTPS_PROXY, then http_proxy ?? HTTP_PROXY.

  • Lowercase wins when both cases are set. The old notice named HTTPS_PROXY.
  • An https fetch falls back to the http pair, so HTTP_PROXY alone is now reported. Before, pharn said nothing.
  • A present but empty https_proxy shadows HTTPS_PROXY.
  • A mixed-case name like Https_Proxy is never read on POSIX. It now gets an "ignored spelling" notice; before, it was reported as the proxy in use.

Whether fetch uses it. Where Node lists --use-env-proxy (22.21+, 24.5+):

  • The last --use-env-proxy / --no-use-env-proxy token decides. Either - or _ works; NODE_OPTIONS is read first, then the command line; =false still turns it on.
  • With no token, only NODE_USE_ENV_PROXY=1 counts.
  • On 24.0–24.4 the flag doesn't exist yet, but any non-empty NODE_USE_ENV_PROXY turns it on. The old notice said "this Node has no support".
  • An opt-out names --no-use-env-proxy, instead of asking for the variable already set.

Only the notice changes. Network behaviour is Node's own.

F27: host-dependent tests. tests/setup/hermetic-env.ts (vitest setupFiles) deletes the proxy variables and NODE_OPTIONS before every test file. Whole-suite runs:

extra environment before after
NODE_USE_ENV_PROXY=1 15 failed 0
HTTPS_PROXY=… exported 1 failed 0
NODE_OPTIONS=--use-env-proxy + HTTPS_PROXY — 0

Type of change

  • feat — new stack option, wizard step, or command capability
  • fix — bug fix
  • docs — docs-only change
  • chore / refactor — tooling or internal restructure, no behavior change

Area(s) touched

src/lib/proxy-env.ts · src/lib/proxy-env-format.ts · tests (+ tests/setup/, vitest.config.ts) · docs/troubleshooting.md · CHANGELOG · .dev/features/proxy-notice-truth/

Checklist

  • Read the existing file(s) before editing; followed the ESM .js-extension import convention.
  • Updated the matching tests: one case per measured row. 21 of the cases fail on the base code.
  • Updated the relevant docs/ page (the troubleshooting proxy section).
  • Preserved the security invariants: proxy values still print only through redactProxyUrl, and a printed variable name case-folds to a proxy name.

Quality gates

  • npm run check passes locally (format:check + lint + typecheck + test): 1554 tests.
  • npm run build succeeds.
  • npm run test:coverage passes (coverage thresholds met).

Notes for the reviewer

  • For the maintainer: LIMITS.md §3a needs your edit. It still says "setting a proxy variable will not change that", which has been untrue since PHARN-12. Every "will not use it" notice cites it. The file is hook-protected (human-only), so this PR does not touch it.
  • Pipeline results: validate exit 0, regress no-regressions, verify PASS, review GREEN with 2 advisory findings (.dev/features/proxy-notice-truth/REVIEW.md).
  • The win32 case-insensitivity follows from process.env semantics. It was not measured, since CI runs no Windows.

🤖 Generated with Claude Code

https://claude.ai/code/session_0199owRmYfskqYQVQrVP679o


Generated by Claude Code

…p reading the host's proxy env

Measured with a CONNECT-logging proxy on the official 20.20.2, 21.7.3,
22.20.0, 22.21.0, 22.22.2, 23.11.1, 24.0.0, 24.4.1 and 24.5.0 binaries.
The notice (PHARN-12) was wrong in several cases; now:

- Which variable: undici's own `??` lookup over the exact spellings —
  https_proxy ?? HTTPS_PROXY, then http_proxy ?? HTTP_PROXY (an https
  fetch falls back to the http pair; an empty https_proxy shadows
  HTTPS_PROXY). Lowercase wins. A mixed-case name like Https_Proxy is
  reported as ignored, never as proxied. HTTP_PROXY alone is reported.
- Whether it is used: where Node lists --use-env-proxy (22.21+, 24.5+)
  the LAST --use-env-proxy / --no-use-env-proxy token decides (`-` or
  `_`, NODE_OPTIONS then the command line), else NODE_USE_ENV_PROXY === '1'.
  On 24.0-24.4 (no flag yet) any non-empty NODE_USE_ENV_PROXY turns it on.
  An opt-out names --no-use-env-proxy instead of the variable already set.
- tests/setup/hermetic-env.ts (vitest setupFiles) deletes the proxy
  variables and NODE_OPTIONS before every test file: under
  NODE_USE_ENV_PROXY=1 the suite had 15 failures, under an exported
  HTTPS_PROXY 1; now 0.

LIMITS.md §3a (human-only) still says a proxy variable changes nothing;
flagged for the maintainer in the PR.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0199owRmYfskqYQVQrVP679o
@coderabbitai

coderabbitai Bot commented Sep 25, 2026

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: b6170c4e-f2f6-4bc3-86b9-382b724ab3c3


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.

@PrzemekGalarowicz
PrzemekGalarowicz merged commit 36ecf70 into main Sep 25, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants