fix(gui): never ask a local dashboard for an admin token - #3492
fix(gui): never ask a local dashboard for an admin token#3492lidge-jun wants to merge 2 commits into
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
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
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. |
✅ READY
Hygiene✅ Deterministic PR hygiene checks passed. |
리뷰 · 우선순위 72 / 80이 PR은 로컬(루프백) 대시보드 사용자가 관리자 토큰 입력창을 보지 않아야 하는데도 보게 되는 UX 사고(#3353)를 고칩니다. 지금 고침의 핵심은 “역할(role)이 아니라 바인드(bind)”입니다. 현재 gui/src/api-targets.ts · adminTokenPromptAllowed - 바인드 선언을 읽는 설계가 맞고, 주석이 왜 역할이 틀린지도 분명히 적혀 있습니다. 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
![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: 8bbb404e97
ℹ️ 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".
| ?? (typeof CustomEvent === "function" ? CustomEvent : null); | ||
| if (!Ctor) return; | ||
| try { | ||
| window.dispatchEvent(new Ctor(SESSION_UNAVAILABLE_EVENT, { detail: { plane } })); |
There was a problem hiding this comment.
When a loopback session bootstrap receives a definitive 4xx, this path suppresses all later authentication resolution and dispatches an event, but no production code listens for SESSION_UNAVAILABLE_EVENT (the only production occurrences are the declaration and this emitter). Consequently, the promised actionable Host/Origin notice never appears and the dashboard continues failing with 401 responses without explaining how to recover; add a shell-level listener that renders a localized alert before setting promptCancelled permanently.
Useful? React with 👍 / 👎.
| undefined, | ||
| guiSessionCandidate ?? undefined, | ||
| config.runtimeRole ?? "standalone", | ||
| isApiAuthRequired(config), |
There was a problem hiding this comment.
This new argument controls whether deployed dashboards can recover by entering an admin token, but the added GUI tests manually construct the meta tag and no server-side test asserts that startServer/serveGuiFile emits 1 for non-loopback binds and 0 for loopback binds. A wiring or predicate regression would therefore either lock remote operators out or restore the unwanted local prompt while all new tests remain green; add focused coverage at the server/static-serving boundary.
AGENTS.md reference: src/AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
lidge-jun
added
the
landed-via-maintainer
1 participant
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
devafter fix(gui): stop the admin-token dialog painting an empty error notice #3491Closes #3353