diff --git a/ts/challenge.ts b/ts/challenge.ts index 34449d9701..8322328fa0 100644 --- a/ts/challenge.ts +++ b/ts/challenge.ts @@ -14,15 +14,15 @@ import { assertDev } from './util/assert'; import { isOlderThan } from './util/timestamp'; -import { parseRetryAfterWithDefault } from './util/parseRetryAfter'; import { clearTimeoutIfNecessary } from './util/clearTimeoutIfNecessary'; import { missingCaseError } from './util/missingCaseError'; import type { StorageInterface } from './types/Storage.d'; import * as Errors from './types/errors'; -import { HTTPError } from './textsecure/Errors'; -import type { SendMessageChallengeData } from './textsecure/Errors'; +import { HTTPError, type SendMessageChallengeData } from './textsecure/Errors'; import * as log from './logging/log'; import { drop } from './util/drop'; +import { findRetryAfterTimeFromError } from './jobs/helpers/findRetryAfterTimeFromError'; +import { MINUTE } from './util/durations'; export type ChallengeResponse = Readonly<{ captcha: string; @@ -425,59 +425,53 @@ export class ChallengeHandler { log.info(`challenge(${reason}): sending challenge to server`); try { - await this.sendChallengeResponse({ + await this.options.sendChallengeResponse({ type: 'captcha', token: lastToken, captcha, }); } catch (error) { + // If we get an error back from server after solving a captcha, it could be that we + // are rate-limited (413, 429), that we need to solve another captcha (428), or any + // other possible 4xx, 5xx error. + + // In general, unless we're being rate-limited, we don't want to wait to show + // another captcha: this may be a time-critical situation (e.g. user is in a call), + // and if the server 500s, for instance, we want to allow the user to immediately + // try again. + let defaultRetryAfter = 0; + + if (error instanceof HTTPError) { + if ([413, 429].includes(error.code)) { + // These rate-limit codes should have a retry-after in the response, but just in + // case, let's wait a minute + defaultRetryAfter = MINUTE; + } + } + + const retryAfter = findRetryAfterTimeFromError(error, defaultRetryAfter); + log.error( - `challenge(${reason}): challenge failure, error:`, + `challenge(${reason}): challenge solve failure; will retry after ${retryAfter}ms; error:`, Errors.toLogFormat(error) ); - if (error.code === 413 || error.code === 429) { - this.options.setChallengeStatus('idle'); - } else { - this.options.setChallengeStatus('required'); - } - this.solving -= 1; + + const retryAt = retryAfter + Date.now(); + + // Remove the challenge dialog, and trigger the conversationJobQueue to retry the + // sends, which will likely trigger another captcha + this.options.setChallengeStatus('idle'); + this.options.onChallengeFailed(retryAfter); + this.forceWaitOnAll(retryAt); return; + } finally { + this.solving -= 1; } log.info(`challenge(${reason}): challenge success. force sending`); this.options.setChallengeStatus('idle'); - - this.startAllQueues({ force: true }); - this.solving -= 1; - } - - private async sendChallengeResponse(data: ChallengeData): Promise { - try { - await this.options.sendChallengeResponse(data); - } catch (error) { - if ( - !(error instanceof HTTPError) || - !(error.code === 413 || error.code === 429) || - !error.responseHeaders - ) { - this.options.onChallengeFailed(); - throw error; - } - - const retryAfter = parseRetryAfterWithDefault( - error.responseHeaders['retry-after'] - ); - - log.info(`challenge: retry after ${retryAfter}ms`); - const retryAt = retryAfter + Date.now(); - this.forceWaitOnAll(retryAt); - - this.options.onChallengeFailed(retryAfter); - - throw error; - } - this.options.onChallengeSolved(); + this.startAllQueues({ force: true }); } } diff --git a/ts/jobs/helpers/findRetryAfterTimeFromError.ts b/ts/jobs/helpers/findRetryAfterTimeFromError.ts index 8714537047..6f82666ac8 100644 --- a/ts/jobs/helpers/findRetryAfterTimeFromError.ts +++ b/ts/jobs/helpers/findRetryAfterTimeFromError.ts @@ -5,7 +5,10 @@ import { isRecord } from '../../util/isRecord'; import { HTTPError } from '../../textsecure/Errors'; import { parseRetryAfterWithDefault } from '../../util/parseRetryAfter'; -export function findRetryAfterTimeFromError(err: unknown): number { +export function findRetryAfterTimeFromError( + err: unknown, + defaultValue?: number +): number { let rawValue: unknown; if (isRecord(err)) { @@ -17,8 +20,8 @@ export function findRetryAfterTimeFromError(err: unknown): number { } if (Array.isArray(rawValue)) { - return parseRetryAfterWithDefault(rawValue[0]); + return parseRetryAfterWithDefault(rawValue[0], defaultValue); } - return parseRetryAfterWithDefault(rawValue); + return parseRetryAfterWithDefault(rawValue, defaultValue); } diff --git a/ts/test-mock/rate-limit/viewed_test.ts b/ts/test-mock/rate-limit/viewed_test.ts index 6644f9198b..4ad4de6267 100644 --- a/ts/test-mock/rate-limit/viewed_test.ts +++ b/ts/test-mock/rate-limit/viewed_test.ts @@ -259,7 +259,7 @@ describe('challenge/receipts', function (this: Mocha.Suite) { } }); - it('should show a toast and not another challenge if completion results in 413', async () => { + it('if server rejects our captcha, should show a toast and defer challenge based on error code', async () => { const { server, desktop } = bootstrap; debug( @@ -296,21 +296,47 @@ describe('challenge/receipts', function (this: Mocha.Suite) { await input.press('Enter'); } + /** First, challenge returns 428 (try again) */ debug('Waiting for challenge'); - const request = await app.waitForChallenge(); + const firstChallengeRequest = await app.waitForChallenge(); + const challengeDialog = await window + .getByTestId('CaptchaDialog.pending') + .elementHandle(); + + assert.exists(challengeDialog); + server.respondToChallengesWith(428); + + debug('Solving challenge'); + await app.solveChallenge({ + seq: firstChallengeRequest.seq, + data: { captcha: 'anything' }, + }); + + debug('Waiting for verification failure toast'); + const failedChallengeToastLocator = window.locator( + '.Toast__content >> "Verification failed. Please retry later."' + ); + await failedChallengeToastLocator.isVisible(); + // The existing dialog is removed, but then the conversations will retry their sends, + // which will result in another one + await challengeDialog.isHidden(); + + /** Second, challenge returns 413 (rate limit) */ + debug( + 'Waiting for second challenge, should be triggered quickly with the sends being retried' + ); + const secondChallengeRequest = await app.waitForChallenge(); server.respondToChallengesWith(413); debug('Solving challenge'); await app.solveChallenge({ - seq: request.seq, + seq: secondChallengeRequest.seq, data: { captcha: 'anything' }, }); debug('Waiting for verification failure toast'); - await window - .locator('.Toast__content >> "Verification failed. Please retry later."') - .isVisible(); + await failedChallengeToastLocator.isVisible(); debug('Sending another message - this time it should not trigger captcha!'); { @@ -346,8 +372,8 @@ describe('challenge/receipts', function (this: Mocha.Suite) { debug('Checking for no other captcha dialogs'); assert.equal( await app.getPendingEventCount('captchaDialog'), - 1, - 'Just one captcha dialog, the first one' + 2, + 'Just two captcha dialogs, the first one, and the one after the 428' ); const requests = server.stopRateLimiting({ @@ -356,7 +382,7 @@ describe('challenge/receipts', function (this: Mocha.Suite) { }); debug(`Rate-limited requests: ${requests}`); - assert.strictEqual(requests, 1, 'rate limit requests'); + assert.strictEqual(requests, 2, 'rate limit requests'); const requestsContactB = server.stopRateLimiting({ source: desktop.aci, diff --git a/ts/test-node/jobs/helpers/findRetryAfterTimeFromError_test.ts b/ts/test-node/jobs/helpers/findRetryAfterTimeFromError_test.ts index 00ad942797..1a59546c4e 100644 --- a/ts/test-node/jobs/helpers/findRetryAfterTimeFromError_test.ts +++ b/ts/test-node/jobs/helpers/findRetryAfterTimeFromError_test.ts @@ -8,7 +8,7 @@ import { HTTPError } from '../../../textsecure/Errors'; import { MINUTE } from '../../../util/durations'; describe('findRetryAfterTimeFromError', () => { - it('returns 1 minute if no Retry-After time is found', () => { + it('returns 1 minute or provided default if no Retry-After time is found', () => { [ undefined, null, @@ -47,6 +47,7 @@ describe('findRetryAfterTimeFromError', () => { }, ].forEach(input => { assert.strictEqual(findRetryAfterTimeFromError(input), MINUTE); + assert.strictEqual(findRetryAfterTimeFromError(input, 42), 42); }); }); diff --git a/ts/util/parseRetryAfter.ts b/ts/util/parseRetryAfter.ts index 6cd2042200..19aa9319b8 100644 --- a/ts/util/parseRetryAfter.ts +++ b/ts/util/parseRetryAfter.ts @@ -7,10 +7,13 @@ import { isNormalNumber } from './isNormalNumber'; const DEFAULT_RETRY_AFTER = MINUTE; const MINIMAL_RETRY_AFTER = SECOND; -export function parseRetryAfterWithDefault(value: unknown): number { +export function parseRetryAfterWithDefault( + value: unknown, + defaultValue: number = DEFAULT_RETRY_AFTER +): number { const retryAfter = parseRetryAfter(value); if (retryAfter === undefined) { - return DEFAULT_RETRY_AFTER; + return defaultValue; } return Math.max(retryAfter, MINIMAL_RETRY_AFTER);