Skip to content

feat(proxy): add per-provider HTTP proxy overrides - #3901

Draft
jingzxy wants to merge 2 commits into
lidge-jun:devfrom
jingzxy:codex/provider-proxy-phase1-ready
Draft

feat(proxy): add per-provider HTTP proxy overrides#3901
jingzxy wants to merge 2 commits into
lidge-jun:devfrom
jingzxy:codex/provider-proxy-phase1-ready

Conversation

@jingzxy

@jingzxy jingzxy commented Sep 7, 2026

Copy link
Copy Markdown

Summary

  • Add an optional per-provider proxy setting for explicit HTTP(S) proxy routing.
  • Apply the provider route to core HTTP/SSE and provider-owned outbound requests while preserving the existing global proxy / noProxy behavior when the field is omitted.
  • Validate unsupported values fail closed and keep proxy credentials out of dashboard and log-facing surfaces.
  • Document the provider configuration and behavior supported by this PR.

Related to #2894

Verification

  • bun run typecheck
  • bun test tests/lib/provider-egress.test.ts tests/providers/provider-egress-outbound.test.ts tests/providers/provider-outbound.test.ts tests/providers/provider-outbound-private-network.test.ts tests/responses/provider-egress-fetch.test.ts tests/providers/provider-config-validation.test.ts tests/server/management-provider-validation.test.ts (180 pass, 0 fail)
  • bun run privacy:scan
  • GitHub comparison: 2 commits, 16 changed files, automatically mergeable into dev
  • Full repository suite is not attested locally; this PR is intentionally opened as Draft.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults. Maintainer security review is requested for proxy credential handling and the management DTO boundary.

Review readiness checklist

This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:

  • All CI tests are green on my local testing.

  • I pushed my PR to the latest dev commit.

  • I resolved all correct Codex and CodeRabbit findings.

  • My PR is ready for review.

@coderabbitai

coderabbitai Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

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.

@github-actions github-actions Bot added the intake: hygiene-blocked Deterministic PR hygiene checks failed label Sep 7, 2026
@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

⚠️ Deterministic hygiene checks failed.

  • unsponsored_surface — This changes an authentication, workflow, release-automation, or dependency surface. MAINTAINERS.md requires security review for these; ask a maintainer to apply maintainer-sponsored once they have reviewed it. Paths: src/server/auth-cors.ts.

@github-actions github-actions Bot added the enhancement New feature or request label Sep 7, 2026
@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

⏳ DRAFT

  • hygiene: unsponsored_surface.

What to do

  • Fix unsponsored_surface — This changes an authentication, workflow, release-automation, or dependency surface. MAINTAINERS.md requires security review for these; ask a maintainer to apply maintainer-sponsored once they have reviewed it. Paths: src/server/auth-cors.ts.
  • Tick all four boxes in the PR description once you're done (currently 0/4).

Review readiness checklist

  • ⬜ All CI tests are green on my local testing.
  • ⬜ I pushed my PR to the latest dev commit.
  • ⬜ I resolved all correct Codex and CodeRabbit findings.
  • ⬜ My PR is ready for review.

0/4 boxes ticked.

This pull request was already a draft. Its draft status will be preserved after every issue above is resolved.
@jingzxy Tick the boxes once your local CI is green, your branch is on the latest dev commit, and every correct Codex and CodeRabbit finding is resolved.

@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 52 / 80

설명

이 PR은 관련 이슈 #2894의 Phase 1A 조각이다. 서버 전역 proxy / noProxy / proxy: "auto"만으로는 “이 제공자만 다른 HTTP(S) 프록시로 보내고, 나머지는 지금처럼”을 표현할 수 없어서, providers.<name>.proxy 선택 필드를 추가한다. 값이 없으면 기존 전역 라우팅을 그대로 물려받고, 있으면 그 제공자의 핵심 HTTP/SSE·provider outbound 요청에 명시 프록시를 붙인다. direct / auto / null / 빈 문자열 / SOCKS / 잘못된 URL은 설정·런타임 모두에서 거절(fail closed)한다. 프록시 URL에 들어간 자격 증명은 로그 sanitize와 대시보드 DTO에서 빼도록 PROVIDER_CONFIG_FIELD_POLICYproxy: "redacted"를 넣었다.

닿는 면이 넓다. 새 src/lib/provider-egress.ts(해석·routeKey·로그 sanitize), src/lib/provider-outbound.ts(명시 프록시가 있으면 상속 NO_PROXY 우회를 끄고 Bun fetchproxy를 전달), src/server/responses/fetch-helpers.tsproviderFetch(명시 라우트면 prebuilt Request·caller-owned fetch executor는 거절, WS 경로 전에 HTTP/SSE로 보냄), src/types/provider.ts·src/config.ts·src/config/provider-validation.ts·src/server/auth-cors.ts 검증/관리 DTO, providers 문서, 테스트 여러 개(약 +800/-9, 파일 16개)다. 작성자 로컬로는 typecheck·privacy:scan과 지정 egress/outbound/validation 테스트 180 pass를 적었고, 전체 스위트는 증언하지 않은 채 의도적으로 draft로 열었다. 지금 dev HEAD는 f802f7112(#3892 흡수)라서, 이 브랜치가 그 tip 위에 있는지는 랜딩 전에 rebase/재검증이 필요하다.

#2894 본문이 요구하는 SOCKS5·제공자별 “direct”·Omniroute식 3단 선택까지는 이번 조각에 없다. Phase 1A라고 문서·에러 메시지에 명시해 둔 점은 정직하다. 다만 hygiene가 src/server/auth-cors.ts 변경으로 unsponsored_surface / intake: hygiene-blocked를 걸었고, 체크리스트는 0/4, 보안 체크박스도 “메인테이너 보안 리뷰 요청”으로 비어 있다. types/config 분할 캠페인 한가운데 src/types/provider.tssrc/config.ts에 필드를 동시에 더하는 PR이라, 분할 열차와 겹치면 rebase보다 닫고 다시 여는 쪽이 맞을 수 있다. 지금은 분할에 의해 즉시 무효로 보이지는 않지만, 표면이 큰 enhancement draft로 mid 점수가 맞다.

라인 / 경로 관찰

src/lib/provider-egress.ts / resolveProviderEgress - 명시 프록시가 있으면 대상 URL 파싱 실패도 fail closed로 둔 것은 좋다. providerEgressRouteKey는 호스트·포트·해시만 남겨 자격 증명을 키 문자열에 안 넣는다. Phase 1A에서 direct/auto를 막아 둔 이유가 에러 문자열에 드러나 있어 운영자가 #2894 기대를 오해할 여지를 줄인다.

src/lib/provider-outbound.ts - 명시 프록시일 때 noProxyBypass = false로 전역 NO_PROXY를 무시한다. 문서에도 “verified HTTP/HTTPS fetch paths에서 전역 NO_PROXY보다 우선”이라고 적혀 있다. 의도된 계약이면 유지하되, “이 호스트는 직접 가라”는 전역 예외가 제공자 오버라이드에 삼켜지는 보안·운영 트레이드오프라서 스폰서 리뷰에서 한 번 고정해야 한다.

src/server/responses/fetch-helpers.ts / providerFetch - wrapped가 pacing 전에 resolveEgress를 한 번 하고, httpFetch/unpaced에서 또 해석한다. 실패는 빨리 나는 편이지만 이중 호출이다. prebuilt Request + proxy, custom fetch executor + proxy를 명시적으로 throw하는 가드는 런타임 한계를 숨기지 않아서 좋다.

src/server/auth-cors.ts - providerProxyConfigError 연결과 proxy: "redacted"는 대시보드 응답에 프록시 비밀이 안 나가게 한다. 테스트도 safe/editor DTO에 proxy·자격 증명이 안 실리는지, editor round-trip이 redacted 필드를 거절하는지 본다. 반대로 GUI 에디터로 provider.proxy를 넣고 저장하는 경로가 Phase 1A에서 막힌다는 뜻이라, 설정 파일(또는 redacted가 아닌 management 경로)로만 넣는다는 운영 제약을 문서에 더 분명히 써야 한다.

src/config.ts + src/types/provider.ts - zod는 proxy: z.string().min(1).optional()이고, 거부 세부(direct/auto/scheme)는 superRefine의 providerProxyConfigError에 있다. 이중 검증은 맞지만, types/config 분할 캠페인 중이라 이 두 파일을 건드리는 큰 PR은 타이밍 리스크가 있다.

hygiene / intake: hygiene-blocked - auth-cors.ts가 unsponsored_surface다. MAINTAINERS 규칙상 보안 리뷰 후 maintainer-sponsored 없이는 merge 불가. 작성자도 보안 리뷰를 요청해 두었다.

PR 상태 - draft, 체크리스트 0/4, mergeable UNKNOWN, 전체 스위트 미증언. 관련 #2894의 SOCKS·per-provider direct는 후속 Phase로 남아 이 PR만으로 이슈를 닫으면 안 된다.

메인테이너의 판단이 필요한 지점

  • Phase 1A(HTTP(S) 명시 오버라이드만, GUI redacted, NO_PROXY 무시)를 지금 스폰서해 dev에 넣을지, SOCKS/direct UX가 더 나온 뒤 한 번에 받을지
  • 명시 provider proxy가 전역 NO_PROXY를 이기는 계약을 공식 보안 모델로 받아들일지
  • types/config 분할 열차와 충돌이 보이면 이 PR을 close-don't-rebase 할지, 분할 이후에 필드를 다시 제출하게 할지
  • maintainer-sponsored를 붙이기 전에 요구할 최소 증거(전체 CI, latest dev rebase, management로 proxy를 넣는 허용 경로 문서화)

너의 추천
지금은 merge하지 말고 draft로 유지한다. 방향(#2894의 per-provider 조각)과 fail-closed·redaction·테스트 밀도는 좋지만, unsponsored_surface·0/4 체크리스트·대형 egress 표면·NO_PROXY 우선 계약이 남아 mid-train 직전이다. 스폰서하려면 (1) 최신 dev(f802f7112 이후)에 rebase하고, (2) 보안 리뷰로 NO_PROXY 우선·자격 증명 redaction·auth-cors DTO를 확인한 뒤 maintainer-sponsored를 붙이며, (3) GUI로는 못 넣고 파일/허용 API로만 설정한다는 문장을 providers 문서에 더 명확히 하고, (4) 체크리스트·CI를 채운 다음 랜딩한다. #2894는 SOCKS/direct가 오기 전까지 open으로 두고, 이 PR 설명에 Phase 1A 범위만 닫힌다고 적어 둔다. 분할 캠페인이 이 필드를 삼키면 rebase하지 말고 닫는다.

이 댓글은 grok-bot이 작성했습니다

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request intake: hygiene-blocked Deterministic PR hygiene checks failed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants