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 11ad9b278..a44307107 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/grpc/ValidatingInterceptor.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/grpc/ValidatingInterceptor.java @@ -94,6 +94,12 @@ public class ValidatingInterceptor implements ServerInterceptor { final Descriptors.FieldDescriptor extensionFieldDescriptor = entry.getKey(); final String extensionName = extensionFieldDescriptor.getFullName(); + // If this is a oneof, but this field isn't set, we shouldn't validate it. We assume if you have a validator + // that requires presence, you don't actually want presence enforcement on a oneof case. + if (fd.getRealContainingOneof() != null && !msg.hasField(fd)) { + continue; + } + // first validate the field final FieldValidator validator = fieldValidators.get(extensionName); // not all extensions are validators, so `validator` value here could legitimately be `null` diff --git a/service/src/main/proto/org/signal/chat/messages.proto b/service/src/main/proto/org/signal/chat/messages.proto index 7a51e9d80..9042567fa 100644 --- a/service/src/main/proto/org/signal/chat/messages.proto +++ b/service/src/main/proto/org/signal/chat/messages.proto @@ -198,7 +198,7 @@ message SendSealedSenderMessageRequest { oneof authorization { // The unidentified access key (UAK) for the destination account. - bytes unidentified_access_key = 5; + bytes unidentified_access_key = 5 [(require.exactlySize) = 16]; // A group send endorsement token for the destination account. bytes group_send_token = 6; diff --git a/service/src/main/proto/org/signal/chat/require.proto b/service/src/main/proto/org/signal/chat/require.proto index abc870a1c..071c12a2a 100644 --- a/service/src/main/proto/org/signal/chat/require.proto +++ b/service/src/main/proto/org/signal/chat/require.proto @@ -14,8 +14,9 @@ import "google/protobuf/descriptor.proto"; extend google.protobuf.FieldOptions { /* * Requires a field to have content of non-zero size/length. - * Applies to both `optional` and regular fields, i.e. if the field is not set or has a default value, - * it's considered to be empty. + * Applies to both `optional` and regular fields, i.e. if the field is not set + * or has a default value, it's considered to be empty. This does not apply + * to fields that are contained in a `oneof`. * * ``` * import "org/signal/chat/require.proto"; @@ -57,8 +58,10 @@ extend google.protobuf.FieldOptions { /* * Requires a size/length of a field to be within certain boundaries. - * Applies to both `optional` and regular fields, i.e. if the field is not set or has a default value, - * its size considered to be zero. + * Applies to both `optional` and regular fields, i.e. if the field is not set + * or has a default value, its size considered to be zero. However, if the + * field is contained in a `oneof` and is not set, this annotation does not + * apply. * * ``` * import "org/signal/chat/require.proto"; @@ -76,9 +79,11 @@ extend google.protobuf.FieldOptions { optional SizeConstraint size = 70003; /* - * Requires a size/length of a field to be one of the specified values. - * Applies to both `optional` and regular fields, i.e. if the field is not set or has a default value, - * its size considered to be zero. + * Requires a size/length of a field to be within certain boundaries. + * Applies to both `optional` and regular fields, i.e. if the field is not set + * or has a default value, its size considered to be zero. However, if the + * field is contained in a `oneof` and is not set, this annotation does not + * apply. * * ``` * import "org/signal/chat/require.proto"; @@ -130,7 +135,8 @@ extend google.protobuf.FieldOptions { * Require a value of a message field to be present. * * Applies to both `optional` and regular fields (both of which have explicit - * presence for the message type anyways) + * presence for the message type anyways). This does not apply to fields that + * are contained in a `oneof`. * * ``` * import "org/signal/chat/require.proto"; 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 024831d16..a98fb47a2 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/grpc/ValidatingInterceptorTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/grpc/ValidatingInterceptorTest.java @@ -5,6 +5,7 @@ package org.whispersystems.textsecuregcm.grpc; +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertEquals; import com.google.protobuf.ByteString; @@ -406,6 +407,22 @@ public class ValidatingInterceptorTest { stub.validationsEndpoint(builderWithValidDefaults().setRecursiveMessage(recursiveMessage).build())); } + @Test + public void oneOfValidation() throws Exception { + + // These should pass even if we don't meet the requirements on the one-of case that we're not setting + assertDoesNotThrow(() -> + stub.validationsEndpoint(builderWithValidDefaults() + .setOneOfNonEmptyBytes(ByteString.copyFrom(new byte[1])) + .build())); + assertDoesNotThrow(() -> + stub.validationsEndpoint(builderWithValidDefaults() + .setOneOfMessage(ValidationsRequest.RequirePresentMessage.getDefaultInstance()) + .build())); + + assertStatusException(Status.INVALID_ARGUMENT, () -> stub.validationsEndpoint(ValidationsRequest.newBuilder().setOneOfNonEmptyBytes(ByteString.EMPTY).build())); + } + @Nonnull private static ValidationsRequest.Builder builderWithValidDefaults() { return ValidationsRequest.newBuilder() diff --git a/service/src/test/proto/validation_test.proto b/service/src/test/proto/validation_test.proto index 0cc0326fb..66f519cb7 100644 --- a/service/src/test/proto/validation_test.proto +++ b/service/src/test/proto/validation_test.proto @@ -72,6 +72,11 @@ message ValidationsRequest { map mapNested = 29; RecursiveMessage recursiveMessage = 30; + + oneof oneOf { + RequirePresentMessage oneOfMessage = 31 [(require.present) = true]; + bytes oneOfNonEmptyBytes = 32 [(require.nonEmpty) = true]; + } } message NestedMessage {