This is an automated email from the ASF dual-hosted git repository. kenhuuu pushed a commit to branch bp-str-reg in repository https://gitbox.apache.org/repos/asf/tinkerpop.git
commit 51d1c3b42710e98cf22856c52c872321768072e2 Author: Ken Hu <[email protected]> AuthorDate: Tue Aug 18 15:24:53 2026 -0700 Restrict GraphBinary traversal strategy deserialization Assisted-by: Codex:gpt-5.6-sol --- CHANGELOG.asciidoc | 2 + docs/src/reference/gremlin-applications.asciidoc | 16 ++ docs/src/upgrade/release-3.7.x.asciidoc | 23 +++ .../process/traversal/TraversalStrategies.java | 20 +++ .../binary/types/TraversalStrategySerializer.java | 6 +- .../gremlin/process/TraversalStrategiesTest.java | 57 +++++++ .../types/TraversalStrategySerializerTest.java | 176 +++++++++++++++++++++ 7 files changed, 299 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 55be83bfc1..fff2b43cf7 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -45,6 +45,8 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima * Fixed a panic in `gremlin-go` `PartitionStrategy` when `ReadPartitions` was left unset. * Fixed `gremlin-python` `ProductiveByStrategy` to pass through the `productiveKeys` argument, which was previously accepted but never serialized to the server. * Deprecated `ProductiveByStrategy` which was introduced as a temporary way to mimic pre-3.5.0 null processing behavior. +* Backported `TraversalStrategy` registration mechanism in `TraversalStrategies` from the 3.8.x line. +* Restricted GraphBinary `TraversalStrategy` deserialization to strategies registered with `registerStrategy()` or `registerStrategies()`. * Fixed `gremlin-python` GraphBinary serialization of `BigInteger`/`BigDecimal` negative boundary values (e.g. `-129`) that raised `OverflowError`. * Fixed `gremlin-go` GraphBinary serialization of zero `BigInteger`/`BigDecimal` values, which were encoded with zero length and rejected by Java servers. diff --git a/docs/src/reference/gremlin-applications.asciidoc b/docs/src/reference/gremlin-applications.asciidoc index 1cc494ad30..76a2ebc40f 100644 --- a/docs/src/reference/gremlin-applications.asciidoc +++ b/docs/src/reference/gremlin-applications.asciidoc @@ -1274,6 +1274,22 @@ It has the MIME type of `application/vnd.graphbinary-v1.0` and the following con |builder |Name of the `TypeSerializerRegistry.Builder` instance to be used to construct the `TypeSerializerRegistry`. |_none_ |========================================================= +The GraphBinary reference implementation in `gremlin-core`, which is used by Gremlin Server, only deserializes a +`TraversalStrategy` when its class is registered with `TraversalStrategies.GlobalCache`. TinkerPop's built-in +strategies are registered by default. Providers must register every custom strategy before GraphBinary input is read, +either as part of a graph or graph computer strategy set with `registerStrategies()`, or individually with +`registerStrategy()`: + +[source,java] +---- +TraversalStrategies.GlobalCache.registerStrategies(MyGraph.class, traversalStrategies); +// or +TraversalStrategies.GlobalCache.registerStrategy(MyStrategy.class); +---- + +The presence of a strategy on the application class path is not sufficient. Registration permits serialized data to +construct the strategy from its configuration, so all strategies should be registered. + As described above, there are multiple ways in which to register serializers for GraphBinary-based serialization. Note that the `ioRegistries` setting is applied first, followed by the `custom` setting. diff --git a/docs/src/upgrade/release-3.7.x.asciidoc b/docs/src/upgrade/release-3.7.x.asciidoc index 38e939150b..90ecfbbf11 100644 --- a/docs/src/upgrade/release-3.7.x.asciidoc +++ b/docs/src/upgrade/release-3.7.x.asciidoc @@ -187,6 +187,29 @@ Applications that store or transmit `InetAddress` values via GraphSON (as a vert Gremlin parameter) must use literal IP address strings going forward. Existing serialized data containing hostname strings will fail to deserialize after upgrading and will need to be migrated to literal IP addresses. +=== Upgrading for Providers + +==== Graph System Providers + +===== GraphBinary Strategy Registry + +Before 3.7.7, GraphBinary deserialization loaded a custom `TraversalStrategy` from the application class path based on +its serialized class name. Starting with 3.7.7, GraphBinary only deserializes strategies registered with +`TraversalStrategies.GlobalCache`. + +Providers that send custom strategies over GraphBinary must register every strategy before requests are deserialized. +Either the existing `registerStrategies()` method or the new `registerStrategy()` method (backported from 3.8.x line) +can be used: + +[source,text] +---- +// Existing mechanism +TraversalStrategies.GlobalCache.registerStrategies(MyGraph.class, traversalStrategies); + +// New mechanism +TraversalStrategies.GlobalCache.registerStrategy(MyStrategy.class); +---- + == TinkerPop 3.7.6 *Release Date: April 1, 2026* diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/TraversalStrategies.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/TraversalStrategies.java index bb07187b14..1eb27aab9d 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/TraversalStrategies.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/TraversalStrategies.java @@ -350,6 +350,26 @@ public interface TraversalStrategies extends Serializable, Cloneable, Iterable<T return Optional.empty(); } + /** + * Looks up a strategy by the fully qualified class name that a serialized traversal carries, without loading + * the named class. Serializers use this when they resolve a strategy name that arrived as bytes, so that the + * only strategies they can construct are those registered in advance by trusted code, by way of + * {@link #registerStrategies(Class, TraversalStrategies)} or {@link #registerStrategy(Class)}. + * <p/> + * A name only resolves when it is the {@link Class#getName()} of the class registered under its simple name, + * so an unregistered class that shares a simple name with a registered one does not resolve. + */ + public static Optional<? extends Class<? extends TraversalStrategy>> getRegisteredStrategyClassByFullName( + final String className) { + if (null == className) return Optional.empty(); + + // a nested class is registered under the simple name, which is the segment after the last '$' + final int start = Math.max(className.lastIndexOf('.'), className.lastIndexOf('$')) + 1; + final Class<? extends TraversalStrategy> clazz = GLOBAL_REGISTRY.get(className.substring(start)); + + return null != clazz && className.equals(clazz.getName()) ? Optional.of(clazz) : Optional.empty(); + } + public static TraversalStrategies getStrategies(final Class graphOrGraphComputerClass) { try { // be sure to load the class so that its static{} traversal strategy registration component is loaded. diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/binary/types/TraversalStrategySerializer.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/binary/types/TraversalStrategySerializer.java index 7790b213ea..5618a76b37 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/binary/types/TraversalStrategySerializer.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/binary/types/TraversalStrategySerializer.java @@ -24,6 +24,7 @@ import org.apache.tinkerpop.gremlin.structure.io.binary.DataType; import org.apache.tinkerpop.gremlin.structure.io.binary.GraphBinaryReader; import org.apache.tinkerpop.gremlin.structure.io.binary.GraphBinaryWriter; import org.apache.tinkerpop.gremlin.process.traversal.Traversal; +import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategies; import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategy; import org.apache.tinkerpop.gremlin.process.traversal.strategy.TraversalStrategyProxy; import org.apache.tinkerpop.gremlin.structure.io.Buffer; @@ -43,7 +44,10 @@ public class TraversalStrategySerializer extends SimpleTypeSerializer<TraversalS @Override protected TraversalStrategy readValue(final Buffer buffer, final GraphBinaryReader context) throws IOException { - final Class<TraversalStrategy> clazz = context.readValue(buffer, Class.class, false); + final String className = context.readValue(buffer, String.class, false); + final Class<? extends TraversalStrategy> clazz = + TraversalStrategies.GlobalCache.getRegisteredStrategyClassByFullName(className). + orElseThrow(() -> new IOException("TraversalStrategy not recognized - " + className)); final Map config = context.readValue(buffer, Map.class, false); return new TraversalStrategyProxy(clazz, new MapConfiguration(config)); diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/TraversalStrategiesTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/TraversalStrategiesTest.java index e7a5b88b84..16189f0e08 100644 --- a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/TraversalStrategiesTest.java +++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/TraversalStrategiesTest.java @@ -56,6 +56,7 @@ import java.util.stream.Collectors; import java.util.stream.Stream; import static org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategies.GlobalCache.getRegisteredStrategyClass; +import static org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategies.GlobalCache.getRegisteredStrategyClassByFullName; import static org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategies.GlobalCache.registerStrategy; import static org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategies.GlobalCache.unregisterStrategy; import static org.junit.Assert.assertEquals; @@ -324,6 +325,62 @@ public class TraversalStrategiesTest { } } + @Test + public void shouldResolveRegisteredStrategyByFullName() { + assertEquals(ReadOnlyStrategy.class, + getRegisteredStrategyClassByFullName(ReadOnlyStrategy.class.getName()).get()); + assertEquals(RemoteStrategy.class, + getRegisteredStrategyClassByFullName(RemoteStrategy.class.getName()).get()); + assertEquals(RequirementsStrategy.class, + getRegisteredStrategyClassByFullName(RequirementsStrategy.class.getName()).get()); + assertEquals(SackStrategy.class, + getRegisteredStrategyClassByFullName(SackStrategy.class.getName()).get()); + assertEquals(SideEffectStrategy.class, + getRegisteredStrategyClassByFullName(SideEffectStrategy.class.getName()).get()); + } + + @Test + public void shouldResolveNestedRegisteredStrategyByFullName() { + // StrategyA is nested, so it is registered under the segment of its name that follows the '$' + assertEquals(StrategyA.class, + getRegisteredStrategyClassByFullName(StrategyA.class.getName()).get()); + } + + @Test + public void shouldNotResolveUnregisteredStrategyByFullName() { + unregisterStrategy(AbsentStrategy.class); + assertFalse(getRegisteredStrategyClassByFullName(AbsentStrategy.class.getName()).isPresent()); + } + + @Test + public void shouldNotResolveStrategySharingASimpleNameWithARegisteredOne() { + // borrowing the simple name of a registered strategy must not admit some other class of that name + assertFalse(getRegisteredStrategyClassByFullName("com.example.ReadOnlyStrategy").isPresent()); + } + + @Test + public void shouldNotResolveSimpleNameByFullName() { + assertFalse(getRegisteredStrategyClassByFullName(ReadOnlyStrategy.class.getSimpleName()).isPresent()); + } + + @Test + public void shouldNotResolveNullByFullName() { + assertFalse(getRegisteredStrategyClassByFullName(null).isPresent()); + } + + @Test + public void shouldResolveStrategyByFullNameAfterItIsRegistered() { + unregisterStrategy(AbsentStrategy.class); + + try { + registerStrategy(AbsentStrategy.class); + assertEquals(AbsentStrategy.class, + getRegisteredStrategyClassByFullName(AbsentStrategy.class.getName()).get()); + } finally { + unregisterStrategy(AbsentStrategy.class); + } + } + /** * Tests that {@link org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategies#sortStrategies(java.util.Set)} * works as advertised. This class defines a bunch of dummy strategies which define an order. It is verified diff --git a/gremlin-util/src/test/java/org/apache/tinkerpop/gremlin/util/ser/binary/types/TraversalStrategySerializerTest.java b/gremlin-util/src/test/java/org/apache/tinkerpop/gremlin/util/ser/binary/types/TraversalStrategySerializerTest.java new file mode 100644 index 0000000000..cfe08fec8f --- /dev/null +++ b/gremlin-util/src/test/java/org/apache/tinkerpop/gremlin/util/ser/binary/types/TraversalStrategySerializerTest.java @@ -0,0 +1,176 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.tinkerpop.gremlin.util.ser.binary.types; + +import io.netty.buffer.ByteBufAllocator; +import org.apache.tinkerpop.gremlin.process.traversal.Traversal; +import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategies; +import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategy; +import org.apache.tinkerpop.gremlin.process.traversal.strategy.AbstractTraversalStrategy; +import org.apache.tinkerpop.gremlin.process.traversal.strategy.TraversalStrategyProxy; +import org.apache.tinkerpop.gremlin.process.traversal.strategy.decoration.SubgraphStrategy; +import org.apache.tinkerpop.gremlin.process.traversal.strategy.verification.ReadOnlyStrategy; +import org.apache.tinkerpop.gremlin.structure.io.Buffer; +import org.apache.tinkerpop.gremlin.structure.io.binary.DataType; +import org.apache.tinkerpop.gremlin.structure.io.binary.GraphBinaryReader; +import org.apache.tinkerpop.gremlin.structure.io.binary.GraphBinaryWriter; +import org.apache.tinkerpop.gremlin.structure.io.binary.TypeSerializerRegistry; +import org.apache.tinkerpop.gremlin.structure.io.binary.types.SimpleTypeSerializer; +import org.apache.tinkerpop.gremlin.util.ser.NettyBufferFactory; +import org.junit.After; +import org.junit.Test; + +import java.io.IOException; +import java.util.Collections; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.core.StringContains.containsString; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.fail; + +public class TraversalStrategySerializerTest { + + private static final NettyBufferFactory bufferFactory = new NettyBufferFactory(); + private static boolean loadRecordingStrategyInitialized; + private final ByteBufAllocator allocator = ByteBufAllocator.DEFAULT; + + @After + public void unregisterStrategy() { + TraversalStrategies.GlobalCache.unregisterStrategy(LoadRecordingStrategy.class); + } + + @Test + public void shouldRejectStrategyThatIsNotRegistered() throws Exception { + final String fqcn = LoadRecordingStrategy.class.getName(); + try { + readStrategy(reader(), fqcn); + fail("A strategy that is not registered must not deserialize"); + } catch (IOException ex) { + assertThat(ex.getMessage(), containsString("TraversalStrategy not recognized - " + fqcn)); + } + } + + @Test + public void shouldRejectStrategyThatIsNotRegisteredWithoutInitializingIt() throws Exception { + // a class literal does not initialize the class, so naming it this way keeps the assertion below meaningful + final String fqcn = LoadRecordingStrategy.class.getName(); + try { + readStrategy(reader(), fqcn); + fail("A strategy that is not registered must not deserialize"); + } catch (IOException ignored) { + // asserted on by shouldRejectStrategyThatIsNotRegistered + } + + assertFalse("The rejected strategy was initialized, so the check ran after the class was loaded", + loadRecordingStrategyInitialized); + } + + @Test + public void shouldAdmitStrategyRegisteredAsABuiltIn() throws Exception { + assertEquals(SubgraphStrategy.class, + readStrategy(reader(), SubgraphStrategy.class.getName()).getStrategyClass()); + } + + @Test + public void shouldAdmitStrategyRegisteredByAProvider() throws Exception { + TraversalStrategies.GlobalCache.registerStrategy(LoadRecordingStrategy.class); + + assertEquals(LoadRecordingStrategy.class, + readStrategy(reader(), LoadRecordingStrategy.class.getName()).getStrategyClass()); + } + + /** + * Reading the class as a name rather than as a {@code Class} value must not change the format, since the value of a + * {@code Class} is the class name written as a {@code String} value. Reading back what the writer produced the way + * a reader before this change did shows that the two agree. + */ + @Test + public void shouldWriteTheStrategyClassInTheClassValueFormat() throws Exception { + final Buffer buffer = bufferFactory.create(allocator.buffer()); + new GraphBinaryWriter().writeValue(ReadOnlyStrategy.instance(), buffer, false); + + assertEquals(ReadOnlyStrategy.class, new GraphBinaryReader().readValue(buffer, Class.class, false)); + } + + /** + * The strategy class no longer reaches the {@code ClassSerializer}, which matters because that serializer resolves + * whatever name it is given. A registry whose {@code Class} serializer refuses to read anything still reads a + * strategy. + */ + @Test + public void shouldNotReadTheStrategyClassThroughTheClassSerializer() throws Exception { + final GraphBinaryReader reader = new GraphBinaryReader(TypeSerializerRegistry.build(). + add(Class.class, new RefusingClassSerializer()).create()); + + assertEquals(SubgraphStrategy.class, readStrategy(reader, SubgraphStrategy.class.getName()).getStrategyClass()); + } + + private GraphBinaryReader reader() { + return new GraphBinaryReader(TypeSerializerRegistry.build().create()); + } + + /** + * Writes the value of a {@code TraversalStrategy} as a name followed by an empty configuration, which is what a + * client sends for a strategy that takes no configuration. + */ + private TraversalStrategyProxy readStrategy(final GraphBinaryReader reader, final String fqcn) throws IOException { + final GraphBinaryWriter writer = new GraphBinaryWriter(); + final Buffer buffer = bufferFactory.create(allocator.buffer()); + writer.writeValue(fqcn, buffer, false); + writer.writeValue(Collections.emptyMap(), buffer, false); + + return (TraversalStrategyProxy) reader.readValue(buffer, TraversalStrategy.class, false); + } + + private static final class LoadRecordingStrategy + extends AbstractTraversalStrategy<TraversalStrategy.DecorationStrategy> + implements TraversalStrategy.DecorationStrategy { + + static { + loadRecordingStrategyInitialized = true; + } + + @Override + public void apply(final Traversal.Admin<?, ?> traversal) { + // do nothing + } + } + + /** + * Stands in for the {@code ClassSerializer} to show that nothing consults it while a strategy is read. + */ + private static class RefusingClassSerializer extends SimpleTypeSerializer<Class> { + + RefusingClassSerializer() { + super(DataType.CLASS); + } + + @Override + protected Class readValue(final Buffer buffer, final GraphBinaryReader context) throws IOException { + throw new IOException("the Class serializer must not be consulted"); + } + + @Override + protected void writeValue(final Class value, final Buffer buffer, + final GraphBinaryWriter context) throws IOException { + throw new IOException("the Class serializer must not be consulted"); + } + } +}
