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:"));