feat(security): use opaque browser admin sessions - #788
Conversation
|
Warning Review limit reachedNext included review available in 15 minutes. View limit detailsLimit details: You’ve used the included review 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: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthrough관리자 브라우저 세션을 위한 서버 측 opaque 세션과 쿠키 인증을 추가했습니다. 관리자 UI와 CLI가 새 세션 계약을 사용합니다. 상태 변경 요청에는 Origin 검증을 적용합니다. 세션 만료, 수 제한, 폐기 및 보안 속성을 테스트와 문서에 반영했습니다. Changes관리자 세션 인증
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: 🔵 Low · up to Failed admin-session attempts can leave the raw bearer credential visible in the browser form until manually cleared. The risk is localized and straightforward to fix by clearing the field in all outcomes; merge is reasonable with explicit owner follow-up. Sequence Diagram(s)sequenceDiagram
participant 운영자
participant 관리자콘솔 as 관리자 콘솔
participant 서버 as contextual_orchestrator.server
participant 세션저장소 as SecurityConfig 세션 저장소
운영자->>관리자콘솔: bearer 입력
관리자콘솔->>서버: POST /admin/session
서버->>세션저장소: bearer 검증 후 opaque 세션 저장
서버-->>관리자콘솔: HttpOnly 세션 쿠키 반환
관리자콘솔->>서버: 쿠키 포함 관리자 API 요청
서버->>세션저장소: 세션 활성 상태 확인
세션저장소-->>서버: 세션 권한 반환
서버-->>관리자콘솔: 관리자 API 응답
운영자->>관리자콘솔: 세션 종료
관리자콘솔->>서버: DELETE /admin/session
서버->>세션저장소: 세션 폐기
서버-->>관리자콘솔: 삭제 쿠키 반환
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
Exact current head |
|
Review pass on exact current HEAD
@opencode-agent please review this exact HEAD. |
|
Current-head proof for
The PR remains protected |
|
@devin review exact current HEAD |
|
Exact-head local verification for
Hosted required Checks and an independent protected approval remain the merge gates; no bypass or self-approval used. |
|
@devin review exact current HEAD `d670601e61ca181a7b7134c7d0219f310334ff05` against `main`; focus on session lifecycle, CSRF/origin enforcement, cookie scope, auth-token fallback, and fail-closed behavior. |
|
@devin review exact current HEAD d670601. The historical Strix failure for this SHA was traced to NVIDIA NIM rate-limit/connection exhaustion before a structured report; the deleted workflow cannot be rerun directly. Do not treat that provider outage as a source finding; revalidate the current diff and request fresh central required workflows for this exact head. |
|
Strix failure RCA for exact HEAD
This is a Strix/provider false positive plus backend exhaustion, not a source credential finding. Re-running the failed Strix job now; no gate weakening or bypass. |
|
Exact-head security/admin/CLI suite |
|
Exact-head local verification for
@opencode-agent please review this exact current head against |
|
@opencode-agent Review exact current HEAD only. Verify opaque admin-session lifecycle, origin/CSRF enforcement, cookie scope, token fallback, and fail-closed behavior using same-head Checks. Publish a formal verdict; do not push unrelated changes. |
|
@opencode-agent Review exact current HEAD d670601 only. Verify opaque admin-session lifecycle, origin/CSRF enforcement, cookie scope, token fallback, and fail-closed behavior using same-head Checks. Publish a formal verdict; do not push unrelated changes. |
|
Exact-head verification for 8000659:
@opencode-agent review exact current HEAD 8000659 against main; confirm session scope isolation, cookie flags, TTL/bounded eviction, revocation, and same-origin protection for state-changing cookie requests. |
|
Exact current HEAD was revalidated: opaque admin session, CLI authentication, and security-hardening suite passed 32 tests; Ruff, compileall, and diff checks passed. @opencode-agent review this exact SHA only and report any reproducible issue against the current code. |
|
Exact current HEAD |
|
Current-head validation for
@opencode-agent Review only exact current HEAD |
|
Exact current HEAD 8000659 was revalidated for the buyer-visible admin security surface.
Please review and run protected Checks for this exact HEAD only. |
Exact-head validation — PR #788
@opencode-agent please review only exact current HEAD |
opencode-agent
Bot
added
area: security
80fb123 to
d1a579c
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Merge-gate evidence (2026-08-24): Review fixes applied and pushed (see commits); all required checks green on current head except |
d1a579c to
d5ebe73
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| }); | ||
| state.last = await res.json(); | ||
| state.recent_workflow_runs = [state.last, ...(state.recent_workflow_runs || [])].slice(0, 8); | ||
| await refreshAnalytics(); |
There was a problem hiding this comment.
🔍 Readiness panels no longer refresh after simulate or evaluation
simulate() (contextual_orchestrator/admin.py:1588) and runEvaluation() (contextual_orchestrator/admin.py:1627) drop the previous refreshReadiness() call. Readiness reports include evaluation-usage metrics, so those panels now stay stale until a full reload. This may be an intentional cut of ~24 authenticated requests per action, or an accidental regression.
Was this helpful? React with 👍 or 👎 to provide feedback.
| scope = ( | ||
| "admin" | ||
| if path in {"/admin/simulate", "/api/v1/evaluation_runs"} | ||
| or path.startswith("/api/v1/agent_pools/") | ||
| else "inference" | ||
| ) |
There was a problem hiding this comment.
🔍 evaluation_runs POST now requires admin scope
The scope selector now routes /api/v1/evaluation_runs to admin scope, whereas it was previously inference. This is needed so the admin session cookie can drive runEvaluation, but it is a contract change: existing clients using an inference bearer to start evaluation runs will now receive 401.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Merge-gate evidence (2026-08-24): Deep diff review + fixes applied; all required checks green on current head except strix (org-wide NVIDIA NIM quota exhaustion — external provider-capacity blocker; serialization fix in ContextualWisdomLab/.github#1297). Full local suite green on this head. |
…earer (#844) * fix: session-cookie requests partition the response cache without a bearer Merging #772 (distributed cache) with #788 (opaque admin sessions) left a gap: _cache_partition required a bearer header, so every state-changing admin POST from a cookie-authenticated operator failed with 401 before any handler logic ran. An active opaque session id now derives the partition (random per login, so cross-session reuse stays impossible), and a regression test locks the flow in. * test: join session cache test server thread
) * fix: separate liveness from readiness probes * fix: keep readiness backend identifiers private * fix: make trace endpoint authorization explicit * fix(security): require trace purpose authorization (#781) * fix: require trace purpose authorization * fix: apply trace purpose policy across evidence routes * test: restore lint-clean contract baseline --------- Co-authored-by: opencode-agent[bot] <219766164+opencode-agent[bot]@users.noreply.github.com> * docs: reserve unique liveness and trace ADR numbers * fix: preserve trace authorization on read endpoints * test: harden trace authorization regression fixtures * fix: adopt opaque admin-session authorize docstring from merged #788 * fix: register trace disclosure purpose --------- Co-authored-by: opencode-agent[bot] <219766164+opencode-agent[bot]@users.noreply.github.com>
1 participant
Closes #116
Buyer-visible outcome
The embedded admin console no longer needs to reuse a long-lived admin bearer as a browser cookie. POST /admin/session validates once and mints a bounded opaque server-side session; DELETE revokes it. Session cookies are HttpOnly, SameSite=Strict, Secure by default, admin-scope only, and state-changing cookie requests require same-origin Origin/Host evidence.
Review repair
The cookie-authenticated DELETE /admin/session path now uses the same shared origin-validation guard as other admin state-changing routes, so cross-origin logout cannot bypass the documented CSRF boundary.
Current-head verification
8000659b7dd299c2564d0d50bbea679cf0bb3810server.pydocstring coverage: 100%vsZMd8WAv42HDRgcZuNcWkand APA 7 referencesProduction keeps Secure cookies. Local HTTP requires the explicit
--insecure-admin-session-cookieopt-in. No bearer or business PII is masked into a substitute; access is protected by purpose, expiry, revocation, and audit-compatible boundaries.Hosted Checks are being regenerated for this exact head. Keep normal protected approval and exact-head Checks; do not bypass.
Summary by CodeRabbit
새 기능
문서