fix(devin): bind tenant hosts to the transmitted key and sanitize Connect trailers - #5574
Conversation
Connect-RPC EOS trailers propagated upstream-controlled error.message text into adapter error events, API responses, and diagnostic logs, and that text can reflect the credential carried by the request. Parse trailers with an allowlist for known Connect codes, keep the raw message inside the parser for quota/opaque-denial detection, and throw content-free errors carrying only the allowlisted code and hex trace id. (cherry picked from commit 50eab5a) (cherry picked from commit 6efc759)
The trailer suppression left the thrown CloudChatError without the upstream "limit will reset in N" sentence that streamChatEventsWithResetRetry scraped for its cooldown, so a stated-reset 429 failed immediately instead of waiting out the quota window. Parse the delay inside the trailer parser while the raw text still exists and carry it as a typed retryAfterSeconds field; the retry prefers that field and keeps the message scrape for errors whose text still carries a hint. Adds a regression test that drives the real Connect EOS trailer path. (cherry picked from commit 7b7a8b8) (cherry picked from commit b841abd)
The module-mock spy leaked into sibling test files in the same bun test process, so devin-stated-reset-retry and devin-stated-reset-hardening saw a stubbed helper. Drive the real runTurn with a stubbed fetch returning a Connect EOS-trailer refusal instead: the asserted signals are the surfaced error text and the physical GetChatMessage count, which fail fast under the 5s abort signal if the adapter's maxWaitMs: 0 pin is removed or weakened. Register the file in the test layout map and fixture. (cherry picked from commit 1abc035) (cherry picked from commit 2c4bf00)
Reconcile #5455 zero-wait coverage with #5143 content-free Connect diagnostics: retain 429 and generated retry timing while proving reflected credentials do not escape and only one inference send occurs. Exercise a matching deprecated alias credential through tenant dispatch and the same refusal path. Preserve the zero wait pin, host allowlist, credential binding, and typed reset field. Document the safe typed retry delay and opt-in helper replay contract. (cherry picked from commit 6f2901b)
…tted key The consolidated fix bound alias-slot tenant hosts to the transmitted key but still returned the active literal slot's host for any key. Devin keeps several accounts per provider id and the request path injects the admitted account's token, which need not be the active one, while a configured key, a forwarded bearer, or a test token is not stored at all. Each of those could inherit another account's EU or FedStart tenant. resolveDevinApiServer now looks up the stored credential whose access token is exactly the transmitted key, in the configured provider id first and then its deprecated alias, active account first. Only that owner's validated host is used; an owner with an unusable host, an unmatched key, or no key falls back to the validated configured base URL and then the US default. The lookup uses getCredential and listAccounts, which store mocks elsewhere in the suite leave real. Tests pass the owning key where a stored tenant is expected, so the invalid alias-host case now reaches the host validator. New cases cover a mismatched literal slot, an alias owner beside an occupied literal slot, a key neither slot owns, a missing key, a non-active owning account, and a configured key at the adapter boundary. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
✅ Deterministic PR hygiene checks passed. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Repository: lidge-jun/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (15)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
리뷰 · 우선순위 74 / 80이 PR은 Devin(코그니션)으로 나가는 요청이 잘못된 테넌트(EU/FedStart 같은 다른 집 주소)로 가거나, 에러 글에 키 조각이 섞여 밖으로 나가던 구멍을 막습니다. 기여자 PR #5541의 여덟 커밋을 그대로 가져오고, 메인테이너가 한 곳을 더 고쳤습니다. 예전에 Connect 스트림 끝 트레일러의 어댑터는 호스트 고르기도 바뀌었습니다. 저장소에 적힌 테넌트 주소는, 지금 보내는 키와 라인 - 메인테이너의 판단이 필요한 지점 #5541을 이 PR 머지와 함께 닫을지, 아니면 #5541에 “superseded by #5574”만 남기고 수동으로 닫을지. 보안 체크리스트는 독립 리뷰 댓글이 달린 뒤에 체크할지, 머지 전에 누가 서명할지. 너의 추천 CI test shard가 초록이면 머지해도 됩니다. 합친 뒤 #5541은 닫으세요. types.ts/config.ts 쪼개기나 미리보기 배포 이야기는 이 PR과 무관합니다. 이 댓글은 grok-bot이 작성했습니다 |
|
Independent security review of the repaired diff (origin/dev..e24ef95, merged with dev at 22ac683): PASS, no findings. Reviewed scope and conclusions:
Local validation was not run for this lane; hosted CI on the exact head is the verifier. |
2 participants
Summary
Devin requests could reach the wrong tenant, and Connect error trailers could echo credential-bearing upstream text. This carries the consolidated contributor fix from #5541 and adds one maintainer repair for the part it left open.
Carried from #5541 (eight commits, original authorship kept; it already consolidates #5143 and #5455, which are not re-applied):
retryAfterSeconds, so the shared retry helper still schedules bounded replays for callers that opt in.maxWaitMs: 0, so a provider 429 surfaces immediately and the admitted turn releases its shared capacity instead of sleeping while holding it.Maintainer repair (e24ef95): the literal credential slot still lent its active account's EU or FedStart host to any key. Devin keeps several accounts per provider id, and the request path injects the admitted account's token, which need not be the active one; a provider-configured key or forwarded bearer is not stored at all.
resolveDevinApiServernow takes a stored tenant host only from the account whose credential access equals the transmitted key, searching the configured provider id and then its deprecated alias, active account first. Otherwise it uses the validated configured base URL or the US default.Before: a configured key with a FedStart login in the
devinslot dispatched to the FedStart host. After: it dispatches to the configured base URL. A pooled non-active EU account still reaches its own EU host.The invalid alias-host test now passes the owning key, so it reaches the host validator instead of stopping at the ownership check.
Co-authored-by: luvs01 27862058+luvs01@users.noreply.github.com
Verification
Checklist