From 65113f498d21b3abbf32c22c6661761e25c9b965 Mon Sep 17 00:00:00 2001 From: Minh Nguyen <44449792+quickbeard@users.noreply.github.com> Date: Thu, 10 Sep 2026 17:46:02 +0700 Subject: [PATCH] fix(gateway): harden the SSO login in the TUI and desktop app Three fixes from an investigation of the SSO sign-in flow: - The loopback callback server parsed the request URL against the Host header. A malformed Host (any local process can send one) threw inside the request listener, an uncaught exception that took down the TUI or the desktop main process for the length of the sign-in wait. Parse against a fixed 127.0.0.1 base and answer 400 for a target the parser still rejects. - The TUI configure flow quit the whole app on any SSO failure, even from the startup key re-check or /configure on a working install. Quit only on the first-run gate; with a provider connected, toast the reason and close the dialog. /configure opens dismissable on a configured install. - The desktop startup key check prompted for a missing gateway key as well as a rejected one, so every Custom/Connect provider user saw the configure dialog on each launch. Carry the needs-login reason over IPC and prompt for a rejected key only, matching the TUI. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01KUxGp8Cs7mMifGyDDWf57b --- CHANGELOG.md | 22 ++++++++++ DIVERGENCES.md | 2 +- .../gateway/gateway-configure-gate.tsx | 6 +++ packages/app/src/gateway/types.ts | 7 +++- packages/codev-gateway/src/sso.ts | 14 ++++++- packages/codev-gateway/test/sso.test.ts | 33 +++++++++++++++ .../src/main/gateway-sso/controller.test.ts | 9 +++-- .../src/main/gateway-sso/controller.ts | 9 ++--- packages/tui/src/app.tsx | 4 +- .../tui/src/component/dialog-configure.tsx | 40 ++++++++++++++----- 10 files changed, 122 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 15dd9e55b..a9f899fb9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -79,6 +79,28 @@ file is the human-readable summary of what changed and why. ### Gateway +- **A stray request can no longer crash the app during sign-in** — while + **Login with SSO** waits for the browser, the app listens on a local port for + the callback. A request to that port carrying a malformed `Host` header (any + program on the machine can send one) threw inside the listener, and that + exception took down the whole process: the terminal app, or the desktop + app's main process. The callback path is now read without trusting the + header, and a request the parser still rejects gets a `400` instead. +- **The terminal app no longer quits over a failed sign-in when you already + have a provider** — a login that timed out (five minutes with the browser + tab left open) or a token exchange that failed once used to end the whole + app with a "setup failed" message, even from the startup key re-check or + `/configure` on a working install, where there was a perfectly good chat to + go back to. That exit is now reserved for the first-run gate, where there is + nothing to fall back to; with a provider connected the failure is a toast + and the dialog closes. `/configure` on a configured install also opens as a + dismissable dialog now, like the desktop app's **Get a new API Key**. +- **The desktop app stops asking non-gateway users to sign in on every + launch** — the startup key check treated "no gateway key stored" the same + as "the gateway rejected the key" and opened the configure dialog for both, + so anyone set up through **Custom provider** or **Connect provider** (which + never store a gateway key) got the dialog at each launch. Only a rejected + gateway key prompts now, matching the terminal app. - **A gateway that refuses your account says so, once** — when the CoDev gateway declines to issue a key for an account (today: an email domain outside its allowlist, such as a sub-domain of a listed one), **Login with diff --git a/DIVERGENCES.md b/DIVERGENCES.md index 2f65fbe0a..6c8662964 100644 --- a/DIVERGENCES.md +++ b/DIVERGENCES.md @@ -157,7 +157,7 @@ from the squash message first, or use a plain merge. | `packages/tui/src/feature-plugins/sidebar/footer.tsx` | Added the editor-context label above the `cwd:branch` line: `useEditorLabel()` (fork-only `src/context/editor-label.tsx`) supplies the effective selection, an `editorFile` memo formats it via `editorContextFileLabel(...)` at a fixed width of 36 (the sidebar's 42 columns less 2 of padding a side and 2 for the bullet), and `` renders it — both from the fork-only `./editor-context-label.tsx`. | The open file + selected range belong with the sidebar's other ambient session state rather than in the prompt footer's crowded keybind-hint row; they sit next to the cwd for a single "where am I working" group. Moved out of the prompt footer, which no longer shows it at all. | Keep the `useEditorLabel` import, the `editorFile` memo, and the `` element as the box's first child; take upstream's changes to the path display below it. | | `packages/tui/src/feature-plugins/sidebar/lsp.tsx` | Whole LSP section wrapped in `` so it renders nothing when `config.lsp` is falsy; the "LSPs are disabled" placeholder text removed (empty state is only "LSPs will activate as files are read") | A disabled feature shouldn't occupy sidebar space with a permanent placeholder. | Keep our `Show` wrapper + removed placeholder; take upstream's surrounding changes. | | `packages/opencode/src/cli/cmd/*.ts` (tui, run, serve, attach, pr, upgrade, uninstall, web) | Help `describe` strings "opencode" → "codev" | `--help` is the first thing users read. | Keep our strings. | -| `packages/tui/src/app.tsx` | Terminal title "OpenCode"/"OC \|" → "CoDev Code"/"CC \|"; the configure-gate effect is a stateless forced-setup invariant — while `sync.status === "complete" && !useConnected() && dialog.stack.length === 0` it (re)opens the fork's `DialogConfigure` (component/dialog-configure.tsx, new file), so escaping any gate-launched flow or a credential-only save bounces back to the gate instead of an unconfigured chat (gates on `!useConnected()`, not `provider.length === 0` — hosted Zen free models always register); added the `codev.configure` command (`/configure`) and a `CODEV_CONFIGURE=1` mount hook for `codev configure`; home screen skipped — plain startup seeds the `dummy` session route and auto-opens a new empty session (reusing a leftover default-titled empty one via `openNewSession`/`findEmptySession`), `session.new` and the current-session `session.deleted` handler open a new session instead of navigating home, and the `-c` resolver prefers non-default-titled sessions so it lands on the last real conversation; added the `app.exit.confirm` command + `EXIT_CONFIRM_WINDOW` const (double-press Ctrl+C to quit: first press shows a toast, second within 1.5 s exits) and gathered its `app_exit_confirm` binding alongside `app_exit`; an onMount "Session upload is paused" warning toast next to the raster-font one, shown when `@codev/upload/nudge`'s `shouldNudgeSignIn()` says the background upload stalled for lack of a CoDev SSO session (SSO-origin keys only; deliberately every launch until the sign-in happens — see the session-upload wiring row); a one-shot `createEffect` after `sync.status === "complete"` runs `@codev/gateway`'s `ensureGatewayKey()` (the startup gateway-key pre-flight shared with the desktop controller): a `refreshed` result merges the re-minted key into the auth registry for every id it names via `sdk.client.auth.set`, then `instance.dispose()` + `sync.bootstrap()`; a `needs-login` with reason `rejected` opens `` when no dialog is up (`no-key` is ignored — a non-gateway setup, or the gate's own case) | Tab title branding; first-run setup is a forced gate the user can't escape into an unconfigured chat (the invariant reopens it); `codev` starts directly in the session layout instead of the home screen; a stray Ctrl+C no longer quits on the first press. | Keep our strings, the invariant gate effect, dialog swap + command block, route seed, `openNewSession`/`findEmptySession` + call sites, the `-c` default-title filter, and the `app.exit.confirm` command + binding; take upstream's surrounding changes. | +| `packages/tui/src/app.tsx` | Terminal title "OpenCode"/"OC \|" → "CoDev Code"/"CC \|"; the configure-gate effect is a stateless forced-setup invariant — while `sync.status === "complete" && !useConnected() && dialog.stack.length === 0` it (re)opens the fork's `DialogConfigure` (component/dialog-configure.tsx, new file), so escaping any gate-launched flow or a credential-only save bounces back to the gate instead of an unconfigured chat (gates on `!useConnected()`, not `provider.length === 0` — hosted Zen free models always register); added the `codev.configure` command (`/configure`, opening `` — dismissable on a configured install, the gate otherwise) and a `CODEV_CONFIGURE=1` mount hook for `codev configure`; home screen skipped — plain startup seeds the `dummy` session route and auto-opens a new empty session (reusing a leftover default-titled empty one via `openNewSession`/`findEmptySession`), `session.new` and the current-session `session.deleted` handler open a new session instead of navigating home, and the `-c` resolver prefers non-default-titled sessions so it lands on the last real conversation; added the `app.exit.confirm` command + `EXIT_CONFIRM_WINDOW` const (double-press Ctrl+C to quit: first press shows a toast, second within 1.5 s exits) and gathered its `app_exit_confirm` binding alongside `app_exit`; an onMount "Session upload is paused" warning toast next to the raster-font one, shown when `@codev/upload/nudge`'s `shouldNudgeSignIn()` says the background upload stalled for lack of a CoDev SSO session (SSO-origin keys only; deliberately every launch until the sign-in happens — see the session-upload wiring row); a one-shot `createEffect` after `sync.status === "complete"` runs `@codev/gateway`'s `ensureGatewayKey()` (the startup gateway-key pre-flight shared with the desktop controller): a `refreshed` result merges the re-minted key into the auth registry for every id it names via `sdk.client.auth.set`, then `instance.dispose()` + `sync.bootstrap()`; a `needs-login` with reason `rejected` opens `` when no dialog is up (`no-key` is ignored — a non-gateway setup, or the gate's own case) | Tab title branding; first-run setup is a forced gate the user can't escape into an unconfigured chat (the invariant reopens it); `codev` starts directly in the session layout instead of the home screen; a stray Ctrl+C no longer quits on the first press. | Keep our strings, the invariant gate effect, dialog swap + command block, route seed, `openNewSession`/`findEmptySession` + call sites, the `-c` default-title filter, and the `app.exit.confirm` command + binding; take upstream's surrounding changes. | | `packages/tui/bunfig.toml` | `[test] preload` gains the fork-only `./test/preload.ts`, which sets `CODEV_TEST_HOME` to a fresh temp dir (unless already set) before any test runs | The app mount runs the gateway key pre-flight (`ensureGatewayKey`) and the upload sign-in nudge at startup; both read the shared auth file under `~/.codev`/`~/.codev-hub`, and the pre-flight can write a re-minted key back. View tests boot the real app, so without an isolated home a developer's test run would probe the real gateway with their real key. Same convention as core's `Global` and `packages/opencode/test/preload.ts`. | Keep our preload entry; take upstream's other bunfig changes. | | `packages/tui/src/routes/session/index.tsx` | Flow transcript: the scrollbox opens with a home-like banner (`Logo` + `home_bottom` tips slot, `placement="start"`) as its first entry for root sessions (subagent sessions keep a plain 1-row pad), its `maxHeight` is clamped to the content height (a `transcriptHeight` signal read off the content's yoga node in the renderer root's `layout-changed` handler, which then re-runs `calculateLayout` so the clamp lands in the same frame — `scrollHeight` is a frame stale there, and painting with a stale clamp flickers during streaming) so the `Prompt` rides directly below the last message, and a trailing flexGrow spacer absorbs the leftover space until content fills the screen (then the prompt is effectively bottom-anchored and the banner scrolls away with history); the session `Prompt` gets home's `placeholder` pool when the session has no messages so the input shows an `Ask anything... ""` placeholder like home | Startup lands in an empty session instead of the home screen; the transcript flows top-down like terminal scrollback — logo top-left, input following the conversation down — instead of a bottom-anchored prompt with a void above it. | Keep our banner block inside the scrollbox, the `transcriptHeight` clamp + `layout-changed` listener, the trailing spacer, and the `placeholders` prop; take upstream's surrounding changes. | | `packages/tui/src/routes/session/index.tsx` | Sidebar visibility is session-local: removed upstream's persisted `kv.signal<"auto" \| "hide">("sidebar", "auto")` (and the `sidebar() === "auto" && wide()` branch of `sidebarVisible`); visibility is driven only by the non-persisted `sidebarOpen` signal, which the `session.sidebar.toggle` command flips (supersedes the earlier `"auto"` → `"hide"` default change) | The right sidebar starts hidden in every session regardless of terminal width or what the user did last session; `ctrl+b`/`b` only affects the current session. A stale `sidebar` key in existing kv.json files is simply never read. | Keep the kv signal removed and `sidebarVisible` reading only `parentID` + `sidebarOpen`; take upstream's surrounding changes. | diff --git a/packages/app/src/components/gateway/gateway-configure-gate.tsx b/packages/app/src/components/gateway/gateway-configure-gate.tsx index d81d3ffe6..54d45fae0 100644 --- a/packages/app/src/components/gateway/gateway-configure-gate.tsx +++ b/packages/app/src/components/gateway/gateway-configure-gate.tsx @@ -164,6 +164,12 @@ export function GatewayConfigureGate() { .catch(() => undefined) return } + // "no-key" is a machine set up with some other provider (the Custom and + // Connect provider paths never write the shared auth file) or not at all — + // the gate above owns that case. Prompting for it here asked every + // non-gateway user to sign in on each launch; only a rejected gateway key + // is worth interrupting for, same as the TUI. + if (result.reason !== "rejected") return if (dialog.active !== undefined) return gateDialogOpen = true // Dismissable: unlike the gate above, a provider *is* configured here — its diff --git a/packages/app/src/gateway/types.ts b/packages/app/src/gateway/types.ts index 038521cf7..67fd4a1f1 100644 --- a/packages/app/src/gateway/types.ts +++ b/packages/app/src/gateway/types.ts @@ -57,8 +57,11 @@ export type GatewayKeyCheck = // pre-rename spellings, so a config block written before an identity rename // is re-keyed too instead of 401ing on a credential nothing updates. | { status: "refreshed"; providerIDs: string[]; apiKey: string } - // No key, or one that cannot be replaced without a browser sign-in. - | { status: "needs-login" } + // "no-key": nothing stored — a machine set up with a non-gateway provider + // (the Custom and Connect provider paths never write the shared auth file), + // or never set up. "rejected": the gateway refused the stored key and the + // SSO session could not re-mint it without a browser sign-in. + | { status: "needs-login"; reason: "no-key" | "rejected" } // The signed-in identity from the shared auth file — present whenever an SSO // session (live or refresh-revivable) is on disk, independent of the login diff --git a/packages/codev-gateway/src/sso.ts b/packages/codev-gateway/src/sso.ts index 2b8043209..10605899e 100644 --- a/packages/codev-gateway/src/sso.ts +++ b/packages/codev-gateway/src/sso.ts @@ -423,8 +423,18 @@ function getAuthCode( } const server = createServer((req, res) => { - const host = req.headers.host ?? "127.0.0.1" - const url = new URL(req.url ?? "/", `http://${host}`) + // Parsed against a fixed base, never the Host header: the server is + // bound to 127.0.0.1 and only the path and query matter. Building the + // base from Host used to throw on a malformed value (any local process + // can send `Host: [`), and an exception in the request listener is an + // uncaught exception that took the whole process down for the length of + // the sign-in wait. A target the parser still rejects is answered. + const url = URL.parse(req.url ?? "/", "http://127.0.0.1") + if (!url) { + res.writeHead(400, { "Content-Type": "text/plain" }) + res.end("Bad request") + return + } // force-login only: the IdP has just killed its session cookie and // redirected back here — bounce on to /authorize so it shows the diff --git a/packages/codev-gateway/test/sso.test.ts b/packages/codev-gateway/test/sso.test.ts index ca5b77058..a570cc0fe 100644 --- a/packages/codev-gateway/test/sso.test.ts +++ b/packages/codev-gateway/test/sso.test.ts @@ -1,5 +1,6 @@ import { afterEach, beforeEach, describe, expect, test } from "bun:test" import { existsSync, mkdtempSync, readFileSync, rmSync } from "node:fs" +import { connect } from "node:net" import { tmpdir } from "node:os" import { join } from "node:path" import { isLoginCancelled, login, logout, parseCallbackParams, probeSession, silentSso } from "../src/sso" @@ -217,6 +218,38 @@ describe("login", () => { expect(existsSync(authFilePath())).toBe(false) }) + // Any local process can reach the loopback port during the sign-in wait, + // and a request with a Host header that is not a valid URL authority used to + // throw inside the request listener — an uncaught exception that took the + // whole process down. + test("a malformed Host header is answered instead of crashing the process", async () => { + const abort = new AbortController() + const ready = new Promise((resolve) => { + void login({ + onLog: () => {}, + onReady: (open) => open(), + openUrl: async (url) => resolve(url), + fetchImpl: noFetch, + signal: abort.signal, + }).catch(() => {}) + }) + const port = Number(new URL(new URL(await ready).searchParams.get("redirect_uri")!).port) + const status = await new Promise((resolve, reject) => { + const socket = connect(port, "127.0.0.1", () => { + socket.write("GET /nope HTTP/1.1\r\nHost: [\r\nConnection: close\r\n\r\n") + }) + socket.once("data", (data) => { + resolve(data.toString().split("\r\n")[0]) + socket.destroy() + }) + socket.once("error", reject) + }) + expect(status).toBe("HTTP/1.1 404 Not Found") + // The login is still waiting on the real callback. + expect(await loopbackAlive(String(port))).toBe(true) + abort.abort() + }) + test("an already-aborted signal never opens a server", async () => { const abort = new AbortController() abort.abort() diff --git a/packages/desktop/src/main/gateway-sso/controller.test.ts b/packages/desktop/src/main/gateway-sso/controller.test.ts index 7f4b1ff54..4041c8a93 100644 --- a/packages/desktop/src/main/gateway-sso/controller.test.ts +++ b/packages/desktop/src/main/gateway-sso/controller.test.ts @@ -443,13 +443,16 @@ describe("gateway sso controller", () => { silentSso: async () => null, }) - expect(await controller.ensureKey()).toEqual({ status: "needs-login" }) + expect(await controller.ensureKey()).toEqual({ status: "needs-login", reason: "rejected" }) }) - test("asks for a login when there is no key at all", async () => { + // The reason travels to the renderer, whose gate only prompts for a + // rejected key: a desktop set up with some other provider must not be + // asked to sign in on every launch. + test("reports a missing key as such, not as a rejection", async () => { const controller = controllerWith({ loadApiKey: () => null }) - expect(await controller.ensureKey()).toEqual({ status: "needs-login" }) + expect(await controller.ensureKey()).toEqual({ status: "needs-login", reason: "no-key" }) }) // SSO-minted keys record no base_url, so the saved gateway URL is the only diff --git a/packages/desktop/src/main/gateway-sso/controller.ts b/packages/desktop/src/main/gateway-sso/controller.ts index a8811cd84..d2f4ff919 100644 --- a/packages/desktop/src/main/gateway-sso/controller.ts +++ b/packages/desktop/src/main/gateway-sso/controller.ts @@ -310,16 +310,15 @@ export function createGatewaySsoController(deps: Deps) { // cannot be refreshed without a browser round-trip asks for one. async ensureKey(): Promise { // The check itself is shared with the TUI (@codev/gateway ensureGatewayKey); - // this wrapper adopts a refreshed session into the controller and maps - // the result onto the wire type. Both needs-login reasons prompt here, - // as before: a desktop set up without a gateway key is still asked to - // sign in. + // this wrapper adopts a refreshed session into the controller and passes + // the rest through, needs-login reason included: the renderer's gate + // prompts for a rejected key only, like the TUI. const result = await ensureGatewayKey({ fetchImpl: deps.fetchImpl, timeoutMs: KEY_CHECK_TIMEOUT_MS, fns: gateway, }) - if (result.status !== "refreshed") return { status: result.status } + if (result.status !== "refreshed") return result session = result.auth apiKey = result.apiKey gatewayUrl = result.baseUrl diff --git a/packages/tui/src/app.tsx b/packages/tui/src/app.tsx index df548e900..236d1c603 100644 --- a/packages/tui/src/app.tsx +++ b/packages/tui/src/app.tsx @@ -954,12 +954,14 @@ function App(props: { onSnapshot?: () => Promise; pluginHost: TuiPlugi category: "Provider", }, // Fork: the CoDev gateway configure flow (SSO or own API key). + // Dismissable once a provider is connected: opened on purpose from a + // working chat, it needs a way back; unconfigured, it is the gate. { name: "codev.configure", title: "Configure CoDev gateway", slashName: "configure", run: () => { - dialog.replace(() => ) + dialog.replace(() => ) }, category: "Provider", }, diff --git a/packages/tui/src/component/dialog-configure.tsx b/packages/tui/src/component/dialog-configure.tsx index 3b9485ad0..456f17eae 100644 --- a/packages/tui/src/component/dialog-configure.tsx +++ b/packages/tui/src/component/dialog-configure.tsx @@ -22,6 +22,7 @@ import { useEpilogue } from "../context/epilogue" import { useLocal } from "../context/local" import { runCustomProviderFlow, RETRIES, StatusBox, withRetries } from "./dialog-custom-provider" import { DialogProvider } from "./dialog-provider" +import { useConnected } from "./use-connected" import { useExit } from "../context/exit" import { useSync } from "../context/sync" import { useSDK } from "../context/sdk" @@ -55,6 +56,9 @@ interface FlowContext { exit: ReturnType epilogue: ReturnType local: ReturnType + // Whether a provider is already connected — decides what a failed flow + // falls back to (see failSetup). + connected: Accessor } // `dismissable` is for the startup key pre-flight in app.tsx: a provider IS @@ -69,6 +73,7 @@ export function DialogConfigure(props: { dismissable?: boolean } = {}) { exit: useExit(), epilogue: useEpilogue(), local: useLocal(), + connected: useConnected(), } // The entry chooser is a non-dismissable setup gate: no escape, no @@ -135,10 +140,25 @@ function showSelect( }) } -// The configure flow is a forced setup gate, so an unrecoverable failure must -// not silently drop the user into an unconfigured chat. Print the reason as an -// exit epilogue (shown after the TUI tears down) and quit the app. -function failAndQuit(ctx: FlowContext, message: string) { +// On a fresh install the configure flow is a forced setup gate, so an +// unrecoverable failure must not silently drop the user into an unconfigured +// chat: print the reason as an exit epilogue (shown after the TUI tears down) +// and quit the app. With a provider already connected — the startup key +// re-check, /configure on a working install — there is a chat to go back to, +// and quitting an app the user was in the middle of using over a login timeout +// or one failed token exchange is worse than the failure itself: show the +// reason as a toast and close the dialog instead. +function failSetup(ctx: FlowContext, message: string) { + if (ctx.connected()) { + ctx.toast.show({ + variant: "error", + title: "CoDev Code setup failed", + message: `${message}. Run /configure to try again.`, + duration: 10000, + }) + ctx.dialog.clear() + return + } ctx.epilogue(`CoDev Code setup failed: ${message}\nRun \`codev configure\` to try again.`) ctx.exit() } @@ -210,7 +230,7 @@ async function runSsoFlow(ctx: FlowContext) { // A refusal — the gateway declined this account, e.g. an email domain // outside its allowlist — is neither retryable nor a reason to quit: the // manual path below works with a key issued by hand, so it lands there - // with the gateway's own reason instead of in failAndQuit's generic + // with the gateway's own reason instead of in failSetup's generic // "setup failed". Same handling as codev-hub's install step. const minted = await withRetries("Fetching gateway API key", setStatus, () => fetchApiKey(session.access_token), @@ -247,9 +267,9 @@ async function runSsoFlow(ctx: FlowContext) { if (isLoginCancelled(err)) return // login() is single-attempt (its auth code is single-use, so a silent // retry can't reuse it); the retryable backend steps have already spent - // their budget by the time they throw here. Either way, quit rather than - // fall through to chat. - failAndQuit(ctx, err instanceof Error ? err.message : String(err)) + // their budget by the time they throw here. Either way, fail the flow + // rather than fall through to an unconfigured chat. + failSetup(ctx, err instanceof Error ? err.message : String(err)) } } @@ -343,7 +363,7 @@ async function runModelFlow(ctx: FlowContext, creds: GatewayCreds) { // cached, so the static per-model table stands in. cacheModelLimits(await fetchModelWindows(creds.apiKey, creds.gatewayBaseUrl)) } catch (err) { - failAndQuit( + failSetup( ctx, `couldn't fetch models after ${RETRIES} retries: ${err instanceof Error ? err.message : String(err)}`, ) @@ -406,6 +426,6 @@ async function runModelFlow(ctx: FlowContext, creds: GatewayCreds) { spawnUploadDaemon() ctx.dialog.clear() } catch (err) { - failAndQuit(ctx, err instanceof Error ? err.message : String(err)) + failSetup(ctx, err instanceof Error ? err.message : String(err)) } }