gharris1727 commented on code in PR #14995:
URL: https://github.com/apache/kafka/pull/14995#discussion_r1441959148
##########
clients/src/test/java/org/apache/kafka/common/config/provider/FileConfigProviderTest.java:
##########
@@ -98,8 +117,91 @@ public void testServiceLoaderDiscovery() {
public static class TestFileConfigProvider extends FileConfigProvider {
@Override
- protected Reader reader(String path) throws IOException {
+ protected Reader reader(Path path) throws IOException {
return new
StringReader("testKey=testResult\ntestKey2=testResult2");
}
}
+
+ @Test
+ public void testAllowedDirPath() {
+ Map<String, String> configs = new HashMap<>();
+ configs.put(ALLOWED_PATHS_CONFIG, dir);
+ configProvider.configure(configs);
+
+ ConfigData configData = configProvider.get(dirFile);
+ Map<String, String> result = new HashMap<>();
+ result.put("testKey", "testResult");
+ result.put("testKey2", "testResult2");
+ assertEquals(result, configData.data());
+ assertNull(configData.ttl());
+ }
+
+ @Test
+ public void testAllowedFilePath() {
+ Map<String, String> configs = new HashMap<>();
+ configs.put(ALLOWED_PATHS_CONFIG, dirFile);
+ configProvider.configure(configs);
+
+ ConfigData configData = configProvider.get(dirFile);
+ Map<String, String> result = new HashMap<>();
+ result.put("testKey", "testResult");
+ result.put("testKey2", "testResult2");
+ assertEquals(result, configData.data());
+ assertNull(configData.ttl());
+ }
+
+ @Test
+ public void testMultipleAllowedPaths() {
+ Map<String, String> configs = new HashMap<>();
+ configs.put(ALLOWED_PATHS_CONFIG, dir + "," + siblingDir);
+ configProvider.configure(configs);
+
+ Map<String, String> result = new HashMap<>();
+ result.put("testKey", "testResult");
+ result.put("testKey2", "testResult2");
+
+ ConfigData configData = configProvider.get(dirFile);
+ assertEquals(result, configData.data());
+ assertNull(configData.ttl());
+
+ configData = configProvider.get(siblingDirFile);
+ assertEquals(result, configData.data());
+ assertNull(configData.ttl());
+ }
+
+ @Test
+ public void testNotAllowedDirPath() {
+ Map<String, String> configs = new HashMap<>();
+ configs.put(ALLOWED_PATHS_CONFIG, dir);
+ configProvider.configure(configs);
+
+ ConfigData configData = configProvider.get(siblingDirFile);
+ assertTrue(configData.data().isEmpty());
+ assertNull(configData.ttl());
+ }
+
+ @Test
+ public void testNotAllowedFilePath() throws IOException {
+ Map<String, String> configs = new HashMap<>();
+ configs.put(ALLOWED_PATHS_CONFIG, dirFile);
+ configProvider.configure(configs);
+
+ //another file under the same directory
+ Path dirFile2 = Files.createFile(Paths.get(dir, "dirFile2"));
+ ConfigData configData = configProvider.get(dirFile2.toString());
+ assertTrue(configData.data().isEmpty());
+ assertNull(configData.ttl());
+ }
+
+ @Test
+ public void testNoTraversal() {
+ Map<String, String> configs = new HashMap<>();
+ configs.put(ALLOWED_PATHS_CONFIG, dirFile);
+ configProvider.configure(configs);
+
+ // Check we can't escape outside the path directory
+ ConfigData configData = configProvider.get(dirFile +
Paths.get("/../siblingdir/siblingdirFile"));
Review Comment:
> I did however deduplicate the implementation classes by adding
ConfigProviderUtils class with the common methods.
Rather than making these static utilities and have the caller store the
List<Path>, you can keep the List<Path> as an instance variable in the shared
class. This ensures that the List<Path> is handled completely by the validation
logic and not changed by the config provider implementations.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
