fix(config): start when proxy settings hold values the schema never constrained - #2967
Conversation
…onstrained
The top-level config schema ends in `.passthrough()` and declares neither `proxy`
nor `noProxy`, so whatever is on disk reaches `applyProxyEnv` verbatim. It called
string-only methods on those values, and it runs at every process entry point that
makes outbound provider requests. A number, null, or object therefore did not
degrade proxy behaviour -- it threw before the server could start.
Four shapes were confirmed against the current code:
noProxy: ["ok", 42] TypeError: entry.trim is not a function
noProxy: ["ok", null] TypeError: null is not an object
noProxy: [{a: 1}] TypeError: null is not an object
proxy: 42 TypeError: value.match is not a function
Ignore unusable values instead of throwing: they cannot express a routing intent,
and refusing to start is a worse answer than starting without them. Array
filtering is per-element so one bad entry no longer discards the operator's
usable hosts, and loopback exclusions stay intact in every case.
Malformed proxy settings previously degraded startup silently, allowing direct egress or unexpected proxy traversal without an operator signal. Emit privacy-safe warnings once per process for discarded proxy values, noProxy values, and invalid noProxy elements while preserving graceful fallback behavior.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Warning Review limit reachedNext included review available in 23 minutes. View limit detailsLimit details: You’ve used all 10 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
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 |
|
✅ Deterministic PR hygiene checks passed. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 010c1f7030
ℹ️ 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".
| if (!proxy) { | ||
| if (rawProxy !== undefined) warnProxyConfigDiscardOnce("proxy"); | ||
| return; |
There was a problem hiding this comment.
When proxy is malformed or references an unset environment variable but HTTP_PROXY/HTTPS_PROXY is inherited, this early return leaves that inherited proxy authoritative without processing an otherwise valid config.noProxy or adding the promised loopback exclusions. Requests to an internal provider or localhost can consequently traverse the inherited proxy with their authorization headers despite the configured bypass; this becomes newly reachable because the commit changes this case from a startup failure to continued operation. Skip assigning the malformed proxy, but continue through the NO_PROXY merge whenever an inherited proxy is effective.
AGENTS.md reference: AGENTS.md:L330-L332
Useful? React with 👍 / 👎.
리뷰 · 우선순위 71 / 80설명 이 PR은 수정은 타입을 먼저 가립니다. 기반은 라인 - 문제 src/config.ts src/config.ts src/config.ts - 스키마에 tests/proxy-env.test.ts - 원문 미노출·비문자열·배열 혼합 원소 보존을 잘 잠갔습니다. 메인테이너의 판단이 필요한 지점
너의 추천
이 댓글은 grok-bot이 작성했습니다 |
2 participants
Summary
Carries #2947 by @luvs01, plus one repair commit. A
proxyornoProxyvalue the schema neverconstrained crashed startup instead of being handled.
Opened as a maintainer PR because pushing the repair to the contributor branch reset its
review-readiness checklist and returned it to draft; that attestation belongs to the author. The
cherry-pick preserves @luvs01's authorship.
Carries #2947. Close it as carried once this lands.
The repair
Starting gracefully is right. Doing it silently is not: as originally written, an explicitly configured
but invalid
proxybecame direct outbound traffic with no signal, and invalidnoProxyentrieswere dropped so destinations the operator meant to bypass would traverse the proxy. That is a
fail-open network-policy change an operator has no way to notice.
Each discarded value now emits a privacy-safe warning, once per process, naming what was dropped and
what the effective behavior is. The raw value is never logged — proxy URLs can embed credentials, and
the test asserts it never appears in the output.
Verification
Based on
dev@dca16949b.bun test tests/proxy-env.test.ts tests/config.test.ts→ 174 pass, 0 failbun run privacy:scan→ passbun x tsc --noEmit→ cleanChecklist