diff --git a/app/actions/remote/general.test.ts b/app/actions/remote/general.test.ts index 14517b029e4..b72ad3de30c 100644 --- a/app/actions/remote/general.test.ts +++ b/app/actions/remote/general.test.ts @@ -43,12 +43,12 @@ describe('doPing', () => { expect(result).toHaveProperty('error'); }); - it('should return empty object on successful ping without push proxy verification', async () => { + it('should return serverUrl on successful ping without push proxy verification', async () => { const mockClient = makePingClient(); (NetworkManager.createClient as jest.Mock).mockResolvedValue(mockClient); const result = await doPing(serverUrl, false); - expect(result).toEqual({}); + expect(result).toEqual({serverUrl}); }); it('should return certificate error on 401 response', async () => { @@ -69,6 +69,120 @@ describe('doPing', () => { expect(NetworkManager.invalidateClient).toHaveBeenCalledWith(serverUrl); }); + it('should retry ping once with base_url from 406 response', async () => { + const wrongUrl = 'https://example.com/wrong'; + const correctUrl = 'https://example.com/correct'; + const failingClient = makePingClient({ + ping: jest.fn().mockResolvedValue({ + ok: false, + code: 406, + data: {base_url: correctUrl}, + headers: {}, + }), + }); + const successClient = makePingClient(); + (NetworkManager.createClient as jest.Mock). + mockResolvedValueOnce(failingClient). + mockResolvedValueOnce(successClient); + + const result = await doPing(wrongUrl, false); + expect(result).toEqual({serverUrl: correctUrl}); + expect(NetworkManager.createClient).toHaveBeenCalledTimes(2); + expect(NetworkManager.createClient).toHaveBeenNthCalledWith(1, wrongUrl, undefined, undefined); + expect(NetworkManager.createClient).toHaveBeenNthCalledWith(2, correctUrl, undefined, undefined); + expect(NetworkManager.invalidateClient).toHaveBeenCalledWith(wrongUrl); + }); + + it('should sanitize base_url before retrying ping', async () => { + const wrongUrl = 'https://example.com/wrong'; + const correctUrl = 'https://example.com/correct/'; + const failingClient = makePingClient({ + ping: jest.fn().mockResolvedValue({ + ok: false, + code: 406, + data: {base_url: correctUrl}, + headers: {}, + }), + }); + const successClient = makePingClient(); + (NetworkManager.createClient as jest.Mock). + mockResolvedValueOnce(failingClient). + mockResolvedValueOnce(successClient); + + const result = await doPing(wrongUrl, false); + expect(result).toEqual({serverUrl: 'https://example.com/correct'}); + expect(NetworkManager.createClient).toHaveBeenNthCalledWith(2, 'https://example.com/correct', undefined, undefined); + }); + + it('should create a new client when retrying ping with base_url and external client was provided', async () => { + const wrongUrl = 'https://example.com/wrong'; + const correctUrl = 'https://example.com/correct'; + const externalClient = makePingClient({ + ping: jest.fn().mockResolvedValue({ + ok: false, + code: 406, + data: {base_url: correctUrl}, + headers: {}, + }), + }); + const successClient = makePingClient(); + (NetworkManager.createClient as jest.Mock).mockResolvedValueOnce(successClient); + + const result = await doPing(wrongUrl, false, 5000, undefined, externalClient as any); + expect(result).toEqual({serverUrl: correctUrl}); + expect(NetworkManager.createClient).toHaveBeenCalledTimes(1); + expect(NetworkManager.createClient).toHaveBeenCalledWith(correctUrl, undefined, undefined); + expect(NetworkManager.invalidateClient).not.toHaveBeenCalled(); + }); + + it('should retry ping once with base_url from thrown 406 error', async () => { + const wrongUrl = 'https://example.com/wrong'; + const correctUrl = 'https://example.com/correct'; + const failingClient = makePingClient({ + ping: jest.fn().mockRejectedValue({ + status_code: 406, + details: {base_url: correctUrl}, + }), + }); + const successClient = makePingClient(); + (NetworkManager.createClient as jest.Mock). + mockResolvedValueOnce(failingClient). + mockResolvedValueOnce(successClient); + + const result = await doPing(wrongUrl, false); + expect(result).toEqual({serverUrl: correctUrl}); + expect(NetworkManager.createClient).toHaveBeenCalledTimes(2); + }); + + it('should not retry ping when 406 response has no base_url', async () => { + const mockClient = makePingClient({ + ping: jest.fn().mockResolvedValue({ok: false, code: 406, data: {}, headers: {}}), + }); + (NetworkManager.createClient as jest.Mock).mockResolvedValue(mockClient); + + const result = await doPing(serverUrl, false); + expect(result).toHaveProperty('error'); + expect(NetworkManager.createClient).toHaveBeenCalledTimes(1); + }); + + it('should not retry ping more than once when base_url retry also fails', async () => { + const wrongUrl = 'https://example.com/wrong'; + const correctUrl = 'https://example.com/correct'; + const failingClient = makePingClient({ + ping: jest.fn().mockResolvedValue({ + ok: false, + code: 406, + data: {base_url: correctUrl}, + headers: {}, + }), + }); + (NetworkManager.createClient as jest.Mock).mockResolvedValue(failingClient); + + const result = await doPing(wrongUrl, false); + expect(result).toHaveProperty('error'); + expect(NetworkManager.createClient).toHaveBeenCalledTimes(2); + }); + it('should not invalidate client when client is provided externally', async () => { const mockClient = makePingClient({ping: jest.fn().mockResolvedValue({ok: false, code: 500, headers: {}})}); diff --git a/app/actions/remote/general.ts b/app/actions/remote/general.ts index 598a45e62d9..64d38682027 100644 --- a/app/actions/remote/general.ts +++ b/app/actions/remote/general.ts @@ -13,6 +13,7 @@ import {getExpandedLinks, getPushVerificationStatus} from '@queries/servers/syst import {getFullErrorMessage} from '@utils/errors'; import {getResponseHeader} from '@utils/headers'; import {logDebug} from '@utils/log'; +import {sanitizeUrl} from '@utils/url'; import {forceLogoutIfNecessary} from './session'; @@ -36,8 +37,19 @@ async function getDeviceIdForPing(serverUrl: string, checkDeviceId: boolean) { return getDeviceToken(); } +function getBaseUrlFromResponseData(data: unknown): string | undefined { + if (data && typeof data === 'object' && 'base_url' in data) { + const baseUrl = data.base_url; + if (typeof baseUrl === 'string' && baseUrl.length > 0) { + return baseUrl; + } + } + + return undefined; +} + // Default timeout interval for ping is 5 seconds -export const doPing = async (serverUrl: string, verifyPushProxy: boolean, timeoutInterval = 5000, preauthSecret?: string, client?: Client) => { +export const doPing = async (serverUrl: string, verifyPushProxy: boolean, timeoutInterval = 5000, preauthSecret?: string, client?: Client, hasRetriedBaseUrl = false): Promise<{error?: unknown; canReceiveNotifications?: string; serverUrl?: string; isPreauthError?: boolean}> => { let pingClient: Client; if (client) { @@ -77,38 +89,56 @@ export const doPing = async (serverUrl: string, verifyPushProxy: boolean, timeou if (!client) { NetworkManager.invalidateClient(serverUrl); } + + if (response.code === 406 && !hasRetriedBaseUrl) { + const baseUrl = getBaseUrlFromResponseData(response.data); + if (baseUrl) { + return doPing(sanitizeUrl(baseUrl, false, true), verifyPushProxy, timeoutInterval, preauthSecret, undefined, true); + } + } + if (response.code === 403 && getResponseHeader(response.headers, ClientConstants.HEADER_X_REJECT_REASON) === 'pre-auth') { return {error: {intl: pingError}, isPreauthError: true}; } return {error: {intl: pingError}}; } } catch (error) { - // Check if this is a 403 with pre-auth header + if (!client) { + NetworkManager.invalidateClient(serverUrl); + } + const errorObj = error as ClientError; + + // Check if this is a 406 with base_url in the response data + if (errorObj.status_code === 406 && !hasRetriedBaseUrl) { + const baseUrl = getBaseUrlFromResponseData(errorObj.details); + if (baseUrl) { + return doPing(sanitizeUrl(baseUrl, false, true), verifyPushProxy, timeoutInterval, preauthSecret, undefined, true); + } + } + + // Check if this is a 403 with pre-auth header if (errorObj.status_code === 403) { if (getResponseHeader(errorObj.headers, ClientConstants.HEADER_X_REJECT_REASON) === 'pre-auth') { return {error: {intl: pingError}, isPreauthError: true}; } } - if (!client) { - NetworkManager.invalidateClient(serverUrl); - } return {error: {intl: pingError}}; } if (verifyPushProxy) { - let canReceiveNotifications = response?.data?.CanReceiveNotifications; + let canReceiveNotifications = response?.data?.CanReceiveNotifications as string | undefined; // Already verified or old server if (deviceId === undefined || canReceiveNotifications === null) { canReceiveNotifications = PUSH_PROXY_RESPONSE_VERIFIED; } - return {canReceiveNotifications}; + return {canReceiveNotifications, serverUrl}; } - return {}; + return {serverUrl}; }; export const getRedirectLocation = async (serverUrl: string, link: string) => { diff --git a/app/client/rest/tracking.test.ts b/app/client/rest/tracking.test.ts index a4e6fa2d114..e46fea30e25 100644 --- a/app/client/rest/tracking.test.ts +++ b/app/client/rest/tracking.test.ts @@ -231,6 +231,22 @@ describe('ClientTracking', () => { expect(result).toEqual({success: true}); }); + it('should reject ok responses with text/html content type', async () => { + apiClientMock.get.mockResolvedValue({ + ok: true, + code: 200, + data: 'Not Found', + headers: {'Content-Type': 'text/html; charset=utf-8'}, + }); + + const options = { + method: 'GET', + groupLabel: 'Cold Start' as RequestGroupLabel, + }; + + await expect(client.doFetchWithTracking('https://example.com/api', options)).rejects.toThrow('Received invalid response from the server.'); + }); + it('should handle fetch errors', async () => { apiClientMock.get.mockRejectedValue(new Error('Request failed')); @@ -267,6 +283,7 @@ describe('ClientTracking', () => { expect((clientError as {message: string}).message).toBe('Custom error message'); expect((clientError as {server_error_id: string}).server_error_id).toBe('error_id_123'); expect((clientError as {status_code: number}).status_code).toBe(400); + expect((clientError as {details: {message: string}}).details).toEqual({message: 'Custom error message', id: 'error_id_123'}); } }); diff --git a/app/client/rest/tracking.ts b/app/client/rest/tracking.ts index 5d76e5169cb..a0011fa6961 100644 --- a/app/client/rest/tracking.ts +++ b/app/client/rest/tracking.ts @@ -12,6 +12,7 @@ import PerformanceMetricsManager from '@managers/performance_metrics_manager'; import {NetworkRequestMetrics} from '@managers/performance_metrics_manager/constant'; import {isErrorWithStatusCode} from '@utils/errors'; import {getFormattedFileSize} from '@utils/file'; +import {getResponseHeader} from '@utils/headers'; import {logDebug, logInfo} from '@utils/log'; import {semverFromServerVersion} from '@utils/server'; @@ -432,6 +433,20 @@ export default class ClientTracking { } if (response.ok) { + const contentType = getResponseHeader(headers, 'Content-Type'); + if (contentType?.toLowerCase().includes('text/html')) { + throw new ClientError(this.apiClient.baseUrl, { + message: 'Received invalid response from the server.', + intl: defineMessage({ + id: 'mobile.request.invalid_response', + defaultMessage: 'Received invalid response from the server.', + }), + url, + status_code: response.code, + headers, + }); + } + return returnDataOnly ? (response.data || {}) : response; } @@ -441,6 +456,7 @@ export default class ClientTracking { status_code: response.code, url, headers, + details: response.data, }); }; } diff --git a/app/screens/server/index.tsx b/app/screens/server/index.tsx index 48955210582..1d5b2c8915e 100644 --- a/app/screens/server/index.tsx +++ b/app/screens/server/index.tsx @@ -328,6 +328,11 @@ const Server = ({ return; } + const serverUrlToUse = result.serverUrl || headRequest.url; + if (result.serverUrl && result.serverUrl !== headRequest.url) { + setUrl(result.serverUrl); + } + if (result.error) { if (result.isPreauthError) { setPreauthSecretError(intl.formatMessage({ @@ -354,7 +359,7 @@ const Server = ({ // has already completed. const pushProxyVerification = result.canReceiveNotifications as string; - const data = await fetchConfigAndLicense(headRequest.url, true); + const data = await fetchConfigAndLicense(serverUrlToUse, true); if (data.error) { setButtonDisabled(true); setUrlError(getErrorMessage(data.error, intl)); @@ -372,7 +377,7 @@ const Server = ({ } if (data.config.MobileJailbreakProtection === 'true') { - const isJailbroken = await SecurityManager.isDeviceJailbroken(headRequest.url, data.config.SiteName); + const isJailbroken = await SecurityManager.isDeviceJailbroken(serverUrlToUse, data.config.SiteName); if (isJailbroken) { setConnecting(false); return; @@ -380,7 +385,7 @@ const Server = ({ } if (data.config.MobileEnableBiometrics === 'true') { - const biometricsResult = await SecurityManager.authenticateWithBiometrics(headRequest.url, data.config.SiteName); + const biometricsResult = await SecurityManager.authenticateWithBiometrics(serverUrlToUse, data.config.SiteName); if (!biometricsResult) { setConnecting(false); return; @@ -388,7 +393,7 @@ const Server = ({ } const server = await getServerByIdentifier(data.config.DiagnosticId); - const credentials = await getServerCredentials(headRequest.url); + const credentials = await getServerCredentials(serverUrlToUse); setConnecting(false); if (server && server.lastActiveAt > 0 && credentials?.token) { @@ -400,7 +405,7 @@ const Server = ({ return; } - displayLogin(headRequest.url, data.config!, data.license!); + displayLogin(serverUrlToUse, data.config!, data.license!); // Fire the push-proxy verification alert AFTER the RNN transition to // LoginScreen has FULLY settled. We use setTimeout (not @@ -417,7 +422,7 @@ const Server = ({ // unaffected — the alert still targets the same serverUrl, it just // renders on the login screen instead of the server screen. setTimeout(() => { - canReceiveNotifications(headRequest.url, pushProxyVerification, intl); + canReceiveNotifications(serverUrlToUse, pushProxyVerification, intl); }, 1000); }; diff --git a/app/utils/url/index.ts b/app/utils/url/index.ts index 0cf9c415204..34060b7688f 100644 --- a/app/utils/url/index.ts +++ b/app/utils/url/index.ts @@ -45,17 +45,17 @@ export function isParsableUrl(url: string): boolean { } } -export function sanitizeUrl(url: string, useHttp = false) { +export function sanitizeUrl(url: string, useHttp = false, keepProtocol = false) { let preUrl = urlParse(url, true); - let protocol = useHttp ? 'http:' : preUrl.protocol; if (!preUrl.host || preUrl.protocol === 'file:') { preUrl = urlParse('https://' + stripTrailingSlashes(url), true); } - if (preUrl.protocol === 'http:' && !useHttp) { - protocol = 'https:'; - } else if (!protocol) { + let protocol; + if (keepProtocol) { + protocol = preUrl.protocol; + } else { protocol = useHttp ? 'http:' : 'https:'; } diff --git a/detox/e2e/support/ui/screen/server.ts b/detox/e2e/support/ui/screen/server.ts index 2b24b23cff1..e618feb6c0e 100644 --- a/detox/e2e/support/ui/screen/server.ts +++ b/detox/e2e/support/ui/screen/server.ts @@ -214,6 +214,15 @@ class ServerScreen { await wait(timeouts.ONE_SEC); }; + waitForServerUrl = async (expectedUrl: string, timeout = timeouts.TEN_SEC) => { + if (isAndroid()) { + await waitFor(this.serverUrlInput).toHaveValue(expectedUrl).withTimeout(timeout); + return; + } + + await waitFor(this.serverUrlInput).toHaveText(expectedUrl).withTimeout(timeout); + }; + toggleAdvancedOptions = async () => { await this.advancedOptionsToggle.tap(); await wait(timeouts.ONE_SEC); diff --git a/detox/e2e/test/products/channels/server_login/base_url_redirect.e2e.ts b/detox/e2e/test/products/channels/server_login/base_url_redirect.e2e.ts new file mode 100644 index 00000000000..796910f7c98 --- /dev/null +++ b/detox/e2e/test/products/channels/server_login/base_url_redirect.e2e.ts @@ -0,0 +1,61 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +// ******************************************************************* +// - [#] indicates a test step (e.g. # Go to a screen) +// - [*] indicates an assertion (e.g. * Check the title) +// - Use element testID when selecting an element. Create one if none. +// ******************************************************************* + +import {Setup} from '@support/server_api'; +import {serverOneUrl, siteOneUrl} from '@support/test_config'; +import {Alert} from '@support/ui/component'; +import { + LoginScreen, + ServerScreen, +} from '@support/ui/screen'; +import {isIos, timeouts} from '@support/utils'; +import {waitFor} from 'detox'; + +describe('Server Login - Base URL Redirect', () => { + const serverDisplayName = 'Server 1'; + let channelPageUrl: string; + + beforeAll(async () => { + const {channel, team} = await Setup.apiInit(siteOneUrl); + channelPageUrl = `${serverOneUrl}/${team.name}/channels/${channel.name}`; + }); + + beforeEach(async () => { + await ServerScreen.toBeVisible(); + await ServerScreen.serverUrlInput.clearText(); + await ServerScreen.serverDisplayNameInput.clearText(); + }); + + // Skipping until the server is updated to return the base URL in the 406 response + it.skip('MM-67557_1 - should rewrite server URL from 406 base_url response and connect', async () => { + // # Connect using a channel page URL that returns HTML instead of the API + await ServerScreen.serverUrlInput.replaceText(channelPageUrl); + await ServerScreen.serverDisplayNameInput.replaceText(serverDisplayName); + await ServerScreen.tapConnectButton(); + + if (isIos() && !process.env.CI) { + try { + await waitFor(Alert.okayButton).toExist().withTimeout(timeouts.TEN_SEC); + await Alert.okayButton.tap(); + } catch { + // Push notification alert may not appear in local environments. + } + } + + // * Verify connection succeeds and login screen is shown + await LoginScreen.toBeVisible(); + + // # Go back to the server screen + await LoginScreen.back(); + await ServerScreen.toBeVisible(); + + // * Verify the server URL field shows the corrected base URL + await ServerScreen.waitForServerUrl(serverOneUrl, timeouts.TEN_SEC); + }); +});