diff --git a/catalogs/catalog-jdbc-common/src/main/java/com/datastrato/gravitino/catalog/jdbc/JdbcCatalog.java b/catalogs/catalog-jdbc-common/src/main/java/com/datastrato/gravitino/catalog/jdbc/JdbcCatalog.java index 6c28fae23a1..a621088b9c4 100644 --- a/catalogs/catalog-jdbc-common/src/main/java/com/datastrato/gravitino/catalog/jdbc/JdbcCatalog.java +++ b/catalogs/catalog-jdbc-common/src/main/java/com/datastrato/gravitino/catalog/jdbc/JdbcCatalog.java @@ -13,6 +13,7 @@ import com.datastrato.gravitino.connector.CatalogOperations; import com.datastrato.gravitino.connector.PropertiesMetadata; import com.datastrato.gravitino.connector.PropertyEntry; +import com.datastrato.gravitino.connector.capability.Capability; import java.util.Collections; import java.util.Map; @@ -52,6 +53,11 @@ protected CatalogOperations newOps(Map config) { return ops; } + @Override + public Capability newCapability() { + return new JdbcCatalogCapability(); + } + /** @return The {@link JdbcExceptionConverter} to be used by the catalog. */ protected JdbcExceptionConverter createExceptionConverter() { return new JdbcExceptionConverter() {}; diff --git a/catalogs/catalog-jdbc-common/src/main/java/com/datastrato/gravitino/catalog/jdbc/JdbcCatalogCapability.java b/catalogs/catalog-jdbc-common/src/main/java/com/datastrato/gravitino/catalog/jdbc/JdbcCatalogCapability.java new file mode 100644 index 00000000000..32cba18b8cd --- /dev/null +++ b/catalogs/catalog-jdbc-common/src/main/java/com/datastrato/gravitino/catalog/jdbc/JdbcCatalogCapability.java @@ -0,0 +1,36 @@ +/* + * Copyright 2024 Datastrato Pvt Ltd. + * This software is licensed under the Apache License version 2. + */ +package com.datastrato.gravitino.catalog.jdbc; + +import com.datastrato.gravitino.connector.capability.Capability; +import com.datastrato.gravitino.connector.capability.CapabilityResult; + +public class JdbcCatalogCapability implements Capability { + /** + * Regular expression explanation: Regex that matches any string that maybe a filename with an + * optional extension We adopt a blacklist approach that excludes filename or extension that + * contains '.', '/', or '\' ^[^.\/\\]+(\.[^.\/\\]+)?$ + * + *

^ - Start of the string + * + *

[^.\/\\]+ - matches any filename string that does not contain '.', '/', or '\' + * + *

(\.[^.\/\\]+)? - matches an optional extension + * + *

$ - End of the string + */ + // We use sqlite name pattern to be the default pattern for JDBC catalog for testing purposes + public static final String SQLITE_NAME_PATTERN = "^[^.\\/\\\\]+(\\.[^.\\/\\\\]+)?$"; + + @Override + public CapabilityResult specificationOnName(Scope scope, String name) { + // TODO: Validate the name against reserved words + if (!name.matches(SQLITE_NAME_PATTERN)) { + return CapabilityResult.unsupported( + String.format("The %s name '%s' is illegal.", scope, name)); + } + return CapabilityResult.SUPPORTED; + } +} diff --git a/catalogs/catalog-jdbc-doris/src/main/java/com/datastrato/gravitino/catalog/doris/DorisCatalog.java b/catalogs/catalog-jdbc-doris/src/main/java/com/datastrato/gravitino/catalog/doris/DorisCatalog.java index 035182f4e97..b6a322ed65c 100644 --- a/catalogs/catalog-jdbc-doris/src/main/java/com/datastrato/gravitino/catalog/doris/DorisCatalog.java +++ b/catalogs/catalog-jdbc-doris/src/main/java/com/datastrato/gravitino/catalog/doris/DorisCatalog.java @@ -17,6 +17,7 @@ import com.datastrato.gravitino.catalog.jdbc.operation.JdbcDatabaseOperations; import com.datastrato.gravitino.catalog.jdbc.operation.JdbcTableOperations; import com.datastrato.gravitino.connector.CatalogOperations; +import com.datastrato.gravitino.connector.capability.Capability; import java.util.Map; /** Implementation of a Doris catalog in Gravitino. */ @@ -38,6 +39,11 @@ protected CatalogOperations newOps(Map config) { createJdbcColumnDefaultValueConverter()); } + @Override + public Capability newCapability() { + return new DorisCatalogCapability(); + } + @Override protected JdbcExceptionConverter createExceptionConverter() { return new DorisExceptionConverter(); diff --git a/catalogs/catalog-jdbc-doris/src/main/java/com/datastrato/gravitino/catalog/doris/DorisCatalogCapability.java b/catalogs/catalog-jdbc-doris/src/main/java/com/datastrato/gravitino/catalog/doris/DorisCatalogCapability.java new file mode 100644 index 00000000000..ffa7ec057ed --- /dev/null +++ b/catalogs/catalog-jdbc-doris/src/main/java/com/datastrato/gravitino/catalog/doris/DorisCatalogCapability.java @@ -0,0 +1,13 @@ +/* + * Copyright 2024 Datastrato Pvt Ltd. + * This software is licensed under the Apache License version 2. + */ +package com.datastrato.gravitino.catalog.doris; + +import com.datastrato.gravitino.connector.capability.Capability; + +public class DorisCatalogCapability implements Capability { + // Doris best practice mention that the name should be in lowercase, separated by underscores + // https://doris.apache.org/docs/2.0/table-design/best-practice/ + // We can use the more general DEFAULT_NAME_PATTERN for Doris and update as needed in the future +} diff --git a/catalogs/catalog-jdbc-doris/src/test/java/com/datastrato/gravitino/catalog/doris/integration/test/CatalogDorisIT.java b/catalogs/catalog-jdbc-doris/src/test/java/com/datastrato/gravitino/catalog/doris/integration/test/CatalogDorisIT.java index 5c542a9f148..42b8b5bff67 100644 --- a/catalogs/catalog-jdbc-doris/src/test/java/com/datastrato/gravitino/catalog/doris/integration/test/CatalogDorisIT.java +++ b/catalogs/catalog-jdbc-doris/src/test/java/com/datastrato/gravitino/catalog/doris/integration/test/CatalogDorisIT.java @@ -26,18 +26,22 @@ import com.datastrato.gravitino.rel.expressions.NamedReference; import com.datastrato.gravitino.rel.expressions.distributions.Distribution; import com.datastrato.gravitino.rel.expressions.distributions.Distributions; +import com.datastrato.gravitino.rel.expressions.sorts.SortOrder; import com.datastrato.gravitino.rel.expressions.transforms.Transforms; import com.datastrato.gravitino.rel.indexes.Index; import com.datastrato.gravitino.rel.indexes.Indexes; import com.datastrato.gravitino.rel.types.Types; +import com.datastrato.gravitino.utils.RandomNameUtils; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Maps; import java.io.IOException; import java.util.Arrays; import java.util.Collections; +import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.concurrent.TimeUnit; +import org.apache.commons.lang3.StringUtils; import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Assertions; @@ -256,6 +260,65 @@ void testDropDorisSchema() { }); } + @Test + void testSchemaWithIllegalName() { + SupportsSchemas schemas = catalog.asSchemas(); + String databaseName = RandomNameUtils.genRandomName("it_db"); + Map properties = new HashMap<>(); + String comment = "comment"; + + // should throw an exception with string that might contain SQL injection + String sqlInjection = databaseName + "`; DROP TABLE important_table; -- "; + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + schemas.createSchema(sqlInjection, comment, properties); + }); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + schemas.dropSchema(sqlInjection, false); + }); + + String sqlInjection1 = databaseName + "`; SLEEP(10); -- "; + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + schemas.createSchema(sqlInjection1, comment, properties); + }); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + schemas.dropSchema(sqlInjection1, false); + }); + + String sqlInjection2 = + databaseName + "`; UPDATE Users SET password = 'newpassword' WHERE username = 'admin'; -- "; + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + schemas.createSchema(sqlInjection2, comment, properties); + }); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + schemas.dropSchema(sqlInjection2, false); + }); + + // should throw an exception with input that has more than 64 characters + String invalidInput = StringUtils.repeat("a", 65); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + schemas.createSchema(invalidInput, comment, properties); + }); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + schemas.dropSchema(invalidInput, false); + }); + } + @Test void testDorisTableBasicOperation() { // create a table @@ -299,6 +362,118 @@ void testDorisTableBasicOperation() { newTableName, table_comment, Arrays.asList(columns), properties, indexes, renamedTable); } + @Test + void testDorisIllegalTableName() { + Map properties = createTableProperties(); + TableCatalog tableCatalog = catalog.asTableCatalog(); + String table_name = "t123"; + + String t1_name = table_name + "`; DROP TABLE important_table; -- "; + Column t1_col = Column.of(t1_name, Types.LongType.get(), "id", false, false, null); + Column[] columns = {t1_col}; + Index[] t1_indexes = {Indexes.unique("u1_key", new String[][] {{t1_name}})}; + NameIdentifier tableIdentifier = + NameIdentifier.of(metalakeName, catalogName, schemaName, t1_name); + + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + tableCatalog.createTable( + tableIdentifier, + columns, + table_comment, + properties, + Transforms.EMPTY_TRANSFORM, + Distributions.NONE, + new SortOrder[0], + t1_indexes); + }); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + catalog.asTableCatalog().dropTable(tableIdentifier); + }); + + String t2_name = table_name + "`; SLEEP(10); -- "; + Column t2_col = Column.of(t2_name, Types.LongType.get(), "id", false, false, null); + Index[] t2_indexes = {Indexes.unique("u2_key", new String[][] {{t2_name}})}; + Column[] columns2 = new Column[] {t2_col}; + NameIdentifier tableIdentifier2 = + NameIdentifier.of(metalakeName, catalogName, schemaName, t2_name); + + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + tableCatalog.createTable( + tableIdentifier2, + columns2, + table_comment, + properties, + Transforms.EMPTY_TRANSFORM, + Distributions.NONE, + new SortOrder[0], + t2_indexes); + }); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + catalog.asTableCatalog().dropTable(tableIdentifier2); + }); + + String t3_name = + table_name + "`; UPDATE Users SET password = 'newpassword' WHERE username = 'admin'; -- "; + Column t3_col = Column.of(t3_name, Types.LongType.get(), "id", false, false, null); + Index[] t3_indexes = {Indexes.unique("u3_key", new String[][] {{t3_name}})}; + Column[] columns3 = new Column[] {t3_col}; + NameIdentifier tableIdentifier3 = + NameIdentifier.of(metalakeName, catalogName, schemaName, t3_name); + + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + tableCatalog.createTable( + tableIdentifier3, + columns3, + table_comment, + properties, + Transforms.EMPTY_TRANSFORM, + Distributions.NONE, + new SortOrder[0], + t3_indexes); + }); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + catalog.asTableCatalog().dropTable(tableIdentifier3); + }); + + String invalidInput = StringUtils.repeat("a", 65); + Column t4_col = Column.of(invalidInput, Types.LongType.get(), "id", false, false, null); + Index[] t4_indexes = {Indexes.unique("u4_key", new String[][] {{invalidInput}})}; + Column[] columns4 = new Column[] {t4_col}; + NameIdentifier tableIdentifier4 = + NameIdentifier.of(metalakeName, catalogName, schemaName, invalidInput); + + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + tableCatalog.createTable( + tableIdentifier4, + columns4, + table_comment, + properties, + Transforms.EMPTY_TRANSFORM, + Distributions.NONE, + new SortOrder[0], + t4_indexes); + }); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + catalog.asTableCatalog().dropTable(tableIdentifier4); + }); + } + @Test void testAlterDorisTable() { // create a table diff --git a/catalogs/catalog-jdbc-mysql/src/main/java/com/datastrato/gravitino/catalog/mysql/MysqlCatalog.java b/catalogs/catalog-jdbc-mysql/src/main/java/com/datastrato/gravitino/catalog/mysql/MysqlCatalog.java index 0ed1c3e8441..c8bb6bc1b7c 100644 --- a/catalogs/catalog-jdbc-mysql/src/main/java/com/datastrato/gravitino/catalog/mysql/MysqlCatalog.java +++ b/catalogs/catalog-jdbc-mysql/src/main/java/com/datastrato/gravitino/catalog/mysql/MysqlCatalog.java @@ -18,6 +18,7 @@ import com.datastrato.gravitino.catalog.mysql.operation.MysqlTableOperations; import com.datastrato.gravitino.connector.CatalogOperations; import com.datastrato.gravitino.connector.PropertiesMetadata; +import com.datastrato.gravitino.connector.capability.Capability; import java.util.Map; /** Implementation of a Mysql catalog in Gravitino. */ @@ -42,6 +43,11 @@ protected CatalogOperations newOps(Map config) { createJdbcColumnDefaultValueConverter()); } + @Override + public Capability newCapability() { + return new MysqlCatalogCapability(); + } + @Override protected JdbcExceptionConverter createExceptionConverter() { return new MysqlExceptionConverter(); diff --git a/catalogs/catalog-jdbc-mysql/src/main/java/com/datastrato/gravitino/catalog/mysql/MysqlCatalogCapability.java b/catalogs/catalog-jdbc-mysql/src/main/java/com/datastrato/gravitino/catalog/mysql/MysqlCatalogCapability.java new file mode 100644 index 00000000000..44dafbdd389 --- /dev/null +++ b/catalogs/catalog-jdbc-mysql/src/main/java/com/datastrato/gravitino/catalog/mysql/MysqlCatalogCapability.java @@ -0,0 +1,37 @@ +/* + * Copyright 2024 Datastrato Pvt Ltd. + * This software is licensed under the Apache License version 2. + */ +package com.datastrato.gravitino.catalog.mysql; + +import com.datastrato.gravitino.connector.capability.Capability; +import com.datastrato.gravitino.connector.capability.CapabilityResult; + +public class MysqlCatalogCapability implements Capability { + /** + * Regular expression explanation: ^[\w\p{L}-$/=]{1,64}$ + * + *

^ - Start of the string + * + *

[\w\p{L}-$/=]{1,64} - Consist of 1 to 64 characters of letters (both cases), digits, + * underscores, any kind of letter from any language, hyphens, dollar signs, slashes or equal + * signs + * + *

\w - matches [a-zA-Z0-9_] + * + *

\p{L} - matches any kind of letter from any language + * + *

$ - End of the string + */ + public static final String MYSQL_NAME_PATTERN = "^[\\w\\p{L}-$/=]{1,64}$"; + + @Override + public CapabilityResult specificationOnName(Scope scope, String name) { + // TODO: Validate the name against reserved words + if (!name.matches(MYSQL_NAME_PATTERN)) { + return CapabilityResult.unsupported( + String.format("The %s name '%s' is illegal.", scope, name)); + } + return CapabilityResult.SUPPORTED; + } +} diff --git a/catalogs/catalog-jdbc-mysql/src/test/java/com/datastrato/gravitino/catalog/mysql/integration/test/CatalogMysqlIT.java b/catalogs/catalog-jdbc-mysql/src/test/java/com/datastrato/gravitino/catalog/mysql/integration/test/CatalogMysqlIT.java index 8230f2eb2f9..c6ce8c70f37 100644 --- a/catalogs/catalog-jdbc-mysql/src/test/java/com/datastrato/gravitino/catalog/mysql/integration/test/CatalogMysqlIT.java +++ b/catalogs/catalog-jdbc-mysql/src/test/java/com/datastrato/gravitino/catalog/mysql/integration/test/CatalogMysqlIT.java @@ -43,6 +43,7 @@ import com.datastrato.gravitino.rel.indexes.Indexes; import com.datastrato.gravitino.rel.types.Decimal; import com.datastrato.gravitino.rel.types.Types; +import com.datastrato.gravitino.utils.RandomNameUtils; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Maps; import com.google.common.collect.Sets; @@ -50,6 +51,7 @@ import java.sql.SQLException; import java.util.Arrays; import java.util.Collections; +import java.util.HashMap; import java.util.Map; import java.util.Objects; import java.util.Set; @@ -1219,6 +1221,118 @@ void testMySQLSpecialTableName() { t4_name, table_comment, Arrays.asList(t4_col), properties, t4_indexes, t4); } + @Test + void testMySqlIllegalTableName() { + Map properties = createProperties(); + TableCatalog tableCatalog = catalog.asTableCatalog(); + String table_name = "t123"; + + String t1_name = table_name + "`; DROP TABLE important_table; -- "; + Column t1_col = Column.of(t1_name, Types.LongType.get(), "id", false, false, null); + Column[] columns = {t1_col}; + Index[] t1_indexes = {Indexes.unique("u1_key", new String[][] {{t1_name}})}; + NameIdentifier tableIdentifier = + NameIdentifier.of(metalakeName, catalogName, schemaName, t1_name); + + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + tableCatalog.createTable( + tableIdentifier, + columns, + table_comment, + properties, + Transforms.EMPTY_TRANSFORM, + Distributions.NONE, + new SortOrder[0], + t1_indexes); + }); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + catalog.asTableCatalog().dropTable(tableIdentifier); + }); + + String t2_name = table_name + "`; SLEEP(10); -- "; + Column t2_col = Column.of(t2_name, Types.LongType.get(), "id", false, false, null); + Index[] t2_indexes = {Indexes.unique("u2_key", new String[][] {{t2_name}})}; + Column[] columns2 = new Column[] {t2_col}; + NameIdentifier tableIdentifier2 = + NameIdentifier.of(metalakeName, catalogName, schemaName, t2_name); + + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + tableCatalog.createTable( + tableIdentifier2, + columns2, + table_comment, + properties, + Transforms.EMPTY_TRANSFORM, + Distributions.NONE, + new SortOrder[0], + t2_indexes); + }); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + catalog.asTableCatalog().dropTable(tableIdentifier2); + }); + + String t3_name = + table_name + "`; UPDATE Users SET password = 'newpassword' WHERE username = 'admin'; -- "; + Column t3_col = Column.of(t3_name, Types.LongType.get(), "id", false, false, null); + Index[] t3_indexes = {Indexes.unique("u3_key", new String[][] {{t3_name}})}; + Column[] columns3 = new Column[] {t3_col}; + NameIdentifier tableIdentifier3 = + NameIdentifier.of(metalakeName, catalogName, schemaName, t3_name); + + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + tableCatalog.createTable( + tableIdentifier3, + columns3, + table_comment, + properties, + Transforms.EMPTY_TRANSFORM, + Distributions.NONE, + new SortOrder[0], + t3_indexes); + }); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + catalog.asTableCatalog().dropTable(tableIdentifier3); + }); + + String invalidInput = StringUtils.repeat("a", 65); + Column t4_col = Column.of(invalidInput, Types.LongType.get(), "id", false, false, null); + Index[] t4_indexes = {Indexes.unique("u4_key", new String[][] {{invalidInput}})}; + Column[] columns4 = new Column[] {t4_col}; + NameIdentifier tableIdentifier4 = + NameIdentifier.of(metalakeName, catalogName, schemaName, invalidInput); + + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + tableCatalog.createTable( + tableIdentifier4, + columns4, + table_comment, + properties, + Transforms.EMPTY_TRANSFORM, + Distributions.NONE, + new SortOrder[0], + t4_indexes); + }); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + catalog.asTableCatalog().dropTable(tableIdentifier4); + }); + } + @Test void testMySQLTableNameCaseSensitive() { Column col1 = Column.of("col_1", Types.LongType.get(), "id", false, false, null); @@ -1307,8 +1421,68 @@ void testNameSpec() { Assertions.assertTrue(catalog.asTableCatalog().dropTable(tableIdent)); Assertions.assertFalse(catalog.asTableCatalog().tableExists(tableIdent)); - Assertions.assertFalse(catalog.asTableCatalog().purgeTable(tableIdent)); + Assertions.assertThrows( + UnsupportedOperationException.class, + () -> { + catalog.asTableCatalog().purgeTable(tableIdent); + }); catalog.asSchemas().dropSchema(testSchemaName, true); + + // sql injection + String schemaName = RandomNameUtils.genRandomName("ct_db"); + Map properties = new HashMap<>(); + String comment = null; + + // should throw an exception with string that might contain SQL injection + String sqlInjection = schemaName + "`; DROP TABLE important_table; -- "; + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + catalog.asSchemas().createSchema(sqlInjection, comment, properties); + }); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + catalog.asSchemas().dropSchema(sqlInjection, false); + }); + + String sqlInjection1 = schemaName + "`; SLEEP(10); -- "; + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + catalog.asSchemas().createSchema(sqlInjection1, comment, properties); + }); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + catalog.asSchemas().dropSchema(sqlInjection1, false); + }); + + String sqlInjection2 = + schemaName + "`; UPDATE Users SET password = 'newpassword' WHERE username = 'admin'; -- "; + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + catalog.asSchemas().createSchema(sqlInjection2, comment, properties); + }); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + catalog.asSchemas().dropSchema(sqlInjection2, false); + }); + + // should throw an exception with input that has more than 64 characters + String invalidInput = StringUtils.repeat("a", 65); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + catalog.asSchemas().createSchema(invalidInput, comment, properties); + }); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + catalog.asSchemas().dropSchema(invalidInput, false); + }); } @Test diff --git a/catalogs/catalog-jdbc-postgresql/src/main/java/com/datastrato/gravitino/catalog/postgresql/PostgreSqlCatalog.java b/catalogs/catalog-jdbc-postgresql/src/main/java/com/datastrato/gravitino/catalog/postgresql/PostgreSqlCatalog.java index c6293500af8..ecf9e7b510d 100644 --- a/catalogs/catalog-jdbc-postgresql/src/main/java/com/datastrato/gravitino/catalog/postgresql/PostgreSqlCatalog.java +++ b/catalogs/catalog-jdbc-postgresql/src/main/java/com/datastrato/gravitino/catalog/postgresql/PostgreSqlCatalog.java @@ -16,6 +16,7 @@ import com.datastrato.gravitino.catalog.postgresql.operation.PostgreSqlSchemaOperations; import com.datastrato.gravitino.catalog.postgresql.operation.PostgreSqlTableOperations; import com.datastrato.gravitino.connector.CatalogOperations; +import com.datastrato.gravitino.connector.capability.Capability; import java.util.Map; public class PostgreSqlCatalog extends JdbcCatalog { @@ -36,6 +37,11 @@ protected CatalogOperations newOps(Map config) { createJdbcColumnDefaultValueConverter()); } + @Override + public Capability newCapability() { + return new PostgreSqlCatalogCapability(); + } + @Override protected JdbcExceptionConverter createExceptionConverter() { return new PostgreSqlExceptionConverter(); diff --git a/catalogs/catalog-jdbc-postgresql/src/main/java/com/datastrato/gravitino/catalog/postgresql/PostgreSqlCatalogCapability.java b/catalogs/catalog-jdbc-postgresql/src/main/java/com/datastrato/gravitino/catalog/postgresql/PostgreSqlCatalogCapability.java new file mode 100644 index 00000000000..93817c447f1 --- /dev/null +++ b/catalogs/catalog-jdbc-postgresql/src/main/java/com/datastrato/gravitino/catalog/postgresql/PostgreSqlCatalogCapability.java @@ -0,0 +1,33 @@ +/* + * Copyright 2024 Datastrato Pvt Ltd. + * This software is licensed under the Apache License version 2. + */ +package com.datastrato.gravitino.catalog.postgresql; + +import com.datastrato.gravitino.connector.capability.Capability; +import com.datastrato.gravitino.connector.capability.CapabilityResult; + +public class PostgreSqlCatalogCapability implements Capability { + /** + * Regular expression explanation: ^[_a-zA-Z\p{L}/][\w\p{L}-$/=]{0,62}$ + * + *

^[_a-zA-Z\p{L}/] - Start with an underscore, a letter, or a letter from any language + * + *

[\w\p{L}-$/=]{0,62} - Consist of 0 to 62 characters (making the total length at most 63) of + * letters (both cases), digits, underscores, any kind of letter from any language, hyphens, + * dollar signs, slashes or equal signs + * + *

$ - End of the string + */ + public static final String POSTGRESQL_NAME_PATTERN = "^[_a-zA-Z\\p{L}/][\\w\\p{L}-$/=]{0,62}$"; + + @Override + public CapabilityResult specificationOnName(Scope scope, String name) { + // TODO: Validate the name against reserved words + if (!name.matches(POSTGRESQL_NAME_PATTERN)) { + return CapabilityResult.unsupported( + String.format("The %s name '%s' is illegal.", scope, name)); + } + return CapabilityResult.SUPPORTED; + } +} diff --git a/catalogs/catalog-jdbc-postgresql/src/test/java/com/datastrato/gravitino/catalog/postgresql/integration/test/CatalogPostgreSqlIT.java b/catalogs/catalog-jdbc-postgresql/src/test/java/com/datastrato/gravitino/catalog/postgresql/integration/test/CatalogPostgreSqlIT.java index 532567081ca..edecbc2a722 100644 --- a/catalogs/catalog-jdbc-postgresql/src/test/java/com/datastrato/gravitino/catalog/postgresql/integration/test/CatalogPostgreSqlIT.java +++ b/catalogs/catalog-jdbc-postgresql/src/test/java/com/datastrato/gravitino/catalog/postgresql/integration/test/CatalogPostgreSqlIT.java @@ -42,6 +42,7 @@ import com.datastrato.gravitino.rel.types.Decimal; import com.datastrato.gravitino.rel.types.Types; import com.datastrato.gravitino.rel.types.Types.IntegerType; +import com.datastrato.gravitino.utils.RandomNameUtils; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Maps; import com.google.common.collect.Sets; @@ -351,6 +352,76 @@ void testOperationPostgreSqlSchema() { Assertions.assertTrue(schemaNames.contains(schemaName)); } + @Test + void testSchemaWithIllegalName() { + SupportsSchemas schemas = catalog.asSchemas(); + String schemaName = RandomNameUtils.genRandomName("ct_db"); + + // should throw an exception with string that might contain SQL injection + String sqlInjection = schemaName + "; DROP TABLE important_table; -- "; + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + schemas.createSchema(sqlInjection, null, null); + }); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + schemas.dropSchema(sqlInjection, false); + }); + + String sqlInjection1 = schemaName + "; SELECT pg_sleep(10);"; + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + schemas.createSchema(sqlInjection1, null, null); + }); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + schemas.dropSchema(sqlInjection1, false); + }); + + String sqlInjection2 = + schemaName + "`; UPDATE Users SET password = 'newpassword' WHERE username = 'admin'; -- "; + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + schemas.createSchema(sqlInjection2, null, null); + }); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + schemas.dropSchema(sqlInjection2, false); + }); + + // should throw an exception with input that has more than 63 characters + String invalidInput = StringUtils.repeat("a", 64); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + schemas.createSchema(invalidInput, null, null); + }); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + schemas.dropSchema(invalidInput, false); + }); + + // should throw an exception with schema name that starts with special character + String invalidInput2 = RandomNameUtils.genRandomName("$test_db"); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + schemas.createSchema(invalidInput2, null, null); + }); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + schemas.dropSchema(invalidInput2, false); + }); + } + @Test void testCreateAndLoadPostgreSqlTable() { // Create table from Gravitino API @@ -992,6 +1063,144 @@ void testPGSpecialTableName() { t4_name, table_comment, Arrays.asList(t4_col), properties, t4_indexes, t4); } + @Test + void testPGIllegalTableName() { + Map properties = createProperties(); + TableCatalog tableCatalog = catalog.asTableCatalog(); + String table_name = "t123"; + + String t1_name = table_name + "`; DROP TABLE important_table; -- "; + Column t1_col = Column.of(t1_name, Types.LongType.get(), "id", false, false, null); + Column[] columns = {t1_col}; + Index[] t1_indexes = {Indexes.unique("u1_key", new String[][] {{t1_name}})}; + NameIdentifier tableIdentifier = + NameIdentifier.of(metalakeName, catalogName, schemaName, t1_name); + + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + tableCatalog.createTable( + tableIdentifier, + columns, + table_comment, + properties, + Transforms.EMPTY_TRANSFORM, + Distributions.NONE, + new SortOrder[0], + t1_indexes); + }); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + catalog.asTableCatalog().dropTable(tableIdentifier); + }); + + String t2_name = table_name + "`; SLEEP(10); -- "; + Column t2_col = Column.of(t2_name, Types.LongType.get(), "id", false, false, null); + Index[] t2_indexes = {Indexes.unique("u2_key", new String[][] {{t2_name}})}; + Column[] columns2 = new Column[] {t2_col}; + NameIdentifier tableIdentifier2 = + NameIdentifier.of(metalakeName, catalogName, schemaName, t2_name); + + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + tableCatalog.createTable( + tableIdentifier2, + columns2, + table_comment, + properties, + Transforms.EMPTY_TRANSFORM, + Distributions.NONE, + new SortOrder[0], + t2_indexes); + }); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + catalog.asTableCatalog().dropTable(tableIdentifier2); + }); + + String t3_name = + table_name + "`; UPDATE Users SET password = 'newpassword' WHERE username = 'admin'; -- "; + Column t3_col = Column.of(t3_name, Types.LongType.get(), "id", false, false, null); + Index[] t3_indexes = {Indexes.unique("u3_key", new String[][] {{t3_name}})}; + Column[] columns3 = new Column[] {t3_col}; + NameIdentifier tableIdentifier3 = + NameIdentifier.of(metalakeName, catalogName, schemaName, t3_name); + + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + tableCatalog.createTable( + tableIdentifier3, + columns3, + table_comment, + properties, + Transforms.EMPTY_TRANSFORM, + Distributions.NONE, + new SortOrder[0], + t3_indexes); + }); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + catalog.asTableCatalog().dropTable(tableIdentifier3); + }); + + String invalidInput = StringUtils.repeat("a", 64); + Column t4_col = Column.of(invalidInput, Types.LongType.get(), "id", false, false, null); + Index[] t4_indexes = {Indexes.unique("u4_key", new String[][] {{invalidInput}})}; + Column[] columns4 = new Column[] {t4_col}; + NameIdentifier tableIdentifier4 = + NameIdentifier.of(metalakeName, catalogName, schemaName, invalidInput); + + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + tableCatalog.createTable( + tableIdentifier4, + columns4, + table_comment, + properties, + Transforms.EMPTY_TRANSFORM, + Distributions.NONE, + new SortOrder[0], + t4_indexes); + }); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + catalog.asTableCatalog().dropTable(tableIdentifier4); + }); + + String invalidInput2 = RandomNameUtils.genRandomName("$test_db"); + Column t5_col = Column.of(invalidInput2, Types.LongType.get(), "id", false, false, null); + Index[] t5_indexes = {Indexes.unique("u5_key", new String[][] {{invalidInput2}})}; + Column[] columns5 = new Column[] {t5_col}; + NameIdentifier tableIdentifier5 = + NameIdentifier.of(metalakeName, catalogName, schemaName, invalidInput2); + + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + tableCatalog.createTable( + tableIdentifier5, + columns5, + table_comment, + properties, + Transforms.EMPTY_TRANSFORM, + Distributions.NONE, + new SortOrder[0], + t5_indexes); + }); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + catalog.asTableCatalog().dropTable(tableIdentifier5); + }); + } + @Test void testPGTableNameCaseSensitive() { Column col1 = Column.of("col_1", Types.LongType.get(), "id", false, false, null); diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/SchemaNormalizeDispatcher.java b/core/src/main/java/com/datastrato/gravitino/catalog/SchemaNormalizeDispatcher.java index 9c004989e80..62ad378d826 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/SchemaNormalizeDispatcher.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/SchemaNormalizeDispatcher.java @@ -65,10 +65,7 @@ public Schema alterSchema(NameIdentifier ident, SchemaChange... changes) @Override public boolean dropSchema(NameIdentifier ident, boolean cascade) throws NonEmptySchemaException { - // The constraints of the name spec may be more strict than underlying catalog, - // and for compatibility reasons, we only apply case-sensitive capabilities here. - return dispatcher.dropSchema( - applyCaseSensitive(ident, Capability.Scope.SCHEMA, dispatcher), cascade); + return dispatcher.dropSchema(normalizeNameIdentifier(ident), cascade); } private NameIdentifier normalizeNameIdentifier(NameIdentifier ident) { diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/TableNormalizeDispatcher.java b/core/src/main/java/com/datastrato/gravitino/catalog/TableNormalizeDispatcher.java index ac7da6cbc55..ddb62146a0d 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/TableNormalizeDispatcher.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/TableNormalizeDispatcher.java @@ -82,9 +82,7 @@ public Table alterTable(NameIdentifier ident, TableChange... changes) @Override public boolean dropTable(NameIdentifier ident) { - // The constraints of the name spec may be more strict than underlying catalog, - // and for compatibility reasons, we only apply case-sensitive capabilities here. - return dispatcher.dropTable(applyCaseSensitive(ident, Capability.Scope.TABLE, dispatcher)); + return dispatcher.dropTable(normalizeNameIdentifier(ident)); } @Override