Added more invalid binding keys to Gremlin Server OpProcessor validation. These "invalid" keys are reserved terms for Gremlin Server as they are statically imported enums and shouldn't be used as binding keys. You get some less than easy to understand error messages if those keys are used.
Project: http://git-wip-us.apache.org/repos/asf/tinkerpop/repo Commit: http://git-wip-us.apache.org/repos/asf/tinkerpop/commit/c37c3016 Tree: http://git-wip-us.apache.org/repos/asf/tinkerpop/tree/c37c3016 Diff: http://git-wip-us.apache.org/repos/asf/tinkerpop/diff/c37c3016 Branch: refs/heads/TINKERPOP-1254 Commit: c37c3016bd05b3caeb09084f5469c606c439bfe5 Parents: 1c9bd08 Author: Stephen Mallette <sp...@genoprime.com> Authored: Thu Jun 30 13:07:44 2016 -0400 Committer: Stephen Mallette <sp...@genoprime.com> Committed: Thu Jun 30 13:07:44 2016 -0400 ---------------------------------------------------------------------- CHANGELOG.asciidoc | 1 + .../upgrade/release-3.1.x-incubating.asciidoc | 12 +++++ .../gremlin/process/traversal/Scope.java | 2 - .../server/op/AbstractEvalOpProcessor.java | 54 +++++++++++++++++--- .../server/GremlinServerIntegrateTest.java | 21 +++++++- 5 files changed, 79 insertions(+), 11 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/c37c3016/CHANGELOG.asciidoc ---------------------------------------------------------------------- diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 2ae2e77..3fd646a 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -33,6 +33,7 @@ TinkerPop 3.1.3 (NOT OFFICIALLY RELEASED YET) * Improved `TinkerGraph` performance when iterating vertices and edges. * Fixed a bug where timeout functions provided to the `GremlinExecutor` were not executing in the same thread as the script evaluation. * Optimized a few special cases in `RangeByIsCountStrategy`. +* Added more "invalid" variable bindings to the list used by Gremlin Server to validate incoming bindings on requests. * Named the thread pool used by Gremlin Server sessions: "gremlin-server-session-$n". * Fixed a bug in `BulkSet.equals()` which made itself apparent when using `store()` and `aggregate()` with labeled `cap()`. * Fixed a bug where `Result.one()` could potentially block indefinitely under certain circumstances. http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/c37c3016/docs/src/upgrade/release-3.1.x-incubating.asciidoc ---------------------------------------------------------------------- diff --git a/docs/src/upgrade/release-3.1.x-incubating.asciidoc b/docs/src/upgrade/release-3.1.x-incubating.asciidoc index b1b99f8..504baaa 100644 --- a/docs/src/upgrade/release-3.1.x-incubating.asciidoc +++ b/docs/src/upgrade/release-3.1.x-incubating.asciidoc @@ -32,6 +32,16 @@ Please see the link:https://github.com/apache/incubator-tinkerpop/blob/3.1.3/CHA Upgrading for Users ~~~~~~~~~~~~~~~~~~~ +Reserved Gremlin Server Keys +^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +Gremlin Server has always considered certain binding keys (request parameters) as reserved, but that list has now +expanded to be more inclusive all the static enums that are imported to the script engine. It is possible that those +using Gremlin Server may have to rename their keys if they somehow successfully were using some of the now reserved +terms in previous versions. + +See: https://issues.apache.org/jira/browse/TINKERPOP-1354[TINKERPOP-1354] + Remote Timeout ^^^^^^^^^^^^^^ @@ -54,6 +64,8 @@ set to `none`. See: https://issues.apache.org/jira/browse/TINKERPOP-1267[TINKERPOP-1267] + + Upgrading for Providers ~~~~~~~~~~~~~~~~~~~~~~~ http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/c37c3016/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/Scope.java ---------------------------------------------------------------------- diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/Scope.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/Scope.java index 1cce1e0..11979de 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/Scope.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/Scope.java @@ -18,8 +18,6 @@ */ package org.apache.tinkerpop.gremlin.process.traversal; -import java.util.Map; - /** * Many {@link Step} instance can have a variable scope. * {@link Scope#global}: the step operates on the entire traversal. http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/c37c3016/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/op/AbstractEvalOpProcessor.java ---------------------------------------------------------------------- diff --git a/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/op/AbstractEvalOpProcessor.java b/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/op/AbstractEvalOpProcessor.java index a5dd0a0..f712b44 100644 --- a/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/op/AbstractEvalOpProcessor.java +++ b/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/op/AbstractEvalOpProcessor.java @@ -28,10 +28,15 @@ import org.apache.tinkerpop.gremlin.driver.message.ResponseStatusCode; import org.apache.tinkerpop.gremlin.driver.ser.MessageTextSerializer; import org.apache.tinkerpop.gremlin.groovy.engine.GremlinExecutor; import org.apache.tinkerpop.gremlin.groovy.jsr223.customizer.TimedInterruptTimeoutException; +import org.apache.tinkerpop.gremlin.process.traversal.Operator; +import org.apache.tinkerpop.gremlin.process.traversal.Order; +import org.apache.tinkerpop.gremlin.process.traversal.Pop; +import org.apache.tinkerpop.gremlin.process.traversal.Scope; import org.apache.tinkerpop.gremlin.server.GraphManager; import org.apache.tinkerpop.gremlin.server.handler.Frame; import org.apache.tinkerpop.gremlin.server.handler.GremlinResponseFrameEncoder; import org.apache.tinkerpop.gremlin.server.handler.StateKey; +import org.apache.tinkerpop.gremlin.structure.Column; import org.apache.tinkerpop.gremlin.structure.T; import org.apache.tinkerpop.gremlin.server.Context; import org.apache.tinkerpop.gremlin.server.GremlinServer; @@ -54,11 +59,13 @@ import java.util.Iterator; import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.Set; import java.util.concurrent.CompletableFuture; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; import java.util.function.Supplier; import java.util.regex.Pattern; +import java.util.stream.Stream; import static com.codahale.metrics.MetricRegistry.name; @@ -84,7 +91,12 @@ public abstract class AbstractEvalOpProcessor implements OpProcessor { /** * Regex for validating that binding variables. + * + * @deprecated As of release 3.1.2-incubating, not replaced. This {@code Pattern} is not used internally. + * Deprecated rather than just removing as it's possible that someone else might be using it when developing + * custom {@link OpProcessor} implementations. */ + @Deprecated protected static final Pattern validBindingName = Pattern.compile("[a-zA-Z$_][a-zA-Z0-9$_]*"); /** @@ -95,12 +107,37 @@ public abstract class AbstractEvalOpProcessor implements OpProcessor { * Use of {@code toUpperCase()} on the accessor values of {@link T} solves an issue where the {@code ScriptEngine} * ignores private scope on {@link T} and imports static fields. */ - private static final List<String> invalidBindingsKeys = Arrays.asList( - T.id.getAccessor(), T.key.getAccessor(), - T.label.getAccessor(), T.value.getAccessor(), - T.id.getAccessor().toUpperCase(), T.key.getAccessor().toUpperCase(), - T.label.getAccessor().toUpperCase(), T.value.getAccessor().toUpperCase()); - private static final String invalidBindingKeysJoined = String.join(",", invalidBindingsKeys); + protected static final Set<String> INVALID_BINDINGS_KEYS = new HashSet<>(); + + static { + INVALID_BINDINGS_KEYS.addAll(Arrays.asList( + T.id.name(), T.key.name(), + T.label.name(), T.value.name(), + T.id.getAccessor(), T.key.getAccessor(), + T.label.getAccessor(), T.value.getAccessor(), + T.id.getAccessor().toUpperCase(), T.key.getAccessor().toUpperCase(), + T.label.getAccessor().toUpperCase(), T.value.getAccessor().toUpperCase())); + + for (Column enumItem : Column.values()) { + INVALID_BINDINGS_KEYS.add(enumItem.name()); + } + + for (Order enumItem : Order.values()) { + INVALID_BINDINGS_KEYS.add(enumItem.name()); + } + + for (Operator enumItem : Operator.values()) { + INVALID_BINDINGS_KEYS.add(enumItem.name()); + } + + for (Scope enumItem : Scope.values()) { + INVALID_BINDINGS_KEYS.add(enumItem.name()); + } + + for (Pop enumItem : Pop.values()) { + INVALID_BINDINGS_KEYS.add(enumItem.name()); + } + } protected final boolean manageTransactions; @@ -155,8 +192,9 @@ public abstract class AbstractEvalOpProcessor implements OpProcessor { throw new OpProcessorException(msg, ResponseMessage.build(message).code(ResponseStatusCode.REQUEST_ERROR_INVALID_REQUEST_ARGUMENTS).statusMessage(msg).create()); } - if (bindings.keySet().stream().anyMatch(invalidBindingsKeys::contains)) { - final String msg = String.format("The [%s] message is using at least one of the invalid binding key of [%s]. It conflicts with standard static imports to Gremlin Server.", Tokens.OPS_EVAL, invalidBindingKeysJoined); + final Set<String> badBindings = IteratorUtils.set(IteratorUtils.<String>filter(bindings.keySet().iterator(), INVALID_BINDINGS_KEYS::contains)); + if (!badBindings.isEmpty()) { + final String msg = String.format("The [%s] message supplies one or more invalid parameters key of [%s] - these are reserved names.", Tokens.OPS_EVAL, badBindings); throw new OpProcessorException(msg, ResponseMessage.build(message).code(ResponseStatusCode.REQUEST_ERROR_INVALID_REQUEST_ARGUMENTS).statusMessage(msg).create()); } } http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/c37c3016/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinServerIntegrateTest.java ---------------------------------------------------------------------- diff --git a/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinServerIntegrateTest.java b/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinServerIntegrateTest.java index 99bb745..2f091d9 100644 --- a/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinServerIntegrateTest.java +++ b/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinServerIntegrateTest.java @@ -55,7 +55,6 @@ import org.junit.Before; import org.junit.Test; import java.nio.channels.ClosedChannelException; -import java.security.cert.CertificateException; import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; @@ -412,6 +411,26 @@ public class GremlinServerIntegrateTest extends AbstractGremlinServerIntegration fail("Request should have returned error, but instead timed out"); assertTrue(pass.get()); } + + try (SimpleClient client = new WebSocketClient()) { + final Map<String, Object> bindings = new HashMap<>(); + bindings.put("id", "123"); + final RequestMessage request = RequestMessage.build(Tokens.OPS_EVAL) + .addArg(Tokens.ARGS_GREMLIN, "[1,2,3,4,5,6,7,8,9,0]") + .addArg(Tokens.ARGS_BINDINGS, bindings).create(); + final CountDownLatch latch = new CountDownLatch(1); + final AtomicBoolean pass = new AtomicBoolean(false); + client.submit(request, result -> { + if (result.getStatus().getCode() != ResponseStatusCode.PARTIAL_CONTENT) { + pass.set(ResponseStatusCode.REQUEST_ERROR_INVALID_REQUEST_ARGUMENTS == result.getStatus().getCode()); + latch.countDown(); + } + }); + + if (!latch.await(3000, TimeUnit.MILLISECONDS)) + fail("Request should have returned error, but instead timed out"); + assertTrue(pass.get()); + } } @Test
