- Code quality and security improvements (#10665)

java/services/src/main/java/it/geosolutions/mapstore/controllers/configs/ConfigsController.java

@@ -7,7 +7,9 @@
 */
 package it.geosolutions.mapstore.controllers.configs;
 
+import java.io.File;
 import java.io.IOException;
+import java.nio.file.Path;
 import java.nio.file.Paths;
 
 import org.springframework.stereotype.Controller;
@@ -30,31 +32,82 @@
 @Controller
 public class ConfigsController extends BaseConfigController {
 
-
     /**
      * Loads the resource, from the configured location (datadir or web root).
-     * Both locations are tested and the resource is returned from the first location found.
-     * The resource name should be in the allowed.resources list.
-     *
+     * Both locations are tested, and the resource is returned from the first location found.
+     * The resource name should be in the allowed. Resources list.
+     * <p>
      * It is also possible to store in datadir files in the json-patch format (with a .json.patch extension)
      * so that the final resource is built merging a static configuration with a patch.
      *
-     * @param {String} resourceName name of the resource to load (e.g. localConfig)
-     * @param {boolean} overrides apply overrides from the configured properties file (if any)
+     * @param resourceName Name of the resource to load (e.g., localConfig).
+     * @param applyOverrides Apply overrides from the configured properties file (if any).
+     * @return The configuration file as a byte array.
+     * @throws IOException If an error occurs while reading the resource.
      */
-    @RequestMapping(value="/{resource}", method = RequestMethod.GET) // TODO: search in configs directory base
-    public @ResponseBody byte[] loadResource(@PathVariable("resource") String resourceName, @RequestParam(value="overrides", defaultValue="true") boolean applyOverrides) throws IOException {
-        return toBytes(readResource( Paths.get(getConfigsFolder(),  normalizeExtension(resourceName, "json")).toString(), applyOverrides, Paths.get(getConfigsFolder(),  normalizePatchExtension(resourceName, "json", "patch")).toString()));
+    @RequestMapping(value = "/{resource}", method = RequestMethod.GET)
+    public @ResponseBody byte[] loadResource(@PathVariable("resource") String resourceName,
+                                             @RequestParam(value = "overrides", defaultValue = "true") boolean applyOverrides) throws IOException {
+
+        // Validate the resource name to prevent path traversal
+        String sanitizedResourceName = sanitizeResourceName(resourceName);
+
+        // Construct paths for the primary configuration and patch files
+        Path configPath = Paths.get(getConfigsFolder(), normalizedFilePath(sanitizedResourceName, "json"));
+        Path patchPath = Paths.get(getConfigsFolder(), normalizePatchExtension(sanitizedResourceName, "json", "patch"));
+
+        // Ensure that both paths stay within the base configurations directory
+        validatePathWithinConfigDirectory(configPath);
+        validatePathWithinConfigDirectory(patchPath);
+
+        // Load the primary configuration and apply the patch if it exists
+        return toBytes(readResource(configPath.toString(), applyOverrides, patchPath.toString()));
     }
 
-    private String normalizeExtension(String name, String extension) {
-        if (name.toLowerCase().endsWith("." + extension.toLowerCase()))
-            return name;
-        return name + "." + extension;
+    /**
+     * Sanitizes the resource name to prevent directory traversal attacks.
+     *
+     * @param resourceName The original resource name from the request.
+     * @return A sanitized resource name with only valid file name characters.
+     * @throws IOException If the resource name is invalid.
+     */
+    private String sanitizeResourceName(String resourceName) throws IOException {
+        if (resourceName.contains("..") || resourceName.contains(File.separator)) {
+            throw new IOException("Invalid resource name: Directory traversal attempt detected.");
+        }
+        return resourceName;
     }
+
+    /**
+     * Normalize the file path by adding an extension if it's missing.
+     *
+     * @param name The file name or resource name.
+     * @param extension The extension to ensure on the file name.
+     * @return The normalized file path with the correct extension.
+     */
+    private String normalizedFilePath(String name, String extension) {
+        if (!name.toLowerCase().endsWith("." + extension)) {
+            return name + "." + extension;
+        }
+        return name;
+    }
+
     private String normalizePatchExtension(String name, String extension, String patchExtension) {
         if (name.toLowerCase().endsWith("." + extension.toLowerCase()))
             return name + "." + patchExtension; // config.json --> config.json+.patch
         return name + "." + extension + "." + patchExtension; // config --> config+.json.patch
     }
+
+    /**
+     * Validates that a path is within the base configurations' directory.
+     *
+     * @param path The path to validate.
+     * @throws IOException If the path is outside the base directory.
+     */
+    private void validatePathWithinConfigDirectory(Path path) throws IOException {
+        Path baseDir = Paths.get(getConfigsFolder()).toAbsolutePath().normalize();
+        if (!path.toAbsolutePath().normalize().startsWith(baseDir)) {
+            throw new IOException("Access to the specified resource is denied.");
+        }
+    }
 }

java/services/src/main/java/it/geosolutions/mapstore/controllers/rest/config/UploadPluginController.java

@@ -13,6 +13,7 @@
 import java.io.FileOutputStream;
 import java.io.IOException;
 import java.io.InputStream;
+import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.util.HashMap;
 import java.util.Map;
@@ -161,72 +162,114 @@ public boolean test(String folder) {
 	 *
 	 * @param pluginName name of the extension to be removed
 	 */
-	@Secured({ "ROLE_ADMIN" })
-	@RequestMapping(value = "/uninstallPlugin/{pluginName}", method = RequestMethod.DELETE)
-	public @ResponseBody String uninstallPlugin(@PathVariable String pluginName) throws IOException {
-		ObjectNode configObj = getExtensionConfig();
-		String key = pluginName;
-		if (configObj.has(key)) {
-			JsonNode pluginConfig = configObj.get(key);
-			String pluginBundle = pluginConfig.get("bundle").asText();
-			String pluginFolder = pluginBundle.substring(0, pluginBundle.lastIndexOf("/"));
-			removeFolder(Paths.get(getExtensionsFolder(), pluginFolder).toString());
-			configObj.remove(key);
-			storeJSONConfig(configObj, getExtensionsConfigPath());
-			ObjectNode pluginsConfigObj = null;
-			ArrayNode plugins;
-			if (shouldStorePluginsConfigAsPatch()) {
-				plugins = getPluginsConfigurationPatch();
-			} else {
-				pluginsConfigObj = getPluginsConfiguration();
-				plugins = (ArrayNode) pluginsConfigObj.get("plugins");
-			}
-			int toRemove = -1;
-			for (int i = 0; i < plugins.size(); i++) {
-				JsonNode plugin = plugins.get(i);
-				String name = plugin.has("name") ? plugin.get("name").asText()
-						: plugin.get("value").get("name").asText();
-				if (name.contentEquals(pluginName)) {
-					toRemove = i;
-				}
-			}
-			if (toRemove >= 0) {
-				plugins.remove(toRemove);
-			}
-			if (shouldStorePluginsConfigAsPatch()) {
-				storeJSONConfig(plugins, getPluginsConfigPatchFilePath());
-			} else {
-				storeJSONConfig(pluginsConfigObj, getPluginsConfig());
-			}
-			return pluginConfig.toString();
-		} else {
-			return "{}";
-		}
+    @Secured({ "ROLE_ADMIN" })
+    @RequestMapping(value = "/uninstallPlugin/{pluginName}", method = RequestMethod.DELETE)
+    public @ResponseBody String uninstallPlugin(@PathVariable String pluginName) throws IOException {
+        // Basic validation to avoid path traversal characters in pluginName
+        if (pluginName.contains("..") || pluginName.contains("\\")) {
+            throw new IllegalArgumentException("Invalid plugin name.");
+        }
+
+        ObjectNode configObj = getExtensionConfig();
+        if (configObj.has(pluginName)) {
+            JsonNode pluginConfig = configObj.get(pluginName);
+            String pluginBundle = pluginConfig.get("bundle").asText();
+            String pluginFolder = pluginBundle.substring(0, pluginBundle.lastIndexOf("/"));
+
+            // Securely remove the folder by passing normalized path
+            Path pluginFolderPath = Paths.get(getExtensionsFolder(), pluginFolder).normalize();
+            removeFolderSecurely(pluginFolderPath);
+
+            // Update configurations after removing the folder
+            configObj.remove(pluginName);
+            storeJSONConfig(configObj, getExtensionsConfigPath());
+
+            ObjectNode pluginsConfigObj = null;
+            ArrayNode plugins;
+            if (shouldStorePluginsConfigAsPatch()) {
+                plugins = getPluginsConfigurationPatch();
+            } else {
+                pluginsConfigObj = getPluginsConfiguration();
+                plugins = (ArrayNode) pluginsConfigObj.get("plugins");
+            }
+
+            int toRemove = -1;
+            for (int i = 0; i < plugins.size(); i++) {
+                JsonNode plugin = plugins.get(i);
+                String name = plugin.has("name") ? plugin.get("name").asText()
+                    : plugin.get("value").get("name").asText();
+                if (name.contentEquals(pluginName)) {
+                    toRemove = i;
+                }
+            }
+
+            if (toRemove >= 0) {
+                plugins.remove(toRemove);
+            }
+
+            if (shouldStorePluginsConfigAsPatch()) {
+                storeJSONConfig(plugins, getPluginsConfigPatchFilePath());
+            } else {
+                storeJSONConfig(pluginsConfigObj, getPluginsConfig());
+            }
+
+            return pluginConfig.toString();
+        } else {
+            return "{}";
+        }
+    }
+
+    // Updated removeFolder method with path traversal prevention
+    private void removeFolderSecurely(Path pluginFolderPath) throws IOException {
+        if (pluginFolderPath == null) {
+            throw new IllegalArgumentException("Plugin folder path cannot be null.");
+        }
+
+        // Define base directory to ensure path remains within it
+        Path baseDirectory = Paths.get(getExtensionsFolder()).toAbsolutePath().normalize();
+        if (baseDirectory == null) {
+            throw new IllegalStateException("Extensions folder path is not set correctly.");
+        }
+
+        Path fullPath = pluginFolderPath.toAbsolutePath().normalize();
+
+        // Ensure the path is within the base directory
+        if (!fullPath.startsWith(baseDirectory)) {
+            throw new IOException("Unauthorized path traversal attempt detected.");
+        }
+
+        File folderPath = new File(ResourceUtils.getResourcePath(getWriteStorage(), context, pluginFolderPath.toString()));
+        if (folderPath.exists()) {
+            FileUtils.cleanDirectory(folderPath);
+            folderPath.delete();
+        } else {
+            throw new FileNotFoundException("The specified folder path does not exist: " + folderPath.getAbsolutePath());
+        }
+    }
+
+    private Optional<File> findResource(String resourceName) {
+		return ResourceUtils.findResource(getDataDir(), context, resourceName);
 	}
 
-	private void removeFolder(String pluginFolder) throws IOException {
-		File folderPath = new File(ResourceUtils.getResourcePath(getWriteStorage(), context, pluginFolder));
-		if (folderPath.exists()) {
-			FileUtils.cleanDirectory(folderPath);
-			folderPath.delete();
-		}
-	}
+    private void moveAsset(File tempAsset, String finalAsset) throws FileNotFoundException, IOException {
+        String assetPath = ResourceUtils.getResourcePath(getWriteStorage(), context, finalAsset);
 
-	private Optional<File> findResource(String resourceName) {
-		return ResourceUtils.findResource(getDataDir(), context, resourceName);
-	}
+        // Check if the resource path is null and handle it
+        if (assetPath == null) {
+            throw new FileNotFoundException("Resource path could not be resolved for: " + finalAsset);
+        }
 
-	private void moveAsset(File tempAsset, String finalAsset) throws FileNotFoundException, IOException {
-		String assetPath = ResourceUtils.getResourcePath(getWriteStorage(), context, finalAsset);
-		new File(assetPath).getParentFile().mkdirs();
-		try (FileInputStream input = new FileInputStream(tempAsset);
-				FileOutputStream output = new FileOutputStream(assetPath)) {
-			IOUtils.copy(input, output);
-		}
-		tempAsset.delete();
-	}
+        // Ensure the parent directory exists
+        new File(assetPath).getParentFile().mkdirs();
+
+        try (FileInputStream input = new FileInputStream(tempAsset);
+             FileOutputStream output = new FileOutputStream(assetPath)) {
+            IOUtils.copy(input, output);
+        }
+        tempAsset.delete();
+    }
 
-	private String getWriteStorage() {
+    private String getWriteStorage() {
 		return getDataDir().isEmpty() ? "" : Stream.of(getDataDir().split(",")).filter(new Predicate<String>() {
 			@Override
 			public boolean test(String folder) {

java/services/src/main/java/it/geosolutions/mapstore/utils/ResourceUtils.java

@@ -27,15 +27,15 @@ public static Optional<String> findExisting(String[] candidates) {
             public boolean test(String path) {
                 return path != null && new File(path).exists();
             }
-            
+
         })
         .findFirst();
     }
-	
+
 	/**
 	 * Finds a resource, recursively looking at a list of folders, and at last at
 	 * the web application context path.
-	 * 
+	 *
 	 * @param baseFolders  comma delimited list of folders, in order of search
 	 * @param context      web application context (last resource)
 	 * @param resourceName name of the resource to be found
@@ -53,7 +53,7 @@ public boolean test(String string) {
 						}
 						return true;
 					}
-					
+
 				})
 				.map(new Function<String, String>() {
 			@Override
@@ -73,15 +73,15 @@ public String[] apply(int size) {
 		}
 		return Optional.empty();
 	}
-	
+
 	public static String getResourcePath(String baseFolder, ServletContext context, String path) {
         return getResourcePath(baseFolder, context, path, false);
     }
-	
+
 	public static String getResourcePath(String baseFolder, ServletContext context, String path, boolean write) {
 		return baseFolder.isEmpty() ? getContextPath(context, path, write) : baseFolder + "/" + path;
 	}
-	
+
 	private static String getContextPath(ServletContext context, String path, boolean write) {
 	    String candidate = context.getRealPath(path);
 	    if (candidate == null && write) {

java/services/src/test/java/it/geosolutions/mapstore/ConfigControllerTest.java

@@ -63,7 +63,7 @@ public void testLoadFromManyDataDir() throws IOException {
         controller.setContext(context);
         String resource1 = new String(controller.loadResource("localConfig.json", false), "UTF-8");
         assertEquals("{}", resource1.trim());
-        
+
         tempResource1.delete();
     }
 
@@ -113,6 +113,7 @@ public void testPatch() throws IOException {
         tempResource.delete();
         tempPatch.delete();
     }
+
     @Test
     public void testPatchWithNoExtension() throws IOException {
         File dataDir = TestUtils.getDataDir();
@@ -128,4 +129,24 @@ public void testPatchWithNoExtension() throws IOException {
         tempResource.delete();
         tempPatch.delete();
     }
+
+    @Test(expected = IOException.class)
+    public void testPathTraversalAttempt() throws IOException {
+        // Attempt to load a resource with path traversal characters
+        String maliciousResourceName = "../etc/passwd";
+        controller.loadResource(maliciousResourceName, false);
+    }
+
+    @Test
+    public void testValidResourceAccess() throws IOException {
+        ServletContext context = Mockito.mock(ServletContext.class);
+        File tempResource = TestUtils.copyToTemp(ConfigControllerTest.class, "/localConfig.json");
+        Mockito.when(context.getRealPath(Mockito.anyString())).thenReturn(tempResource.getAbsolutePath());
+        controller.setContext(context);
+
+        String resource = new String(controller.loadResource("localConfig", false), "UTF-8");
+        assertEquals("{}", resource.trim());
+
+        tempResource.delete();
+    }
 }