Skip to content
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

Simplify Util#makeResource #777

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Simplify Util#makeResource #777

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 2 additions & 5 deletions src/main/java/hudson/remoting/Util.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,13 +55,10 @@ public static void copy(InputStream in, OutputStream out) throws IOException {
}

@NonNull
@SuppressFBWarnings(
value = "PATH_TRAVERSAL_IN",
justification = "This path exists within a temp directory so the potential traversal is limited.")
@SuppressFBWarnings(value = "PATH_TRAVERSAL_IN", justification = "Sanitized by #getBaseName")
static File makeResource(String name, byte[] image) throws IOException {
Path tmpDir = Files.createTempDirectory("resource-");
File resource = new File(tmpDir.toFile(), name);
Files.createDirectories(PathUtils.fileToPath(resource.getParentFile()));
File resource = new File(tmpDir.toFile(), getBaseName(name.replace('\\', '/')));
Copy link
Member

@jtnord jtnord Dec 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I see this quite a lot presumably to attempt to normalize windows paths.
but Linux paths can (even though it is really really not nornal) have a \ as part of the path.
EXPORT TMP=/tmp/this_is_a_directory_with_a_\\_ok

Happens when the home directory uses a username that comes from samba and using multiple domains and you need to distinguish DOMAIN1\user and DOMAIN2\user and the full domainname e.g. DOMAIN\user and so normalizing this will lead to something unexpected

Suggested change
File resource = new File(tmpDir.toFile(), getBaseName(name.replace('\\', '/')));
File resource = new File(tmpDir.toFile(), getBaseName(name.replace(File.separatorChar, '/')));

I don't think you can have a / inside a directory on windows

Copy link
Member Author

Choose a reason for hiding this comment

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

So this breaks if someone passes in a filename whose last character is \. Unsure whether such callers exist.

Otherwise this will just use a substring of the filename. Since there's one directory per file, there's no risk of collisions, making diagnostics a bit more difficult.

Given existing use of this idiom in the codebase I'm tempted to leave this as is. WDYT?

Copy link
Member

@jtnord jtnord Dec 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

So this breaks if someone passes in a filename whose last character is \.

it breaks if someone passes in a name whose filename would contain a \ not necessarily just the last character of the filename when running on non windows OSes.

And by break, it would just mean the returned File does not have the filename that the caller passed in. If this actually breaks anything or not I have no Idea!

Unsure whether such callers exist.

I am not sure what the users of this function are, but I would the case to be exceptionally rare in any case.

Given existing use of this idiom in the codebase

I believe those are no-ops as the path in those is the path within the URL and a URL should even for windows paths use / as separators (file://c:/foo/bar/wibble) and any \ would have to be encoded as %5C

That said you can construct invalid URLs so if they are user supplied or built by hand 🤷

I'm tempted to leave this as is. WDYT?

I'm not blocking this PR on this, feel free to choose whatever path you desire.

Files.createFile(PathUtils.fileToPath(resource));

try (FileOutputStream fos = new FileOutputStream(resource)) {
Expand Down
Loading