From eabd2a27f5555a50cba6dc8b74d1f246a91081c0 Mon Sep 17 00:00:00 2001 From: omniCoder77 Date: Fri, 30 Jan 2026 09:16:13 +0530 Subject: [PATCH] Add a guardrail for misprepared statements patch by Rishabh Saraswat; reviewed by Stefan Miklosovic, Brad Schoening for CASSANDRA-21139 --- CHANGES.txt | 1 + conf/cassandra.yaml | 7 + conf/cassandra_latest.yaml | 7 + .../org/apache/cassandra/config/Config.java | 2 + .../cassandra/config/GuardrailsOptions.java | 30 ++++ .../apache/cassandra/cql3/CQLStatement.java | 12 ++ .../apache/cassandra/cql3/QueryProcessor.java | 1 + .../cql3/statements/BatchStatement.java | 9 ++ .../statements/ModificationStatement.java | 6 + .../cql3/statements/SelectStatement.java | 6 + .../cassandra/db/guardrails/Guardrails.java | 27 ++++ .../db/guardrails/GuardrailsConfig.java | 26 ++++ .../db/guardrails/GuardrailsMBean.java | 26 ++++ ...tatementParameterRequirementGuardrail.java | 66 +++++++++ .../nodetool/GuardrailsConfigCommand.java | 1 + .../guardrails/MispreparedStatementsTest.java | 126 ++++++++++++++++ .../guardrails/MispreparedStatementsTest.java | 134 ++++++++++++++++++ .../GuardrailsConfigCommandsTest.java | 2 + 18 files changed, 489 insertions(+) create mode 100644 src/java/org/apache/cassandra/db/guardrails/PreparedStatementParameterRequirementGuardrail.java create mode 100644 test/distributed/org/apache/cassandra/distributed/test/guardrails/MispreparedStatementsTest.java create mode 100644 test/unit/org/apache/cassandra/db/guardrails/MispreparedStatementsTest.java diff --git a/CHANGES.txt b/CHANGES.txt index ab74fbc095..b1def717a3 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,4 +1,5 @@ 7.0 + * Add a guardrail for misprepared statements (CASSANDRA-21139) Merged from 6.0: * Ensure schema created before 2.1 without tableId in folder name can be loaded in SnapshotLoader (CASSANDRA-21246) * Differentiate between legitimate cases where the first entry is the same as the last entry and empty bounds in SSTableCursorWriter#addIndexBlock() (CASSANDRA-21255) diff --git a/conf/cassandra.yaml b/conf/cassandra.yaml index af65d54b4b..422ad0a482 100644 --- a/conf/cassandra.yaml +++ b/conf/cassandra.yaml @@ -2565,6 +2565,13 @@ drop_compact_storage_enabled: false # This would also apply to system keyspaces. # maximum_replication_factor_warn_threshold: -1 # maximum_replication_factor_fail_threshold: -1 +# +# Guardrail to warn or fail when a statement is prepared without bind markers (parameters). +# This prevents the Prepared Statement Cache from being flooded with unique entries caused +# by hardcoded literals. When warned is true, logs a warning on the usage of misprepared statements. +# When enabled is true, rejects misprepared statements with an error. +# prepared_statements_require_parameters_warned: true +# prepared_statements_require_parameters_enabled: false #role_name_policy: # # Implementation class of a validator. When not in form of FQCN, the diff --git a/conf/cassandra_latest.yaml b/conf/cassandra_latest.yaml index 2599b6e720..ee68c70082 100644 --- a/conf/cassandra_latest.yaml +++ b/conf/cassandra_latest.yaml @@ -2343,6 +2343,13 @@ drop_compact_storage_enabled: false # This would also apply to system keyspaces. # maximum_replication_factor_warn_threshold: -1 # maximum_replication_factor_fail_threshold: -1 +# +# Guardrail to warn or fail when a statement is prepared without bind markers (parameters). +# This prevents the Prepared Statement Cache from being flooded with unique entries caused +# by hardcoded literals. When warned is true, logs a warning on the usage of misprepared statements. +# When enabled is true, rejects misprepared statements with an error. +# prepared_statements_require_parameters_warned: true +# prepared_statements_require_parameters_enabled: false #role_name_policy: # # Implementation class of a validator. When not in form of FQCN, the diff --git a/src/java/org/apache/cassandra/config/Config.java b/src/java/org/apache/cassandra/config/Config.java index dd75d910c8..3670fb062b 100644 --- a/src/java/org/apache/cassandra/config/Config.java +++ b/src/java/org/apache/cassandra/config/Config.java @@ -938,6 +938,8 @@ public class Config public volatile boolean user_timestamps_enabled = true; public volatile boolean alter_table_enabled = true; public volatile boolean group_by_enabled = true; + public volatile boolean prepared_statements_require_parameters_warned = true; + public volatile boolean prepared_statements_require_parameters_enabled = false; public volatile boolean bulk_load_enabled = true; public volatile boolean drop_truncate_table_enabled = true; public volatile boolean drop_keyspace_enabled = true; diff --git a/src/java/org/apache/cassandra/config/GuardrailsOptions.java b/src/java/org/apache/cassandra/config/GuardrailsOptions.java index c166e71ec1..54b88b6926 100644 --- a/src/java/org/apache/cassandra/config/GuardrailsOptions.java +++ b/src/java/org/apache/cassandra/config/GuardrailsOptions.java @@ -1403,6 +1403,36 @@ public class GuardrailsOptions implements GuardrailsConfig x -> config.minimum_client_driver_versions_disallowed = x); } + @Override + public boolean getPreparedStatementsRequireParametersWarned() + { + return config.prepared_statements_require_parameters_warned; + } + + @Override + public boolean getPreparedStatementsRequireParametersEnabled() + { + return config.prepared_statements_require_parameters_enabled; + } + + @Override + public void setPreparedStatementsRequireParametersWarned(boolean warned) + { + updatePropertyWithLogging("prepared_statements_require_parameters_warned", + warned, + () -> config.prepared_statements_require_parameters_warned, + x -> config.prepared_statements_require_parameters_warned = x); + } + + @Override + public void setPreparedStatementsRequireParametersEnabled(boolean enabled) + { + updatePropertyWithLogging("prepared_statements_require_parameters_enabled", + enabled, + () -> config.prepared_statements_require_parameters_enabled, + x -> config.prepared_statements_require_parameters_enabled = x); + } + private static void updatePropertyWithLogging(String propertyName, T newValue, Supplier getter, Consumer setter) { T oldValue = getter.get(); diff --git a/src/java/org/apache/cassandra/cql3/CQLStatement.java b/src/java/org/apache/cassandra/cql3/CQLStatement.java index db9896361e..25c4213669 100644 --- a/src/java/org/apache/cassandra/cql3/CQLStatement.java +++ b/src/java/org/apache/cassandra/cql3/CQLStatement.java @@ -73,6 +73,18 @@ public interface CQLStatement */ void validate(ClientState state); + /** + * Performs validation specific to the preparation phase of a CQL statement. + *

+ * Unlike {@link #validate(ClientState)}, which is invoked during normal execution, + * this method is explicitly triggered only when a statement is being prepared by a client. + * By default, this is a no-op. Subclasses can override this to enforce preparation-specific + * rules or guardrails (e.g., ensuring prepared statements contain bind markers). + * + * @param state the current client state + */ + default void validatePrepare(ClientState state) {} + /** * Execute the statement and return the resulting result or null if there is no result. * diff --git a/src/java/org/apache/cassandra/cql3/QueryProcessor.java b/src/java/org/apache/cassandra/cql3/QueryProcessor.java index 606da47305..27903c8f71 100644 --- a/src/java/org/apache/cassandra/cql3/QueryProcessor.java +++ b/src/java/org/apache/cassandra/cql3/QueryProcessor.java @@ -494,6 +494,7 @@ public class QueryProcessor implements QueryHandler // Note: if 2 threads prepare the same query, we'll live so don't bother synchronizing CQLStatement statement = raw.prepare(clientState); statement.validate(clientState); + statement.validatePrepare(clientState); // Set CQL string for AlterSchemaStatement as this is used to serialize the transformation // in the cluster metadata log diff --git a/src/java/org/apache/cassandra/cql3/statements/BatchStatement.java b/src/java/org/apache/cassandra/cql3/statements/BatchStatement.java index 302d923436..ad21a6182c 100644 --- a/src/java/org/apache/cassandra/cql3/statements/BatchStatement.java +++ b/src/java/org/apache/cassandra/cql3/statements/BatchStatement.java @@ -338,6 +338,15 @@ public class BatchStatement implements CQLStatement.CompositeCQLStatement statement.validate(state); } + @Override + public void validatePrepare(ClientState state) + { + for (ModificationStatement statement : statements) + { + statement.validatePrepare(state); + } + } + @Override public List getStatements() { diff --git a/src/java/org/apache/cassandra/cql3/statements/ModificationStatement.java b/src/java/org/apache/cassandra/cql3/statements/ModificationStatement.java index ae2bcaacae..ec1c59f53d 100644 --- a/src/java/org/apache/cassandra/cql3/statements/ModificationStatement.java +++ b/src/java/org/apache/cassandra/cql3/statements/ModificationStatement.java @@ -414,6 +414,12 @@ public abstract class ModificationStatement implements CQLStatement.SingleKeyspa Guardrails.userTimestampsEnabled.ensureEnabled(state); } + @Override + public void validatePrepare(ClientState state) + { + Guardrails.preparedStatementsRequireParameters.guard(this, restrictions, state, metadata.keyspace, metadata.getTableName()); + } + public void validateDiskUsage(QueryOptions options, ClientState state) { diff --git a/src/java/org/apache/cassandra/cql3/statements/SelectStatement.java b/src/java/org/apache/cassandra/cql3/statements/SelectStatement.java index 91361ae91c..23796b5618 100644 --- a/src/java/org/apache/cassandra/cql3/statements/SelectStatement.java +++ b/src/java/org/apache/cassandra/cql3/statements/SelectStatement.java @@ -344,6 +344,12 @@ public class SelectStatement implements CQLStatement.SingleKeyspaceCqlStatement, Guardrails.allowFilteringEnabled.ensureEnabled(state); } + @Override + public void validatePrepare(ClientState state) + { + Guardrails.preparedStatementsRequireParameters.guard(this, restrictions, state, table.keyspace, table.getTableName()); + } + @Override public ResultMessage.Rows execute(QueryState state, QueryOptions options, Dispatcher.RequestTime requestTime) { diff --git a/src/java/org/apache/cassandra/db/guardrails/Guardrails.java b/src/java/org/apache/cassandra/db/guardrails/Guardrails.java index cea9d59511..618d5dfde9 100644 --- a/src/java/org/apache/cassandra/db/guardrails/Guardrails.java +++ b/src/java/org/apache/cassandra/db/guardrails/Guardrails.java @@ -709,6 +709,9 @@ public final class Guardrails implements GuardrailsMBean state -> CONFIG_PROVIDER.getOrCreate(state).getUnsetTrainingMinFrequencyEnabled(), "unset minimum frequency of training for dictionary compressor"); + public static final PreparedStatementParameterRequirementGuardrail preparedStatementsRequireParameters = + new PreparedStatementParameterRequirementGuardrail(); + private Guardrails() { MBeanWrapper.instance.registerMBean(this, MBEAN_NAME); @@ -1921,6 +1924,30 @@ public final class Guardrails implements GuardrailsMBean } } + @Override + public boolean getPreparedStatementsRequireParametersWarned() + { + return DEFAULT_CONFIG.getPreparedStatementsRequireParametersWarned(); + } + + @Override + public boolean getPreparedStatementsRequireParametersEnabled() + { + return DEFAULT_CONFIG.getPreparedStatementsRequireParametersEnabled(); + } + + @Override + public void setPreparedStatementsRequireParametersWarned(boolean warned) + { + DEFAULT_CONFIG.setPreparedStatementsRequireParametersWarned(warned); + } + + @Override + public void setPreparedStatementsRequireParametersEnabled(boolean enabled) + { + DEFAULT_CONFIG.setPreparedStatementsRequireParametersEnabled(enabled); + } + private static String toCSV(Set values) { return values == null || values.isEmpty() ? "" : String.join(",", values); diff --git a/src/java/org/apache/cassandra/db/guardrails/GuardrailsConfig.java b/src/java/org/apache/cassandra/db/guardrails/GuardrailsConfig.java index 2d893fe3f5..c0ff7b34fe 100644 --- a/src/java/org/apache/cassandra/db/guardrails/GuardrailsConfig.java +++ b/src/java/org/apache/cassandra/db/guardrails/GuardrailsConfig.java @@ -695,4 +695,30 @@ public interface GuardrailsConfig * driver version is below the specified minimum. */ Map getMinimumClientDriverVersionsDisallowed(); + + /** + * @return {@code true} if a warning is logged when a prepared statement is created without parameters, + * {@code false} otherwise. + */ + boolean getPreparedStatementsRequireParametersWarned(); + + /** + * @return {@code true} if creating a prepared statement without parameters is rejected, + * {@code false} otherwise. + */ + boolean getPreparedStatementsRequireParametersEnabled(); + + /** + * Sets whether to log a warning when a prepared statement is created without parameters. + * + * @param warned {@code true} to enable the warning, {@code false} to disable it. + */ + void setPreparedStatementsRequireParametersWarned(boolean warned); + + /** + * Sets whether to reject creating a prepared statement without parameters. + * + * @param enabled {@code true} to reject, {@code false} to allow. + */ + void setPreparedStatementsRequireParametersEnabled(boolean enabled); } diff --git a/src/java/org/apache/cassandra/db/guardrails/GuardrailsMBean.java b/src/java/org/apache/cassandra/db/guardrails/GuardrailsMBean.java index 9b341425df..71273c8a61 100644 --- a/src/java/org/apache/cassandra/db/guardrails/GuardrailsMBean.java +++ b/src/java/org/apache/cassandra/db/guardrails/GuardrailsMBean.java @@ -1214,4 +1214,30 @@ public interface GuardrailsMBean * @param value JSON representation of a map of driver name to minimum version string. */ void setMinimumClientDriverVersionsDisallowed(String value); + + /** + * @return {@code true} if a warning is logged when a prepared statement is created without parameters, + * {@code false} otherwise. + */ + boolean getPreparedStatementsRequireParametersWarned(); + + /** + * @return {@code true} if creating a prepared statement without parameters is rejected, + * {@code false} otherwise. + */ + boolean getPreparedStatementsRequireParametersEnabled(); + + /** + * Sets whether to log a warning when a prepared statement is created without parameters. + * + * @param warned {@code true} to enable the warning, {@code false} to disable it. + */ + void setPreparedStatementsRequireParametersWarned(boolean warned); + + /** + * Sets whether to reject creating a prepared statement without parameters. + * + * @param enabled {@code true} to reject, {@code false} to allow. + */ + void setPreparedStatementsRequireParametersEnabled(boolean enabled); } diff --git a/src/java/org/apache/cassandra/db/guardrails/PreparedStatementParameterRequirementGuardrail.java b/src/java/org/apache/cassandra/db/guardrails/PreparedStatementParameterRequirementGuardrail.java new file mode 100644 index 0000000000..06e3f567e7 --- /dev/null +++ b/src/java/org/apache/cassandra/db/guardrails/PreparedStatementParameterRequirementGuardrail.java @@ -0,0 +1,66 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.cassandra.db.guardrails; + +import javax.annotation.Nullable; + +import com.google.common.annotations.VisibleForTesting; + +import org.apache.cassandra.cql3.CQLStatement; +import org.apache.cassandra.cql3.restrictions.StatementRestrictions; +import org.apache.cassandra.service.ClientState; + +public class PreparedStatementParameterRequirementGuardrail extends Guardrail +{ + @VisibleForTesting + public static final String MISPREPARED_STATEMENT_MESSAGE = "The query contains only literal values and no bind markers. Using one or more '?' placeholder values (bind markers) allows a prepared statement to be reused."; + + PreparedStatementParameterRequirementGuardrail() + { + super("prepared_statements_require_parameters", null); + } + + public void guard(CQLStatement statement, StatementRestrictions restrictions, @Nullable ClientState state, String keyspace, String table) + { + if (restrictions == null + || !statement.eligibleAsPreparedStatement() + || !statement.getBindVariables().isEmpty() + || (!restrictions.hasPartitionKeyRestrictions() + && !restrictions.hasClusteringColumnsRestrictions() + && !restrictions.hasNonPrimaryKeyRestrictions())) + return; + + if (!enabled(state)) + return; + + final GuardrailsConfig config = Guardrails.CONFIG_PROVIDER.getOrCreate(state); + + boolean failOn = config.getPreparedStatementsRequireParametersEnabled(); + if (!failOn && !config.getPreparedStatementsRequireParametersWarned()) + return; + + final String message = MISPREPARED_STATEMENT_MESSAGE + " Query executed on keyspace '" + keyspace + "', table '" + table + "'."; + + if (failOn) + fail(message, state); + else + warn(message); + } +} + diff --git a/src/java/org/apache/cassandra/tools/nodetool/GuardrailsConfigCommand.java b/src/java/org/apache/cassandra/tools/nodetool/GuardrailsConfigCommand.java index 780112b47d..f1efadc0cb 100644 --- a/src/java/org/apache/cassandra/tools/nodetool/GuardrailsConfigCommand.java +++ b/src/java/org/apache/cassandra/tools/nodetool/GuardrailsConfigCommand.java @@ -373,6 +373,7 @@ public abstract class GuardrailsConfigCommand extends AbstractCommand * Set of guardrails which are flags, even though their suffix would suggest they are part of "values" which have warned, ignored, and disallowed sub-categories */ private static final Set specialFlags = Set.of("intersect_filtering_query_warned", + "prepared_statements_require_parameters_warned", "unset_training_min_frequency_warned", "zero_ttl_on_twcs_warned"); diff --git a/test/distributed/org/apache/cassandra/distributed/test/guardrails/MispreparedStatementsTest.java b/test/distributed/org/apache/cassandra/distributed/test/guardrails/MispreparedStatementsTest.java new file mode 100644 index 0000000000..516ec150c7 --- /dev/null +++ b/test/distributed/org/apache/cassandra/distributed/test/guardrails/MispreparedStatementsTest.java @@ -0,0 +1,126 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.cassandra.distributed.test.guardrails; + +import java.io.IOException; + +import com.datastax.driver.core.Session; +import com.datastax.driver.core.SimpleStatement; +import com.datastax.driver.core.exceptions.InvalidQueryException; + +import org.assertj.core.api.ThrowableAssert; +import org.junit.AfterClass; +import org.junit.BeforeClass; +import org.junit.Test; + +import org.apache.cassandra.db.guardrails.Guardrails; +import org.apache.cassandra.db.guardrails.PreparedStatementParameterRequirementGuardrail; +import org.apache.cassandra.distributed.Cluster; +import org.apache.cassandra.distributed.api.Feature; + +import static org.assertj.core.api.Assertions.assertThatCode; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +public class MispreparedStatementsTest extends GuardrailTester +{ + private static Cluster cluster; + private static com.datastax.driver.core.Cluster driverCluster; + private static Session driverSession; + + @BeforeClass + public static void setupCluster() throws IOException + { + cluster = init(Cluster.build(1) + .withConfig(c -> c.with(Feature.GOSSIP, Feature.NATIVE_PROTOCOL) + .set("authenticator", "PasswordAuthenticator") + .set("prepared_statements_require_parameters_warned", true) + .set("prepared_statements_require_parameters_enabled", false)) + .start()); + driverCluster = buildDriverCluster(cluster); + driverSession = driverCluster.connect(); + } + + @AfterClass + public static void teardownCluster() + { + if (driverSession != null) + driverSession.close(); + + if (driverCluster != null) + driverCluster.close(); + + if (cluster != null) + cluster.close(); + } + + @Override + protected Cluster getCluster() + { + return cluster; + } + + @Test + public void testInvalidConstantSelectStatements() + { + schemaChange("create table %s (pk int, ck int, data text, primary key((pk), ck))"); + + cluster.get(1).runOnInstance(() -> { + Guardrails.instance.setPreparedStatementsRequireParametersEnabled(true); + }); + assertGuardrailViolated(() -> prepare("select * from %s where pk = 1")); + assertGuardrailViolated(() -> prepare("select * from %s where ck = 1 allow filtering")); + assertGuardrailViolated(() -> prepare("select * from %s where data = 'a' allow filtering")); + assertGuardrailViolated(() -> prepare("insert into %s(pk, ck, data) values (1,1,'a')")); + assertGuardrailViolated(() -> prepare("update %s set data = 'a' where pk = 1 and ck = 1")); + + // non-prepare queries remain unaffected + assertThatCode(() -> execute("select * from %s where pk = 1")).doesNotThrowAnyException(); + // at-least 1 bind marker + assertThatCode(() -> prepare("select * from %s where pk = 1 AND data = ? allow filtering")).doesNotThrowAnyException(); + // statement having bind marker but no where clause + assertThatCode(() -> prepare("insert into %s(pk, ck, data) values (1,1,?)")).doesNotThrowAnyException(); + } + + private void assertGuardrailViolated(ThrowableAssert.ThrowingCallable throwable) + { + assertThatThrownBy(throwable) + .isInstanceOf(InvalidQueryException.class) + .hasMessageContaining(PreparedStatementParameterRequirementGuardrail.MISPREPARED_STATEMENT_MESSAGE); + } + + @Override + protected Session getSession() + { + return driverSession; + } + + private void execute(String query) + { + SimpleStatement stmt = new SimpleStatement(format(query)); + stmt.setConsistencyLevel(com.datastax.driver.core.ConsistencyLevel.ALL); + driverSession.execute(stmt); + } + + private void prepare(String query) + { + SimpleStatement stmt = new SimpleStatement(format(query)); + stmt.setConsistencyLevel(com.datastax.driver.core.ConsistencyLevel.ALL); + driverSession.prepare(stmt); + } +} diff --git a/test/unit/org/apache/cassandra/db/guardrails/MispreparedStatementsTest.java b/test/unit/org/apache/cassandra/db/guardrails/MispreparedStatementsTest.java new file mode 100644 index 0000000000..9c8ffa6d26 --- /dev/null +++ b/test/unit/org/apache/cassandra/db/guardrails/MispreparedStatementsTest.java @@ -0,0 +1,134 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.cassandra.db.guardrails; + +import java.util.List; + +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + +import org.apache.cassandra.cql3.QueryProcessor; + +import static org.apache.cassandra.db.guardrails.PreparedStatementParameterRequirementGuardrail.MISPREPARED_STATEMENT_MESSAGE; + +public class MispreparedStatementsTest extends GuardrailTester +{ + private boolean preparedStatementsRequireParametersWarned; + private boolean preparedStatementsRequireParametersEnabled; + private String[] mispreparedQueries; + private String[] preparableQueries; + + @Before + public void setup() + { + preparedStatementsRequireParametersWarned = guardrails().getPreparedStatementsRequireParametersWarned(); + preparedStatementsRequireParametersEnabled = guardrails().getPreparedStatementsRequireParametersEnabled(); + createTable("create table %s (pk int, ck int, v text, PRIMARY KEY (pk, ck))"); + mispreparedQueries = new String[]{ + String.format("SELECT * FROM %s WHERE pk = 1", currentTable()), + String.format("SELECT * FROM %s WHERE pk = 1 AND ck = 1", currentTable()), + String.format("SELECT * FROM %s WHERE ck = 1 ALLOW FILTERING", currentTable()), + String.format("SELECT * FROM %s WHERE v = 'a' ALLOW FILTERING", currentTable()), + String.format("INSERT INTO %s (pk, ck, v) VALUES (1, 1, 'a')", currentTable()), + String.format("UPDATE %s SET v = 'b' WHERE pk = 1 AND ck = 1", currentTable()), + String.format("DELETE FROM %s WHERE pk = 1 AND ck = 1", currentTable()), + String.format("BEGIN BATCH " + + "INSERT INTO %s (pk, ck, v) VALUES (1, 1, 'a');" + + "UPDATE %s SET v = 'b' WHERE pk = 2 AND ck = 2;" + + "APPLY BATCH;", currentTable(), currentTable()) }; + preparableQueries = new String[]{ + String.format("SELECT * FROM %s WHERE pk = ?", currentTable()), + String.format("INSERT INTO %s (pk, ck, v) VALUES (?, 1, 'a')", currentTable()), + String.format("UPDATE %s SET v = ? WHERE pk = 1 AND ck = 1", currentTable()), + String.format("SELECT * FROM %s WHERE pk = 1 AND ck = ?", currentTable()), + String.format("TRUNCATE %s", currentTable()) + }; + userClientState.setKeyspace(KEYSPACE); + } + + @After + public void tear() + { + guardrails().setPreparedStatementsRequireParametersEnabled(preparedStatementsRequireParametersEnabled); + guardrails().setPreparedStatementsRequireParametersWarned(preparedStatementsRequireParametersWarned); + } + + @Test + public void testPreparedStatementsRequireParametersEnabledGuardrailEnabled() throws Throwable + { + guardrails().setPreparedStatementsRequireParametersEnabled(true); + + for (String query : mispreparedQueries) + assertGuardrailViolated(query); + + for (String query : preparableQueries) + assertGuardrailAllowed(query); + } + + @Test + public void testPreparedStatementsRequireParametersWarnEnabled() throws Throwable + { + guardrails().setPreparedStatementsRequireParametersEnabled(false); + for (String query : mispreparedQueries) + { + // Skip the batch query in this loop because it generates multiple warnings + if (query.contains("BEGIN BATCH")) continue; + + assertWarns(() -> QueryProcessor.getStatement(query, userClientState).validatePrepare(userClientState), + MISPREPARED_STATEMENT_MESSAGE + " Query executed on keyspace '" + KEYSPACE + "', table '" + currentTable() + "'."); + } + } + + @Test + public void testDoesNotWarnTwiceForSameQuery() throws Throwable + { + guardrails().setPreparedStatementsRequireParametersEnabled(false); + // We use a fully qualified name (KEYSPACE.table) here and ensure ClientState keyspace is null + // to suppress the "USE anti-pattern" warning, which otherwise conflicts with + // assertWarns by adding an unexpected second warning to the result. + String query = String.format("SELECT * FROM %s.%s WHERE pk = 1", KEYSPACE, currentTable()); + assertWarns(() -> QueryProcessor.instance.prepare(query, userClientState), + MISPREPARED_STATEMENT_MESSAGE + " Query executed on keyspace '" + KEYSPACE + "', table '" + currentTable() + "'."); + assertValid(() -> QueryProcessor.instance.prepare(query, userClientState)); + } + + @Test + public void testBatchPreparedStatementsWarnEnabled() throws Throwable + { + guardrails().setPreparedStatementsRequireParametersEnabled(false); + String batchQuery = mispreparedQueries[7]; + + String message = MISPREPARED_STATEMENT_MESSAGE + " Query executed on keyspace '" + KEYSPACE + "', table '" + currentTable() + "'."; + + assertWarns(() -> QueryProcessor.getStatement(batchQuery, userClientState).validatePrepare(userClientState), + List.of(message, message)); + } + + private void assertGuardrailViolated(String query) throws Throwable + { + assertFails(() -> QueryProcessor.getStatement(query, userClientState).validatePrepare(userClientState), + MISPREPARED_STATEMENT_MESSAGE + " Query executed on keyspace '" + KEYSPACE + "', table '" + currentTable() + "'."); + } + + private void assertGuardrailAllowed(String query) throws Throwable + { + assertValid(() -> QueryProcessor.getStatement(query, userClientState).validatePrepare(userClientState)); + } +} diff --git a/test/unit/org/apache/cassandra/tools/nodetool/GuardrailsConfigCommandsTest.java b/test/unit/org/apache/cassandra/tools/nodetool/GuardrailsConfigCommandsTest.java index 2512d30da9..86430dba97 100644 --- a/test/unit/org/apache/cassandra/tools/nodetool/GuardrailsConfigCommandsTest.java +++ b/test/unit/org/apache/cassandra/tools/nodetool/GuardrailsConfigCommandsTest.java @@ -209,6 +209,8 @@ public class GuardrailsConfigCommandsTest extends CQLTester "intersect_filtering_query_enabled true\n" + "intersect_filtering_query_warned true\n" + "non_partition_restricted_index_query_enabled true\n" + + "prepared_statements_require_parameters_enabled false\n" + + "prepared_statements_require_parameters_warned true\n" + "read_before_write_list_operations_enabled true\n" + "secondary_indexes_enabled true\n" + "simplestrategy_enabled true\n" +