From e94b8341ec8a238dfa1728a5779538ecdd4f6611 Mon Sep 17 00:00:00 2001 From: Nikolay Antonov Date: Thu, 12 Nov 2026 13:21:51 +0500 Subject: [PATCH] Hardening: enforce simple JDBC named query filenames Apply consistent query filename rules on Unix and Windows. Resolve query files within the selected server configuration directory. Add validation tests and document naming requirements and upgrade steps. Issue reported by: Sergey Bobrov --- .../configuring/jdbc-connector/jdbc_cfg.md | 2 + docs/upgrade/upgrade_pxf.md | 8 +++ docs/upgrade/upgrade_pxf_rpm.md | 2 + .../pxf/plugins/jdbc/JdbcAccessor.java | 13 ++-- .../pxf/plugins/jdbc/JdbcBasePlugin.java | 5 ++ .../pxf/plugins/jdbc/JdbcAccessorTest.java | 62 +++++++++++++++++-- .../jdbc/JdbcBasePluginTestInitialize.java | 26 ++++++++ 7 files changed, 110 insertions(+), 8 deletions(-) diff --git a/docs/administering/configuring/jdbc-connector/jdbc_cfg.md b/docs/administering/configuring/jdbc-connector/jdbc_cfg.md index cc17d2473..c1fd002f7 100644 --- a/docs/administering/configuring/jdbc-connector/jdbc_cfg.md +++ b/docs/administering/configuring/jdbc-connector/jdbc_cfg.md @@ -328,6 +328,8 @@ You may optionally provide the ending semicolon (`;`) for the SQL statement. The Apache Cloudberry user references a named query by specifying the query file name without the extension. For example, if you define a query in a file named `report.sql`, the name of that query is `report`. +The query name must be a file name without directory components. Forward slashes (`/`), backslashes (`\`), colons (`:`), and null characters are not allowed. The query file must reside directly in the selected JDBC server configuration directory. Symbolic links to query files outside that directory are not allowed. + Named queries are associated with a specific JDBC server configuration. You will provide the available query names to the Apache Cloudberry users that you allow to create external tables using the server configuration. #### Referencing a Named Query diff --git a/docs/upgrade/upgrade_pxf.md b/docs/upgrade/upgrade_pxf.md index abb291319..9e7bab2d7 100644 --- a/docs/upgrade/upgrade_pxf.md +++ b/docs/upgrade/upgrade_pxf.md @@ -89,3 +89,11 @@ After you install the new version of PXF, perform the following procedure: ``` shell gpadmin@coordinator$ pxf cluster start ``` + +## Backward-Incompatible Changes + +### JDBC Named Queries + +JDBC named query files must now reside directly in `$PXF_BASE/servers//`. Query names containing `/`, `\`, `:`, or null characters, and symbolic links to files outside that directory, are rejected. + +To migrate, copy affected SQL files into that directory, rename them as needed, and update external table `LOCATION` clauses or foreign table `resource` options to use `query:` without the `.sql` extension or directory components. Run `pxf cluster sync` before restarting PXF. See [JDBC Named Query Configuration](../administering/configuring/jdbc-connector/jdbc_cfg.md#jdbc-named-query-configuration). diff --git a/docs/upgrade/upgrade_pxf_rpm.md b/docs/upgrade/upgrade_pxf_rpm.md index 67623f4e5..d97fa9495 100644 --- a/docs/upgrade/upgrade_pxf_rpm.md +++ b/docs/upgrade/upgrade_pxf_rpm.md @@ -6,6 +6,8 @@ sidebar_position: 4 If you installed PXF from the `rpm` or `deb` package and have configured and are using PXF in your current Apache Cloudberry installation, you must perform some upgrade actions when you install a new version of PXF. +Before upgrading, review the [backward-incompatible changes](./upgrade_pxf.md#backward-incompatible-changes). + The PXF upgrade procedure has two parts. You perform one procedure before, and one procedure after, you install the new version: - [Step 1: Complete the PXF Pre-Upgrade Actions](#step-1-complete-the-pxf-pre-upgrade-actions) diff --git a/server/pxf-jdbc/src/main/java/org/apache/cloudberry/pxf/plugins/jdbc/JdbcAccessor.java b/server/pxf-jdbc/src/main/java/org/apache/cloudberry/pxf/plugins/jdbc/JdbcAccessor.java index 1472130d3..a2130fa6b 100644 --- a/server/pxf-jdbc/src/main/java/org/apache/cloudberry/pxf/plugins/jdbc/JdbcAccessor.java +++ b/server/pxf-jdbc/src/main/java/org/apache/cloudberry/pxf/plugins/jdbc/JdbcAccessor.java @@ -33,9 +33,9 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import java.io.File; import java.io.IOException; import java.nio.charset.Charset; +import java.nio.file.Path; import java.sql.Connection; import java.sql.PreparedStatement; import java.sql.ResultSet; @@ -354,11 +354,16 @@ private String getQueryText() { String queryText; try { - File queryFile = new File(serverDirectory, queryName + ".sql"); + Path serverDirectoryPath = Path.of(serverDirectory).toRealPath(); + Path queryFile = serverDirectoryPath.resolve(queryName + ".sql").toRealPath(); + // Resolve symbolic links and compare directory objects, not string prefixes. + if (!serverDirectoryPath.equals(queryFile.getParent())) { + throw new IllegalArgumentException("Query file must reside in the server configuration directory"); + } if (LOG.isDebugEnabled()) { - LOG.debug("Reading text of query={} from {}", queryName, queryFile.getCanonicalPath()); + LOG.debug("Reading text of query={} from {}", queryName, queryFile); } - queryText = FileUtils.readFileToString(queryFile, Charset.defaultCharset()); + queryText = FileUtils.readFileToString(queryFile.toFile(), Charset.defaultCharset()); } catch (IOException e) { throw new RuntimeException(String.format("Failed to read text of query %s : %s", queryName, e.getMessage()), e); } diff --git a/server/pxf-jdbc/src/main/java/org/apache/cloudberry/pxf/plugins/jdbc/JdbcBasePlugin.java b/server/pxf-jdbc/src/main/java/org/apache/cloudberry/pxf/plugins/jdbc/JdbcBasePlugin.java index 6a500a1ac..78412ec70 100644 --- a/server/pxf-jdbc/src/main/java/org/apache/cloudberry/pxf/plugins/jdbc/JdbcBasePlugin.java +++ b/server/pxf-jdbc/src/main/java/org/apache/cloudberry/pxf/plugins/jdbc/JdbcBasePlugin.java @@ -221,6 +221,11 @@ public void afterPropertiesSet() { if (StringUtils.isBlank(queryName)) { throw new IllegalArgumentException(String.format("Query name is not provided in data source [%s]", dataSource)); } + // A named query is a file name within the selected server configuration, not a path. + // Check both Unix and Windows syntax, including drive-relative paths and NTFS streams. + if (StringUtils.containsAny(queryName, "/\\:\0")) { + throw new IllegalArgumentException("Invalid query name: " + queryName); + } LOG.debug("Query name is {}", queryName); } else { tableName = dataSource; diff --git a/server/pxf-jdbc/src/test/java/org/apache/cloudberry/pxf/plugins/jdbc/JdbcAccessorTest.java b/server/pxf-jdbc/src/test/java/org/apache/cloudberry/pxf/plugins/jdbc/JdbcAccessorTest.java index e659fbc67..1205d977d 100644 --- a/server/pxf-jdbc/src/test/java/org/apache/cloudberry/pxf/plugins/jdbc/JdbcAccessorTest.java +++ b/server/pxf-jdbc/src/test/java/org/apache/cloudberry/pxf/plugins/jdbc/JdbcAccessorTest.java @@ -9,12 +9,17 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.api.io.TempDir; import org.mockito.ArgumentCaptor; import org.mockito.Mock; import org.mockito.MockedStatic; import org.mockito.junit.jupiter.MockitoExtension; import java.io.File; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.NoSuchFileException; +import java.nio.file.Path; import java.sql.Connection; import java.sql.DatabaseMetaData; import java.sql.PreparedStatement; @@ -23,10 +28,12 @@ import java.sql.Statement; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyBoolean; import static org.mockito.Mockito.times; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import static org.mockito.Mockito.mockStatic; @@ -34,6 +41,9 @@ @ExtendWith(MockitoExtension.class) public class JdbcAccessorTest { + @TempDir + Path tempDirectory; + private Configuration configuration; private JdbcAccessor accessor; private RequestContext context; @@ -103,22 +113,25 @@ public void testReadFromQueryFailsWhenServerDirectoryDoesNotExist() throws SQLEx accessor.afterPropertiesSet(); Exception e = assertThrows(RuntimeException.class, () -> accessor.openForRead()); - assertEquals("Failed to read text of query foo : File '/non-existing-directory/foo.sql' does not exist", e.getMessage()); + assertInstanceOf(NoSuchFileException.class, e.getCause().getCause()); + assertEquals("Failed to read text of query foo : /non-existing-directory", e.getMessage()); jdbcBasePluginMockedStatic.verify(() -> JdbcBasePlugin.closeConnection(mockConnection)); } } @Test - public void testReadFromQueryFailsWhenQueryFileIsNotFoundInExistingDirectory() throws SQLException { + public void testReadFromQueryFailsWhenQueryFileIsNotFoundInExistingDirectory() throws Exception { try (MockedStatic jdbcBasePluginMockedStatic = mockStatic(JdbcBasePlugin.class)) { wireMocksForRead(); - configuration.set("pxf.config.server.directory", "/tmp/"); + configuration.set("pxf.config.server.directory", tempDirectory.toString()); context.setDataSource("query:foo"); accessor.setRequestContext(context); accessor.afterPropertiesSet(); Exception e = assertThrows(RuntimeException.class, () -> accessor.openForRead()); - assertEquals("Failed to read text of query foo : File '/tmp/foo.sql' does not exist", e.getMessage()); + assertInstanceOf(NoSuchFileException.class, e.getCause().getCause()); + assertEquals("Failed to read text of query foo : " + + tempDirectory.toRealPath().resolve("foo.sql"), e.getMessage()); jdbcBasePluginMockedStatic.verify(() -> JdbcBasePlugin.closeConnection(mockConnection)); } } @@ -139,6 +152,47 @@ public void testReadFromQueryFailsWhenQueryFileIsEmpty() throws Exception { } } + @Test + public void testReadFromQueryFailsWhenSymbolicLinkEscapesServerDirectory() throws Exception { + Path serverDirectory = Files.createDirectory(tempDirectory.resolve("server")); + Path otherDirectory = Files.createDirectory(tempDirectory.resolve("server-other")); + Path outsideQuery = Files.write(otherDirectory.resolve("report.sql"), + "SELECT secret FROM private_table".getBytes(StandardCharsets.UTF_8)); + Files.createSymbolicLink(serverDirectory.resolve("report.sql"), outsideQuery); + configuration.set("pxf.config.server.directory", serverDirectory.toString()); + context.setDataSource("query:report"); + accessor.setRequestContext(context); + accessor.afterPropertiesSet(); + + try (MockedStatic jdbcBasePluginMockedStatic = mockStatic(JdbcBasePlugin.class)) { + wireMocksForRead(); + + Exception e = assertThrows(PxfRuntimeException.class, () -> accessor.openForRead()); + assertEquals("Query file must reside in the server configuration directory", e.getMessage()); + verify(mockConnection, never()).createStatement(); + jdbcBasePluginMockedStatic.verify(() -> JdbcBasePlugin.closeConnection(mockConnection)); + } + } + + @Test + public void testReadFromQueryWithSymbolicLinksWithinServerDirectory() throws Exception { + Path serverDirectory = Files.createDirectory(tempDirectory.resolve("server")); + Path queryFile = Files.write(serverDirectory.resolve("report.sql"), + "SELECT 1".getBytes(StandardCharsets.UTF_8)); + Files.createSymbolicLink(serverDirectory.resolve("alias.sql"), queryFile); + Path serverLink = Files.createSymbolicLink(tempDirectory.resolve("server-link"), serverDirectory); + configuration.set("pxf.config.server.directory", serverLink.toString()); + context.setDataSource("query:alias"); + wireMocksForReadWithCreateStatement(); + when(mockStatement.executeQuery(any())).thenReturn(mockResultSet); + + accessor.setRequestContext(context); + accessor.afterPropertiesSet(); + accessor.openForRead(); + + verify(mockStatement).executeQuery("SELECT FROM (SELECT 1) pxfsubquery"); + } + @Test public void testReadFromQuery() throws Exception { String serversDirectory = new File(this.getClass().getClassLoader().getResource("servers").toURI()).getCanonicalPath(); diff --git a/server/pxf-jdbc/src/test/java/org/apache/cloudberry/pxf/plugins/jdbc/JdbcBasePluginTestInitialize.java b/server/pxf-jdbc/src/test/java/org/apache/cloudberry/pxf/plugins/jdbc/JdbcBasePluginTestInitialize.java index 80f1a76f7..0a4346803 100644 --- a/server/pxf-jdbc/src/test/java/org/apache/cloudberry/pxf/plugins/jdbc/JdbcBasePluginTestInitialize.java +++ b/server/pxf-jdbc/src/test/java/org/apache/cloudberry/pxf/plugins/jdbc/JdbcBasePluginTestInitialize.java @@ -31,6 +31,8 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; import org.mockito.internal.util.reflection.FieldReader; import org.mockito.junit.jupiter.MockitoExtension; @@ -592,6 +594,30 @@ public void testDatasourceIsQuery() throws Exception { assertNull(getInternalState(plugin, "tableName")); } + @ParameterizedTest + @ValueSource(strings = {"report_2026-11", "report.v2", "monthly report", "résumé", "..report"}) + public void testDatasourceIsQueryWithValidFileName(String queryName) throws Exception { + plugin.setRequestContext(makeContextWithDataSource(makeConfiguration(), "query:" + queryName)); + plugin.afterPropertiesSet(); + + assertEquals(queryName, getInternalState(plugin, "queryName")); + assertNull(getInternalState(plugin, "tableName")); + } + + @ParameterizedTest + @ValueSource(strings = { + "../other-server/report", "../../report", "nested/report", "./report", "/tmp/report", + "..\\other-server\\report", "nested\\report", "C:\\queries\\report", "\\\\host\\queries\\report", + "C:report", "C:/queries/report", "\\report", "\\\\?\\C:\\queries\\report", + "\\\\.\\C:\\queries\\report", "report:stream", "report.sql::$DATA", "report\0ignored" + }) + public void testInitializationFailsWhenQueryNameContainsPathCharacters(String queryName) { + plugin.setRequestContext(makeContextWithDataSource(makeConfiguration(), "query:" + queryName)); + + Exception ex = assertThrows(IllegalArgumentException.class, plugin::afterPropertiesSet); + assertEquals("Invalid query name: " + queryName, ex.getMessage()); + } + @Test public void testInitializationFailsWhenDatasourceIsEmptyQuery() { plugin.setRequestContext(makeContextWithDataSource(makeConfiguration(), "query:"));