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

Changes from 1 commit
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
11 changes: 11 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 @@ -3762,6 +3762,12 @@ public Boolean invoke(@NonNull File parentFile, @NonNull VirtualChannel channel)
}

Path parentAbsolutePath = Util.fileToPath(parentFile.getAbsoluteFile());
// --- FIX START: Handle potential null from Util.fileToPath ---
if (parentAbsolutePath == null) {
LOGGER.log(Level.FINE, "Invalid path characters in parentFile: {0}", parentFile);
return false;
}
// --- FIX END ---
Copy link

Copilot AI Jan 12, 2026 •
edited by MarkEWaite
Loading

Choose a reason for hiding this comment

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

This null check is unnecessary. The Util.fileToPath method is annotated with @NonNull and throws IOException on invalid paths rather than returning null. If an InvalidPathException occurs, Util.fileToPath will throw IOException which will propagate from this invoke method (which declares throws IOException). Remove this null check as it creates unreachable code.

Suggested change
// --- FIX START: Handle potential null from Util.fileToPath ---
if (parentAbsolutePath == null) {
LOGGER.log(Level.FINE, "Invalid path characters in parentFile: {0}", parentFile);
return false;
}
// --- FIX END ---

Copilot uses AI. Check for mistakes.
Path parentRealPath;
try {
if (Functions.isWindows()) {
Expand All @@ -3781,6 +3787,11 @@ public Boolean invoke(@NonNull File parentFile, @NonNull VirtualChannel channel)
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) {
return false;
}
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
Loading