From 1b83b6c1fd3b6d7cb325722cee6d60de93df71fd Mon Sep 17 00:00:00 2001 From: Sean Huh Date: Wed, 4 Nov 2026 14:47:15 -0800 Subject: [PATCH] Validate Duration and Timestamp bounds on CEL message construction. PiperOrigin-RevId: 1014153140 --- .../cel/common/internal/ProtoTimeUtils.java | 9 ++ .../java/dev/cel/common/values/BUILD.bazel | 1 + .../values/ProtoMessageValueProvider.java | 6 +- .../common/internal/ProtoTimeUtilsTest.java | 31 +++++++ .../values/ProtoMessageValueProviderTest.java | 87 +++++++++++++------ .../test/java/dev/cel/conformance/BUILD.bazel | 22 ++--- runtime/src/test/resources/wrappers.baseline | 10 ++- .../dev/cel/testing/BaseInterpreterTest.java | 13 ++- 8 files changed, 128 insertions(+), 51 deletions(-) diff --git a/common/src/main/java/dev/cel/common/internal/ProtoTimeUtils.java b/common/src/main/java/dev/cel/common/internal/ProtoTimeUtils.java index e4eec565c..be2c1d411 100644 --- a/common/src/main/java/dev/cel/common/internal/ProtoTimeUtils.java +++ b/common/src/main/java/dev/cel/common/internal/ProtoTimeUtils.java @@ -196,6 +196,15 @@ public static Duration fromMillisToDuration(long milliseconds) { (int) (milliseconds % MILLIS_PER_SECOND * NANOS_PER_MILLISECOND)); } + /** Validates {@link Duration} and {@link Timestamp} bounds; ignores all other values. */ + public static void validateWellKnownProto(Object message) { + if (message instanceof Duration) { + checkValid((Duration) message); + } else if (message instanceof Timestamp) { + checkValid((Timestamp) message); + } + } + /** Throws an {@link IllegalArgumentException} if the given {@link Duration} is not valid. */ @CanIgnoreReturnValue private static Duration checkValid(Duration duration) { diff --git a/common/src/main/java/dev/cel/common/values/BUILD.bazel b/common/src/main/java/dev/cel/common/values/BUILD.bazel index 774f720e4..53d3788bc 100644 --- a/common/src/main/java/dev/cel/common/values/BUILD.bazel +++ b/common/src/main/java/dev/cel/common/values/BUILD.bazel @@ -310,6 +310,7 @@ java_library( "//common/annotations", "//common/internal:dynamic_proto", "//common/internal:proto_message_factory", + "//common/internal:proto_time_utils", "//common/values", "//common/values:cel_value_provider", "@maven//:com_google_errorprone_error_prone_annotations", diff --git a/common/src/main/java/dev/cel/common/values/ProtoMessageValueProvider.java b/common/src/main/java/dev/cel/common/values/ProtoMessageValueProvider.java index 7beb40c61..7cb5d4e39 100644 --- a/common/src/main/java/dev/cel/common/values/ProtoMessageValueProvider.java +++ b/common/src/main/java/dev/cel/common/values/ProtoMessageValueProvider.java @@ -23,6 +23,7 @@ import dev.cel.common.internal.DynamicProto; import dev.cel.common.internal.ProtoAdapter; import dev.cel.common.internal.ProtoMessageFactory; +import dev.cel.common.internal.ProtoTimeUtils; import java.util.Map; import java.util.Optional; @@ -61,7 +62,10 @@ public Optional newValue(String structType, Map fields) fieldValue.ifPresent(o -> builder.setField(fieldDescriptor, o)); } - return Optional.of(protoCelValueConverter.toRuntimeValue(builder.build())); + Message message = builder.build(); + // Validate before conversion, as toJavaDuration normalizes mismatched signs and nanos >= 1e9. + ProtoTimeUtils.validateWellKnownProto(message); + return Optional.of(protoCelValueConverter.toRuntimeValue(message)); } catch (ArrayIndexOutOfBoundsException e) { throw new IllegalArgumentException(e.getMessage()); } diff --git a/common/src/test/java/dev/cel/common/internal/ProtoTimeUtilsTest.java b/common/src/test/java/dev/cel/common/internal/ProtoTimeUtilsTest.java index 9c6dae34b..56ef226ca 100644 --- a/common/src/test/java/dev/cel/common/internal/ProtoTimeUtilsTest.java +++ b/common/src/test/java/dev/cel/common/internal/ProtoTimeUtilsTest.java @@ -255,4 +255,35 @@ public void between_invalidTimestampThrows(long fromSeconds, int fromNanos, long assertThrows(IllegalArgumentException.class, () -> ProtoTimeUtils.between(from, to)); } + + @Test + public void validateWellKnownProto_validValues_succeeds() { + ProtoTimeUtils.validateWellKnownProto( + Duration.newBuilder().setSeconds(DURATION_SECONDS_MAX).setNanos(MAX_NANOS).build()); + ProtoTimeUtils.validateWellKnownProto( + Timestamp.newBuilder().setSeconds(TIMESTAMP_SECONDS_MIN).build()); + ProtoTimeUtils.validateWellKnownProto("not a proto"); + } + + @Test + public void validateWellKnownProto_invalidDuration_throws() { + Duration invalid = Duration.newBuilder().setSeconds(DURATION_SECONDS_MAX + 1).build(); + + IllegalArgumentException thrown = + assertThrows( + IllegalArgumentException.class, () -> ProtoTimeUtils.validateWellKnownProto(invalid)); + + assertThat(thrown).hasMessageThat().contains("Duration is not valid"); + } + + @Test + public void validateWellKnownProto_invalidTimestamp_throws() { + Timestamp invalid = Timestamp.newBuilder().setSeconds(TIMESTAMP_SECONDS_MAX + 1).build(); + + IllegalArgumentException thrown = + assertThrows( + IllegalArgumentException.class, () -> ProtoTimeUtils.validateWellKnownProto(invalid)); + + assertThat(thrown).hasMessageThat().contains("Timestamp is not valid"); + } } diff --git a/common/src/test/java/dev/cel/common/values/ProtoMessageValueProviderTest.java b/common/src/test/java/dev/cel/common/values/ProtoMessageValueProviderTest.java index ed8fc7b2a..106a6508a 100644 --- a/common/src/test/java/dev/cel/common/values/ProtoMessageValueProviderTest.java +++ b/common/src/test/java/dev/cel/common/values/ProtoMessageValueProviderTest.java @@ -21,6 +21,7 @@ import com.google.common.collect.ImmutableMap; import com.google.common.primitives.UnsignedLong; import com.google.testing.junit.testparameterinjector.TestParameterInjector; +import com.google.testing.junit.testparameterinjector.TestParameters; import dev.cel.common.CelDescriptorUtil; import dev.cel.common.CelOptions; import dev.cel.common.internal.CelDescriptorPool; @@ -37,6 +38,7 @@ import org.junit.runner.RunWith; @RunWith(TestParameterInjector.class) +@SuppressWarnings("GoodTime-ApiWithNumericTimeUnit") // Raw seconds/nanos boundary tests public class ProtoMessageValueProviderTest { private static final CelDescriptorPool DESCRIPTOR_POOL = @@ -47,15 +49,14 @@ public class ProtoMessageValueProviderTest { private static final ProtoMessageFactory MESSAGE_FACTORY = DefaultMessageFactory.create(DESCRIPTOR_POOL); private static final DynamicProto DYNAMIC_PROTO = DynamicProto.create(MESSAGE_FACTORY); + private static final ProtoMessageValueProvider VALUE_PROVIDER = + ProtoMessageValueProvider.newInstance(CelOptions.DEFAULT, DYNAMIC_PROTO); @Test public void newValue_createEmptyProtoMessage() { - ProtoMessageValueProvider protoMessageValueProvider = - ProtoMessageValueProvider.newInstance(CelOptions.DEFAULT, DYNAMIC_PROTO); - ProtoMessageValue protoMessageValue = (ProtoMessageValue) - protoMessageValueProvider + VALUE_PROVIDER .newValue(TestAllTypes.getDescriptor().getFullName(), ImmutableMap.of()) .get(); @@ -107,12 +108,9 @@ public void newValue_createProtoMessage_fieldsPopulated() { @Test public void newValue_createProtoMessage_unsignedLongFieldsPopulated() { - ProtoMessageValueProvider protoMessageValueProvider = - ProtoMessageValueProvider.newInstance(CelOptions.DEFAULT, DYNAMIC_PROTO); - ProtoMessageValue protoMessageValue = (ProtoMessageValue) - protoMessageValueProvider + VALUE_PROVIDER .newValue( TestAllTypes.getDescriptor().getFullName(), ImmutableMap.of( @@ -126,12 +124,9 @@ public void newValue_createProtoMessage_unsignedLongFieldsPopulated() { @Test public void newValue_createProtoMessage_wrappersPopulated() { - ProtoMessageValueProvider protoMessageValueProvider = - ProtoMessageValueProvider.newInstance(CelOptions.DEFAULT, DYNAMIC_PROTO); - ProtoMessageValue protoMessageValue = (ProtoMessageValue) - protoMessageValueProvider + VALUE_PROVIDER .newValue( TestAllTypes.getDescriptor().getFullName(), ImmutableMap.of( @@ -166,12 +161,9 @@ public void newValue_createProtoMessage_wrappersPopulated() { @Test public void newValue_createProtoMessage_extensionFieldsPopulated() { - ProtoMessageValueProvider protoMessageValueProvider = - ProtoMessageValueProvider.newInstance(CelOptions.DEFAULT, DYNAMIC_PROTO); - ProtoMessageValue protoMessageValue = (ProtoMessageValue) - protoMessageValueProvider + VALUE_PROVIDER .newValue( TestAllTypes.getDescriptor().getFullName(), ImmutableMap.of("cel.expr.conformance.proto2.int32_ext", 1)) @@ -183,22 +175,16 @@ public void newValue_createProtoMessage_extensionFieldsPopulated() { @Test public void newValue_invalidMessageName_returnsEmpty() { - ProtoMessageValueProvider protoMessageValueProvider = - ProtoMessageValueProvider.newInstance(CelOptions.DEFAULT, DYNAMIC_PROTO); - - assertThat(protoMessageValueProvider.newValue("bogus", ImmutableMap.of())).isEmpty(); + assertThat(VALUE_PROVIDER.newValue("bogus", ImmutableMap.of())).isEmpty(); } @Test public void newValue_invalidField_throws() { - ProtoMessageValueProvider protoMessageValueProvider = - ProtoMessageValueProvider.newInstance(CelOptions.DEFAULT, DYNAMIC_PROTO); - IllegalArgumentException e = assertThrows( IllegalArgumentException.class, () -> - protoMessageValueProvider.newValue( + VALUE_PROVIDER.newValue( TestAllTypes.getDescriptor().getFullName(), ImmutableMap.of("bogus", 1))); assertThat(e) @@ -211,10 +197,8 @@ public void newValue_invalidField_throws() { @Test public void newValue_onCombinedProvider() { CelValueProvider celValueProvider = (structType, fields) -> Optional.empty(); - ProtoMessageValueProvider protoMessageValueProvider = - ProtoMessageValueProvider.newInstance(CelOptions.DEFAULT, DYNAMIC_PROTO); CelValueProvider combinedProvider = - CombinedCelValueProvider.combine(celValueProvider, protoMessageValueProvider); + CombinedCelValueProvider.combine(celValueProvider, VALUE_PROVIDER); ProtoMessageValue protoMessageValue = (ProtoMessageValue) @@ -226,4 +210,53 @@ public void newValue_onCombinedProvider() { assertThat(protoMessageValue.isZeroValue()).isFalse(); assertThat(protoMessageValue.select("single_int32")).isEqualTo(1L); } + + @Test + @TestParameters("{seconds: 315576000000, nanos: 999999999}") + @TestParameters("{seconds: -315576000000, nanos: -999999999}") + @TestParameters("{seconds: -315576000000, nanos: 0}") + @TestParameters("{seconds: 0, nanos: -999999999}") + public void newValue_durationAtBounds(long seconds, long nanos) { + ImmutableMap fields = ImmutableMap.of("seconds", seconds, "nanos", nanos); + + Optional result = VALUE_PROVIDER.newValue("google.protobuf.Duration", fields); + + assertThat(result).hasValue(Duration.ofSeconds(seconds, nanos)); + } + + @Test + @TestParameters("{seconds: 253402300799, nanos: 999999999}") + @TestParameters("{seconds: -62135596800, nanos: 0}") + @TestParameters("{seconds: -62135596800, nanos: 999999999}") + @TestParameters("{seconds: -1, nanos: 999999999}") + public void newValue_timestampAtBounds(long seconds, long nanos) { + ImmutableMap fields = ImmutableMap.of("seconds", seconds, "nanos", nanos); + + Optional result = VALUE_PROVIDER.newValue("google.protobuf.Timestamp", fields); + + assertThat(result).hasValue(Instant.ofEpochSecond(seconds, nanos)); + } + + @Test + @TestParameters("{typeName: 'google.protobuf.Duration', seconds: 315576000001, nanos: 0}") + @TestParameters("{typeName: 'google.protobuf.Duration', seconds: -315576000001, nanos: 0}") + @TestParameters("{typeName: 'google.protobuf.Duration', seconds: 0, nanos: 1000000000}") + @TestParameters("{typeName: 'google.protobuf.Duration', seconds: 0, nanos: -1000000000}") + @TestParameters("{typeName: 'google.protobuf.Duration', seconds: 1, nanos: -1}") + @TestParameters("{typeName: 'google.protobuf.Duration', seconds: -1, nanos: 1}") + @TestParameters("{typeName: 'google.protobuf.Timestamp', seconds: 253402300800, nanos: 0}") + @TestParameters("{typeName: 'google.protobuf.Timestamp', seconds: -62135596801, nanos: 0}") + @TestParameters("{typeName: 'google.protobuf.Timestamp', seconds: 0, nanos: 1000000000}") + @TestParameters("{typeName: 'google.protobuf.Timestamp', seconds: 0, nanos: -1}") + @TestParameters("{typeName: 'google.protobuf.Timestamp', seconds: -1, nanos: -1}") + public void newValue_durationAndTimestamp_outOfRangeThrows( + String typeName, long seconds, long nanos) { + ImmutableMap fields = ImmutableMap.of("seconds", seconds, "nanos", nanos); + + IllegalArgumentException thrown = + assertThrows( + IllegalArgumentException.class, () -> VALUE_PROVIDER.newValue(typeName, fields)); + + assertThat(thrown).hasMessageThat().contains("is not valid"); + } } diff --git a/conformance/src/test/java/dev/cel/conformance/BUILD.bazel b/conformance/src/test/java/dev/cel/conformance/BUILD.bazel index 82bae1afc..8143222fc 100644 --- a/conformance/src/test/java/dev/cel/conformance/BUILD.bazel +++ b/conformance/src/test/java/dev/cel/conformance/BUILD.bazel @@ -137,22 +137,10 @@ _ALL_TESTS = [ "@cel_spec//tests/simple:testdata/wrappers.textproto", ] -_TESTS_TO_SKIP_SPEC_IMPORT = [ - # Skip until proto wrapper validation is updated for spec parity. - "wrappers/duration/nanos_over", - "wrappers/duration/nanos_under", - "wrappers/duration/negative_seconds_positive_nanos", - "wrappers/duration/positive_seconds_negative_nanos", - "wrappers/duration/seconds_over", - "wrappers/duration/seconds_under", - "wrappers/timestamp/nanos_over", - "wrappers/timestamp/nanos_under", - "wrappers/timestamp/negative_seconds_negative_nanos", - "wrappers/timestamp/seconds_over", - "wrappers/timestamp/seconds_under", -] - -_TESTS_TO_SKIP_LEGACY = _TESTS_TO_SKIP_SPEC_IMPORT + [ +_TESTS_TO_SKIP_LEGACY = [ + # Legacy runtime does not validate Duration/Timestamp struct literals. + "wrappers/duration/nanos_over,nanos_under,negative_seconds_positive_nanos,positive_seconds_negative_nanos,seconds_over,seconds_under", + "wrappers/timestamp/nanos_over,nanos_under,negative_seconds_negative_nanos,seconds_over,seconds_under", # Broken test cases which should be supported. # TODO: Support setting / getting enum values out of the defined enum value range. @@ -189,7 +177,7 @@ _TESTS_TO_SKIP_LEGACY = _TESTS_TO_SKIP_SPEC_IMPORT + [ "type_deductions/type_decay", ] -_TESTS_TO_SKIP_PLANNER = _TESTS_TO_SKIP_SPEC_IMPORT + [ +_TESTS_TO_SKIP_PLANNER = [ # TODO: This is actually a user experience degradation. # Not worth fixing until we see a concrete need. "basic/functions/unbound_is_runtime_error", diff --git a/runtime/src/test/resources/wrappers.baseline b/runtime/src/test/resources/wrappers.baseline index d8212059b..67540e2de 100644 --- a/runtime/src/test/resources/wrappers.baseline +++ b/runtime/src/test/resources/wrappers.baseline @@ -195,7 +195,13 @@ bindings: {int32_list=[value: 1 ]} result: true -Source: google.protobuf.Timestamp{ seconds: 253402300800 } +Source: x.single_timestamp +declare x { + value cel.expr.conformance.proto3.TestAllTypes +} =====> -bindings: {} +bindings: {x=single_timestamp { + seconds: 253402300800 +} +} result: +10000-01-01T00:00:00Z diff --git a/testing/src/main/java/dev/cel/testing/BaseInterpreterTest.java b/testing/src/main/java/dev/cel/testing/BaseInterpreterTest.java index bda56a19e..13a3e3e62 100644 --- a/testing/src/main/java/dev/cel/testing/BaseInterpreterTest.java +++ b/testing/src/main/java/dev/cel/testing/BaseInterpreterTest.java @@ -2154,10 +2154,15 @@ public void wrappers() throws Exception { .buildOrThrow()); clearAllDeclarations(); - // Currently allowed, but will be an error - // See https://github.com/google/cel-spec/pull/501 - source = "google.protobuf.Timestamp{ seconds: 253402300800 }"; - runTest(); + declareVariable("x", StructTypeReference.create(TestAllTypes.getDescriptor().getFullName())); + source = "x.single_timestamp"; + runTest( + ImmutableMap.of( + "x", + TestAllTypes.newBuilder() + .setSingleTimestamp( + Timestamp.newBuilder().setSeconds(ProtoTimeUtils.TIMESTAMP_SECONDS_MAX + 1)) + .build())); } @Test