mirror of
https://github.com/signalapp/Signal-Server
synced 2026-04-21 23:28:04 +01:00
Add per-action captcha site-key configuration
- reject captcha requests without valid actions - require specific site keys for each action
This commit is contained in:
Ravi Khadiwala
committed by
ravi-signal
ravi-signal
parent
fd8918eaff
commit
a8eb27940d
@@ -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<Arguments> 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<Arguments> badToken() {
|
||||
static Stream<Arguments> 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));
|
||||
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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<DynamicConfiguration> m = mock(
|
||||
DynamicConfigurationManager.class);
|
||||
|
||||
@@ -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();
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
|
||||
|
||||
@@ -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());
|
||||
|
||||
Reference in New Issue
Block a user