gharris1727 commented on code in PR #14995:
URL: https://github.com/apache/kafka/pull/14995#discussion_r1439665912
##########
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:
This is still an invalid path traversal attack. The duplicate test in
DirectoryConfigProviderTest appears to be fine.
Have you looked into deduplicating these two classes? That would also avoid
requiring two different tests for the same traversal attack.
##########
clients/src/test/java/org/apache/kafka/common/config/provider/DirectoryConfigProviderTest.java:
##########
@@ -153,5 +159,57 @@ public void testServiceLoaderDiscovery() {
ServiceLoader<ConfigProvider> serviceLoader =
ServiceLoader.load(ConfigProvider.class);
assertTrue(StreamSupport.stream(serviceLoader.spliterator(),
false).anyMatch(configProvider -> configProvider instanceof
DirectoryConfigProvider));
}
+
+ @Test
+ public void testAllowedPath() {
+ Map<String, String> configs = new HashMap<>();
+ configs.put(ALLOWED_PATHS_CONFIG, parent.getAbsolutePath());
+ provider.configure(configs);
+
+ ConfigData configData = provider.get(dir);
+ assertEquals(toSet(asList(foo, bar)), configData.data().keySet());
+ assertEquals("FOO", configData.data().get(foo));
+ assertEquals("BAR", configData.data().get(bar));
+ assertNull(configData.ttl());
+ }
+
+ @Test
+ public void testMultipleAllowedPaths() {
+ Map<String, String> configs = new HashMap<>();
+ configs.put(ALLOWED_PATHS_CONFIG, dir + "," + siblingDir);
+ provider.configure(configs);
+
+ ConfigData configData = provider.get(subdir);
+ assertEquals(toSet(asList(subdirFileName)),
configData.data().keySet());
+ assertEquals("SUBDIRFILE", configData.data().get(subdirFileName));
+ assertNull(configData.ttl());
+
+ configData = provider.get(siblingDir);
+ assertEquals(toSet(asList(siblingDirFileName)),
configData.data().keySet());
+ assertEquals("SIBLINGDIRFILE",
configData.data().get(siblingDirFileName));
+ assertNull(configData.ttl());
+ }
+
+ @Test
+ public void testNotAllowedPath() {
+ Map<String, String> configs = new HashMap<>();
+ configs.put(ALLOWED_PATHS_CONFIG, dir);
+ provider.configure(configs);
+
+ ConfigData configData = provider.get(siblingDir);
+ assertTrue(configData.data().isEmpty());
+ assertNull(configData.ttl());
+ }
+
+ @Test
+ public void testNoTraversalAllowedPath() {
+ Map<String, String> configs = new HashMap<>();
+ configs.put(ALLOWED_PATHS_CONFIG, dir);
+ provider.configure(configs);
+
+ ConfigData configData = provider.get(dir +
Paths.get("/../siblingdir"));
Review Comment:
This is mis-using the Paths.get function, which is intended to do all of the
directory delimiting on your behalf.
If the leading `/` was missing in this string, the test would be invalid.
```suggestion
ConfigData configData = provider.get(Paths.get(dir, "..",
"siblingdir").toString());
```
In this form, giving `".."`, `"/.."`, `"../"`, or `"/../"` are all
equivalent, so it is more resistant to typos.
--
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]
