From fc52d14ab1702ef8c897dcf5d34fe19f9d12be7a Mon Sep 17 00:00:00 2001 From: Anthony Stirling <77850077+Frooodle@users.noreply.github.com.> Date: Fri, 25 Jul 2025 15:23:56 +0100 Subject: [PATCH] fixes --- .../software/common/util/GeneralUtils.java | 202 +++++++++++++++++- .../api/AdminSettingsController.java | 180 ++++++++++++---- 2 files changed, 341 insertions(+), 41 deletions(-) diff --git a/app/common/src/main/java/stirling/software/common/util/GeneralUtils.java b/app/common/src/main/java/stirling/software/common/util/GeneralUtils.java index a164faec2..6c1d7ae1a 100644 --- a/app/common/src/main/java/stirling/software/common/util/GeneralUtils.java +++ b/app/common/src/main/java/stirling/software/common/util/GeneralUtils.java @@ -5,6 +5,8 @@ import java.io.FileOutputStream; import java.io.IOException; import java.io.InputStream; import java.net.*; +import java.nio.channels.FileChannel; +import java.nio.channels.FileLock; import java.nio.charset.StandardCharsets; import java.nio.file.*; import java.nio.file.attribute.BasicFileAttributes; @@ -15,6 +17,8 @@ import java.util.Enumeration; import java.util.List; import java.util.Locale; import java.util.UUID; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.locks.ReentrantReadWriteLock; import org.springframework.core.io.ClassPathResource; import org.springframework.core.io.Resource; @@ -37,6 +41,25 @@ public class GeneralUtils { private static final List DEFAULT_VALID_SCRIPTS = List.of("png_to_webp.py", "split_photos.py"); + // Concurrency control for settings file operations + private static final ReentrantReadWriteLock settingsLock = + new ReentrantReadWriteLock(true); // fair locking + private static volatile String lastSettingsHash = null; + + // Lock timeout configuration + private static final long LOCK_TIMEOUT_SECONDS = 30; // Maximum time to wait for locks + private static final long FILE_LOCK_TIMEOUT_MS = 5000; // File lock timeout + + // Initialize settings hash on first access + static { + try { + lastSettingsHash = calculateSettingsHash(); + } catch (Exception e) { + log.warn("Could not initialize settings hash: {}", e.getMessage()); + lastSettingsHash = ""; + } + } + public static File convertMultipartFileToFile(MultipartFile multipartFile) throws IOException { String customTempDir = System.getenv("STIRLING_TEMPFILES_DIRECTORY"); if (customTempDir == null || customTempDir.isEmpty()) { @@ -397,12 +420,183 @@ public class GeneralUtils { * Internal Implementation Details * *------------------------------------------------------------------------*/ + /** + * Thread-safe method to save a key-value pair to settings file with concurrency control. + * Prevents race conditions and data corruption when multiple threads/admins modify settings. + * + * @param key The setting key in dot notation (e.g., "security.enableCSRF") + * @param newValue The new value to set + * @throws IOException If file operations fail + * @throws IllegalStateException If settings file was modified by another process + */ public static void saveKeyToSettings(String key, Object newValue) throws IOException { - String[] keyArray = key.split("\\."); + // Use timeout to prevent infinite blocking + boolean lockAcquired = false; + try { + lockAcquired = settingsLock.writeLock().tryLock(LOCK_TIMEOUT_SECONDS, TimeUnit.SECONDS); + if (!lockAcquired) { + throw new IOException( + String.format( + "Could not acquire write lock for setting '%s' within %d seconds. " + + "Another admin operation may be in progress or the system may be under heavy load.", + key, LOCK_TIMEOUT_SECONDS)); + } + Path settingsPath = Paths.get(InstallationPathConfig.getSettingsPath()); + + // Attempt file locking with timeout and retry logic + FileLock fileLock = null; + long startTime = System.currentTimeMillis(); + + while (fileLock == null + && (System.currentTimeMillis() - startTime) < FILE_LOCK_TIMEOUT_MS) { + try (FileChannel channel = + FileChannel.open( + settingsPath, + StandardOpenOption.READ, + StandardOpenOption.WRITE, + StandardOpenOption.CREATE)) { + + // Try non-blocking lock first + fileLock = channel.tryLock(); + + if (fileLock != null) { + try { + // Validate that we can actually read/write to detect stale locks + if (!Files.isWritable(settingsPath)) { + throw new IOException( + "Settings file is not writable - permissions issue"); + } + + // Check for concurrent modifications + String currentHash = calculateSettingsHash(); + if (lastSettingsHash != null && !lastSettingsHash.equals(currentHash)) { + log.info( + "Settings file was modified externally for key: {} - updating hash", + key); + lastSettingsHash = currentHash; + } + + // Perform the actual update + String[] keyArray = key.split("\\."); + YamlHelper settingsYaml = new YamlHelper(settingsPath); + settingsYaml.updateValue(Arrays.asList(keyArray), newValue); + settingsYaml.saveOverride(settingsPath); + + // Update hash after successful write + lastSettingsHash = calculateSettingsHash(); + + log.debug("Successfully updated setting: {} = {}", key, newValue); + return; // Success - exit method + + } finally { + // Ensure file lock is always released + if (fileLock != null && fileLock.isValid()) { + try { + fileLock.release(); + } catch (IOException e) { + log.warn( + "Failed to release file lock for setting {}: {}", + key, + e.getMessage()); + } + } + } + } else { + // Lock not available, wait briefly before retry + Thread.sleep(100); + } + + } catch (IOException e) { + if (fileLock != null && fileLock.isValid()) { + try { + fileLock.release(); + } catch (IOException releaseError) { + log.warn( + "Failed to release file lock after error: {}", + releaseError.getMessage()); + } + } + throw e; + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new IOException("Interrupted while waiting for file lock", e); + } + } + + // If we get here, we couldn't acquire the file lock within timeout + throw new IOException( + String.format( + "Could not acquire file lock for setting '%s' within %d ms. " + + "The settings file may be locked by another process or there may be file system issues.", + key, FILE_LOCK_TIMEOUT_MS)); + + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new IOException("Interrupted while waiting for settings lock", e); + } catch (Exception e) { + log.error("Unexpected error updating setting {}: {}", key, e.getMessage(), e); + if (e instanceof IOException) { + throw (IOException) e; + } + throw new IOException("Failed to update settings: " + e.getMessage(), e); + } finally { + if (lockAcquired) { + settingsLock.writeLock().unlock(); + } + } + } + + /** + * Calculates MD5 hash of the settings file for change detection + * + * @return Hash string, or empty string if file doesn't exist + */ + private static String calculateSettingsHash() throws Exception { Path settingsPath = Paths.get(InstallationPathConfig.getSettingsPath()); - YamlHelper settingsYaml = new YamlHelper(settingsPath); - settingsYaml.updateValue(Arrays.asList(keyArray), newValue); - settingsYaml.saveOverride(settingsPath); + if (!Files.exists(settingsPath)) { + return ""; + } + + byte[] fileBytes = Files.readAllBytes(settingsPath); + MessageDigest md = MessageDigest.getInstance("MD5"); + byte[] hashBytes = md.digest(fileBytes); + + StringBuilder sb = new StringBuilder(); + for (byte b : hashBytes) { + sb.append(String.format("%02x", b)); + } + return sb.toString(); + } + + /** + * Thread-safe method to read settings values with proper locking and timeout + * + * @return YamlHelper instance for reading + * @throws IOException If timeout occurs or file operations fail + */ + public static YamlHelper getSettingsReader() throws IOException { + boolean lockAcquired = false; + try { + lockAcquired = settingsLock.readLock().tryLock(LOCK_TIMEOUT_SECONDS, TimeUnit.SECONDS); + if (!lockAcquired) { + throw new IOException( + String.format( + "Could not acquire read lock for settings within %d seconds. " + + "System may be under heavy load or there may be a deadlock.", + LOCK_TIMEOUT_SECONDS)); + } + + Path settingsPath = Paths.get(InstallationPathConfig.getSettingsPath()); + return new YamlHelper(settingsPath); + + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new IOException("Interrupted while waiting for settings read lock", e); + } finally { + if (lockAcquired) { + settingsLock.readLock().unlock(); + } + } } public static String generateMachineFingerprint() { diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/security/controller/api/AdminSettingsController.java b/app/proprietary/src/main/java/stirling/software/proprietary/security/controller/api/AdminSettingsController.java index 6ef89a1c9..b87378055 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/security/controller/api/AdminSettingsController.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/security/controller/api/AdminSettingsController.java @@ -2,6 +2,7 @@ package stirling.software.proprietary.security.controller.api; import java.io.IOException; import java.util.Map; +import java.util.regex.Pattern; import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; @@ -14,6 +15,8 @@ import org.springframework.web.bind.annotation.RequestBody; import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.util.HtmlUtils; +import com.fasterxml.jackson.databind.ObjectMapper; + import io.swagger.v3.oas.annotations.Operation; import io.swagger.v3.oas.annotations.responses.ApiResponse; import io.swagger.v3.oas.annotations.responses.ApiResponses; @@ -38,20 +41,8 @@ import stirling.software.proprietary.security.model.api.admin.UpdateSettingsRequ @Slf4j public class AdminSettingsController { - private static final java.util.Set VALID_SECTIONS = - java.util.Set.of( - "security", - "system", - "ui", - "endpoints", - "metrics", - "mail", - "premium", - "processExecutor", - "autoPipeline", - "legal"); - private final ApplicationProperties applicationProperties; + private final ObjectMapper objectMapper; @GetMapping @Operation( @@ -89,12 +80,20 @@ public class AdminSettingsController { @Valid @RequestBody UpdateSettingsRequest request) { try { Map settings = request.getSettings(); + if (settings == null || settings.isEmpty()) { + return ResponseEntity.badRequest().body("No settings provided to update"); + } int updatedCount = 0; for (Map.Entry entry : settings.entrySet()) { String key = entry.getKey(); Object value = entry.getValue(); + if (!isValidSettingKey(key)) { + return ResponseEntity.badRequest() + .body("Invalid setting key format: " + HtmlUtils.htmlEscape(key)); + } + log.info("Admin updating setting: {} = {}", key, value); GeneralUtils.saveKeyToSettings(key, value); updatedCount++; @@ -110,10 +109,14 @@ public class AdminSettingsController { return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR) .body("Failed to save settings to configuration file."); + } catch (IllegalArgumentException e) { + log.error("Invalid setting key or value: {}", e.getMessage(), e); + return ResponseEntity.status(HttpStatus.BAD_REQUEST) + .body("Invalid setting key or value: " + e.getMessage()); } catch (Exception e) { log.error("Unexpected error while updating settings: {}", e.getMessage(), e); - return ResponseEntity.status(HttpStatus.BAD_REQUEST) - .body("Invalid setting key or value."); + return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR) + .body("Internal server error occurred while updating settings."); } } @@ -140,10 +143,15 @@ public class AdminSettingsController { .body( "Invalid section name: " + HtmlUtils.htmlEscape(sectionName) - + ". Valid sections: security, system, ui, endpoints, metrics, mail, premium, processExecutor, autoPipeline, legal"); + + ". Valid sections: " + + String.join(", ", VALID_SECTION_NAMES)); } log.debug("Admin requested settings section: {}", sectionName); return ResponseEntity.ok(sectionData); + } catch (IllegalArgumentException e) { + log.error("Invalid section name {}: {}", sectionName, e.getMessage(), e); + return ResponseEntity.status(HttpStatus.BAD_REQUEST) + .body("Invalid section name: " + HtmlUtils.htmlEscape(sectionName)); } catch (Exception e) { log.error("Error retrieving section {}: {}", sectionName, e.getMessage(), e); return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR) @@ -178,16 +186,23 @@ public class AdminSettingsController { .body( "Invalid section name: " + HtmlUtils.htmlEscape(sectionName) - + ". Valid sections: security, system, ui, endpoints, metrics, mail, premium, processExecutor, autoPipeline, legal"); + + ". Valid sections: " + + String.join(", ", VALID_SECTION_NAMES)); } int updatedCount = 0; for (Map.Entry entry : sectionData.entrySet()) { - String key = sectionName + "." + entry.getKey(); + String propertyKey = entry.getKey(); + String fullKey = sectionName + "." + propertyKey; Object value = entry.getValue(); - log.info("Admin updating section setting: {} = {}", key, value); - GeneralUtils.saveKeyToSettings(key, value); + if (!isValidSettingKey(fullKey)) { + return ResponseEntity.badRequest() + .body("Invalid setting key format: " + HtmlUtils.htmlEscape(fullKey)); + } + + log.info("Admin updating section setting: {} = {}", fullKey, value); + GeneralUtils.saveKeyToSettings(fullKey, value); updatedCount++; } @@ -201,9 +216,14 @@ public class AdminSettingsController { log.error("Failed to save section settings to file: {}", e.getMessage(), e); return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR) .body("Failed to save settings to configuration file."); + } catch (IllegalArgumentException e) { + log.error("Invalid section data: {}", e.getMessage(), e); + return ResponseEntity.status(HttpStatus.BAD_REQUEST) + .body("Invalid section data: " + e.getMessage()); } catch (Exception e) { log.error("Unexpected error while updating section settings: {}", e.getMessage(), e); - return ResponseEntity.status(HttpStatus.BAD_REQUEST).body("Invalid section data."); + return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR) + .body("Internal server error occurred while updating section settings."); } } @@ -224,6 +244,11 @@ public class AdminSettingsController { }) public ResponseEntity getSettingValue(@PathVariable String key) { try { + if (!isValidSettingKey(key)) { + return ResponseEntity.badRequest() + .body("Invalid setting key format: " + HtmlUtils.htmlEscape(key)); + } + Object value = getSettingByKey(key); if (value == null) { return ResponseEntity.badRequest() @@ -231,6 +256,10 @@ public class AdminSettingsController { } log.debug("Admin requested setting: {}", key); return ResponseEntity.ok(new SettingValueResponse(key, value)); + } catch (IllegalArgumentException e) { + log.error("Invalid setting key {}: {}", key, e.getMessage(), e); + return ResponseEntity.status(HttpStatus.BAD_REQUEST) + .body("Invalid setting key: " + HtmlUtils.htmlEscape(key)); } catch (Exception e) { log.error("Error retrieving setting {}: {}", key, e.getMessage(), e); return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR) @@ -255,6 +284,11 @@ public class AdminSettingsController { public ResponseEntity updateSettingValue( @PathVariable String key, @Valid @RequestBody UpdateSettingValueRequest request) { try { + if (!isValidSettingKey(key)) { + return ResponseEntity.badRequest() + .body("Invalid setting key format: " + HtmlUtils.htmlEscape(key)); + } + Object value = request.getValue(); log.info("Admin updating single setting: {} = {}", key, value); GeneralUtils.saveKeyToSettings(key, value); @@ -269,14 +303,22 @@ public class AdminSettingsController { log.error("Failed to save setting to file: {}", e.getMessage(), e); return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR) .body("Failed to save setting to configuration file."); + } catch (IllegalArgumentException e) { + log.error("Invalid setting key or value: {}", e.getMessage(), e); + return ResponseEntity.status(HttpStatus.BAD_REQUEST) + .body("Invalid setting key or value: " + e.getMessage()); } catch (Exception e) { log.error("Unexpected error while updating setting: {}", e.getMessage(), e); - return ResponseEntity.status(HttpStatus.BAD_REQUEST) - .body("Invalid setting key or value."); + return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR) + .body("Internal server error occurred while updating setting."); } } private Object getSectionData(String sectionName) { + if (sectionName == null || sectionName.trim().isEmpty()) { + return null; + } + return switch (sectionName.toLowerCase()) { case "security" -> applicationProperties.getSecurity(); case "system" -> applicationProperties.getSystem(); @@ -285,8 +327,8 @@ public class AdminSettingsController { case "metrics" -> applicationProperties.getMetrics(); case "mail" -> applicationProperties.getMail(); case "premium" -> applicationProperties.getPremium(); - case "processexecutor" -> applicationProperties.getProcessExecutor(); - case "autopipeline" -> applicationProperties.getAutoPipeline(); + case "processexecutor", "processExecutor" -> applicationProperties.getProcessExecutor(); + case "autopipeline", "autoPipeline" -> applicationProperties.getAutoPipeline(); case "legal" -> applicationProperties.getLegal(); default -> null; }; @@ -296,7 +338,54 @@ public class AdminSettingsController { return getSectionData(sectionName) != null; } + private static final java.util.Set VALID_SECTION_NAMES = + java.util.Set.of( + "security", + "system", + "ui", + "endpoints", + "metrics", + "mail", + "premium", + "processExecutor", + "processexecutor", + "autoPipeline", + "autopipeline", + "legal"); + + // Pattern to validate safe property paths - only alphanumeric, dots, and underscores + private static final Pattern SAFE_KEY_PATTERN = Pattern.compile("^[a-zA-Z0-9._]+$"); + private static final int MAX_NESTING_DEPTH = 10; + + private boolean isValidSettingKey(String key) { + if (key == null || key.trim().isEmpty()) { + return false; + } + + // Check against pattern to prevent injection attacks + if (!SAFE_KEY_PATTERN.matcher(key).matches()) { + return false; + } + + // Prevent excessive nesting depth + String[] parts = key.split("\\."); + if (parts.length > MAX_NESTING_DEPTH) { + return false; + } + + // Ensure first part is a valid section name + if (parts.length > 0 && !VALID_SECTION_NAMES.contains(parts[0].toLowerCase())) { + return false; + } + + return true; + } + private Object getSettingByKey(String key) { + if (key == null || key.trim().isEmpty()) { + return null; + } + String[] parts = key.split("\\.", 2); if (parts.length < 2) { return null; @@ -311,29 +400,46 @@ public class AdminSettingsController { } try { - return getNestedProperty(section, propertyPath); - } catch (Exception e) { + return getNestedProperty(section, propertyPath, 0); + } catch (NoSuchFieldException | IllegalAccessException e) { log.warn("Failed to get nested property {}: {}", key, e.getMessage()); return null; } } - private Object getNestedProperty(Object obj, String propertyPath) throws Exception { + private Object getNestedProperty(Object obj, String propertyPath, int depth) + throws NoSuchFieldException, IllegalAccessException { if (obj == null) { return null; } - String[] parts = propertyPath.split("\\.", 2); - String currentProperty = parts[0]; + // Prevent excessive recursion depth + if (depth > MAX_NESTING_DEPTH) { + throw new IllegalAccessException("Maximum nesting depth exceeded"); + } - java.lang.reflect.Field field = obj.getClass().getDeclaredField(currentProperty); - field.setAccessible(true); - Object value = field.get(obj); + try { + // Use Jackson ObjectMapper for safer property access + @SuppressWarnings("unchecked") + Map objectMap = objectMapper.convertValue(obj, Map.class); - if (parts.length == 1) { - return value; - } else { - return getNestedProperty(value, parts[1]); + String[] parts = propertyPath.split("\\.", 2); + String currentProperty = parts[0]; + + if (!objectMap.containsKey(currentProperty)) { + throw new NoSuchFieldException("Property not found: " + currentProperty); + } + + Object value = objectMap.get(currentProperty); + + if (parts.length == 1) { + return value; + } else { + return getNestedProperty(value, parts[1], depth + 1); + } + } catch (IllegalArgumentException e) { + // If Jackson fails, the property doesn't exist or isn't accessible + throw new NoSuchFieldException("Property not accessible: " + propertyPath); } } }