Conversation
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks 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 |
Contributor
|
✅ Deterministic PR hygiene checks passed. |
Contributor
✅ READY
Hygiene✅ Deterministic PR hygiene checks passed. |
Owner
리뷰 · 우선순위 46 / 80이 PR은 “설정 파일에는 프록시가 없는데, 프로그램을 실행한 쪽에 이미 프록시가 있는 경우”를 고칩니다. 예전에는 그때 라인 라인 라인 PR 본문 체크리스트 - 아직 Draft입니다. 로컬 CI, 최신 메인테이너의 판단이 필요한 지점 설정 너의 추천 고치려는 방향은 맞습니다. 루프백이 아닌 주소는 프록시를 그대로 타는지 한 번만 더 확인하고, 설정 이 댓글은 grok-bot이 작성했습니다 |
Collaborator
Author
|
Review feedback applied on |
3 tasks
lidge-jun
added a commit
that referenced
this pull request
Sep 22, 2026
Review of the carried #5430 fix: Bun's native fetch reads a non-empty lowercase no_proxy before NO_PROXY, so with HTTP_PROXY and an inherited no_proxy the loopback entries written to NO_PROXY never applied and local calls could still go through the proxy. Merge the same configured and loopback entries into an inherited no_proxy, keeping its own entries. The non-loopback SOCKS test now uses a local proxy that drops every connection and an IP-literal target, so it no longer waits on DNS.
lidge-jun
added a commit
that referenced
this pull request
Sep 22, 2026
Security review of the carried #5430 change: on the no-config.proxy path loopback names were added to NO_PROXY for any inherited proxy, and both matchers treat entries as domain suffixes, so "localhost" also sent any *.localhost name direct, which need not resolve to loopback. Add the loopback entries only when an inherited SOCKS proxy owns HTTP(S) traffic; that proxy is applied by the in-process matcher, which now treats a bare localhost or IP-literal entry as one host (a leading dot still means subdomains). An inherited HTTP(S) proxy is Bun's own and is left as it was. Tests cover *.localhost staying on the SOCKS proxy, the no-widening cases, and the exact-match rules.
Owner
|
Carried into #5598 as a cherry-pick (3746991, 7dd72d1), with your authorship kept. Security review narrowed the bypass. The in-process SOCKS matcher now treats a bare loopback host as exact, while Bun applies an inherited HTTP(S) proxy with suffix matching. So the full loopback list is added only when SOCKS is the sole inherited proxy, and only the loopback addresses when Bun applies an inherited HTTP(S) proxy. Thank you, @luvs01. Closing in favor of #5598. |
lidge-jun
added a commit
that referenced
this pull request
Sep 23, 2026
…ck proxy, OAuth, remote hub docs) (#5598) * fix(kiro): avoid mixed-script estimator allocations (cherry picked from commit 24c3cd3) * test(kiro): register kiro-wire-estimate in test layout (cherry picked from commit e796b5c) * fix(crash-guard): count unparenthesized JS frames as real throw sites isBenignAbortTeardown only recognised parenthesized "(file:line:col)" frames, so a genuine TypeError whose stack carries "at /abs/x.ts:1:2", "at file:///x.ts:1:2" or a Windows drive path was folded into the rate-limited benign-teardown summary. Scan every "at ..." frame and treat any non-native line:col location as a JS source frame. Hidden JSC fields (sourceURL/line/column) deliberately do not veto the benign class: Bun can attach them to errors raised from builtin frames, and the benign summary already records them through diagnose(). Reimplements #5286. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> * fix(proxy): keep loopback traffic outside inherited SOCKS (cherry picked from commit 3746991) * fix(proxy): pin non-loopback SOCKS ownership and document the ambient noProxy scope (cherry picked from commit 7dd72d1) * fix(oauth): preserve hashes in command code callback JSON (cherry picked from commit 225eb81) * test(oauth): drive Command Code callback JSON through the shared submit path The provider parser, not the shared code#state gate, owns the state check for pasted Command Code callback JSON. Pin both halves end to end: a wrong-state paste is accepted by submitManualLoginCode, rejected by the provider loop and re-prompted without a whoami call, and the right-state paste keeps "#" inside its fields intact through to the stored key. Follow-up to the carried #5413 fix. * docs(remote-hub): warn about shared-host unauthenticated loopback companion (cherry picked from commit a0233ca) * docs(remote-hub): align the loopback warning with the reference and cover the ported form (cherry picked from commit b00ed1e) * fix(oauth): drop legacy credential backup on destructive mutations auth.json.pre-multiauth copies the whole legacy store for downgrade recovery, but removeCredential/removeAccount left it behind — and a legacy-shaped store re-created it — so logout and account deletion kept a file holding the very refresh tokens the user destroyed. mutateStore gains a removeLegacyBackup option: it skips the one-time create and unlinks the backup after persist. Removal is best-effort (ENOENT ignored, other failures warn) because it runs after the store is persisted — a failed unlink must not report a failed logout for an account that is already gone. A stale uninstall-manifest entry is harmless: removeOwnedConfigState skips missing paths. Covers the destructive-migration, pre-existing-backup, and stale-backup-on-migrated-store cases in oauth-store-multi tests. (cherry picked from commit f456a4f) * docs(structure): note legacy backup removal on destructive auth mutations (cherry picked from commit b00133c) * fix(oauth): only drop the downgrade backup when a credential was actually removed removeCredential and removeAccount passed removeLegacyBackup unconditionally, so a stale or concurrent request that returned "not-found" or false still unlinked auth.json.pre-multiauth and still skipped creating it for a legacy store. That backup is a whole-store copy, so a removal that deleted nothing destroyed downgrade recovery for every provider in it. Decide from the mutation result instead. The decision moves to just after the mutation body, which only edits the in-memory store, so nothing has touched disk when it is taken, and the create and remove paths stay mutually exclusive as before. Cover both no-op results. (cherry picked from commit 160d3ab) * fix(oauth): drop the downgrade backup when a provider is deleted Deleting a provider from the dashboard clears its credentials through replaceProviderAccountSet(name, null), which bypassed the carried logout/account-deletion rule and kept (or first created) the auth.json.pre-multiauth copy of the deleted tokens. Treat clearing a provider that had credentials as destructive; a no-op clear and a non-empty replacement keep the backup. Pin the warning path when the backup cannot be removed after the credential is already gone. Completes the reimplementation of #4949. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> * fix(crash-guard): log a new hidden throw site inside the benign fold window Review of the carried #5286 frame fix found the remaining gap: a benign teardown whose JSC hidden sourceURL/line/column names a different throw site was still folded silently for five minutes, so a distinct fault could vanish between summaries. Log the summary when that hidden throw site differs from the last one logged; repeats of the same site, and errors without one, still fold. Classification is unchanged. * fix(proxy): merge loopback into an inherited lowercase no_proxy Review of the carried #5430 fix: Bun's native fetch reads a non-empty lowercase no_proxy before NO_PROXY, so with HTTP_PROXY and an inherited no_proxy the loopback entries written to NO_PROXY never applied and local calls could still go through the proxy. Merge the same configured and loopback entries into an inherited no_proxy, keeping its own entries. The non-loopback SOCKS test now uses a local proxy that drops every connection and an IP-literal target, so it no longer waits on DNS. * fix(oauth): keep other providers in the downgrade backup on deletion Review of the carried #4949 change: deleting one provider removed the whole auth.json.pre-multiauth file, so a legacy store with several providers lost downgrade recovery for every provider the user kept (the migrated auth.json is unreadable to an older loader). Destructive mutations now remove only the affected provider's entry and delete the file once it is empty. A backup entry that is not a regular file is removed rather than followed, and the rewrite replaces the entry itself without resolving a symlink. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> * fix(oauth): check an explicit #state on a raw Command Code paste Review of the carried #5413 change found the direct CLI prompt path, which does not go through submitManualLoginCode, accepting a raw "key#state" paste whose state did not match: only URL- and query-shaped input was compared. Treat a raw paste with an explicit #state suffix as state-bearing, as the shared submit gate already does; a bare key still needs no state. The end-to-end submit test now observes the login promise from the start so an early failure cannot leave it unhandled. * test: tighten the carried Kiro estimate and remote-hub warning checks Review nits on the carried #5248 and #5444 units: document that the count-based estimator takes non-negative integer counts, pin the pure-CJK and empty-model-id cases against the replacement-string formula, and require the dedicated-host warning to sit inside the danger callout in both locales rather than anywhere in the page. * fix(crash-guard): keep the hidden throw-site read inside the handler Re-review found that reading the hidden JSC fields for the benign fold could throw from an unusual accessor before the rejection was logged, escaping the process unhandledRejection listener. Make the read best-effort, as diagnose() already is, and pin it with a Proxy whose sourceURL getter throws. * fix(oauth): never write the downgrade backup through a link or claim it Re-review of the backup scrub found two gaps. backupLegacyOnce used existsSync, which reports a dangling symlink as absent, and then copied through it, so credentials could land outside the config directory; it now treats any existing entry as occupied and copies exclusively. The scrub rewrite went through the shared atomic writer, which records the path for uninstall, so a backup this install never registered was claimed and later deleted; it now replaces the entry through an exclusive private temp and a rename, leaving ownership unchanged. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> * test(kiro): make the pure-CJK estimate case actually zero-Latin * fix(oauth): rewrite the downgrade backup through the shared writer Re-review: the local temp-and-rename used for the backup scrub skipped what the shared atomic writer guarantees (Windows ACL hardening before the temp holds a byte, an explicit 0600 on POSIX regardless of umask, scrub-and-unlink of a failed temp, the Windows rename retry), which structure/config.md forbids replacing. Add a no-follow variant of that writer that leaves the owner manifest untouched, and use it, so a backup an earlier install left unregistered still stays unclaimed. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> * docs(structure): state when an existing OAuth downgrade copy is rewritten * fix(proxy): scope the inherited-proxy loopback bypass to exact hosts Security review of the carried #5430 change: on the no-config.proxy path loopback names were added to NO_PROXY for any inherited proxy, and both matchers treat entries as domain suffixes, so "localhost" also sent any *.localhost name direct, which need not resolve to loopback. Add the loopback entries only when an inherited SOCKS proxy owns HTTP(S) traffic; that proxy is applied by the in-process matcher, which now treats a bare localhost or IP-literal entry as one host (a leading dot still means subdomains). An inherited HTTP(S) proxy is Bun's own and is left as it was. Tests cover *.localhost staying on the SOCKS proxy, the no-widening cases, and the exact-match rules. * fix(proxy): key the loopback bypass on SOCKS precedence Security re-review: with an inherited SOCKS proxy beside an inherited HTTP(S) proxy the fetch wrapper still applies SOCKS first, so skipping the loopback entries in that case let local requests pass through SOCKS. Add them whenever an inherited SOCKS proxy exists; its exact matcher decides first, so only a request already judged loopback reaches Bun's suffix-matching proxy. Pin the mixed case. * fix(proxy): keep 127.0.0.1 off an inherited HTTP proxy without suffix risk Delta review: with no config.proxy and only an inherited HTTP(S) proxy, loopback calls such as the CLI's own management and health requests to 127.0.0.1 could go to that proxy. Add the loopback addresses there (a URL host ending in a numeric label parses as IPv4, so Bun's suffix matching cannot widen them) but not "localhost", which Bun would match as *.localhost. An inherited SOCKS proxy keeps the full list, and a process with no inherited proxy is still left untouched. * fix(proxy): add bare localhost only when SOCKS is the sole inherited proxy Security re-review: beside an inherited HTTP(S) proxy, a bare localhost in process-wide NO_PROXY is also read by Bun, whose suffix matching can send an unwrapped fetch for any *.localhost name past that proxy. Write the full loopback list only when an inherited SOCKS proxy is the only inherited proxy; whenever Bun applies an inherited HTTP(S) proxy, add only the loopback addresses. The mixed case keeps 127.0.0.1 direct through the SOCKS wrapper and sends *.localhost to SOCKS. * fix(proxy): never add a bare localhost to an inherited lowercase no_proxy Review: with a configured HTTP(S) proxy and an inherited lowercase no_proxy, which Bun reads first and matches by domain suffix, adding a bare localhost newly let *.localhost names bypass the proxy. The lowercase value now gains the configured entries and the loopback addresses only. * fix(proxy): add only loopback addresses to an inherited lowercase no_proxy Delta review: configured noProxy entries were also copied into an inherited lowercase no_proxy, which Bun reads first and matches by suffix; on dev that inherited value always shadowed them, so a configured "localhost" newly let *.localhost bypass a configured proxy. The lowercase value now gains the loopback addresses only. * test(oauth): assert a cleared provider set as null, as getAccountSet returns Hosted CI shard 1/4 at 00e5111: getAccountSet returns null for a missing provider, so the two new backup tests' toBeUndefined() failed. * fix: bypass inherited HTTP ALL_PROXY for loopback Cover both ALL_PROXY casings and repair review notes for hub docs, Kiro fallback coverage, and OAuth backup comments. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> * fix: keep localhost direct with mixed inherited proxies Force native direct fetch for exact localhost when the inherited SOCKS wrapper is active, and cover both ALL_PROXY casing combinations. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> --------- Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> Co-authored-by: Epinephrine <luvs01@hanmail.net> Co-authored-by: lidge-jun <lidge-jun@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
localhost,127.0.0.1,::1,[::1]) traffic could still be routed through an inherited SOCKS/HTTP proxy. In a proxy-free process, writingNO_PROXYis itself a proxy-env mutation that sandboxed callers reject as a forbidden leak.Description
applyProxyEnvWithnow merges loopback entries intoNO_PROXYwhen the environment already carries proxy state, even when no configured proxy is set; in a proxy-free process it leavesNO_PROXYuntouched.mergeNoProxyEntries, so configurednoProxyentries still precede the unconditional loopback set.structure/config.mddocuments the proxy-free early-return behavior.tests/server/proxy-env.test.tsasserts the proxy-free process is not mutated and that loopback bypass is added when ambient proxy state exists.Testing
bun test tests/server/proxy-env.test.ts: 42 tests pass.