From ae3a5c5f5e3708a06b88556574a9b4e4c5093e32 Mon Sep 17 00:00:00 2001 From: Ravi Khadiwala Date: Wed, 16 Feb 2022 14:34:25 -0600 Subject: [PATCH] Return a Retry-After on rate-limited responses Previously, only endpoints throwing a RetryLaterException would include a Retry-After header in the 413 response. Now, by default, all RateLimitExceededExceptions will be marshalled into a 413 with a Retry-After included if possible. --- .../textsecuregcm/WhisperServerService.java | 2 -- .../controllers/AccountController.java | 34 ++++++++----------- .../controllers/ChallengeController.java | 4 +-- .../RateLimitExceededException.java | 8 +++-- .../controllers/RetryLaterException.java | 19 ----------- .../RateLimitExceededExceptionMapper.java | 12 ++++++- .../mappers/RetryLaterExceptionMapper.java | 23 ------------- .../controllers/ChallengeControllerTest.java | 4 +-- .../controllers/AccountControllerTest.java | 32 +++++++++++++++++ .../tests/controllers/KeysControllerTest.java | 23 ++++++++++--- .../controllers/ProfileControllerTest.java | 19 +++++++++++ 11 files changed, 103 insertions(+), 77 deletions(-) delete mode 100644 service/src/main/java/org/whispersystems/textsecuregcm/controllers/RetryLaterException.java delete mode 100644 service/src/main/java/org/whispersystems/textsecuregcm/mappers/RetryLaterExceptionMapper.java diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java b/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java index b7e06c777..2cca1bec1 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java @@ -123,7 +123,6 @@ import org.whispersystems.textsecuregcm.mappers.InvalidWebsocketAddressException import org.whispersystems.textsecuregcm.mappers.NonNormalizedPhoneNumberExceptionMapper; import org.whispersystems.textsecuregcm.mappers.RateLimitChallengeExceptionMapper; import org.whispersystems.textsecuregcm.mappers.RateLimitExceededExceptionMapper; -import org.whispersystems.textsecuregcm.mappers.RetryLaterExceptionMapper; import org.whispersystems.textsecuregcm.mappers.ServerRejectedExceptionMapper; import org.whispersystems.textsecuregcm.metrics.ApplicationShutdownMonitor; import org.whispersystems.textsecuregcm.metrics.BufferPoolGauges; @@ -758,7 +757,6 @@ public class WhisperServerService extends Application client, @QueryParam("captcha") Optional captcha, @QueryParam("challenge") Optional pushChallenge) - throws RateLimitExceededException, RetryLaterException, ImpossiblePhoneNumberException, NonNormalizedPhoneNumberException { + throws RateLimitExceededException, ImpossiblePhoneNumberException, NonNormalizedPhoneNumberException { Util.requireNormalizedNumber(number); @@ -234,24 +234,16 @@ public class AccountController { return Response.status(402).build(); } - try { - switch (transport) { - case "sms": - rateLimiters.getSmsDestinationLimiter().validate(number); - break; - case "voice": - rateLimiters.getVoiceDestinationLimiter().validate(number); - rateLimiters.getVoiceDestinationDailyLimiter().validate(number); - break; - default: - throw new WebApplicationException(Response.status(422).build()); - } - } catch (RateLimitExceededException e) { - if (!e.getRetryDuration().isNegative()) { - throw new RetryLaterException(e); - } else { - throw e; - } + switch (transport) { + case "sms": + rateLimiters.getSmsDestinationLimiter().validate(number); + break; + case "voice": + rateLimiters.getVoiceDestinationLimiter().validate(number); + rateLimiters.getVoiceDestinationDailyLimiter().validate(number); + break; + default: + throw new WebApplicationException(Response.status(422).build()); } VerificationCode verificationCode = generateVerificationCode(number); @@ -643,7 +635,9 @@ public class AccountController { } final String mostRecentProxy = ForwardedIpUtil.getMostRecentProxy(forwardedFor) - .orElseThrow(() -> new RateLimitExceededException(Duration.ofHours(1))); + // Missing/malformed Forwarded-For, cannot calculate a reasonable backoff + // duration + .orElseThrow(() -> new RateLimitExceededException(Duration.ofHours(-1))); rateLimiters.getCheckAccountExistenceLimiter().validate(mostRecentProxy); diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/ChallengeController.java b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/ChallengeController.java index 7a7907ce0..861a31be7 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/ChallengeController.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/ChallengeController.java @@ -51,7 +51,7 @@ public class ChallengeController { public Response handleChallengeResponse(@Auth final AuthenticatedAccount auth, @Valid final AnswerChallengeRequest answerRequest, @HeaderParam("X-Forwarded-For") final String forwardedFor, - @HeaderParam(HttpHeaders.USER_AGENT) final String userAgent) throws RetryLaterException { + @HeaderParam(HttpHeaders.USER_AGENT) final String userAgent) throws RateLimitExceededException { Tags tags = Tags.of(UserAgentTagUtil.getPlatformTag(userAgent)); @@ -76,8 +76,6 @@ public class ChallengeController { } else { tags = tags.and(CHALLENGE_TYPE_TAG, "unrecognized"); } - } catch (final RateLimitExceededException e) { - throw new RetryLaterException(e); } finally { Metrics.counter(CHALLENGE_RESPONSE_COUNTER_NAME, tags).increment(); } diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/RateLimitExceededException.java b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/RateLimitExceededException.java index 776acd6c8..98f1445c8 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/RateLimitExceededException.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/RateLimitExceededException.java @@ -5,10 +5,11 @@ package org.whispersystems.textsecuregcm.controllers; import java.time.Duration; +import java.util.Optional; public class RateLimitExceededException extends Exception { - private final Duration retryDuration; + private final Optional retryDuration; public RateLimitExceededException(final Duration retryDuration) { this(null, retryDuration); @@ -16,8 +17,9 @@ public class RateLimitExceededException extends Exception { public RateLimitExceededException(final String message, final Duration retryDuration) { super(message, null, true, false); - this.retryDuration = retryDuration; + // we won't provide a backoff in the case the duration is negative + this.retryDuration = retryDuration.isNegative() ? Optional.empty() : Optional.of(retryDuration); } - public Duration getRetryDuration() { return retryDuration; } + public Optional getRetryDuration() { return retryDuration; } } diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/RetryLaterException.java b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/RetryLaterException.java deleted file mode 100644 index ff19efc11..000000000 --- a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/RetryLaterException.java +++ /dev/null @@ -1,19 +0,0 @@ -/* - * Copyright 2013-2021 Signal Messenger, LLC - * SPDX-License-Identifier: AGPL-3.0-only - */ - -package org.whispersystems.textsecuregcm.controllers; - -import java.time.Duration; - -public class RetryLaterException extends Exception { - private final Duration backoffDuration; - - public RetryLaterException(RateLimitExceededException e) { - super(null, e, true, false); - this.backoffDuration = e.getRetryDuration(); - } - - public Duration getBackoffDuration() { return backoffDuration; } -} diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/mappers/RateLimitExceededExceptionMapper.java b/service/src/main/java/org/whispersystems/textsecuregcm/mappers/RateLimitExceededExceptionMapper.java index 77e9c11e1..846397416 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/mappers/RateLimitExceededExceptionMapper.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/mappers/RateLimitExceededExceptionMapper.java @@ -12,8 +12,18 @@ import javax.ws.rs.ext.Provider; @Provider public class RateLimitExceededExceptionMapper implements ExceptionMapper { + + /** + * Convert a RateLimitExceededException to a 413 response with a + * Retry-After header. + * + * @param e A RateLimitExceededException potentially containing a reccomended retry duration + * @return the response + */ @Override public Response toResponse(RateLimitExceededException e) { - return Response.status(413).build(); + return e.getRetryDuration() + .map(d -> Response.status(413).header("Retry-After", d.toSeconds())) + .orElseGet(() -> Response.status(413)).build(); } } diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/mappers/RetryLaterExceptionMapper.java b/service/src/main/java/org/whispersystems/textsecuregcm/mappers/RetryLaterExceptionMapper.java deleted file mode 100644 index f3c29b4be..000000000 --- a/service/src/main/java/org/whispersystems/textsecuregcm/mappers/RetryLaterExceptionMapper.java +++ /dev/null @@ -1,23 +0,0 @@ -/* - * Copyright 2013-2020 Signal Messenger, LLC - * SPDX-License-Identifier: AGPL-3.0-only - */ - -package org.whispersystems.textsecuregcm.mappers; - -import org.whispersystems.textsecuregcm.controllers.RetryLaterException; - -import javax.ws.rs.core.Response; -import javax.ws.rs.ext.ExceptionMapper; -import javax.ws.rs.ext.Provider; - -@Provider -public class RetryLaterExceptionMapper implements ExceptionMapper { - @Override - public Response toResponse(RetryLaterException e) { - return Response.status(413) - .header("Retry-After", e.getBackoffDuration().toSeconds()) - .build(); - } -} - diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/controllers/ChallengeControllerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/controllers/ChallengeControllerTest.java index 279967924..f28c9d553 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/controllers/ChallengeControllerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/controllers/ChallengeControllerTest.java @@ -27,7 +27,7 @@ import org.junit.jupiter.api.extension.ExtendWith; import org.whispersystems.textsecuregcm.auth.AuthenticatedAccount; import org.whispersystems.textsecuregcm.auth.DisabledPermittedAuthenticatedAccount; import org.whispersystems.textsecuregcm.limits.RateLimitChallengeManager; -import org.whispersystems.textsecuregcm.mappers.RetryLaterExceptionMapper; +import org.whispersystems.textsecuregcm.mappers.RateLimitExceededExceptionMapper; import org.whispersystems.textsecuregcm.push.NotPushRegisteredException; import org.whispersystems.textsecuregcm.tests.util.AuthHelper; import org.whispersystems.textsecuregcm.util.SystemMapper; @@ -45,7 +45,7 @@ class ChallengeControllerTest { Set.of(AuthenticatedAccount.class, DisabledPermittedAuthenticatedAccount.class))) .setMapper(SystemMapper.getMapper()) .setTestContainerFactory(new GrizzlyWebTestContainerFactory()) - .addResource(new RetryLaterExceptionMapper()) + .addResource(new RateLimitExceededExceptionMapper()) .addResource(challengeController) .build(); diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/AccountControllerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/AccountControllerTest.java index a967a1ea9..392ae47e6 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/AccountControllerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/AccountControllerTest.java @@ -1804,6 +1804,38 @@ class AccountControllerTest { .getStatus()).isEqualTo(404); } + @Test + void testAccountExistsRateLimited() throws RateLimitExceededException { + final Account account = mock(Account.class); + final UUID accountIdentifier = UUID.randomUUID(); + when(accountsManager.getByAccountIdentifier(accountIdentifier)).thenReturn(Optional.of(account)); + + final RateLimiter checkAccountLimiter = mock(RateLimiter.class); + when(rateLimiters.getCheckAccountExistenceLimiter()).thenReturn(checkAccountLimiter); + doThrow(new RateLimitExceededException(Duration.ofSeconds(13))).when(checkAccountLimiter).validate("127.0.0.1"); + + final Response response = resources.getJerseyTest() + .target(String.format("/v1/accounts/account/%s", accountIdentifier)) + .request() + .header("X-Forwarded-For", "127.0.0.1") + .head(); + + assertThat(response.getStatus()).isEqualTo(413); + assertThat(response.getHeaderString("Retry-After")).isEqualTo(String.valueOf(Duration.ofSeconds(13).toSeconds())); + } + + @Test + void testAccountExistsNoForwardedFor() throws RateLimitExceededException { + final Response response = resources.getJerseyTest() + .target(String.format("/v1/accounts/account/%s", UUID.randomUUID())) + .request() + .header("X-Forwarded-For", "") + .head(); + + assertThat(response.getStatus()).isEqualTo(413); + assertThat(response.getHeaderString("Retry-After")).isNull(); + } + @Test void testAccountExistsAuthenticated() { assertThat(resources.getJerseyTest() diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/KeysControllerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/KeysControllerTest.java index ad287b7c8..bf305a2b6 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/KeysControllerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/KeysControllerTest.java @@ -8,6 +8,7 @@ package org.whispersystems.textsecuregcm.tests.controllers; import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyLong; +import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.clearInvocations; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.eq; @@ -38,8 +39,6 @@ import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; -import org.junit.jupiter.params.ParameterizedTest; -import org.junit.jupiter.params.provider.ValueSource; import org.mockito.ArgumentCaptor; import org.whispersystems.textsecuregcm.auth.AuthenticatedAccount; import org.whispersystems.textsecuregcm.auth.DisabledPermittedAuthenticatedAccount; @@ -50,11 +49,11 @@ import org.whispersystems.textsecuregcm.entities.PreKey; import org.whispersystems.textsecuregcm.entities.PreKeyCount; import org.whispersystems.textsecuregcm.entities.PreKeyResponse; import org.whispersystems.textsecuregcm.entities.PreKeyState; -import org.whispersystems.textsecuregcm.entities.RateLimitChallenge; import org.whispersystems.textsecuregcm.entities.SignedPreKey; -import org.whispersystems.textsecuregcm.limits.RateLimitChallengeManager; import org.whispersystems.textsecuregcm.limits.RateLimiter; import org.whispersystems.textsecuregcm.limits.RateLimiters; +import org.whispersystems.textsecuregcm.mappers.RateLimitChallengeExceptionMapper; +import org.whispersystems.textsecuregcm.mappers.RateLimitExceededExceptionMapper; import org.whispersystems.textsecuregcm.mappers.ServerRejectedExceptionMapper; import org.whispersystems.textsecuregcm.storage.Account; import org.whispersystems.textsecuregcm.storage.AccountsManager; @@ -107,6 +106,7 @@ class KeysControllerTest { .setTestContainerFactory(new GrizzlyWebTestContainerFactory()) .addResource(new ServerRejectedExceptionMapper()) .addResource(new KeysController(rateLimiters, KEYS, accounts)) + .addResource(new RateLimitExceededExceptionMapper()) .build(); @BeforeEach @@ -316,6 +316,21 @@ class KeysControllerTest { verifyNoMoreInteractions(KEYS); } + @Test + void testGetKeysRateLimited() throws RateLimitExceededException { + Duration retryAfter = Duration.ofSeconds(31); + doThrow(new RateLimitExceededException(retryAfter)).when(rateLimiter).validate(anyString()); + + Response result = resources.getJerseyTest() + .target(String.format("/v2/keys/%s/*", EXISTS_PNI)) + .request() + .header("Authorization", AuthHelper.getAuthHeader(AuthHelper.VALID_UUID, AuthHelper.VALID_PASSWORD)) + .get(); + + assertThat(result.getStatus()).isEqualTo(413); + assertThat(result.getHeaderString("Retry-After")).isEqualTo(String.valueOf(retryAfter.toSeconds())); + } + @Test void testUnidentifiedRequest() { PreKeyResponse result = resources.getJerseyTest() diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/ProfileControllerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/ProfileControllerTest.java index 773b73010..6036e9d8e 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/ProfileControllerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/ProfileControllerTest.java @@ -10,6 +10,7 @@ import static org.assertj.core.api.Assertions.assertThatNoException; import static org.mockito.ArgumentMatchers.refEq; import static org.mockito.Mockito.any; import static org.mockito.Mockito.clearInvocations; +import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; @@ -26,6 +27,7 @@ import io.dropwizard.testing.junit5.ResourceExtension; import java.nio.charset.StandardCharsets; import java.security.SecureRandom; import java.time.Clock; +import java.time.Duration; import java.time.Instant; import java.util.Base64; import java.util.Collections; @@ -82,6 +84,7 @@ import org.whispersystems.textsecuregcm.entities.ProfileKeyCredentialProfileResp import org.whispersystems.textsecuregcm.entities.VersionedProfileResponse; import org.whispersystems.textsecuregcm.limits.RateLimiter; import org.whispersystems.textsecuregcm.limits.RateLimiters; +import org.whispersystems.textsecuregcm.mappers.RateLimitExceededExceptionMapper; import org.whispersystems.textsecuregcm.s3.PolicySigner; import org.whispersystems.textsecuregcm.s3.PostPolicyGenerator; import org.whispersystems.textsecuregcm.storage.Account; @@ -123,6 +126,7 @@ class ProfileControllerTest { .addProvider(AuthHelper.getAuthFilter()) .addProvider(new PolymorphicAuthValueFactoryProvider.Binder<>( ImmutableSet.of(AuthenticatedAccount.class, DisabledPermittedAuthenticatedAccount.class))) + .addProvider(new RateLimitExceededExceptionMapper()) .setMapper(SystemMapper.getMapper()) .setTestContainerFactory(new GrizzlyWebTestContainerFactory()) .addResource(new ProfileController( @@ -210,6 +214,7 @@ class ProfileControllerTest { @AfterEach void teardown() { reset(accountsManager); + reset(rateLimiter); } @Test @@ -228,6 +233,20 @@ class ProfileControllerTest { verify(rateLimiter, times(1)).validate(AuthHelper.VALID_UUID); } + @Test + void testProfileGetByUuidRateLimited() throws RateLimitExceededException { + doThrow(new RateLimitExceededException(Duration.ofSeconds(13))).when(rateLimiter).validate(AuthHelper.VALID_UUID); + + Response response= resources.getJerseyTest() + .target("/v1/profile/" + AuthHelper.VALID_UUID_TWO) + .request() + .header("Authorization", AuthHelper.getAuthHeader(AuthHelper.VALID_UUID, AuthHelper.VALID_PASSWORD)) + .get(); + + assertThat(response.getStatus()).isEqualTo(413); + assertThat(response.getHeaderString("Retry-After")).isEqualTo(String.valueOf(Duration.ofSeconds(13).toSeconds())); + } + @Test void testProfileGetByUuidUnidentified() throws RateLimitExceededException { BaseProfileResponse profile = resources.getJerseyTest()