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 {