Conversation
github-actions
Bot
added
intake: hygiene-blocked
Contributor
|
Contributor
|
Understand this PR’s impact Explore downstream dependencies and potential security impact with Blast Radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: lidge-jun/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe manual login flow now accepts Command Code JSON callbacks as opaque payloads. It skips shared ChangesCommand Code callback handling
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
Contributor
⏳ DRAFT
What to do
Review readiness checklist
✅ 4/4 boxes ticked. This pull request was already a draft. Its draft status will be preserved after every issue above is resolved. |
Owner
리뷰 · 우선순위 56 / 80Command Code 로그인은 브라우저가 돌려준 JSON을 복사해 붙일 수 있습니다. 같이 쓰는 파서는 이 PR은 제공자가 command-code이고 글이 PR 본문은 잘린 뒷조각이 다른 로그인 상태값과 맞을 수도 있다고 적습니다. 파서는 첫 tests/oauth/oauth-manual-code.test.ts:194 - 새 테스트는 상태값이 맞은 경우만 봅니다. 메인테이너의 판단이 필요한 지점 이 파일은 로그인 경로입니다. 자동 검사가 메인테이너 확인 표시( 너의 추천 수정 방향은 유지하면 됩니다. 공용 파서가 JSON 안의 이 댓글은 grok-bot이 작성했습니다 |
luvs01
marked this pull request as ready for review
September 21, 2026 06:09
luvs01
force-pushed
the
codex/fix-raw-state-check-for-command-code-json
branch
from
September 21, 2026 13:49
b776d62 to
225eb81
Compare
Collaborator
Author
|
The readiness checklist is complete and the branch is rebased on current dev. The remaining gate is |
3 tasks
lidge-jun
added a commit
that referenced
this pull request
Sep 22, 2026
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.
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
code#statesplit on pasted input treated a#anywhere as an OAuth state suffix and rejected provider-specific Command Code JSON when any provider field contained#, breaking the manual-paste fallback for Command Code.Description
src/oauth/login-flow-state.tsadjust the shared manual-submit gate so Command Code manual callback JSON (inputs starting with{) is treated opaque to the genericcode#staterule and exempted from the raw suffix state classification, and makestateBearingignore those Command Code JSON cases.tests/oauth/oauth-manual-code.test.tsthat stashes a manual slot and verifies a Command Code callback JSON containing#characters in provider-controlled fields is accepted and delivered unchanged to the waiting flow.structure/providers-and-adapters.mdto document that Command Code manual callback JSON is validated by the provider parser and remains opaque to the sharedcode#stateparsing rule.Testing
bun test tests/oauth/oauth-manual-code.test.ts, and the modified tests passed (new regression included).bun run structure:checkandbun run privacy:scan, both passed.bun run typecheck, which passed.bun run test; the OAuth-focused changes passed in that run but the full suite exited nonzero due to unrelated environment-sensitive failures in management-auth / Compatibility Lab / bearer-admission and some integration tests in this container; those failures are not related to the OAuth fix.Codex Task
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.