From e9097e8e3118762948212caa72293aba21bdba32 Mon Sep 17 00:00:00 2001 From: Eric Hare Date: Mon, 18 May 2026 18:48:09 -0700 Subject: [PATCH 1/5] security: build keyspace DDL via SchemaBuilder/CqlIdentifier (follow-up to #2472) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #2472 closed the GHSA-p64p-96pw-mxwv injection vectors with input validation. This follow-up addresses the root cause for the keyspace-name sinks: replace the String.format CQL interpolation in CreateKeyspaceOperation and DropKeyspaceOperation with the driver's SchemaBuilder using CqlIdentifier.fromInternal(name). CqlIdentifier doubles embedded quote characters, so the keyspace identifier is well-formed regardless of upstream validation. Driver-gap finding for datacenter names: SchemaBuilder.withNetworkTopologyStrategy(Map) does NOT escape map keys (see OptionsUtils.extractOptionValue in java-driver-query-builder 4.19.0-preview1 — a hostile DC name with a single quote produces broken CQL). The DC-name allowlist applied by the resolver is therefore the actual security control for the replication map, not just defense-in-depth. The resolver comment is updated to flag this so the allowlist is not removed in a later cleanup. Changes: - CreateKeyspaceOperation: take (name, strategy, strategyOptions) and build the SimpleStatement via SchemaBuilder.createKeyspace(CqlIdentifier). - DropKeyspaceOperation: build via SchemaBuilder.dropKeyspace(CqlIdentifier). - CreateNamespaceKeyspaceCommandResolver: drop the dead getReplicationMap string-building; keep DC-name validation (now wired via validateStrategyOptions). Updated comment documents the driver-gap rationale. - New operation-level tests pin the SchemaBuilder behaviour: identifier escaping for keyspace names, fallback to SimpleStrategy, hyphenated DC name support, and a regression-pinning test that documents the driver's unescaped-map-key behaviour. - Existing resolver tests adjusted to the new record fields and existing injection-rejection tests retained. --- .../jsonapi/exception/SchemaException.java | 1 + .../keyspaces/CreateKeyspaceOperation.java | 51 +++++-- .../keyspaces/DropKeyspaceOperation.java | 18 ++- .../CreateKeyspaceCommandResolver.java | 9 +- .../CreateNamespaceCommandResolver.java | 9 +- ...reateNamespaceKeyspaceCommandResolver.java | 62 ++++---- src/main/resources/errors.yaml | 11 ++ .../CreateKeyspaceOperationTest.java | 106 +++++++++++++ .../keyspaces/DropKeyspaceOperationTest.java | 30 ++++ .../CreateKeyspaceCommandResolverTest.java | 140 ++++++++++++++++-- 10 files changed, 372 insertions(+), 65 deletions(-) create mode 100644 src/test/java/io/stargate/sgv2/jsonapi/service/operation/keyspaces/CreateKeyspaceOperationTest.java create mode 100644 src/test/java/io/stargate/sgv2/jsonapi/service/operation/keyspaces/DropKeyspaceOperationTest.java diff --git a/src/main/java/io/stargate/sgv2/jsonapi/exception/SchemaException.java b/src/main/java/io/stargate/sgv2/jsonapi/exception/SchemaException.java index ce0af3608e..06a22cb98e 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/exception/SchemaException.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/exception/SchemaException.java @@ -85,6 +85,7 @@ public enum Code implements ErrorCode { UNSUPPORTED_JSON_TYPE_FOR_TEXT_INDEX, UNSUPPORTED_LIST_DEFINITION, UNSUPPORTED_MAP_DEFINITION, + UNSUPPORTED_REPLICATION_DATA_CENTER_NAME, UNSUPPORTED_SCHEMA_NAME, UNSUPPORTED_SET_DEFINITION, UNSUPPORTED_TEXT_ANALYSIS_FOR_DATA_TYPES, diff --git a/src/main/java/io/stargate/sgv2/jsonapi/service/operation/keyspaces/CreateKeyspaceOperation.java b/src/main/java/io/stargate/sgv2/jsonapi/service/operation/keyspaces/CreateKeyspaceOperation.java index 0df3177abd..185f5098b6 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/service/operation/keyspaces/CreateKeyspaceOperation.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/service/operation/keyspaces/CreateKeyspaceOperation.java @@ -1,38 +1,71 @@ package io.stargate.sgv2.jsonapi.service.operation.keyspaces; +import com.datastax.oss.driver.api.core.CqlIdentifier; import com.datastax.oss.driver.api.core.cql.SimpleStatement; +import com.datastax.oss.driver.api.querybuilder.SchemaBuilder; +import com.datastax.oss.driver.api.querybuilder.schema.CreateKeyspace; +import com.datastax.oss.driver.api.querybuilder.schema.CreateKeyspaceStart; import io.smallrye.mutiny.Uni; import io.stargate.sgv2.jsonapi.api.model.command.CommandResult; import io.stargate.sgv2.jsonapi.api.request.RequestContext; import io.stargate.sgv2.jsonapi.service.cqldriver.executor.QueryExecutor; import io.stargate.sgv2.jsonapi.service.operation.Operation; import io.stargate.sgv2.jsonapi.service.operation.collections.SchemaChangeResult; +import java.util.Map; import java.util.function.Supplier; /** * Operation that creates a new Cassandra keyspace that serves as a namespace for the Data API. * + *

The CQL statement is constructed via the driver's {@link SchemaBuilder} using a {@link + * CqlIdentifier} for the keyspace name and the typed {@code withSimpleStrategy} / {@code + * withNetworkTopologyStrategy} APIs for the replication map. {@code CqlIdentifier} escapes embedded + * double quotes, removing the previous keyspace-name interpolation sink. + * + *

The driver's {@code withNetworkTopologyStrategy(Map)} does not escape map keys, so + * datacenter-name map keys must still be validated by callers (see the resolver's allowlist). + * * @param name Name of the keyspace to create. - * @param replicationMap A replication json, see - * https://docs.datastax.com/en/cql-oss/3.3/cql/cql_reference/cqlCreateKeyspace.html#Table2.Replicationstrategyclassandfactorsettings. + * @param strategy Replication strategy name. {@code "NetworkTopologyStrategy"} selects the + * network-topology strategy; any other value (including {@code null}) selects the simple + * strategy. + * @param strategyOptions Strategy options. For {@code NetworkTopologyStrategy} this is the + * datacenter-name to replication-factor map. For the simple strategy the {@code + * replication_factor} entry is used (defaulting to 1). May be {@code null}. */ -public record CreateKeyspaceOperation(String name, String replicationMap) implements Operation { +public record CreateKeyspaceOperation( + String name, String strategy, Map strategyOptions) implements Operation { - // simple pattern for the cql - private static final String CREATE_KEYSPACE_CQL = - "CREATE KEYSPACE IF NOT EXISTS \"%s\" WITH REPLICATION = %s;"; + private static final String NETWORK_TOPOLOGY_STRATEGY = "NetworkTopologyStrategy"; + private static final int DEFAULT_REPLICATION_FACTOR = 1; /** {@inheritDoc} */ @Override public Uni> execute( RequestContext dataApiRequestInfo, QueryExecutor queryExecutor) { - SimpleStatement createKeyspace = - SimpleStatement.newInstance(String.format(CREATE_KEYSPACE_CQL, name, replicationMap)); - // execute + SimpleStatement createKeyspace = buildStatement(); return queryExecutor .executeCreateSchemaChange(dataApiRequestInfo, createKeyspace) // if we have a result always respond positively .map(any -> new SchemaChangeResult(any.wasApplied())); } + + SimpleStatement buildStatement() { + CreateKeyspaceStart start = + SchemaBuilder.createKeyspace(CqlIdentifier.fromInternal(name)).ifNotExists(); + + CreateKeyspace withReplication; + if (NETWORK_TOPOLOGY_STRATEGY.equals(strategy)) { + withReplication = + start.withNetworkTopologyStrategy(strategyOptions != null ? strategyOptions : Map.of()); + } else { + int replicationFactor = + (strategyOptions != null) + ? strategyOptions.getOrDefault("replication_factor", DEFAULT_REPLICATION_FACTOR) + : DEFAULT_REPLICATION_FACTOR; + withReplication = start.withSimpleStrategy(replicationFactor); + } + return withReplication.build(); + } } diff --git a/src/main/java/io/stargate/sgv2/jsonapi/service/operation/keyspaces/DropKeyspaceOperation.java b/src/main/java/io/stargate/sgv2/jsonapi/service/operation/keyspaces/DropKeyspaceOperation.java index fb707e44ca..1e87a7329e 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/service/operation/keyspaces/DropKeyspaceOperation.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/service/operation/keyspaces/DropKeyspaceOperation.java @@ -1,6 +1,8 @@ package io.stargate.sgv2.jsonapi.service.operation.keyspaces; +import com.datastax.oss.driver.api.core.CqlIdentifier; import com.datastax.oss.driver.api.core.cql.SimpleStatement; +import com.datastax.oss.driver.api.querybuilder.SchemaBuilder; import io.smallrye.mutiny.Uni; import io.stargate.sgv2.jsonapi.api.model.command.CommandResult; import io.stargate.sgv2.jsonapi.api.request.RequestContext; @@ -12,24 +14,26 @@ /** * Operation that drops a Cassandra keyspace if it exists. * + *

The CQL statement is constructed via the driver's {@link SchemaBuilder} using a {@link + * CqlIdentifier}, which handles identifier quoting/escaping. This removes the previous {@code + * String.format} interpolation sink so CQL safety no longer depends on upstream name validation. + * * @param name Name of the keyspace to drop. */ public record DropKeyspaceOperation(String name) implements Operation { - // simple pattern for the cql - private static final String DROP_KEYSPACE_CQL = "DROP KEYSPACE IF EXISTS \"%s\";"; - /** {@inheritDoc} */ @Override public Uni> execute( RequestContext dataApiRequestInfo, QueryExecutor queryExecutor) { - SimpleStatement deleteStatement = - SimpleStatement.newInstance(DROP_KEYSPACE_CQL.formatted(name)); - // execute return queryExecutor - .executeDropSchemaChange(dataApiRequestInfo, deleteStatement) + .executeDropSchemaChange(dataApiRequestInfo, buildStatement()) // if we have a result always respond positively .map(any -> new SchemaChangeResult(any.wasApplied())); } + + SimpleStatement buildStatement() { + return SchemaBuilder.dropKeyspace(CqlIdentifier.fromInternal(name)).ifExists().build(); + } } diff --git a/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/CreateKeyspaceCommandResolver.java b/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/CreateKeyspaceCommandResolver.java index ca013050e9..36d8badb06 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/CreateKeyspaceCommandResolver.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/CreateKeyspaceCommandResolver.java @@ -10,8 +10,8 @@ import java.util.Map; /** - * Command resolver for {@link CreateKeyspaceCommand}. Responsible for creating the replication map. - * Resolve a {@link CreateKeyspaceCommand} to a {@link CreateKeyspaceOperation} + * Command resolver for {@link CreateKeyspaceCommand}. Resolves to a {@link CreateKeyspaceOperation} + * which builds the {@code CREATE KEYSPACE} statement via the driver's {@code SchemaBuilder}. */ @ApplicationScoped public class CreateKeyspaceCommandResolver @@ -38,7 +38,8 @@ public Operation resolveDatabaseCommand( (command.options() != null && command.options().replication() != null) ? command.options().replication().strategyOptions() : null; - String replicationMap = getReplicationMap(strategy, strategyOptions); - return new CreateKeyspaceOperation(keyspaceName, replicationMap); + + validateStrategyOptions(strategy, strategyOptions); + return new CreateKeyspaceOperation(keyspaceName, strategy, strategyOptions); } } diff --git a/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/CreateNamespaceCommandResolver.java b/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/CreateNamespaceCommandResolver.java index cda5c7c16d..9623e9471b 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/CreateNamespaceCommandResolver.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/CreateNamespaceCommandResolver.java @@ -10,8 +10,9 @@ import java.util.Map; /** - * Command resolver for {@link CreateNamespaceCommand}. Responsible for creating the replication - * map. Resolve a {@link CreateNamespaceCommand} to a {@link CreateKeyspaceOperation} + * Command resolver for {@link CreateNamespaceCommand}. Resolves to a {@link + * CreateKeyspaceOperation} which builds the {@code CREATE KEYSPACE} statement via the driver's + * {@code SchemaBuilder}. */ @ApplicationScoped public class CreateNamespaceCommandResolver @@ -39,7 +40,7 @@ public Operation resolveDatabaseCommand( ? command.options().replication().strategyOptions() : null; - String replicationMap = getReplicationMap(strategy, strategyOptions); - return new CreateKeyspaceOperation(keyspaceName, replicationMap); + validateStrategyOptions(strategy, strategyOptions); + return new CreateKeyspaceOperation(keyspaceName, strategy, strategyOptions); } } diff --git a/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/CreateNamespaceKeyspaceCommandResolver.java b/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/CreateNamespaceKeyspaceCommandResolver.java index ae5880d812..e9d1a1fa4b 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/CreateNamespaceKeyspaceCommandResolver.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/CreateNamespaceKeyspaceCommandResolver.java @@ -1,44 +1,50 @@ package io.stargate.sgv2.jsonapi.service.resolver; import io.stargate.sgv2.jsonapi.api.model.command.Command; +import io.stargate.sgv2.jsonapi.exception.ErrorTemplate; +import io.stargate.sgv2.jsonapi.exception.SchemaException; import java.util.Map; +import java.util.regex.Pattern; public abstract class CreateNamespaceKeyspaceCommandResolver implements CommandResolver { - // default if omitted - private static final String DEFAULT_REPLICATION_MAP = - "{'class': 'SimpleStrategy', 'replication_factor': 1}"; + private static final String NETWORK_TOPOLOGY_STRATEGY = "NetworkTopologyStrategy"; - // resolve the replication map - String getReplicationMap(String strategy, Map strategyOptions) { - if (strategy == null && strategyOptions == null) { - return DEFAULT_REPLICATION_MAP; - } - if ("NetworkTopologyStrategy".equals(strategy)) { - return networkTopologyStrategyMap(strategyOptions); - } else { - return simpleStrategyMap(strategyOptions); - } - } + // Allowlist for datacenter names. The driver's SchemaBuilder.withNetworkTopologyStrategy(Map) + // does NOT escape map keys (see OptionsUtils.extractOptionValue in java-driver-query-builder), + // so a hostile DC-name key passed to the driver would still produce broken CQL. This allowlist + // is therefore the actual security control for the replication map, not just API policy — + // do not remove without first ensuring DC names are escaped before reaching the driver. + private static final Pattern VALID_DATA_CENTER_NAME = Pattern.compile("[A-Za-z0-9_\\-]+"); + private static final int MAX_DATA_CENTER_NAME_LENGTH = 48; - private static String networkTopologyStrategyMap(Map strategyOptions) { - StringBuilder map = new StringBuilder("{'class': 'NetworkTopologyStrategy'"); - if (null != strategyOptions) { - for (Map.Entry dcEntry : strategyOptions.entrySet()) { - map.append(", '%s': %d".formatted(dcEntry.getKey(), dcEntry.getValue())); - } + /** + * Validate datacenter-name map keys when the strategy is {@code NetworkTopologyStrategy}. For + * other strategies the {@code strategyOptions} map is interpreted by the driver as simple- + * strategy options (e.g. {@code replication_factor}) and is not validated here. + */ + protected static void validateStrategyOptions( + String strategy, Map strategyOptions) { + if (!NETWORK_TOPOLOGY_STRATEGY.equals(strategy) || strategyOptions == null) { + return; + } + for (String dcName : strategyOptions.keySet()) { + checkDataCenterName(dcName); } - map.append("}"); - return map.toString(); } - private static String simpleStrategyMap(Map strategyOptions) { - if (null == strategyOptions || strategyOptions.isEmpty()) { - return DEFAULT_REPLICATION_MAP; + private static void checkDataCenterName(String name) { + if (name == null + || name.isEmpty() + || name.length() > MAX_DATA_CENTER_NAME_LENGTH + || !VALID_DATA_CENTER_NAME.matcher(name).matches()) { + throw SchemaException.Code.UNSUPPORTED_REPLICATION_DATA_CENTER_NAME.get( + Map.of( + "maxNameLength", + String.valueOf(MAX_DATA_CENTER_NAME_LENGTH), + "unsupportedDataCenterName", + ErrorTemplate.replaceIfNull(name))); } - - Integer replicationFactor = strategyOptions.getOrDefault("replication_factor", 1); - return "{'class': 'SimpleStrategy', 'replication_factor': " + replicationFactor + "}"; } } diff --git a/src/main/resources/errors.yaml b/src/main/resources/errors.yaml index f3408c38e1..2859d8a876 100644 --- a/src/main/resources/errors.yaml +++ b/src/main/resources/errors.yaml @@ -1624,6 +1624,17 @@ request-errors: Resend the command using a supported value type. + - scope: SCHEMA + code: UNSUPPORTED_REPLICATION_DATA_CENTER_NAME + title: The used data center name in the replication options is not supported + body: |- + The command included a data center name in the NetworkTopologyStrategy replication options that is not supported. + + Supported data center names must not be empty, more than ${maxNameLength} characters long, and may contain only alphanumeric, underscore, and hyphen characters. + The command used the unsupported data center name: '${unsupportedDataCenterName}'. + + Resend the command using a supported data center name. + - scope: SCHEMA code: UNSUPPORTED_SCHEMA_NAME title: The used schema name is not supported diff --git a/src/test/java/io/stargate/sgv2/jsonapi/service/operation/keyspaces/CreateKeyspaceOperationTest.java b/src/test/java/io/stargate/sgv2/jsonapi/service/operation/keyspaces/CreateKeyspaceOperationTest.java new file mode 100644 index 0000000000..83871dc1f7 --- /dev/null +++ b/src/test/java/io/stargate/sgv2/jsonapi/service/operation/keyspaces/CreateKeyspaceOperationTest.java @@ -0,0 +1,106 @@ +package io.stargate.sgv2.jsonapi.service.operation.keyspaces; + +import static org.assertj.core.api.Assertions.assertThat; + +import java.util.Map; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; + +/** + * Verifies that {@link CreateKeyspaceOperation} produces CQL via the driver's {@code SchemaBuilder} + * rather than via {@code String.format} interpolation. + * + *

This removes the keyspace-name interpolation sink at the root: {@code CqlIdentifier} doubles + * embedded double quotes, so a hostile keyspace name cannot break out of the identifier the way the + * prior {@code "CREATE KEYSPACE \"%s\""} pattern allowed. + * + *

Note on datacenter names: the driver's {@code + * SchemaBuilder.withNetworkTopologyStrategy(Map)} does not escape map keys (see {@code + * OptionsUtils.extractOptionValue} in {@code java-driver-query-builder}). The DC-name allowlist + * applied by the resolver therefore remains the actual security control for the replication map; + * the SchemaBuilder conversion only fixes the keyspace-identifier sink. + */ +class CreateKeyspaceOperationTest { + + @Nested + class SimpleStrategy { + + @Test + public void defaultsToReplicationFactorOne() { + var op = new CreateKeyspaceOperation("red_star_belgrade", null, null); + String cql = op.buildStatement().getQuery(); + assertThat(cql) + .contains("CREATE KEYSPACE IF NOT EXISTS") + .contains("red_star_belgrade") + .contains("'class':'SimpleStrategy'") + .contains("'replication_factor':1"); + } + + @Test + public void honoursExplicitReplicationFactor() { + var op = + new CreateKeyspaceOperation( + "red_star_belgrade", "SimpleStrategy", Map.of("replication_factor", 5)); + String cql = op.buildStatement().getQuery(); + assertThat(cql).contains("'class':'SimpleStrategy'").contains("'replication_factor':5"); + } + + @Test + public void unknownStrategyFallsBackToSimple() { + var op = new CreateKeyspaceOperation("k", "SomeOtherStrategy", null); + String cql = op.buildStatement().getQuery(); + assertThat(cql).contains("'class':'SimpleStrategy'"); + } + } + + @Nested + class NetworkTopologyStrategy { + + @Test + public void supportsRealisticCloudDataCenterNames() { + var op = new CreateKeyspaceOperation("ks", "NetworkTopologyStrategy", Map.of("us-east-1", 3)); + String cql = op.buildStatement().getQuery(); + assertThat(cql).contains("'us-east-1':3"); + } + + @Test + public void allowsEmptyDataCenterMap() { + var op = new CreateKeyspaceOperation("ks", "NetworkTopologyStrategy", Map.of()); + String cql = op.buildStatement().getQuery(); + assertThat(cql).contains("'class':'NetworkTopologyStrategy'"); + } + + @Test + public void driverDoesNotEscapeDataCenterMapKeys() { + // Pinning behaviour: the driver's SchemaBuilder leaves map keys unescaped, so the + // resolver-level DC-name allowlist is the actual defense against injection here. If this + // test ever starts failing because the driver begins escaping, the allowlist becomes + // defense-in-depth and the resolver wiring can be revisited. + var op = + new CreateKeyspaceOperation("ks", "NetworkTopologyStrategy", Map.of("dc'); EVIL --", 1)); + String cql = op.buildStatement().getQuery(); + assertThat(cql).contains("'dc'); EVIL --':1"); + } + } + + @Nested + class KeyspaceIdentifier { + + @Test + public void unquotedForSimpleAsciiName() { + var op = new CreateKeyspaceOperation("simple_name", null, null); + String cql = op.buildStatement().getQuery(); + assertThat(cql).contains("CREATE KEYSPACE IF NOT EXISTS simple_name"); + } + + @Test + public void escapesEmbeddedDoubleQuote() { + // CqlIdentifier.fromInternal escapes embedded double quotes by doubling them, so a name + // containing a quote cannot break out of the identifier the way the prior String.format + // ("\"%s\"") sink allowed. This is the root-cause defense the previous fix was missing. + var op = new CreateKeyspaceOperation("foo\"bar", null, null); + String cql = op.buildStatement().getQuery(); + assertThat(cql).contains("\"foo\"\"bar\""); + } + } +} diff --git a/src/test/java/io/stargate/sgv2/jsonapi/service/operation/keyspaces/DropKeyspaceOperationTest.java b/src/test/java/io/stargate/sgv2/jsonapi/service/operation/keyspaces/DropKeyspaceOperationTest.java new file mode 100644 index 0000000000..024039cef0 --- /dev/null +++ b/src/test/java/io/stargate/sgv2/jsonapi/service/operation/keyspaces/DropKeyspaceOperationTest.java @@ -0,0 +1,30 @@ +package io.stargate.sgv2.jsonapi.service.operation.keyspaces; + +import static org.assertj.core.api.Assertions.assertThat; + +import org.junit.jupiter.api.Test; + +/** + * Verifies that {@link DropKeyspaceOperation} produces CQL via the driver's {@code SchemaBuilder} + * rather than via {@code String.format} interpolation. The previous {@code "DROP KEYSPACE IF EXISTS + * \"%s\""} interpolation could be broken by a name containing a double quote; {@code + * CqlIdentifier.fromInternal} escapes those by doubling, so the resulting CQL stays well-formed. + */ +class DropKeyspaceOperationTest { + + @Test + public void buildsExpectedCqlForSimpleName() { + var op = new DropKeyspaceOperation("red_star_belgrade"); + String cql = op.buildStatement().getQuery(); + assertThat(cql).contains("DROP KEYSPACE IF EXISTS").contains("red_star_belgrade"); + } + + @Test + public void escapesEmbeddedDoubleQuoteInIdentifier() { + var op = new DropKeyspaceOperation("foo\"bar"); + String cql = op.buildStatement().getQuery(); + // Doubled quote keeps the identifier well-formed; the prior String.format sink would have + // produced "foo"bar" — a broken identifier that could form an injection vector. + assertThat(cql).contains("\"foo\"\"bar\"").doesNotContain("\"foo\"bar\""); + } +} diff --git a/src/test/java/io/stargate/sgv2/jsonapi/service/resolver/CreateKeyspaceCommandResolverTest.java b/src/test/java/io/stargate/sgv2/jsonapi/service/resolver/CreateKeyspaceCommandResolverTest.java index 2c9b860b82..46b528f6d5 100644 --- a/src/test/java/io/stargate/sgv2/jsonapi/service/resolver/CreateKeyspaceCommandResolverTest.java +++ b/src/test/java/io/stargate/sgv2/jsonapi/service/resolver/CreateKeyspaceCommandResolverTest.java @@ -58,8 +58,8 @@ public void noOptions() throws Exception { CreateKeyspaceOperation.class, op -> { assertThat(op.name()).isEqualTo("red_star_belgrade"); - assertThat(op.replicationMap()) - .isEqualTo("{'class': 'SimpleStrategy', 'replication_factor': 1}"); + assertThat(op.strategy()).isNull(); + assertThat(op.strategyOptions()).isNull(); }); } @@ -87,8 +87,7 @@ public void simpleStrategy() throws Exception { CreateKeyspaceOperation.class, op -> { assertThat(op.name()).isEqualTo("red_star_belgrade"); - assertThat(op.replicationMap()) - .isEqualTo("{'class': 'SimpleStrategy', 'replication_factor': 1}"); + assertThat(op.strategy()).isEqualTo("SimpleStrategy"); }); } @@ -117,8 +116,8 @@ public void simpleStrategyWithReplication() throws Exception { CreateKeyspaceOperation.class, op -> { assertThat(op.name()).isEqualTo("red_star_belgrade"); - assertThat(op.replicationMap()) - .isEqualTo("{'class': 'SimpleStrategy', 'replication_factor': 2}"); + assertThat(op.strategy()).isEqualTo("SimpleStrategy"); + assertThat(op.strategyOptions()).containsEntry("replication_factor", 2); }); } @@ -148,10 +147,10 @@ public void networkTopologyStrategy() throws Exception { CreateKeyspaceOperation.class, op -> { assertThat(op.name()).isEqualTo("red_star_belgrade"); - assertThat(op.replicationMap()) - .isIn( - "{'class': 'NetworkTopologyStrategy', 'Boston': 2, 'Berlin': 3}", - "{'class': 'NetworkTopologyStrategy', 'Berlin': 3, 'Boston': 2}"); + assertThat(op.strategy()).isEqualTo("NetworkTopologyStrategy"); + assertThat(op.strategyOptions()) + .containsEntry("Boston", 2) + .containsEntry("Berlin", 3); }); } @@ -180,7 +179,7 @@ public void networkTopologyStrategyNoDataCenter() throws Exception { CreateKeyspaceOperation.class, op -> { assertThat(op.name()).isEqualTo("red_star_belgrade"); - assertThat(op.replicationMap()).isEqualTo("{'class': 'NetworkTopologyStrategy'}"); + assertThat(op.strategy()).isEqualTo("NetworkTopologyStrategy"); }); } @@ -206,8 +205,7 @@ public void createKeyspaceWithSupportedName() throws Exception { CreateKeyspaceOperation.class, op -> { assertThat(op.name()).isEqualTo(name); - assertThat(op.replicationMap()) - .isEqualTo("{'class': 'SimpleStrategy', 'replication_factor': 1}"); + assertThat(op.strategy()).isNull(); }); } } @@ -348,6 +346,122 @@ public void createKeyspaceWithSpecialCharacter() throws Exception { "The supported Keyspace names must not be empty, more than 48 characters long, or contain non-alphanumeric-underscore characters.", "The command used the unsupported Keyspace name: '!@-'."); } + + @Test + public void rejectsInjectionInDataCenterName() throws Exception { + // A datacenter key with characters outside the API allowlist is rejected up front. + // The driver's SchemaBuilder.withNetworkTopologyStrategy(Map) does NOT escape map keys + // (see OptionsUtils.extractOptionValue in java-driver-query-builder), so this allowlist + // is the actual security control for the replication map. + String json = + """ + { + "createNamespace": { + "name" : "red_star_belgrade", + "options": { + "replication": { + "class": "NetworkTopologyStrategy", + "dc1', 'class': 'SimpleStrategy" : 1 + } + } + } + } + """; + + CreateNamespaceCommand command = objectMapper.readValue(json, CreateNamespaceCommand.class); + Throwable throwable = catchThrowable(() -> resolver.resolveCommand(commandContext, command)); + + verifySchemaException( + throwable, + SchemaException.Code.UNSUPPORTED_REPLICATION_DATA_CENTER_NAME, + "data center name in the NetworkTopologyStrategy replication options that is not supported", + "must not be empty, more than 48 characters long, and may contain only alphanumeric, underscore, and hyphen characters", + "The command used the unsupported data center name: 'dc1', 'class': 'SimpleStrategy'."); + } + + @Test + public void rejectsDataCenterNameWithSpace() throws Exception { + String json = + """ + { + "createNamespace": { + "name" : "red_star_belgrade", + "options": { + "replication": { + "class": "NetworkTopologyStrategy", + "dc one" : 1 + } + } + } + } + """; + + CreateNamespaceCommand command = objectMapper.readValue(json, CreateNamespaceCommand.class); + Throwable throwable = catchThrowable(() -> resolver.resolveCommand(commandContext, command)); + + verifySchemaException( + throwable, SchemaException.Code.UNSUPPORTED_REPLICATION_DATA_CENTER_NAME, "dc one"); + } + + @Test + public void rejectsDataCenterNameTooLong() throws Exception { + String dcName = RandomStringUtils.insecure().nextAlphabetic(49); + String json = + """ + { + "createNamespace": { + "name" : "red_star_belgrade", + "options": { + "replication": { + "class": "NetworkTopologyStrategy", + "%s" : 1 + } + } + } + } + """ + .formatted(dcName); + + CreateNamespaceCommand command = objectMapper.readValue(json, CreateNamespaceCommand.class); + Throwable throwable = catchThrowable(() -> resolver.resolveCommand(commandContext, command)); + + verifySchemaException( + throwable, SchemaException.Code.UNSUPPORTED_REPLICATION_DATA_CENTER_NAME, dcName); + } + } + + @Nested + class CreateKeyspaceDataCenterNameSuccess { + + @Test + public void hyphenatedDataCenterNameIsAllowed() throws Exception { + // Cloud-style DC names (Astra, AWS regions) contain hyphens and must keep working. + String json = + """ + { + "createNamespace": { + "name" : "red_star_belgrade", + "options": { + "replication": { + "class": "NetworkTopologyStrategy", + "us-east-1" : 3 + } + } + } + } + """; + + CreateNamespaceCommand command = objectMapper.readValue(json, CreateNamespaceCommand.class); + Operation result = resolver.resolveCommand(commandContext, command); + + assertThat(result) + .isInstanceOfSatisfying( + CreateKeyspaceOperation.class, + op -> { + assertThat(op.strategy()).isEqualTo("NetworkTopologyStrategy"); + assertThat(op.strategyOptions()).containsEntry("us-east-1", 3); + }); + } } private void verifySchemaException( From 51640a8511d75f8e9fcee94b6ebc1d8753527784 Mon Sep 17 00:00:00 2001 From: Eric Hare Date: Tue, 19 May 2026 14:20:18 -0700 Subject: [PATCH 2/5] fix: Review comments from @amorton --- .../command/impl/CreateKeyspaceCommand.java | 4 +- .../command/impl/CreateNamespaceCommand.java | 4 +- .../keyspaces/CreateKeyspaceOperation.java | 31 ++---- .../keyspaces/DropKeyspaceOperation.java | 17 +-- .../CreateKeyspaceCommandResolver.java | 22 ++-- .../CreateNamespaceCommandResolver.java | 22 ++-- ...reateNamespaceKeyspaceCommandResolver.java | 27 ++--- .../resolver/DropKeyspaceCommandResolver.java | 4 +- .../DropNamespaceCommandResolver.java | 9 +- .../KeyspaceCommandResolverSupport.java | 29 +++++ src/main/resources/errors.yaml | 2 +- .../impl/CreateKeyspaceCommandTest.java | 100 +++++++++++++++++- .../CreateKeyspaceOperationTest.java | 91 +++++++++------- .../keyspaces/DropKeyspaceOperationTest.java | 47 +++++--- .../CreateKeyspaceCommandResolverTest.java | 40 +++---- .../DropKeyspaceCommandResolverTest.java | 55 +++++++++- 16 files changed, 340 insertions(+), 164 deletions(-) create mode 100644 src/main/java/io/stargate/sgv2/jsonapi/service/resolver/KeyspaceCommandResolverSupport.java diff --git a/src/main/java/io/stargate/sgv2/jsonapi/api/model/command/impl/CreateKeyspaceCommand.java b/src/main/java/io/stargate/sgv2/jsonapi/api/model/command/impl/CreateKeyspaceCommand.java index 5e75e1f0b1..45897c4050 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/api/model/command/impl/CreateKeyspaceCommand.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/api/model/command/impl/CreateKeyspaceCommand.java @@ -39,7 +39,7 @@ public record Options(@Nullable @Valid Replication replication) {} + "For NetworkTopologyStrategy, use {\"class\": \"NetworkTopologyStrategy\", \"datacenter_name\": N, ...}.") public record Replication( @NotNull - @Pattern(regexp = "SimpleStrategy|NetworkTopologyStrategy") + @Pattern(regexp = "(?i)(SimpleStrategy|NetworkTopologyStrategy)") @JsonProperty("class") @Schema( description = @@ -53,7 +53,7 @@ public record Replication( + "For NetworkTopologyStrategy, use datacenter names as keys with replication factor as values " + "(e.g. 'dc1': 3, 'dc2': 2).", type = SchemaType.OBJECT) - Map strategyOptions) {} + Map strategyOptions) {} /** {@inheritDoc} */ @Override diff --git a/src/main/java/io/stargate/sgv2/jsonapi/api/model/command/impl/CreateNamespaceCommand.java b/src/main/java/io/stargate/sgv2/jsonapi/api/model/command/impl/CreateNamespaceCommand.java index c506421145..40dcd421d4 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/api/model/command/impl/CreateNamespaceCommand.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/api/model/command/impl/CreateNamespaceCommand.java @@ -53,7 +53,7 @@ public record Options(@Nullable @Valid Replication replication) {} deprecated = true) public record Replication( @NotNull() - @Pattern(regexp = "SimpleStrategy|NetworkTopologyStrategy") + @Pattern(regexp = "(?i)(SimpleStrategy|NetworkTopologyStrategy)") @JsonProperty("class") @Schema( description = @@ -67,7 +67,7 @@ public record Replication( + "For NetworkTopologyStrategy, use datacenter names as keys with replication factor as values " + "(e.g. 'dc1': 3, 'dc2': 2).", type = SchemaType.OBJECT) - Map strategyOptions) {} + Map strategyOptions) {} /** {@inheritDoc} */ @Override diff --git a/src/main/java/io/stargate/sgv2/jsonapi/service/operation/keyspaces/CreateKeyspaceOperation.java b/src/main/java/io/stargate/sgv2/jsonapi/service/operation/keyspaces/CreateKeyspaceOperation.java index 185f5098b6..d73a562ae1 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/service/operation/keyspaces/CreateKeyspaceOperation.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/service/operation/keyspaces/CreateKeyspaceOperation.java @@ -15,26 +15,14 @@ import java.util.function.Supplier; /** - * Operation that creates a new Cassandra keyspace that serves as a namespace for the Data API. + * Operation that creates a Cassandra keyspace for the Data API. * - *

The CQL statement is constructed via the driver's {@link SchemaBuilder} using a {@link - * CqlIdentifier} for the keyspace name and the typed {@code withSimpleStrategy} / {@code - * withNetworkTopologyStrategy} APIs for the replication map. {@code CqlIdentifier} escapes embedded - * double quotes, removing the previous keyspace-name interpolation sink. - * - *

The driver's {@code withNetworkTopologyStrategy(Map)} does not escape map keys, so - * datacenter-name map keys must still be validated by callers (see the resolver's allowlist). - * - * @param name Name of the keyspace to create. - * @param strategy Replication strategy name. {@code "NetworkTopologyStrategy"} selects the - * network-topology strategy; any other value (including {@code null}) selects the simple - * strategy. - * @param strategyOptions Strategy options. For {@code NetworkTopologyStrategy} this is the - * datacenter-name to replication-factor map. For the simple strategy the {@code - * replication_factor} entry is used (defaulting to 1). May be {@code null}. + *

The keyspace name is already a CQL identifier when it reaches this operation. Replication map + * keys are still plain strings because that is what the driver accepts, so the resolver validates + * datacenter names before creating this operation. */ public record CreateKeyspaceOperation( - String name, String strategy, Map strategyOptions) implements Operation { + CqlIdentifier name, String strategy, Map strategyOptions) implements Operation { private static final String NETWORK_TOPOLOGY_STRATEGY = "NetworkTopologyStrategy"; private static final int DEFAULT_REPLICATION_FACTOR = 1; @@ -46,17 +34,14 @@ public Uni> execute( SimpleStatement createKeyspace = buildStatement(); return queryExecutor .executeCreateSchemaChange(dataApiRequestInfo, createKeyspace) - - // if we have a result always respond positively .map(any -> new SchemaChangeResult(any.wasApplied())); } - SimpleStatement buildStatement() { - CreateKeyspaceStart start = - SchemaBuilder.createKeyspace(CqlIdentifier.fromInternal(name)).ifNotExists(); + private SimpleStatement buildStatement() { + CreateKeyspaceStart start = SchemaBuilder.createKeyspace(name).ifNotExists(); CreateKeyspace withReplication; - if (NETWORK_TOPOLOGY_STRATEGY.equals(strategy)) { + if (NETWORK_TOPOLOGY_STRATEGY.equalsIgnoreCase(strategy)) { withReplication = start.withNetworkTopologyStrategy(strategyOptions != null ? strategyOptions : Map.of()); } else { diff --git a/src/main/java/io/stargate/sgv2/jsonapi/service/operation/keyspaces/DropKeyspaceOperation.java b/src/main/java/io/stargate/sgv2/jsonapi/service/operation/keyspaces/DropKeyspaceOperation.java index 1e87a7329e..659d1bd1c7 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/service/operation/keyspaces/DropKeyspaceOperation.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/service/operation/keyspaces/DropKeyspaceOperation.java @@ -11,16 +11,7 @@ import io.stargate.sgv2.jsonapi.service.operation.collections.SchemaChangeResult; import java.util.function.Supplier; -/** - * Operation that drops a Cassandra keyspace if it exists. - * - *

The CQL statement is constructed via the driver's {@link SchemaBuilder} using a {@link - * CqlIdentifier}, which handles identifier quoting/escaping. This removes the previous {@code - * String.format} interpolation sink so CQL safety no longer depends on upstream name validation. - * - * @param name Name of the keyspace to drop. - */ -public record DropKeyspaceOperation(String name) implements Operation { +public record DropKeyspaceOperation(CqlIdentifier name) implements Operation { /** {@inheritDoc} */ @Override @@ -28,12 +19,10 @@ public Uni> execute( RequestContext dataApiRequestInfo, QueryExecutor queryExecutor) { return queryExecutor .executeDropSchemaChange(dataApiRequestInfo, buildStatement()) - - // if we have a result always respond positively .map(any -> new SchemaChangeResult(any.wasApplied())); } - SimpleStatement buildStatement() { - return SchemaBuilder.dropKeyspace(CqlIdentifier.fromInternal(name)).ifExists().build(); + private SimpleStatement buildStatement() { + return SchemaBuilder.dropKeyspace(name).ifExists().build(); } } diff --git a/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/CreateKeyspaceCommandResolver.java b/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/CreateKeyspaceCommandResolver.java index 36d8badb06..5b0425380f 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/CreateKeyspaceCommandResolver.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/CreateKeyspaceCommandResolver.java @@ -1,18 +1,17 @@ package io.stargate.sgv2.jsonapi.service.resolver; +import static io.stargate.sgv2.jsonapi.service.resolver.KeyspaceCommandResolverSupport.keyspaceIdentifierForCreate; +import static io.stargate.sgv2.jsonapi.util.ApiOptionUtils.getOrDefault; + import io.stargate.sgv2.jsonapi.api.model.command.CommandContext; import io.stargate.sgv2.jsonapi.api.model.command.impl.CreateKeyspaceCommand; import io.stargate.sgv2.jsonapi.service.operation.Operation; import io.stargate.sgv2.jsonapi.service.operation.keyspaces.CreateKeyspaceOperation; import io.stargate.sgv2.jsonapi.service.schema.DatabaseSchemaObject; -import io.stargate.sgv2.jsonapi.service.schema.naming.NamingRules; import jakarta.enterprise.context.ApplicationScoped; import java.util.Map; -/** - * Command resolver for {@link CreateKeyspaceCommand}. Resolves to a {@link CreateKeyspaceOperation} - * which builds the {@code CREATE KEYSPACE} statement via the driver's {@code SchemaBuilder}. - */ +/** Command resolver for {@link CreateKeyspaceCommand}. */ @ApplicationScoped public class CreateKeyspaceCommandResolver extends CreateNamespaceKeyspaceCommandResolver { @@ -27,17 +26,14 @@ public Class getCommandClass() { public Operation resolveDatabaseCommand( CommandContext ctx, CreateKeyspaceCommand command) { - var keyspaceName = NamingRules.KEYSPACE.checkRule(command.name()); + var keyspaceName = keyspaceIdentifierForCreate(command.name()); + var replication = + getOrDefault(command.options(), CreateKeyspaceCommand.Options::replication, null); - String strategy = - (command.options() != null && command.options().replication() != null) - ? command.options().replication().strategy() - : null; + String strategy = getOrDefault(replication, CreateKeyspaceCommand.Replication::strategy, null); Map strategyOptions = - (command.options() != null && command.options().replication() != null) - ? command.options().replication().strategyOptions() - : null; + getOrDefault(replication, CreateKeyspaceCommand.Replication::strategyOptions, null); validateStrategyOptions(strategy, strategyOptions); return new CreateKeyspaceOperation(keyspaceName, strategy, strategyOptions); diff --git a/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/CreateNamespaceCommandResolver.java b/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/CreateNamespaceCommandResolver.java index 9623e9471b..de48954315 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/CreateNamespaceCommandResolver.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/CreateNamespaceCommandResolver.java @@ -1,19 +1,17 @@ package io.stargate.sgv2.jsonapi.service.resolver; +import static io.stargate.sgv2.jsonapi.service.resolver.KeyspaceCommandResolverSupport.keyspaceIdentifierForCreate; +import static io.stargate.sgv2.jsonapi.util.ApiOptionUtils.getOrDefault; + import io.stargate.sgv2.jsonapi.api.model.command.CommandContext; import io.stargate.sgv2.jsonapi.api.model.command.impl.CreateNamespaceCommand; import io.stargate.sgv2.jsonapi.service.operation.Operation; import io.stargate.sgv2.jsonapi.service.operation.keyspaces.CreateKeyspaceOperation; import io.stargate.sgv2.jsonapi.service.schema.DatabaseSchemaObject; -import io.stargate.sgv2.jsonapi.service.schema.naming.NamingRules; import jakarta.enterprise.context.ApplicationScoped; import java.util.Map; -/** - * Command resolver for {@link CreateNamespaceCommand}. Resolves to a {@link - * CreateKeyspaceOperation} which builds the {@code CREATE KEYSPACE} statement via the driver's - * {@code SchemaBuilder}. - */ +/** Command resolver for {@link CreateNamespaceCommand}. */ @ApplicationScoped public class CreateNamespaceCommandResolver extends CreateNamespaceKeyspaceCommandResolver { @@ -28,17 +26,15 @@ public Class getCommandClass() { public Operation resolveDatabaseCommand( CommandContext ctx, CreateNamespaceCommand command) { - var keyspaceName = NamingRules.KEYSPACE.checkRule(command.name()); + var keyspaceName = keyspaceIdentifierForCreate(command.name()); + var replication = + getOrDefault(command.options(), CreateNamespaceCommand.Options::replication, null); String strategy = - (command.options() != null && command.options().replication() != null) - ? command.options().replication().strategy() - : null; + getOrDefault(replication, CreateNamespaceCommand.Replication::strategy, null); Map strategyOptions = - (command.options() != null && command.options().replication() != null) - ? command.options().replication().strategyOptions() - : null; + getOrDefault(replication, CreateNamespaceCommand.Replication::strategyOptions, null); validateStrategyOptions(strategy, strategyOptions); return new CreateKeyspaceOperation(keyspaceName, strategy, strategyOptions); diff --git a/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/CreateNamespaceKeyspaceCommandResolver.java b/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/CreateNamespaceKeyspaceCommandResolver.java index e9d1a1fa4b..06448ac885 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/CreateNamespaceKeyspaceCommandResolver.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/CreateNamespaceKeyspaceCommandResolver.java @@ -11,22 +11,12 @@ public abstract class CreateNamespaceKeyspaceCommandResolver private static final String NETWORK_TOPOLOGY_STRATEGY = "NetworkTopologyStrategy"; - // Allowlist for datacenter names. The driver's SchemaBuilder.withNetworkTopologyStrategy(Map) - // does NOT escape map keys (see OptionsUtils.extractOptionValue in java-driver-query-builder), - // so a hostile DC-name key passed to the driver would still produce broken CQL. This allowlist - // is therefore the actual security control for the replication map, not just API policy — - // do not remove without first ensuring DC names are escaped before reaching the driver. - private static final Pattern VALID_DATA_CENTER_NAME = Pattern.compile("[A-Za-z0-9_\\-]+"); - private static final int MAX_DATA_CENTER_NAME_LENGTH = 48; + // The driver writes NetworkTopologyStrategy map keys as CQL string literals without escaping them. + private static final Pattern VALID_DATA_CENTER_NAME = Pattern.compile("[^']+"); - /** - * Validate datacenter-name map keys when the strategy is {@code NetworkTopologyStrategy}. For - * other strategies the {@code strategyOptions} map is interpreted by the driver as simple- - * strategy options (e.g. {@code replication_factor}) and is not validated here. - */ protected static void validateStrategyOptions( String strategy, Map strategyOptions) { - if (!NETWORK_TOPOLOGY_STRATEGY.equals(strategy) || strategyOptions == null) { + if (!isNetworkTopologyStrategy(strategy) || strategyOptions == null) { return; } for (String dcName : strategyOptions.keySet()) { @@ -34,15 +24,14 @@ protected static void validateStrategyOptions( } } + private static boolean isNetworkTopologyStrategy(String strategy) { + return NETWORK_TOPOLOGY_STRATEGY.equalsIgnoreCase(strategy); + } + private static void checkDataCenterName(String name) { - if (name == null - || name.isEmpty() - || name.length() > MAX_DATA_CENTER_NAME_LENGTH - || !VALID_DATA_CENTER_NAME.matcher(name).matches()) { + if (name == null || !VALID_DATA_CENTER_NAME.matcher(name).matches()) { throw SchemaException.Code.UNSUPPORTED_REPLICATION_DATA_CENTER_NAME.get( Map.of( - "maxNameLength", - String.valueOf(MAX_DATA_CENTER_NAME_LENGTH), "unsupportedDataCenterName", ErrorTemplate.replaceIfNull(name))); } diff --git a/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/DropKeyspaceCommandResolver.java b/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/DropKeyspaceCommandResolver.java index b6943f5819..f87767e810 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/DropKeyspaceCommandResolver.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/DropKeyspaceCommandResolver.java @@ -1,5 +1,7 @@ package io.stargate.sgv2.jsonapi.service.resolver; +import static io.stargate.sgv2.jsonapi.service.resolver.KeyspaceCommandResolverSupport.keyspaceIdentifierForDrop; + import io.stargate.sgv2.jsonapi.api.model.command.CommandContext; import io.stargate.sgv2.jsonapi.api.model.command.impl.DropKeyspaceCommand; import io.stargate.sgv2.jsonapi.service.operation.Operation; @@ -21,6 +23,6 @@ public Class getCommandClass() { @Override public Operation resolveDatabaseCommand( CommandContext ctx, DropKeyspaceCommand command) { - return new DropKeyspaceOperation(command.name()); + return new DropKeyspaceOperation(keyspaceIdentifierForDrop(command.name())); } } diff --git a/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/DropNamespaceCommandResolver.java b/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/DropNamespaceCommandResolver.java index ce0c886d2f..e7333e74fe 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/DropNamespaceCommandResolver.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/DropNamespaceCommandResolver.java @@ -1,5 +1,7 @@ package io.stargate.sgv2.jsonapi.service.resolver; +import static io.stargate.sgv2.jsonapi.service.resolver.KeyspaceCommandResolverSupport.keyspaceIdentifierForDrop; + import io.stargate.sgv2.jsonapi.api.model.command.CommandContext; import io.stargate.sgv2.jsonapi.api.model.command.impl.DropNamespaceCommand; import io.stargate.sgv2.jsonapi.service.operation.Operation; @@ -7,10 +9,7 @@ import io.stargate.sgv2.jsonapi.service.schema.DatabaseSchemaObject; import jakarta.enterprise.context.ApplicationScoped; -/** - * Command resolver for {@link DropNamespaceCommand}. Resolve a {@link DropNamespaceCommand} to a - * {@link DropKeyspaceOperation} - */ +/** Command resolver for {@link DropNamespaceCommand}. */ @ApplicationScoped public class DropNamespaceCommandResolver implements CommandResolver { @@ -24,6 +23,6 @@ public Class getCommandClass() { @Override public Operation resolveDatabaseCommand( CommandContext ctx, DropNamespaceCommand command) { - return new DropKeyspaceOperation(command.name()); + return new DropKeyspaceOperation(keyspaceIdentifierForDrop(command.name())); } } diff --git a/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/KeyspaceCommandResolverSupport.java b/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/KeyspaceCommandResolverSupport.java new file mode 100644 index 0000000000..a377d76be4 --- /dev/null +++ b/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/KeyspaceCommandResolverSupport.java @@ -0,0 +1,29 @@ +package io.stargate.sgv2.jsonapi.service.resolver; + +import static io.stargate.sgv2.jsonapi.util.CqlIdentifierUtil.cqlIdentifierFromUserInput; + +import com.datastax.oss.driver.api.core.CqlIdentifier; +import io.stargate.sgv2.jsonapi.exception.ErrorTemplate; +import io.stargate.sgv2.jsonapi.exception.SchemaException; +import io.stargate.sgv2.jsonapi.service.schema.naming.NamingRules; +import java.util.Map; +import java.util.regex.Pattern; + +final class KeyspaceCommandResolverSupport { + + private static final Pattern DROP_KEYSPACE_NAME = Pattern.compile("\\w+"); + + private KeyspaceCommandResolverSupport() {} + + static CqlIdentifier keyspaceIdentifierForCreate(String name) { + return cqlIdentifierFromUserInput(NamingRules.KEYSPACE.checkRule(name)); + } + + static CqlIdentifier keyspaceIdentifierForDrop(String name) { + if (name == null || !DROP_KEYSPACE_NAME.matcher(name).matches()) { + throw SchemaException.Code.INVALID_KEYSPACE.get( + Map.of("keyspace", ErrorTemplate.replaceIfNull(name))); + } + return cqlIdentifierFromUserInput(name); + } +} diff --git a/src/main/resources/errors.yaml b/src/main/resources/errors.yaml index 2859d8a876..4c346b0b53 100644 --- a/src/main/resources/errors.yaml +++ b/src/main/resources/errors.yaml @@ -1630,7 +1630,7 @@ request-errors: body: |- The command included a data center name in the NetworkTopologyStrategy replication options that is not supported. - Supported data center names must not be empty, more than ${maxNameLength} characters long, and may contain only alphanumeric, underscore, and hyphen characters. + Supported data center names must not be empty or contain single quote characters. The command used the unsupported data center name: '${unsupportedDataCenterName}'. Resend the command using a supported data center name. diff --git a/src/test/java/io/stargate/sgv2/jsonapi/api/model/command/impl/CreateKeyspaceCommandTest.java b/src/test/java/io/stargate/sgv2/jsonapi/api/model/command/impl/CreateKeyspaceCommandTest.java index c5f3333aeb..41d0e42ab4 100644 --- a/src/test/java/io/stargate/sgv2/jsonapi/api/model/command/impl/CreateKeyspaceCommandTest.java +++ b/src/test/java/io/stargate/sgv2/jsonapi/api/model/command/impl/CreateKeyspaceCommandTest.java @@ -65,6 +65,32 @@ public void strategyNull() throws Exception { .contains("must not be null"); } + @Test + public void strategyOptionValueNull() throws Exception { + String json = + """ + { + "createKeyspace": { + "name": "red_star_belgrade", + "options": { + "replication": { + "class": "NetworkTopologyStrategy", + "dc1": null + } + } + } + } + """; + + CreateKeyspaceCommand command = objectMapper.readValue(json, CreateKeyspaceCommand.class); + Set> result = validator.validate(command); + + assertThat(result) + .isNotEmpty() + .extracting(ConstraintViolation::getMessage) + .contains("must not be null"); + } + @Test public void strategyWrong() throws Exception { String json = @@ -87,7 +113,29 @@ public void strategyWrong() throws Exception { assertThat(result) .isNotEmpty() .extracting(ConstraintViolation::getMessage) - .contains("must match \"SimpleStrategy|NetworkTopologyStrategy\""); + .contains("must match \"(?i)(SimpleStrategy|NetworkTopologyStrategy)\""); + } + + @Test + public void strategyPatternIsCaseInsensitive() throws Exception { + String json = + """ + { + "createKeyspace": { + "name": "red_star_belgrade", + "options": { + "replication": { + "class": "networktopologystrategy" + } + } + } + } + """; + + CreateKeyspaceCommand command = objectMapper.readValue(json, CreateKeyspaceCommand.class); + Set> result = validator.validate(command); + + assertThat(result).isEmpty(); } } @@ -135,6 +183,32 @@ public void strategyNull() throws Exception { .contains("must not be null"); } + @Test + public void strategyOptionValueNull() throws Exception { + String json = + """ + { + "createNamespace": { + "name": "red_star_belgrade", + "options": { + "replication": { + "class": "NetworkTopologyStrategy", + "dc1": null + } + } + } + } + """; + + CreateNamespaceCommand command = objectMapper.readValue(json, CreateNamespaceCommand.class); + Set> result = validator.validate(command); + + assertThat(result) + .isNotEmpty() + .extracting(ConstraintViolation::getMessage) + .contains("must not be null"); + } + @Test public void strategyWrong() throws Exception { String json = @@ -157,7 +231,29 @@ public void strategyWrong() throws Exception { assertThat(result) .isNotEmpty() .extracting(ConstraintViolation::getMessage) - .contains("must match \"SimpleStrategy|NetworkTopologyStrategy\""); + .contains("must match \"(?i)(SimpleStrategy|NetworkTopologyStrategy)\""); + } + + @Test + public void strategyPatternIsCaseInsensitive() throws Exception { + String json = + """ + { + "createNamespace": { + "name": "red_star_belgrade", + "options": { + "replication": { + "class": "networktopologystrategy" + } + } + } + } + """; + + CreateNamespaceCommand command = objectMapper.readValue(json, CreateNamespaceCommand.class); + Set> result = validator.validate(command); + + assertThat(result).isEmpty(); } } } diff --git a/src/test/java/io/stargate/sgv2/jsonapi/service/operation/keyspaces/CreateKeyspaceOperationTest.java b/src/test/java/io/stargate/sgv2/jsonapi/service/operation/keyspaces/CreateKeyspaceOperationTest.java index 83871dc1f7..5b44024f38 100644 --- a/src/test/java/io/stargate/sgv2/jsonapi/service/operation/keyspaces/CreateKeyspaceOperationTest.java +++ b/src/test/java/io/stargate/sgv2/jsonapi/service/operation/keyspaces/CreateKeyspaceOperationTest.java @@ -1,24 +1,26 @@ package io.stargate.sgv2.jsonapi.service.operation.keyspaces; import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; +import com.datastax.oss.driver.api.core.CqlIdentifier; +import com.datastax.oss.driver.api.core.cql.AsyncResultSet; +import com.datastax.oss.driver.api.core.cql.SimpleStatement; +import io.smallrye.mutiny.Uni; +import io.stargate.sgv2.jsonapi.api.request.RequestContext; +import io.stargate.sgv2.jsonapi.service.cqldriver.executor.QueryExecutor; import java.util.Map; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; +import org.mockito.ArgumentCaptor; /** - * Verifies that {@link CreateKeyspaceOperation} produces CQL via the driver's {@code SchemaBuilder} - * rather than via {@code String.format} interpolation. - * - *

This removes the keyspace-name interpolation sink at the root: {@code CqlIdentifier} doubles - * embedded double quotes, so a hostile keyspace name cannot break out of the identifier the way the - * prior {@code "CREATE KEYSPACE \"%s\""} pattern allowed. - * - *

Note on datacenter names: the driver's {@code - * SchemaBuilder.withNetworkTopologyStrategy(Map)} does not escape map keys (see {@code - * OptionsUtils.extractOptionValue} in {@code java-driver-query-builder}). The DC-name allowlist - * applied by the resolver therefore remains the actual security control for the replication map; - * the SchemaBuilder conversion only fixes the keyspace-identifier sink. + * Verifies the keyspace CQL passed to the query executor. The operation gets a safe keyspace + * identifier from the resolver; replication map keys are still validated before this point. */ class CreateKeyspaceOperationTest { @@ -27,8 +29,8 @@ class SimpleStrategy { @Test public void defaultsToReplicationFactorOne() { - var op = new CreateKeyspaceOperation("red_star_belgrade", null, null); - String cql = op.buildStatement().getQuery(); + var op = new CreateKeyspaceOperation(identifier("red_star_belgrade"), null, null); + String cql = createCql(op); assertThat(cql) .contains("CREATE KEYSPACE IF NOT EXISTS") .contains("red_star_belgrade") @@ -40,15 +42,15 @@ public void defaultsToReplicationFactorOne() { public void honoursExplicitReplicationFactor() { var op = new CreateKeyspaceOperation( - "red_star_belgrade", "SimpleStrategy", Map.of("replication_factor", 5)); - String cql = op.buildStatement().getQuery(); + identifier("red_star_belgrade"), "SimpleStrategy", Map.of("replication_factor", 5)); + String cql = createCql(op); assertThat(cql).contains("'class':'SimpleStrategy'").contains("'replication_factor':5"); } @Test public void unknownStrategyFallsBackToSimple() { - var op = new CreateKeyspaceOperation("k", "SomeOtherStrategy", null); - String cql = op.buildStatement().getQuery(); + var op = new CreateKeyspaceOperation(identifier("k"), "SomeOtherStrategy", null); + String cql = createCql(op); assertThat(cql).contains("'class':'SimpleStrategy'"); } } @@ -58,28 +60,27 @@ class NetworkTopologyStrategy { @Test public void supportsRealisticCloudDataCenterNames() { - var op = new CreateKeyspaceOperation("ks", "NetworkTopologyStrategy", Map.of("us-east-1", 3)); - String cql = op.buildStatement().getQuery(); + var op = + new CreateKeyspaceOperation( + identifier("ks"), "NetworkTopologyStrategy", Map.of("us-east-1", 3)); + String cql = createCql(op); assertThat(cql).contains("'us-east-1':3"); } @Test public void allowsEmptyDataCenterMap() { - var op = new CreateKeyspaceOperation("ks", "NetworkTopologyStrategy", Map.of()); - String cql = op.buildStatement().getQuery(); + var op = new CreateKeyspaceOperation(identifier("ks"), "NetworkTopologyStrategy", Map.of()); + String cql = createCql(op); assertThat(cql).contains("'class':'NetworkTopologyStrategy'"); } @Test - public void driverDoesNotEscapeDataCenterMapKeys() { - // Pinning behaviour: the driver's SchemaBuilder leaves map keys unescaped, so the - // resolver-level DC-name allowlist is the actual defense against injection here. If this - // test ever starts failing because the driver begins escaping, the allowlist becomes - // defense-in-depth and the resolver wiring can be revisited. + public void strategyNameIsCaseInsensitive() { var op = - new CreateKeyspaceOperation("ks", "NetworkTopologyStrategy", Map.of("dc'); EVIL --", 1)); - String cql = op.buildStatement().getQuery(); - assertThat(cql).contains("'dc'); EVIL --':1"); + new CreateKeyspaceOperation( + identifier("ks"), "networktopologystrategy", Map.of("dc1", 3)); + String cql = createCql(op); + assertThat(cql).contains("'class':'NetworkTopologyStrategy'").contains("'dc1':3"); } } @@ -88,19 +89,35 @@ class KeyspaceIdentifier { @Test public void unquotedForSimpleAsciiName() { - var op = new CreateKeyspaceOperation("simple_name", null, null); - String cql = op.buildStatement().getQuery(); + var op = new CreateKeyspaceOperation(identifier("simple_name"), null, null); + String cql = createCql(op); assertThat(cql).contains("CREATE KEYSPACE IF NOT EXISTS simple_name"); } @Test public void escapesEmbeddedDoubleQuote() { - // CqlIdentifier.fromInternal escapes embedded double quotes by doubling them, so a name - // containing a quote cannot break out of the identifier the way the prior String.format - // ("\"%s\"") sink allowed. This is the root-cause defense the previous fix was missing. - var op = new CreateKeyspaceOperation("foo\"bar", null, null); - String cql = op.buildStatement().getQuery(); + var op = new CreateKeyspaceOperation(identifier("foo\"bar"), null, null); + String cql = createCql(op); assertThat(cql).contains("\"foo\"\"bar\""); } } + + private static CqlIdentifier identifier(String name) { + return CqlIdentifier.fromInternal(name); + } + + private static String createCql(CreateKeyspaceOperation op) { + var requestContext = mock(RequestContext.class); + var queryExecutor = mock(QueryExecutor.class); + var resultSet = mock(AsyncResultSet.class); + when(resultSet.wasApplied()).thenReturn(true); + when(queryExecutor.executeCreateSchemaChange(eq(requestContext), any(SimpleStatement.class))) + .thenReturn(Uni.createFrom().item(resultSet)); + + op.execute(requestContext, queryExecutor).await().indefinitely(); + + var statement = ArgumentCaptor.forClass(SimpleStatement.class); + verify(queryExecutor).executeCreateSchemaChange(eq(requestContext), statement.capture()); + return statement.getValue().getQuery(); + } } diff --git a/src/test/java/io/stargate/sgv2/jsonapi/service/operation/keyspaces/DropKeyspaceOperationTest.java b/src/test/java/io/stargate/sgv2/jsonapi/service/operation/keyspaces/DropKeyspaceOperationTest.java index 024039cef0..301ff8de7d 100644 --- a/src/test/java/io/stargate/sgv2/jsonapi/service/operation/keyspaces/DropKeyspaceOperationTest.java +++ b/src/test/java/io/stargate/sgv2/jsonapi/service/operation/keyspaces/DropKeyspaceOperationTest.java @@ -1,30 +1,53 @@ package io.stargate.sgv2.jsonapi.service.operation.keyspaces; import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; +import com.datastax.oss.driver.api.core.CqlIdentifier; +import com.datastax.oss.driver.api.core.cql.AsyncResultSet; +import com.datastax.oss.driver.api.core.cql.SimpleStatement; +import io.smallrye.mutiny.Uni; +import io.stargate.sgv2.jsonapi.api.request.RequestContext; +import io.stargate.sgv2.jsonapi.service.cqldriver.executor.QueryExecutor; import org.junit.jupiter.api.Test; +import org.mockito.ArgumentCaptor; -/** - * Verifies that {@link DropKeyspaceOperation} produces CQL via the driver's {@code SchemaBuilder} - * rather than via {@code String.format} interpolation. The previous {@code "DROP KEYSPACE IF EXISTS - * \"%s\""} interpolation could be broken by a name containing a double quote; {@code - * CqlIdentifier.fromInternal} escapes those by doubling, so the resulting CQL stays well-formed. - */ class DropKeyspaceOperationTest { @Test public void buildsExpectedCqlForSimpleName() { - var op = new DropKeyspaceOperation("red_star_belgrade"); - String cql = op.buildStatement().getQuery(); + var op = new DropKeyspaceOperation(identifier("red_star_belgrade")); + String cql = dropCql(op); assertThat(cql).contains("DROP KEYSPACE IF EXISTS").contains("red_star_belgrade"); } @Test public void escapesEmbeddedDoubleQuoteInIdentifier() { - var op = new DropKeyspaceOperation("foo\"bar"); - String cql = op.buildStatement().getQuery(); - // Doubled quote keeps the identifier well-formed; the prior String.format sink would have - // produced "foo"bar" — a broken identifier that could form an injection vector. + var op = new DropKeyspaceOperation(identifier("foo\"bar")); + String cql = dropCql(op); assertThat(cql).contains("\"foo\"\"bar\"").doesNotContain("\"foo\"bar\""); } + + private static CqlIdentifier identifier(String name) { + return CqlIdentifier.fromInternal(name); + } + + private static String dropCql(DropKeyspaceOperation op) { + var requestContext = mock(RequestContext.class); + var queryExecutor = mock(QueryExecutor.class); + var resultSet = mock(AsyncResultSet.class); + when(resultSet.wasApplied()).thenReturn(true); + when(queryExecutor.executeDropSchemaChange(eq(requestContext), any(SimpleStatement.class))) + .thenReturn(Uni.createFrom().item(resultSet)); + + op.execute(requestContext, queryExecutor).await().indefinitely(); + + var statement = ArgumentCaptor.forClass(SimpleStatement.class); + verify(queryExecutor).executeDropSchemaChange(eq(requestContext), statement.capture()); + return statement.getValue().getQuery(); + } } diff --git a/src/test/java/io/stargate/sgv2/jsonapi/service/resolver/CreateKeyspaceCommandResolverTest.java b/src/test/java/io/stargate/sgv2/jsonapi/service/resolver/CreateKeyspaceCommandResolverTest.java index 46b528f6d5..dd24663e71 100644 --- a/src/test/java/io/stargate/sgv2/jsonapi/service/resolver/CreateKeyspaceCommandResolverTest.java +++ b/src/test/java/io/stargate/sgv2/jsonapi/service/resolver/CreateKeyspaceCommandResolverTest.java @@ -57,7 +57,7 @@ public void noOptions() throws Exception { .isInstanceOfSatisfying( CreateKeyspaceOperation.class, op -> { - assertThat(op.name()).isEqualTo("red_star_belgrade"); + assertThat(op.name().asInternal()).isEqualTo("red_star_belgrade"); assertThat(op.strategy()).isNull(); assertThat(op.strategyOptions()).isNull(); }); @@ -86,7 +86,7 @@ public void simpleStrategy() throws Exception { .isInstanceOfSatisfying( CreateKeyspaceOperation.class, op -> { - assertThat(op.name()).isEqualTo("red_star_belgrade"); + assertThat(op.name().asInternal()).isEqualTo("red_star_belgrade"); assertThat(op.strategy()).isEqualTo("SimpleStrategy"); }); } @@ -115,7 +115,7 @@ public void simpleStrategyWithReplication() throws Exception { .isInstanceOfSatisfying( CreateKeyspaceOperation.class, op -> { - assertThat(op.name()).isEqualTo("red_star_belgrade"); + assertThat(op.name().asInternal()).isEqualTo("red_star_belgrade"); assertThat(op.strategy()).isEqualTo("SimpleStrategy"); assertThat(op.strategyOptions()).containsEntry("replication_factor", 2); }); @@ -146,7 +146,7 @@ public void networkTopologyStrategy() throws Exception { .isInstanceOfSatisfying( CreateKeyspaceOperation.class, op -> { - assertThat(op.name()).isEqualTo("red_star_belgrade"); + assertThat(op.name().asInternal()).isEqualTo("red_star_belgrade"); assertThat(op.strategy()).isEqualTo("NetworkTopologyStrategy"); assertThat(op.strategyOptions()) .containsEntry("Boston", 2) @@ -178,7 +178,7 @@ public void networkTopologyStrategyNoDataCenter() throws Exception { .isInstanceOfSatisfying( CreateKeyspaceOperation.class, op -> { - assertThat(op.name()).isEqualTo("red_star_belgrade"); + assertThat(op.name().asInternal()).isEqualTo("red_star_belgrade"); assertThat(op.strategy()).isEqualTo("NetworkTopologyStrategy"); }); } @@ -204,7 +204,7 @@ public void createKeyspaceWithSupportedName() throws Exception { .isInstanceOfSatisfying( CreateKeyspaceOperation.class, op -> { - assertThat(op.name()).isEqualTo(name); + assertThat(op.name().asInternal()).isEqualTo(name); assertThat(op.strategy()).isNull(); }); } @@ -349,10 +349,8 @@ public void createKeyspaceWithSpecialCharacter() throws Exception { @Test public void rejectsInjectionInDataCenterName() throws Exception { - // A datacenter key with characters outside the API allowlist is rejected up front. - // The driver's SchemaBuilder.withNetworkTopologyStrategy(Map) does NOT escape map keys - // (see OptionsUtils.extractOptionValue in java-driver-query-builder), so this allowlist - // is the actual security control for the replication map. + // The driver does not escape NetworkTopologyStrategy map keys, so reject names that could + // break out of the generated CQL string literal. String json = """ { @@ -375,12 +373,12 @@ public void rejectsInjectionInDataCenterName() throws Exception { throwable, SchemaException.Code.UNSUPPORTED_REPLICATION_DATA_CENTER_NAME, "data center name in the NetworkTopologyStrategy replication options that is not supported", - "must not be empty, more than 48 characters long, and may contain only alphanumeric, underscore, and hyphen characters", + "must not be empty or contain single quote characters", "The command used the unsupported data center name: 'dc1', 'class': 'SimpleStrategy'."); } @Test - public void rejectsDataCenterNameWithSpace() throws Exception { + public void allowsDataCenterNameWithSpace() throws Exception { String json = """ { @@ -397,14 +395,16 @@ public void rejectsDataCenterNameWithSpace() throws Exception { """; CreateNamespaceCommand command = objectMapper.readValue(json, CreateNamespaceCommand.class); - Throwable throwable = catchThrowable(() -> resolver.resolveCommand(commandContext, command)); + Operation result = resolver.resolveCommand(commandContext, command); - verifySchemaException( - throwable, SchemaException.Code.UNSUPPORTED_REPLICATION_DATA_CENTER_NAME, "dc one"); + assertThat(result) + .isInstanceOfSatisfying( + CreateKeyspaceOperation.class, + op -> assertThat(op.strategyOptions()).containsEntry("dc one", 1)); } @Test - public void rejectsDataCenterNameTooLong() throws Exception { + public void allowsDataCenterNameWithoutLengthLimit() throws Exception { String dcName = RandomStringUtils.insecure().nextAlphabetic(49); String json = """ @@ -423,10 +423,12 @@ public void rejectsDataCenterNameTooLong() throws Exception { .formatted(dcName); CreateNamespaceCommand command = objectMapper.readValue(json, CreateNamespaceCommand.class); - Throwable throwable = catchThrowable(() -> resolver.resolveCommand(commandContext, command)); + Operation result = resolver.resolveCommand(commandContext, command); - verifySchemaException( - throwable, SchemaException.Code.UNSUPPORTED_REPLICATION_DATA_CENTER_NAME, dcName); + assertThat(result) + .isInstanceOfSatisfying( + CreateKeyspaceOperation.class, + op -> assertThat(op.strategyOptions()).containsEntry(dcName, 1)); } } diff --git a/src/test/java/io/stargate/sgv2/jsonapi/service/resolver/DropKeyspaceCommandResolverTest.java b/src/test/java/io/stargate/sgv2/jsonapi/service/resolver/DropKeyspaceCommandResolverTest.java index a1b6ab8dbd..612cd1dd89 100644 --- a/src/test/java/io/stargate/sgv2/jsonapi/service/resolver/DropKeyspaceCommandResolverTest.java +++ b/src/test/java/io/stargate/sgv2/jsonapi/service/resolver/DropKeyspaceCommandResolverTest.java @@ -1,13 +1,16 @@ package io.stargate.sgv2.jsonapi.service.resolver; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.catchThrowable; import com.fasterxml.jackson.databind.ObjectMapper; import io.quarkus.test.junit.QuarkusTest; import io.quarkus.test.junit.TestProfile; import io.stargate.sgv2.jsonapi.TestConstants; import io.stargate.sgv2.jsonapi.api.model.command.CommandContext; +import io.stargate.sgv2.jsonapi.api.model.command.impl.DropKeyspaceCommand; import io.stargate.sgv2.jsonapi.api.model.command.impl.DropNamespaceCommand; +import io.stargate.sgv2.jsonapi.exception.SchemaException; import io.stargate.sgv2.jsonapi.service.operation.Operation; import io.stargate.sgv2.jsonapi.service.operation.keyspaces.DropKeyspaceOperation; import io.stargate.sgv2.jsonapi.service.schema.DatabaseSchemaObject; @@ -22,6 +25,7 @@ class DropKeyspaceCommandResolverTest { @Inject ObjectMapper objectMapper; @Inject DropNamespaceCommandResolver resolver; + @Inject DropKeyspaceCommandResolver dropKeyspaceResolver; private final TestConstants testConstants = new TestConstants(); CommandContext commandContext; @@ -48,6 +52,55 @@ public void happyPath() throws Exception { assertThat(result) .isInstanceOfSatisfying( DropKeyspaceOperation.class, - op -> assertThat(op.name()).isEqualTo("red_star_belgrade")); + op -> assertThat(op.name().asInternal()).isEqualTo("red_star_belgrade")); + } + + @Test + public void dropKeyspaceHappyPath() throws Exception { + String json = + """ + { + "dropKeyspace": { + "name" : "red_star_belgrade" + } + } + """; + + DropKeyspaceCommand command = objectMapper.readValue(json, DropKeyspaceCommand.class); + Operation result = dropKeyspaceResolver.resolveCommand(commandContext, command); + + assertThat(result) + .isInstanceOfSatisfying( + DropKeyspaceOperation.class, + op -> assertThat(op.name().asInternal()).isEqualTo("red_star_belgrade")); + } + + @Test + public void dropKeyspaceAllowsLongWordName() { + String name = "a".repeat(64); + var command = new DropKeyspaceCommand(name); + + Operation result = dropKeyspaceResolver.resolveCommand(commandContext, command); + + assertThat(result) + .isInstanceOfSatisfying( + DropKeyspaceOperation.class, op -> assertThat(op.name().asInternal()).isEqualTo(name)); + } + + @Test + public void dropKeyspaceRejectsNonWordName() { + var command = new DropKeyspaceCommand("bad-name"); + + Throwable throwable = + catchThrowable(() -> dropKeyspaceResolver.resolveCommand(commandContext, command)); + + assertThat(throwable) + .isInstanceOf(SchemaException.class) + .satisfies( + e -> { + SchemaException exception = (SchemaException) e; + assertThat(exception.code).isEqualTo(SchemaException.Code.INVALID_KEYSPACE.name()); + assertThat(exception.getMessage()).contains("bad-name"); + }); } } From 765e18dae4729992c860dd327916b9dff88dd720 Mon Sep 17 00:00:00 2001 From: Eric Hare Date: Tue, 19 May 2026 14:29:59 -0700 Subject: [PATCH 3/5] Fix linting --- .../operation/keyspaces/CreateKeyspaceOperation.java | 3 ++- .../service/resolver/CreateNamespaceCommandResolver.java | 3 +-- .../resolver/CreateNamespaceKeyspaceCommandResolver.java | 7 +++---- 3 files changed, 6 insertions(+), 7 deletions(-) diff --git a/src/main/java/io/stargate/sgv2/jsonapi/service/operation/keyspaces/CreateKeyspaceOperation.java b/src/main/java/io/stargate/sgv2/jsonapi/service/operation/keyspaces/CreateKeyspaceOperation.java index d73a562ae1..c51204d736 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/service/operation/keyspaces/CreateKeyspaceOperation.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/service/operation/keyspaces/CreateKeyspaceOperation.java @@ -22,7 +22,8 @@ * datacenter names before creating this operation. */ public record CreateKeyspaceOperation( - CqlIdentifier name, String strategy, Map strategyOptions) implements Operation { + CqlIdentifier name, String strategy, Map strategyOptions) + implements Operation { private static final String NETWORK_TOPOLOGY_STRATEGY = "NetworkTopologyStrategy"; private static final int DEFAULT_REPLICATION_FACTOR = 1; diff --git a/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/CreateNamespaceCommandResolver.java b/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/CreateNamespaceCommandResolver.java index de48954315..461911f690 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/CreateNamespaceCommandResolver.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/CreateNamespaceCommandResolver.java @@ -30,8 +30,7 @@ public Operation resolveDatabaseCommand( var replication = getOrDefault(command.options(), CreateNamespaceCommand.Options::replication, null); - String strategy = - getOrDefault(replication, CreateNamespaceCommand.Replication::strategy, null); + String strategy = getOrDefault(replication, CreateNamespaceCommand.Replication::strategy, null); Map strategyOptions = getOrDefault(replication, CreateNamespaceCommand.Replication::strategyOptions, null); diff --git a/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/CreateNamespaceKeyspaceCommandResolver.java b/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/CreateNamespaceKeyspaceCommandResolver.java index 06448ac885..d1d14a7335 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/CreateNamespaceKeyspaceCommandResolver.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/CreateNamespaceKeyspaceCommandResolver.java @@ -11,7 +11,8 @@ public abstract class CreateNamespaceKeyspaceCommandResolver private static final String NETWORK_TOPOLOGY_STRATEGY = "NetworkTopologyStrategy"; - // The driver writes NetworkTopologyStrategy map keys as CQL string literals without escaping them. + // The driver writes NetworkTopologyStrategy map keys as CQL string literals without escaping + // them. private static final Pattern VALID_DATA_CENTER_NAME = Pattern.compile("[^']+"); protected static void validateStrategyOptions( @@ -31,9 +32,7 @@ private static boolean isNetworkTopologyStrategy(String strategy) { private static void checkDataCenterName(String name) { if (name == null || !VALID_DATA_CENTER_NAME.matcher(name).matches()) { throw SchemaException.Code.UNSUPPORTED_REPLICATION_DATA_CENTER_NAME.get( - Map.of( - "unsupportedDataCenterName", - ErrorTemplate.replaceIfNull(name))); + Map.of("unsupportedDataCenterName", ErrorTemplate.replaceIfNull(name))); } } } From 640905d561a29fb95f2278b1e4520740c0e77097 Mon Sep 17 00:00:00 2001 From: Eric Hare Date: Wed, 20 May 2026 17:50:39 -0700 Subject: [PATCH 4/5] fix: Address @amorton review comments --- .../command/impl/CreateKeyspaceCommand.java | 2 +- .../command/impl/CreateNamespaceCommand.java | 2 +- .../jsonapi/exception/SchemaException.java | 2 +- .../keyspaces/CreateKeyspaceOperation.java | 24 ++++++--- .../CreateKeyspaceCommandResolver.java | 3 +- .../CreateNamespaceCommandResolver.java | 3 +- ...reateNamespaceKeyspaceCommandResolver.java | 38 -------------- .../resolver/DropKeyspaceCommandResolver.java | 4 +- .../DropNamespaceCommandResolver.java | 5 +- .../KeyspaceCommandResolverSupport.java | 29 ----------- .../resolver/KeyspaceDDLCommandResolver.java | 52 +++++++++++++++++++ .../schema/naming/SchemaObjectNamingRule.java | 48 ++++++++++++----- src/main/resources/errors.yaml | 12 ++--- .../impl/CreateKeyspaceCommandTest.java | 18 ++++--- .../CreateKeyspaceOperationTest.java | 36 ++++++++++++- .../CreateKeyspaceCommandResolverTest.java | 27 ++++++++-- .../DropKeyspaceCommandResolverTest.java | 3 +- .../service/schema/NamingRulesTests.java | 36 +++++++++++++ 18 files changed, 227 insertions(+), 117 deletions(-) delete mode 100644 src/main/java/io/stargate/sgv2/jsonapi/service/resolver/CreateNamespaceKeyspaceCommandResolver.java delete mode 100644 src/main/java/io/stargate/sgv2/jsonapi/service/resolver/KeyspaceCommandResolverSupport.java create mode 100644 src/main/java/io/stargate/sgv2/jsonapi/service/resolver/KeyspaceDDLCommandResolver.java diff --git a/src/main/java/io/stargate/sgv2/jsonapi/api/model/command/impl/CreateKeyspaceCommand.java b/src/main/java/io/stargate/sgv2/jsonapi/api/model/command/impl/CreateKeyspaceCommand.java index 45897c4050..1c2da90054 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/api/model/command/impl/CreateKeyspaceCommand.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/api/model/command/impl/CreateKeyspaceCommand.java @@ -39,7 +39,7 @@ public record Options(@Nullable @Valid Replication replication) {} + "For NetworkTopologyStrategy, use {\"class\": \"NetworkTopologyStrategy\", \"datacenter_name\": N, ...}.") public record Replication( @NotNull - @Pattern(regexp = "(?i)(SimpleStrategy|NetworkTopologyStrategy)") + @Pattern(regexp = "(SimpleStrategy|NetworkTopologyStrategy)") @JsonProperty("class") @Schema( description = diff --git a/src/main/java/io/stargate/sgv2/jsonapi/api/model/command/impl/CreateNamespaceCommand.java b/src/main/java/io/stargate/sgv2/jsonapi/api/model/command/impl/CreateNamespaceCommand.java index 40dcd421d4..bb14bc742c 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/api/model/command/impl/CreateNamespaceCommand.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/api/model/command/impl/CreateNamespaceCommand.java @@ -53,7 +53,7 @@ public record Options(@Nullable @Valid Replication replication) {} deprecated = true) public record Replication( @NotNull() - @Pattern(regexp = "(?i)(SimpleStrategy|NetworkTopologyStrategy)") + @Pattern(regexp = "(SimpleStrategy|NetworkTopologyStrategy)") @JsonProperty("class") @Schema( description = diff --git a/src/main/java/io/stargate/sgv2/jsonapi/exception/SchemaException.java b/src/main/java/io/stargate/sgv2/jsonapi/exception/SchemaException.java index 06a22cb98e..37f99b0eca 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/exception/SchemaException.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/exception/SchemaException.java @@ -45,6 +45,7 @@ public enum Code implements ErrorCode { INVALID_CREATE_COLLECTION_OPTIONS, INVALID_FORMAT_FOR_INDEX_CREATION_COLUMN, INVALID_INDEXING_DEFINITION, + INVALID_REPLICATION_DATA_CENTER_NAME, INVALID_USAGE_OF_VECTORIZE, // legacy: converted from ErrorCodeV1 INVALID_USER_DEFINED_TYPE_NAME, LEXICAL_NOT_AVAILABLE_FOR_DATABASE, @@ -85,7 +86,6 @@ public enum Code implements ErrorCode { UNSUPPORTED_JSON_TYPE_FOR_TEXT_INDEX, UNSUPPORTED_LIST_DEFINITION, UNSUPPORTED_MAP_DEFINITION, - UNSUPPORTED_REPLICATION_DATA_CENTER_NAME, UNSUPPORTED_SCHEMA_NAME, UNSUPPORTED_SET_DEFINITION, UNSUPPORTED_TEXT_ANALYSIS_FOR_DATA_TYPES, diff --git a/src/main/java/io/stargate/sgv2/jsonapi/service/operation/keyspaces/CreateKeyspaceOperation.java b/src/main/java/io/stargate/sgv2/jsonapi/service/operation/keyspaces/CreateKeyspaceOperation.java index c51204d736..5b27886739 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/service/operation/keyspaces/CreateKeyspaceOperation.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/service/operation/keyspaces/CreateKeyspaceOperation.java @@ -40,18 +40,30 @@ public Uni> execute( private SimpleStatement buildStatement() { CreateKeyspaceStart start = SchemaBuilder.createKeyspace(name).ifNotExists(); + Map safeStrategyOptions = strategyOptionsOrEmpty(); CreateKeyspace withReplication; - if (NETWORK_TOPOLOGY_STRATEGY.equalsIgnoreCase(strategy)) { - withReplication = - start.withNetworkTopologyStrategy(strategyOptions != null ? strategyOptions : Map.of()); + if (NETWORK_TOPOLOGY_STRATEGY.equals(strategy)) { + withReplication = start.withNetworkTopologyStrategy(safeStrategyOptions); } else { int replicationFactor = - (strategyOptions != null) - ? strategyOptions.getOrDefault("replication_factor", DEFAULT_REPLICATION_FACTOR) - : DEFAULT_REPLICATION_FACTOR; + safeStrategyOptions.getOrDefault("replication_factor", DEFAULT_REPLICATION_FACTOR); withReplication = start.withSimpleStrategy(replicationFactor); } return withReplication.build(); } + + private Map strategyOptionsOrEmpty() { + if (strategyOptions == null) { + return Map.of(); + } + strategyOptions.forEach( + (key, value) -> { + if (value == null) { + throw new IllegalArgumentException( + "Replication strategy option value must not be null for key '%s'".formatted(key)); + } + }); + return strategyOptions; + } } diff --git a/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/CreateKeyspaceCommandResolver.java b/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/CreateKeyspaceCommandResolver.java index 5b0425380f..05bdc01c1b 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/CreateKeyspaceCommandResolver.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/CreateKeyspaceCommandResolver.java @@ -1,6 +1,5 @@ package io.stargate.sgv2.jsonapi.service.resolver; -import static io.stargate.sgv2.jsonapi.service.resolver.KeyspaceCommandResolverSupport.keyspaceIdentifierForCreate; import static io.stargate.sgv2.jsonapi.util.ApiOptionUtils.getOrDefault; import io.stargate.sgv2.jsonapi.api.model.command.CommandContext; @@ -14,7 +13,7 @@ /** Command resolver for {@link CreateKeyspaceCommand}. */ @ApplicationScoped public class CreateKeyspaceCommandResolver - extends CreateNamespaceKeyspaceCommandResolver { + extends KeyspaceDDLCommandResolver { @Override public Class getCommandClass() { diff --git a/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/CreateNamespaceCommandResolver.java b/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/CreateNamespaceCommandResolver.java index 461911f690..726f31e89f 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/CreateNamespaceCommandResolver.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/CreateNamespaceCommandResolver.java @@ -1,6 +1,5 @@ package io.stargate.sgv2.jsonapi.service.resolver; -import static io.stargate.sgv2.jsonapi.service.resolver.KeyspaceCommandResolverSupport.keyspaceIdentifierForCreate; import static io.stargate.sgv2.jsonapi.util.ApiOptionUtils.getOrDefault; import io.stargate.sgv2.jsonapi.api.model.command.CommandContext; @@ -14,7 +13,7 @@ /** Command resolver for {@link CreateNamespaceCommand}. */ @ApplicationScoped public class CreateNamespaceCommandResolver - extends CreateNamespaceKeyspaceCommandResolver { + extends KeyspaceDDLCommandResolver { @Override public Class getCommandClass() { diff --git a/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/CreateNamespaceKeyspaceCommandResolver.java b/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/CreateNamespaceKeyspaceCommandResolver.java deleted file mode 100644 index d1d14a7335..0000000000 --- a/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/CreateNamespaceKeyspaceCommandResolver.java +++ /dev/null @@ -1,38 +0,0 @@ -package io.stargate.sgv2.jsonapi.service.resolver; - -import io.stargate.sgv2.jsonapi.api.model.command.Command; -import io.stargate.sgv2.jsonapi.exception.ErrorTemplate; -import io.stargate.sgv2.jsonapi.exception.SchemaException; -import java.util.Map; -import java.util.regex.Pattern; - -public abstract class CreateNamespaceKeyspaceCommandResolver - implements CommandResolver { - - private static final String NETWORK_TOPOLOGY_STRATEGY = "NetworkTopologyStrategy"; - - // The driver writes NetworkTopologyStrategy map keys as CQL string literals without escaping - // them. - private static final Pattern VALID_DATA_CENTER_NAME = Pattern.compile("[^']+"); - - protected static void validateStrategyOptions( - String strategy, Map strategyOptions) { - if (!isNetworkTopologyStrategy(strategy) || strategyOptions == null) { - return; - } - for (String dcName : strategyOptions.keySet()) { - checkDataCenterName(dcName); - } - } - - private static boolean isNetworkTopologyStrategy(String strategy) { - return NETWORK_TOPOLOGY_STRATEGY.equalsIgnoreCase(strategy); - } - - private static void checkDataCenterName(String name) { - if (name == null || !VALID_DATA_CENTER_NAME.matcher(name).matches()) { - throw SchemaException.Code.UNSUPPORTED_REPLICATION_DATA_CENTER_NAME.get( - Map.of("unsupportedDataCenterName", ErrorTemplate.replaceIfNull(name))); - } - } -} diff --git a/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/DropKeyspaceCommandResolver.java b/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/DropKeyspaceCommandResolver.java index f87767e810..25649900c2 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/DropKeyspaceCommandResolver.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/DropKeyspaceCommandResolver.java @@ -1,7 +1,5 @@ package io.stargate.sgv2.jsonapi.service.resolver; -import static io.stargate.sgv2.jsonapi.service.resolver.KeyspaceCommandResolverSupport.keyspaceIdentifierForDrop; - import io.stargate.sgv2.jsonapi.api.model.command.CommandContext; import io.stargate.sgv2.jsonapi.api.model.command.impl.DropKeyspaceCommand; import io.stargate.sgv2.jsonapi.service.operation.Operation; @@ -11,7 +9,7 @@ /** Command resolver for {@link DropKeyspaceCommand}. */ @ApplicationScoped -public class DropKeyspaceCommandResolver implements CommandResolver { +public class DropKeyspaceCommandResolver extends KeyspaceDDLCommandResolver { /** {@inheritDoc} */ @Override diff --git a/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/DropNamespaceCommandResolver.java b/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/DropNamespaceCommandResolver.java index e7333e74fe..55637b336c 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/DropNamespaceCommandResolver.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/DropNamespaceCommandResolver.java @@ -1,7 +1,5 @@ package io.stargate.sgv2.jsonapi.service.resolver; -import static io.stargate.sgv2.jsonapi.service.resolver.KeyspaceCommandResolverSupport.keyspaceIdentifierForDrop; - import io.stargate.sgv2.jsonapi.api.model.command.CommandContext; import io.stargate.sgv2.jsonapi.api.model.command.impl.DropNamespaceCommand; import io.stargate.sgv2.jsonapi.service.operation.Operation; @@ -11,7 +9,8 @@ /** Command resolver for {@link DropNamespaceCommand}. */ @ApplicationScoped -public class DropNamespaceCommandResolver implements CommandResolver { +public class DropNamespaceCommandResolver + extends KeyspaceDDLCommandResolver { /** {@inheritDoc} */ @Override diff --git a/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/KeyspaceCommandResolverSupport.java b/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/KeyspaceCommandResolverSupport.java deleted file mode 100644 index a377d76be4..0000000000 --- a/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/KeyspaceCommandResolverSupport.java +++ /dev/null @@ -1,29 +0,0 @@ -package io.stargate.sgv2.jsonapi.service.resolver; - -import static io.stargate.sgv2.jsonapi.util.CqlIdentifierUtil.cqlIdentifierFromUserInput; - -import com.datastax.oss.driver.api.core.CqlIdentifier; -import io.stargate.sgv2.jsonapi.exception.ErrorTemplate; -import io.stargate.sgv2.jsonapi.exception.SchemaException; -import io.stargate.sgv2.jsonapi.service.schema.naming.NamingRules; -import java.util.Map; -import java.util.regex.Pattern; - -final class KeyspaceCommandResolverSupport { - - private static final Pattern DROP_KEYSPACE_NAME = Pattern.compile("\\w+"); - - private KeyspaceCommandResolverSupport() {} - - static CqlIdentifier keyspaceIdentifierForCreate(String name) { - return cqlIdentifierFromUserInput(NamingRules.KEYSPACE.checkRule(name)); - } - - static CqlIdentifier keyspaceIdentifierForDrop(String name) { - if (name == null || !DROP_KEYSPACE_NAME.matcher(name).matches()) { - throw SchemaException.Code.INVALID_KEYSPACE.get( - Map.of("keyspace", ErrorTemplate.replaceIfNull(name))); - } - return cqlIdentifierFromUserInput(name); - } -} diff --git a/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/KeyspaceDDLCommandResolver.java b/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/KeyspaceDDLCommandResolver.java new file mode 100644 index 0000000000..bb61e2c6db --- /dev/null +++ b/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/KeyspaceDDLCommandResolver.java @@ -0,0 +1,52 @@ +package io.stargate.sgv2.jsonapi.service.resolver; + +import static io.stargate.sgv2.jsonapi.util.CqlIdentifierUtil.cqlIdentifierFromUserInput; + +import com.datastax.oss.driver.api.core.CqlIdentifier; +import io.stargate.sgv2.jsonapi.api.model.command.Command; +import io.stargate.sgv2.jsonapi.exception.ErrorTemplate; +import io.stargate.sgv2.jsonapi.exception.SchemaException; +import io.stargate.sgv2.jsonapi.service.schema.naming.NamingRules; +import java.util.Map; +import java.util.regex.Pattern; + +public abstract class KeyspaceDDLCommandResolver implements CommandResolver { + + private static final String NETWORK_TOPOLOGY_STRATEGY = "NetworkTopologyStrategy"; + + // NetworkTopologyStrategy uses replication map option keys as data center names. Cassandra + // validates those keys semantically against known data centers; they are not CQL identifiers. + // The Java driver query builder renders replication map keys as single-quoted CQL string + // literals, appending the raw key without escaping it. Because the CQL delimiter in this position + // is the ASCII single quote, reject that character and leave non-delimiters such as double quotes, + // backticks, and curly quotes to Cassandra's own data-center validation. + private static final Pattern VALID_DATA_CENTER_NAME = Pattern.compile("^[^']+$"); + + protected CqlIdentifier keyspaceIdentifierForCreate(String name) { + return cqlIdentifierFromUserInput(NamingRules.KEYSPACE.checkRule(name)); + } + + protected CqlIdentifier keyspaceIdentifierForDrop(String name) { + return cqlIdentifierFromUserInput(NamingRules.KEYSPACE.sanitizeInput(name)); + } + + protected void validateStrategyOptions(String strategy, Map strategyOptions) { + if (!isNetworkTopologyStrategy(strategy) || strategyOptions == null) { + return; + } + for (String dcName : strategyOptions.keySet()) { + checkDataCenterName(dcName); + } + } + + private boolean isNetworkTopologyStrategy(String strategy) { + return NETWORK_TOPOLOGY_STRATEGY.equals(strategy); + } + + private void checkDataCenterName(String name) { + if (name == null || !VALID_DATA_CENTER_NAME.matcher(name).matches()) { + throw SchemaException.Code.INVALID_REPLICATION_DATA_CENTER_NAME.get( + Map.of("invalidDataCenterName", ErrorTemplate.replaceIfNull(name))); + } + } +} diff --git a/src/main/java/io/stargate/sgv2/jsonapi/service/schema/naming/SchemaObjectNamingRule.java b/src/main/java/io/stargate/sgv2/jsonapi/service/schema/naming/SchemaObjectNamingRule.java index 0a4d9cea3d..a7362babae 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/service/schema/naming/SchemaObjectNamingRule.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/service/schema/naming/SchemaObjectNamingRule.java @@ -45,10 +45,7 @@ public SchemaObjectType schemaType() { * @return true if the name is valid, false otherwise */ public boolean apply(String name) { - return name != null - && !name.isEmpty() - && name.length() <= getMaxLength() - && PATTERN_WORD_CHARS.matcher(name).matches(); + return hasAllowedCharacters(name) && name.length() <= getMaxLength(); } /** @@ -58,6 +55,25 @@ public int getMaxLength() { return MAX_NAME_LENGTH; } + /** + * Validates user input without applying the Data API length limit. + * + *

This is useful for schema objects that may have been created outside the Data API, where the + * backing database allows longer names but the input still needs to be safe to pass through the + * API. + * + * @param name The name to validate. + * @return The validated name. + * @throws SchemaException with {@link SchemaException.Code#UNSUPPORTED_SCHEMA_NAME} if the name + * is null, empty, or contains unsupported characters. + */ + public String sanitizeInput(String name) { + if (!hasAllowedCharacters(name)) { + throw unsupportedName(name); + } + return name; + } + /** * Validate the name against the naming rule, and throw a {@link SchemaException} if the name is * invalid. @@ -70,15 +86,23 @@ public int getMaxLength() { public String checkRule(String name) { if (!apply(name)) { - throw SchemaException.Code.UNSUPPORTED_SCHEMA_NAME.get( - Map.of( - "schemaType", - schemaType().apiName(), - "maxNameLength", - String.valueOf(getMaxLength()), - "unsupportedSchemaName", - ErrorTemplate.replaceIfNull(name))); + throw unsupportedName(name); } return name; } + + private boolean hasAllowedCharacters(String name) { + return name != null && PATTERN_WORD_CHARS.matcher(name).matches(); + } + + private SchemaException unsupportedName(String name) { + return SchemaException.Code.UNSUPPORTED_SCHEMA_NAME.get( + Map.of( + "schemaType", + schemaType().apiName(), + "maxNameLength", + String.valueOf(getMaxLength()), + "unsupportedSchemaName", + ErrorTemplate.replaceIfNull(name))); + } } diff --git a/src/main/resources/errors.yaml b/src/main/resources/errors.yaml index 4c346b0b53..4ca3ad01ee 100644 --- a/src/main/resources/errors.yaml +++ b/src/main/resources/errors.yaml @@ -1625,15 +1625,15 @@ request-errors: Resend the command using a supported value type. - scope: SCHEMA - code: UNSUPPORTED_REPLICATION_DATA_CENTER_NAME - title: The used data center name in the replication options is not supported + code: INVALID_REPLICATION_DATA_CENTER_NAME + title: The used data center name in the replication options is not valid body: |- - The command included a data center name in the NetworkTopologyStrategy replication options that is not supported. + The command included a data center name in the NetworkTopologyStrategy replication options that is not valid. - Supported data center names must not be empty or contain single quote characters. - The command used the unsupported data center name: '${unsupportedDataCenterName}'. + Valid data center names must not be empty or contain ASCII single quote characters. + The command used the invalid data center name: '${invalidDataCenterName}'. - Resend the command using a supported data center name. + Resend the command using a valid data center name. - scope: SCHEMA code: UNSUPPORTED_SCHEMA_NAME diff --git a/src/test/java/io/stargate/sgv2/jsonapi/api/model/command/impl/CreateKeyspaceCommandTest.java b/src/test/java/io/stargate/sgv2/jsonapi/api/model/command/impl/CreateKeyspaceCommandTest.java index 41d0e42ab4..6df6bfec05 100644 --- a/src/test/java/io/stargate/sgv2/jsonapi/api/model/command/impl/CreateKeyspaceCommandTest.java +++ b/src/test/java/io/stargate/sgv2/jsonapi/api/model/command/impl/CreateKeyspaceCommandTest.java @@ -113,11 +113,11 @@ public void strategyWrong() throws Exception { assertThat(result) .isNotEmpty() .extracting(ConstraintViolation::getMessage) - .contains("must match \"(?i)(SimpleStrategy|NetworkTopologyStrategy)\""); + .contains("must match \"(SimpleStrategy|NetworkTopologyStrategy)\""); } @Test - public void strategyPatternIsCaseInsensitive() throws Exception { + public void strategyPatternIsCaseSensitive() throws Exception { String json = """ { @@ -135,7 +135,10 @@ public void strategyPatternIsCaseInsensitive() throws Exception { CreateKeyspaceCommand command = objectMapper.readValue(json, CreateKeyspaceCommand.class); Set> result = validator.validate(command); - assertThat(result).isEmpty(); + assertThat(result) + .isNotEmpty() + .extracting(ConstraintViolation::getMessage) + .contains("must match \"(SimpleStrategy|NetworkTopologyStrategy)\""); } } @@ -231,11 +234,11 @@ public void strategyWrong() throws Exception { assertThat(result) .isNotEmpty() .extracting(ConstraintViolation::getMessage) - .contains("must match \"(?i)(SimpleStrategy|NetworkTopologyStrategy)\""); + .contains("must match \"(SimpleStrategy|NetworkTopologyStrategy)\""); } @Test - public void strategyPatternIsCaseInsensitive() throws Exception { + public void strategyPatternIsCaseSensitive() throws Exception { String json = """ { @@ -253,7 +256,10 @@ public void strategyPatternIsCaseInsensitive() throws Exception { CreateNamespaceCommand command = objectMapper.readValue(json, CreateNamespaceCommand.class); Set> result = validator.validate(command); - assertThat(result).isEmpty(); + assertThat(result) + .isNotEmpty() + .extracting(ConstraintViolation::getMessage) + .contains("must match \"(SimpleStrategy|NetworkTopologyStrategy)\""); } } } diff --git a/src/test/java/io/stargate/sgv2/jsonapi/service/operation/keyspaces/CreateKeyspaceOperationTest.java b/src/test/java/io/stargate/sgv2/jsonapi/service/operation/keyspaces/CreateKeyspaceOperationTest.java index 5b44024f38..9a7aef1983 100644 --- a/src/test/java/io/stargate/sgv2/jsonapi/service/operation/keyspaces/CreateKeyspaceOperationTest.java +++ b/src/test/java/io/stargate/sgv2/jsonapi/service/operation/keyspaces/CreateKeyspaceOperationTest.java @@ -1,6 +1,7 @@ package io.stargate.sgv2.jsonapi.service.operation.keyspaces; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.catchThrowable; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; @@ -13,6 +14,7 @@ import io.smallrye.mutiny.Uni; import io.stargate.sgv2.jsonapi.api.request.RequestContext; import io.stargate.sgv2.jsonapi.service.cqldriver.executor.QueryExecutor; +import java.util.Collections; import java.util.Map; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; @@ -53,6 +55,21 @@ public void unknownStrategyFallsBackToSimple() { String cql = createCql(op); assertThat(cql).contains("'class':'SimpleStrategy'"); } + + @Test + public void rejectsNullReplicationFactorValue() { + var op = + new CreateKeyspaceOperation( + identifier("ks"), + "SimpleStrategy", + Collections.singletonMap("replication_factor", null)); + + Throwable throwable = catchThrowable(() -> createCql(op)); + + assertThat(throwable) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("replication_factor"); + } } @Nested @@ -75,12 +92,27 @@ public void allowsEmptyDataCenterMap() { } @Test - public void strategyNameIsCaseInsensitive() { + public void strategyNameIsCaseSensitive() { var op = new CreateKeyspaceOperation( identifier("ks"), "networktopologystrategy", Map.of("dc1", 3)); String cql = createCql(op); - assertThat(cql).contains("'class':'NetworkTopologyStrategy'").contains("'dc1':3"); + assertThat(cql).contains("'class':'SimpleStrategy'").doesNotContain("'dc1':3"); + } + + @Test + public void rejectsNullDataCenterReplicationFactorValue() { + var op = + new CreateKeyspaceOperation( + identifier("ks"), + "NetworkTopologyStrategy", + Collections.singletonMap("dc1", null)); + + Throwable throwable = catchThrowable(() -> createCql(op)); + + assertThat(throwable) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("dc1"); } } diff --git a/src/test/java/io/stargate/sgv2/jsonapi/service/resolver/CreateKeyspaceCommandResolverTest.java b/src/test/java/io/stargate/sgv2/jsonapi/service/resolver/CreateKeyspaceCommandResolverTest.java index dd24663e71..09bad9b36a 100644 --- a/src/test/java/io/stargate/sgv2/jsonapi/service/resolver/CreateKeyspaceCommandResolverTest.java +++ b/src/test/java/io/stargate/sgv2/jsonapi/service/resolver/CreateKeyspaceCommandResolverTest.java @@ -15,6 +15,7 @@ import io.stargate.sgv2.jsonapi.service.schema.DatabaseSchemaObject; import io.stargate.sgv2.jsonapi.testresource.NoGlobalResourcesTestProfile; import jakarta.inject.Inject; +import java.util.Map; import org.apache.commons.lang3.RandomStringUtils; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Nested; @@ -371,10 +372,10 @@ public void rejectsInjectionInDataCenterName() throws Exception { verifySchemaException( throwable, - SchemaException.Code.UNSUPPORTED_REPLICATION_DATA_CENTER_NAME, - "data center name in the NetworkTopologyStrategy replication options that is not supported", - "must not be empty or contain single quote characters", - "The command used the unsupported data center name: 'dc1', 'class': 'SimpleStrategy'."); + SchemaException.Code.INVALID_REPLICATION_DATA_CENTER_NAME, + "data center name in the NetworkTopologyStrategy replication options that is not valid", + "must not be empty or contain ASCII single quote characters", + "The command used the invalid data center name: 'dc1', 'class': 'SimpleStrategy'."); } @Test @@ -464,6 +465,24 @@ public void hyphenatedDataCenterNameIsAllowed() throws Exception { assertThat(op.strategyOptions()).containsEntry("us-east-1", 3); }); } + + @Test + public void onlyAsciiSingleQuoteIsRejectedAsCqlStringDelimiter() { + String dcName = "dc`\"\u2018\u2019\u201C\u201D"; + CreateNamespaceCommand command = + new CreateNamespaceCommand( + "red_star_belgrade", + new CreateNamespaceCommand.Options( + new CreateNamespaceCommand.Replication( + "NetworkTopologyStrategy", Map.of(dcName, 1)))); + + Operation result = resolver.resolveCommand(commandContext, command); + + assertThat(result) + .isInstanceOfSatisfying( + CreateKeyspaceOperation.class, + op -> assertThat(op.strategyOptions()).containsEntry(dcName, 1)); + } } private void verifySchemaException( diff --git a/src/test/java/io/stargate/sgv2/jsonapi/service/resolver/DropKeyspaceCommandResolverTest.java b/src/test/java/io/stargate/sgv2/jsonapi/service/resolver/DropKeyspaceCommandResolverTest.java index 612cd1dd89..93c7acd730 100644 --- a/src/test/java/io/stargate/sgv2/jsonapi/service/resolver/DropKeyspaceCommandResolverTest.java +++ b/src/test/java/io/stargate/sgv2/jsonapi/service/resolver/DropKeyspaceCommandResolverTest.java @@ -99,7 +99,8 @@ public void dropKeyspaceRejectsNonWordName() { .satisfies( e -> { SchemaException exception = (SchemaException) e; - assertThat(exception.code).isEqualTo(SchemaException.Code.INVALID_KEYSPACE.name()); + assertThat(exception.code) + .isEqualTo(SchemaException.Code.UNSUPPORTED_SCHEMA_NAME.name()); assertThat(exception.getMessage()).contains("bad-name"); }); } diff --git a/src/test/java/io/stargate/sgv2/jsonapi/service/schema/NamingRulesTests.java b/src/test/java/io/stargate/sgv2/jsonapi/service/schema/NamingRulesTests.java index 3b0ec43607..216f11085b 100644 --- a/src/test/java/io/stargate/sgv2/jsonapi/service/schema/NamingRulesTests.java +++ b/src/test/java/io/stargate/sgv2/jsonapi/service/schema/NamingRulesTests.java @@ -1,7 +1,9 @@ package io.stargate.sgv2.jsonapi.service.schema; import static org.assertj.core.api.AssertionsForClassTypes.assertThat; +import static org.assertj.core.api.AssertionsForClassTypes.catchThrowable; +import io.stargate.sgv2.jsonapi.exception.SchemaException; import io.stargate.sgv2.jsonapi.service.schema.naming.NamingRules; import io.stargate.sgv2.jsonapi.service.schema.naming.SchemaObjectNamingRule; import java.util.Arrays; @@ -26,6 +28,32 @@ public void schemaObjectNamingValidation( assertThat(namingRule.apply(invalidName)).as(description).isEqualTo(expectedResult); } + @ParameterizedTest + @MethodSource("schemaObjectNamingRules") + public void sanitizeInputAllowsNamesLongerThanMaxLength(SchemaObjectNamingRule namingRule) { + String name = "a".repeat(namingRule.getMaxLength() + 1); + + assertThat(namingRule.sanitizeInput(name)).isEqualTo(name); + } + + @ParameterizedTest + @MethodSource("schemaObjectNamingRules") + public void sanitizeInputRejectsUnsupportedCharacters(SchemaObjectNamingRule namingRule) { + String name = "bad-name"; + + Throwable throwable = catchThrowable(() -> namingRule.sanitizeInput(name)); + + assertThat(throwable) + .isInstanceOf(SchemaException.class) + .satisfies( + e -> { + SchemaException exception = (SchemaException) e; + assertThat(exception.code) + .isEqualTo(SchemaException.Code.UNSUPPORTED_SCHEMA_NAME.name()); + assertThat(exception.getMessage()).contains(name); + }); + } + private static Stream schemaObjectNamingTestCases() { // Define a simple record to encapsulate a test case. record TestCase(String name, boolean expected, String description) {} @@ -80,4 +108,12 @@ record TestCase(String name, boolean expected, String description) {} return Stream.concat(generalTestCases, specialTestCases); } + + private static Stream schemaObjectNamingRules() { + return Stream.of( + Arguments.of(NamingRules.KEYSPACE), + Arguments.of(NamingRules.COLLECTION), + Arguments.of(NamingRules.TABLE), + Arguments.of(NamingRules.INDEX)); + } } From 7a1c4f1cb34cfb6b8233f2582f356bcc34afc68e Mon Sep 17 00:00:00 2001 From: Eric Hare Date: Wed, 20 May 2026 17:54:58 -0700 Subject: [PATCH 5/5] fix: lint issues --- .../service/resolver/DropNamespaceCommandResolver.java | 3 +-- .../jsonapi/service/resolver/KeyspaceDDLCommandResolver.java | 4 ++-- .../operation/keyspaces/CreateKeyspaceOperationTest.java | 4 +--- 3 files changed, 4 insertions(+), 7 deletions(-) diff --git a/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/DropNamespaceCommandResolver.java b/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/DropNamespaceCommandResolver.java index 55637b336c..7a0b227c0b 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/DropNamespaceCommandResolver.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/DropNamespaceCommandResolver.java @@ -9,8 +9,7 @@ /** Command resolver for {@link DropNamespaceCommand}. */ @ApplicationScoped -public class DropNamespaceCommandResolver - extends KeyspaceDDLCommandResolver { +public class DropNamespaceCommandResolver extends KeyspaceDDLCommandResolver { /** {@inheritDoc} */ @Override diff --git a/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/KeyspaceDDLCommandResolver.java b/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/KeyspaceDDLCommandResolver.java index bb61e2c6db..93e8d06bbc 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/KeyspaceDDLCommandResolver.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/KeyspaceDDLCommandResolver.java @@ -17,8 +17,8 @@ public abstract class KeyspaceDDLCommandResolver implements C // NetworkTopologyStrategy uses replication map option keys as data center names. Cassandra // validates those keys semantically against known data centers; they are not CQL identifiers. // The Java driver query builder renders replication map keys as single-quoted CQL string - // literals, appending the raw key without escaping it. Because the CQL delimiter in this position - // is the ASCII single quote, reject that character and leave non-delimiters such as double quotes, + // literals, appending the raw key without escaping it. Because the CQL delimiter here is the + // ASCII single quote, reject only that character. Leave non-delimiters such as double quotes, // backticks, and curly quotes to Cassandra's own data-center validation. private static final Pattern VALID_DATA_CENTER_NAME = Pattern.compile("^[^']+$"); diff --git a/src/test/java/io/stargate/sgv2/jsonapi/service/operation/keyspaces/CreateKeyspaceOperationTest.java b/src/test/java/io/stargate/sgv2/jsonapi/service/operation/keyspaces/CreateKeyspaceOperationTest.java index 9a7aef1983..b55c180ff0 100644 --- a/src/test/java/io/stargate/sgv2/jsonapi/service/operation/keyspaces/CreateKeyspaceOperationTest.java +++ b/src/test/java/io/stargate/sgv2/jsonapi/service/operation/keyspaces/CreateKeyspaceOperationTest.java @@ -104,9 +104,7 @@ public void strategyNameIsCaseSensitive() { public void rejectsNullDataCenterReplicationFactorValue() { var op = new CreateKeyspaceOperation( - identifier("ks"), - "NetworkTopologyStrategy", - Collections.singletonMap("dc1", null)); + identifier("ks"), "NetworkTopologyStrategy", Collections.singletonMap("dc1", null)); Throwable throwable = catchThrowable(() -> createCql(op));