Copilot commented on code in PR #4596: URL: https://github.com/apache/cassandra/pull/4596#discussion_r2878549437
########## test/unit/org/apache/cassandra/cql3/MispreparedStatementsTest.java: ########## @@ -0,0 +1,396 @@ +/* + * 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.cql3; + +import java.net.InetSocketAddress; +import java.util.List; + +import org.junit.After; +import org.junit.Assert; +import org.junit.Before; +import org.junit.BeforeClass; +import org.junit.Test; + +import org.apache.cassandra.auth.AuthenticatedUser; +import org.apache.cassandra.auth.CassandraAuthorizer; +import org.apache.cassandra.auth.PasswordAuthenticator; +import org.apache.cassandra.config.DatabaseDescriptor; +import org.apache.cassandra.db.guardrails.GuardrailViolatedException; +import org.apache.cassandra.db.guardrails.Guardrails; +import org.apache.cassandra.service.ClientState; +import org.apache.cassandra.service.ClientWarn; + +import static org.junit.Assert.assertTrue; + +public class MispreparedStatementsTest extends CQLTester +{ + private static final ClientState state = ClientState.forExternalCalls(new InetSocketAddress("127.0.0.1", 9042)); + + @BeforeClass + public static void setupGlobalConfig() + { + DatabaseDescriptor.setAuthenticator(new PasswordAuthenticator()); + DatabaseDescriptor.setAuthorizer(new CassandraAuthorizer()); + DatabaseDescriptor.setPreparedStatementsRequireParametersEnabled(true); + } + + @Before + public void setUp() + { + AuthenticatedUser nonSuperUser = new AuthenticatedUser("regular-user") + { + @Override + public boolean isSuper() + { + return false; + } + + @Override + public boolean isSystem() + { + return false; + } + + @Override + public boolean isAnonymous() + { + return false; + } + + @Override + public boolean canLogin() + { + return true; + } + }; + + state.login(nonSuperUser); + state.setKeyspace(KEYSPACE); + ClientWarn.instance.captureWarnings(); + createTable("CREATE TABLE %s (id int, description text, age int, name text, PRIMARY KEY (id, name, age))"); + } + + @After + public void tearDown() + { + DatabaseDescriptor.setPreparedStatementsRequireParametersEnabled(true); + } + + @Test + public void testSelectWithPartitionKey() + { + assertGuardrailViolated(String.format("SELECT * FROM %s WHERE id = 1", currentTable())); + assertNoWarnings(); + } + + @Test + public void testSelectWithClusteringKey() + { + assertGuardrailViolated(String.format("SELECT * FROM %s WHERE name = 'v1' ALLOW FILTERING", currentTable())); + assertNoWarnings(); + } + + @Test + public void testSelectWithFullPrimaryKey() + { + assertGuardrailViolated(String.format("SELECT * FROM %s WHERE id = 1 AND name = 'v1'", currentTable())); + assertNoWarnings(); + } + + @Test + public void testSelectWithWhereNonPrimaryKeyColumn() + { + assertGuardrailViolated(String.format("SELECT * FROM %s WHERE description = 'foo' ALLOW FILTERING", currentTable())); + assertNoWarnings(); + } + + + @Test + public void testSelectWithCompositeRestriction() + { + assertGuardrailViolated(String.format("SELECT sum(id) from %s WHERE (name, age) = ('a', 1) ALLOW FILTERING", currentTable())); + } + + @Test + public void testSelectWithFunction() + { + assertGuardrailViolated(String.format("SELECT sum(id) from %s where name = 'v1' ALLOW FILTERING", currentTable())); + } + + @Test + public void testSelectWithRangeRestriction() + { + assertGuardrailViolated(String.format("SELECT * FROM %s WHERE id = 1 AND name > 'a'", currentTable())); + assertNoWarnings(); + } + + @Test + public void testSelectInRestrictionOnPartitionKey() + { + assertGuardrailViolated(String.format("SELECT * FROM %s WHERE id IN (1, 2, 3)", currentTable())); + assertNoWarnings(); + } + + @Test + public void testSelectInRestrictionOnClusteringKey() + { + assertGuardrailViolated(String.format("SELECT * FROM %s WHERE name IN ('a', 'b') ALLOW FILTERING", currentTable())); + assertNoWarnings(); + } + + @Test + public void testSelectInRestrictionOnFullPrimaryKey() + { + assertGuardrailViolated(String.format("SELECT * FROM %s WHERE id IN (1, 2, 3) AND name in ('a', 'b')", currentTable())); + assertNoWarnings(); + } + + @Test + public void testDDLStatementsBypass() + { + assertGuardrailPassed("CREATE TABLE IF NOT EXISTS test_table (id INT PRIMARY KEY)"); + assertNoWarnings(); + } + + @Test + public void testWhereLiteralWithLike() + { + assertGuardrailViolated(String.format("SELECT * FROM %s WHERE name LIKE 'prefix%%' ALLOW FILTERING", currentTable())); + assertNoWarnings(); + } + + @Test + public void testInsertJsonGuardrail() + { + assertGuardrailViolated(String.format("INSERT INTO %s JSON '{\"id\": 1, \"name\": \"v1\"}'", currentTable())); + assertNoWarnings(); + } + + @Test + public void testUpdateWithPartitionKey() + { + assertGuardrailViolated(String.format("UPDATE %s SET description = 'new' WHERE id = 1 AND name = 'name' AND age = 1", KEYSPACE + '.' + currentTable())); + assertNoWarnings(); + } + + @Test + public void testUpdateWithIfCondition() + { + assertGuardrailViolated(String.format("UPDATE %s SET description = 'v2' WHERE id = 1 AND name = 'v1' AND age = 1 IF description = 'v0'", currentTable())); + assertNoWarnings(); + } + + @Test + public void testDeleteWithFullPrimaryKey() + { + assertGuardrailViolated(String.format("DELETE FROM %s WHERE id = 1 AND name = 'v1'", currentTable())); + assertNoWarnings(); + } + + @Test + public void testMultiTableBatchGuardrailAllLiteral() + { + String table1 = currentTable(); + String table2 = createTable("CREATE TABLE %s (id int PRIMARY KEY, val text)"); + String query = String.format("BEGIN BATCH " + + "UPDATE %s.%s SET description = 'v1' WHERE id = 1 AND name = 'n1' AND age = 1; " + + "INSERT INTO %s.%s (id, val) VALUES (2, 'v2'); " + + "APPLY BATCH", + KEYSPACE, table1, KEYSPACE, table2); + + assertGuardrailViolated(query); + assertNoWarnings(); + } + + @Test + public void testSucceedWithOnlyBindMarkers() + { + assertGuardrailPassed(String.format("SELECT * FROM %s WHERE id = ? AND name = ?", currentTable())); + assertNoWarnings(); + } + + @Test + public void testSucceedWithOneBindMarkerOneLiteral() + { + + assertGuardrailPassed(String.format("SELECT * FROM %s WHERE id = ? AND name = 'v1'", currentTable())); + assertNoWarnings(); + } + + @Test + public void testSelectAllPasses() + { + assertGuardrailPassed(String.format("SELECT * FROM %s", currentTable())); + assertNoWarnings(); + } + + @Test + public void testLiteralInProjectionIsAllowed() + { + assertGuardrailPassed(String.format("SELECT id, (text)'const_val' FROM %s WHERE id = ?", currentTable())); + assertNoWarnings(); + } + + @Test + public void testInternalBypass() + { + assertGuardrailPassed("SELECT * FROM " + KEYSPACE + '.' + currentTable() + " WHERE id = 1", ClientState.forInternalCalls()); + } + + @Test + public void testSuperUserBypass() + { + AuthenticatedUser superUser = new AuthenticatedUser("super-user") + { + @Override + public boolean isSuper() + { + return true; + } + + @Override + public boolean isSystem() + { + return false; + } + + @Override + public boolean isAnonymous() + { + return false; + } + + @Override + public boolean canLogin() + { + return true; + } + }; + ClientState superUserState = ClientState.forExternalCalls(new InetSocketAddress("127.0.0.1", 9042)); + superUserState.login(superUser); + assertGuardrailPassed("SELECT * FROM " + KEYSPACE + '.' + currentTable() + " WHERE id = 1", superUserState); + } + + @Test + public void testSystemKeyspaceBypassForRegularUser() + { + assertGuardrailPassed("SELECT * FROM system.local WHERE key = 'local'"); + assertNoWarnings(); + } + + @Test + public void testGuardrailDisabledAlowsLiterals() Review Comment: Typo in test name: `Alows` → `Allows`. ```suggestion public void testGuardrailDisabledAllowsLiterals() ``` ########## src/java/org/apache/cassandra/db/guardrails/Guardrails.java: ########## @@ -56,6 +56,9 @@ public final class Guardrails implements GuardrailsMBean public static final String MBEAN_NAME = "org.apache.cassandra.db:type=Guardrails"; public static final GuardrailsConfigProvider CONFIG_PROVIDER = GuardrailsConfigProvider.instance; + + public static final String MISPREPARED_STATEMENT_WARN_MESSAGE = "This query contains only literal values in the WHERE clause. " + "Using one or more '?' placeholder values (bind markers) allow a prepared statement to be reused."; Review Comment: Grammar in the warning message: "Using one or more '?' ... allow" should be "allows" (singular subject). ```suggestion public static final String MISPREPARED_STATEMENT_WARN_MESSAGE = "This query contains only literal values in the WHERE clause. " + "Using one or more '?' placeholder values (bind markers) allows a prepared statement to be reused."; ``` ########## src/java/org/apache/cassandra/db/guardrails/GuardrailsConfig.java: ########## @@ -406,6 +406,40 @@ public interface GuardrailsConfig */ boolean getVectorTypeEnabled(); + /** + * <p> + * A statement is considered "mis-prepared" if it contains only hardcoded liter values and without any bind markers + * instead of bind markers. This is a performance anti-pattern because it prevents + * query plan reuse and floods the server-side Prepared Statement Cache with + * unique entries, leading to heap exhaustion and high GC pressure. + * <p> + * <b>LWT and Batch Considerations:</b> + * This check applies to both the {@code WHERE} clause and the {@code IF} conditions + * in Lightweight Transactions (LWT). For example, the following is considered + * mis-prepared because of the hardcoded literals: + * <pre>{@code + * UPDATE users SET description = 'v2' WHERE id = 1 AND name = 'v1' IF description = 'v0' + * }</pre> + * To be compliant, it should use bind markers ({@code ?}): + * <pre>{@code + * UPDATE users SET description = ? WHERE id = ? AND name = ? IF description = ? + * }</pre> + * For {@code BATCH} statements, if any individual statement within the batch is + * identified as mis-prepared, the entire batch triggers this guardrail. + * @return {@code true} if the requirement for parameters in prepared statements is strictly enforced (queries will fail). + * {@code false} if the requirement is not strictly enforced (queries will only log a warning). + * Defaults to {@code false}. + */ + + boolean getPreparedStatementsRequireParametersEnabled(); + + /** + * Sets whether prepared statements are strictly required to have parameters. + * + * @param enabled {@code true} to reject statements without parameters, {@code false} to only warn. + */ + void setPreparedStatementsRequireParamtersEnabled(boolean enabled); Review Comment: The new setter is misspelled as `setPreparedStatementsRequireParamtersEnabled` (missing 'e' in Parameters). Since this is a public interface method, the typo will leak into API/implementations and be hard to change later; rename it to `setPreparedStatementsRequireParametersEnabled` and update callers/implementations accordingly. ```suggestion void setPreparedStatementsRequireParametersEnabled(boolean enabled); ``` ########## test/unit/org/apache/cassandra/tools/nodetool/GuardrailsConfigCommandsTest.java: ########## @@ -208,6 +208,7 @@ private Set<String> getConfigFieldNames() "group_by_enabled true\n" + "intersect_filtering_query_enabled true\n" + "intersect_filtering_query_warned true\n" + + "misprepared_statements_enabled true\n" + Review Comment: `ALL_FLAGS_GETTER_OUTPUT` expects `misprepared_statements_enabled`, but the corresponding `Config` field added in this PR is `prepared_statements_require_parameters_enabled`. Unless the MBean/config naming is changed to match, this test (and `testParsedGuardrailNamesFromMBeanExistInCassandraYaml`) will fail because `misprepared_statements_enabled` is not a valid config key. ```suggestion "prepared_statements_require_parameters_enabled true\n" + ``` ########## src/java/org/apache/cassandra/config/Config.java: ########## @@ -933,6 +933,7 @@ public static void setClientMode(boolean clientMode) 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_enabled = false; public volatile boolean bulk_load_enabled = true; Review Comment: PR description says the new flag’s default is `true` for backward compatibility, but the introduced config field `prepared_statements_require_parameters_enabled` defaults to `false`. Please align the implementation or update the PR description so operators understand the default behavior (warn vs reject). ########## src/java/org/apache/cassandra/db/guardrails/GuardrailsConfig.java: ########## @@ -406,6 +406,40 @@ public interface GuardrailsConfig */ boolean getVectorTypeEnabled(); + /** + * <p> + * A statement is considered "mis-prepared" if it contains only hardcoded liter values and without any bind markers + * instead of bind markers. This is a performance anti-pattern because it prevents Review Comment: Javadoc has a few typos/grammar issues: "liter values" should be "literal values", and the phrase "without any bind markers instead of bind markers" is redundant/unclear. Cleaning this up will make the guardrail definition easier to understand for operators. ```suggestion * A statement is considered "mis-prepared" if it contains only hardcoded literal values and no bind markers. * This is a performance anti-pattern because it prevents ``` ########## conf/cassandra.yaml: ########## @@ -2536,6 +2536,15 @@ 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 true, the server rejects the preparation of any statement that does not contains atleast one parameter. +# When false, the server only emits a warning. +# Defaults to false. +# prepare_statements_require_parameters_enabled: false Review Comment: The documented yaml key is `prepare_statements_require_parameters_enabled`, but the actual `Config` field introduced is `prepared_statements_require_parameters_enabled` (with 'prepared'). If operators copy/paste the commented key, it won’t take effect. Also the comment has typos ("does not contains", "atleast"). ```suggestion # When true, the server rejects the preparation of any statement that does not contain at least one parameter. # When false, the server only emits a warning. # Defaults to false. # prepared_statements_require_parameters_enabled: false ``` ########## src/java/org/apache/cassandra/config/DatabaseDescriptor.java: ########## @@ -5522,6 +5522,15 @@ public static void setUseStatementsEnabled(boolean enabled) } } + public static boolean getPreparedStatementsRequireParametersEnabled() + { + return conf.prepared_statements_require_parameters_enabled; + } + + public static void setPreparedStatementsRequireParametersEnabled(boolean enabled) + { + conf.prepared_statements_require_parameters_enabled = enabled; Review Comment: `setPreparedStatementsRequireParametersEnabled` sets the config flag without any logging or change check, unlike nearby dynamic setters (e.g. `setUseStatementsEnabled`). Consider logging when the value changes to help operators audit runtime config changes via JMX/tests. ```suggestion if (enabled != conf.prepared_statements_require_parameters_enabled) { logger.info("Setting prepared_statements_require_parameters_enabled to {}", enabled); conf.prepared_statements_require_parameters_enabled = enabled; } ``` ########## src/java/org/apache/cassandra/db/guardrails/GuardrailsMBean.java: ########## @@ -54,6 +54,13 @@ public interface GuardrailsMBean */ void setKeyspacesThreshold(int warn, int fail); + /** + * @return the use of misprepared statement is enabled + */ + boolean getMispreparedStatementsEnabled(); + + void setMispreparedStatementsEnabled(boolean enabled); Review Comment: The new MBean attribute name (`getMispreparedStatementsEnabled` → `misprepared_statements_enabled`) doesn’t correspond to any `Config` field / cassandra.yaml key (the added config is `prepared_statements_require_parameters_enabled`). This will break `GuardrailsConfigCommand` name parsing and runtime configuration via nodetool/JMX unless the names are aligned (either rename the MBean methods or rename the config key). ```suggestion * @return whether the guardrail enforcing that prepared statements require parameters is enabled */ boolean getPreparedStatementsRequireParametersEnabled(); void setPreparedStatementsRequireParametersEnabled(boolean enabled); ``` ########## src/java/org/apache/cassandra/config/GuardrailsOptions.java: ########## @@ -1329,6 +1329,21 @@ public boolean getVectorTypeEnabled() return config.vector_type_enabled; } + @Override + public boolean getPreparedStatementsRequireParametersEnabled() + { + return config.prepared_statements_require_parameters_enabled; + } + + + public void setPreparedStatementsRequireParamtersEnabled(boolean enabled) + { + updatePropertyWithLogging("prepared_statements_require_parameters_enabled", + enabled, + () -> config.prepared_statements_require_parameters_enabled, + x -> config.prepared_statements_require_parameters_enabled = x); + } Review Comment: `setPreparedStatementsRequireParamtersEnabled` is misspelled and also lacks `@Override` even though `GuardrailsOptions` implements `GuardrailsConfig`. Once the interface method is corrected, this will stop compiling; please rename the method to match the corrected interface signature and add `@Override` for compile-time safety. ########## test/unit/org/apache/cassandra/cql3/MispreparedStatementsTest.java: ########## @@ -0,0 +1,396 @@ +/* + * 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.cql3; + +import java.net.InetSocketAddress; +import java.util.List; + +import org.junit.After; +import org.junit.Assert; +import org.junit.Before; +import org.junit.BeforeClass; +import org.junit.Test; + +import org.apache.cassandra.auth.AuthenticatedUser; +import org.apache.cassandra.auth.CassandraAuthorizer; +import org.apache.cassandra.auth.PasswordAuthenticator; +import org.apache.cassandra.config.DatabaseDescriptor; +import org.apache.cassandra.db.guardrails.GuardrailViolatedException; +import org.apache.cassandra.db.guardrails.Guardrails; +import org.apache.cassandra.service.ClientState; +import org.apache.cassandra.service.ClientWarn; + +import static org.junit.Assert.assertTrue; + +public class MispreparedStatementsTest extends CQLTester +{ + private static final ClientState state = ClientState.forExternalCalls(new InetSocketAddress("127.0.0.1", 9042)); + + @BeforeClass + public static void setupGlobalConfig() + { + DatabaseDescriptor.setAuthenticator(new PasswordAuthenticator()); + DatabaseDescriptor.setAuthorizer(new CassandraAuthorizer()); + DatabaseDescriptor.setPreparedStatementsRequireParametersEnabled(true); + } + + @Before + public void setUp() + { + AuthenticatedUser nonSuperUser = new AuthenticatedUser("regular-user") + { + @Override + public boolean isSuper() + { + return false; + } + + @Override + public boolean isSystem() + { + return false; + } + + @Override + public boolean isAnonymous() + { + return false; + } + + @Override + public boolean canLogin() + { + return true; + } + }; + + state.login(nonSuperUser); + state.setKeyspace(KEYSPACE); + ClientWarn.instance.captureWarnings(); + createTable("CREATE TABLE %s (id int, description text, age int, name text, PRIMARY KEY (id, name, age))"); + } + + @After + public void tearDown() + { + DatabaseDescriptor.setPreparedStatementsRequireParametersEnabled(true); + } + + @Test + public void testSelectWithPartitionKey() + { + assertGuardrailViolated(String.format("SELECT * FROM %s WHERE id = 1", currentTable())); + assertNoWarnings(); + } + + @Test + public void testSelectWithClusteringKey() + { + assertGuardrailViolated(String.format("SELECT * FROM %s WHERE name = 'v1' ALLOW FILTERING", currentTable())); + assertNoWarnings(); + } + + @Test + public void testSelectWithFullPrimaryKey() + { + assertGuardrailViolated(String.format("SELECT * FROM %s WHERE id = 1 AND name = 'v1'", currentTable())); + assertNoWarnings(); + } + + @Test + public void testSelectWithWhereNonPrimaryKeyColumn() + { + assertGuardrailViolated(String.format("SELECT * FROM %s WHERE description = 'foo' ALLOW FILTERING", currentTable())); + assertNoWarnings(); + } + + + @Test + public void testSelectWithCompositeRestriction() + { + assertGuardrailViolated(String.format("SELECT sum(id) from %s WHERE (name, age) = ('a', 1) ALLOW FILTERING", currentTable())); + } + + @Test + public void testSelectWithFunction() + { + assertGuardrailViolated(String.format("SELECT sum(id) from %s where name = 'v1' ALLOW FILTERING", currentTable())); + } + + @Test + public void testSelectWithRangeRestriction() + { + assertGuardrailViolated(String.format("SELECT * FROM %s WHERE id = 1 AND name > 'a'", currentTable())); + assertNoWarnings(); + } + + @Test + public void testSelectInRestrictionOnPartitionKey() + { + assertGuardrailViolated(String.format("SELECT * FROM %s WHERE id IN (1, 2, 3)", currentTable())); + assertNoWarnings(); + } + + @Test + public void testSelectInRestrictionOnClusteringKey() + { + assertGuardrailViolated(String.format("SELECT * FROM %s WHERE name IN ('a', 'b') ALLOW FILTERING", currentTable())); + assertNoWarnings(); + } + + @Test + public void testSelectInRestrictionOnFullPrimaryKey() + { + assertGuardrailViolated(String.format("SELECT * FROM %s WHERE id IN (1, 2, 3) AND name in ('a', 'b')", currentTable())); + assertNoWarnings(); + } + + @Test + public void testDDLStatementsBypass() + { + assertGuardrailPassed("CREATE TABLE IF NOT EXISTS test_table (id INT PRIMARY KEY)"); + assertNoWarnings(); + } + + @Test + public void testWhereLiteralWithLike() + { + assertGuardrailViolated(String.format("SELECT * FROM %s WHERE name LIKE 'prefix%%' ALLOW FILTERING", currentTable())); + assertNoWarnings(); + } + + @Test + public void testInsertJsonGuardrail() + { + assertGuardrailViolated(String.format("INSERT INTO %s JSON '{\"id\": 1, \"name\": \"v1\"}'", currentTable())); + assertNoWarnings(); + } + + @Test + public void testUpdateWithPartitionKey() + { + assertGuardrailViolated(String.format("UPDATE %s SET description = 'new' WHERE id = 1 AND name = 'name' AND age = 1", KEYSPACE + '.' + currentTable())); + assertNoWarnings(); + } + + @Test + public void testUpdateWithIfCondition() + { + assertGuardrailViolated(String.format("UPDATE %s SET description = 'v2' WHERE id = 1 AND name = 'v1' AND age = 1 IF description = 'v0'", currentTable())); + assertNoWarnings(); + } + + @Test + public void testDeleteWithFullPrimaryKey() + { + assertGuardrailViolated(String.format("DELETE FROM %s WHERE id = 1 AND name = 'v1'", currentTable())); + assertNoWarnings(); + } + + @Test + public void testMultiTableBatchGuardrailAllLiteral() + { + String table1 = currentTable(); + String table2 = createTable("CREATE TABLE %s (id int PRIMARY KEY, val text)"); + String query = String.format("BEGIN BATCH " + + "UPDATE %s.%s SET description = 'v1' WHERE id = 1 AND name = 'n1' AND age = 1; " + + "INSERT INTO %s.%s (id, val) VALUES (2, 'v2'); " + + "APPLY BATCH", + KEYSPACE, table1, KEYSPACE, table2); + + assertGuardrailViolated(query); + assertNoWarnings(); + } + + @Test + public void testSucceedWithOnlyBindMarkers() + { + assertGuardrailPassed(String.format("SELECT * FROM %s WHERE id = ? AND name = ?", currentTable())); + assertNoWarnings(); + } + + @Test + public void testSucceedWithOneBindMarkerOneLiteral() + { + + assertGuardrailPassed(String.format("SELECT * FROM %s WHERE id = ? AND name = 'v1'", currentTable())); + assertNoWarnings(); + } + + @Test + public void testSelectAllPasses() + { + assertGuardrailPassed(String.format("SELECT * FROM %s", currentTable())); + assertNoWarnings(); + } + + @Test + public void testLiteralInProjectionIsAllowed() + { + assertGuardrailPassed(String.format("SELECT id, (text)'const_val' FROM %s WHERE id = ?", currentTable())); + assertNoWarnings(); + } + + @Test + public void testInternalBypass() + { + assertGuardrailPassed("SELECT * FROM " + KEYSPACE + '.' + currentTable() + " WHERE id = 1", ClientState.forInternalCalls()); + } + + @Test + public void testSuperUserBypass() + { + AuthenticatedUser superUser = new AuthenticatedUser("super-user") + { + @Override + public boolean isSuper() + { + return true; + } + + @Override + public boolean isSystem() + { + return false; + } + + @Override + public boolean isAnonymous() + { + return false; + } + + @Override + public boolean canLogin() + { + return true; + } + }; + ClientState superUserState = ClientState.forExternalCalls(new InetSocketAddress("127.0.0.1", 9042)); + superUserState.login(superUser); + assertGuardrailPassed("SELECT * FROM " + KEYSPACE + '.' + currentTable() + " WHERE id = 1", superUserState); + } + + @Test + public void testSystemKeyspaceBypassForRegularUser() + { + assertGuardrailPassed("SELECT * FROM system.local WHERE key = 'local'"); + assertNoWarnings(); + } + + @Test + public void testGuardrailDisabledAlowsLiterals() + { + DatabaseDescriptor.setPreparedStatementsRequireParametersEnabled(false); + assertGuardrailPassed(String.format("SELECT * FROM %s WHERE id = 1", currentTable())); + assertWarnings(); + } + + @Test + public void testWarningIsIssuedWhenGuardrailIsAllowed() + { + DatabaseDescriptor.setPreparedStatementsRequireParametersEnabled(false); + assertGuardrailPassed(String.format("SELECT * FROM %s WHERE id = 1", currentTable())); + assertWarnings(); + Review Comment: `testWarningIsIssuedWhenGuardrailIsAllowed` duplicates the same steps/assertions as `testGuardrailDisabledAlowsLiterals`. Consider removing one or changing it to cover a distinct scenario (e.g., verifying the warning text includes keyspace/table, or that the strict mode rejects). ```suggestion // Capture warnings for this operation ClientWarn.instance.captureWarnings(); String tableName = currentTable(); assertGuardrailPassed(String.format("SELECT * FROM %s WHERE id = 1", tableName)); List<String> warnings = ClientWarn.instance.getWarnings(); Assert.assertNotNull("Warnings list should not be null", warnings); Assert.assertFalse("Expected at least one warning to be issued", warnings.isEmpty()); String firstWarning = warnings.get(0); Assert.assertTrue("Warning should mention keyspace", firstWarning.contains(KEYSPACE)); Assert.assertTrue("Warning should mention table name", firstWarning.contains(tableName)); ``` ########## src/java/org/apache/cassandra/service/StorageService.java: ########## @@ -4839,6 +4839,18 @@ public void setColumnIndexCacheSize(int cacheSizeInKB) logger.info("Updated column_index_cache_size to {}", cacheSizeInKB); } + @Override + public boolean getMispreparedStatementsEnabled() + { + return DatabaseDescriptor.getPreparedStatementsRequireParametersEnabled(); + } + + @Override + public void setMispreparedStatementsEnabled(boolean enabled) + { + DatabaseDescriptor.setPreparedStatementsRequireParametersEnabled(enabled); + } + Review Comment: The JMX methods are named `get/setMispreparedStatementsEnabled`, but they actually proxy `DatabaseDescriptor.get/setPreparedStatementsRequireParametersEnabled`. The names imply enabling misprepared statements (allowing the anti-pattern), while the underlying flag indicates enforcing parameters. Consider renaming these methods to match the underlying config semantics/key to avoid operator confusion (and to stay consistent with the GuardrailsMBean naming you choose). ```suggestion /** * Returns the value of the {@code prepared_statements_require_parameters_enabled} flag. * * This method is misnamed: the underlying configuration controls whether prepared statements * are required to use parameters (i.e. disallowing "misprepared" statements that inline * literal values), not whether such misprepared statements are enabled. * * @deprecated Use {@link #getPreparedStatementsRequireParametersEnabled()} instead. */ @Deprecated(since = "5.0") @Override public boolean getMispreparedStatementsEnabled() { return DatabaseDescriptor.getPreparedStatementsRequireParametersEnabled(); } /** * Sets the value of the {@code prepared_statements_require_parameters_enabled} flag. * * This method is misnamed: the underlying configuration controls whether prepared statements * are required to use parameters (i.e. disallowing "misprepared" statements that inline * literal values), not whether such misprepared statements are enabled. * * @deprecated Use {@link #setPreparedStatementsRequireParametersEnabled(boolean)} instead. */ @Deprecated(since = "5.0") @Override public void setMispreparedStatementsEnabled(boolean enabled) { DatabaseDescriptor.setPreparedStatementsRequireParametersEnabled(enabled); } /** * JMX accessor for the {@code prepared_statements_require_parameters_enabled} configuration flag. * * When {@code true}, prepared statements must use bind parameters instead of inlining literals. */ public boolean getPreparedStatementsRequireParametersEnabled() { return DatabaseDescriptor.getPreparedStatementsRequireParametersEnabled(); } /** * JMX mutator for the {@code prepared_statements_require_parameters_enabled} configuration flag. * * @param enabled {@code true} to require parameters for prepared statements, {@code false} to allow * prepared statements that inline literal values. */ public void setPreparedStatementsRequireParametersEnabled(boolean enabled) { DatabaseDescriptor.setPreparedStatementsRequireParametersEnabled(enabled); } ``` ########## test/unit/org/apache/cassandra/config/DatabaseDescriptorTest.java: ########## @@ -781,6 +781,27 @@ public void testRowIndexSizeWarnEqAbort() DatabaseDescriptor.applyReadThresholdsValidations(conf); } + @Test + public void testMispreparedStatementsEnabled() { + boolean originalValue = DatabaseDescriptor.getPreparedStatementsRequireParametersEnabled(); + try + { + DatabaseDescriptor.setPreparedStatementsRequireParametersEnabled(true); + Assert.assertTrue("Value should be true after setting to true", + DatabaseDescriptor.getPreparedStatementsRequireParametersEnabled()); + + DatabaseDescriptor.setPreparedStatementsRequireParametersEnabled(false); + Assert.assertFalse("Value should be false after setting to false", + DatabaseDescriptor.getPreparedStatementsRequireParametersEnabled()); + + Assert.assertTrue("Default value of misprepared_statement_enabled must be true", originalValue); Review Comment: This test asserts the default value is `true` (`Assert.assertTrue(... originalValue)`), but the new config field is initialized to `false` in `Config` and the yaml comment also says default is false. This assertion will be unstable/failing depending on test initialization; the test should assert the actual default (or avoid asserting a hard-coded default and only validate the setter/getter round-trip). ```suggestion ``` ########## test/unit/org/apache/cassandra/cql3/MispreparedStatementsTest.java: ########## @@ -0,0 +1,396 @@ +/* + * 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.cql3; + +import java.net.InetSocketAddress; +import java.util.List; + +import org.junit.After; +import org.junit.Assert; +import org.junit.Before; +import org.junit.BeforeClass; +import org.junit.Test; + +import org.apache.cassandra.auth.AuthenticatedUser; +import org.apache.cassandra.auth.CassandraAuthorizer; +import org.apache.cassandra.auth.PasswordAuthenticator; +import org.apache.cassandra.config.DatabaseDescriptor; +import org.apache.cassandra.db.guardrails.GuardrailViolatedException; +import org.apache.cassandra.db.guardrails.Guardrails; +import org.apache.cassandra.service.ClientState; +import org.apache.cassandra.service.ClientWarn; + +import static org.junit.Assert.assertTrue; + +public class MispreparedStatementsTest extends CQLTester +{ + private static final ClientState state = ClientState.forExternalCalls(new InetSocketAddress("127.0.0.1", 9042)); + + @BeforeClass + public static void setupGlobalConfig() + { + DatabaseDescriptor.setAuthenticator(new PasswordAuthenticator()); + DatabaseDescriptor.setAuthorizer(new CassandraAuthorizer()); + DatabaseDescriptor.setPreparedStatementsRequireParametersEnabled(true); + } + + @Before + public void setUp() + { + AuthenticatedUser nonSuperUser = new AuthenticatedUser("regular-user") + { + @Override + public boolean isSuper() + { + return false; + } + + @Override + public boolean isSystem() + { + return false; + } + + @Override + public boolean isAnonymous() + { + return false; + } + + @Override + public boolean canLogin() + { + return true; + } + }; + + state.login(nonSuperUser); + state.setKeyspace(KEYSPACE); + ClientWarn.instance.captureWarnings(); + createTable("CREATE TABLE %s (id int, description text, age int, name text, PRIMARY KEY (id, name, age))"); + } + + @After + public void tearDown() + { + DatabaseDescriptor.setPreparedStatementsRequireParametersEnabled(true); + } Review Comment: `tearDown()` unconditionally sets `prepared_statements_require_parameters_enabled` back to `true`, which can leak global state into other tests (especially since the default appears to be `false`). Capture the original value and restore it in `@After`/`@AfterClass` instead of hard-coding `true`. ########## src/java/org/apache/cassandra/db/guardrails/Guardrails.java: ########## @@ -1925,4 +1951,16 @@ private static long minimumTimestampAsRelativeMicros(@Nullable DurationSpec.Long ? Long.MIN_VALUE : (ClientState.getLastTimestampMicros() - duration.toMicroseconds()); } + + public static void onMisprepared(ClientState state, String keyspace, String tableName) + { + if (preparedStatementsRequireParametersEnabled.isEnabled(state) && preparedStatementsRequireParametersEnabled.enabled(state)) + { + preparedStatementsRequireParametersEnabled.fail(preparedStatementsRequireParametersEnabled.name + " is not allowed", state); + } + else + { + preparedStatementsRequireParametersEnabled.warn(MISPREPARED_STATEMENT_WARN_MESSAGE + "\n Keyspace: " + keyspace + "\n table: " + tableName); + } Review Comment: `onMisprepared` falls into the warning path whenever the guardrail is bypassed (internal calls, superusers, daemon not initialized) because the `else` runs when `enabled(state)` is false. This defeats the guardrail bypass semantics and can emit warnings for internal/superuser queries. Consider returning early when `!preparedStatementsRequireParametersEnabled.enabled(state)`, then using the config predicate to choose fail vs warn. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
