Print auth URL to console to support a remote auth flow#32
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Summary by CodeRabbit
WalkthroughThe ChangesCLI login paste URL alternative flow
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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)
✨ Simplify code
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/lib/auth/login.ts (1)
65-112:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSerialize the callback exchange instead of guarding it with
completed.
completedis only set afterawait state.handleCallback(...)returns. If the browser redirect and the pasted URL arrive close together, both branches can enterhandleCallback(...)with the same auth code. That makes the login outcome timing-dependent: one path can spend the code while the other rejects, andPromise.race(...)may fail even though the other path already completed successfully.Suggested fix
- let completed = false; + let completed = false; + let completionPromise: Promise<void> | undefined; + + const completeOnce = (callbackUrl: URL): Promise<void> => { + if (!completionPromise) { + completionPromise = (async () => { + await state.handleCallback(callbackUrl); + completed = true; + })(); + } + + return completionPromise; + }; const httpResult = new Promise<void>((resolve, reject) => { server.on("request", async (req, res) => { @@ try { - await state.handleCallback(url); - completed = true; + await completeOnce(url); } catch (error) { @@ const pasteResult = waitForPastedCallback({ input, output, signal: pasteAbort.signal, }).then(async (pastedUrl) => { if (pastedUrl === null || completed) return; - await state.handleCallback(pastedUrl); - completed = true; + await completeOnce(pastedUrl); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/lib/auth/login.ts` around lines 65 - 112, The race exists because both the HTTP handler and the pasteResult branch call state.handleCallback concurrently and only set completed after awaiting it; replace the boolean-guard pattern by serializing the exchange: introduce a single shared "inflight" promise/lock (e.g., handleCallbackPromise or similar) that both the server request handler and the waitForPastedCallback .then callback consult—if it's null they assign it to the Promise returned by state.handleCallback(url) and await it, otherwise they await the existing promise; set completed (or resolve the inflight promise) only after that promise settles and use that shared symbol in place of the current completed checks so handleCallback is invoked at most once (refer to completed, state.handleCallback, the server "request" handler, and the pasteResult callback).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/lib/auth/login.ts`:
- Around line 76-82: The server is being closed immediately when the pasted-flow
loses the Promise.race, which prevents late browser redirects from reaching the
request handler's completed branch (the branch that calls
state.resolveWorkspaceName() and renderSuccessPage). Modify the shutdown logic
so the HTTP server isn't closed until after any in-flight request can be
handled: either move the server.close() call into the request handler after
sending the success response (ensure you call server.close() only once), or, if
you must close from the race winner path, wait for a short grace period or check
a shared completed flag and defer closing until completed is true or the current
response finishes; update the code paths that call server.close() around the
Promise.race handling (the shutdown block currently invoked after the race) to
use this deferred-close approach so renderSuccessPage and
state.resolveWorkspaceName() can complete.
---
Outside diff comments:
In `@packages/cli/src/lib/auth/login.ts`:
- Around line 65-112: The race exists because both the HTTP handler and the
pasteResult branch call state.handleCallback concurrently and only set completed
after awaiting it; replace the boolean-guard pattern by serializing the
exchange: introduce a single shared "inflight" promise/lock (e.g.,
handleCallbackPromise or similar) that both the server request handler and the
waitForPastedCallback .then callback consult—if it's null they assign it to the
Promise returned by state.handleCallback(url) and await it, otherwise they await
the existing promise; set completed (or resolve the inflight promise) only after
that promise settles and use that shared symbol in place of the current
completed checks so handleCallback is invoked at most once (refer to completed,
state.handleCallback, the server "request" handler, and the pasteResult
callback).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 48967437-4948-48bd-acb9-26b22d407a66
📒 Files selected for processing (1)
packages/cli/src/lib/auth/login.ts
luanvdw
left a comment
luanvdw
left a comment
There was a problem hiding this comment.
Thanks @sneub, this adds a solid QoL improvement and gives users a practical escape hatch when the CLI is running on one machine and the browser is on another machine!
Could you/your agent review these small changes before we merge this in?
- Only enable the paste fallback when we can actually prompt
If stdin is not a TTY, there is nowhere for the user to paste the callback URL. In that case, let’s keep the existing browser-callback behaviour, i.e. don’t start/race the paste flow, and don’t treat paste as an available fallback if opening the browser fails.
Put differently, the paste path should only change behaviour in interactive terminals.
-
Make completion run once
Since the browser callback and pasted callback can both arrive, let’s centralize completion through a smallcompleteOnce(callbackUrl)helper. That helper should make surestate.handleCallback(...)runs only once, and both paths await the same completion if they arrive close together. -
Make the remote-machine instruction more concrete
I think “resulting callback URL” may be unclear to users. Could we make it more explicit
Suggested copy:
Open this URL to sign in: <auth-url>
If the browser opens on another machine, finish sign-in there. When it redirects to localhost, copy the full localhost URL from the address bar and paste it here.
- Keep login alive after a bad paste
If the user presses Enter too early or pastes the wrong thing, we should not end the whole login. Can we show a short message and ask again, while still leaving the browser callback path open? If this opens a big conditional loop, skip this for now - we can always return to this if needed.
- Gate the paste fallback on a TTY: only swallow browser-launch failures when stdin is interactive, and only start/race the paste prompt then. Non-TTY environments keep the existing browser-callback behaviour and surface a failed browser launch instead of hanging. - Funnel both completion paths through a single completeOnce() so the token exchange runs at most once when the browser redirect and a pasted URL arrive together; clear the in-flight promise on failure to allow retries. - Make the remote sign-in instructions concrete. - Re-prompt after a premature Enter, an unparseable paste, or a callback the exchange rejects, instead of ending the whole login. The browser callback path stays open throughout.
|
Thanks @luanvdw — all four addressed in 20dbbcb (single file
One CodeRabbit point I intentionally didn't act on: it flagged that the HTTP listener is torn down right after the race resolves, so a late browser redirect that arrives after the paste path already won won't get the success page. Once the paste path completes, the user is already signed in at their terminal — the only cost is a stray browser tab showing a connection error, which I judged not worth the deferred-shutdown complexity. Happy to revisit if you'd rather close that gap. Tests added for: paste-only completion, run-once when both paths fire, re-prompt on bad paste / rejected exchange, and non-TTY browser-failure surfacing. Full suite green (232 tests). |
luanvdw
left a comment
luanvdw
left a comment
There was a problem hiding this comment.
Re-reviewed after branch update. Previous feedback is addressed, and checks are green.
2 participants
Summary
prisma-cli auth loginopens a browser and waits on alocalhost:<port>/auth/callbackHTTP server for the OAuth redirect. This breaks on remote machines (SSH, Docker,
cloud VMs): no browser to open, and even if the user copies the URL to a local
browser, the redirect lands on the local machine's
localhost, never reachingthe remote CLI.
This change adds a second completion path — paste-the-callback-URL — that runs in
parallel with the existing browser callback. No backend changes, no SDK changes,
no new flag. Laptop users see no behavior change; remote users get a path that
actually works.
How it works
readlineprompt (Paste URL here:) runs in parallel when stdin is a TTY.Promise.race([httpCallback, pastedUrl])— whichever delivers a callback URLfirst wins. Both paths call the same
state.handleCallback(url).completedguard ensures the OAuth code exchange runs exactly once even ifboth fire close together; the losing path is aborted via
AbortControllerandthe existing server-close
finally.open(url)failures are now swallowed instead of bubbling — the paste pathis a viable fallback when the browser launch silently fails on headless boxes.
UX notes
behavior is identical to today.
code) so we can keepstatevalidation intact with zero new parsing logic.
Scope
Single file:
packages/cli/src/lib/auth/login.ts(~70 net lines). No changes tothe controller, use-cases, types, SDK, or any backend/OAuth-provider config.