fix unable to login if authentication is delegated to another CAS ser…#6865
fix unable to login if authentication is delegated to another CAS ser…#6865vasusadariya wants to merge 7 commits intoRocketChat:developfrom
Conversation
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a boolean guard Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant WebView
participant AppHandler as App (Auth Handler)
participant Server as Rocket.Chat Server
User->>WebView: Start SSO (SAML/CAS)
WebView->>Server: Navigate IdP / CAS flow
Server-->>WebView: Redirect to callback URL (may include ticket or saml_idp_credentialToken)
WebView->>AppHandler: onNavigation(url)
alt url startsWith(server) (isRocketChatServer)
AppHandler->>AppHandler: validate URL and extract token (ticket or saml_idp_credentialToken)
alt saml_idp_credentialToken present
AppHandler->>AppHandler: build payload { credentialToken, saml: true }
else CAS token path
AppHandler->>AppHandler: build payload { cas: { credentialToken: ssoToken } }
end
AppHandler->>Server: submit authentication payload
AppHandler->>WebView: debouncedLogin(...) -> close webview
else external redirect (not Rocket.Chat server)
AppHandler->>WebView: ignore / keep webview open
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
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)
app/views/AuthenticationWebView.tsx (1)
112-121: Declare thepayloadvariable before use.The variable
payloadis assigned at lines 115 and 117 but never declared, causing a TypeScript compilation error with strict mode enabled. Addlet payload;before the conditional at line 112.Additionally, while the
isRocketChatServerguard correctly prevents premature webview closure during CAS delegation, ensure the SAML vs CAS payload distinction is tested with both single-factor and delegated authentication flows.
📜 Review details
Configuration used: Organization 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)
app/views/AuthenticationWebView.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
app/views/AuthenticationWebView.tsx (1)
app/sagas/login.js (4)
server(86-86)server(230-230)server(299-299)server(375-375)
|
Hey |
|
Hello, and thank you for this PR. if (authType === 'saml' || authType === 'cas') {
const parsedUrl = parse(url, true);
// Only close the webview when redirected back to the Rocket.Chat server
// This prevents premature closure when CAS delegates to another CAS server for MFA
const isRocketChatServer = url.startsWith(server);
// ticket -> cas / validate & saml_idp_credentialToken -> saml
if (isRocketChatServer && (parsedUrl.pathname?.includes('validate') || parsedUrl.query?.ticket || parsedUrl.query?.saml_idp_credentialToken)) {
let payload: ICredentials;
if (authType === 'saml') {
const token = parsedUrl.query?.saml_idp_credentialToken || ssoToken;
const credentialToken = { credentialToken: token };
payload = { ...credentialToken, saml: true };
} else {
payload = { cas: { credentialToken: ssoToken } };
}
debouncedLogin(payload);
}
}We hope that this will be quickly incorporated into a future update of the mobile application 🙏 |
|
@vasusadariya can you add @floriannari changes? |
|
hey @diegolmello thanks! i will add these changes that @floriannari suggest me |
|
hey @diegolmello i have added all the changes that @floriannari has suggested so please go through it and if all good then you can merge it! |
|
Hello, So I'm afraid the PR won't be accepted as is. I've rewrite the commit if needed: floriannari@5aaeb23 |
|
hey @floriannari i will make the change and commit by tonight |
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Failure
OtavioStasiak
had a problem deploying
to
experimental_android_build
GitHub Actions
Failure
OtavioStasiak
had a problem deploying
to
official_android_build
GitHub Actions
Failure
OtavioStasiak
had a problem deploying
to
experimental_ios_build
GitHub Actions
Failure
OtavioStasiak
had a problem deploying
to
official_ios_build
GitHub Actions
Failure
…server for MFA (issue RocketChat#6833)
vasusadariya
force-pushed
the
authentication_fix
branch
from
3df6039 to
6de0ddd
Compare
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 (1)
app/views/AuthenticationWebView.tsx (1)
106-123:url.startsWith(server)is vulnerable to prefix-domain bypass attacks.An attacker can craft a redirect to
https://your.server.evil.tld/path?validatewhich will pass thestartsWithcheck and trigger login with untrusted credentials. Use parsed origin comparison instead: compareprotocol + hostof both URLs.Proposed fix (origin comparison)
if (authType === 'saml' || authType === 'cas') { const parsedUrl = parse(url, true); // Only close the webview when redirected back to the Rocket.Chat server // This prevents premature closure when CAS delegates to another CAS server for MFA - const isRocketChatServer = url.startsWith(server); + const parsedServer = parse(server, true); + const urlOrigin = `${parsedUrl.protocol}//${parsedUrl.host}`; + const serverOrigin = `${parsedServer.protocol}//${parsedServer.host}`; + const isRocketChatServer = urlOrigin === serverOrigin; // ticket -> cas / validate & saml_idp_credentialToken -> saml if (isRocketChatServer && (parsedUrl.pathname?.includes('validate') || parsedUrl.query?.ticket || parsedUrl.query?.saml_idp_credentialToken)) { let payload: ICredentials;
📜 Review details
Configuration used: Organization 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)
app/views/AuthenticationWebView.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
app/views/AuthenticationWebView.tsx (1)
app/sagas/login.js (4)
server(86-86)server(230-230)server(299-299)server(375-375)
|
Hello, |
|
@diegolmello Please review the commit and merge with the main repo if there is no errors! |
5 participants
Proposed changes
Fixed a critical bug in the CAS authentication flow that prevented multi-factor authentication (MFA) from completing. The issue occurred when using cascaded CAS servers where the first CAS server delegates to a second CAS server for MFA.
Previously, the
AuthenticationWebViewwould close prematurely when detecting any CAS ticket in the URL, including intermediate redirects between CAS servers. This fix adds a check to ensure the webview only closes when the authentication flow redirects back to the Rocket.Chat server itself, allowing the complete MFA flow to finish successfully.Changes:
Issue(s)
Fixes the CAS MFA authentication issue where the app closes the authentication webview before completing the second factor validation.
How to test or reproduce
Setup:
Steps to reproduce the bug (before fix):
Expected behavior (after fix):
Affected screens:
AuthenticationWebView.tsx- CAS/SAML authentication flowScreenshots
N/A - Authentication flow behavior (no visual UI changes)
Types of changes
Checklist
Further comments
This is a targeted fix that addresses the root cause without affecting other authentication flows. The change is minimal and adds a single conditional check to verify the redirect URL matches the Rocket.Chat server before triggering the login process.
Technical details:
url.includes(server)before processing CAS/SAML ticketsThis fix is backward compatible and doesn't affect:
Summary by CodeRabbit