-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[MM-67557] Handle HTML responses on API calls #9922
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there any chance that this throws an error that's caught by the outer try/catch? If it can and
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In reality, the normal path is going to be the try catch. We have this as defensive coding. Invalidating the same client twice is not an issue. |
||
| } | ||
| } | ||
|
|
||
| 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) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are we handling this in both the try and catch blocks? Should we perhaps take these out of the try/catch?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is the current pattern. Not great, but I didn't want to deviate from it. |
||
| 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}; | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| }; | ||
|
|
||
| export const getRedirectLocation = async (serverUrl: string, link: string) => { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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')) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why test for
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just in case in any API we are sending a empty content type, or anything similar. But you are probably right, and we should go with the |
||
| 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, | ||
| }); | ||
| }; | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure about adding this
base_urlto the server response? I wouldn't expect the server to return anything like that for a 406 error, so I'd expect something like we trim the path fromserverUrl. If we want to fully support subpaths with that, we could even retry multiple times, each time with one more path segment removedThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See https://hub.mattermost.com/private-core/pl/z4rgearu6ignuqrr1szpaxqf6e for more context on this solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the context. I'm still not sure if I'd expect that since there's other reasons someone might accidentally request JSON from another endpoint, but I guess that makes sense when you compare to how we return the web app for any non-API URL