[
https://issues.apache.org/jira/browse/TINKERPOP-3247?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18078325#comment-18078325
]
ASF GitHub Bot commented on TINKERPOP-3247:
-------------------------------------------
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.
> String-Based Parameters
> -----------------------
>
> Key: TINKERPOP-3247
> URL: https://issues.apache.org/jira/browse/TINKERPOP-3247
> Project: TinkerPop
> Issue Type: Improvement
> Components: language, server
> Affects Versions: 4.0.0
> Reporter: Ken Hu
> Priority: Major
>
> Based on the discussion in the devlist, this revolves changing the
> bindings/parameters from being a Map to a gremlin-lang string version of the
> map. This decouples the evolution of the language from the evolution of the
> serializers as new types can be added without having to update the
> serializers as well.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
