diff --git a/.changeset/saml-mfa-deeplink-relaystate-fix.md b/.changeset/saml-mfa-deeplink-relaystate-fix.md new file mode 100644 index 0000000000000..60ae867b5d3a0 --- /dev/null +++ b/.changeset/saml-mfa-deeplink-relaystate-fix.md @@ -0,0 +1,9 @@ +--- +'@rocket.chat/meteor': patch +--- + +Fixes SAML + IdP MFA deep-link callback failure on mobile devices introduced in 8.8.0. + +When an Identity Provider (IdP) enforces multi-step 2FA/MFA challenges (such as Okta Verify, Azure AD Conditional Access, Duo, or PingIdentity), the SAML `RelayState` parameter containing the `loginClient` context was not correctly decoded. IdPs may URL-encode the `RelayState` value or reorder its query parameters across MFA redirect hops. As a result, `loginClient` was silently lost, causing the server to omit the `&loginClient=mobile` segment from the post-authentication redirect. The mobile app never received the `rocketchat://auth` deep-link callback and remained stuck on the login screen. + +The fix updates `SAMLUtils.decodeAuthorizeRelayState` to handle position-independent parameter parsing and safely attempt `decodeURIComponent` only when the string contains no literal separators, preserving provider values that legitimately contain `&` or `=` characters. diff --git a/apps/meteor/server/lib/saml/lib/Utils.ts b/apps/meteor/server/lib/saml/lib/Utils.ts index efaf3490c072d..9768306b7baef 100644 --- a/apps/meteor/server/lib/saml/lib/Utils.ts +++ b/apps/meteor/server/lib/saml/lib/Utils.ts @@ -1,3 +1,4 @@ +import { Buffer } from 'node:buffer'; import crypto from 'node:crypto'; import { EventEmitter } from 'node:events'; import zlib from 'node:zlib'; @@ -159,12 +160,78 @@ export class SAMLUtils { return new URLSearchParams({ provider, loginClient }).toString(); } + // public static decodeAuthorizeRelayState(relayState?: string | null): { provider?: string; loginClient?: 'desktop' | 'mobile' } { + // if (!relayState) { + // return {}; + // } + + // // CASE 1: Compound RelayState in the format produced by encodeAuthorizeRelayState. + // // 'tenant&provider=other&loginClient=mobile' — will never begin with 'provider='. + + // if (relayState.includes('loginClient=')) { + // const params = new URLSearchParams(relayState); + // const provider = params.get('provider') ?? undefined; + // const loginClient = params.get('loginClient'); + // return { + // provider, + // loginClient: this.isSupportedLoginClient(loginClient) ? loginClient : undefined, + // }; + // } + + // // CASE 2: The IdP wrapped the entire RelayState in encodeURIComponent, producing a + // // single opaque blob with no literal separators (e.g. 'provider%3Dtest-sp%26loginClient%3Dmobile'). + + // if (!relayState.includes('&') && !relayState.includes('=') && relayState.includes('%')) { + // try { + // const decoded = decodeURIComponent(relayState); + // if (decoded.startsWith('provider=') && decoded.includes('&loginClient=')) { + // const params = new URLSearchParams(decoded); + // const provider = params.get('provider') ?? undefined; + // const loginClient = params.get('loginClient'); + // if (provider && this.isSupportedLoginClient(loginClient)) { + // return { provider, loginClient }; + // } + // } + // } catch (err) { + // this.log({ msg: 'Failed to decode relay state', err }); + // } + // } + + // // CASE 3: The IdP encoded the '=' signs within each segment but kept literal '&' + // // as the outer separator (e.g. 'provider%3Dtest-sp&loginClient%3Dmobile'). + + // if ((relayState.includes('%3D') || relayState.includes('%3d')) && relayState.includes('&')) { + // try { + // const rebuilt = relayState + // .split('&') + // .map((s) => s.replace(/%3D/i, '=')) + // .join('&'); + + // const params = new URLSearchParams(rebuilt); + // const provider = params.get('provider') ?? undefined; + // const loginClient = params.get('loginClient'); + + // if (provider && this.isSupportedLoginClient(loginClient)) { + // return { provider, loginClient }; + // } + // } catch (err) { + // this.log({ msg: 'Failed to decode relay state', err }); + // } + // } + + // return { provider: relayState }; + // } + public static decodeAuthorizeRelayState(relayState?: string | null): { provider?: string; loginClient?: 'desktop' | 'mobile' } { if (!relayState) { return {}; } - if (relayState.startsWith('provider=') && relayState.includes('&loginClient=')) { + // CASE 1: Compound RelayState in the format produced by encodeAuthorizeRelayState. + // Parameter order is irrelevant: + // 'provider=test-sp&loginClient=mobile' + // 'loginClient=mobile&provider=test-sp' + if (relayState.includes('loginClient=')) { const params = new URLSearchParams(relayState); const provider = params.get('provider') ?? undefined; const loginClient = params.get('loginClient'); @@ -175,6 +242,53 @@ export class SAMLUtils { }; } + // CASE 2: The IdP URL-encoded the complete RelayState. + // Example: + // 'provider%3Dtest-sp%26loginClient%3Dmobile' + if ((relayState.includes('%3D') || relayState.includes('%3d')) && !relayState.includes('&')) { + try { + const decoded = decodeURIComponent(relayState); + const params = new URLSearchParams(decoded); + + const provider = params.get('provider') ?? undefined; + const loginClient = params.get('loginClient'); + + if (provider && this.isSupportedLoginClient(loginClient)) { + return { provider, loginClient }; + } + } catch (err) { + this.log({ msg: 'Failed to decode relay state', err }); + } + } + + // CASE 3: The IdP encoded the '=' separators within each segment + // but kept '&' as the outer separator. + // + // Example: + // 'provider%3Dtest-sp&loginClient%3Dmobile' + // + // Decode only the '=' separator instead of decoding the whole segment. + // This prevents an encoded '%26' inside a value from becoming a new + // query separator. + if ((relayState.includes('%3D') || relayState.includes('%3d')) && relayState.includes('&')) { + try { + const rebuilt = relayState + .split('&') + .map((segment) => segment.replace(/%3D/gi, '=')) + .join('&'); + + const params = new URLSearchParams(rebuilt); + const provider = params.get('provider') ?? undefined; + const loginClient = params.get('loginClient'); + + if (provider && this.isSupportedLoginClient(loginClient)) { + return { provider, loginClient }; + } + } catch (err) { + this.log({ msg: 'Failed to decode relay state', err }); + } + } + return { provider: relayState }; } @@ -198,7 +312,7 @@ export class SAMLUtils { public static async inflateXml(deflatedXml: Buffer): Promise> { return new Promise((resolve, reject) => { - zlib.inflateRaw(deflatedXml, (err, inflatedXml) => { + zlib.inflateRaw(deflatedXml, (err: Error | null, inflatedXml: Buffer) => { if (err) { this.log({ msg: 'Error while inflating.', err }); return reject(err); diff --git a/apps/meteor/tests/unit/server/lib/saml/server.tests.ts b/apps/meteor/tests/unit/server/lib/saml/server.tests.ts index a2dc7f3bbf9a4..2eec57b2ad79d 100644 --- a/apps/meteor/tests/unit/server/lib/saml/server.tests.ts +++ b/apps/meteor/tests/unit/server/lib/saml/server.tests.ts @@ -1160,6 +1160,28 @@ describe('SAML', () => { const encoded = SAMLUtils.encodeAuthorizeRelayState('a b&c=d', 'mobile'); expect(SAMLUtils.decodeAuthorizeRelayState(encoded)).to.be.deep.equal({ provider: 'a b&c=d', loginClient: 'mobile' }); }); + + it('should handle fully URL-encoded RelayState from multi-step IdPs', () => { + // IdP wrapped the entire RelayState in encodeURIComponent + const encoded = encodeURIComponent('provider=test-sp&loginClient=mobile'); + expect(SAMLUtils.decodeAuthorizeRelayState(encoded)).to.be.deep.equal({ provider: 'test-sp', loginClient: 'mobile' }); + }); + + it('should handle mixed-encoded RelayState where only "=" is encoded but "&" is literal', () => { + + expect(SAMLUtils.decodeAuthorizeRelayState('provider%3Dtest-sp&loginClient%3Dmobile')).to.be.deep.equal({ + provider: 'test-sp', + loginClient: 'mobile', + }); + }); + + it('should not misread a raw provider name that contains query-like substrings', () => { + // A provider named 'tenant&provider=other&loginClient=mobile' must not be + // parsed as a compound RelayState — it does not start with 'provider='. + expect(SAMLUtils.decodeAuthorizeRelayState('tenant&provider=other&loginClient=mobile')).to.be.deep.equal({ + provider: 'tenant&provider=other&loginClient=mobile', + }); + }); }); });