Skip to content

Commit 37c4de6

Browse files
mattermost-buildpvev
mattermost-build
and
pvev
authored
MM-66813 - enhance sso callback metadata (#9422) (#9522)
* MM-66813 - enhance sso callback metadata * fix translation order * Add serverUrl validation, remove non-null assertions (cherry picked from commit 3735aa6) Co-authored-by: Pablo Vélez <pablovv2016@gmail.com>
1 parent 4ef7d5d commit 37c4de6

File tree

5 files changed

+134
-9
lines changed

5 files changed

+134
-9
lines changed

‎app/screens/sso/index.tsx‎

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,18 @@ const SSO = ({
5555
const intl = useIntl();
5656
const [loginError, setLoginError] = useState<string>('');
5757

58+
// Validate serverUrl is provided
59+
if (!serverUrl) {
60+
return (
61+
<View
62+
nativeID={SecurityManager.getShieldScreenId(componentId, false, true)}
63+
style={styles.flex}
64+
>
65+
<Background theme={theme}/>
66+
</View>
67+
);
68+
}
69+
5870
let loginUrl = '';
5971
let shouldUseNativeEntra = false;
6072

@@ -109,7 +121,7 @@ const SSO = ({
109121
}, [intl]);
110122

111123
const doSSOLogin = async (bearerToken: string, csrfToken: string) => {
112-
const result: LoginActionResponse = await ssoLogin(serverUrl!, serverDisplayName, config.DiagnosticId!, bearerToken, csrfToken, serverPreauthSecret);
124+
const result: LoginActionResponse = await ssoLogin(serverUrl, serverDisplayName, config.DiagnosticId!, bearerToken, csrfToken, serverPreauthSecret);
113125
if (result?.error && result.failed) {
114126
onLoadEndError(result.error);
115127
return;
@@ -118,7 +130,7 @@ const SSO = ({
118130
};
119131

120132
const doSSOCodeExchange = async (loginCode: string, samlChallenge: {codeVerifier: string; state: string}) => {
121-
const result: LoginActionResponse = await ssoLoginWithCodeExchange(serverUrl!, serverDisplayName, config.DiagnosticId!, loginCode, samlChallenge, serverPreauthSecret);
133+
const result: LoginActionResponse = await ssoLoginWithCodeExchange(serverUrl, serverDisplayName, config.DiagnosticId!, loginCode, samlChallenge, serverPreauthSecret);
122134
if (result?.error && result.failed) {
123135
onLoadEndError(result.error);
124136
return;
@@ -133,7 +145,7 @@ const SSO = ({
133145

134146
const doEntraLogin = useCallback(async () => {
135147
const result = await nativeEntraLogin(
136-
serverUrl!,
148+
serverUrl,
137149
serverDisplayName,
138150
config.DiagnosticId!,
139151
config.IntuneScope!,
@@ -173,6 +185,7 @@ const SSO = ({
173185
doSSOCodeExchange,
174186
loginError,
175187
loginUrl,
188+
serverUrl,
176189
setLoginError,
177190
theme,
178191
};

‎app/screens/sso/sso.test.tsx‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import SSOAuthentication from './sso_authentication';
1212
jest.mock('@utils/url', () => {
1313
return {
1414
tryOpenURL: () => null,
15+
sanitizeUrl: (url: string) => url.replace(/\/+$/, '').toLowerCase(),
1516
};
1617
});
1718

@@ -23,6 +24,7 @@ describe('SSO with redirect url', () => {
2324
intl: {},
2425
loginError: '',
2526
loginUrl: '',
27+
serverUrl: 'https://example.mattermost.com',
2628
setLoginError: jest.fn(),
2729
theme: Preferences.THEMES.denim,
2830
};
@@ -49,3 +51,48 @@ describe('SSO with redirect url', () => {
4951
expect(browser).toBeUndefined();
5052
});
5153
});
54+
55+
describe('Server origin verification', () => {
56+
// Test the URL normalization logic used in server origin verification
57+
const {sanitizeUrl} = jest.requireMock('@utils/url');
58+
59+
test('should normalize URLs by removing trailing slashes', () => {
60+
expect(sanitizeUrl('https://example.com/')).toBe('https://example.com');
61+
expect(sanitizeUrl('https://example.com')).toBe('https://example.com');
62+
});
63+
64+
test('should normalize URLs to lowercase', () => {
65+
expect(sanitizeUrl('https://EXAMPLE.COM')).toBe('https://example.com');
66+
expect(sanitizeUrl('https://Example.Mattermost.Com')).toBe('https://example.mattermost.com');
67+
});
68+
69+
test('should match identical normalized URLs', () => {
70+
const serverUrl = 'https://example.mattermost.com';
71+
const srvParam = 'https://example.mattermost.com';
72+
expect(sanitizeUrl(serverUrl)).toBe(sanitizeUrl(srvParam));
73+
});
74+
75+
test('should match URLs with different casing', () => {
76+
const serverUrl = 'https://example.mattermost.com';
77+
const srvParam = 'https://EXAMPLE.MATTERMOST.COM';
78+
expect(sanitizeUrl(serverUrl)).toBe(sanitizeUrl(srvParam));
79+
});
80+
81+
test('should match URLs with trailing slash differences', () => {
82+
const serverUrl = 'https://example.mattermost.com';
83+
const srvParam = 'https://example.mattermost.com/';
84+
expect(sanitizeUrl(serverUrl)).toBe(sanitizeUrl(srvParam));
85+
});
86+
87+
test('should not match different server URLs', () => {
88+
const serverUrl = 'https://legitimate.mattermost.com';
89+
const srvParam = 'https://malicious.attacker.com';
90+
expect(sanitizeUrl(serverUrl)).not.toBe(sanitizeUrl(srvParam));
91+
});
92+
93+
test('should not match when server URL contains attacker URL as substring', () => {
94+
const serverUrl = 'https://legitimate.mattermost.com';
95+
const srvParam = 'https://legitimate.mattermost.com.attacker.com';
96+
expect(sanitizeUrl(serverUrl)).not.toBe(sanitizeUrl(srvParam));
97+
});
98+
});

‎app/screens/sso/sso_authentication.tsx‎

Lines changed: 42 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import urlParse from 'url-parse';
1111
import {Sso} from '@constants';
1212
import {isBetaApp} from '@utils/general';
1313
import {createSamlChallenge} from '@utils/saml_challenge';
14+
import {sanitizeUrl} from '@utils/url';
1415

1516
import AuthError from './components/auth_error';
1617
import AuthRedirect from './components/auth_redirect';
@@ -21,6 +22,7 @@ interface SSOAuthenticationProps {
2122
doSSOCodeExchange: (loginCode: string, samlChallenge: {codeVerifier: string; state: string}) => void;
2223
loginError: string;
2324
loginUrl: string;
25+
serverUrl: string;
2426
setLoginError: (value: string) => void;
2527
theme: Theme;
2628
}
@@ -32,7 +34,7 @@ const style = StyleSheet.create({
3234
},
3335
});
3436

35-
const SSOAuthentication = ({doSSOLogin, doSSOCodeExchange, loginError, loginUrl, setLoginError, theme}: SSOAuthenticationProps) => {
37+
const SSOAuthentication = ({doSSOLogin, doSSOCodeExchange, loginError, loginUrl, serverUrl, setLoginError, theme}: SSOAuthenticationProps) => {
3638
const [error, setError] = useState<string>('');
3739
const [loginSuccess, setLoginSuccess] = useState(false);
3840
const intl = useIntl();
@@ -43,6 +45,17 @@ const SSOAuthentication = ({doSSOLogin, doSSOCodeExchange, loginError, loginUrl,
4345

4446
const redirectUrl = customUrlScheme + 'callback';
4547
const samlChallenge = useMemo(() => createSamlChallenge(), []);
48+
49+
// Verify that the srv parameter from the callback matches the expected server
50+
const verifyServerOrigin = useCallback((srvParam: string | undefined): boolean => {
51+
if (!srvParam) {
52+
// Old servers don't send srv parameter - allow for backwards compatibility
53+
return true;
54+
}
55+
const normalizedExpected = sanitizeUrl(serverUrl);
56+
const normalizedActual = sanitizeUrl(srvParam);
57+
return normalizedExpected === normalizedActual;
58+
}, [serverUrl]);
4659
const init = useCallback(async (resetErrors = true) => {
4760
setLoginSuccess(false);
4861
if (resetErrors !== false) {
@@ -62,6 +75,19 @@ const SSOAuthentication = ({doSSOLogin, doSSOCodeExchange, loginError, loginUrl,
6275
const result = await openAuthSessionAsync(url, null, {preferEphemeralSession: true, createTask: false});
6376
if ('url' in result && result.url) {
6477
const resultUrl = urlParse(result.url, true);
78+
const srvParam = resultUrl.query?.srv as string | undefined;
79+
80+
// Verify server origin before accepting credentials
81+
if (!verifyServerOrigin(srvParam)) {
82+
setError(
83+
intl.formatMessage({
84+
id: 'mobile.oauth.server_mismatch',
85+
defaultMessage: 'Login failed: Unable to complete authentication with this server. Please try again.',
86+
}),
87+
);
88+
return;
89+
}
90+
6591
const loginCode = resultUrl.query?.login_code as string | undefined;
6692
if (loginCode) {
6793
// Prefer code exchange when available
@@ -83,7 +109,7 @@ const SSOAuthentication = ({doSSOLogin, doSSOCodeExchange, loginError, loginUrl,
83109
}),
84110
);
85111
}
86-
}, [doSSOCodeExchange, doSSOLogin, intl, loginUrl, samlChallenge, redirectUrl, setLoginError]);
112+
}, [doSSOCodeExchange, doSSOLogin, intl, loginUrl, samlChallenge, redirectUrl, setLoginError, verifyServerOrigin]);
87113

88114
useEffect(() => {
89115
let listener: EventSubscription | null = null;
@@ -93,6 +119,19 @@ const SSOAuthentication = ({doSSOLogin, doSSOCodeExchange, loginError, loginUrl,
93119
setError('');
94120
if (url && url.startsWith(redirectUrl)) {
95121
const parsedUrl = urlParse(url, true);
122+
const srvParam = parsedUrl.query?.srv as string | undefined;
123+
124+
// Verify server origin before accepting credentials
125+
if (!verifyServerOrigin(srvParam)) {
126+
setError(
127+
intl.formatMessage({
128+
id: 'mobile.oauth.server_mismatch',
129+
defaultMessage: 'Login failed: Unable to complete authentication with this server. Please try again.',
130+
}),
131+
);
132+
return;
133+
}
134+
96135
const loginCode = parsedUrl.query?.login_code as string | undefined;
97136
if (loginCode) {
98137
setLoginSuccess(true);
@@ -126,7 +165,7 @@ const SSOAuthentication = ({doSSOLogin, doSSOCodeExchange, loginError, loginUrl,
126165
clearTimeout(timeout);
127166
listener?.remove();
128167
};
129-
}, [doSSOCodeExchange, doSSOLogin, init, intl, samlChallenge, redirectUrl]);
168+
}, [doSSOCodeExchange, doSSOLogin, init, intl, samlChallenge, redirectUrl, verifyServerOrigin]);
130169

131170
let content;
132171
if (loginSuccess) {

‎app/screens/sso/sso_authentication_with_external_browser.tsx‎

Lines changed: 28 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ import {isBetaApp} from '@utils/general';
1313
import {createSamlChallenge} from '@utils/saml_challenge';
1414
import {changeOpacity, makeStyleSheetFromTheme} from '@utils/theme';
1515
import {typography} from '@utils/typography';
16-
import {tryOpenURL} from '@utils/url';
16+
import {sanitizeUrl, tryOpenURL} from '@utils/url';
1717

1818
import AuthError from './components/auth_error';
1919
import AuthRedirect from './components/auth_redirect';
@@ -24,6 +24,7 @@ interface SSOWithRedirectURLProps {
2424
doSSOCodeExchange: (loginCode: string, samlChallenge: {codeVerifier: string; state: string}) => void;
2525
loginError: string;
2626
loginUrl: string;
27+
serverUrl: string;
2728
setLoginError: (value: string) => void;
2829
theme: Theme;
2930
}
@@ -59,7 +60,7 @@ const getStyleSheet = makeStyleSheetFromTheme((theme: Theme) => {
5960
};
6061
});
6162

62-
const SSOAuthenticationWithExternalBrowser = ({doSSOLogin, doSSOCodeExchange, loginError, loginUrl, setLoginError, theme}: SSOWithRedirectURLProps) => {
63+
const SSOAuthenticationWithExternalBrowser = ({doSSOLogin, doSSOCodeExchange, loginError, loginUrl, serverUrl, setLoginError, theme}: SSOWithRedirectURLProps) => {
6364
const [error, setError] = useState<string>('');
6465
const [loginSuccess, setLoginSuccess] = useState(false);
6566
const style = getStyleSheet(theme);
@@ -71,6 +72,17 @@ const SSOAuthenticationWithExternalBrowser = ({doSSOLogin, doSSOCodeExchange, lo
7172

7273
const redirectUrl = customUrlScheme + 'callback';
7374
const samlChallenge = useMemo(() => createSamlChallenge(), []);
75+
76+
// Verify that the srv parameter from the callback matches the expected server
77+
const verifyServerOrigin = useCallback((srvParam: string | undefined): boolean => {
78+
if (!srvParam) {
79+
// Old servers don't send srv parameter - allow for backwards compatibility
80+
return true;
81+
}
82+
const normalizedExpected = sanitizeUrl(serverUrl);
83+
const normalizedActual = sanitizeUrl(srvParam);
84+
return normalizedExpected === normalizedActual;
85+
}, [serverUrl]);
7486
const init = useCallback((resetErrors = true) => {
7587
setLoginSuccess(false);
7688
if (resetErrors !== false) {
@@ -114,6 +126,19 @@ const SSOAuthenticationWithExternalBrowser = ({doSSOLogin, doSSOCodeExchange, lo
114126
const onURLChange = ({url}: { url: string }) => {
115127
if (url && url.startsWith(redirectUrl)) {
116128
const parsedUrl = urlParse(url, true);
129+
const srvParam = parsedUrl.query?.srv as string | undefined;
130+
131+
// Verify server origin before accepting credentials
132+
if (!verifyServerOrigin(srvParam)) {
133+
setError(
134+
intl.formatMessage({
135+
id: 'mobile.oauth.server_mismatch',
136+
defaultMessage: 'Login failed: Unable to complete authentication with this server. Please try again.',
137+
}),
138+
);
139+
return;
140+
}
141+
117142
const loginCode = parsedUrl.query?.login_code as string | undefined;
118143
if (loginCode) {
119144
setLoginSuccess(true);
@@ -148,7 +173,7 @@ const SSOAuthenticationWithExternalBrowser = ({doSSOLogin, doSSOCodeExchange, lo
148173
listener.remove();
149174
clearTimeout(timeout);
150175
};
151-
}, [doSSOCodeExchange, doSSOLogin, init, intl, samlChallenge, redirectUrl]);
176+
}, [doSSOCodeExchange, doSSOLogin, init, intl, samlChallenge, redirectUrl, verifyServerOrigin]);
152177

153178
let content;
154179
if (loginSuccess) {

‎assets/base/i18n/en.json‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -796,6 +796,7 @@
796796
"mobile.oauth.failed_to_login": "Your login attempt failed. Please try again.",
797797
"mobile.oauth.failed_to_open_link": "The link failed to open. Please try again.",
798798
"mobile.oauth.failed_to_open_link_no_browser": "The link failed to open. Please verify that a browser is installed on the device.",
799+
"mobile.oauth.server_mismatch": "Login failed: Unable to complete authentication with this server. Please try again.",
799800
"mobile.oauth.something_wrong.okButton": "OK",
800801
"mobile.oauth.success.description": "Signing in now, just a moment...",
801802
"mobile.oauth.success.title": "Authentication successful",

0 commit comments

Comments
 (0)