Skip to content

Add whitelist to block non-query statements in unified SQL path #5330

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dai-chen merged 1 commit into from
Apr 13, 2026
Merged

Add whitelist to block non-query statements in unified SQL path #5330

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import org.apache.calcite.rel.RelRoot;
import org.apache.calcite.rel.core.Sort;
import org.apache.calcite.rel.logical.LogicalSort;
import org.apache.calcite.sql.SqlKind;
import org.apache.calcite.sql.SqlNode;
import org.apache.calcite.tools.Frameworks;
import org.apache.calcite.tools.Planner;
Expand Down Expand Up @@ -60,8 +61,7 @@ public UnifiedQueryPlanner(UnifiedQueryContext context) {
public RelNode plan(String query) {
try {
return context.measure(ANALYZE, () -> strategy.plan(query));
} catch (SyntaxCheckException e) {
// Re-throw syntax error without wrapping
} catch (SyntaxCheckException | UnsupportedOperationException e) {
throw e;
} catch (Exception e) {
throw new IllegalStateException("Failed to plan query", e);
Expand All @@ -82,6 +82,11 @@ private static class CalciteNativeStrategy implements PlanningStrategy {
public RelNode plan(String query) throws Exception {
try (Planner planner = Frameworks.getPlanner(context.getPlanContext().config)) {
SqlNode parsed = planner.parse(query);
if (!parsed.isA(SqlKind.QUERY)) {
throw new UnsupportedOperationException(
"Only query statements are supported. Got: " + parsed.getKind());
Copy link
Copy Markdown
Collaborator

@Swiddis Swiddis Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit (clarity): Would a user know what this means?

Intuitively I'm guessing "query" means "non-modifying & non-explain," so maybe something like "Only read-only querying statements are supported, got an {explain} query" with some case-based substitution.

Copy link
Copy Markdown
Collaborator Author

@dai-chen dai-chen Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point on clarity. But I think EXPLAIN, SHOW, and DESCRIBE are also read-only and not supported here either? I'll give more careful thought for the wording in this new api module later. Thanks!

}

SqlNode rewritten = parsed.accept(NamedArgRewriter.INSTANCE);
SqlNode validated = planner.validate(rewritten);
RelRoot relRoot = planner.rel(validated);
Expand Down
47 changes: 47 additions & 0 deletions api/src/test/java/org/opensearch/sql/api/UnifiedQueryPlannerSqlTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertThrows;

import java.util.List;
import java.util.Map;
import org.apache.calcite.schema.Schema;
import org.apache.calcite.schema.impl.AbstractSchema;
Expand Down Expand Up @@ -211,4 +212,50 @@ public void testSqlQueryPlanningWithMultipleCatalogs() {
public void testInvalidSqlThrowsException() {
assertThrows(IllegalStateException.class, () -> planner.plan("SELECT FROM"));
}

@Test
public void testNonQueryStatementsBlockedByWhitelist() {
List.of(
"""
INSERT INTO catalog.employees (id, name, age, department)
VALUES (99, 'injected', 0, 'hacked')\
""",
"""
DELETE FROM catalog.employees
WHERE age > 30\
""",
"""
UPDATE catalog.employees
SET department = 'Fired'
WHERE age > 50\
""",
"""
EXPLAIN PLAN FOR
SELECT * FROM catalog.employees\
""",
"""
MERGE INTO catalog.employees AS t
USING (SELECT 99 AS id) AS s ON t.id = s.id
WHEN MATCHED THEN UPDATE SET name = 'hacked'\
""")
.forEach(
sql ->
givenInvalidQuery(sql).assertErrorMessage("Only query statements are supported"));
}

@Test
public void testNonQueryStatementsBlockedByParser() {
List.of(
"""
CREATE MATERIALIZED VIEW mv AS
SELECT department, count(*)
FROM catalog.employees
GROUP BY department\
""",
"""
SHOW TABLES\
""")
.forEach(
sql -> givenInvalidQuery(sql).assertErrorMessage("Incorrect syntax near the keyword"));
}
}
2 changes: 1 addition & 1 deletion api/src/test/java/org/opensearch/sql/api/UnifiedQueryPlannerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ public void testPPLQueryPlanningWithMultipleCatalogsAndDefaultNamespace() {
assertNotNull("Plan should be created with multiple catalogs", plan);
}

@Test(expected = IllegalStateException.class)
@Test(expected = UnsupportedOperationException.class)
public void testUnsupportedStatementType() {
planner.plan("explain source = catalog.employees"); // explain statement
}
Expand Down
Loading