Repository navigation
Conversation
`superset auth login` opens the authorization URL via xdg-open (Linux), open (macOS), or start (Windows). On headless Linux hosts — the typical target of the CLI release — xdg-open is often missing, and exec() silently swallows the failure. The user sees only "Waiting for browser authorization..." with no URL anywhere; the login hangs forever. Make the URL discoverable via a new `onAuthUrl` callback on `login()`. The callback fires just before the browser launch, letting the command layer render through clack — printing a fresh `p.log.info` with the URL which works on TTY (interleaved with the spinner) and headless alike. Browser auto-launch behaviour is unchanged for GUI users. Verified: an SSH user can now `ssh -L 51789:localhost:51789 <host>`, read the printed URL, paste it into a local browser, and complete the OAuth loopback callback through the SSH tunnel.
📝 WalkthroughWalkthroughThe login API now accepts a ChangesAuth Login Flow
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
Greptile SummaryThis PR surfaces the OAuth authorization URL via a new optional Confidence Score: 5/5Safe to merge — the change is a small, well-scoped addition with no logic regressions. All findings are P2 or lower. The implementation is correct: the loopback server is bound before onAuthUrl fires, the callback is optional so no existing callers break, and the known openBrowser fire-and-forget issue is pre-existing and out of scope. No files require special attention.
|
| Filename | Overview |
|---|---|
| packages/cli/src/lib/auth.ts | Introduces LoginOptions interface, refactors login() to accept options object, and hoists the auth URL to a const so onAuthUrl can be called before openBrowser(). Logic is correct: the loopback server is already bound before the callback fires. |
| packages/cli/src/commands/auth/login/command.ts | Updates the single internal login() call to use the new LoginOptions shape and adds an onAuthUrl callback that surfaces the URL via p.log.info for both TTY and non-TTY environments. |
Sequence Diagram
sequenceDiagram
participant CMD as command.ts
participant AUTH as auth.ts (login)
participant SERVER as Loopback Server
participant BROWSER as openBrowser()
participant USER as User/Terminal
CMD->>AUTH: login({ signal, onAuthUrl })
AUTH->>SERVER: bindLoopbackServer()
SERVER-->>AUTH: { server, port }
AUTH->>AUTH: build authorizeUrl
AUTH->>CMD: onAuthUrl(url) callback
CMD->>USER: p.log.info("If browser doesn't open…\n{url}")
AUTH->>BROWSER: openBrowser(url)
Note over BROWSER: fire-and-forget exec (xdg-open / open / start)
AUTH->>SERVER: waitForCallback(...)
USER-->>SERVER: paste URL → OAuth callback
SERVER-->>AUTH: code
AUTH->>AUTH: POST /api/cli/exchange
AUTH-->>CMD: LoginResult
Reviews (1): Last reviewed commit: "fix(cli): surface auth URL so headless l..." | Re-trigger Greptile
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/cli/src/lib/auth.ts">
<violation number="1" location="packages/cli/src/lib/auth.ts:158">
P2: User-provided `onAuthUrl` callback can throw and leak the bound loopback server</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
If a caller-supplied `onAuthUrl` callback throws, the loopback HTTP server bound earlier by `bindLoopbackServer` would never be closed — leaking the port and starving subsequent login attempts (loopback candidates are 51789 + 51790; both being held would surface as "All loopback ports in use"). Wrap the callback in try/catch and close the server before re-throwing. The error still propagates so the caller knows their callback failed. Reported by @cubic-dev-ai (P2) on ParadevAI#3950.
|
Thanks @cubic-dev-ai — fixed in e798930. The |
@johrld The implementation in This aligns well with the team's preference for explicit error handling and resource cleanup. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/cli/src/lib/auth.ts (1)
134-142: ⚡ Quick win
onAuthUrltyped asvoiddoesn't prevent async callers — the try/catch protection is incomplete
(url: string) => voidlooks safe, but TypeScript allowsPromise<void>to satisfyvoidin a callback position without a type error. A caller can pass anasyncfunction without any compiler complaint. Becauseoptions.onAuthUrl?.(url)is notawait-ed, an async rejection:
- Silently escapes the
try/catchas an unhandled promise rejection.- Bypasses
server.close(), leaking the bound loopback port — the very scenario the commit message says this block is designed to prevent.The fix is a one-liner: widen the return type and
awaitthe call.🛡️ Proposed fix — support async callbacks and close the server on rejection
- onAuthUrl?: (url: string) => void; + onAuthUrl?: (url: string) => void | Promise<void>;- try { - options.onAuthUrl?.(url); - } catch (err) { - // Don't leak the bound loopback server if the callback throws. - server.close(); - throw err; - } + try { + await options.onAuthUrl?.(url); + } catch (err) { + // Don't leak the bound loopback server if the callback throws. + server.close(); + throw err; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/lib/auth.ts` around lines 134 - 142, The onAuthUrl callback in LoginOptions can be async but is typed as (url: string) => void and is invoked without await, so rejections can escape the try/catch and prevent server.close(); update LoginOptions.onAuthUrl to accept a possibly async callback (e.g. (url: string) => void | Promise<void>) and change the invocation sites (where options.onAuthUrl?.(url) is called) to await the call inside the existing try/catch so any rejection is caught and server.close() always runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/cli/src/lib/auth.ts`:
- Around line 134-142: The onAuthUrl callback in LoginOptions can be async but
is typed as (url: string) => void and is invoked without await, so rejections
can escape the try/catch and prevent server.close(); update
LoginOptions.onAuthUrl to accept a possibly async callback (e.g. (url: string)
=> void | Promise<void>) and change the invocation sites (where
options.onAuthUrl?.(url) is called) to await the call inside the existing
try/catch so any rejection is caught and server.close() always runs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eac460cc-6332-469f-b167-fef54010c4ea
📒 Files selected for processing (1)
packages/cli/src/lib/auth.ts
|
Superseded by #3965 (cross-device login via OAuth code + PKCE, merged 2026-05-01 23:20 UTC). The new flow ships URL surfacing via the |
Summary
Closes #3949.
superset auth loginhangs on headless Linux hosts becauseopenBrowser()invokesxdg-openvia fire-and-forgetexec()— whenxdg-openis missing the failure is silent and no URL is ever surfaced.Fix
onAuthUrl(url)callback to a newLoginOptionsinterface inlib/auth.ts.login(signal)→login(options). Single internal caller updated.p.log.info(...). On TTY clack interleaves it with the running spinner; on non-TTY it's a clean info line.Browser auto-launch is unchanged for GUI users —
openBrowser()is still invoked after the URL is surfaced.Test plan
p.log.infoline, interleaves cleanly with the spinner.xdg-open): URL is now printed. Combined withssh -L 51789:localhost:51789 <host>the OAuth loopback callback completes through the tunnel and CLI logs in successfully.bun run lint:fix && bun run typecheck— couldn't run locally; relying on CI.Scope
packages/cli/src/lib/auth.ts— newLoginOptions;login()signature refactor; URL hoisted to a const;onAuthUrlinvoked beforeopenBrowser.packages/cli/src/commands/auth/login/command.ts— pass options including the callback.Out of scope (potential follow-ups)
openBrowser()still doesn't actually awaitexec()completion. Surfacing the URL eagerly sidesteps that for users; makingopenBrowsertruly synchronous is a separate change.--no-browserflag for purely scripted flows.Summary by cubic
Print the OAuth authorization URL during
superset auth loginso headless Linux hosts can complete login when no default browser is available. Auto-launch still works; this adds a visible URL fallback.LoginOptionswith optionalonAuthUrl(url)and refactoredlogin(signal)tologin({ signal, ... }).onAuthUrlcallback throws, close the loopback server to avoid port leaks and “All loopback ports in use” errors.Written for commit e798930. Summary will update on new commits.
Summary by CodeRabbit