Skip to content

Commit 3f740fb

Browse files
authored
Add query-type whitelist to unified SQL execution path (#5330)
Validate parsed SQL statements against Calcite's SqlKind.QUERY set before planning, rejecting DML (INSERT, DELETE, UPDATE, MERGE) and DDL statements. This brings the unified path in line with the legacy grammar's parser-level restriction to read-only queries. Whitelist: SqlKind.QUERY (SELECT, UNION, INTERSECT, EXCEPT, VALUES, WITH, ORDER_BY, EXPLICIT_TABLE). Note: EXPLAIN is also blocked because Calcite's SqlToRelConverter does not handle SqlExplain in convertQueryRecursive. Supporting EXPLAIN requires unwrapping the inner query and formatting the plan separately. Signed-off-by: Chen Dai <daichen@amazon.com>
1 parent f61f14d commit 3f740fb

3 files changed

Lines changed: 55 additions & 3 deletions

File tree

api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
import org.apache.calcite.rel.RelRoot;
1515
import org.apache.calcite.rel.core.Sort;
1616
import org.apache.calcite.rel.logical.LogicalSort;
17+
import org.apache.calcite.sql.SqlKind;
1718
import org.apache.calcite.sql.SqlNode;
1819
import org.apache.calcite.tools.Frameworks;
1920
import org.apache.calcite.tools.Planner;
@@ -60,8 +61,7 @@ public UnifiedQueryPlanner(UnifiedQueryContext context) {
6061
public RelNode plan(String query) {
6162
try {
6263
return context.measure(ANALYZE, () -> strategy.plan(query));
63-
} catch (SyntaxCheckException e) {
64-
// Re-throw syntax error without wrapping
64+
} catch (SyntaxCheckException | UnsupportedOperationException e) {
6565
throw e;
6666
} catch (Exception e) {
6767
throw new IllegalStateException("Failed to plan query", e);
@@ -82,6 +82,11 @@ private static class CalciteNativeStrategy implements PlanningStrategy {
8282
public RelNode plan(String query) throws Exception {
8383
try (Planner planner = Frameworks.getPlanner(context.getPlanContext().config)) {
8484
SqlNode parsed = planner.parse(query);
85+
if (!parsed.isA(SqlKind.QUERY)) {
86+
throw new UnsupportedOperationException(
87+
"Only query statements are supported. Got: " + parsed.getKind());
88+
}
89+
8590
SqlNode rewritten = parsed.accept(NamedArgRewriter.INSTANCE);
8691
SqlNode validated = planner.validate(rewritten);
8792
RelRoot relRoot = planner.rel(validated);

api/src/test/java/org/opensearch/sql/api/UnifiedQueryPlannerSqlTest.java

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
import static org.junit.Assert.assertNotNull;
99
import static org.junit.Assert.assertThrows;
1010

11+
import java.util.List;
1112
import java.util.Map;
1213
import org.apache.calcite.schema.Schema;
1314
import org.apache.calcite.schema.impl.AbstractSchema;
@@ -211,4 +212,50 @@ public void testSqlQueryPlanningWithMultipleCatalogs() {
211212
public void testInvalidSqlThrowsException() {
212213
assertThrows(IllegalStateException.class, () -> planner.plan("SELECT FROM"));
213214
}
215+
216+
@Test
217+
public void testNonQueryStatementsBlockedByWhitelist() {
218+
List.of(
219+
"""
220+
INSERT INTO catalog.employees (id, name, age, department)
221+
VALUES (99, 'injected', 0, 'hacked')\
222+
""",
223+
"""
224+
DELETE FROM catalog.employees
225+
WHERE age > 30\
226+
""",
227+
"""
228+
UPDATE catalog.employees
229+
SET department = 'Fired'
230+
WHERE age > 50\
231+
""",
232+
"""
233+
EXPLAIN PLAN FOR
234+
SELECT * FROM catalog.employees\
235+
""",
236+
"""
237+
MERGE INTO catalog.employees AS t
238+
USING (SELECT 99 AS id) AS s ON t.id = s.id
239+
WHEN MATCHED THEN UPDATE SET name = 'hacked'\
240+
""")
241+
.forEach(
242+
sql ->
243+
givenInvalidQuery(sql).assertErrorMessage("Only query statements are supported"));
244+
}
245+
246+
@Test
247+
public void testNonQueryStatementsBlockedByParser() {
248+
List.of(
249+
"""
250+
CREATE MATERIALIZED VIEW mv AS
251+
SELECT department, count(*)
252+
FROM catalog.employees
253+
GROUP BY department\
254+
""",
255+
"""
256+
SHOW TABLES\
257+
""")
258+
.forEach(
259+
sql -> givenInvalidQuery(sql).assertErrorMessage("Incorrect syntax near the keyword"));
260+
}
214261
}

api/src/test/java/org/opensearch/sql/api/UnifiedQueryPlannerTest.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,7 @@ public void testPPLQueryPlanningWithMultipleCatalogsAndDefaultNamespace() {
106106
assertNotNull("Plan should be created with multiple catalogs", plan);
107107
}
108108

109-
@Test(expected = IllegalStateException.class)
109+
@Test(expected = UnsupportedOperationException.class)
110110
public void testUnsupportedStatementType() {
111111
planner.plan("explain source = catalog.employees"); // explain statement
112112
}

0 commit comments

Comments
 (0)