Cole-Greer commented on code in PR #3402:
URL: https://github.com/apache/tinkerpop/pull/3402#discussion_r3184969517
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/language/grammar/GremlinQueryParser.java:
##########
@@ -91,4 +88,94 @@ public static Object parse(final String query, final
GremlinVisitor<Object> visi
throw new GremlinParserException("Failed to interpret Gremlin
query: " + ex.getMessage(), ex);
}
}
+
+ /**
+ * Parses a gremlin-lang map literal string into a {@code Map<String,
Object>} for use as parameters.
+ * <p>
+ * Uses {@link ParameterMapVisitor} to prevent traversal injection and
validates that all keys are strings
+ * and no values contain traversals.
+ *
+ * @param parameterMapString the gremlin-lang map literal string (e.g.
{@code [x:1,y:"marko"]}) or {@code null}/empty
+ * @return the parsed and validated parameter map
+ * @throws GremlinParserException if parsing fails or validation detects
invalid content
+ */
+ public static Map<String, Object> parseParameters(final String
parameterMapString) {
+ if (parameterMapString == null || parameterMapString.isEmpty()) {
+ return Map.of();
+ }
+
+ final GremlinParser parser = createParser(parameterMapString);
+ final GremlinParser.GenericMapLiteralContext mapCtx =
parser.genericMapLiteral();
+
+ final ParameterMapVisitor visitor = new ParameterMapVisitor(new
GremlinAntlrToJava());
+ final Map<Object, Object> rawMap = (Map<Object, Object>)
visitor.visitGenericMapLiteral(mapCtx);
+
+ if (rawMap == null) {
+ return Map.of();
+ }
+
+ for (final Map.Entry<?, ?> entry : rawMap.entrySet()) {
+ if (!(entry.getKey() instanceof String)) {
+ throw new GremlinParserException(
+ String.format("Parameter map keys must be String,
found: %s",
+ entry.getKey() == null ? "null" :
entry.getKey().getClass().getSimpleName()));
+ }
+ final String key = (String) entry.getKey();
+ if (!SourceVersion.isIdentifier(key)) {
+ throw new GremlinParserException(
+ String.format("Parameter map key must be a valid
identifier: %s", key));
+ }
+ validateParameterValue(entry.getValue());
+ }
+
+ return (Map<String, Object>) (Map<?, ?>) rawMap;
+ }
+
+ /**
+ * Recursively validates that a parameter value does not contain a {@link
Traversal}. Nested validation is needed
+ * because steps like mergeV iterate map values, so a Traversal hiding
inside a nested map or collection would still
+ * be dangerous.
+ */
+ private static void validateParameterValue(final Object value) {
+ if (value instanceof Traversal) {
+ throw new GremlinParserException("Traversals are not allowed as
parameter values");
+ }
+ if (value instanceof Map) {
Review Comment:
Should we also recurse through map keys?
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/language/grammar/GremlinQueryParser.java:
##########
@@ -91,4 +88,94 @@ public static Object parse(final String query, final
GremlinVisitor<Object> visi
throw new GremlinParserException("Failed to interpret Gremlin
query: " + ex.getMessage(), ex);
}
}
+
+ /**
+ * Parses a gremlin-lang map literal string into a {@code Map<String,
Object>} for use as parameters.
+ * <p>
+ * Uses {@link ParameterMapVisitor} to prevent traversal injection and
validates that all keys are strings
+ * and no values contain traversals.
+ *
+ * @param parameterMapString the gremlin-lang map literal string (e.g.
{@code [x:1,y:"marko"]}) or {@code null}/empty
+ * @return the parsed and validated parameter map
+ * @throws GremlinParserException if parsing fails or validation detects
invalid content
+ */
+ public static Map<String, Object> parseParameters(final String
parameterMapString) {
+ if (parameterMapString == null || parameterMapString.isEmpty()) {
+ return Map.of();
+ }
+
+ final GremlinParser parser = createParser(parameterMapString);
+ final GremlinParser.GenericMapLiteralContext mapCtx =
parser.genericMapLiteral();
+
+ final ParameterMapVisitor visitor = new ParameterMapVisitor(new
GremlinAntlrToJava());
+ final Map<Object, Object> rawMap = (Map<Object, Object>)
visitor.visitGenericMapLiteral(mapCtx);
Review Comment:
Do we need some error handling here? Am I right in assuming `mapCtx` will be
null if the `parameterMapString` is not actually a gremlin-lang map? How would
such an error propagate if the user provides a bad parameter string?
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/language/grammar/ParameterMapVisitor.java:
##########
@@ -0,0 +1,102 @@
+/*
+ * 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.language.grammar;
+
+/**
+ * A visitor for parsing parameter map strings that prevents traversal
injection.
+ * <p>
+ * Extends {@link GenericLiteralVisitor} and overrides traversal-related visit
methods
+ * to throw, ensuring that no traversal can be constructed or executed during
the
+ * parameter map parsing walk. This is critical for security because
+ * {@code visitTerminatedTraversal} in the base class would execute the
traversal
+ * immediately via {@link TraversalTerminalMethodVisitor}.
+ */
+public class ParameterMapVisitor extends GenericLiteralVisitor {
+
+ private static final int DEFAULT_MAX_NESTING_DEPTH = 32;
+
+ private final int maxNestingDepth;
+ private int currentNestingDepth = 0;
+
+ public ParameterMapVisitor(final GremlinAntlrToJava antlr) {
+ this(antlr, DEFAULT_MAX_NESTING_DEPTH);
+ }
+
+ public ParameterMapVisitor(final GremlinAntlrToJava antlr, final int
maxNestingDepth) {
+ super(antlr);
+ this.maxNestingDepth = maxNestingDepth;
+ }
+
+ /**
+ * Overridden to prevent nested traversal construction in parameter maps.
+ */
+ @Override
+ public Object visitNestedTraversal(final
GremlinParser.NestedTraversalContext ctx) {
+ throw new GremlinParserException("Traversals are not allowed in
parameter maps");
+ }
+
+ /**
+ * Overridden to prevent terminated traversal execution in parameter maps.
+ * This is the critical override because the base class would execute the
traversal
+ * immediately via {@link TraversalTerminalMethodVisitor}.
+ */
+ @Override
+ public Object visitTerminatedTraversal(final
GremlinParser.TerminatedTraversalContext ctx) {
+ throw new GremlinParserException("Traversals are not allowed in
parameter maps");
+ }
+
+ @Override
+ public Object visitGenericMapLiteral(final
GremlinParser.GenericMapLiteralContext ctx) {
+ currentNestingDepth++;
+ if (currentNestingDepth > maxNestingDepth) {
+ throw new GremlinParserException("Parameter map nesting depth
exceeds maximum of " + maxNestingDepth);
+ }
+ try {
+ return super.visitGenericMapLiteral(ctx);
+ } finally {
+ currentNestingDepth--;
+ }
+ }
+
+ @Override
+ public Object visitGenericCollectionLiteral(final
GremlinParser.GenericCollectionLiteralContext ctx) {
+ currentNestingDepth++;
+ if (currentNestingDepth > maxNestingDepth) {
+ throw new GremlinParserException("Parameter map nesting depth
exceeds maximum of " + maxNestingDepth);
+ }
+ try {
+ return super.visitGenericCollectionLiteral(ctx);
Review Comment:
Am I right in understanding that this will result in all objects contained
in the collection to be parsed by `GenericLiteralVisitor.visitGenericLiteral()`
instead of `ParameterMapVisitor.visitGenericLiteral()`? Do we need more careful
handling of collections and any composite types to ensure that they are
recursively parsed through this class and not handed off to the unguarded
`GenericLiteralVisitor`?
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/language/grammar/GremlinQueryParser.java:
##########
@@ -91,4 +88,94 @@ public static Object parse(final String query, final
GremlinVisitor<Object> visi
throw new GremlinParserException("Failed to interpret Gremlin
query: " + ex.getMessage(), ex);
}
}
+
+ /**
+ * Parses a gremlin-lang map literal string into a {@code Map<String,
Object>} for use as parameters.
+ * <p>
+ * Uses {@link ParameterMapVisitor} to prevent traversal injection and
validates that all keys are strings
+ * and no values contain traversals.
+ *
+ * @param parameterMapString the gremlin-lang map literal string (e.g.
{@code [x:1,y:"marko"]}) or {@code null}/empty
+ * @return the parsed and validated parameter map
+ * @throws GremlinParserException if parsing fails or validation detects
invalid content
+ */
+ public static Map<String, Object> parseParameters(final String
parameterMapString) {
+ if (parameterMapString == null || parameterMapString.isEmpty()) {
+ return Map.of();
+ }
+
+ final GremlinParser parser = createParser(parameterMapString);
+ final GremlinParser.GenericMapLiteralContext mapCtx =
parser.genericMapLiteral();
+
+ final ParameterMapVisitor visitor = new ParameterMapVisitor(new
GremlinAntlrToJava());
+ final Map<Object, Object> rawMap = (Map<Object, Object>)
visitor.visitGenericMapLiteral(mapCtx);
+
+ if (rawMap == null) {
+ return Map.of();
+ }
+
+ for (final Map.Entry<?, ?> entry : rawMap.entrySet()) {
+ if (!(entry.getKey() instanceof String)) {
+ throw new GremlinParserException(
+ String.format("Parameter map keys must be String,
found: %s",
+ entry.getKey() == null ? "null" :
entry.getKey().getClass().getSimpleName()));
+ }
+ final String key = (String) entry.getKey();
+ if (!SourceVersion.isIdentifier(key)) {
+ throw new GremlinParserException(
+ String.format("Parameter map key must be a valid
identifier: %s", key));
+ }
+ validateParameterValue(entry.getValue());
+ }
+
+ return (Map<String, Object>) (Map<?, ?>) rawMap;
+ }
+
+ /**
+ * Recursively validates that a parameter value does not contain a {@link
Traversal}. Nested validation is needed
+ * because steps like mergeV iterate map values, so a Traversal hiding
inside a nested map or collection would still
+ * be dangerous.
+ */
+ private static void validateParameterValue(final Object value) {
+ if (value instanceof Traversal) {
+ throw new GremlinParserException("Traversals are not allowed as
parameter values");
Review Comment:
I think this should be kept out of scope from this work, but I'm curious if
there may be demand to parameterize something like `CardinalityValueTraversal`
in the future. That's a funky case which generally acts as a static value but
is technically a Traversal.
##########
gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/language/grammar/ParameterMapVisitorTest.java:
##########
@@ -0,0 +1,220 @@
+/*
+ * 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.language.grammar;
+
+import org.junit.Test;
+
+import java.util.List;
+import java.util.Map;
+import java.util.UUID;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
+
+/**
+ * Tests for parameter map parsing, validation, and security.
+ */
+public class ParameterMapVisitorTest {
+
+ @Test
+ public void shouldParseEmptyMap() {
+ final Map<String, Object> result =
GremlinQueryParser.parseParameters("[:]");
+ assertNotNull(result);
+ assertTrue(result.isEmpty());
+ }
+
+ @Test
+ public void shouldParseNullInput() {
+ final Map<String, Object> result =
GremlinQueryParser.parseParameters(null);
+ assertNotNull(result);
+ assertTrue(result.isEmpty());
+ }
+
+ @Test
+ public void shouldParseEmptyStringInput() {
+ final Map<String, Object> result =
GremlinQueryParser.parseParameters("");
+ assertNotNull(result);
+ assertTrue(result.isEmpty());
+ }
+
+ @Test
+ public void shouldParseSingleIntegerParameter() {
+ final Map<String, Object> result =
GremlinQueryParser.parseParameters("[\"x\":1]");
+ assertEquals(1, result.size());
+ assertEquals(1, result.get("x"));
+ }
+
+ @Test
+ public void shouldParseSingleStringParameter() {
+ final Map<String, Object> result =
GremlinQueryParser.parseParameters("[\"name\":\"marko\"]");
+ assertEquals(1, result.size());
+ assertEquals("marko", result.get("name"));
+ }
+
+ @Test
+ public void shouldParseSingleLongParameter() {
+ final Map<String, Object> result =
GremlinQueryParser.parseParameters("[\"x\":1L]");
+ assertEquals(1, result.size());
+ assertEquals(1L, result.get("x"));
+ }
+
+ @Test
+ public void shouldParseMultipleMixedParameters() {
+ final Map<String, Object> result =
GremlinQueryParser.parseParameters("[\"x\":1,\"name\":\"marko\",\"flag\":true]");
+ assertEquals(3, result.size());
+ assertEquals(1, result.get("x"));
+ assertEquals("marko", result.get("name"));
+ assertEquals(true, result.get("flag"));
+ }
+
+ @Test
+ public void shouldParseNullValue() {
+ final Map<String, Object> result =
GremlinQueryParser.parseParameters("[\"x\":null]");
+ assertEquals(1, result.size());
+ assertNull(result.get("x"));
+ }
+
+ @Test
+ public void shouldParseUuidValue() {
+ final Map<String, Object> result =
GremlinQueryParser.parseParameters("[\"id\":UUID(\"bfa9bbe8-c3a3-4017-acc3-cd02dda55e3e\")]");
+ assertEquals(1, result.size());
+ assertEquals(UUID.fromString("bfa9bbe8-c3a3-4017-acc3-cd02dda55e3e"),
result.get("id"));
+ }
+
+ @Test
+ public void shouldParseNestedMapValue() {
+ final Map<String, Object> result =
GremlinQueryParser.parseParameters("[\"m\":[\"name\":\"marko\"]]");
+ assertEquals(1, result.size());
+ assertTrue(result.get("m") instanceof Map);
+ assertEquals("marko", ((Map<?, ?>) result.get("m")).get("name"));
+ }
+
+ @Test
+ public void shouldParseListValue() {
+ final Map<String, Object> result =
GremlinQueryParser.parseParameters("[\"x\":[1,2,3]]");
+ assertEquals(1, result.size());
+ assertTrue(result.get("x") instanceof List);
+ assertEquals(3, ((List<?>) result.get("x")).size());
+ }
+
+ @Test
+ public void shouldParseUnicodeKey() {
+ final Map<String, Object> result =
GremlinQueryParser.parseParameters("[\"caf\\u00E9\":1]");
+ assertEquals(1, result.size());
+ assertEquals(1, result.get("caf\u00e9"));
+ }
+
+ @Test(expected = GremlinParserException.class)
+ public void shouldRejectMalformedInput() {
+ GremlinQueryParser.parseParameters("[\"x\":");
+ }
+
+ @Test(expected = GremlinParserException.class)
+ public void shouldRejectNumericKey() {
+ GremlinQueryParser.parseParameters("[1:\"value\"]");
+ }
+
+ @Test(expected = GremlinParserException.class)
+ public void shouldRejectEnumKey() {
+ GremlinQueryParser.parseParameters("[(T.id):\"value\"]");
+ }
+
+ @Test(expected = GremlinParserException.class)
+ public void shouldRejectNonIdentifierStringKey() {
+ GremlinQueryParser.parseParameters("[\"~id\":1]");
+ }
+
+ @Test(expected = GremlinParserException.class)
+ public void shouldRejectKeyWithSpaces() {
+ GremlinQueryParser.parseParameters("[\"my key\":1]");
+ }
+
+ @Test(expected = GremlinParserException.class)
+ public void shouldRejectEmptyStringKey() {
+ GremlinQueryParser.parseParameters("[\"\":1]");
+ }
+
+ @Test(expected = GremlinParserException.class)
+ public void shouldRejectExcessiveNestingDepth() {
+ final StringBuilder sb = new StringBuilder();
+ for (int i = 0; i < 35; i++) {
+ sb.append("[\"a\":");
+ }
+ sb.append("1");
+ for (int i = 0; i < 35; i++) {
+ sb.append("]");
+ }
+ GremlinQueryParser.parseParameters(sb.toString());
+ }
+
+ @Test(expected = GremlinParserException.class)
+ public void shouldRejectNestedTraversalInValue() {
+ GremlinQueryParser.parseParameters("[\"x\":__.out()]");
+ }
+
+ @Test(expected = GremlinParserException.class)
+ public void shouldRejectTerminatedTraversalInValue() {
+ GremlinQueryParser.parseParameters("[\"x\":g.V().drop().iterate()]");
Review Comment:
I think it might be worth a few more cases which try to bury a
terminatedTraversal deeper inside other collections/composite types.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
