This is an automated email from the ASF dual-hosted git repository. kenhuuu pushed a commit to branch 3.7-dev in repository https://gitbox.apache.org/repos/asf/tinkerpop.git
commit cff2a1339b3de30bc62aa10d7065b75c4306ead8 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 | 1 + docs/src/reference/gremlin-applications.asciidoc | 17 ++ docs/src/upgrade/release-3.7.x.asciidoc | 10 + .../process/traversal/TraversalStrategies.java | 20 ++ .../binary/types/TraversalStrategySerializer.java | 6 +- .../gremlin/process/TraversalStrategiesTest.java | 54 ++++++ .../binary/GraphBinaryMessageSerializerV1Test.java | 30 +++ .../types/TraversalStrategySerializerTest.java | 201 +++++++++++++++++++++ 8 files changed, 338 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 637c0306a1..7303ca3939 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -47,6 +47,7 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima * 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 registered strategies that have not been denied with `denyStrategy()`. * 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..cc80ae32e9 100644 --- a/docs/src/reference/gremlin-applications.asciidoc +++ b/docs/src/reference/gremlin-applications.asciidoc @@ -1274,6 +1274,23 @@ 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 in +<<traversalstrategy-registration,TraversalStrategy Registration>>. + 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 fcb76221b7..93cd5fd283 100644 --- a/docs/src/upgrade/release-3.7.x.asciidoc +++ b/docs/src/upgrade/release-3.7.x.asciidoc @@ -215,6 +215,16 @@ TraversalStrategies.GlobalCache.denyStrategy(MyStrategy.class); See: link:https://tinkerpop.apache.org/docs/3.7.7/reference/#traversalstrategy[TraversalStrategy Reference] +===== GraphBinary Strategy Deserialization + +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 strategy classes registered with +`TraversalStrategies.GlobalCache` and rejects denied strategies. + +Providers that send custom strategies over GraphBinary must register every strategy before requests are deserialized +using `registerStrategies()` or `registerStrategy()`, as described above. The presence of a strategy on the application +class path is no longer sufficient. + == 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 1f41c86696..a8ce6b498c 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 @@ -374,6 +374,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 91b6b0286d..1311fef017 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 @@ -55,6 +55,7 @@ import java.util.stream.Stream; import static org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategies.GlobalCache.denyStrategy; 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; @@ -235,6 +236,7 @@ public class TraversalStrategiesTest { registerStrategy(DeniedStrategy.class); assertFalse(getRegisteredStrategyClass(DeniedStrategy.class.getSimpleName()).isPresent()); + assertFalse(getRegisteredStrategyClassByFullName(DeniedStrategy.class.getName()).isPresent()); } @Test @@ -374,6 +376,58 @@ public class TraversalStrategiesTest { public static class DeniedTestGraphComputer extends TestGraphComputer { } + @Test + public void shouldResolveRegisteredStrategyByFullName() { + assertEquals(OptionsStrategy.class, + getRegisteredStrategyClassByFullName(OptionsStrategy.class.getName()).get()); + assertEquals(MatchAlgorithmStrategy.class, + getRegisteredStrategyClassByFullName(MatchAlgorithmStrategy.class.getName()).get()); + assertEquals(ReadOnlyStrategy.class, + getRegisteredStrategyClassByFullName(ReadOnlyStrategy.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/GraphBinaryMessageSerializerV1Test.java b/gremlin-util/src/test/java/org/apache/tinkerpop/gremlin/util/ser/binary/GraphBinaryMessageSerializerV1Test.java index b1e42d56ae..86e9da7947 100644 --- a/gremlin-util/src/test/java/org/apache/tinkerpop/gremlin/util/ser/binary/GraphBinaryMessageSerializerV1Test.java +++ b/gremlin-util/src/test/java/org/apache/tinkerpop/gremlin/util/ser/binary/GraphBinaryMessageSerializerV1Test.java @@ -21,7 +21,9 @@ package org.apache.tinkerpop.gremlin.util.ser.binary; import io.netty.buffer.ByteBuf; import io.netty.buffer.ByteBufAllocator; import org.apache.tinkerpop.gremlin.process.traversal.Traversal; +import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategy; import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource; +import org.apache.tinkerpop.gremlin.process.traversal.strategy.AbstractTraversalStrategy; import org.apache.tinkerpop.gremlin.util.Tokens; import org.apache.tinkerpop.gremlin.util.message.RequestMessage; import org.apache.tinkerpop.gremlin.util.message.ResponseMessage; @@ -43,6 +45,8 @@ import static org.apache.tinkerpop.gremlin.util.MockitoHamcrestMatcherAdapter.re import static org.junit.Assert.assertEquals; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.hasItemInArray; +import static org.hamcrest.core.StringContains.containsString; +import static org.junit.Assert.fail; public class GraphBinaryMessageSerializerV1Test { private final ByteBufAllocator allocator = ByteBufAllocator.DEFAULT; @@ -129,6 +133,22 @@ public class GraphBinaryMessageSerializerV1Test { assertResponseEquals(response, deserialized); } + @Test + public void shouldRejectUnregisteredTraversalStrategyInResponse() throws SerializationException { + final ResponseMessage response = ResponseMessage.build(UUID.randomUUID()) + .result(new UnregisteredStrategy()) + .create(); + final ByteBuf buffer = serializer.serializeResponseAsBinary(response, allocator); + + try { + serializer.deserializeResponse(buffer); + fail("An unregistered strategy in a server response must not deserialize"); + } catch (SerializationException ex) { + assertThat(ex.getMessage(), containsString( + "TraversalStrategy not recognized - " + UnregisteredStrategy.class.getName())); + } + } + @Test public void shouldSerializeAndDeserializeResponseOverText() throws SerializationException { final ResponseMessage response = ResponseMessage.build(UUID.randomUUID()) @@ -276,4 +296,14 @@ public class GraphBinaryMessageSerializerV1Test { return super.create(); } } + + private static final class UnregisteredStrategy + extends AbstractTraversalStrategy<TraversalStrategy.DecorationStrategy> + implements TraversalStrategy.DecorationStrategy { + + @Override + public void apply(final Traversal.Admin<?, ?> traversal) { + // do nothing + } + } } 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..1c70494000 --- /dev/null +++ b/gremlin-util/src/test/java/org/apache/tinkerpop/gremlin/util/ser/binary/types/TraversalStrategySerializerTest.java @@ -0,0 +1,201 @@ +/* + * 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()); + } + + @Test + public void shouldRejectDeniedStrategyAfterRegistrationAttempt() throws Exception { + TraversalStrategies.GlobalCache.registerStrategy(DeniedStrategy.class); + TraversalStrategies.GlobalCache.denyStrategy(DeniedStrategy.class); + TraversalStrategies.GlobalCache.registerStrategy(DeniedStrategy.class); + + final String fqcn = DeniedStrategy.class.getName(); + try { + readStrategy(reader(), fqcn); + fail("A denied strategy must not deserialize"); + } catch (IOException ex) { + assertThat(ex.getMessage(), containsString("TraversalStrategy not recognized - " + fqcn)); + } + } + + /** + * 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 + } + } + + private static final class DeniedStrategy + extends AbstractTraversalStrategy<TraversalStrategy.DecorationStrategy> + implements TraversalStrategy.DecorationStrategy { + + @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"); + } + } +}
