Merge with Python Services - #26
Conversation
… external ML service
… recognition data, including faces, embeddings, and associated metadata.
…ecognition improvements
| String contentType = file.getContentType(); | ||
| if (contentType == null || !contentType.startsWith("image/")) { | ||
| log.warn("Validation failed: Invalid content type '{}' for file '{}'", | ||
| contentType, file.getOriginalFilename()); |
Check failure
Code scanning / CodeQL
Log Injection High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 months ago
In general, the fix is to ensure that any user-controlled data is sanitized before being included in log messages. For plain-text logs, removing or replacing carriage return (\r) and line feed (\n) characters is sufficient to prevent log injection via multi-line entries. Since this class already provides a sanitizeForLog(String value) helper that replaces CR and LF with spaces, we should apply it to the tainted values before logging them.
The best minimal-change fix here is to sanitize contentType (and, by the same reasoning, the file name from file.getOriginalFilename(), which is also user-controlled) when passing them to the logger. We should not change the application’s behavior other than how data is represented in logs. Concretely, in validateImageFile:
- For the invalid content-type branch, wrap both
contentTypeandfile.getOriginalFilename()withsanitizeForLog(...)in thelog.warncall on line 207–208. - For the file-size-exceeds branch, wrap
file.getOriginalFilename()withsanitizeForLog(...)in thelog.warncall on line 215–216.
No new methods or imports are needed: sanitizeForLog already exists in this class, and all logging is via the existing Lombok @Slf4j logger.
| @@ -204,16 +204,16 @@ | ||
|
|
||
| String contentType = file.getContentType(); | ||
| if (contentType == null || !contentType.startsWith("image/")) { | ||
| log.warn("Validation failed: Invalid content type '{}' for file '{}'", | ||
| contentType, file.getOriginalFilename()); | ||
| log.warn("Validation failed: Invalid content type '{}' for file '{}'", | ||
| sanitizeForLog(contentType), sanitizeForLog(file.getOriginalFilename())); | ||
| return ResponseEntity.badRequest() | ||
| .body(Map.of("error", "Invalid file type. Please upload an image.")); | ||
| } | ||
|
|
||
| long fileSize = file.getSize(); | ||
| if (fileSize > maxSizeBytes) { | ||
| log.warn("Validation failed: Image size {} exceeds limit {} for file '{}'", | ||
| fileSize, maxSizeBytes, file.getOriginalFilename()); | ||
| log.warn("Validation failed: Image size {} exceeds limit {} for file '{}'", | ||
| fileSize, maxSizeBytes, sanitizeForLog(file.getOriginalFilename())); | ||
| return ResponseEntity.badRequest() | ||
| .body(Map.of("error", "File size exceeds maximum limit of " + | ||
| (maxSizeBytes / (1024 * 1024)) + "MB: " + file.getOriginalFilename())); |
| String contentType = file.getContentType(); | ||
| if (contentType == null || !contentType.startsWith("image/")) { | ||
| log.warn("Validation failed: Invalid content type '{}' for file '{}'", | ||
| contentType, file.getOriginalFilename()); |
Check failure
Code scanning / CodeQL
Log Injection High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 months ago
In general, user-controlled values must be sanitized or validated before logging so they cannot inject line breaks or other control characters that make logs ambiguous or forge new entries. For plain-text logs, stripping or replacing \r and \n is a standard mitigation.
The best fix here, without changing existing functionality, is to apply the existing sanitizeForLog helper when logging the original filename in validateImageFile. This preserves the functional behavior (same filename is used for responses and validation) while ensuring that what gets written to the logs cannot introduce additional log lines. Specifically, we should:
- Extract
file.getOriginalFilename()into a local variable (e.g.,String originalFilename = sanitizeForLog(file.getOriginalFilename());) before logging. - Use this sanitized variable in all log statements that include the filename: the invalid content-type warning and the file-size-exceeded warning.
- Leave the rest of the method unchanged.
All required methods and imports already exist in FacialRecognitionController: sanitizeForLog is defined at lines 232–237, and Lombok’s @Slf4j supplies the log instance. No new imports or dependencies are needed.
| @@ -203,19 +203,20 @@ | ||
| } | ||
|
|
||
| String contentType = file.getContentType(); | ||
| String originalFilenameForLog = sanitizeForLog(file.getOriginalFilename()); | ||
| if (contentType == null || !contentType.startsWith("image/")) { | ||
| log.warn("Validation failed: Invalid content type '{}' for file '{}'", | ||
| contentType, file.getOriginalFilename()); | ||
| log.warn("Validation failed: Invalid content type '{}' for file '{}'", | ||
| contentType, originalFilenameForLog); | ||
| return ResponseEntity.badRequest() | ||
| .body(Map.of("error", "Invalid file type. Please upload an image.")); | ||
| } | ||
|
|
||
| long fileSize = file.getSize(); | ||
| if (fileSize > maxSizeBytes) { | ||
| log.warn("Validation failed: Image size {} exceeds limit {} for file '{}'", | ||
| fileSize, maxSizeBytes, file.getOriginalFilename()); | ||
| log.warn("Validation failed: Image size {} exceeds limit {} for file '{}'", | ||
| fileSize, maxSizeBytes, originalFilenameForLog); | ||
| return ResponseEntity.badRequest() | ||
| .body(Map.of("error", "File size exceeds maximum limit of " + | ||
| .body(Map.of("error", "File size exceeds maximum limit of " + | ||
| (maxSizeBytes / (1024 * 1024)) + "MB: " + file.getOriginalFilename())); | ||
| } | ||
|
|
| log.warn("Validation failed: Image size {} exceeds limit {} for file '{}'", | ||
| fileSize, maxSizeBytes, file.getOriginalFilename()); |
Check failure
Code scanning / CodeQL
Log Injection High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 months ago
To fix the problem, any user-controlled value logged in validateImageFile should be sanitized to remove newline and carriage-return characters before being passed to the logger. This aligns with the existing sanitizeForLog method in the same class, which already replaces \r and \n with spaces.
Concretely, in validateImageFile, we should not pass file.getOriginalFilename() directly into log.warn. Instead, we should call sanitizeForLog(file.getOriginalFilename()) and log that sanitized value. The same applies to any other log statement in this method that uses the original filename; in the current snippet, both the invalid content-type warning (line 207–208) and the size-exceeds-limit warning (line 215–216) log the filename. We will update both warnings to log the sanitized filename, and leave the response bodies unchanged so that existing functionality and user-facing behavior is preserved.
No new methods or imports are needed: sanitizeForLog already exists in this class, and we can simply call it.
| @@ -204,18 +204,18 @@ | ||
|
|
||
| String contentType = file.getContentType(); | ||
| if (contentType == null || !contentType.startsWith("image/")) { | ||
| log.warn("Validation failed: Invalid content type '{}' for file '{}'", | ||
| contentType, file.getOriginalFilename()); | ||
| log.warn("Validation failed: Invalid content type '{}' for file '{}'", | ||
| contentType, sanitizeForLog(file.getOriginalFilename())); | ||
| return ResponseEntity.badRequest() | ||
| .body(Map.of("error", "Invalid file type. Please upload an image.")); | ||
| } | ||
|
|
||
| long fileSize = file.getSize(); | ||
| if (fileSize > maxSizeBytes) { | ||
| log.warn("Validation failed: Image size {} exceeds limit {} for file '{}'", | ||
| fileSize, maxSizeBytes, file.getOriginalFilename()); | ||
| log.warn("Validation failed: Image size {} exceeds limit {} for file '{}'", | ||
| fileSize, maxSizeBytes, sanitizeForLog(file.getOriginalFilename())); | ||
| return ResponseEntity.badRequest() | ||
| .body(Map.of("error", "File size exceeds maximum limit of " + | ||
| .body(Map.of("error", "File size exceeds maximum limit of " + | ||
| (maxSizeBytes / (1024 * 1024)) + "MB: " + file.getOriginalFilename())); | ||
| } | ||
|
|
| public String getFilename() { | ||
| return file.getOriginalFilename(); | ||
| public JsonNode analyzeCallRecord(MultipartFile file) { | ||
| log.info("Forwarding call record analysis to ML service: {}", file.getOriginalFilename()); |
Check failure
Code scanning / CodeQL
Log Injection High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 months ago
In general, to fix log injection issues, any user-controlled value must be sanitized or validated before being passed to a logger. For filenames, a reasonable approach is to strip carriage returns and newlines (and optionally other control characters), and possibly replace them with safe characters like _ or space. This preserves useful information for debugging while preventing attackers from injecting additional log lines or confusing log structure.
For this specific class, the best minimal fix is to sanitize the filename once per logging call. Since we must not change external behavior beyond logging, we should only transform the value used in log messages, not the value used elsewhere (e.g., for sending to the Python service). We can do this inline by calling replace on the string to remove \r and \n. No new imports are required because String.replace is part of the standard library. Concretely:
- On line 48, wrap
file.getOriginalFilename()with.replace("\r", "_").replace("\n", "_")(or similar). - On line 63, do the same wrapping for the filename inside the error log.
All other behavior remains unchanged.
| @@ -45,7 +45,8 @@ | ||
| * @return JSON response with analysis results | ||
| */ | ||
| public JsonNode analyzeCallRecord(MultipartFile file) { | ||
| log.info("Forwarding call record analysis to ML service: {}", file.getOriginalFilename()); | ||
| log.info("Forwarding call record analysis to ML service: {}", file.getOriginalFilename() | ||
| .replace("\r", "_").replace("\n", "_")); | ||
|
|
||
| String url = callAnalysisServiceUrl + "/analyze"; | ||
|
|
||
| @@ -61,7 +62,7 @@ | ||
| fileBytes = file.getBytes(); | ||
| } catch (IOException e) { | ||
| log.error("Failed to read file bytes from uploaded file '{}': {}", | ||
| file.getOriginalFilename(), e.getMessage()); | ||
| file.getOriginalFilename().replace("\r", "_").replace("\n", "_"), e.getMessage()); | ||
| throw new RuntimeException("Failed to read uploaded file contents", e); | ||
| } | ||
|
|
| fileBytes = file.getBytes(); | ||
| } catch (IOException e) { | ||
| log.error("Failed to read file bytes from uploaded file '{}': {}", | ||
| file.getOriginalFilename(), e.getMessage()); |
Check failure
Code scanning / CodeQL
Log Injection High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 months ago
To fix the problem, all user-controlled values written into logs should be sanitized to remove control characters (especially \r and \n) or otherwise constrained to a safe character set before logging. This preserves diagnostic value while preventing attackers from forging additional log lines.
In this specific code, the problematic value is file.getOriginalFilename() in the log.error call within analyzeCallRecord. The best low-impact fix is to introduce a small private helper method in CallAnalysisService that sanitizes strings for logging by stripping carriage returns and newlines (and optionally other control characters) and then use this helper whenever logging user-controlled values. We will:
- Add a
private String sanitizeForLog(String value)method near the bottom (or any suitable place) ofCallAnalysisServicethat:- Returns a fallback like
"<null>"if the value is null. - Replaces
\rand\nwith a space or removes them.
- Returns a fallback like
- Update the
log.errorcall on line 63–64 to passsanitizeForLog(file.getOriginalFilename())instead of the raw filename.
This change is confined to src/main/java/com/crimeLink/analyzer/service/CallAnalysisService.java, adds no external dependencies, and does not alter the external behavior beyond how filenames appear in logs.
| @@ -60,8 +60,8 @@ | ||
| try { | ||
| fileBytes = file.getBytes(); | ||
| } catch (IOException e) { | ||
| log.error("Failed to read file bytes from uploaded file '{}': {}", | ||
| file.getOriginalFilename(), e.getMessage()); | ||
| log.error("Failed to read file bytes from uploaded file '{}': {}", | ||
| sanitizeForLog(file.getOriginalFilename()), e.getMessage()); | ||
| throw new RuntimeException("Failed to read uploaded file contents", e); | ||
| } | ||
|
|
||
| @@ -94,6 +94,21 @@ | ||
| } | ||
|
|
||
| /** | ||
| * Sanitize potentially user-controlled input before logging to prevent log injection. | ||
| * Removes carriage returns and newlines, which could otherwise break log lines. | ||
| * | ||
| * @param value the original value to sanitize | ||
| * @return a sanitized representation safe for logging | ||
| */ | ||
| private String sanitizeForLog(String value) { | ||
| if (value == null) { | ||
| return "<null>"; | ||
| } | ||
| // Remove CR and LF characters to avoid log forging via newlines. | ||
| return value.replace('\r', ' ').replace('\n', ' '); | ||
| } | ||
|
|
||
| /** | ||
| * Analyze multiple call record PDFs in batch. | ||
| * | ||
| * @param files List of PDF files |
There was a problem hiding this comment.
Pull request overview
This PR integrates Python ML microservices for Call Analysis and Facial Recognition into the Spring Boot application, implementing a hybrid monolith + microservices architecture. The Spring Boot backend acts as an API gateway, handling authentication, authorization, and request validation before forwarding requests to Python FastAPI services for ML processing.
Changes:
- Added new REST controllers and services to proxy requests to Python ML microservices (Call Analysis and Facial Recognition)
- Refactored CallAnalysisService from DTO-based to JsonNode-based responses for flexibility with ML service responses
- Removed local database persistence for call analysis (deleted CallAnalysisRepository, CallAnalysisRecord entity, and CallAnalysisResultDTO)
- Added comprehensive database schema for facial recognition features with audit logging
- Configured RestTemplate for ML service communication with appropriate timeouts
- Updated security configuration to restrict ML endpoints to Investigator role
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 18 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/resources/application.properties | Added file upload configuration (10MB max file size, 50MB max request size) |
| src/main/java/com/crimeLink/analyzer/config/RestTemplateConfig.java | New configuration bean for RestTemplate with 10s connect and 30s read timeouts for ML services |
| src/main/java/com/crimeLink/analyzer/config/SecurityConfig.java | Added security rules for ML endpoints: public health checks, Investigator-only access to analysis endpoints |
| src/main/java/com/crimeLink/analyzer/service/FacialRecognitionService.java | New service to proxy facial recognition requests to Python ML service with validation and error handling |
| src/main/java/com/crimeLink/analyzer/service/CallAnalysisService.java | Refactored to proxy requests to Python service; removed local database persistence |
| src/main/java/com/crimeLink/analyzer/controller/FacialRecognitionController.java | New REST controller for facial recognition endpoints with input validation and authentication |
| src/main/java/com/crimeLink/analyzer/controller/CallAnalysisController.java | Refactored to proxy pattern; simplified to forward requests to ML service |
| src/main/java/com/crimeLink/analyzer/repository/CallAnalysisRepository.java | Deleted - persistence moved to Python microservice |
| src/main/java/com/crimeLink/analyzer/entity/CallAnalysisRecord.java | Deleted - no longer storing call analysis records locally |
| src/main/java/com/crimeLink/analyzer/dto/CallAnalysisResultDTO.java | Deleted - using JsonNode for flexible response handling |
| database/facial_recognition_tables.sql | Comprehensive schema for criminals, suspect photos, and facial recognition audit logs |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public String getFilename() { | ||
| return file.getOriginalFilename(); | ||
| public JsonNode analyzeCallRecord(MultipartFile file) { | ||
| log.info("Forwarding call record analysis to ML service: {}", file.getOriginalFilename()); |
There was a problem hiding this comment.
The filename is logged without sanitization on line 48. While filenames are typically less risky than user-provided text, they can still contain newline characters or control characters that could be exploited for log injection attacks. For consistency with the security measures taken elsewhere (e.g., in analyzeBatch on line 120), consider using the sanitizeForLog method here.
| log.info("Forwarding call record analysis to ML service: {}", file.getOriginalFilename()); | |
| log.info("Forwarding call record analysis to ML service: {}", sanitizeForLog(file.getOriginalFilename())); |
| error.put("error", "No file provided"); | ||
| return ResponseEntity.badRequest().body(error); | ||
| } | ||
| log.info("Call record analysis requested: {}", sanitizeForLog(file.getOriginalFilename())); |
There was a problem hiding this comment.
The filename is logged without sanitization on line 62. For consistency with the security measures taken in CallAnalysisService (which sanitizes filenames in logs), consider using the sanitizeForLog method here to prevent potential log injection attacks.
| private String sanitizeForLog(String value) { | ||
| if (value == null) { | ||
| return null; | ||
| } | ||
| // Replace CR and LF to prevent multi-line log injection | ||
| return value.replace('\r', ' ').replace('\n', ' '); | ||
| } |
There was a problem hiding this comment.
The sanitizeForLog method is duplicated across three classes (CallAnalysisController, FacialRecognitionController, and CallAnalysisService). This is duplicated logic that should be extracted to a common utility class to improve maintainability. Consider creating a LogSanitizer utility class or adding this method to an existing utility class that can be shared across all components.
| private String sanitizeForLog(String value) { | ||
| if (value == null) { | ||
| return null; | ||
| } | ||
| // Replace CR and LF to prevent multi-line log injection | ||
| return value.replace('\r', ' ').replace('\n', ' '); | ||
| } |
There was a problem hiding this comment.
The sanitizeForLog method is duplicated across CallAnalysisService, CallAnalysisController, and FacialRecognitionController. This duplicated logic should be extracted to a common utility class to improve maintainability and consistency.
| throw new RuntimeException("Call analysis service unavailable: " + e.getMessage(), e); | ||
| } catch (IOException e) { | ||
| log.error("Failed to process response from ML service: {}", e.getMessage()); | ||
| throw new RuntimeException("Failed to process ML service response: " + e.getMessage(), e); |
There was a problem hiding this comment.
The error messages directly include exception messages from RestClientException or IOException, which could expose internal implementation details to clients. This same pattern exists throughout CallAnalysisService. Consider providing more generic error messages or sanitizing exception messages.
| throw new RuntimeException("Call analysis service unavailable: " + e.getMessage(), e); | |
| } catch (IOException e) { | |
| log.error("Failed to process response from ML service: {}", e.getMessage()); | |
| throw new RuntimeException("Failed to process ML service response: " + e.getMessage(), e); | |
| throw new RuntimeException("Call analysis service unavailable", e); | |
| } catch (IOException e) { | |
| log.error("Failed to process response from ML service: {}", e.getMessage()); | |
| throw new RuntimeException("Failed to process ML service response", e); |
| log.error("Failed to read file bytes from uploaded file '{}': {}", | ||
| file.getOriginalFilename(), e.getMessage()); |
There was a problem hiding this comment.
The filename is logged without sanitization on line 64. For consistency with security measures implemented elsewhere in the codebase (e.g., in analyzeBatch on line 120), consider using the sanitizeForLog method here to prevent potential log injection attacks.
| } catch (RestClientException e) { | ||
| log.error("Failed to register criminal: {}", e.getMessage()); | ||
| throw new RuntimeException("Facial recognition service unavailable: " + e.getMessage(), e); | ||
| } catch (IOException e) { | ||
| log.error("Failed to process response from ML service: {}", e.getMessage()); | ||
| throw new RuntimeException("Failed to process ML service response: " + e.getMessage(), e); |
There was a problem hiding this comment.
The error message includes the raw exception message from RestClientException or IOException, which could expose internal implementation details (e.g., internal service URLs, network information) to the client. This pattern is repeated throughout the service. Consider providing more generic error messages or sanitizing exception messages before including them in thrown RuntimeExceptions.
| .body(Map.of("error", "Invalid file type: " + safeFilename + | ||
| ". Only PDF files are allowed.")); |
There was a problem hiding this comment.
The error message includes the unsanitized filename in the response body on line 163. While the log message sanitizes it, the client receives the potentially malicious filename. Consider sanitizing the filename before including it in error responses to prevent any potential client-side issues.
| private String sanitizeForLog(String value) { | ||
| if (value == null) { | ||
| return null; | ||
| } | ||
| // Replace CR and LF to prevent multi-line log injection | ||
| return value.replace('\r', ' ').replace('\n', ' '); | ||
| } |
There was a problem hiding this comment.
The sanitizeForLog method is duplicated in CallAnalysisService, CallAnalysisController, and here. This duplicated logic should be extracted to a common utility class to improve maintainability and consistency.
| String url = facialRecognitionServiceUrl + "/history"; | ||
| if (limit != null) { | ||
| url += "?limit=" + limit; | ||
| } |
There was a problem hiding this comment.
The URL is constructed by string concatenation with a user-provided limit parameter. If the limit parameter is not properly validated (though it's defined as Integer, it could still be negative or maliciously crafted), this could lead to unexpected behavior. Consider using UriComponentsBuilder for safer URL construction with query parameters, which would properly encode the parameter value.
Merge with Python Services ( Call + Face Recogniion Services)