gharris1727 commented on code in PR #14995:
URL: https://github.com/apache/kafka/pull/14995#discussion_r1436602326
##########
clients/src/test/java/org/apache/kafka/common/config/provider/DirectoryConfigProviderTest.java:
##########
@@ -83,27 +89,27 @@ public void close() throws IOException {
@Test
public void testGetAllKeysAtPath() {
- ConfigData configData = provider.get(dir.getAbsolutePath());
Review Comment:
What is the importance of these changes to the existing tests? Do existing
tests fail without changing the inputs? Is this a backwards incompatibility?
##########
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(dir +
"../siblingdir/siblingdirFile");
Review Comment:
I believe this test is invalid.
The directory structure looks like this:
```
<tmpdir>
dir
dirFile
siblingdir
siblingdirFile
```
This test sets up the allowed paths to be `<tmpdir>/dir/dirFile` and
attempts to access `<tmpdir>/dir../siblingDir/siblingDirFile`
This doesn't read any data, but only due to typos in the test:
1. `dir..` isn't a valid directory. This can be fixed by using Paths.get()
to perform the path extension or by inserting another `/`
2. `<tmpdir>/dir/../siblingDirFile` is a path traversal attack on
`<tmpdir>/dir/` and not on `<tmpdir>/dir/dirFile`.
If I fix both of these typos, the test fails because the path traversal
attack succeeds.
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
