Merge with Python Services - #25
Conversation
… external ML service
… recognition data, including faces, embeddings, and associated metadata.
…ecognition improvements
| error.put("error", "No file provided"); | ||
| return ResponseEntity.badRequest().body(error); | ||
| } | ||
| log.info("Call record analysis requested: {}", 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 you should sanitize or validate any user-controlled values before logging them. The minimum is to neutralize characters that can break log structure (particularly \r and \n) or other control characters. Alternatively, you can validate against a whitelist of allowed characters and replace anything else.
For this specific case, the best minimal fix without changing functionality is to introduce a small helper that sanitizes the filename by replacing carriage returns and newlines with a safe character (such as a space or underscore) before logging. We keep the original MultipartFile unchanged and only use the sanitized string in the log message. That means modifying CallAnalysisController.analyzeCallRecord to compute a sanitized filename and log that, and adding a private helper method (in the same class) to perform the sanitization. No new imports are required; we can use plain String operations.
Concretely:
- In
analyzeCallRecord, replacefile.getOriginalFilename()in thelog.infocall with a local variable likeString safeFilename = sanitizeForLog(file.getOriginalFilename());and logsafeFilename. - Add a private method
sanitizeForLog(String value)near the bottom of the class that returnsnullif input isnull, otherwise replaces\rand\nwith a space (or strips them).
| @@ -44,7 +44,8 @@ | ||
| @RequestParam("file") MultipartFile file) { | ||
|
|
||
| try { | ||
| log.info("Call record analysis requested: {}", file.getOriginalFilename()); | ||
| String safeFilename = sanitizeForLog(file.getOriginalFilename()); | ||
| log.info("Call record analysis requested: {}", safeFilename); | ||
|
|
||
| // Validate file | ||
| ResponseEntity<?> validationError = validatePdfFile(file, 10 * 1024 * 1024); // 10MB | ||
| @@ -157,4 +158,18 @@ | ||
|
|
||
| return null; // Validation passed | ||
| } | ||
| /** | ||
| * Sanitize user-controlled strings before logging to prevent log injection. | ||
| * | ||
| * @param value original string value | ||
| * @return sanitized value with line breaks removed or null if input is null | ||
| */ | ||
| private String sanitizeForLog(String value) { | ||
| if (value == null) { | ||
| return null; | ||
| } | ||
| // Replace carriage returns and newlines to avoid log forging | ||
| return value.replace('\r', ' ').replace('\n', ' '); | ||
| } | ||
|
|
||
| } |
| String contentType = file.getContentType(); | ||
| if (contentType == null || !contentType.equals("application/pdf")) { | ||
| 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
To fix the problem, user-controlled input should be sanitized before being passed to the logger. For plain-text logging, this typically means removing or replacing line breaks and other control characters so that an attacker cannot inject additional log lines or manipulate log structure.
The best, minimal-impact fix here is to introduce a local sanitized variable derived from contentType and log that instead. For example, replace any \r and \n characters with a space. This preserves the informational value of the log while preventing new lines from being introduced. We do not need new imports; we can implement sanitization with standard String.replace calls. We should leave the rest of the behavior (validation logic, response messages) unchanged.
Concretely, in validatePdfFile in CallAnalysisController, right before the log.warn call that currently logs contentType, we will create a String safeContentType = contentType == null ? "null" : contentType.replace('\n', ' ').replace('\r', ' '); and then use safeContentType in the log message instead of contentType. This keeps null-handling explicit and avoids introducing null into the logger. No other lines need to change.
| @@ -139,10 +139,13 @@ | ||
|
|
||
| String contentType = file.getContentType(); | ||
| if (contentType == null || !contentType.equals("application/pdf")) { | ||
| log.warn("Validation failed: Invalid content type '{}' for file '{}'", | ||
| contentType, file.getOriginalFilename()); | ||
| String safeContentType = contentType == null | ||
| ? "null" | ||
| : contentType.replace('\n', ' ').replace('\r', ' '); | ||
| log.warn("Validation failed: Invalid content type '{}' for file '{}'", | ||
| safeContentType, file.getOriginalFilename()); | ||
| return ResponseEntity.badRequest() | ||
| .body(Map.of("error", "Invalid file type: " + file.getOriginalFilename() + | ||
| .body(Map.of("error", "Invalid file type: " + file.getOriginalFilename() + | ||
| ". Only PDF files are allowed.")); | ||
| } | ||
|
|
| String contentType = file.getContentType(); | ||
| if (contentType == null || !contentType.equals("application/pdf")) { | ||
| 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 should be sanitized or validated before being written to logs. For plain-text logs, this typically means removing or replacing newline and other control characters and, optionally, constraining to a safe character set. The key is that an attacker should not be able to inject line breaks or format sequences that make one log entry look like multiple or forged entries.
The best fix here is to derive a sanitized version of the original filename inside validatePdfFile, and then use that sanitized value in both the log.warn calls and the error messages. We can implement a small private helper method in CallAnalysisController that takes a String and returns a cleaned version—for example, replacing \r and \n (and optionally other control characters) with a safe placeholder like _. Then, in validatePdfFile, we call this helper on file.getOriginalFilename() once (handling possible null), store the result in a local variable such as safeFilename, and use safeFilename everywhere instead of the raw getOriginalFilename() in lines 142–146 and 151–155. This preserves existing functionality while ensuring log entries cannot be split or forged via filenames.
Concretely:
- Add a private method
sanitizeForLogging(String input)near the bottom ofCallAnalysisControllerthat:- Returns
nullif input isnull. - Replaces
\rand\nwith a space or underscore. - Optionally removes other ISO control characters via
chars().filter(...).
- Returns
- In
validatePdfFile, after obtainingcontentType, computeString originalFilename = file.getOriginalFilename(); String safeFilename = sanitizeForLogging(originalFilename); - Use
safeFilenamein thelog.warnmessage parameters and in the"error"messages returned in theResponseEntity.
No external dependencies are required; we can implement this with core Java.
| @@ -138,23 +138,47 @@ | ||
| } | ||
|
|
||
| String contentType = file.getContentType(); | ||
| String originalFilename = file.getOriginalFilename(); | ||
| String safeFilename = sanitizeForLogging(originalFilename); | ||
| if (contentType == null || !contentType.equals("application/pdf")) { | ||
| log.warn("Validation failed: Invalid content type '{}' for file '{}'", | ||
| contentType, file.getOriginalFilename()); | ||
| log.warn("Validation failed: Invalid content type '{}' for file '{}'", | ||
| contentType, safeFilename); | ||
| return ResponseEntity.badRequest() | ||
| .body(Map.of("error", "Invalid file type: " + file.getOriginalFilename() + | ||
| .body(Map.of("error", "Invalid file type: " + safeFilename + | ||
| ". Only PDF files are allowed.")); | ||
| } | ||
|
|
||
| long fileSize = file.getSize(); | ||
| if (fileSize > maxSizeBytes) { | ||
| log.warn("Validation failed: File size {} exceeds limit {} for file '{}'", | ||
| fileSize, maxSizeBytes, file.getOriginalFilename()); | ||
| log.warn("Validation failed: File size {} exceeds limit {} for file '{}'", | ||
| fileSize, maxSizeBytes, safeFilename); | ||
| return ResponseEntity.badRequest() | ||
| .body(Map.of("error", "File size exceeds maximum limit of " + | ||
| (maxSizeBytes / (1024 * 1024)) + "MB: " + file.getOriginalFilename())); | ||
| .body(Map.of("error", "File size exceeds maximum limit of " + | ||
| (maxSizeBytes / (1024 * 1024)) + "MB: " + safeFilename)); | ||
| } | ||
|
|
||
| return null; // Validation passed | ||
| } | ||
|
|
||
| /** | ||
| * Sanitize a string for safe inclusion in log messages by removing | ||
| * control characters that could be used for log injection. | ||
| * | ||
| * @param input the original string, possibly null | ||
| * @return a sanitized string with control characters removed, or null if input was null | ||
| */ | ||
| private String sanitizeForLogging(String input) { | ||
| if (input == null) { | ||
| return null; | ||
| } | ||
| String withoutNewlines = input.replace('\r', ' ').replace('\n', ' '); | ||
| StringBuilder sanitized = new StringBuilder(withoutNewlines.length()); | ||
| for (int i = 0; i < withoutNewlines.length(); i++) { | ||
| char ch = withoutNewlines.charAt(i); | ||
| if (!Character.isISOControl(ch)) { | ||
| sanitized.append(ch); | ||
| } | ||
| } | ||
| return sanitized.toString(); | ||
| } | ||
| } |
| log.warn("Validation failed: File 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
In general, to fix log injection issues, any user-provided data included in log messages should be normalized so it cannot break the intended log structure. A straightforward approach for plain-text logs is to strip or replace control characters such as \r and \n with safe alternatives (for example, spaces or an underscore). This keeps the information but prevents an attacker from injecting fake entries or manipulating the layout of logs.
For this specific code, the only tainted value in the flagged statement is file.getOriginalFilename(). We can introduce a small private helper method in CallAnalysisController that takes a String and returns a sanitized version with newline and carriage-return characters replaced by spaces. We will then call this helper when passing the filename to the logger. To avoid changing external behavior, we will leave the error response bodies as they are, since the alert only concerns the log sink and the main injection risk is with multi-line log records. Concretely:
- Add a private method inside
CallAnalysisController, nearvalidatePdfFile, e.g.private String sanitizeForLog(String value), which returnsnullfornullinput and otherwise performsreplace('\n',' ').replace('\r',' '). - Update the
log.warncalls that currently logfile.getOriginalFilename()(lines 142–143 and 151–152) to instead logsanitizeForLog(file.getOriginalFilename()). - No new imports are needed; we only use core
Stringmethods.
This keeps all existing functionality—messages and HTTP responses—intact while ensuring that log messages cannot contain raw newlines from user-controlled filenames.
| @@ -31,6 +31,19 @@ | ||
| @Slf4j | ||
| public class CallAnalysisController { | ||
|
|
||
| /** | ||
| * Sanitize user-controlled strings for safe logging by removing line breaks. | ||
| * | ||
| * @param value the original string, possibly null | ||
| * @return a string safe to include in single-line log entries | ||
| */ | ||
| private String sanitizeForLog(String value) { | ||
| if (value == null) { | ||
| return null; | ||
| } | ||
| return value.replace('\n', ' ').replace('\r', ' '); | ||
| } | ||
|
|
||
| private final CallAnalysisService callAnalysisService; | ||
|
|
||
| /** | ||
| @@ -139,19 +152,19 @@ | ||
|
|
||
| String contentType = file.getContentType(); | ||
| if (contentType == null || !contentType.equals("application/pdf")) { | ||
| 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: " + file.getOriginalFilename() + | ||
| .body(Map.of("error", "Invalid file type: " + file.getOriginalFilename() + | ||
| ". Only PDF files are allowed.")); | ||
| } | ||
|
|
||
| long fileSize = file.getSize(); | ||
| if (fileSize > maxSizeBytes) { | ||
| log.warn("Validation failed: File size {} exceeds limit {} for file '{}'", | ||
| fileSize, maxSizeBytes, file.getOriginalFilename()); | ||
| log.warn("Validation failed: File 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())); | ||
| } | ||
|
|
| @RequestParam(value = "risk_level", required = false, defaultValue = "medium") String riskLevel) { | ||
|
|
||
| try { | ||
| log.info("Criminal registration requested: {} ({})", name, nic); |
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 sanitize or validate user-controlled data before including it in log messages. For plain-text logs, removing or replacing newline, carriage-return, and other control characters is usually sufficient to prevent log forging via line breaks. You can either (a) validate the string against a safe pattern (e.g., alphanumeric plus a small set of allowed punctuation) and reject or replace unsafe values, or (b) normalize it by replacing disallowed characters (especially \r and \n) with safe substitutes (like space or _) right before logging.
The least invasive, functionality-preserving fix here is to sanitize name only for logging while leaving the original value unchanged for business logic. This avoids changing application behavior while still protecting logs. We can do this by introducing a small private helper method in FacialRecognitionController, for example sanitizeForLog(String input), that replaces \r and \n with spaces and trims the result, and then using this sanitized value in the log call. The rest of the flow (validation, service call) can continue using the original name. Concretely, in FacialRecognitionController.java, add a private helper method near the bottom of the class, and change the line log.info("Criminal registration requested: {} ({})", name, nic); to log sanitizeForLog(name) instead of the raw name. No new imports are needed, and existing functionality is preserved.
| @@ -100,7 +100,7 @@ | ||
| @RequestParam(value = "risk_level", required = false, defaultValue = "medium") String riskLevel) { | ||
|
|
||
| try { | ||
| log.info("Criminal registration requested: {} ({})", name, nic); | ||
| log.info("Criminal registration requested: {} ({})", sanitizeForLog(name), nic); | ||
|
|
||
| // Validate required text fields | ||
| ResponseEntity<?> nameValidation = validateRequiredText(name, "name"); |
| 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
In general, to fix log injection, sanitize or validate any user-controlled data before logging it. For plain-text logs, the primary concerns are removing or replacing newline (\n, \r) and other control characters so an attacker cannot break the log line or insert additional fake ones. Optionally, you can also restrict to a safe character set or clearly delimit user input.
For this specific case, the safest minimal change is to introduce a small private helper method in CallAnalysisService that takes a String and returns a sanitized version suitable for logging (e.g., replacing \r and \n with spaces). Then, use this helper when logging file.getOriginalFilename(). This preserves existing functionality (the filename is still visible in logs) while preventing multi-line log entries. No new imports are required; we only add a private method and update the single log statement on line 119–120 to call it.
Concretely:
- Add a private method
sanitizeForLogging(String input)somewhere insideCallAnalysisService(e.g., near the bottom before the class closing brace). This method can returnnullunchanged, otherwise replace\rand\nwith spaces (and optionally any other control characters if you wish, but we’ll keep the change minimal). - Change the
log.errorcall that currently passesfile.getOriginalFilename()to instead passsanitizeForLogging(file.getOriginalFilename()). - No other behavior or APIs are altered.
| @@ -116,8 +116,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 '{}': {}", | ||
| sanitizeForLogging(file.getOriginalFilename()), e.getMessage()); | ||
| throw new RuntimeException("Failed to read uploaded file: " + file.getOriginalFilename(), e); | ||
| } | ||
|
|
||
| @@ -173,4 +173,18 @@ | ||
| .put("error", "Invalid response"); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Sanitize user-controlled strings before logging to prevent log injection. | ||
| * | ||
| * @param input the original string, possibly null | ||
| * @return a version safe for single-line log entries | ||
| */ | ||
| private String sanitizeForLogging(String input) { | ||
| if (input == null) { | ||
| return null; | ||
| } | ||
| // Replace CR and LF with spaces to avoid multi-line log entries | ||
| return input.replace('\r', ' ').replace('\n', ' '); | ||
| } | ||
| } |
| imageBytes = image.getBytes(); | ||
| } catch (IOException e) { | ||
| log.error("Failed to read image bytes from uploaded file '{}': {}", | ||
| image.getOriginalFilename(), e.getMessage()); |
Check failure
Code scanning / CodeQL
Log Injection High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 months ago
In general, log injection is mitigated by sanitizing or validating user-controlled data before writing it to logs. For plain-text logs, the common fix is to strip or replace newline (\n), carriage return (\r), and other control characters so that a single log call cannot visually or structurally appear as multiple log entries. Optionally, very suspicious input can be replaced with a placeholder.
For this specific case, we should sanitize image.getOriginalFilename() before using it in log.error. Since we must not change external behavior beyond logging, the safest fix is to derive a local String safeFilename that replaces \r and \n with spaces (or removes them) and then log that sanitized value. We should also handle the possibility that getOriginalFilename() returns null, to avoid a NullPointerException in the new sanitization code. No new external dependencies are required; simple String.replace operations are enough.
Concretely, in FacialRecognitionService.analyzeImage, in the inner catch (IOException e) where we currently log "Failed to read image bytes from uploaded file '{}': {}" with image.getOriginalFilename(), we will introduce a small sanitization step right before the log.error call:
- Read the original filename into a local variable.
- If it is
null, use a placeholder like"<unknown>". - Replace any
\ror\nwith spaces (or empty strings). - Use this sanitized
safeFilenamein the log message.
This change is localized to the catch (IOException e) block around lines 64–71 and does not alter how the method behaves otherwise.
| @@ -65,8 +65,11 @@ | ||
| try { | ||
| imageBytes = image.getBytes(); | ||
| } catch (IOException e) { | ||
| String originalFilename = image.getOriginalFilename(); | ||
| String safeFilename = originalFilename == null ? "<unknown>" : | ||
| originalFilename.replace('\r', ' ').replace('\n', ' '); | ||
| log.error("Failed to read image bytes from uploaded file '{}': {}", | ||
| image.getOriginalFilename(), e.getMessage()); | ||
| safeFilename, e.getMessage()); | ||
| throw new RuntimeException("Failed to read uploaded image contents", e); | ||
| } | ||
|
|
| */ | ||
| public JsonNode registerCriminal(MultipartFile photo, String criminalId, String name, | ||
| String nic, String riskLevel) { | ||
| log.info("Forwarding criminal registration to ML service: {} ({})", name, nic); |
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 you must sanitize or validate any user-controlled data before including it in log messages. For plain-text logs, it’s usually sufficient to remove or normalize control characters that can alter log structure, especially \r and \n, and sometimes to truncate excessively long input.
The best targeted fix here is to sanitize the name (and, for consistency and safety, the nic) in the registerCriminal method of FacialRecognitionService before logging. We will introduce local variables safeName and safeNic that replace carriage return and newline characters with a safe placeholder (e.g., a space) and then log those sanitized values. This preserves existing functionality (the raw name and nic are still sent to the ML service in the body and not altered) while ensuring the log line cannot be split or forged via newline injection.
Concretely:
- In
src/main/java/com/crimeLink/analyzer/service/FacialRecognitionService.java, insideregisterCriminal, just before the firstlog.infoon line 125, create sanitized versions ofnameandnicby replacing\rand\nwith spaces. - Update the
log.infocall to use the sanitized variables. - No changes are needed in
FacialRecognitionController.javafor this particular issue, since the vulnerable sink is in the service.
No new methods or imports are strictly required; we can use String.replace (or replaceAll) which is already available.
| @@ -122,7 +122,9 @@ | ||
| */ | ||
| public JsonNode registerCriminal(MultipartFile photo, String criminalId, String name, | ||
| String nic, String riskLevel) { | ||
| log.info("Forwarding criminal registration to ML service: {} ({})", name, nic); | ||
| String safeName = name == null ? null : name.replace('\n', ' ').replace('\r', ' '); | ||
| String safeNic = nic == null ? null : nic.replace('\n', ' ').replace('\r', ' '); | ||
| log.info("Forwarding criminal registration to ML service: {} ({})", safeName, safeNic); | ||
|
|
||
| String url = facialRecognitionServiceUrl + "/register"; | ||
|
|
| */ | ||
| public JsonNode registerCriminal(MultipartFile photo, String criminalId, String name, | ||
| String nic, String riskLevel) { | ||
| log.info("Forwarding criminal registration to ML service: {} ({})", name, nic); |
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 you must ensure that any user-controlled data written to logs is sanitized or normalized first. For text logs, the typical mitigation is to strip or replace newline and other control characters so a single log entry cannot be split into multiple lines or inject misleading content. Optionally you can also enforce an allowed character set (e.g., alphanumeric plus a few punctuation characters) for sensitive identifiers like NIC.
The single best minimally invasive fix here is to sanitize nic before including it in the log message in FacialRecognitionService.registerCriminal. We can implement a small private helper method inside FacialRecognitionService that replaces \r and \n with safe spaces (or removes them) and trims the result, and then log the sanitized value instead of the raw nic. This preserves functional behavior for valid NICs (which should not contain control characters) while preventing an attacker from injecting line breaks or similar into the logs. We only need to edit src/main/java/com/crimeLink/analyzer/service/FacialRecognitionService.java: add the helper method and update the log.info call on line 125 to use it. No changes are required to the controller.
Concretely:
- Add a private method in
FacialRecognitionService, e.g.private String sanitizeForLog(String value), that:- Returns
nullif the input isnull. - Replaces
\rand\nwith a space (or empty string) and optionally removes other non-printable control characters.
- Returns
- Change the logging statement in
registerCriminalto:log.info("Forwarding criminal registration to ML service: {} ({})", name, sanitizeForLog(nic));
This keeps the rest of the behavior (including how nic is sent to the ML service) unchanged.
| @@ -39,6 +39,19 @@ | ||
| } | ||
|
|
||
| /** | ||
| * Sanitize a value before logging to prevent log injection. | ||
| * Currently removes newline and carriage return characters. | ||
| */ | ||
| private String sanitizeForLog(String value) { | ||
| if (value == null) { | ||
| return null; | ||
| } | ||
| // Replace CR and LF with spaces to avoid creating new log lines | ||
| return value.replace('\r', ' ') | ||
| .replace('\n', ' '); | ||
| } | ||
|
|
||
| /** | ||
| * Analyze a suspect image for facial recognition matches. | ||
| * Forwards the request to Python ML service and returns the response. | ||
| * | ||
| @@ -122,7 +135,7 @@ | ||
| */ | ||
| public JsonNode registerCriminal(MultipartFile photo, String criminalId, String name, | ||
| String nic, String riskLevel) { | ||
| log.info("Forwarding criminal registration to ML service: {} ({})", name, nic); | ||
| log.info("Forwarding criminal registration to ML service: {} ({})", name, sanitizeForLog(nic)); | ||
|
|
||
| String url = facialRecognitionServiceUrl + "/register"; | ||
|
|
| photoBytes = photo.getBytes(); | ||
| } catch (IOException e) { | ||
| log.error("Failed to read photo bytes from uploaded file '{}': {}", | ||
| photo.getOriginalFilename(), e.getMessage()); |
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, sanitize or validate any user-controlled data before including it in log messages. For plain-text logs, the most important step is to remove or replace newline and other control characters so that a single log call always produces a single physical log entry. Optionally, you can also restrict allowed characters (for example, to a safe subset) or clearly delimit user input.
For this concrete case, the best fix with minimal functional change is to derive a sanitized version of the original filename specifically for logging. The code can keep using the raw photo.getOriginalFilename() when constructing the ByteArrayResource so that application behavior (e.g., how the filename is forwarded to the ML service) is unchanged. Only the logging call should use a cleaned version. A practical approach is to replace carriage return and linefeed characters (and optionally other control characters) with a safe placeholder, such as an underscore or space. We can implement this with a small private helper method inside FacialRecognitionService that takes a String and returns a sanitized version, and then call it in the log.error statement at line 140–141.
Concretely:
- Add a private method in
FacialRecognitionService(within the shown file) likeprivate String sanitizeForLog(String value)that:- Returns
nullif input isnull. - Replaces
\rand\nwith a space or underscore. - Optionally strips other non-printable control characters using a simple regex.
- Returns
- Modify the error log in the
IOExceptioncatch block to usesanitizeForLog(photo.getOriginalFilename())instead of the raw filename. - No new imports are required; the sanitization uses core
Stringmethods and regex.
| @@ -120,6 +120,20 @@ | ||
| * @param riskLevel Risk level (high, medium, low) | ||
| * @return JSON response from ML service | ||
| */ | ||
| /** | ||
| * Sanitize a string for safe logging by removing newline and other control characters. | ||
| */ | ||
| private String sanitizeForLog(String value) { | ||
| if (value == null) { | ||
| return null; | ||
| } | ||
| // Replace CR and LF with a space to prevent log injection via new lines | ||
| String sanitized = value.replace('\r', ' ').replace('\n', ' '); | ||
| // Optionally remove other non-printable control characters | ||
| sanitized = sanitized.replaceAll("\\p{Cntrl}", ""); | ||
| return sanitized; | ||
| } | ||
|
|
||
| public JsonNode registerCriminal(MultipartFile photo, String criminalId, String name, | ||
| String nic, String riskLevel) { | ||
| log.info("Forwarding criminal registration to ML service: {} ({})", name, nic); | ||
| @@ -138,7 +152,7 @@ | ||
| photoBytes = photo.getBytes(); | ||
| } catch (IOException e) { | ||
| log.error("Failed to read photo bytes from uploaded file '{}': {}", | ||
| photo.getOriginalFilename(), e.getMessage()); | ||
| sanitizeForLog(photo.getOriginalFilename()), e.getMessage()); | ||
| throw new RuntimeException("Failed to read uploaded photo contents", e); | ||
| } | ||
|
|
There was a problem hiding this comment.
Pull request overview
This PR integrates Python ML microservices for call analysis and facial recognition with the Spring Boot backend, implementing a hybrid monolith + microservices architecture. The Spring Boot application now acts as an API gateway, handling authentication, authorization, and request routing, while delegating ML inference to Python FastAPI microservices.
Changes:
- Added proxy services and REST controllers to forward requests to Python ML microservices for call analysis and facial recognition
- Removed local database persistence layer (entities, repositories, DTOs) in favor of delegating storage to Python services
- Configured multipart file upload support, RestTemplate HTTP client with timeouts, and security rules for ML endpoints
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 23 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/resources/application.properties | Added multipart file upload configuration (10MB max file size, 50MB max request size) |
| src/main/java/com/crimeLink/analyzer/config/RestTemplateConfig.java | New RestTemplate bean configuration with 10s connect and 30s read timeouts for ML service calls |
| src/main/java/com/crimeLink/analyzer/config/SecurityConfig.java | Added security rules for ML service endpoints (health checks public, analysis requires Investigator role) |
| src/main/java/com/crimeLink/analyzer/service/FacialRecognitionService.java | New proxy service forwarding facial recognition requests to Python ML service |
| src/main/java/com/crimeLink/analyzer/service/CallAnalysisService.java | Refactored to proxy call analysis requests to Python ML service instead of local processing |
| src/main/java/com/crimeLink/analyzer/controller/FacialRecognitionController.java | New REST controller for facial recognition endpoints with validation and error handling |
| src/main/java/com/crimeLink/analyzer/controller/CallAnalysisController.java | Refactored REST controller for call analysis endpoints to forward to ML service |
| src/main/java/com/crimeLink/analyzer/repository/CallAnalysisRepository.java | Deleted - no longer needed as storage is delegated to Python service |
| src/main/java/com/crimeLink/analyzer/entity/CallAnalysisRecord.java | Deleted - entity removed as part of delegating persistence to Python service |
| src/main/java/com/crimeLink/analyzer/dto/CallAnalysisResultDTO.java | Deleted - using generic JsonNode for ML service responses instead |
| database/facial_recognition_tables.sql | New PostgreSQL schema for facial recognition feature with criminal records, photos, and audit logs |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| MultiValueMap<String, Object> body = new LinkedMultiValueMap<>(); | ||
|
|
||
| // Extract file bytes with explicit error handling | ||
| byte[] fileBytes; | ||
| try { | ||
| fileBytes = file.getBytes(); | ||
| } catch (IOException e) { | ||
| log.error("Failed to read file bytes from uploaded file '{}': {}", | ||
| file.getOriginalFilename(), e.getMessage()); | ||
| throw new RuntimeException("Failed to read uploaded file contents", e); | ||
| } | ||
| }); | ||
|
|
||
| HttpEntity<MultiValueMap<String, Object>> requestEntity = new HttpEntity<>(body, headers); | ||
|
|
||
| // Call Python service | ||
| ResponseEntity<Map> response = restTemplate.postForEntity(url, requestEntity, Map.class); | ||
|
|
||
| if (response.getStatusCode() == HttpStatus.OK) { | ||
| Map<String, Object> responseBody = response.getBody(); | ||
| return (String) responseBody.get("analysis_id"); | ||
| } else { | ||
| throw new Exception("Failed to analyze call records: " + response.getStatusCode()); | ||
|
|
||
| body.add("file", new ByteArrayResource(fileBytes) { | ||
| @Override | ||
| public String getFilename() { | ||
| return file.getOriginalFilename(); | ||
| } | ||
| }); | ||
|
|
||
| HttpEntity<MultiValueMap<String, Object>> requestEntity = new HttpEntity<>(body, headers); | ||
|
|
||
| ResponseEntity<String> response = restTemplate.exchange( | ||
| url, | ||
| HttpMethod.POST, | ||
| requestEntity, | ||
| String.class | ||
| ); | ||
|
|
||
| log.info("ML service responded with status: {}", response.getStatusCode()); | ||
| return objectMapper.readTree(response.getBody()); | ||
|
|
||
| } catch (RestClientException e) { | ||
| log.error("Failed to communicate with call analysis service: {}", e.getMessage()); | ||
| 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 handling pattern with try-catch blocks for IOException converting to RuntimeException is duplicated across multiple methods in this service (lines 56-93, 107-151, 162-174). Consider extracting a common helper method like executeRequest(String url, HttpMethod method, HttpEntity<?> request) that handles the RestTemplate call and error handling to reduce duplication and improve maintainability.
| public JsonNode analyzeCallRecord(MultipartFile file) { | ||
| log.info("Forwarding call record analysis to ML service: {}", file.getOriginalFilename()); | ||
|
|
||
| String url = callAnalysisServiceUrl + "/analyze"; |
There was a problem hiding this comment.
The URL construction is vulnerable to double slashes if the configured base URL already ends with a slash. This pattern appears throughout the service (lines 50, 105, 160, 186, 209, 232). Consider extracting a helper method to properly join base URLs with paths, ensuring no double slashes are created.
| try { | ||
| // Build multipart request | ||
| HttpHeaders headers = new HttpHeaders(); | ||
| headers.setContentType(MediaType.MULTIPART_FORM_DATA); | ||
|
|
||
| MultiValueMap<String, Object> body = new LinkedMultiValueMap<>(); | ||
|
|
||
| // Add image file with explicit error handling | ||
| byte[] imageBytes; | ||
| try { | ||
| imageBytes = image.getBytes(); | ||
| } catch (IOException e) { | ||
| log.error("Failed to read image bytes from uploaded file '{}': {}", | ||
| image.getOriginalFilename(), e.getMessage()); | ||
| throw new RuntimeException("Failed to read uploaded image contents", e); | ||
| } | ||
|
|
||
| body.add("image", new ByteArrayResource(imageBytes) { | ||
| @Override | ||
| public String getFilename() { | ||
| return image.getOriginalFilename(); | ||
| } | ||
| }); | ||
|
|
||
| // Add optional parameters | ||
| if (threshold != null) { | ||
| body.add("threshold", threshold.toString()); | ||
| } | ||
| if (userId != null) { | ||
| body.add("user_id", userId); | ||
| } | ||
| if (caseId != null) { | ||
| body.add("case_id", caseId); | ||
| } | ||
|
|
||
| HttpEntity<MultiValueMap<String, Object>> requestEntity = new HttpEntity<>(body, headers); | ||
|
|
||
| log.debug("Sending request to: {}", url); | ||
| ResponseEntity<String> response = restTemplate.exchange( | ||
| url, | ||
| HttpMethod.POST, | ||
| requestEntity, | ||
| String.class | ||
| ); | ||
|
|
||
| log.info("ML service responded with status: {}", response.getStatusCode()); | ||
| return objectMapper.readTree(response.getBody()); | ||
|
|
||
| } catch (RestClientException e) { | ||
| log.error("Failed to communicate with facial recognition service: {}", 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 handling pattern and request building logic is duplicated across multiple methods in this service (lines 56-111, 129-175, 188-197, 214-223). The same pattern is also duplicated in CallAnalysisService. Consider creating a shared base class or utility class with common methods for building multipart requests, executing REST calls, and handling errors to reduce code duplication between the two ML service proxies.
| @RequestParam(value = "risk_level", required = false, defaultValue = "medium") String riskLevel) { | ||
|
|
||
| try { | ||
| log.info("Criminal registration requested: {} ({})", name, nic); | ||
|
|
||
| // Validate required text fields | ||
| ResponseEntity<?> nameValidation = validateRequiredText(name, "name"); | ||
| if (nameValidation != null) { | ||
| return nameValidation; | ||
| } | ||
|
|
||
| ResponseEntity<?> nicValidation = validateRequiredText(nic, "nic"); | ||
| if (nicValidation != null) { | ||
| return nicValidation; | ||
| } | ||
|
|
||
| // Validate photo | ||
| ResponseEntity<?> photoValidation = validateImageFile(photo, 10 * 1024 * 1024); // 10MB | ||
| if (photoValidation != null) { | ||
| return photoValidation; | ||
| } | ||
|
|
||
| // Forward to ML service | ||
| JsonNode result = facialRecognitionService.registerCriminal( | ||
| photo, criminalId, name, nic, riskLevel); |
There was a problem hiding this comment.
The validation checks if name and nic are null or empty after trimming, but doesn't validate the riskLevel parameter. Since riskLevel has a default value of "medium", it will never be null, but a user could provide an invalid value like "invalid" or "extreme" that doesn't match the expected values (low, medium, high). Consider validating the riskLevel against allowed values or documenting that validation is delegated to the Python ML service.
| @PostMapping("/register") | ||
| public ResponseEntity<?> registerCriminal( | ||
| @RequestParam("photo") MultipartFile photo, | ||
| @RequestParam(value = "criminal_id", required = false) String criminalId, |
There was a problem hiding this comment.
The criminal_id parameter is accepted as a String but based on the database schema (database/facial_recognition_tables.sql:23), the criminal_id is defined as SERIAL (INTEGER). If a non-numeric string is provided, it may cause issues when the Python service tries to use it. Consider validating that criminal_id, when provided, can be parsed as an integer, or document that format validation is delegated to the Python service.
| public JsonNode analyzeImage(MultipartFile image, Float threshold, String userId, String caseId) { | ||
| log.info("Forwarding facial recognition request to ML service for user: {}", userId); | ||
|
|
||
| String url = facialRecognitionServiceUrl + "/analyze"; |
There was a problem hiding this comment.
The URL construction is vulnerable to double slashes if the configured base URL already ends with a slash. If python.facial-recognition.url is configured as http://localhost:5002/ (with trailing slash), the resulting URL will be http://localhost:5002//analyze. While this typically still works in HTTP clients, it's not ideal. Consider using a utility method to normalize URL joining or trim trailing slashes from the base URL.
| String nic, String riskLevel) { | ||
| log.info("Forwarding criminal registration to ML service: {} ({})", name, nic); | ||
|
|
||
| String url = facialRecognitionServiceUrl + "/register"; |
There was a problem hiding this comment.
The URL construction is vulnerable to double slashes if the configured base URL already ends with a slash. If python.facial-recognition.url is configured as http://localhost:5002/ (with trailing slash), the resulting URL will be http://localhost:5002//register. While this typically still works in HTTP clients, it's not ideal. Consider using a utility method to normalize URL joining or trim trailing slashes from the base URL.
| String contentType = file.getContentType(); | ||
| if (contentType == null || !contentType.startsWith("image/")) { | ||
| log.warn("Validation failed: Invalid content type '{}' for file '{}'", | ||
| contentType, 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()); | ||
| return ResponseEntity.badRequest() | ||
| .body(Map.of("error", "File size exceeds maximum limit of " + | ||
| (maxSizeBytes / (1024 * 1024)) + "MB: " + file.getOriginalFilename())); |
There was a problem hiding this comment.
The error message includes the user-provided filename which could potentially be used for log injection attacks if the filename contains newline characters or other control characters. While the logging framework may sanitize these, it's safer to sanitize or truncate filenames in error messages. Consider using file.getOriginalFilename().replaceAll("[\\r\\n]", "") or just omitting the filename from user-facing error messages.
| String contentType = file.getContentType(); | |
| if (contentType == null || !contentType.startsWith("image/")) { | |
| log.warn("Validation failed: Invalid content type '{}' for file '{}'", | |
| contentType, 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()); | |
| return ResponseEntity.badRequest() | |
| .body(Map.of("error", "File size exceeds maximum limit of " + | |
| (maxSizeBytes / (1024 * 1024)) + "MB: " + file.getOriginalFilename())); | |
| String originalFilename = file.getOriginalFilename(); | |
| String safeFilename = originalFilename == null | |
| ? "unknown" | |
| : originalFilename.replaceAll("[\\r\\n]", ""); | |
| String contentType = file.getContentType(); | |
| if (contentType == null || !contentType.startsWith("image/")) { | |
| log.warn("Validation failed: Invalid content type '{}' for file '{}'", | |
| contentType, safeFilename); | |
| 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, safeFilename); | |
| return ResponseEntity.badRequest() | |
| .body(Map.of("error", "File size exceeds maximum limit of " | |
| + (maxSizeBytes / (1024 * 1024)) + "MB")); |
| return objectMapper.readTree(response.getBody()); | ||
|
|
There was a problem hiding this comment.
The code calls objectMapper.readTree(response.getBody()) but doesn't check if response.getBody() is null. While unlikely for successful HTTP responses, if the ML service returns an empty response body, this will cause a NullPointerException. Consider adding a null check and throwing a more descriptive error message if the response body is null. This same issue exists on lines 85, 143, 164, 190, and 216.
| return objectMapper.readTree(response.getBody()); | |
| String responseBody = response.getBody(); | |
| if (responseBody == null || responseBody.isBlank()) { | |
| log.error("Call analysis service returned an empty response body for file '{}'", | |
| file.getOriginalFilename()); | |
| throw new RuntimeException("Call analysis service returned an empty response body"); | |
| } | |
| return objectMapper.readTree(responseBody); |
| String url = facialRecognitionServiceUrl + "/history"; | ||
| if (limit != null) { | ||
| url += "?limit=" + limit; |
There was a problem hiding this comment.
The limit parameter is concatenated directly into the URL without proper encoding or validation. While the controller has a default value, if a malicious value is passed (e.g., containing special characters like & or #), it could manipulate the query string. Use UriComponentsBuilder or URL encoding to properly construct URLs with query parameters instead of string concatenation.
Merge with Python Services ( Call + Face Analysing)