From a8eb27940d1209275001d44df426a38e20277203 Mon Sep 17 00:00:00 2001 From: Ravi Khadiwala Date: Mon, 13 Mar 2023 09:59:03 -0500 Subject: [PATCH] Add per-action captcha site-key configuration - reject captcha requests without valid actions - require specific site keys for each action --- .../textsecuregcm/captcha/Action.java | 43 ++++++++++++ .../textsecuregcm/captcha/CaptchaChecker.java | 64 ++++++++++++------ .../textsecuregcm/captcha/CaptchaClient.java | 16 +++-- .../textsecuregcm/captcha/HCaptchaClient.java | 35 ++++++---- .../captcha/RecaptchaClient.java | 40 +++++++++--- .../captcha/RegistrationCaptchaManager.java | 2 +- .../dynamic/DynamicCaptchaConfiguration.java | 33 +++++++++- .../limits/RateLimitChallengeManager.java | 3 +- .../captcha/CaptchaCheckerTest.java | 65 ++++++++++++------- .../captcha/HCaptchaClientTest.java | 36 +++++++--- .../dynamic/DynamicConfigurationTest.java | 21 +++++- .../controllers/AccountControllerTest.java | 9 +-- .../limits/RateLimitChallengeManagerTest.java | 3 +- 13 files changed, 281 insertions(+), 89 deletions(-) create mode 100644 service/src/main/java/org/whispersystems/textsecuregcm/captcha/Action.java diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/captcha/Action.java b/service/src/main/java/org/whispersystems/textsecuregcm/captcha/Action.java new file mode 100644 index 000000000..26f4889e6 --- /dev/null +++ b/service/src/main/java/org/whispersystems/textsecuregcm/captcha/Action.java @@ -0,0 +1,43 @@ +/* + * Copyright 2023 Signal Messenger, LLC + * SPDX-License-Identifier: AGPL-3.0-only + */ + +package org.whispersystems.textsecuregcm.captcha; + +import com.fasterxml.jackson.annotation.JsonCreator; +import java.util.Arrays; +import java.util.Locale; +import java.util.Map; +import java.util.Optional; +import java.util.function.Function; +import java.util.stream.Collectors; + +public enum Action { + CHALLENGE("challenge"), + REGISTRATION("registration"); + + private final String actionName; + + Action(String actionName) { + this.actionName = actionName; + } + + public String getActionName() { + return actionName; + } + + private static final Map ENUM_MAP = Arrays + .stream(Action.values()) + .collect(Collectors.toMap( + a -> a.actionName, + Function.identity())); + @JsonCreator + public static Action fromString(String key) { + return ENUM_MAP.get(key.toLowerCase(Locale.ROOT).strip()); + } + + static Optional parse(final String action) { + return Optional.ofNullable(fromString(action)); + } +} diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/captcha/CaptchaChecker.java b/service/src/main/java/org/whispersystems/textsecuregcm/captcha/CaptchaChecker.java index 4a3603aa9..920ec679e 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/captcha/CaptchaChecker.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/captcha/CaptchaChecker.java @@ -5,19 +5,26 @@ package org.whispersystems.textsecuregcm.captcha; +import static org.whispersystems.textsecuregcm.metrics.MetricsUtil.name; + import com.google.common.annotations.VisibleForTesting; import io.micrometer.core.instrument.Metrics; import java.io.IOException; import java.util.List; +import java.util.Locale; import java.util.Map; +import java.util.Set; import java.util.function.Function; import java.util.stream.Collectors; import javax.ws.rs.BadRequestException; - -import static org.whispersystems.textsecuregcm.metrics.MetricsUtil.name; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; public class CaptchaChecker { + private static final Logger logger = LoggerFactory.getLogger(CaptchaChecker.class); + private static final String INVALID_SITEKEY_COUNTER_NAME = name(CaptchaChecker.class, "invalidSiteKey"); private static final String ASSESSMENTS_COUNTER_NAME = name(RecaptchaClient.class, "assessments"); + private static final String INVALID_ACTION_COUNTER_NAME = name(CaptchaChecker.class, "invalidActions"); @VisibleForTesting static final String SEPARATOR = "."; @@ -29,44 +36,61 @@ public class CaptchaChecker { .collect(Collectors.toMap(CaptchaClient::scheme, Function.identity())); } + /** * Check if a solved captcha should be accepted - *

* - * @param input expected to contain a prefix indicating the captcha scheme, sitekey, token, and action. The expected - * format is {@code version-prefix.sitekey.[action.]token} - * @param ip IP of the solver + * @param expectedAction the {@link Action} for which this captcha solution is intended + * @param input expected to contain a prefix indicating the captcha scheme, sitekey, token, and action. The + * expected format is {@code version-prefix.sitekey.action.token} + * @param ip IP of the solver * @return An {@link AssessmentResult} indicating whether the solution should be accepted, and a score that can be * used for metrics * @throws IOException if there is an error validating the captcha with the underlying service * @throws BadRequestException if input is not in the expected format */ - public AssessmentResult verify(final String input, final String ip) throws IOException { - /* - * For action to be optional, there is a strong assumption that the token will never contain a {@value SEPARATOR}. - * Observation suggests {@code token} is base-64 encoded. In practice, an action should always be present, but we - * don’t need to be strict. - */ + public AssessmentResult verify( + final Action expectedAction, + final String input, + final String ip) throws IOException { final String[] parts = input.split("\\" + SEPARATOR, 4); // we allow missing actions, if we're missing 1 part, assume it's the action - if (parts.length < 3) { + if (parts.length < 4) { throw new BadRequestException("too few parts"); } - int idx = 0; - final String prefix = parts[idx++]; - final String siteKey = parts[idx++]; - final String action = parts.length == 3 ? null : parts[idx++]; - final String token = parts[idx]; + final String prefix = parts[0]; + final String siteKey = parts[1].toLowerCase(Locale.ROOT).strip(); + final String action = parts[2]; + final String token = parts[3]; final CaptchaClient client = this.captchaClientMap.get(prefix); if (client == null) { throw new BadRequestException("invalid captcha scheme"); } - final AssessmentResult result = client.verify(siteKey, action, token, ip); + + final Action parsedAction = Action.parse(action) + .orElseThrow(() -> { + Metrics.counter(INVALID_ACTION_COUNTER_NAME, "action", action).increment(); + throw new BadRequestException("invalid captcha action"); + }); + + if (!parsedAction.equals(expectedAction)) { + Metrics.counter(INVALID_ACTION_COUNTER_NAME, "action", action).increment(); + throw new BadRequestException("invalid captcha action"); + } + + final Set allowedSiteKeys = client.validSiteKeys(parsedAction); + if (!allowedSiteKeys.contains(siteKey)) { + logger.debug("invalid site-key {}, action={}, token={}", siteKey, action, token); + Metrics.counter(INVALID_SITEKEY_COUNTER_NAME, "action", action).increment(); + throw new BadRequestException("invalid captcha site-key"); + } + + final AssessmentResult result = client.verify(siteKey, parsedAction, token, ip); Metrics.counter(ASSESSMENTS_COUNTER_NAME, - "action", String.valueOf(action), + "action", action, "valid", String.valueOf(result.valid()), "score", result.score(), "provider", prefix) diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/captcha/CaptchaClient.java b/service/src/main/java/org/whispersystems/textsecuregcm/captcha/CaptchaClient.java index 3c8fd5a2a..f22a52ab1 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/captcha/CaptchaClient.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/captcha/CaptchaClient.java @@ -5,29 +5,37 @@ package org.whispersystems.textsecuregcm.captcha; -import javax.annotation.Nullable; import java.io.IOException; +import java.util.Optional; +import java.util.Set; public interface CaptchaClient { + /** * @return the identifying captcha scheme that this CaptchaClient handles */ String scheme(); + /** + * @param action the action to retrieve site keys for + * @return siteKeys this client is willing to accept + */ + Set validSiteKeys(final Action action); + /** * Verify a provided captcha solution * * @param siteKey identifying string for the captcha service - * @param action an optional action indicating the purpose of the captcha + * @param action an action indicating the purpose of the captcha * @param token the captcha solution that will be verified - * @param ip the ip of the captcha solve + * @param ip the ip of the captcha solver * @return An {@link AssessmentResult} indicating whether the solution should be accepted * @throws IOException if the underlying captcha provider returns an error */ AssessmentResult verify( final String siteKey, - final @Nullable String action, + final Action action, final String token, final String ip) throws IOException; } diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/captcha/HCaptchaClient.java b/service/src/main/java/org/whispersystems/textsecuregcm/captcha/HCaptchaClient.java index a081bc6c1..a68c40334 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/captcha/HCaptchaClient.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/captcha/HCaptchaClient.java @@ -16,7 +16,9 @@ import java.net.http.HttpClient; import java.net.http.HttpRequest; import java.net.http.HttpResponse; import java.nio.charset.StandardCharsets; -import javax.annotation.Nullable; +import java.util.Collections; +import java.util.Optional; +import java.util.Set; import javax.ws.rs.core.Response; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -31,6 +33,7 @@ public class HCaptchaClient implements CaptchaClient { private static final String PREFIX = "signal-hcaptcha"; private static final String ASSESSMENT_REASON_COUNTER_NAME = name(HCaptchaClient.class, "assessmentReason"); private static final String INVALID_REASON_COUNTER_NAME = name(HCaptchaClient.class, "invalidReason"); + private static final String INVALID_SITEKEY_COUNTER_NAME = name(HCaptchaClient.class, "invalidSiteKey"); private final String apiKey; private final HttpClient client; private final DynamicConfigurationManager dynamicConfigurationManager; @@ -50,21 +53,31 @@ public class HCaptchaClient implements CaptchaClient { } @Override - public AssessmentResult verify(final String siteKey, final @Nullable String action, final String token, + public Set validSiteKeys(final Action action) { + final DynamicCaptchaConfiguration config = dynamicConfigurationManager.getConfiguration().getCaptchaConfiguration(); + if (!config.isAllowHCaptcha()) { + logger.warn("Received request to verify an hCaptcha, but hCaptcha is not enabled"); + return Collections.emptySet(); + } + return Optional + .ofNullable(config.getHCaptchaSiteKeys().get(action)) + .orElse(Collections.emptySet()); + } + + @Override + public AssessmentResult verify( + final String siteKey, + final Action action, + final String token, final String ip) throws IOException { final DynamicCaptchaConfiguration config = dynamicConfigurationManager.getConfiguration().getCaptchaConfiguration(); - if (!config.isAllowHCaptcha()) { - logger.warn("Received request to verify an hCaptcha, but hCaptcha is not enabled"); - return AssessmentResult.invalid(); - } - final String body = String.format("response=%s&secret=%s&remoteip=%s", URLEncoder.encode(token, StandardCharsets.UTF_8), URLEncoder.encode(this.apiKey, StandardCharsets.UTF_8), ip); - HttpRequest request = HttpRequest.newBuilder() + final HttpRequest request = HttpRequest.newBuilder() .uri(URI.create("https://hcaptcha.com/siteverify")) .header("Content-Type", "application/x-www-form-urlencoded") .POST(HttpRequest.BodyPublishers.ofString(body)) @@ -90,14 +103,14 @@ public class HCaptchaClient implements CaptchaClient { if (!hCaptchaResponse.success) { for (String errorCode : hCaptchaResponse.errorCodes) { Metrics.counter(INVALID_REASON_COUNTER_NAME, - "action", String.valueOf(action), + "action", action.getActionName(), "reason", errorCode).increment(); } return AssessmentResult.invalid(); } // hcaptcha uses the inverse scheme of recaptcha (for hcaptcha, a low score is less risky) - float score = 1.0f - hCaptchaResponse.score; + final float score = 1.0f - hCaptchaResponse.score; if (score < 0.0f || score > 1.0f) { logger.error("Invalid score {} from hcaptcha response {}", hCaptchaResponse.score, hCaptchaResponse); return AssessmentResult.invalid(); @@ -106,7 +119,7 @@ public class HCaptchaClient implements CaptchaClient { for (String reason : hCaptchaResponse.scoreReasons) { Metrics.counter(ASSESSMENT_REASON_COUNTER_NAME, - "action", String.valueOf(action), + "action", action.getActionName(), "reason", reason, "score", scoreString).increment(); } diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/captcha/RecaptchaClient.java b/service/src/main/java/org/whispersystems/textsecuregcm/captcha/RecaptchaClient.java index de1c7ffeb..092a6b2de 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/captcha/RecaptchaClient.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/captcha/RecaptchaClient.java @@ -20,9 +20,11 @@ import java.io.ByteArrayInputStream; import java.io.IOException; import java.math.BigDecimal; import java.nio.charset.StandardCharsets; +import java.util.Collections; import java.util.Objects; +import java.util.Optional; +import java.util.Set; import javax.annotation.Nonnull; -import javax.annotation.Nullable; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.whispersystems.textsecuregcm.configuration.dynamic.DynamicCaptchaConfiguration; @@ -35,8 +37,10 @@ public class RecaptchaClient implements CaptchaClient { private static final String V2_PREFIX = "signal-recaptcha-v2"; private static final String INVALID_REASON_COUNTER_NAME = name(RecaptchaClient.class, "invalidReason"); + private static final String INVALID_SITEKEY_COUNTER_NAME = name(RecaptchaClient.class, "invalidSiteKey"); private static final String ASSESSMENT_REASON_COUNTER_NAME = name(RecaptchaClient.class, "assessmentReason"); + private final String projectPath; private final RecaptchaEnterpriseServiceClient client; private final DynamicConfigurationManager dynamicConfigurationManager; @@ -64,12 +68,28 @@ public class RecaptchaClient implements CaptchaClient { } @Override - public org.whispersystems.textsecuregcm.captcha.AssessmentResult verify(final String sitekey, - final @Nullable String expectedAction, - final String token, final String ip) throws IOException { + public Set validSiteKeys(final Action action) { final DynamicCaptchaConfiguration config = dynamicConfigurationManager.getConfiguration().getCaptchaConfiguration(); if (!config.isAllowRecaptcha()) { log.warn("Received request to verify a recaptcha, but recaptcha is not enabled"); + return Collections.emptySet(); + } + return Optional + .ofNullable(config.getRecaptchaSiteKeys().get(action)) + .orElse(Collections.emptySet()); + } + + @Override + public org.whispersystems.textsecuregcm.captcha.AssessmentResult verify( + final String sitekey, + final Action action, + final String token, + final String ip) throws IOException { + final DynamicCaptchaConfiguration config = dynamicConfigurationManager.getConfiguration().getCaptchaConfiguration(); + final Set allowedSiteKeys = config.getRecaptchaSiteKeys().get(action); + if (allowedSiteKeys != null && !allowedSiteKeys.contains(sitekey)) { + log.info("invalid recaptcha sitekey {}, action={}, token={}", action, token); + Metrics.counter(INVALID_SITEKEY_COUNTER_NAME, "action", action.getActionName()).increment(); return AssessmentResult.invalid(); } @@ -78,8 +98,8 @@ public class RecaptchaClient implements CaptchaClient { .setToken(token) .setUserIpAddress(ip); - if (expectedAction != null) { - eventBuilder.setExpectedAction(expectedAction); + if (action != null) { + eventBuilder.setExpectedAction(action.getActionName()); } final Event event = eventBuilder.build(); @@ -92,21 +112,21 @@ public class RecaptchaClient implements CaptchaClient { if (assessment.getTokenProperties().getValid()) { final float score = assessment.getRiskAnalysis().getScore(); - log.debug("assessment for {} was valid, score: {}", expectedAction, score); + log.debug("assessment for {} was valid, score: {}", action.getActionName(), score); for (RiskAnalysis.ClassificationReason reason : assessment.getRiskAnalysis().getReasonsList()) { Metrics.counter(ASSESSMENT_REASON_COUNTER_NAME, - "action", String.valueOf(expectedAction), + "action", action.getActionName(), "score", AssessmentResult.scoreString(score), "reason", reason.name()) .increment(); } - final BigDecimal threshold = config.getScoreFloorByAction().getOrDefault(expectedAction, config.getScoreFloor()); + final BigDecimal threshold = config.getScoreFloorByAction().getOrDefault(action, config.getScoreFloor()); return new AssessmentResult( score >= threshold.floatValue(), AssessmentResult.scoreString(score)); } else { Metrics.counter(INVALID_REASON_COUNTER_NAME, - "action", String.valueOf(expectedAction), + "action", action.getActionName(), "reason", assessment.getTokenProperties().getInvalidReason().name()) .increment(); return AssessmentResult.invalid(); diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/captcha/RegistrationCaptchaManager.java b/service/src/main/java/org/whispersystems/textsecuregcm/captcha/RegistrationCaptchaManager.java index a6f0a9ba8..044c70048 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/captcha/RegistrationCaptchaManager.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/captcha/RegistrationCaptchaManager.java @@ -54,7 +54,7 @@ public class RegistrationCaptchaManager { public Optional assessCaptcha(final Optional captcha, final String sourceHost) throws IOException { return captcha.isPresent() - ? Optional.of(captchaChecker.verify(captcha.get(), sourceHost)) + ? Optional.of(captchaChecker.verify(Action.REGISTRATION, captcha.get(), sourceHost)) : Optional.empty(); } diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/configuration/dynamic/DynamicCaptchaConfiguration.java b/service/src/main/java/org/whispersystems/textsecuregcm/configuration/dynamic/DynamicCaptchaConfiguration.java index b85ec3358..77a63bae7 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/configuration/dynamic/DynamicCaptchaConfiguration.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/configuration/dynamic/DynamicCaptchaConfiguration.java @@ -7,8 +7,10 @@ package org.whispersystems.textsecuregcm.configuration.dynamic; import com.fasterxml.jackson.annotation.JsonProperty; import com.google.common.annotations.VisibleForTesting; +import org.whispersystems.textsecuregcm.captcha.Action; import java.math.BigDecimal; import java.util.Collections; +import java.util.List; import java.util.Map; import java.util.Set; import javax.validation.constraints.DecimalMax; @@ -29,10 +31,17 @@ public class DynamicCaptchaConfiguration { @JsonProperty private boolean allowRecaptcha = true; + @JsonProperty + @NotNull + private Map> hCaptchaSiteKeys = Collections.emptyMap(); @JsonProperty @NotNull - private Map scoreFloorByAction = Collections.emptyMap(); + private Map> recaptchaSiteKeys = Collections.emptyMap(); + + @JsonProperty + @NotNull + private Map scoreFloorByAction = Collections.emptyMap(); @JsonProperty @NotNull @@ -72,7 +81,7 @@ public class DynamicCaptchaConfiguration { return allowRecaptcha; } - public Map getScoreFloorByAction() { + public Map getScoreFloorByAction() { return scoreFloorByAction; } @@ -85,4 +94,24 @@ public class DynamicCaptchaConfiguration { public void setScoreFloor(final BigDecimal scoreFloor) { this.scoreFloor = scoreFloor; } + + public Map> getHCaptchaSiteKeys() { + return hCaptchaSiteKeys; + } + + @VisibleForTesting + public void setHCaptchaSiteKeys(final Map> hCaptchaSiteKeys) { + this.hCaptchaSiteKeys = hCaptchaSiteKeys; + } + + public Map> getRecaptchaSiteKeys() { + return recaptchaSiteKeys; + } + + @VisibleForTesting + public void setRecaptchaSiteKeys(final Map> recaptchaSiteKeys) { + this.recaptchaSiteKeys = recaptchaSiteKeys; + } + + } diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/limits/RateLimitChallengeManager.java b/service/src/main/java/org/whispersystems/textsecuregcm/limits/RateLimitChallengeManager.java index fcb105256..3295324ab 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/limits/RateLimitChallengeManager.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/limits/RateLimitChallengeManager.java @@ -14,6 +14,7 @@ import java.io.IOException; import java.util.ArrayList; import java.util.Collections; import java.util.List; +import org.whispersystems.textsecuregcm.captcha.Action; import org.whispersystems.textsecuregcm.captcha.CaptchaChecker; import org.whispersystems.textsecuregcm.controllers.RateLimitExceededException; import org.whispersystems.textsecuregcm.metrics.UserAgentTagUtil; @@ -67,7 +68,7 @@ public class RateLimitChallengeManager { rateLimiters.getRecaptchaChallengeAttemptLimiter().validate(account.getUuid()); - final boolean challengeSuccess = captchaChecker.verify(captcha, mostRecentProxyIp).valid(); + final boolean challengeSuccess = captchaChecker.verify(Action.CHALLENGE, captcha, mostRecentProxyIp).valid(); final Tags tags = Tags.of( Tag.of(SOURCE_COUNTRY_TAG_NAME, Util.getCountryCode(account.getNumber())), diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/captcha/CaptchaCheckerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/captcha/CaptchaCheckerTest.java index 83d932d9f..0896ab896 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/captcha/CaptchaCheckerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/captcha/CaptchaCheckerTest.java @@ -16,9 +16,9 @@ import static org.mockito.Mockito.when; import static org.whispersystems.textsecuregcm.captcha.CaptchaChecker.SEPARATOR; import java.io.IOException; +import java.util.Collections; import java.util.List; import java.util.stream.Stream; -import javax.annotation.Nullable; import javax.ws.rs.BadRequestException; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; @@ -27,7 +27,8 @@ import org.junit.jupiter.params.provider.MethodSource; public class CaptchaCheckerTest { - private static final String SITE_KEY = "site-key"; + private static final String CHALLENGE_SITE_KEY = "challenge-site-key"; + private static final String REG_SITE_KEY = "registration-site-key"; private static final String TOKEN = "some-token"; private static final String PREFIX = "prefix"; private static final String PREFIX_A = "prefix-a"; @@ -36,26 +37,33 @@ public class CaptchaCheckerTest { static Stream parseInputToken() { return Stream.of( Arguments.of( - String.join(SEPARATOR, PREFIX, SITE_KEY, TOKEN), + String.join(SEPARATOR, PREFIX, CHALLENGE_SITE_KEY, "challenge", TOKEN), TOKEN, - SITE_KEY, - null), + CHALLENGE_SITE_KEY, + Action.CHALLENGE), Arguments.of( - String.join(SEPARATOR, PREFIX, SITE_KEY, "an-action", TOKEN), + String.join(SEPARATOR, PREFIX, REG_SITE_KEY, "registration", TOKEN), TOKEN, - SITE_KEY, - "an-action"), + REG_SITE_KEY, + Action.REGISTRATION), Arguments.of( - String.join(SEPARATOR, PREFIX, SITE_KEY, "an-action", TOKEN, "something-else"), + String.join(SEPARATOR, PREFIX, CHALLENGE_SITE_KEY, "challenge", TOKEN, "something-else"), TOKEN + SEPARATOR + "something-else", - SITE_KEY, - "an-action") + CHALLENGE_SITE_KEY, + Action.CHALLENGE), + Arguments.of( + String.join(SEPARATOR, PREFIX, CHALLENGE_SITE_KEY, "ChAlLeNgE", TOKEN), + TOKEN, + CHALLENGE_SITE_KEY, + Action.CHALLENGE) ); } private static CaptchaClient mockClient(final String prefix) throws IOException { final CaptchaClient captchaClient = mock(CaptchaClient.class); when(captchaClient.scheme()).thenReturn(prefix); + when(captchaClient.validSiteKeys(eq(Action.CHALLENGE))).thenReturn(Collections.singleton(CHALLENGE_SITE_KEY)); + when(captchaClient.validSiteKeys(eq(Action.REGISTRATION))).thenReturn(Collections.singleton(REG_SITE_KEY)); when(captchaClient.verify(any(), any(), any(), any())).thenReturn(AssessmentResult.invalid()); return captchaClient; } @@ -63,10 +71,13 @@ public class CaptchaCheckerTest { @ParameterizedTest @MethodSource - void parseInputToken(final String input, final String expectedToken, final String siteKey, - @Nullable final String expectedAction) throws IOException { + void parseInputToken( + final String input, + final String expectedToken, + final String siteKey, + final Action expectedAction) throws IOException { final CaptchaClient captchaClient = mockClient(PREFIX); - new CaptchaChecker(List.of(captchaClient)).verify(input, null); + new CaptchaChecker(List.of(captchaClient)).verify(expectedAction, input, null); verify(captchaClient, times(1)).verify(eq(siteKey), eq(expectedAction), eq(expectedToken), any()); } @@ -89,31 +100,37 @@ public class CaptchaCheckerTest { @Test public void choose() throws IOException { - String ainput = String.join(SEPARATOR, PREFIX_A, SITE_KEY, TOKEN); - String binput = String.join(SEPARATOR, PREFIX_B, SITE_KEY, TOKEN); + String ainput = String.join(SEPARATOR, PREFIX_A, CHALLENGE_SITE_KEY, "challenge", TOKEN); + String binput = String.join(SEPARATOR, PREFIX_B, CHALLENGE_SITE_KEY, "challenge", TOKEN); final CaptchaClient a = mockClient(PREFIX_A); final CaptchaClient b = mockClient(PREFIX_B); - new CaptchaChecker(List.of(a, b)).verify(ainput, null); + new CaptchaChecker(List.of(a, b)).verify(Action.CHALLENGE, ainput, null); verify(a, times(1)).verify(any(), any(), any(), any()); - new CaptchaChecker(List.of(a, b)).verify(binput, null); + new CaptchaChecker(List.of(a, b)).verify(Action.CHALLENGE, binput, null); verify(b, times(1)).verify(any(), any(), any(), any()); } - static Stream badToken() { + static Stream badArgs() { return Stream.of( - Arguments.of(String.join(SEPARATOR, "invalid", SITE_KEY, "action", TOKEN)), - Arguments.of(String.join(SEPARATOR, PREFIX, TOKEN)), - Arguments.of(String.join(SEPARATOR, SITE_KEY, PREFIX, "action", TOKEN)) + Arguments.of(String.join(SEPARATOR, "invalid", CHALLENGE_SITE_KEY, "challenge", TOKEN)), // bad prefix + Arguments.of(String.join(SEPARATOR, PREFIX, "challenge", TOKEN)), // no site key + Arguments.of(String.join(SEPARATOR, CHALLENGE_SITE_KEY, PREFIX, "challenge", TOKEN)), // incorrect order + Arguments.of(String.join(SEPARATOR, PREFIX, CHALLENGE_SITE_KEY, "unknown_action", TOKEN)), // bad action + Arguments.of(String.join(SEPARATOR, PREFIX, CHALLENGE_SITE_KEY, "registration", TOKEN)), // action mismatch + Arguments.of(String.join(SEPARATOR, PREFIX, "bad-site-key", "challenge", TOKEN)), // invalid site key + Arguments.of(String.join(SEPARATOR, PREFIX, CHALLENGE_SITE_KEY, "registration", TOKEN)), // site key for wrong type + Arguments.of(String.join(SEPARATOR, PREFIX, REG_SITE_KEY, "challenge", TOKEN)) // site key for wrong type ); } @ParameterizedTest @MethodSource - public void badToken(final String input) throws IOException { + public void badArgs(final String input) throws IOException { final CaptchaClient cc = mockClient(PREFIX); - assertThrows(BadRequestException.class, () -> new CaptchaChecker(List.of(cc)).verify(input, null)); + assertThrows(BadRequestException.class, + () -> new CaptchaChecker(List.of(cc)).verify(Action.CHALLENGE, input, null)); } } diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/captcha/HCaptchaClientTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/captcha/HCaptchaClientTest.java index e293b2346..58bf6f364 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/captcha/HCaptchaClientTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/captcha/HCaptchaClientTest.java @@ -2,6 +2,7 @@ package org.whispersystems.textsecuregcm.captcha; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -10,6 +11,10 @@ import java.io.IOException; import java.math.BigDecimal; import java.net.http.HttpClient; import java.net.http.HttpResponse; +import java.util.Arrays; +import java.util.Collections; +import java.util.Map; +import java.util.Set; import java.util.stream.Stream; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; @@ -49,7 +54,7 @@ public class HCaptchaClientTest { success, 1 - score)); // hCaptcha scores are inverted compared to recaptcha scores. (low score is good) final AssessmentResult result = new HCaptchaClient("fake", client, mockConfig(true, 0.5)) - .verify(SITE_KEY, "whatever", TOKEN, null); + .verify(SITE_KEY, Action.CHALLENGE, TOKEN, null); if (!success) { assertThat(result).isEqualTo(AssessmentResult.invalid()); } else { @@ -62,31 +67,40 @@ public class HCaptchaClientTest { public void errorResponse() throws IOException, InterruptedException { final HttpClient httpClient = mockResponder(503, ""); final HCaptchaClient client = new HCaptchaClient("fake", httpClient, mockConfig(true, 0.5)); - assertThrows(IOException.class, () -> client.verify(SITE_KEY, "whatever", TOKEN, null)); + assertThrows(IOException.class, () -> client.verify(SITE_KEY, Action.CHALLENGE, TOKEN, null)); } @Test public void invalidScore() throws IOException, InterruptedException { final HttpClient httpClient = mockResponder(200, """ - {"success" : true, "score": 1.1} - """); + {"success" : true, "score": 1.1} + """); final HCaptchaClient client = new HCaptchaClient("fake", httpClient, mockConfig(true, 0.5)); - assertThat(client.verify(SITE_KEY, "whatever", TOKEN, null)).isEqualTo(AssessmentResult.invalid()); + assertThat(client.verify(SITE_KEY, Action.CHALLENGE, TOKEN, null)).isEqualTo(AssessmentResult.invalid()); } @Test public void badBody() throws IOException, InterruptedException { final HttpClient httpClient = mockResponder(200, """ - {"success" : true, - """); + {"success" : true, + """); final HCaptchaClient client = new HCaptchaClient("fake", httpClient, mockConfig(true, 0.5)); - assertThrows(IOException.class, () -> client.verify(SITE_KEY, "whatever", TOKEN, null)); + assertThrows(IOException.class, () -> client.verify(SITE_KEY, Action.CHALLENGE, TOKEN, null)); } @Test public void disabled() throws IOException { final HCaptchaClient hc = new HCaptchaClient("fake", null, mockConfig(false, 0.5)); - assertThat(hc.verify(SITE_KEY, null, TOKEN, null)).isEqualTo(AssessmentResult.invalid()); + assertTrue(Arrays.stream(Action.values()).map(hc::validSiteKeys).allMatch(Set::isEmpty)); + } + + @Test + public void badSiteKey() throws IOException { + final HCaptchaClient hc = new HCaptchaClient("fake", null, mockConfig(true, 0.5)); + for (Action action : Action.values()) { + assertThat(hc.validSiteKeys(action)).contains(SITE_KEY); + assertThat(hc.validSiteKeys(action)).doesNotContain("invalid"); + } } private static HttpClient mockResponder(final int statusCode, final String jsonBody) @@ -105,6 +119,10 @@ public class HCaptchaClientTest { final DynamicCaptchaConfiguration config = new DynamicCaptchaConfiguration(); config.setAllowHCaptcha(enabled); config.setScoreFloor(BigDecimal.valueOf(scoreFloor)); + config.setHCaptchaSiteKeys(Map.of( + Action.REGISTRATION, Collections.singleton(SITE_KEY), + Action.CHALLENGE, Collections.singleton(SITE_KEY) + )); @SuppressWarnings("unchecked") final DynamicConfigurationManager m = mock( DynamicConfigurationManager.class); diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/configuration/dynamic/DynamicConfigurationTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/configuration/dynamic/DynamicConfigurationTest.java index 4362a5f7b..c37c6ded1 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/configuration/dynamic/DynamicConfigurationTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/configuration/dynamic/DynamicConfigurationTest.java @@ -20,6 +20,7 @@ import java.util.Optional; import java.util.Set; import java.util.UUID; import org.junit.jupiter.api.Test; +import org.whispersystems.textsecuregcm.captcha.Action; import org.whispersystems.textsecuregcm.limits.RateLimiterConfig; import org.whispersystems.textsecuregcm.limits.RateLimiters; import org.whispersystems.textsecuregcm.storage.DynamicConfigurationManager; @@ -265,6 +266,15 @@ class DynamicConfigurationTest { scoreFloorByAction: challenge: 0.1 registration: 0.2 + hCaptchaSiteKeys: + challenge: + - ab317f2a-2b76-4098-84c9-ecdf8ea44f53 + registration: + - e4ddb6ff-05e7-497b-9a29-b76e7331789c + - 52fdbc88-f246-4705-a7dd-05ad85b93420 + recaptchaSiteKeys: + challenge: + - 299068b6-ac78-4288-a90b-2e2ce5a6ddfe """; final DynamicCaptchaConfiguration config = @@ -273,8 +283,15 @@ class DynamicConfigurationTest { assertEquals(Set.of("1"), config.getSignupCountryCodes()); assertEquals(0.9f, config.getScoreFloor().floatValue()); - assertEquals(0.1f, config.getScoreFloorByAction().get("challenge").floatValue()); - assertEquals(0.2f, config.getScoreFloorByAction().get("registration").floatValue()); + assertEquals(0.1f, config.getScoreFloorByAction().get(Action.CHALLENGE).floatValue()); + assertEquals(0.2f, config.getScoreFloorByAction().get(Action.REGISTRATION).floatValue()); + + assertThat(config.getHCaptchaSiteKeys().get(Action.CHALLENGE)).contains("ab317f2a-2b76-4098-84c9-ecdf8ea44f53"); + assertThat(config.getHCaptchaSiteKeys().get(Action.REGISTRATION)).contains("e4ddb6ff-05e7-497b-9a29-b76e7331789c"); + assertThat(config.getHCaptchaSiteKeys().get(Action.REGISTRATION)).contains("52fdbc88-f246-4705-a7dd-05ad85b93420"); + + assertThat(config.getRecaptchaSiteKeys().get(Action.CHALLENGE)).contains("299068b6-ac78-4288-a90b-2e2ce5a6ddfe"); + assertThat(config.getRecaptchaSiteKeys().get(Action.REGISTRATION)).isNull(); } } diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/controllers/AccountControllerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/controllers/AccountControllerTest.java index d5e1ecd9c..aed2d23d9 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/controllers/AccountControllerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/controllers/AccountControllerTest.java @@ -74,6 +74,7 @@ import org.whispersystems.textsecuregcm.auth.SaltedTokenHash; import org.whispersystems.textsecuregcm.auth.StoredRegistrationLock; import org.whispersystems.textsecuregcm.auth.StoredVerificationCode; import org.whispersystems.textsecuregcm.auth.TurnTokenGenerator; +import org.whispersystems.textsecuregcm.captcha.Action; import org.whispersystems.textsecuregcm.captcha.AssessmentResult; import org.whispersystems.textsecuregcm.captcha.CaptchaChecker; import org.whispersystems.textsecuregcm.captcha.RegistrationCaptchaManager; @@ -345,9 +346,9 @@ class AccountControllerTest { when(dynamicConfiguration.getCaptchaConfiguration()).thenReturn(signupCaptchaConfig); } - when(captchaChecker.verify(eq(INVALID_CAPTCHA_TOKEN), anyString())) + when(captchaChecker.verify(eq(Action.REGISTRATION), eq(INVALID_CAPTCHA_TOKEN), anyString())) .thenReturn(AssessmentResult.invalid()); - when(captchaChecker.verify(eq(VALID_CAPTCHA_TOKEN), anyString())) + when(captchaChecker.verify(eq(Action.REGISTRATION), eq(VALID_CAPTCHA_TOKEN), anyString())) .thenReturn(new AssessmentResult(true, "")); doThrow(new RateLimitExceededException(Duration.ZERO, true)).when(pinLimiter).validate(eq(SENDER_OVER_PIN)); @@ -849,7 +850,7 @@ class AccountControllerTest { assertThat(response.getStatus()).isEqualTo(200); - verify(captchaChecker).verify(eq(VALID_CAPTCHA_TOKEN), eq(NICE_HOST)); + verify(captchaChecker).verify(eq(Action.REGISTRATION), eq(VALID_CAPTCHA_TOKEN), eq(NICE_HOST)); verify(registrationServiceClient).sendRegistrationCode(sessionId, MessageTransport.SMS, ClientType.UNKNOWN, null, AccountController.REGISTRATION_RPC_TIMEOUT); } @@ -866,7 +867,7 @@ class AccountControllerTest { assertThat(response.getStatus()).isEqualTo(402); - verify(captchaChecker).verify(eq(INVALID_CAPTCHA_TOKEN), eq(NICE_HOST)); + verify(captchaChecker).verify(eq(Action.REGISTRATION), eq(INVALID_CAPTCHA_TOKEN), eq(NICE_HOST)); verifyNoInteractions(registrationServiceClient); } diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/limits/RateLimitChallengeManagerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/limits/RateLimitChallengeManagerTest.java index 0525241c6..60a2afd65 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/limits/RateLimitChallengeManagerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/limits/RateLimitChallengeManagerTest.java @@ -17,6 +17,7 @@ import java.util.UUID; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.ValueSource; +import org.whispersystems.textsecuregcm.captcha.Action; import org.whispersystems.textsecuregcm.captcha.AssessmentResult; import org.whispersystems.textsecuregcm.captcha.CaptchaChecker; import org.whispersystems.textsecuregcm.controllers.RateLimitExceededException; @@ -75,7 +76,7 @@ class RateLimitChallengeManagerTest { when(account.getNumber()).thenReturn("+18005551234"); when(account.getUuid()).thenReturn(UUID.randomUUID()); - when(captchaChecker.verify(any(), any())) + when(captchaChecker.verify(eq(Action.CHALLENGE), any(), any())) .thenReturn(successfulChallenge ? new AssessmentResult(true, "") : AssessmentResult.invalid());