From eaabbd51888b32a361ffcc0f0b370721b69f00d0 Mon Sep 17 00:00:00 2001 From: ravi-signal <99042880+ravi-signal@users.noreply.github.com> Date: Mon, 5 Jan 2026 17:23:11 -0500 Subject: [PATCH] Add nested message validation --- .../grpc/ValidatingInterceptor.java | 17 +++++++++ .../main/proto/org/signal/chat/backups.proto | 2 +- .../grpc/ValidatingInterceptorTest.java | 38 ++++++++++++++++++- service/src/test/proto/validation_test.proto | 15 ++++++++ 4 files changed, 70 insertions(+), 2 deletions(-) diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/grpc/ValidatingInterceptor.java b/service/src/main/java/org/whispersystems/textsecuregcm/grpc/ValidatingInterceptor.java index db7b640f6..ca571278c 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/grpc/ValidatingInterceptor.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/grpc/ValidatingInterceptor.java @@ -15,6 +15,7 @@ import io.grpc.ServerCall; import io.grpc.ServerCallHandler; import io.grpc.ServerInterceptor; import io.grpc.StatusException; +import java.util.List; import java.util.Map; import org.whispersystems.textsecuregcm.grpc.validators.E164FieldValidator; import org.whispersystems.textsecuregcm.grpc.validators.EnumSpecifiedFieldValidator; @@ -81,12 +82,28 @@ public class ValidatingInterceptor implements ServerInterceptor { for (final Map.Entry entry: fd.getOptions().getAllFields().entrySet()) { final Descriptors.FieldDescriptor extensionFieldDescriptor = entry.getKey(); final String extensionName = extensionFieldDescriptor.getFullName(); + + // first validate the field final FieldValidator validator = fieldValidators.get(extensionName); // not all extensions are validators, so `validator` value here could legitimately be `null` if (validator != null) { validator.validate(entry.getValue(), fd, msg); } } + + // Recursively validate the field's value(s) if it is a message or a repeated field + // gRPC's proto deserialization limits nesting to 100 so this has bounded stack usage + if (fd.isRepeated() && msg.getField(fd) instanceof List list) { + // Checking for repeated fields also handles maps, because maps are syntax sugar for repeated MapEntries + // which themselves are Messages that will be recursively descended. + for (final Object o : list) { + validateMessage(o); + } + } else if (fd.hasPresence() && msg.hasField(fd)) { + // If the field has presence information and is present, recursively validate it. Not all fields have + // presence, but we only validate Message type fields anyway, which always have explicit presence. + validateMessage(msg.getField(fd)); + } } } catch (final StatusException e) { throw e; diff --git a/service/src/main/proto/org/signal/chat/backups.proto b/service/src/main/proto/org/signal/chat/backups.proto index c3027401b..73773bedc 100644 --- a/service/src/main/proto/org/signal/chat/backups.proto +++ b/service/src/main/proto/org/signal/chat/backups.proto @@ -408,7 +408,7 @@ message CopyMediaItem { /** * The attachment cdn of the object to copy into the backup */ - int32 source_attachment_cdn = 1 [(require.present) = true]; + int32 source_attachment_cdn = 1 [(require.range).min = 1, (require.range).max = 3]; /** * The attachment key of the object to copy into the backup diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/grpc/ValidatingInterceptorTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/grpc/ValidatingInterceptorTest.java index 99a3d222c..67b13bb78 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/grpc/ValidatingInterceptorTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/grpc/ValidatingInterceptorTest.java @@ -32,9 +32,11 @@ import org.junit.jupiter.params.provider.MethodSource; import org.junit.jupiter.params.provider.ValueSource; import org.signal.chat.require.Auth; import org.signal.chat.rpc.Color; +import org.signal.chat.rpc.NestedMessage; import org.signal.chat.rpc.ReactorAnonymousServiceGrpc; import org.signal.chat.rpc.ReactorAuthServiceGrpc; import org.signal.chat.rpc.ReactorValidationTestServiceGrpc; +import org.signal.chat.rpc.RecursiveMessage; import org.signal.chat.rpc.ValidationTestServiceGrpc; import org.signal.chat.rpc.ValidationsRequest; import org.signal.chat.rpc.ValidationsResponse; @@ -374,6 +376,37 @@ public class ValidatingInterceptorTest { stub.validationsEndpoint(builderWithValidDefaults().build()); } + @Test + public void testFailedValidationOnNestedMessage() { + assertStatusException(Status.INVALID_ARGUMENT, () -> + stub.validationsEndpoint(builderWithValidDefaults().setNested(NestedMessage.newBuilder().setI32(101)).build())); + + assertStatusException(Status.INVALID_ARGUMENT, () -> + stub.validationsEndpoint(builderWithValidDefaults() + .clearRepeatedNested() + .addRepeatedNested(NestedMessage.newBuilder().setI32(101)).build())); + + assertStatusException(Status.INVALID_ARGUMENT, () -> + stub.validationsEndpoint(builderWithValidDefaults() + .clearMapNested() + .putMapNested("foo", NestedMessage.newBuilder().setI32(101).build()).build())); + } + + @Test + public void deeplyNestedValidation() throws Exception { + // The default proto nesting limit (what we use in gRPC) is 100. Make sure we can handle twice that amount of nesting + int numNestedMessages = 100 * 2; + RecursiveMessage.Builder curr = RecursiveMessage.newBuilder().setI32(101); + for (int i = 0; i < numNestedMessages; i++) { + final RecursiveMessage.Builder pred = RecursiveMessage.newBuilder(); + pred.setNext(curr); + curr = pred; + } + final RecursiveMessage recursiveMessage = curr.build(); + assertStatusException(Status.INVALID_ARGUMENT, () -> + stub.validationsEndpoint(builderWithValidDefaults().setRecursiveMessage(recursiveMessage).build())); + } + @Nonnull private static ValidationsRequest.Builder builderWithValidDefaults() { return ValidationsRequest.newBuilder() @@ -398,7 +431,10 @@ public class ValidatingInterceptorTest { .setRangeSizeBytes(ByteString.copyFrom(new byte[3])) .addAllFixedSizeList(List.of("a", "b", "c", "d", "e")) .addAllRangeSizeList(List.of("a", "b", "c", "d", "e")) - .setI32Range(15); + .setI32Range(15) + .setNested(NestedMessage.getDefaultInstance()) + .addRepeatedNested(NestedMessage.getDefaultInstance()) + .putMapNested("test", NestedMessage.getDefaultInstance()); } private static void assertStatusException(final Status expected, final Executable serviceCall) { diff --git a/service/src/test/proto/validation_test.proto b/service/src/test/proto/validation_test.proto index 6b35d0804..0cc0326fb 100644 --- a/service/src/test/proto/validation_test.proto +++ b/service/src/test/proto/validation_test.proto @@ -66,6 +66,21 @@ message ValidationsRequest { message RequirePresentMessage {} RequirePresentMessage presentMessage = 25 [(require.present) = true]; optional RequirePresentMessage optionalPresentMessage = 26 [(require.present) = true]; + + NestedMessage nested = 27; + repeated NestedMessage repeatedNested = 28; + map mapNested = 29; + + RecursiveMessage recursiveMessage = 30; +} + +message NestedMessage { + int32 i32 = 1 [(require.range).max = 100]; +} + +message RecursiveMessage { + RecursiveMessage next = 1; + int32 i32 = 2 [(require.range).max = 100]; } message MessageWithInvalidRangeConstraint {