Update captcha response error handling

Co-authored-by: trevor-signal <131492920+trevor-signal@users.noreply.github.com>

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<void> {
-    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 });
   }
 }

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);
 }

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,

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);
     });
   });
 

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);