FrankChen021 commented on code in PR #19813:
URL: https://github.com/apache/druid/pull/19813#discussion_r3686788503
##########
processing/src/main/java/org/apache/druid/java/util/common/FileUtils.java:
##########
@@ -446,6 +446,24 @@ public static Path getTempDir()
return new File(parentDirectory).toPath();
}
+ /**
+ * Resolves {@code path} below {@code directory}, rejecting absolute paths
and parent traversal that would escape it.
+ * This is intended for paths containing externally supplied identifiers.
+ */
+ public static File resolveFileWithinDirectory(final File directory, final
String path)
+ {
+ final Path normalizedDirectory =
directory.toPath().toAbsolutePath().normalize();
+ final Path childPath = Path.of(path);
+ if (childPath.isAbsolute()) {
+ throw new IAE("Path[%s] is not within directory[%s]", path, directory);
+ }
+ final Path resolvedPath =
normalizedDirectory.resolve(childPath).normalize();
Review Comment:
Addressed in 89a35d0149. InvalidPathException is converted to IAE with the
original cause, with focused test coverage.
##########
server/src/main/java/org/apache/druid/segment/loading/SegmentLocalCacheManager.java:
##########
@@ -458,7 +463,7 @@ public void removeInfoFile(final DataSegment segment)
private void deleteSegmentInfoFile(DataSegment segment)
{
- final File segmentInfoCacheFile = new File(getEffectiveInfoDir(),
segment.getId().toString());
+ final File segmentInfoCacheFile = getSegmentInfoFile(segment);
if (!segmentInfoCacheFile.delete()) {
log.warn("Unable to delete cache file[%s] for segment[%s].",
segmentInfoCacheFile, segment.getId());
Review Comment:
Addressed in 89a35d0149. Unsafe segment info paths are now logged and
treated as a no-op by deleteSegmentInfoFile, including deferred unmount cleanup.
##########
server/src/test/java/org/apache/druid/segment/loading/SegmentLocalCacheManagerTest.java:
##########
@@ -247,6 +249,26 @@ public void testGetCachedSegmentsWithMissingSegmentFile()
throws IOException
Assert.assertFalse(segment3InfoFile.exists());
}
+ @Test
+ public void testSegmentInfoFileRejectsPathTraversal() throws IOException
+ {
+ final DataSegment segment = TestSegmentUtils.makeSegment(
+ "../../outside",
+ "v0",
+ Intervals.of("2014-10-20T00:00:00Z/P1D")
+ );
+ final File infoDir = new File(localSegmentCacheDir, "info_dir");
+ final File unsafeInfoFile = new File(infoDir, segment.getId().toString());
+ FileUtils.mkdirp(infoDir);
+
+ Assert.assertThrows(IAE.class, () -> manager.storeInfoFile(segment));
+ Assert.assertFalse(unsafeInfoFile.exists());
+
+ Files.write(unsafeInfoFile.toPath(), new byte[]{1});
+ Assert.assertThrows(IAE.class, () -> manager.removeInfoFile(segment));
Review Comment:
Addressed in 89a35d0149. The traversal test now confirms removeInfoFile
returns normally and preserves the unsafe file.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
