feat: add browser-based OAuth login (vargai login)#212
Open
SecurityQQ wants to merge 1 commit into
Open
Conversation
Adds a third login mode that opens the browser for authentication, reusing the MCP OAuth 2.1 server infrastructure. Users can now sign in via Google or email through the app consent page instead of typing OTP codes in the terminal. Flow: CLI starts localhost callback server -> opens browser to mcp.varg.ai/oauth/authorize -> user authenticates on app.varg.ai -> callback redirects to localhost with auth code -> CLI exchanges code for API key via PKCE token exchange. Browser login is now the default (option 1). Email OTP and API key paste remain as options 2 and 3 for headless environments.
Contributor
📝 Walkthroughwalkthroughadds oauth browser-based login with pkce to cli. introduces callback server for handling authorization code flow, pkce utilities for secure token exchange, and updates login command to support 3-option selection with browser oauth as default. changes
sequence diagram(s)sequenceDiagram
participant User
participant CLI
participant Localhost as Localhost<br/>Callback Server
participant Browser
participant OAuth as MCP OAuth<br/>Endpoints
participant Gateway
User->>CLI: run login (browser option)
CLI->>CLI: generate pkce state & verifier
CLI->>Localhost: start callback server
Localhost-->>CLI: return port
CLI->>Browser: open auth url<br/>(code_challenge, state, redirect_uri)
Browser->>OAuth: authorization request
OAuth-->>Browser: redirect to localhost callback
Browser->>Localhost: GET /callback?code=X&state=Y
Localhost->>Localhost: validate state
Localhost-->>Browser: success page
Localhost-->>CLI: resolve with code & state
CLI->>OAuth: exchange code for token<br/>(code, verifier, client_id)
OAuth-->>CLI: access_token
CLI->>Gateway: validate token (check balance)
Gateway-->>CLI: success
CLI-->>User: logged in
estimated code review effort🎯 4 (complex) | ⏱️ ~45 minutes possibly related prs
poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/cli/commands/login.tsx (1)
316-329: potential resource leak - server not closed on success pathon success (lines 310-315), the callback server isn't explicitly closed. the server does auto-shutdown after responding, but if the token exchange fails after receiving the callback,
closeServer()isn't called in all paths.actually wait - looking closer, the callback server shuts itself down after handling the request (via
setTimeout(shutdown, 500)in oauth-callback-server.ts), so this should be fine. but you could still add a finally block for cleanliness.🔧 optional: wrap in try-finally for clarity
} catch (err) { process.stdout.write("\r\x1b[K"); - closeServer(); const message = err instanceof Error ? err.message : String(err); if (message.includes("timed out")) { log.error("Authentication timed out. Please try again."); } else if (message.includes("State mismatch")) { log.error("Security check failed. Please try again."); } else { log.error(`Authentication failed: ${message}`); } return null; + } finally { + closeServer(); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/commands/login.tsx` around lines 316 - 329, The callback server may not be explicitly closed on all success/error paths; wrap the existing try/catch in a try/finally (or add a finally block) so closeServer() is always called regardless of outcome—ensure this change touches the same scope where closeServer() is invoked (the login flow handling the OAuth callback, around the code that currently catches errors and calls closeServer()), leaving existing error handling (message checks for "timed out" and "State mismatch") intact; this mirrors the auto-shutdown in oauth-callback-server.ts but guarantees cleanup if token exchange fails after callback.src/cli/oauth/oauth-callback-server.ts (1)
11-11: consider using bun.serve() per coding guidelinescoding guidelines suggest
Bun.serve()over node:http. node:http works fine in bun, but bun.serve is more idiomatic if this is a bun-first project.totally optional since node:http is compatible - just noting for consistency. meow 🐱
as per coding guidelines: "Use
Bun.serve()with support for WebSockets, HTTPS, and routes instead ofexpress"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/oauth/oauth-callback-server.ts` at line 11, Replace the node:http createServer usage with Bun.serve: remove the import of createServer/Server from "node:http", call Bun.serve(...) where createServer(...) is currently used (e.g., in the function that starts the OAuth callback server), convert request/response handling to the Bun Request/Response handler signature, and update any type references from Server to appropriate Bun types or narrow to unknown/void if no Bun types are available; ensure the same port, path handling, and shutdown logic are preserved when migrating the existing createServer-based handlers to Bun.serve.
🤖 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/cli/oauth/oauth-callback-server.ts`:
- Around line 50-80: The ERROR_HTML template function inserts the unescaped msg
parameter directly into the response, allowing XSS via attacker-controlled
error_description; fix by HTML-escaping msg before interpolation (e.g., add an
escapeHtml(html: string) helper that replaces &, <, >, ", ' and ` with entities)
and use ERROR_HTML(escapeHtml(msg)) (or call escapeHtml inside ERROR_HTML) so
any oauth redirect error_description cannot inject raw HTML/script.
- Around line 160-164: The code extracts the port with server.address() and
silently falls back to 0, producing an invalid redirect URI; update the oauth
callback logic (where server.listen, server.address and the port variable are
used) to validate the address/port after listen and throw a clear error if port
is missing or 0 (e.g., if typeof address !== "object" || !address ||
!address.port) so the function fails fast instead of constructing an invalid
redirect URI.
---
Nitpick comments:
In `@src/cli/commands/login.tsx`:
- Around line 316-329: The callback server may not be explicitly closed on all
success/error paths; wrap the existing try/catch in a try/finally (or add a
finally block) so closeServer() is always called regardless of outcome—ensure
this change touches the same scope where closeServer() is invoked (the login
flow handling the OAuth callback, around the code that currently catches errors
and calls closeServer()), leaving existing error handling (message checks for
"timed out" and "State mismatch") intact; this mirrors the auto-shutdown in
oauth-callback-server.ts but guarantees cleanup if token exchange fails after
callback.
In `@src/cli/oauth/oauth-callback-server.ts`:
- Line 11: Replace the node:http createServer usage with Bun.serve: remove the
import of createServer/Server from "node:http", call Bun.serve(...) where
createServer(...) is currently used (e.g., in the function that starts the OAuth
callback server), convert request/response handling to the Bun Request/Response
handler signature, and update any type references from Server to appropriate Bun
types or narrow to unknown/void if no Bun types are available; ensure the same
port, path handling, and shutdown logic are preserved when migrating the
existing createServer-based handlers to Bun.serve.
🪄 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
Run ID: b4f387bb-191a-4df3-a5d9-6d0bf434e8b1
📒 Files selected for processing (3)
src/cli/commands/login.tsxsrc/cli/oauth/oauth-callback-server.tssrc/cli/oauth/pkce.ts
Comment on lines
+50
to
+80
| const ERROR_HTML = (msg: string) => `<!DOCTYPE html> | ||
| <html> | ||
| <head> | ||
| <meta charset="utf-8"> | ||
| <title>varg.ai — Authentication Failed</title> | ||
| <style> | ||
| * { margin: 0; padding: 0; box-sizing: border-box; } | ||
| body { | ||
| font-family: -apple-system, BlinkMacSystemFont, "Segoe UI", Roboto, sans-serif; | ||
| background: #0a0a0a; color: #fafafa; | ||
| display: flex; align-items: center; justify-content: center; | ||
| min-height: 100vh; | ||
| } | ||
| .card { | ||
| text-align: center; padding: 3rem 2rem; | ||
| border: 1px solid #262626; border-radius: 12px; | ||
| background: #141414; max-width: 400px; | ||
| } | ||
| .icon { font-size: 48px; margin-bottom: 1rem; } | ||
| h1 { font-size: 1.25rem; margin-bottom: 0.5rem; } | ||
| p { color: #a3a3a3; font-size: 0.875rem; } | ||
| </style> | ||
| </head> | ||
| <body> | ||
| <div class="card"> | ||
| <div class="icon">✗</div> | ||
| <h1>Authentication Failed</h1> | ||
| <p>${msg}</p> | ||
| </div> | ||
| </body> | ||
| </html>`; |
Contributor
There was a problem hiding this comment.
xss vulnerability - error message not escaped
the msg parameter is injected directly into html without escaping. since error_description comes from the oauth redirect url (attacker-controlled), a malicious auth server or mitm could inject script tags.
🛡️ fix: escape html entities
+function escapeHtml(str: string): string {
+ return str
+ .replace(/&/g, "&")
+ .replace(/</g, "<")
+ .replace(/>/g, ">")
+ .replace(/"/g, """)
+ .replace(/'/g, "'");
+}
+
const ERROR_HTML = (msg: string) => `<!DOCTYPE html>
<html>
...
<h1>Authentication Failed</h1>
- <p>${msg}</p>
+ <p>${escapeHtml(msg)}</p>
</div>
</body>
</html>`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/oauth/oauth-callback-server.ts` around lines 50 - 80, The ERROR_HTML
template function inserts the unescaped msg parameter directly into the
response, allowing XSS via attacker-controlled error_description; fix by
HTML-escaping msg before interpolation (e.g., add an escapeHtml(html: string)
helper that replaces &, <, >, ", ' and ` with entities) and use
ERROR_HTML(escapeHtml(msg)) (or call escapeHtml inside ERROR_HTML) so any oauth
redirect error_description cannot inject raw HTML/script.
Comment on lines
+160
to
+164
| // Listen on a random available port on loopback | ||
| server.listen(0, "127.0.0.1"); | ||
|
|
||
| const address = server.address(); | ||
| const port = typeof address === "object" && address ? address.port : 0; |
Contributor
There was a problem hiding this comment.
edge case: port extraction could fail silently
if server.address() returns null (shouldn't happen after successful listen, but edge case), port becomes 0 and the redirect uri would be invalid. might want to throw early if port is 0.
🔧 suggested fix
const address = server.address();
const port = typeof address === "object" && address ? address.port : 0;
+ if (port === 0) {
+ throw new Error("Failed to start callback server: could not get port");
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/oauth/oauth-callback-server.ts` around lines 160 - 164, The code
extracts the port with server.address() and silently falls back to 0, producing
an invalid redirect URI; update the oauth callback logic (where server.listen,
server.address and the port variable are used) to validate the address/port
after listen and throw a clear error if port is missing or 0 (e.g., if typeof
address !== "object" || !address || !address.port) so the function fails fast
instead of constructing an invalid redirect URI.
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
vargai login, reusing the MCP OAuth 2.1 server infrastructure atmcp.varg.aiFlow
http://127.0.0.1:<random-port>POST mcp.varg.ai/oauth/registermcp.varg.ai/oauth/authorizewith PKCE challengeapp.varg.ai(Google OAuth or email OTP)~/.varg/credentialsNew files
src/cli/oauth/pkce.tssrc/cli/oauth/oauth-callback-server.tsModified files
src/cli/commands/login.tsxloginWithBrowser(), updated mode selector (browser=1, email=2, API key=3)Companion PR
feature/generic-consent-page— generalizes consent page text for non-MCP clients