fix: prevent workos auth login from hanging indefinitely#139
Merged
Conversation
`login.ts` had its own inline device auth polling loop that lacked an AbortController on fetch requests. A hung TCP connection (proxy, captive portal, IPv6 sinkhole) would block `await fetch()` forever, defeating the 5-minute outer timeout. Refactored `runLogin` to use the shared `requestDeviceCode` and `pollForToken` from `device-auth.ts`, which already has a 30-second per-request abort timeout. Also added the same abort timeout to `requestDeviceCode` for the initial device code request. This removes ~100 lines of duplicated logic (JWT parsing, token response handling, sleep helper, endpoint construction, type definitions) that had diverged from `device-auth.ts`.
Greptile SummaryThis PR fixes a hang in Confidence Score: 5/5Safe to merge — one minor P2 style nit, no logic or security regressions introduced. All changes are clean refactors addressing the reported bug. The two previous thread issues (unwrapped AbortError and fragile string matching) are both resolved. Only finding is a redundant No files require special attention. Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
- Add DeviceAuthTimeoutError subclass so callers can use instanceof instead of fragile string matching on error messages - Wrap AbortError in requestDeviceCode with DeviceAuthTimeoutError instead of letting raw DOMException propagate - Also wrap other fetch errors in DeviceAuthError for consistency
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRefactors the device-code login flow to delegate device authorization and token polling to Changes
Possibly related PRs
🚥 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 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/commands/login.ts`:
- Around line 115-123: The new human-only clack messages around the device auth
flow need JSON branches so --json emits machine-readable output: update the
requestDeviceCode error handling and the status/result blocks (around
requestDeviceCode/deviceAuth and the later 157-177 status/error outputs) to
check the current OutputMode (or the outputMode variable) and, when in JSON
mode, write structured JSON (e.g., { status: "error", error: msg } or { status:
"device_code", data: deviceAuth }) to stdout instead of clack.log.*; keep the
existing clack.log.* for human mode and ensure the same semantic info is present
in both branches using the same identifiers (requestDeviceCode, deviceAuth, and
the login command's status/error variables).
🪄 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: CHILL
Plan: Pro Plus
Run ID: 9d9832e9-4469-4dce-aa0f-99ab0fed28c5
📒 Files selected for processing (2)
src/commands/login.tssrc/lib/device-auth.ts
Comment on lines
+115
to
123
| clack.log.step('Starting authentication...'); | ||
|
|
||
| if (!authResponse.ok) { | ||
| clack.log.error(`Failed to start authentication: ${authResponse.status}`); | ||
| let deviceAuth; | ||
| try { | ||
| deviceAuth = await requestDeviceCode({ clientId, authkitDomain }); | ||
| } catch (error) { | ||
| const msg = error instanceof Error ? error.message : String(error); | ||
| clack.log.error(`Failed to start authentication: ${msg}`); | ||
| process.exit(1); |
There was a problem hiding this comment.
Add JSON-mode handling for the new login status/error output paths.
Line 115, Line 122, and Lines 157-177 introduce/modify human-only clack output without a JSON branch. In --json mode, this can emit non-machine-readable text on the command path you changed.
Suggested direction
- clack.log.step('Starting authentication...');
+ if (isJsonMode()) {
+ console.log(JSON.stringify({ event: 'auth_start' }));
+ } else {
+ clack.log.step('Starting authentication...');
+ }
...
- spinner.stop('Authentication successful!');
- clack.log.success(`Logged in as ${result.email || result.userId}`);
- clack.log.info(`Token expires in ${expiresInSec} seconds`);
+ if (isJsonMode()) {
+ console.log(
+ JSON.stringify({
+ event: 'auth_success',
+ userId: result.userId,
+ email: result.email,
+ expiresInSec,
+ }),
+ );
+ } else {
+ spinner.stop('Authentication successful!');
+ clack.log.success(`Logged in as ${result.email || result.userId}`);
+ clack.log.info(`Token expires in ${expiresInSec} seconds`);
+ }
...
- if (error instanceof DeviceAuthTimeoutError) {
- spinner.stop('Authentication timed out');
- clack.log.error('Authentication timed out. Please try again.');
- } else {
- spinner.stop('Authentication failed');
- const msg = error instanceof Error ? error.message : String(error);
- clack.log.error(`Authentication error: ${msg}`);
- }
+ if (isJsonMode()) {
+ const msg = error instanceof Error ? error.message : String(error);
+ console.log(
+ JSON.stringify({
+ event: 'auth_error',
+ timeout: error instanceof DeviceAuthTimeoutError,
+ message: msg,
+ }),
+ );
+ } else if (error instanceof DeviceAuthTimeoutError) {
+ spinner.stop('Authentication timed out');
+ clack.log.error('Authentication timed out. Please try again.');
+ } else {
+ spinner.stop('Authentication failed');
+ const msg = error instanceof Error ? error.message : String(error);
+ clack.log.error(`Authentication error: ${msg}`);
+ }As per coding guidelines: src/commands/**/*.ts: Implement both human and JSON output modes in commands; check OutputMode usage in src/bin.ts.
Also applies to: 157-177
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/login.ts` around lines 115 - 123, The new human-only clack
messages around the device auth flow need JSON branches so --json emits
machine-readable output: update the requestDeviceCode error handling and the
status/result blocks (around requestDeviceCode/deviceAuth and the later 157-177
status/error outputs) to check the current OutputMode (or the outputMode
variable) and, when in JSON mode, write structured JSON (e.g., { status:
"error", error: msg } or { status: "device_code", data: deviceAuth }) to stdout
instead of clack.log.*; keep the existing clack.log.* for human mode and ensure
the same semantic info is present in both branches using the same identifiers
(requestDeviceCode, deviceAuth, and the login command's status/error variables).
nicknisi
commented
Apr 30, 2026
| continue; | ||
| } | ||
| const provisioned = await provisionStagingEnvironment(result.accessToken); | ||
| if (provisioned) { |
nicknisi
commented
Apr 30, 2026
| try { | ||
| const result = await pollForToken(deviceAuth.device_code, { | ||
| clientId, | ||
| authkitDomain, |
nicknisi
added a commit
to nicknisi/diffdad
that referenced
this pull request
May 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
workos auth logincould hang forever if a TCP connection stalled (proxy, captive portal, IPv6 sinkhole). The inline polling loop inlogin.tshad noAbortControlleron fetch requests, so a single hung connection defeated the 5-minute outer timeout.login.tsreimplemented the entire device auth polling loop (JWT parsing, token handling, sleep, types) that already existed indevice-auth.ts. The two had diverged --device-auth.tshad a 30-second per-request abort timeout,login.tsdid not. RefactoredrunLoginto callrequestDeviceCode+pollForTokendirectly.requestDeviceCode, which also lacked one.Reported via Slack by Fraser Langton -- CLI stuck on "Waiting for authentication..." after browser showed success.
Test plan
pnpm buildpassespnpm testpasses (1621 tests)pnpm typecheckpassesworkos auth logout && workos auth logincompletes successfullyWORKOS_AUTHKIT_DOMAIN=http://127.0.0.1:9999) aborts within 30s instead of hanging foreverSummary by CodeRabbit