feat: redirect authentication popups to default browser #3088
Conversation
Resolves RocketChat#2993 - Add isAuthenticationPopup() helper to detect login/OAuth/SSO popups - Redirect authentication flows to default browser instead of Electron popup - Allow users to access saved credentials and passkeys from their browser - Support major OAuth providers (Google, Microsoft, GitHub, GitLab, etc.) - Maintain compatibility with existing video call and other popup flows This change improves user experience by leveraging browser-saved credentials and enables passkey authentication that was previously blocked in Electron popups.
- Add complete isAuthenticationPopup function definition - Fix broken code structure from manual edits - Ensure all function calls have proper scope and variables - Maintain clean switch statement structure in permission handler
feat/default-browser-auth
|
|
- Require both client identifier (client_id/redirect_uri) AND flow parameter - Prevents false positives from single parameters like 'state=california' - Maintains support for SAML flows with SAMLRequest/SAMLResponse - Addresses maintainer feedback for more robust authentication detection
WalkthroughImplements detection of authentication-related popups and redirects them to the system browser in window-open and did-create-window flows. Adds URL/frame/disposition analysis, handles protocol permission checks, and updates media permission default to deny. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant WV as WebView
participant SV as ServerView Logic
participant D as Default Browser
rect rgba(200,230,255,0.25)
note over WV,SV: window.open handler
WV->>SV: Request to open URL (url, frameName, disposition)
SV->>SV: isAuthenticationPopup(url, frameName, disposition)?
alt Auth popup detected
SV->>SV: Check protocol permission
alt Allowed
SV->>D: openExternal(url)
SV-->>WV: deny Electron popup
else Not allowed
SV-->>WV: proceed with normal handling
end
else Not auth popup
SV-->>WV: proceed with normal handling
end
end
rect rgba(220,255,220,0.25)
note over SV,D: did-create-window flow
SV->>SV: isAuthenticationPopup(newWindow.url,... )?
alt Auth popup detected
SV->>SV: destroy popup window
SV->>D: openExternal(url)
else Not auth popup
SV->>SV: keep popup window
end
end
rect rgba(255,240,200,0.25)
note over SV: Permission handling
SV->>SV: media permission default = false (deny)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/ui/main/serverView/index.ts (2)
417-431: Critical: Media access broken on Windows and Linux.The change at line 430 from
callback(true)tocallback(false)denies all media permissions on non-macOS platforms by default. This will break video calls, voice messages, and any feature requiring camera or microphone access on Windows and Linux.This appears to be an unintended change that breaks core functionality. Please verify:
- Was this change intentional?
- If so, what is the reasoning for blocking media access on Windows/Linux?
- If not, this should be reverted.
Apply this diff to restore media access on Windows/Linux:
- callback(false); + callback(true); return; }If there's a security concern that motivated this change, consider implementing proper permission prompts for Windows/Linux similar to the macOS flow, rather than blocking all media access.
380-391: Remove dead Google Sign-In userAgent override.
SinceisAuthenticationPopupalready redirects any window withframeName === 'Login', theisGoogleSignInbranch (lines 388–391) never executes. Remove that block and the associateduserAgentoverride.
🧹 Nitpick comments (1)
src/ui/main/serverView/index.ts (1)
119-135: Simplify domain pattern matching for known auth providers.The regex patterns for authentication providers (lines 120-131) all follow the same subdomain pattern
^([a-z0-9-]+\.)*domain\.tld$. This is more complex than needed and could be simplified.Consider simplifying the domain checks:
- const authProviders = [ - /^([a-z0-9-]+\.)*google\.com$/, - /^([a-z0-9-]+\.)*microsoft\.com$/, - /^([a-z0-9-]+\.)*microsoftonline\.com$/, - /^([a-z0-9-]+\.)*facebook\.com$/, - /^([a-z0-9-]+\.)*github\.com$/, - /^([a-z0-9-]+\.)*gitlab\.com$/, - /^([a-z0-9-]+\.)*okta\.com$/, - /^([a-z0-9-]+\.)*auth0\.com$/, - /^([a-z0-9-]+\.)*saml\.com$/, - /^([a-z0-9-]+\.)*onelogin\.com$/, - ]; - - if (authProviders.some((pattern) => parsedUrl.hostname.match(pattern))) { - return true; - } + const authProviderDomains = [ + 'google.com', + 'microsoft.com', + 'microsoftonline.com', + 'facebook.com', + 'github.com', + 'gitlab.com', + 'okta.com', + 'auth0.com', + 'saml.com', + 'onelogin.com', + ]; + + if (authProviderDomains.some((domain) => + parsedUrl.hostname === domain || parsedUrl.hostname.endsWith('.' + domain) + )) { + return true; + }This approach is clearer, easier to maintain, and has the same matching behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/ui/main/serverView/index.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Follow existing TypeScript patterns with strict mode enabled
All new code must pass ESLint and TypeScript checks
Avoid unnecessary comments; only add comments for complex logic or non-obvious decisions
Files:
src/ui/main/serverView/index.ts
🧬 Code graph analysis (1)
src/ui/main/serverView/index.ts (2)
src/navigation/main.ts (1)
isProtocolAllowed(213-241)src/utils/browserLauncher.ts (1)
openExternal(58-90)
🔇 Additional comments (2)
src/ui/main/serverView/index.ts (2)
334-347: LGTM! Clean authentication popup handling in window open handler.The integration correctly:
- Detects authentication popups before allowing standard popup creation
- Checks protocol permissions before opening externally
- Handles errors gracefully without crashing
- Returns
{ action: 'deny' }to prevent the Electron popup from being createdThe promise chain with
.then().catch()is appropriate here since the function returns synchronously.
103-117: Restrict auth keyword matching to pathname and search
Matching generic terms (e.g., “auth”, “login”) anywhere in the URL can flag benign popups (e.g.,/unauthorized-error). Instead useparsedUrl.pathname + parsedUrl.searchwith path-segment prefixes (e.g.,'/auth','/login') or word boundaries.
|
Hello, I don't have the skills to say if the code is clean enough, if the edge cases are taken into account, or anything of the sort. I'm just here to highlight #2993 and our need for it to be fixed. |
3 participants
Closes #2993
🎯 Problem Solved
The desktop app was showing authentication popups in isolated Electron windows that don't have access to:
This was particularly problematic for users with SSO setups requiring passkeys, as mentioned in the issue - they couldn't log in at all. Additionally, users with custom/in-house OAuth providers (like on-premise BitWarden) couldn't authenticate properly.
🔧 Solution Implemented
Added intelligent authentication detection that redirects OAuth/SSO login flows to the user's default browser instead of creating isolated Electron popups.
Key Features:
client_id/redirect_uri) AND flow parameters (response_type/state/scope/etc.) to avoid false positivesLogin,OAuth,SSO), URL patterns, and known provider domainsTechnical Implementation:
isAuthenticationPopup()function with multi-layered detection:setWindowOpenHandlerto redirect auth popups to default browserdid-create-windowlistener to handle fallback scenarios📊 Enhanced Detection Logic:
🔒 OAuth Parameter Detection (Stricter Logic)
client_id,redirect_uriresponse_type,state,scope,code_challenge,nonceSAMLRequest,SAMLResponseLogic: Requires both a client identifier AND a flow parameter, or any SAML parameter (definitive indicators). This prevents false positives from single parameters like
state=california.🎯 Detection Priority (Top to Bottom):
Login,OAuth,SSOframe namesoauth,auth,login,signin,sso,authenticate✅ Testing
npm run build)npm run lint)tsc --noEmit)npm test)🚀 Benefits
Before:
After:
🙏 Acknowledgments
Special thanks to @sdlth for the valuable feedback that led to the OAuth parameter detection enhancement and the stricter logic implementation to prevent false positives!
This change significantly improves the user experience while maintaining security and compatibility with existing features.
Summary by CodeRabbit