FrankChen021 commented on code in PR #19813:
URL: https://github.com/apache/druid/pull/19813#discussion_r3686791868
##########
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 commit 89a35d0149. The traversal regression now calls
removeInfoFile without expecting an exception and verifies that the unsafe file
remains untouched.
##########
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 commit 89a35d0149. deleteSegmentInfoFile now catches Druid IAE
for unsafe or malformed segment IDs, logs a warning, and no-ops, so direct
removal and deferred onUnmount cleanup honor the removeInfoFile contract.
##########
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 commit 89a35d0149. resolveFileWithinDirectory now converts
InvalidPathException to Druid IAE with the consistent not-within-directory
message and preserves the cause; a malformed NUL-path regression test was added.
--
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]
