diff --git a/sink-connector-lightweight/src/main/java/com/altinity/clickhouse/debezium/embedded/common/PropertiesHelper.java b/sink-connector-lightweight/src/main/java/com/altinity/clickhouse/debezium/embedded/common/PropertiesHelper.java index 364df4849..afd57ef72 100644 --- a/sink-connector-lightweight/src/main/java/com/altinity/clickhouse/debezium/embedded/common/PropertiesHelper.java +++ b/sink-connector-lightweight/src/main/java/com/altinity/clickhouse/debezium/embedded/common/PropertiesHelper.java @@ -46,7 +46,7 @@ public static Properties getProperties(String fileName) throws Exception { // Load the properties file from the classpath props.load(input); } catch (IOException ex) { - ex.printStackTrace(); + throw new Exception("Error loading properties file: " + fileName, ex); } return props; diff --git a/sink-connector-lightweight/src/main/java/com/altinity/clickhouse/debezium/embedded/config/AppConfiguration.java b/sink-connector-lightweight/src/main/java/com/altinity/clickhouse/debezium/embedded/config/AppConfiguration.java index 49fd5a47f..97b6dc953 100644 --- a/sink-connector-lightweight/src/main/java/com/altinity/clickhouse/debezium/embedded/config/AppConfiguration.java +++ b/sink-connector-lightweight/src/main/java/com/altinity/clickhouse/debezium/embedded/config/AppConfiguration.java @@ -14,77 +14,66 @@ public class AppConfiguration { * The host or IP address of the source database server. */ @Getter - @Setter private final String sourceHost; /** * The port number of the source database server. */ @Getter - @Setter private final String sourcePort; /** * The username used to connect to the source database. */ @Getter - @Setter private final String sourceUserName; /** * The password used to connect to the source database. */ @Getter - @Setter private final String sourcePassword; /** * The name of the source database. */ @Getter - @Setter private final String sourceDatabase; /** * A comma-separated list of source tables to monitor or process. */ @Getter - @Setter private final String sourceTables; /** * The host or IP address of the ClickHouse server. */ @Getter - @Setter private final String clickHouseHost; /** * The port number of the ClickHouse server. */ @Getter - @Setter private final String clickHousePort; /** * The password used to connect to the ClickHouse server. */ @Getter - @Setter private final String clickHousePassword; /** * The name of the ClickHouse database. */ @Getter - @Setter private final String clickHouseDatabase; /** * The username used to connect to the ClickHouse server. */ @Getter - @Setter private final String clickHouseUserName; /** diff --git a/sink-connector-lightweight/src/main/java/com/altinity/clickhouse/debezium/embedded/config/ConfigLoader.java b/sink-connector-lightweight/src/main/java/com/altinity/clickhouse/debezium/embedded/config/ConfigLoader.java index 2019f460a..284bfd366 100644 --- a/sink-connector-lightweight/src/main/java/com/altinity/clickhouse/debezium/embedded/config/ConfigLoader.java +++ b/sink-connector-lightweight/src/main/java/com/altinity/clickhouse/debezium/embedded/config/ConfigLoader.java @@ -5,6 +5,7 @@ import org.yaml.snakeyaml.constructor.SafeConstructor; import java.io.FileInputStream; +import java.io.FileNotFoundException; import java.io.IOException; import java.io.InputStream; import java.util.Map; @@ -17,28 +18,16 @@ */ public class ConfigLoader { - private static Yaml createSafeYaml() { + /** + * Creates a Yaml instance with SafeConstructor to prevent + * arbitrary object deserialization (CVE-2022-1471). + * + * @return A safely-configured Yaml instance. + */ + private Yaml createSafeYaml() { return new Yaml(new SafeConstructor(new LoaderOptions())); } - private static Properties toProperties(Map yamlFile) { - final Properties props = new Properties(); - if (yamlFile == null) { - return props; - } - for (Map.Entry entry : yamlFile.entrySet()) { - Object value = entry.getValue(); - if (value != null) { - String strValue = String.valueOf(value); - if (value instanceof String) { - strValue = strValue.replace("\"", ""); - } - props.setProperty(entry.getKey(), strValue); - } - } - return props; - } - /** * Loads properties from a YAML file located on the classpath. * @@ -47,12 +36,19 @@ private static Properties toProperties(Map yamlFile) { * key-value pairs. */ public Properties load(String resourceFileName) { - InputStream fis = this.getClass() + // Use try-with-resources to ensure the InputStream is closed. 2.10.0's + // variant declared the stream in the resource list but then parsed + // INSIDE the resource specification, which does not compile. + try (InputStream fis = this.getClass() .getClassLoader() - .getResourceAsStream(resourceFileName); + .getResourceAsStream(resourceFileName)) { - Map yamlFile = createSafeYaml().load(fis); - return toProperties(yamlFile); + Map yamlFile = createSafeYaml().load(fis); + + return convertYamlMapToProperties(yamlFile); + } catch (IOException e) { + throw new RuntimeException("Failed to load resource: " + resourceFileName, e); + } } /** @@ -61,14 +57,53 @@ public Properties load(String resourceFileName) { * @param fileName The full path of the YAML file. * @return A {@link Properties} object containing the configuration * key-value pairs. - * @throws IOException If the specified file cannot be read. + * @throws FileNotFoundException If the specified file does not exist. */ public Properties loadFromFile(String fileName) - throws IOException { - + throws FileNotFoundException { + // Use try-with-resources to ensure InputStream is closed try (InputStream fis = new FileInputStream(fileName)) { + Map yamlFile = createSafeYaml().load(fis); - return toProperties(yamlFile); + + return convertYamlMapToProperties(yamlFile); + } catch (FileNotFoundException e) { + throw e; + } catch (IOException e) { + throw new RuntimeException("Failed to load file: " + fileName, e); + } + } + + /** + * Converts a YAML map to a Properties object, handling all value types + * safely (not just String and Integer). + * + * @param yamlFile The map parsed from the YAML file. May be {@code null} + * when the document is empty. + * @return A {@link Properties} object with string representations of all values. + */ + private Properties convertYamlMapToProperties(Map yamlFile) { + final Properties props = new Properties(); + + // An empty YAML document parses to null. This guard is carried over + // from the 2.10.0 side of the merge; without it an empty config file + // throws a NullPointerException instead of yielding empty Properties. + if (yamlFile == null) { + return props; } + + for (Map.Entry entry : yamlFile.entrySet()) { + Object value = entry.getValue(); + if (value == null) { + continue; + } + // Use toString() instead of casting to (String) to handle + // all YAML value types: Integer, Long, Boolean, Double, etc. + String stringValue = value.toString(); + // Strip surrounding quotes if present (legacy behavior) + stringValue = stringValue.replace("\"", ""); + props.setProperty(entry.getKey(), stringValue); + } + return props; } } diff --git a/sink-connector-lightweight/src/main/java/com/altinity/clickhouse/debezium/embedded/config/EnvironmentVariables.java b/sink-connector-lightweight/src/main/java/com/altinity/clickhouse/debezium/embedded/config/EnvironmentVariables.java index 84d00c054..d2a787578 100644 --- a/sink-connector-lightweight/src/main/java/com/altinity/clickhouse/debezium/embedded/config/EnvironmentVariables.java +++ b/sink-connector-lightweight/src/main/java/com/altinity/clickhouse/debezium/embedded/config/EnvironmentVariables.java @@ -52,4 +52,13 @@ public enum EnvironmentVariables { EnvironmentVariables(String s) { this.label = s; } + + /** + * Returns the label (property key) for this environment variable. + * + * @return the label string + */ + public String getLabel() { + return label; + } } diff --git a/sink-connector-lightweight/src/test/java/com/altinity/clickhouse/debezium/embedded/config/ConfigLoaderTest.java b/sink-connector-lightweight/src/test/java/com/altinity/clickhouse/debezium/embedded/config/ConfigLoaderTest.java index 5566ab34b..ecc9ced93 100644 --- a/sink-connector-lightweight/src/test/java/com/altinity/clickhouse/debezium/embedded/config/ConfigLoaderTest.java +++ b/sink-connector-lightweight/src/test/java/com/altinity/clickhouse/debezium/embedded/config/ConfigLoaderTest.java @@ -3,21 +3,47 @@ import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; +import java.io.File; +import java.io.FileNotFoundException; +import java.io.FileWriter; +import java.io.IOException; +import java.nio.file.Path; import java.util.Properties; +/** + * Tests for ConfigLoader — Phase 7 safety and robustness fixes. + *

+ * Validates: + * - SafeConstructor prevents arbitrary deserialization (CVE-2022-1471) + * - All YAML value types (String, Integer, Boolean, Double) are handled + * - Null values are skipped gracefully + * - Quoted values have surrounding quotes stripped + * - InputStream is closed properly (try-with-resources) + * - loadFromFile() throws FileNotFoundException correctly + *

+ */ public class ConfigLoaderTest { @Test - @DisplayName("Unit test to validate loading of config.yml into the application") + @DisplayName("load() should load config.yml from classpath successfully") public void testLoad() { ConfigLoader loader = new ConfigLoader(); Properties props = loader.load("config.yml"); Assertions.assertNotNull(props); Assertions.assertFalse(props.isEmpty()); + // config.yml has known keys + Assertions.assertNotNull(props.getProperty("database.hostname"), + "database.hostname should be present"); } + // Both sides of the merge added coverage here and BOTH are kept: 2.10.0's + // typed-value / malicious-tag tests (config-typed.yml, config-malicious.yml) + // and develop's per-type tests (config_mixed_types.yml). All three test + // resources exist in src/test/resources, so neither set is dropped. + @Test @DisplayName("Load Boolean, Long, and Integer YAML values without ClassCastException") public void testLoadTypedValues() { @@ -36,4 +62,158 @@ public void testRejectsMaliciousYamlTags() { ConfigLoader loader = new ConfigLoader(); Assertions.assertThrows(Exception.class, () -> loader.load("config-malicious.yml")); } + + @Test + @DisplayName("load() should handle Boolean YAML values without ClassCastException") + public void testLoadBooleanValues() { + ConfigLoader loader = new ConfigLoader(); + Properties props = loader.load("config_mixed_types.yml"); + + Assertions.assertNotNull(props); + // Boolean true → "true" string + Assertions.assertEquals("true", props.getProperty("boolean_key"), + "Boolean true should be converted to string 'true'"); + // Boolean false → "false" string + Assertions.assertEquals("false", props.getProperty("enabled"), + "Boolean false should be converted to string 'false'"); + } + + @Test + @DisplayName("load() should handle Integer YAML values without ClassCastException") + public void testLoadIntegerValues() { + ConfigLoader loader = new ConfigLoader(); + Properties props = loader.load("config_mixed_types.yml"); + + Assertions.assertNotNull(props); + // Integer 42 → "42" string + Assertions.assertEquals("42", props.getProperty("integer_key"), + "Integer 42 should be converted to string '42'"); + // Integer 8123 → "8123" string + Assertions.assertEquals("8123", props.getProperty("port"), + "Integer 8123 should be converted to string '8123'"); + } + + @Test + @DisplayName("load() should handle Double/Float YAML values without ClassCastException") + public void testLoadDoubleValues() { + ConfigLoader loader = new ConfigLoader(); + Properties props = loader.load("config_mixed_types.yml"); + + Assertions.assertNotNull(props); + // Double 3.14 → "3.14" string + Assertions.assertEquals("3.14", props.getProperty("double_key"), + "Double 3.14 should be converted to string '3.14'"); + } + + @Test + @DisplayName("load() should skip null YAML values gracefully") + public void testLoadNullValues() { + ConfigLoader loader = new ConfigLoader(); + Properties props = loader.load("config_mixed_types.yml"); + + Assertions.assertNotNull(props); + // null_key should not be present (skipped) + Assertions.assertNull(props.getProperty("null_key"), + "Null YAML values should be skipped, not stored"); + } + + @Test + @DisplayName("load() should strip surrounding double quotes from values") + public void testLoadStripsSurroundingQuotes() { + ConfigLoader loader = new ConfigLoader(); + Properties props = loader.load("config_mixed_types.yml"); + + Assertions.assertNotNull(props); + // String value "hello" should remain "hello" + Assertions.assertEquals("hello", props.getProperty("string_key"), + "String values should not have extra quotes"); + } + + @Test + @DisplayName("load() with SafeConstructor should not deserialize arbitrary objects") + public void testSafeConstructorPreventsArbitraryDeserialization() { + // SafeConstructor only allows basic YAML types (String, Integer, Boolean, + // Double, List, Map). It rejects !!java.lang.Runtime and similar tags. + // This test verifies the ConfigLoader uses SafeConstructor by loading + // a normal YAML file — the key property is that the Yaml instance is + // constructed with SafeConstructor, not the default Constructor. + // A direct test of malicious YAML would require a test resource with + // !!java.lang.Runtime tags, which is unsafe to ship. Instead, we verify + // that the loader works correctly with SafeConstructor (no regression). + ConfigLoader loader = new ConfigLoader(); + Properties props = loader.load("config.yml"); + Assertions.assertNotNull(props, "SafeConstructor should handle normal YAML"); + Assertions.assertTrue(props.size() > 0, + "Properties should not be empty after SafeConstructor load"); + } + + @Test + @DisplayName("loadFromFile() should load YAML from absolute file path") + public void testLoadFromFile(@TempDir Path tempDir) throws Exception { + // Create a temp YAML file + File tempYaml = tempDir.resolve("test_config.yml").toFile(); + try (FileWriter writer = new FileWriter(tempYaml)) { + writer.write("host: localhost\n"); + writer.write("port: 3306\n"); + writer.write("enabled: true\n"); + } + + ConfigLoader loader = new ConfigLoader(); + Properties props = loader.loadFromFile(tempYaml.getAbsolutePath()); + + Assertions.assertNotNull(props); + Assertions.assertEquals("localhost", props.getProperty("host")); + Assertions.assertEquals("3306", props.getProperty("port")); + Assertions.assertEquals("true", props.getProperty("enabled")); + } + + @Test + @DisplayName("loadFromFile() should throw FileNotFoundException for missing file") + public void testLoadFromFileMissing() { + ConfigLoader loader = new ConfigLoader(); + Assertions.assertThrows(FileNotFoundException.class, + () -> loader.loadFromFile("/nonexistent/path/config.yml"), + "loadFromFile() should throw FileNotFoundException for missing file"); + } + + @Test + @DisplayName("loadFromFile() should handle mixed types from file") + public void testLoadFromFileMixedTypes(@TempDir Path tempDir) throws Exception { + File tempYaml = tempDir.resolve("mixed.yml").toFile(); + try (FileWriter writer = new FileWriter(tempYaml)) { + writer.write("string_val: hello\n"); + writer.write("int_val: 100\n"); + writer.write("bool_val: false\n"); + writer.write("float_val: 1.5\n"); + writer.write("null_val: null\n"); + } + + ConfigLoader loader = new ConfigLoader(); + Properties props = loader.loadFromFile(tempYaml.getAbsolutePath()); + + Assertions.assertNotNull(props); + Assertions.assertEquals("hello", props.getProperty("string_val")); + Assertions.assertEquals("100", props.getProperty("int_val")); + Assertions.assertEquals("false", props.getProperty("bool_val")); + Assertions.assertEquals("1.5", props.getProperty("float_val")); + Assertions.assertNull(props.getProperty("null_val"), + "Null values should be skipped"); + } + + @Test + @DisplayName("load() should handle the existing config.yml with all its properties") + public void testLoadExistingConfigPreservesAllProperties() { + ConfigLoader loader = new ConfigLoader(); + Properties props = loader.load("config.yml"); + + // Verify key properties from the standard config.yml + Assertions.assertNotNull(props.getProperty("database.port"), + "database.port should be loaded"); + Assertions.assertNotNull(props.getProperty("clickhouse.server.url"), + "clickhouse.server.url should be loaded"); + Assertions.assertNotNull(props.getProperty("snapshot.mode"), + "snapshot.mode should be loaded"); + Assertions.assertNotNull(props.getProperty("auto.create.tables"), + "auto.create.tables should be loaded (boolean in YAML)"); + } } diff --git a/sink-connector-lightweight/src/test/resources/config_mixed_types.yml b/sink-connector-lightweight/src/test/resources/config_mixed_types.yml new file mode 100644 index 000000000..6d6dede44 --- /dev/null +++ b/sink-connector-lightweight/src/test/resources/config_mixed_types.yml @@ -0,0 +1,8 @@ +string_key: "hello" +integer_key: 42 +boolean_key: true +double_key: 3.14 +null_key: null +quoted_key: '"quoted_value"' +port: 8123 +enabled: false