From f5a75c631964de12ce254c2be2d954a8bd84a0c1 Mon Sep 17 00:00:00 2001 From: Ravi Khadiwala Date: Tue, 22 Feb 2022 11:09:39 -0600 Subject: [PATCH] Simplify RateLimitExceeded with no retry-duration - Avoid passing negative durations in error cases - Drop unused message - Return a duration for a bad forwarded-for --- .../controllers/AccountController.java | 10 ++++++--- .../RateLimitExceededException.java | 22 +++++++++++-------- .../limits/CardinalityRateLimiter.java | 10 ++++++++- .../limits/LockingRateLimiter.java | 2 +- .../textsecuregcm/limits/RateLimiter.java | 2 +- .../RateLimitExceededExceptionMapper.java | 12 +++++++++- .../controllers/AccountControllerTest.java | 14 ++++++------ 7 files changed, 49 insertions(+), 23 deletions(-) diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountController.java b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountController.java index e8d9dea4d..aa31e7e16 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountController.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountController.java @@ -635,9 +635,13 @@ public class AccountController { } final String mostRecentProxy = ForwardedIpUtil.getMostRecentProxy(forwardedFor) - // Missing/malformed Forwarded-For, cannot calculate a reasonable backoff - // duration - .orElseThrow(() -> new RateLimitExceededException(Duration.ofHours(-1))); + .orElseThrow(() -> { + // Missing/malformed Forwarded-For, so we cannot check for a rate-limit. + // This shouldn't happen, so conservatively assume we're over the rate-limit + // and indicate that the client should retry + logger.error("Missing/bad Forwarded-For, cannot check account {}", uuid.toString()); + return new RateLimitExceededException(Duration.ofHours(1)); + }); rateLimiters.getCheckAccountExistenceLimiter().validate(mostRecentProxy); 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 98f1445c8..bd2ca24a5 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/RateLimitExceededException.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/RateLimitExceededException.java @@ -4,22 +4,26 @@ */ package org.whispersystems.textsecuregcm.controllers; +import javax.annotation.Nullable; import java.time.Duration; import java.util.Optional; public class RateLimitExceededException extends Exception { - private final Optional retryDuration; + private final @Nullable + Duration retryDuration; - public RateLimitExceededException(final Duration retryDuration) { - this(null, retryDuration); + /** + * Constructs a new exception indicating when it may become safe to retry + * + * @param retryDuration A duration to wait before retrying, null if no duration can be indicated + */ + public RateLimitExceededException(final @Nullable Duration retryDuration) { + super(null, null, true, false); + this.retryDuration = retryDuration; } - public RateLimitExceededException(final String message, final Duration retryDuration) { - super(message, null, true, false); - // we won't provide a backoff in the case the duration is negative - this.retryDuration = retryDuration.isNegative() ? Optional.empty() : Optional.of(retryDuration); + public Optional getRetryDuration() { + return Optional.ofNullable(retryDuration); } - - public Optional getRetryDuration() { return retryDuration; } } diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/limits/CardinalityRateLimiter.java b/service/src/main/java/org/whispersystems/textsecuregcm/limits/CardinalityRateLimiter.java index 96acef4a5..4f8698d94 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/limits/CardinalityRateLimiter.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/limits/CardinalityRateLimiter.java @@ -54,7 +54,8 @@ public class CardinalityRateLimiter { }); if (rateLimitExceeded) { - throw new RateLimitExceededException(Duration.ofSeconds(getRemainingTtl(key))); + long remainingTtl = getRemainingTtl(key); + throw new RateLimitExceededException(remainingTtl >= 0 ? Duration.ofSeconds(remainingTtl) : null); } } @@ -66,7 +67,14 @@ public class CardinalityRateLimiter { return ttl; } + /** + * Get the remaining ttl for the specified key + * + * @param key with timeout to check + * @return the ttl, or negative in the case of error + */ public long getRemainingTtl(final String key) { + // ttl() returns -2 if key does not exist, -1 if key has no expiration return cacheCluster.withCluster(connection -> connection.sync().ttl(getHllKey(key))); } diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/limits/LockingRateLimiter.java b/service/src/main/java/org/whispersystems/textsecuregcm/limits/LockingRateLimiter.java index 343d684af..81651c395 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/limits/LockingRateLimiter.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/limits/LockingRateLimiter.java @@ -31,7 +31,7 @@ public class LockingRateLimiter extends RateLimiter { public void validate(String key, int amount) throws RateLimitExceededException { if (!acquireLock(key)) { meter.mark(); - throw new RateLimitExceededException("Locked", Duration.ZERO); + throw new RateLimitExceededException(Duration.ZERO); } try { diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/limits/RateLimiter.java b/service/src/main/java/org/whispersystems/textsecuregcm/limits/RateLimiter.java index 463bef678..23c5af012 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/limits/RateLimiter.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/limits/RateLimiter.java @@ -57,7 +57,7 @@ public class RateLimiter { setBucket(key, bucket); } else { meter.mark(); - throw new RateLimitExceededException(key + " , " + amount, bucket.getTimeUntilSpaceAvailable(amount)); + throw new RateLimitExceededException(bucket.getTimeUntilSpaceAvailable(amount)); } } } 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 846397416..e634d4b33 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/mappers/RateLimitExceededExceptionMapper.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/mappers/RateLimitExceededExceptionMapper.java @@ -4,6 +4,8 @@ */ package org.whispersystems.textsecuregcm.mappers; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import org.whispersystems.textsecuregcm.controllers.RateLimitExceededException; import javax.ws.rs.core.Response; @@ -12,17 +14,25 @@ import javax.ws.rs.ext.Provider; @Provider public class RateLimitExceededExceptionMapper implements ExceptionMapper { + private static final Logger logger = LoggerFactory.getLogger(RateLimitExceededExceptionMapper.class); /** * Convert a RateLimitExceededException to a 413 response with a * Retry-After header. * - * @param e A RateLimitExceededException potentially containing a reccomended retry duration + * @param e A RateLimitExceededException potentially containing a recommended retry duration * @return the response */ @Override public Response toResponse(RateLimitExceededException e) { return e.getRetryDuration() + .filter(d -> { + if (d.isNegative()) { + logger.warn("Encountered a negative retry duration: {}, will not include a Retry-After header in response", d); + } + // only include non-negative durations in retry headers + return !d.isNegative(); + }) .map(d -> Response.status(413).header("Retry-After", d.toSeconds())) .orElseGet(() -> Response.status(413)).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 392ae47e6..6385f5b0b 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 @@ -274,14 +274,14 @@ class AccountControllerTest { when(recaptchaClient.verify(eq(INVALID_CAPTCHA_TOKEN), anyString())).thenReturn(false); when(recaptchaClient.verify(eq(VALID_CAPTCHA_TOKEN), anyString())).thenReturn(true); - doThrow(new RateLimitExceededException(SENDER_OVER_PIN, Duration.ZERO)).when(pinLimiter).validate(eq(SENDER_OVER_PIN)); + doThrow(new RateLimitExceededException(Duration.ZERO)).when(pinLimiter).validate(eq(SENDER_OVER_PIN)); - doThrow(new RateLimitExceededException(RATE_LIMITED_PREFIX_HOST, Duration.ZERO)).when(autoBlockLimiter).validate(eq(RATE_LIMITED_PREFIX_HOST)); - doThrow(new RateLimitExceededException(RATE_LIMITED_IP_HOST, Duration.ZERO)).when(autoBlockLimiter).validate(eq(RATE_LIMITED_IP_HOST)); + doThrow(new RateLimitExceededException(Duration.ZERO)).when(autoBlockLimiter).validate(eq(RATE_LIMITED_PREFIX_HOST)); + doThrow(new RateLimitExceededException(Duration.ZERO)).when(autoBlockLimiter).validate(eq(RATE_LIMITED_IP_HOST)); - doThrow(new RateLimitExceededException(SENDER_OVER_PREFIX, Duration.ZERO)).when(smsVoicePrefixLimiter).validate(SENDER_OVER_PREFIX.substring(0, 4+2)); - doThrow(new RateLimitExceededException(RATE_LIMITED_IP_HOST, Duration.ZERO)).when(smsVoiceIpLimiter).validate(RATE_LIMITED_IP_HOST); - doThrow(new RateLimitExceededException(RATE_LIMITED_HOST2, Duration.ZERO)).when(smsVoiceIpLimiter).validate(RATE_LIMITED_HOST2); + doThrow(new RateLimitExceededException(Duration.ZERO)).when(smsVoicePrefixLimiter).validate(SENDER_OVER_PREFIX.substring(0, 4+2)); + doThrow(new RateLimitExceededException(Duration.ZERO)).when(smsVoiceIpLimiter).validate(RATE_LIMITED_IP_HOST); + doThrow(new RateLimitExceededException(Duration.ZERO)).when(smsVoiceIpLimiter).validate(RATE_LIMITED_HOST2); } @AfterEach @@ -1833,7 +1833,7 @@ class AccountControllerTest { .head(); assertThat(response.getStatus()).isEqualTo(413); - assertThat(response.getHeaderString("Retry-After")).isNull(); + assertThat(Long.parseLong(response.getHeaderString("Retry-After"))).isNotNegative(); } @Test