Skip to content

Fix SpotBugs false positive in FilePath.IsDescendant #26108

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Vinamra-tech wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Fix SpotBugs false positive in FilePath.IsDescendant #26108

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2b39931
Fix NullPointerExceptions in FilePath.IsDescendant
Vinamra-tech Jan 12, 2026
9dd83a2
Remove redundant check for Util.fileToPath, keep necessary check for …
Vinamra-tech Jan 12, 2026
a1044e0
Add test case verifying isDescendant handles parent traversal ('..') …
Vinamra-tech Jan 12, 2026
c3332aa
Fix SpotBugs false positive by updating getDirectChild annotation and…
Vinamra-tech Jan 13, 2026
6fe47b6
Fix SpotBugs failure: Throw exception on unreachable null path to sat…
Vinamra-tech Jan 13, 2026
c23d6a0
Merge branch 'master' into fix/filepath-npe
Vinamra-tech Jan 14, 2026
1923e13
Merge branch 'master' into fix/filepath-npe
Vinamra-tech Jan 21, 2026
3361308
Merge branch 'master' into fix/filepath-npe
Vinamra-tech Jan 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions core/src/main/java/hudson/FilePath.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3781,6 +3781,11 @@
Path currentFilePath = parentFile.toPath();
while (!remainingPath.isEmpty()) {
Path directChild = this.getDirectChild(currentFilePath, remainingPath);
// --- FIX #2: Handle if directChild is null ---
Copy link
Contributor

@MarkEWaite MarkEWaite Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The extra comment text is incorrect now and doesn't help comprehension

Suggested change
// --- FIX #2: Handle if directChild is null ---

if (directChild == null) {

Check warning on line 3785 in core/src/main/java/hudson/FilePath.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line 

Line 3785 is only partially covered, one branch is missing
return false;

Check warning on line 3786 in core/src/main/java/hudson/FilePath.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered line 

Line 3786 is not covered by tests
}
Copy link

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add test coverage for the case where getDirectChild returns null. While the existing isDescendant tests are comprehensive, none appear to exercise the specific edge case where getDirectChild would return null (when the child path cannot be resolved to a direct child of the parent path).

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@MarkEWaite MarkEWaite Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

none appear to exercise the specific edge case where getDirectChild would return null (when the child path cannot be resolved to a direct child of the parent path).

I don't think that edge case can be exercised because there is specific code in the method that rejects absolute paths

// ---------------------------------------------
Copy link
Contributor

@MarkEWaite MarkEWaite Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't help comprehension of the change

Suggested change
// ---------------------------------------------

Path childUsingFullPath = currentFilePath.resolve(remainingPath);
String childUsingFullPathAbs = childUsingFullPath.toAbsolutePath().toString();
String directChildAbs = directChild.toAbsolutePath().toString();
Expand Down
13 changes: 13 additions & 0 deletions core/src/test/java/hudson/FilePathTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1297,4 +1297,17 @@ private static File newFolder(File root, String... subDirs) throws IOException {
}
return result;
}

@Test
@Issue("SpotBugs")
Copy link
Contributor

@MarkEWaite MarkEWaite Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use the @Issue annotation for bug reports. No need to annotate a test for a null pointer exception

Suggested change
@Issue("SpotBugs")

public void testIsDescendantWithParentTraversal() throws Exception {
File tmp = Util.createTempDir();
Copy link
Contributor

@MarkEWaite MarkEWaite Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use the JUnit @TempDir annotation to allow the JUnit framework to perform the directory cleanup rather than using try / finally to perform the cleanup yourself. There are several examples available in the Jenkins tests.

try {
FilePath parent = new FilePath(tmp);
Copy link
Contributor

@MarkEWaite MarkEWaite Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The coverage report says that the return false line is not covered. It was also not reached when I ran the test in the debugger. Please adjust the test to show the null pointer exception before your change.

Also, if you use the variable temp it is already available in the test class as a TempDir that will be automatically deleted at the end of the test.

Copy link
Contributor

@MarkEWaite MarkEWaite Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made several attempts to reach the added return null from a test. I was unsuccessful. I'm curious if you can find a way to reach it that I missed.

// This checks the specific case that causes getDirectChild to return null
assertFalse(parent.isDescendant(".."), "Parent directory '..' should not be a descendant");
} finally {
Util.deleteRecursive(tmp);
}
}
Comment on lines +1301 to +1306
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is redundant with existing comprehensive test coverage for isDescendant. Tests in isDescendant_regularFiles() (lines 1042, 1049-1050, 1053-1057, 1060-1061) already validate the behavior with ".." path traversals, including cases like "sub/../sub/sub-regular.txt", "../protected/secret.txt", "./../workspace", and "./../../root/workspace/regular.txt".

The new test doesn't add meaningful coverage beyond what's already tested. Consider removing this test to avoid redundancy, or if there's a specific edge case this test targets that isn't covered by existing tests, update the test comment to explain what unique scenario is being validated.

Copilot uses AI. Check for mistakes.
}
Loading