From de385beea231878486ddfaca76caabd20c75c5c7 Mon Sep 17 00:00:00 2001 From: Stefan Bischof Date: Tue, 25 Aug 2026 10:22:51 +0200 Subject: [PATCH] fix(guard): reject data-modifying CTEs in SqlGuard A statement like `WITH x AS (UPDATE foo ...) SELECT ...` parses as a Select and passed every gate, because the resolver never populates its update/insert column lists for DML wrapped in a WITH item. The guard then returned the write statement as approved SQL. Add ReadOnlyStatementValidator, which walks the parsed Select tree (reusing TablesNamesFinder's recursion) and rejects any WITH item that wraps an UPDATE/INSERT/DELETE instead of a SELECT. Run it before resolve(), which also stops the INSERT-in-CTE ClassCastException leak. Signed-off-by: Stefan Bischof --- .../ReadOnlyStatementValidator.java | 88 +++++++++++++++++ .../jsqltranspiler/TranspilerSqlGuard.java | 20 ++++ .../integration/SqlGuardTest.java | 96 +++++++++++++++++++ 3 files changed, 204 insertions(+) create mode 100644 guard/jsqltranspiler/src/main/java/org/eclipse/daanse/sql/guard/jsqltranspiler/ReadOnlyStatementValidator.java diff --git a/guard/jsqltranspiler/src/main/java/org/eclipse/daanse/sql/guard/jsqltranspiler/ReadOnlyStatementValidator.java b/guard/jsqltranspiler/src/main/java/org/eclipse/daanse/sql/guard/jsqltranspiler/ReadOnlyStatementValidator.java new file mode 100644 index 0000000..9259e26 --- /dev/null +++ b/guard/jsqltranspiler/src/main/java/org/eclipse/daanse/sql/guard/jsqltranspiler/ReadOnlyStatementValidator.java @@ -0,0 +1,88 @@ +/* + * Copyright (c) 2024 Contributors to the Eclipse Foundation. + * + * This program and the accompanying materials are made + * available under the terms of the Eclipse Public License 2.0 + * which is available at https://www.eclipse.org/legal/epl-2.0/ + * + * SPDX-License-Identifier: EPL-2.0 + * + * Contributors: + * SmartCity Jena - initial + * Stefan Bischof (bipolis.org) - initial + */ + +package org.eclipse.daanse.sql.guard.jsqltranspiler; + +import java.util.Optional; + +import net.sf.jsqlparser.statement.ParenthesedStatement; +import net.sf.jsqlparser.statement.Statement; +import net.sf.jsqlparser.statement.select.ParenthesedSelect; +import net.sf.jsqlparser.statement.select.Select; +import net.sf.jsqlparser.statement.select.WithItem; +import net.sf.jsqlparser.statement.delete.ParenthesedDelete; +import net.sf.jsqlparser.statement.insert.ParenthesedInsert; +import net.sf.jsqlparser.statement.update.ParenthesedUpdate; +import net.sf.jsqlparser.util.TablesNamesFinder; + +/** + * Walks a parsed {@link Select} tree and detects data-modifying statements that + * are wrapped in a {@code WITH} item (a data-modifying CTE), such as + * {@code WITH x AS (UPDATE foo ...) SELECT ...}. + *

+ * Such statements parse as a {@code Select} and therefore pass the top-level + * {@code instanceof Select} gate, yet on PostgreSQL/MariaDB they execute the + * wrapped {@code UPDATE}/{@code INSERT}/{@code DELETE}. The upstream resolver + * does not populate its update/insert column lists for a {@code WithItem}, so it + * cannot be relied on to detect this case. + *

+ * This validator reuses {@link TablesNamesFinder}'s full recursive traversal + * (subqueries, joins, set operations, nested {@code WITH} clauses) and inspects + * every {@code WithItem}. A {@code WithItem} is only permitted to wrap a + * {@link ParenthesedSelect}; anything else is a violation. + */ +class ReadOnlyStatementValidator extends TablesNamesFinder { + + static final String DELETE_IS_NOT_PERMITTED = "DELETE is not permitted."; + static final String UPDATE_IS_NOT_PERMITTED = "UPDATE is not permitted."; + static final String INSERT_IS_NOT_PERMITTED = "INSERT is not permitted."; + static final String STATEMENT_IS_NOT_PERMITTED = "Statement is not permitted."; + + private String violation; + + /** + * Runs the read-only traversal over the given select and returns the first + * violation message, or an empty optional when the statement is read-only. + */ + Optional firstViolation(Select select) { + violation = null; + // drives the recursive traversal; the returned table set is irrelevant + getTables((Statement) select); + return Optional.ofNullable(violation); + } + + @Override + public Void visit(WithItem withItem, S context) { + ParenthesedStatement statement = withItem.getParenthesedStatement(); + if (statement instanceof ParenthesedSelect) { + // read-only CTE body: keep descending so nested WITH clauses are inspected too. + // The base implementation resolves getSelect(), which is only valid here. + return super.visit(withItem, context); + } + // Anything other than a ParenthesedSelect is a data-modifying (or otherwise + // unexpected) CTE. Record it and do NOT descend: getSelect() would throw. + if (violation == null) { + if (statement instanceof ParenthesedUpdate) { + violation = UPDATE_IS_NOT_PERMITTED; + } else if (statement instanceof ParenthesedInsert) { + violation = INSERT_IS_NOT_PERMITTED; + } else if (statement instanceof ParenthesedDelete) { + violation = DELETE_IS_NOT_PERMITTED; + } else { + violation = STATEMENT_IS_NOT_PERMITTED; + } + } + return null; + } +} diff --git a/guard/jsqltranspiler/src/main/java/org/eclipse/daanse/sql/guard/jsqltranspiler/TranspilerSqlGuard.java b/guard/jsqltranspiler/src/main/java/org/eclipse/daanse/sql/guard/jsqltranspiler/TranspilerSqlGuard.java index f916f54..81db60f 100644 --- a/guard/jsqltranspiler/src/main/java/org/eclipse/daanse/sql/guard/jsqltranspiler/TranspilerSqlGuard.java +++ b/guard/jsqltranspiler/src/main/java/org/eclipse/daanse/sql/guard/jsqltranspiler/TranspilerSqlGuard.java @@ -16,6 +16,7 @@ import java.util.ArrayList; import java.util.List; +import java.util.Optional; import java.util.Set; import java.util.regex.Pattern; @@ -90,6 +91,13 @@ public String guard(String sqlStr) throws GuardException { // we can test for SELECT, though in practise it won't protect us from harmful statements if (st instanceof Select) { + + // A statement can parse as a Select yet still carry a data-modifying + // statement wrapped in a WITH item (e.g. WITH x AS (UPDATE ...) SELECT ...). + // The resolver does not populate its update/insert column lists for that + // case, so reject it explicitly before anything else. + validateReadOnly((Select) st); + resolver.resolve(st); // select columns should not be empty @@ -172,6 +180,18 @@ public String guard(String sqlStr) throws GuardException { } + private static void validateReadOnly(Select select) throws GuardException { + Optional violation = new ReadOnlyStatementValidator().firstViolation(select); + if (violation.isPresent()) { + String message = violation.get(); + LOGGER.atInfo().log(message); + if (ReadOnlyStatementValidator.STATEMENT_IS_NOT_PERMITTED.equals(message)) { + throw new UnallowedStatementTypeGuardException(message); + } + throw new GuardException(message); + } + } + private static JdbcMetaData calculateMetaData(String currentCatalogName, String currentSchemaName, DatabaseCatalog databaseCatalog) { JdbcMetaData jdbcMetaData = new JdbcMetaData(currentCatalogName, currentSchemaName); diff --git a/guard/jsqltranspiler/src/test/java/org/eclipse/daanse/sql/guard/jsqltranspiler/integration/SqlGuardTest.java b/guard/jsqltranspiler/src/test/java/org/eclipse/daanse/sql/guard/jsqltranspiler/integration/SqlGuardTest.java index aa9f51b..7f788dc 100644 --- a/guard/jsqltranspiler/src/test/java/org/eclipse/daanse/sql/guard/jsqltranspiler/integration/SqlGuardTest.java +++ b/guard/jsqltranspiler/src/test/java/org/eclipse/daanse/sql/guard/jsqltranspiler/integration/SqlGuardTest.java @@ -242,6 +242,33 @@ public class SqlGuardTest { WITH cte1 AS (SELECT foo.id FROM foo), cte2 AS (SELECT fooFact.wrongCol FROM fooFact) SELECT * FROM cte1, cte2"""; + + private static final String SQL_CTE_UPDATE = """ + WITH x AS (UPDATE foo SET name='pwned' WHERE id=1 RETURNING id) SELECT id FROM foo"""; + + private static final String SQL_CTE_UPDATE_ALL_ROWS = """ + WITH x AS (UPDATE foo SET name='pwned') SELECT id FROM foo"""; + + private static final String SQL_MULTIPLE_CTE_UPDATE = """ + WITH x AS (UPDATE foo SET name='a' RETURNING id), + y AS (UPDATE foo SET name='b' RETURNING id) + SELECT id FROM foo"""; + + private static final String SQL_CTE_INSERT = """ + WITH x AS (INSERT INTO foo (id, name) VALUES (1, 'a') RETURNING id) SELECT id FROM foo"""; + + private static final String SQL_CTE_DELETE = """ + WITH x AS (DELETE FROM foo WHERE id=1 RETURNING id) SELECT id FROM foo"""; + + private static final String SQL_NESTED_SUBQUERY_CTE_UPDATE = """ + SELECT foo.id FROM foo + WHERE foo.id IN (SELECT sub.id FROM (WITH y AS (UPDATE foo SET name='z' RETURNING id) SELECT id FROM foo) sub)"""; + + private static final String UPDATE_IS_NOT_PERMITTED = "UPDATE is not permitted."; + + private static final String INSERT_IS_NOT_PERMITTED = "INSERT is not permitted."; + + private static final String DELETE_IS_NOT_PERMITTED = "DELETE is not permitted."; private static final String SQL_LEFT_JOIN_ON_WRONG_COLUMN = """ SELECT foo.id FROM foo LEFT JOIN fooFact ON foo.wrongCol = fooFact.id"""; @@ -993,6 +1020,75 @@ void testMultipleCteWrongColumn(@InjectService SqlGuardFactory sqlGuardFactory) } } + @Nested + @DisplayName("CTE Data-Modifying (DML) Security Tests") + class CteDmlSecurityTests { + + // A data-modifying CTE parses as a Select and resolves against the whitelisted + // table, so it must be rejected by the read-only gate, not the column resolver. + + @Test + void testCteUpdate(@InjectService SqlGuardFactory sqlGuardFactory) throws Exception { + DatabaseCatalog databaseCatalog = schemaWithTwoTableTwoCol(); + SqlGuard guard = sqlGuardFactory.create("", SCH, databaseCatalog, List.of(), dialect); + assertThatThrownBy(() -> guard.guard(SQL_CTE_UPDATE)) + .isInstanceOf(GuardException.class) + .hasMessage(UPDATE_IS_NOT_PERMITTED); + } + + @Test + void testCteUpdateAllRows(@InjectService SqlGuardFactory sqlGuardFactory) throws Exception { + DatabaseCatalog databaseCatalog = schemaWithTwoTableTwoCol(); + SqlGuard guard = sqlGuardFactory.create("", SCH, databaseCatalog, List.of(), dialect); + assertThatThrownBy(() -> guard.guard(SQL_CTE_UPDATE_ALL_ROWS)) + .isInstanceOf(GuardException.class) + .hasMessage(UPDATE_IS_NOT_PERMITTED); + } + + @Test + void testMultipleCteUpdate(@InjectService SqlGuardFactory sqlGuardFactory) throws Exception { + DatabaseCatalog databaseCatalog = schemaWithTwoTableTwoCol(); + SqlGuard guard = sqlGuardFactory.create("", SCH, databaseCatalog, List.of(), dialect); + assertThatThrownBy(() -> guard.guard(SQL_MULTIPLE_CTE_UPDATE)) + .isInstanceOf(GuardException.class) + .hasMessage(UPDATE_IS_NOT_PERMITTED); + } + + @Test + void testCteInsert(@InjectService SqlGuardFactory sqlGuardFactory) throws Exception { + DatabaseCatalog databaseCatalog = schemaWithTwoTableTwoCol(); + SqlGuard guard = sqlGuardFactory.create("", SCH, databaseCatalog, List.of(), dialect); + assertThatThrownBy(() -> guard.guard(SQL_CTE_INSERT)) + .isInstanceOf(GuardException.class) + .hasMessage(INSERT_IS_NOT_PERMITTED); + } + + @Test + void testCteDelete(@InjectService SqlGuardFactory sqlGuardFactory) throws Exception { + DatabaseCatalog databaseCatalog = schemaWithTwoTableTwoCol(); + SqlGuard guard = sqlGuardFactory.create("", SCH, databaseCatalog, List.of(), dialect); + assertThatThrownBy(() -> guard.guard(SQL_CTE_DELETE)) + .isInstanceOf(GuardException.class) + .hasMessage(DELETE_IS_NOT_PERMITTED); + } + + @Test + void testNestedSubqueryCteUpdate(@InjectService SqlGuardFactory sqlGuardFactory) throws Exception { + DatabaseCatalog databaseCatalog = schemaWithTwoTableTwoCol(); + SqlGuard guard = sqlGuardFactory.create("", SCH, databaseCatalog, List.of(), dialect); + assertThatThrownBy(() -> guard.guard(SQL_NESTED_SUBQUERY_CTE_UPDATE)) + .isInstanceOf(GuardException.class) + .hasMessage(UPDATE_IS_NOT_PERMITTED); + } + + @Test + void testValidCteStillPasses(@InjectService SqlGuardFactory sqlGuardFactory) throws Exception { + DatabaseCatalog databaseCatalog = schemaWithTwoTableTwoCol(); + SqlGuard guard = sqlGuardFactory.create("", SCH, databaseCatalog, List.of(), dialect); + assertNotNull(guard.guard(SQL_VALID_CTE)); + } + } + @Nested @DisplayName("JOIN Security Tests") class JoinSecurityTests {