This is an automated email from the ASF dual-hosted git repository. spmallette pushed a commit to branch TINKERPOP-3278-followup in repository https://gitbox.apache.org/repos/asf/tinkerpop.git
commit 32bce2c6c136476fa0a14f8c71e8bca0f5d27c80 Author: Stephen Mallette <[email protected]> AuthorDate: Thu Aug 6 13:58:12 2026 -0400 Require registrationRequired(true) when Gryo Java serialization is disabled GryoMapper.build().javaSerializationAllowed(false) only holds while classes must be registered up front, so create() now throws on the combination rather than return a mapper that looks hardened and is not. Also covers the io() reader/writer wiring and gives the GryoReader canary a positive control. Assisted-by: Claude Code:claude-opus-5 --- .../process/traversal/step/sideEffect/IoStep.java | 4 +- .../gremlin/structure/io/gryo/GryoMapper.java | 24 ++++++- .../gremlin/structure/io/gryo/GryoReader.java | 8 +++ .../gremlin/structure/io/gryo/GryoWriter.java | 8 +++ .../structure/io/gryo/GryoIoStepHardeningTest.java | 84 ++++++++++++++++++++++ .../gremlin/structure/io/gryo/GryoMapperTest.java | 84 ++++++++++++++++++++++ 6 files changed, 208 insertions(+), 4 deletions(-) diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/IoStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/IoStep.java index d42d477f88..dbd4c72ded 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/IoStep.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/IoStep.java @@ -141,7 +141,7 @@ public class IoStep<S> extends AbstractStep<S,S> implements ReadWriting { * Builds a {@link GraphReader} instance to use. Attempts to detect the file format to be read using the file * extension or simply uses configurations provided by the user on the parameters given to the step. */ - private GraphReader constructReader() { + protected GraphReader constructReader() { final Object objectOrClass = parameters.get(IO.reader, this::detectFileType).get(0); if (objectOrClass instanceof GraphReader) return (GraphReader) objectOrClass; @@ -176,7 +176,7 @@ public class IoStep<S> extends AbstractStep<S,S> implements ReadWriting { * Builds a {@link GraphWriter} instance to use. Attempts to detect the file format to be write using the file * extension or simply uses configurations provided by the user on the parameters given to the step. */ - private GraphWriter constructWriter() { + protected GraphWriter constructWriter() { final Object objectOrClass = parameters.get(IO.writer, this::detectFileType).get(0); if (objectOrClass instanceof GraphWriter) return (GraphWriter) objectOrClass; diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/gryo/GryoMapper.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/gryo/GryoMapper.java index 502cc52b7d..3c1292dc5a 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/gryo/GryoMapper.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/gryo/GryoMapper.java @@ -286,8 +286,15 @@ public final class GryoMapper implements Mapper<Kryo> { * not need; a stream that carries one now fails with an unregistered class id. Registrations contributed * through an {@link IoRegistry} or {@code addCustom(...)} are covered on the same terms, including those * whose serializer is a {@code Function} or a class default that resolves to a {@code JavaSerializer}. This - * relies on the default {@link #registrationRequired(boolean)} of {@code true}. Callers that need the full - * fidelity for trusted, in-process work should leave this value at {@code true}. + * relies on the default {@link #registrationRequired(boolean)} of {@code true}, and {@link #create()} throws + * rather than build a mapper that combines the two. Callers that need the full fidelity for trusted, + * in-process work should leave this value at {@code true}. + * <p/> + * The hardened defaults reach {@code io()}, {@link GryoReader}, {@link GryoWriter} and {@link GryoIo}, but + * deliberately not {@link GryoPool} or the wire paths, which construct their own full fidelity mapper and + * hand it to every reader and writer they vend. Bytes read through those remain able to reach + * {@code java.io.ObjectInputStream.readObject()}. That is intended: they carry trusted, in-process traffic + * such as OLAP, which needs the {@code TraversalStrategy} types this filter drops. * * @param javaSerializationAllowed set to {@code false} to drop the {@code JavaSerializer} registrations or * {@code true} to keep them @@ -312,8 +319,21 @@ public final class GryoMapper implements Mapper<Kryo> { /** * Creates a {@code GryoMapper}. + * + * @throws IllegalStateException if {@link #javaSerializationAllowed(boolean)} is {@code false} while + * {@link #registrationRequired(boolean)} is also {@code false}, since that + * combination produces a mapper that only appears to be hardened */ public GryoMapper create() { + // dropping the JavaSerializer registrations only holds while registration is required. without it, a + // stream naming the class rather than its id resolves that name and rebuilds the very serializer that + // createMapper() just dropped, so refuse the combination rather than hand back a mapper that looks + // hardened and is not + if (!javaSerializationAllowed && !registrationRequired) + throw new IllegalStateException( + "javaSerializationAllowed(false) requires registrationRequired(true); " + + "class-by-name resolution would reinstate the JavaSerializer"); + // consult the registry if provided and inject registry entries as custom classes. registries.forEach(registry -> { final List<Pair<Class, Object>> serializers = registry.find(GryoIo.class); diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/gryo/GryoReader.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/gryo/GryoReader.java index 09739f53a9..20bb3a06f8 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/gryo/GryoReader.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/gryo/GryoReader.java @@ -72,6 +72,14 @@ public final class GryoReader implements GraphReader { this.batchSize = batchSize; } + /** + * The {@code Kryo} this reader decodes with, exposed so that tests can assert which types a configured reader + * will and will not accept. Not part of the public API. + */ + Kryo getKryo() { + return kryo; + } + /** * Read data into a {@link Graph} from output generated by any of the {@link GryoWriter} {@code writeVertex} or * {@code writeVertices} methods or by {@link GryoWriter#writeGraph(OutputStream, Graph)}. diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/gryo/GryoWriter.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/gryo/GryoWriter.java index 3451e0b270..55d7e49e87 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/gryo/GryoWriter.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/gryo/GryoWriter.java @@ -52,6 +52,14 @@ public final class GryoWriter implements GraphWriter { this.kryo = gryoMapper.createMapper(); } + /** + * The {@code Kryo} this writer encodes with, exposed so that tests can assert which types a configured writer + * will and will not accept. Not part of the public API. + */ + Kryo getKryo() { + return kryo; + } + /** * {@inheritDoc} */ diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/structure/io/gryo/GryoIoStepHardeningTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/structure/io/gryo/GryoIoStepHardeningTest.java new file mode 100644 index 0000000000..1b4ef710fd --- /dev/null +++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/structure/io/gryo/GryoIoStepHardeningTest.java @@ -0,0 +1,84 @@ +/* + * 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.structure.io.gryo; + +import org.apache.tinkerpop.gremlin.process.traversal.IO; +import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__; +import org.apache.tinkerpop.gremlin.process.traversal.step.sideEffect.IoStep; +import org.apache.tinkerpop.gremlin.process.traversal.strategy.decoration.OptionsStrategy; +import org.apache.tinkerpop.gremlin.structure.io.GraphReader; +import org.apache.tinkerpop.gremlin.structure.io.GraphWriter; +import org.apache.tinkerpop.shaded.kryo.Kryo; +import org.junit.Test; + +import java.util.Arrays; + +import static org.junit.Assert.fail; + +/** + * The {@link IO#gryo} branches of {@link IoStep} build their own hardened {@link GryoMapper}, and nothing else in the + * suite covers them - they could be deleted and the build would stay green. An end-to-end {@code read()} cannot fill + * the gap either: {@code readGraph} rejects crafted bytes on the header check before the mapper is consulted, and a + * legitimate graph document never carries one of the dropped types. So this asserts the wiring instead, against the + * {@code Kryo} the step actually hands to its reader and writer. + * <p/> + * It lives in this package rather than beside {@link IoStep} because {@code getKryo()} is package private here. + * + * @author Stephen Mallette (http://stephen.genoprime.com) + */ +public class GryoIoStepHardeningTest { + + @Test + public void shouldBuildHardenedGryoReaderAndWriter() { + final ExposedIoStep step = new ExposedIoStep("graph.kryo"); + + for (final Kryo kryo : Arrays.asList(((GryoReader) step.reader()).getKryo(), + ((GryoWriter) step.writer()).getKryo())) { + try { + kryo.getRegistration(OptionsStrategy.class); + fail("io() must not register a JavaSerializer backed type such as OptionsStrategy"); + } catch (IllegalArgumentException expected) { + // Kryo refuses an unregistered class while registration is required, which is the whole point of + // dropping the registration rather than replacing its serializer + } + } + } + + /** + * {@code constructReader()} and {@code constructWriter()} are protected, which only grants access through a + * reference of the subclass's own type, so the calls are made from inside the subclass rather than on an + * {@code IoStep} typed variable. + */ + private static final class ExposedIoStep extends IoStep<Object> { + + ExposedIoStep(final String file) { + // the .kryo extension is what drives detectFileType() to IO.gryo, and neither construct method reads the + // graph off the traversal, so an anonymous start is enough to hold the step + super(__.start().asAdmin(), file); + } + + GraphReader reader() { + return constructReader(); + } + + GraphWriter writer() { + return constructWriter(); + } + } +} diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/structure/io/gryo/GryoMapperTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/structure/io/gryo/GryoMapperTest.java index 5de7fa31ec..8d0236e137 100644 --- a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/structure/io/gryo/GryoMapperTest.java +++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/structure/io/gryo/GryoMapperTest.java @@ -81,8 +81,10 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.greaterThan; import static org.hamcrest.core.Is.is; import static org.hamcrest.core.IsInstanceOf.instanceOf; +import static org.hamcrest.Matchers.containsString; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertNotSame; @@ -461,6 +463,31 @@ public class GryoMapperTest { DeserializationCanary.FIRED); } + /** + * Positive control for {@link #shouldNotInvokeJavaDeserializationOnGryoReaderRead()}. That canary needs one more + * than the others do: {@link GryoReader#readObject(InputStream, Class)} does not call {@code readHeader}, and the + * crafted bytes carry no header, so nothing else in the read path would object to them. Point the same reader at + * a full fidelity mapper and the bytes must reach {@code ObjectInputStream.readObject()}, which is what shows the + * assertion over there is answering the hardening rather than some unrelated rejection. + */ + @Test + public void shouldInvokeJavaDeserializationOnDefaultMapperGryoReaderRead() throws Exception { + final GryoReader reader = GryoReader.build(). + mapper(builder.get().create()). // full fidelity on purpose + create(); + + DeserializationCanary.FIRED = false; + try (final InputStream stream = new ByteArrayInputStream(maliciousGryoBytes())) { + reader.readObject(stream, Object.class); + } catch (Exception ignored) { + // as in the mapper level control, a failure is possible but only after readObject() has already run + } + + assertTrue("the crafted stream must reach ObjectInputStream.readObject() through a full fidelity " + + "GryoReader, otherwise shouldNotInvokeJavaDeserializationOnGryoReaderRead proves nothing", + DeserializationCanary.FIRED); + } + /** * Hardening the mapper must not cost anything on the graph structure that a Gryo document actually carries. */ @@ -575,6 +602,63 @@ public class GryoMapperTest { } } + /** + * Dropping the {@code JavaSerializer} registrations only holds while registration is required. Without it a + * stream may name the class instead of presenting its id, and Kryo resolves that name to a fresh registration + * carrying the very serializer that was dropped. The combination is refused rather than silently corrected, so + * that whoever wrote it hears about it instead of holding a mapper that only appears to be hardened. + */ + @Test + public void shouldNotAllowJavaSerializationDisabledWithoutRegistrationRequired() { + try { + builder.get().javaSerializationAllowed(false).registrationRequired(false).create(); + fail("javaSerializationAllowed(false) with registrationRequired(false) must not build a mapper"); + } catch (IllegalStateException expected) { + assertThat(expected.getMessage(), containsString("requires registrationRequired(true)")); + } + } + + /** + * The order in which the flags are set must not matter, since a builder is free to be configured either way. + */ + @Test + public void shouldNotAllowRegistrationRequiredDisabledBeforeJavaSerialization() { + try { + builder.get().registrationRequired(false).javaSerializationAllowed(false).create(); + fail("the illegal combination must be refused regardless of the order the flags were set in"); + } catch (IllegalStateException expected) { + assertThat(expected.getMessage(), containsString("requires registrationRequired(true)")); + } + } + + /** + * The combination is easiest to reach by accident through {@code GryoIo}, whose {@code onMapper} consumer runs + * after the hardening has been applied and so can undo it. That path must be refused on the same terms. + */ + @Test + public void shouldNotAllowGryoIoOnMapperToDropRegistrationRequired() { + final Io.Builder<GryoIo> ioBuilder = GryoIo.build(name.equals("1_0") ? GryoVersion.V1_0 : GryoVersion.V3_0); + ioBuilder.graph(EmptyGraph.instance()); + ioBuilder.onMapper(m -> ((GryoMapper.Builder) m).registrationRequired(false)); + + try { + ioBuilder.create().reader(); + fail("an onMapper consumer must not be able to undo the io() hardening"); + } catch (IllegalStateException expected) { + assertThat(expected.getMessage(), containsString("requires registrationRequired(true)")); + } + } + + /** + * Neither flag on its own is a problem: the full fidelity mapper may drop the registration requirement, and the + * hardened mapper keeps it by default. + */ + @Test + public void shouldAllowRegistrationRequiredDisabledOnFullFidelityMapper() { + assertNotNull(builder.get().registrationRequired(false).create()); + assertNotNull(builder.get().javaSerializationAllowed(false).create()); + } + /** * The full fidelity mapper is unchanged and remains available for trusted, in-process round-trips. This test * documents which registrations that leaves on native Java serialization.
