fix(gui): never ask a local dashboard for an admin token - #3496
Conversation
A plain loopback install mints its own GUI session into the served document, so a refused bootstrap there is a Host/Origin misconfiguration — not a missing credential. The dashboard answered it with a password box the user could not fill and that would not have helped if they could: the token is not what was refused. That also contradicted the shipped contract. The Sign-in guide already promises "on the default loopback bind the dashboard never asks for a token", and #3353 is a user who hit the prompt after an upgrade, assumed config loss, and had to have an LLM read the source to learn what the box wanted. `resolveTokenAfter401` now prompts only when the deployment actually requires a typed credential, and reports an actionable notice otherwise. `promptCancelled` is set alongside it so a failing fan-out does not re-enter resolution on every request; `storeSession` already clears that flag, so recovery stays automatic once the address is corrected. The predicate is the bind, not the topology. `runtimeRole` was the obvious signal and it is wrong in both directions: `standalone` + `hostname: "0.0.0.0"` is an operator who deliberately exposed the dashboard and MUST type the token (tests/server-management-auth.test.ts, "a non-loopback binding never issues a GUI session from a forged loopback Host"), while a `hub` on loopback still mints its own session. Gating on `role === "hub"` would have hidden the prompt from exactly the person who needs it. So the server states `isApiAuthRequired` — the same predicate it gates the mint with — beside the role it already emits, and the GUI reads that. A document without the tag falls back to the role, so a hub dashboard still works against a server that predates it. Four cases are covered: a standalone loopback dashboard is never prompted and emits the notice once, an absent tag behaves the same, an exposed standalone bind is still prompted, and a loopback hub is not. The existing prompt-fallback tests in api-auth-deadline and api-auth-memory now declare the non-loopback shape their own comments already described. Closes #3353
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
✅ Deterministic PR hygiene checks passed. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Team Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthroughThe server now declares management-auth requirements in GUI metadata. The GUI uses that metadata and runtime role to control admin-token prompts. When prompting is unavailable, it emits a session-unavailable event. Tests cover suppression, fallback, prompting, and existing authentication flows. ChangesManagement authentication prompt gating
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The dashboard now avoids requesting an admin token when local session bootstrap cannot be restored and instead reports an actionable session notice, while deployments requiring management authentication retain token prompting. The covered behavior is ready to merge. Sequence Diagram(s)sequenceDiagram
participant Server
participant GUI
participant Shell
Server->>GUI: Serve runtime-role and management-auth metadata
GUI->>GUI: Evaluate adminTokenPromptAllowed()
GUI-->>Shell: Dispatch SESSION_UNAVAILABLE_EVENT when prompting is unavailable
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
Full details: Linked Issues checkExplanation The implementation addresses local dashboard authentication behavior in gui/src/api-targets.ts, gui/src/api.ts, src/server/gui-static.ts, and src/server/index.ts. However, linked issue Resolution Add the requested dashboard user guide that explains how to generate, configure, and persist OPENCODEX_ADMIN_AUTH_TOKEN. If this PR is intended only to fix local-session behavior, link it to a separate issue and remove the claim that it closes Full details: Out of Scope Changes checkExplanation The code changes implement authentication-flow behavior and session-unavailable handling, but linked issue
✨ Finishing Touches 💡 1📝 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 |
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
현재 헤드에는 사용자에게 보이는 안내가 실제로 없습니다.
쉽게 말하면 gui/src/api.ts가 opencodex:session-unavailable 이벤트를 보내기만 하고, gui/src 전체에서 그 이벤트를 받아 배너나 토스트를 그리는 production listener는 0개입니다. 검색 결과 이 이름은 api.ts와 테스트에만 존재합니다. 그래서 로컬 bootstrap이 401을 반환하면 토큰 창은 사라지지만, PR 설명의 "actionable notice naming the real cause"는 화면에 나타나지 않고 사용자는 설명 없는 실패만 보게 됩니다.
SESSION_UNAVAILABLE_EVENT를 실제 앱 shell에서 구독해 Host/Origin 주소 문제와 해결 방법을 표시하고, DOM 수준 회귀 테스트로 이벤트 발생이 아니라 안내 렌더링까지 증명해 주세요. 중복 이벤트가 안내를 쌓지 않고, 정상 session 복구 후 안내가 사라지는 것도 함께 확인해야 합니다. 그 뒤 exact-head CI가 통과하면 다시 보겠습니다.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6da1269f87
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!adminTokenPromptAllowed()) { | ||
| state.promptCancelled = true; | ||
| reportSessionUnavailable(plane); |
There was a problem hiding this comment.
When a connected client's shared hub session expires or renewal is refused, the document was served by src/client/machine-listener.ts with role client and no management-auth meta tag, so this page-global check returns false even though the shared target is a non-loopback hub. The shared runtime is then permanently marked promptCancelled, while App.tsx never resets sharedSessionReady; consequently the pairing form remains hidden and all shared /api/* requests continue returning 401 until the page is reloaded. Make the decision target-specific or have the shared-plane event transition the dashboard back to its pairing flow.
AGENTS.md reference: gui/AGENTS.md:L9-L10
Useful? React with 👍 / 👎.
| try { | ||
| window.dispatchEvent(new Ctor(SESSION_UNAVAILABLE_EVENT, { detail: { plane } })); | ||
| } catch { /* a shell that cannot receive the notice must not break the fetch path */ } |
There was a problem hiding this comment.
For the standalone Host/Origin-mismatch path introduced here, this event is the only replacement for the removed admin-token prompt, but no production module in the commit subscribes to SESSION_UNAVAILABLE_EVENT or its literal event name. Thus the advertised actionable notice is never displayed; users only see unrelated page-level load failures and still receive no explanation of how to correct the dashboard address. Add a shell listener that renders a translated, accessible notice and cover the rendered result rather than merely asserting that an event fired.
AGENTS.md reference: gui/AGENTS.md:L14-L18
Useful? React with 👍 / 👎.
|
추가로 Codex가 잡은 connected-dashboard 회귀도 현재 헤드에서 확인했습니다. machine-listener는 role=client만 넣고 management-auth meta는 넣지 않으므로 shared plane 401도 page-global adminTokenPromptAllowed()에서 false가 됩니다. 이때 shared runtime의 promptCancelled만 고정되고 App.tsx의 sharedSessionReady는 false로 전환되지 않아 페어링 폼이 다시 나타나지 않습니다. 결과적으로 새로고침 전까지 shared API가 계속 401입니다. 따라서 앞서 요청한 notice renderer만 추가해서는 부족합니다. 판단을 API plane별로 나누고, shared plane 세션 만료/거부는 기존 pairing 상태로 되돌려야 합니다. sharedSessionReady 전환, 페어링 폼 재노출, 재페어링 후 자동 복구까지 DOM 통합 테스트로 고정해 주세요. |
리뷰 · 우선순위 70 / 80이 PR은 로컬(loopback) 대시보드가 관리자 토큰 입력창을 띄우면 안 되는데, 지금은 띄울 수 있는 UX 사고를 고칩니다. 지금 기본 설치는 서버가 HTML 문서에 GUI 세션을 직접 넣어 줍니다. 그래서 loopback에서 부트스트랩이 거절되면 원인은 “토큰이 없어서”가 아니라 Host/Origin 설정이 어긋난 경우가 대부분입니다. 그런데 고친 방식의 핵심은 “역할(role)이 아니라 바인드(bind)”를 묻는 것입니다. 프롬프트를 막은 뒤에는 다만 PR 본문이 말하는 “원인 이름을 붙인 actionable notice를 대시보드가 그린다”는 코드 범위와 어긋납니다. 이 diff는 이벤트만 dispatch하고, 라인 - 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
2 participants
Summary
A plain local user should never see the admin-token dialog, and today they can. That is the UX disaster this PR fixes.
A loopback install mints its own GUI session into the served document, so the dashboard never needs a typed credential. But
resolveTokenAfter401treated any definitive bootstrap refusal as "ask the human for a token". On loopback the only ways to get that refusal are a Host or Origin mismatch — a misconfiguration the admin token cannot repair. The user is handed a password box they cannot fill, and filling it would not have helped, because the token is not what was refused.It also contradicted the shipped contract. The Sign-in guide already promises "on the default loopback bind the dashboard never asks for a token". #3353 is a user who met the prompt after upgrading to 2.40.0, assumed config loss, and had to have an LLM read the codebase to find out what the box wanted.
Now the prompt only appears when the deployment actually requires a typed credential; otherwise the dashboard emits an actionable notice naming the real cause.
promptCancelledis set alongside it so a failing fan-out does not re-enter resolution on every request, andstoreSessionalready clears that flag, so recovery is automatic once the address is corrected.The predicate is the bind, not the topology.
runtimeRoleis the obvious signal and it is wrong in both directions:standalone+hostname: "0.0.0.0"is an operator who deliberately exposed the dashboard and must type the token — proven bytests/server-management-auth.test.ts, "a non-loopback binding never issues a GUI session from a forged loopback Host".hubon loopback still mints its own session.Gating on
role === "hub"would have hidden the prompt from exactly the operator who needs it. So the server now statesisApiAuthRequired— the same predicate it gates the mint with — beside the role it already emits, and the GUI reads that. A document without the tag falls back to the role, so a hub dashboard keeps working against a server that predates it.No server admission or credential semantics change:
issueGuiSession,requireManagementAuth, and the CORS resolvers are untouched apart from stating a value they already compute.Verification
bun run typecheck— cleanbun run lint:gui— cleancd gui && bun test— 1366 pass, 0 fail (220 files)bun test tests/gui-static.test.ts tests/server-management-auth.test.ts tests/server-auth.test.ts— 143 pass, 0 failbun run privacy:scan— passedFour new cases in
gui/tests/api-auth-deadline.test.ts: a standalone loopback dashboard is never prompted and emits the notice once; an absent tag behaves the same; an exposed standalone bind is still prompted; a loopback hub is not. The existing prompt-fallback tests inapi-auth-deadlineandapi-auth-memorynow declare the non-loopback shape their own comments already described.Checklist
devCloses #3353
Summary by CodeRabbit