You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "FrankChen021 (via GitHub)" <gi...@apache.org> on 2026/07/30 14:53:01 UTC
[PR] Guard segment info paths against traversal (druid)
FrankChen021 opened a new pull request, #19813:
URL: https://github.com/apache/druid/pull/19813
## Summary
- add a shared helper that resolves a relative path only when its normalized result remains beneath the intended directory
- reject absolute paths, including absolute paths that happen to point beneath the configured directory
- use the helper consistently for complete and partial segment info-file writes and deletion
- cover legitimate nested paths, `..` traversal, absolute paths, and preservation of an out-of-directory sentinel
## Root cause
`SegmentLocalCacheManager` constructed info-file paths directly from `DataSegment#getId()`. A segment identifier includes externally supplied data-source text, so a crafted identifier containing parent components could make the delete sink reported by CodeQL alert #7450 resolve outside the owned `info_dir`. The same construction was used by the corresponding info-file write paths.
## Fix and impact
Segment info paths are now normalized and required to remain under the configured info directory before any file operation. Absolute paths are rejected explicitly. Normal relative paths, including legitimate nested relative paths, continue to resolve normally. This affects only invalid segment identifiers that would escape the configured info directory.
## Validation
- `mvn -ntp -Pskip-static-checks -pl processing -DskipITs -Dtest=FileUtilsTest test` — 20 tests passed
- `mvn -ntp -Pskip-static-checks -pl server -am -DskipITs -Dtest=SegmentLocalCacheManagerTest -Dsurefire.failIfNoSpecifiedTests=false test` — 31 tests passed
- `mvn -ntp -pl processing,server -DskipTests validate` — Checkstyle, PMD, and Maven enforcer passed for both modules
- `git diff --check`
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix: guard segment info paths against traversal (druid)
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix: guard segment info paths against traversal (druid)
Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
FrankChen021 commented on PR #19813:
URL: https://github.com/apache/druid/pull/19813#issuecomment-5133656715
The failing non-transactional `KafkaIndexFaultToleranceTest.test_supervisorRecovers_afterSuspendResume` appears to be an unrelated Kafka test flake during the initial ingestion wait. [PR #19824](https://github.com/apache/druid/pull/19824) addresses the likely initial topic-leader readiness race. [PR #19817](https://github.com/apache/druid/pull/19817) additionally waits for non-transactional producer sends, so delivery failures are no longer silently deferred.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix: guard segment info paths against traversal (druid)
Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix: guard segment info paths against traversal (druid)
Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix: guard segment info paths against traversal (druid)
Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
FrankChen021 merged PR #19813:
URL: https://github.com/apache/druid/pull/19813
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org