Copilot commented on code in PR #19813:
URL: https://github.com/apache/druid/pull/19813#discussion_r3686695433
##########
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:
`removeInfoFile` (and deferred `setOnUnmount` hooks) must not throw on an
unsafe segment id. Right now `deleteSegmentInfoFile` calls
`getSegmentInfoFile`, which can throw `IAE`; this can propagate out of
`removeInfoFile` (whose interface contract says “If the file cannot be deleted,
do nothing”) and can also blow up unmount cleanup because the onUnmount
runnable is invoked without a try/catch. Suggest treating unsafe paths as a
no-op deletion with a warning rather than throwing.
##########
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:
`resolveFileWithinDirectory` uses `Path.of(path)` directly; for malformed
externally-supplied identifiers this can throw `InvalidPathException`, which
will escape as an unexpected runtime exception type. Consider catching
`InvalidPathException` and rethrowing as `IAE` (with the existing message) to
keep the method’s failure mode consistent for callers.
##########
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:
This test currently asserts that `removeInfoFile` throws `IAE` for a
traversal segment id. However `SegmentCacheManager#removeInfoFile` is
documented to “do nothing” if the file cannot be deleted (and callers like
`SegmentManager` do not guard against runtime exceptions). The test should
validate that the unsafe file is preserved without requiring an exception.
--
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]
