Subfolder support for images

josemmo / yamipa

Yet Another Minecraft Image Placing Addon

MIT License

73 stars 19 forks source link

What is this Pr

This pr adds support for subfolders for images in the images folder. This can be important if you are working on a large server with many different images, as it makes sorting and keeping track of the images much easier. I have also read your response to this feature posted at https://github.com/josemmo/yamipa/issues/28#issuecomment-1020464674.

Adressing your concerns regarding performance and potential vulnerabilities

Performance

Yes, this function theoretically degrades the performance of the server. In my opinion, this is negligible, since the only real impact on the startup process is when reading the images from the folders. Let's consider a realistic example: a server with about 300 images and 50 folders. The impact of retrieving the files in those 50 folders is probably much less than the impact of reading a single image file. Monitoring file events in these additional folders does not really affect performance, since java.nio's WatchService only receives the events from the file system and does not actually look for file changes by itself. (See WatchService)

Potential path traversal vulnerabilities

These are not present because the user input is not actually used to retrieve and read the file. Rather, the inputted string is only used to retrieve the cached ImageFile that is created in the cachedImages map of the ImageStorage class at startup. Therefore, any user input is automatically filtered to be one of the valid paths for image files, leaving no opportunity for vulnerabilities.

What is this Pr not

This version does not support individual player image folders. Placing images, etc. still works as usual, but the actual image files can be placed in subfolders of the image folder.

Hello @Hasenzahn1,

The PR you submitted has several issues.

The WatchService#poll() method returns inmediately, without waiting until new events are found. Given it's in inside an infinite loop, it most surely has poor performance:

while(true) {
  WatchKey watchKey = watchService.poll(); // Parse all happened events
  if (watchKey == null)  { // Stop if no events are present
    return;
  }
  // [...]
}

Probably what's best here is to use WatchService#take(), but that would require re-working the ImageStorage class so that the watcher runs in a dedicated separate thread that can be interrupted.

I'm not sure either if this code has been tested, because this infinite loop is inside a Bukkit.getScheduler().runTaskTimerAsynchronously() that gets created every 5 seconds, and that's certainly going to hurt the server at some point.

Also, replacing forward slashes ("/") and backslashes ("\") is asking for trouble in terms of security:

return cachedImages.get(filename.replace("/", "\\"));

Backlashes only work on Windows systems, this is going to an issue for Linux servers. I'm guessing you did this because the "images.dat" file that Yamipa creates uses a "/" as a field separator on purpose. That file would also need to be migrated to a new format if support for directories is added.

TL;DR: What I'm trying to get to here is that, as I said in several occassions already, adding support for directories is not as simple as it sounds because there's a few side effects that need to be taken into account. I'm probably going to add it to Yamipa in the future, but I need to find time for it and think it through.

josemmo / yamipa