Added sanitization filter for request sanitization #78
Conversation
WalkthroughThis pull request introduces a new sanitization filter to the Spring Boot application, enhancing its security by sanitizing incoming requests. The filter removes potentially harmful HTML elements and attributes from the request body, parameters, and headers using the OWASP Java HTML Sanitizer library. The changes also include unit tests for the new filter, updates to the Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 9
Files selected for processing (7)
- pom.xml (1 hunks)
- repo-docs/LOCAL_SETUP.md (1 hunks)
- src/main/java/com/salessparrow/api/config/FilterConfig.java (2 hunks)
- src/main/java/com/salessparrow/api/config/InterceptorConfig.java (2 hunks)
- src/main/java/com/salessparrow/api/filter/SanitizationFilter.java (1 hunks)
- src/main/java/com/salessparrow/api/lib/wrappers/SanitizedRequestWrapper.java (1 hunks)
- src/test/java/com/salessparrow/api/unit/filters/SanitizationFilterTest.java (1 hunks)
Files skipped from review due to trivial changes (3)
- pom.xml
- repo-docs/LOCAL_SETUP.md
- src/main/java/com/salessparrow/api/config/InterceptorConfig.java
Additional comments (Suppressed): 2
src/main/java/com/salessparrow/api/config/FilterConfig.java (2)
19-23: The order of the
SameSiteCookieFilterhas been set to 1. This means it will be executed after theSanitizationFilter. If there is a dependency between these two filters, please ensure that this order is correct.32-37: The new
SanitizationFilteris registered here and its order is set to 0, meaning it will be executed before any other filter. Ensure that this is the intended behavior and that it doesn't interfere with the execution of other filters, especially if they depend on each other.
| public class SanitizationFilterTest { | ||
|
|
||
| @Mock | ||
| private HttpServletRequest request; | ||
|
|
||
| @Mock | ||
| private HttpServletResponse response; | ||
|
|
||
| @Mock | ||
| private FilterChain chain; | ||
|
|
||
| private SanitizationFilter filter; | ||
|
|
||
| @BeforeEach | ||
| public void setUp() { | ||
| MockitoAnnotations.initMocks(this); | ||
| filter = new SanitizationFilter(); | ||
| filter.init(null); | ||
| } | ||
|
|
||
| @Test | ||
| public void testSanitizeHtmlRemovesScriptTags() { | ||
| String input = "<script>alert('Hi');</script>"; | ||
| String expected = ""; // Expecting empty string after removing script tags | ||
|
|
||
| String sanitized = filter.sanitizeHtml(input); | ||
| assertEquals(expected, sanitized); | ||
| } | ||
|
|
||
| @Test | ||
| public void testSanitizeHtmlAllowsSafeTags() { | ||
| String input = "<p>Paragraph</p>"; | ||
| String expected = "<p>Paragraph</p>"; // The string should remain unchanged | ||
|
|
||
| String sanitized = filter.sanitizeHtml(input); | ||
| assertEquals(expected, sanitized); | ||
| } | ||
|
|
||
| @Test | ||
| public void testDoFilterSanitizesScriptTags() throws Exception { | ||
| ArgumentCaptor<ServletRequest> argumentCaptor = ArgumentCaptor.forClass(ServletRequest.class); | ||
|
|
||
| String input = "<script>alert('Hi');</script>"; | ||
| StringReader stringReader = new StringReader(input); | ||
| BufferedReader reader = new BufferedReader(stringReader); | ||
|
|
||
| when(request.getReader()).thenReturn(reader); | ||
| filter.doFilter(request, response, chain); | ||
|
|
||
| verify(chain).doFilter(argumentCaptor.capture(), eq(response)); | ||
|
|
||
| ServletRequest capturedRequest = argumentCaptor.getValue(); | ||
| String sanitizedBody = convertInputStreamToString(capturedRequest.getInputStream()); | ||
|
|
||
| assertEquals("", sanitizedBody); | ||
| } | ||
|
|
||
| @Test | ||
| public void testDoFilterAllowsSafeTags() throws Exception { | ||
| ArgumentCaptor<ServletRequest> argumentCaptor = ArgumentCaptor.forClass(ServletRequest.class); | ||
|
|
||
| String input = "<p>Paragraph</p>"; | ||
| StringReader stringReader = new StringReader(input); | ||
| BufferedReader reader = new BufferedReader(stringReader); | ||
|
|
||
| when(request.getReader()).thenReturn(reader); | ||
| filter.doFilter(request, response, chain); | ||
|
|
||
| verify(chain).doFilter(argumentCaptor.capture(), eq(response)); | ||
|
|
||
| ServletRequest capturedRequest = argumentCaptor.getValue(); | ||
| String sanitizedBody = convertInputStreamToString(capturedRequest.getInputStream()); | ||
|
|
||
| assertEquals("<p>Paragraph</p>", sanitizedBody); | ||
| } | ||
|
|
||
| private String convertInputStreamToString(InputStream inputStream) throws Exception { | ||
| try (BufferedReader br = new BufferedReader(new InputStreamReader(inputStream))) { | ||
| StringBuilder sb = new StringBuilder(); | ||
| String line; | ||
| while ((line = br.readLine()) != null) { | ||
| sb.append(line); | ||
| } | ||
| return sb.toString(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The test cases are well written and cover the main functionalities of the SanitizationFilter. However, there is a potential issue with the convertInputStreamToString method. It reads the input stream line by line and appends each line to a string builder without any separator. This could lead to issues if the original content in the input stream had multiple lines, as the newline characters would be lost in the conversion process. Consider adding a newline character when appending each line to the string builder.
while ((line = br.readLine()) != null) {
- sb.append(line);
+ sb.append(line).append("\n");
}
src/main/java/com/salessparrow/api/lib/wrappers/SanitizedRequestWrapper.java
Outdated
Show resolved
Hide resolved
src/main/java/com/salessparrow/api/lib/wrappers/SanitizedRequestWrapper.java
Outdated
Show resolved
Hide resolved
src/main/java/com/salessparrow/api/lib/wrappers/SanitizedRequestWrapper.java
Outdated
Show resolved
Hide resolved
src/main/java/com/salessparrow/api/filter/SanitizationFilter.java
Outdated
Show resolved
Hide resolved
src/main/java/com/salessparrow/api/filter/SanitizationFilter.java
Outdated
Show resolved
Hide resolved
src/main/java/com/salessparrow/api/filter/SanitizationFilter.java
Outdated
Show resolved
Hide resolved
src/main/java/com/salessparrow/api/filter/SanitizationFilter.java
Outdated
Show resolved
Hide resolved
src/main/java/com/salessparrow/api/filter/SanitizationFilter.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Review Status
Actionable comments generated: 6
Files selected for processing (2)
- src/main/java/com/salessparrow/api/filter/SanitizationFilter.java (1 hunks)
- src/test/java/com/salessparrow/api/unit/filters/SanitizationFilterTest.java (1 hunks)
Additional comments (Suppressed): 1
src/main/java/com/salessparrow/api/filter/SanitizationFilter.java (1)
- 86-90: Ensure that the
getRequestBodymethod handles empty or null bodies correctly. If the body is null or empty, there's no need to create aSanitizedRequestWrapper.
src/test/java/com/salessparrow/api/unit/filters/SanitizationFilterTest.java
Outdated
Show resolved
Hide resolved
src/main/java/com/salessparrow/api/filter/SanitizationFilter.java
Outdated
Show resolved
Hide resolved
src/main/java/com/salessparrow/api/filter/SanitizationFilter.java
Outdated
Show resolved
Hide resolved
src/main/java/com/salessparrow/api/filter/SanitizationFilter.java
Outdated
Show resolved
Hide resolved
src/main/java/com/salessparrow/api/filter/SanitizationFilter.java
Outdated
Show resolved
Hide resolved
src/main/java/com/salessparrow/api/filter/SanitizationFilter.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Review Status
Actionable comments generated: 8
Files ignored due to filter (1)
- docker-compose.yml
Files selected for processing (4)
- src/main/java/com/salessparrow/api/config/FilterConfig.java (2 hunks)
- src/main/java/com/salessparrow/api/filter/SanitizationFilter.java (1 hunks)
- src/main/java/com/salessparrow/api/lib/wrappers/SanitizedRequestWrapper.java (1 hunks)
- src/test/java/com/salessparrow/api/unit/filters/SanitizationFilterTest.java (1 hunks)
Files skipped from review due to trivial changes (1)
- src/main/java/com/salessparrow/api/config/FilterConfig.java
Additional comments (Suppressed): 4
src/test/java/com/salessparrow/api/unit/filters/SanitizationFilterTest.java (3)
63-64: Similar to the previous comment, this assertion does not provide any meaningful checks. It would be more beneficial to check if the request body has been sanitized correctly.
71-72: Again, the assertion here does not provide any meaningful checks. It would be more beneficial to check if the request parameters have been sanitized correctly.
79-80: Once again, the assertion here does not provide any meaningful checks. It would be more beneficial to check if the request headers have been sanitized correctly.
src/main/java/com/salessparrow/api/lib/wrappers/SanitizedRequestWrapper.java (1)
- 70-72: The
setReadListenermethod is not implemented and throws anUnsupportedOperationException. This could cause issues if a ReadListener is required for non-blocking read operations. If you don't plan to support this feature, it would be better to document this limitation clearly.
src/test/java/com/salessparrow/api/unit/filters/SanitizationFilterTest.java
Outdated
Show resolved
Hide resolved
src/main/java/com/salessparrow/api/filter/SanitizationFilter.java
Outdated
Show resolved
Hide resolved
src/main/java/com/salessparrow/api/filter/SanitizationFilter.java
Outdated
Show resolved
Hide resolved
src/main/java/com/salessparrow/api/filter/SanitizationFilter.java
Outdated
Show resolved
Hide resolved
src/main/java/com/salessparrow/api/lib/wrappers/SanitizedRequestWrapper.java
Outdated
Show resolved
Hide resolved
src/main/java/com/salessparrow/api/lib/wrappers/SanitizedRequestWrapper.java
Show resolved
Hide resolved
src/main/java/com/salessparrow/api/lib/wrappers/SanitizedRequestWrapper.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Review Status
Actionable comments generated: 7
Files selected for processing (3)
- src/main/java/com/salessparrow/api/filter/SanitizationFilter.java (1 hunks)
- src/main/java/com/salessparrow/api/lib/wrappers/SanitizedRequestWrapper.java (1 hunks)
- src/test/java/com/salessparrow/api/unit/filters/SanitizationFilterTest.java (1 hunks)
Files skipped from review due to trivial changes (1)
- src/test/java/com/salessparrow/api/unit/filters/SanitizationFilterTest.java
Additional comments (Suppressed): 2
src/main/java/com/salessparrow/api/lib/wrappers/SanitizedRequestWrapper.java (2)
84-88: The
setParametermethod allows setting multiple values for a single parameter key. However, thegetParametermethod only returns the first value of the list. If the intention is to support multiple values per key, consider implementing agetParametersmethod that returns all values for a given key.157-161: Similar to the
setParametermethod, thesetHeadermethod allows setting multiple values for a single header key. However, thegetHeadermethod only returns the first value of the list. If the intention is to support multiple values per key, consider implementing agetHeadersmethod that returns all values for a given key.
src/main/java/com/salessparrow/api/filter/SanitizationFilter.java
Outdated
Show resolved
Hide resolved
src/main/java/com/salessparrow/api/filter/SanitizationFilter.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Review Status
Actionable comments generated: 10
Files selected for processing (7)
- pom.xml (1 hunks)
- repo-docs/LOCAL_SETUP.md (1 hunks)
- src/main/java/com/salessparrow/api/config/FilterConfig.java (2 hunks)
- src/main/java/com/salessparrow/api/config/InterceptorConfig.java (1 hunks)
- src/main/java/com/salessparrow/api/filter/SanitizationFilter.java (1 hunks)
- src/main/java/com/salessparrow/api/lib/wrappers/SanitizedRequestWrapper.java (1 hunks)
- src/test/java/com/salessparrow/api/unit/filters/SanitizationFilterTest.java (1 hunks)
Files skipped from review due to trivial changes (3)
- pom.xml
- repo-docs/LOCAL_SETUP.md
- src/main/java/com/salessparrow/api/config/InterceptorConfig.java
Additional comments (Suppressed): 5
src/main/java/com/salessparrow/api/config/FilterConfig.java (3)
8-8: The import statement for
SanitizationFilterhas been added correctly.19-23: The order of the
SameSiteCookieFilterhas been set to 1. This means it will execute after theSanitizationFilter. Ensure that this is the intended behavior and there are no dependencies between these two filters that could cause issues.31-36: The
SanitizationFilterhas been registered correctly with an order of 0, meaning it will execute before other filters. The URL pattern "/*" indicates that it will apply to all incoming requests.src/main/java/com/salessparrow/api/filter/SanitizationFilter.java (2)
60-64: As pointed out in the comment chains, reading the entire request body into a String can lead to OutOfMemoryError for large requests. Consider using a streaming approach to sanitize the request body.
125-128: The
sanitizeHtmlmethod correctly uses the OWASP Java HTML Sanitizer library to sanitize potentially harmful HTML elements from the input string.
src/main/java/com/salessparrow/api/lib/wrappers/SanitizedRequestWrapper.java
Outdated
Show resolved
Hide resolved
src/test/java/com/salessparrow/api/unit/filters/SanitizationFilterTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/salessparrow/api/unit/filters/SanitizationFilterTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/salessparrow/api/unit/filters/SanitizationFilterTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/salessparrow/api/unit/filters/SanitizationFilterTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/salessparrow/api/unit/filters/SanitizationFilterTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/salessparrow/api/unit/filters/SanitizationFilterTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Review Status
Actionable comments generated: 5
Files selected for processing (1)
- src/test/java/com/salessparrow/api/unit/filters/SanitizationFilterTest.java (1 hunks)
Additional comments (Suppressed): 1
src/test/java/com/salessparrow/api/unit/filters/SanitizationFilterTest.java (1)
- 140-146: In the
testNonHttpRequesttest, you're expecting aServletExceptionto be thrown when a non-HTTP request is passed. This is a good test case to ensure that the filter works correctly with HTTP requests only.
src/test/java/com/salessparrow/api/unit/filters/SanitizationFilterTest.java
Show resolved
Hide resolved
src/test/java/com/salessparrow/api/unit/filters/SanitizationFilterTest.java
Show resolved
Hide resolved
src/test/java/com/salessparrow/api/unit/filters/SanitizationFilterTest.java
Show resolved
Hide resolved
src/test/java/com/salessparrow/api/unit/filters/SanitizationFilterTest.java
Show resolved
Hide resolved
src/test/java/com/salessparrow/api/unit/filters/SanitizationFilterTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Files selected for processing (3)
- src/main/java/com/salessparrow/api/lib/Util.java (1 hunks)
- src/main/java/com/salessparrow/api/lib/crmActions/createAccountNote/CreateSalesforceNote.java (1 hunks)
- src/main/java/com/salessparrow/api/lib/crmActions/createAccountTask/CreateSalesforceAccountTask.java (1 hunks)
Files skipped from review due to trivial changes (2)
- src/main/java/com/salessparrow/api/lib/Util.java
- src/main/java/com/salessparrow/api/lib/crmActions/createAccountNote/CreateSalesforceNote.java
.../java/com/salessparrow/api/lib/crmActions/createAccountTask/CreateSalesforceAccountTask.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Review Status
Actionable comments generated: 10
Files selected for processing (6)
- pom.xml (1 hunks)
- src/main/java/com/salessparrow/api/filter/SanitizationFilter.java (1 hunks)
- src/main/java/com/salessparrow/api/lib/Util.java (2 hunks)
- src/main/java/com/salessparrow/api/lib/crmActions/createAccountNote/CreateSalesforceNote.java (1 hunks)
- src/main/java/com/salessparrow/api/lib/crmActions/createAccountTask/CreateSalesforceAccountTask.java (1 hunks)
- src/main/java/com/salessparrow/api/lib/wrappers/SanitizedRequestWrapper.java (1 hunks)
Files skipped from review due to trivial changes (3)
- pom.xml
- src/main/java/com/salessparrow/api/lib/crmActions/createAccountNote/CreateSalesforceNote.java
- src/main/java/com/salessparrow/api/lib/crmActions/createAccountTask/CreateSalesforceAccountTask.java
Additional comments (Suppressed): 1
src/main/java/com/salessparrow/api/lib/wrappers/SanitizedRequestWrapper.java (1)
- 70-71: The
setReadListenermethod throws anUnsupportedOperationException. This might cause issues if a ReadListener is set on the ServletInputStream. Consider implementing this method properly to support ReadListeners.
src/main/java/com/salessparrow/api/filter/SanitizationFilter.java
Outdated
Show resolved
Hide resolved
src/main/java/com/salessparrow/api/filter/SanitizationFilter.java
Outdated
Show resolved
Hide resolved
src/main/java/com/salessparrow/api/filter/SanitizationFilter.java
Outdated
Show resolved
Hide resolved
src/main/java/com/salessparrow/api/filter/SanitizationFilter.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (1)
- src/main/java/com/salessparrow/api/lib/crmActions/createAccountNote/CreateSalesforceNote.java (1 hunks)
Files skipped from review due to trivial changes (1)
- src/main/java/com/salessparrow/api/lib/crmActions/createAccountNote/CreateSalesforceNote.java
There was a problem hiding this comment.
Review Status
Actionable comments generated: 5
Files selected for processing (2)
- src/main/java/com/salessparrow/api/lib/crmActions/createAccountNote/CreateSalesforceNote.java (2 hunks)
- src/main/java/com/salessparrow/api/lib/wrappers/SanitizedRequestWrapper.java (1 hunks)
Additional comments (Suppressed): 1
src/main/java/com/salessparrow/api/lib/wrappers/SanitizedRequestWrapper.java (1)
- 74-76: This method throws an
UnsupportedOperationException. If this is intentional, please ensure that this does not break any existing functionality where a ReadListener might be set on the ServletInputStream.
| /** | ||
| * Method to get the request parameter map | ||
| * @param key - header key | ||
| * @param value - header value | ||
| * @return void | ||
| */ | ||
| public void setHeader(String key, String value) { | ||
| List<String> headerValues = this.sanitizedHeaders.getOrDefault(key, new ArrayList<String>()); | ||
| headerValues.add(value); | ||
| this.sanitizedHeaders.put(key, headerValues); | ||
| } |
There was a problem hiding this comment.
The setHeader method allows setting multiple values for a single header key. However, the getHeader method only returns the first value of the list. If the intention is to support multiple values per key, consider implementing a getHeaders method that returns all values for a given key.
| public CreateNoteFormatterDto createNote(SalesforceUser user, String accountId, NoteDto note) { | ||
| String salesforceUserId = user.getExternalUserId(); | ||
|
|
||
| logger.info("createNote note text: ", note.getText()); | ||
| String noteTitle = getNoteTitleFromContent(note); | ||
| String encodedNoteContent = base64Helper.base64Encode(note.getText()); | ||
|
|
There was a problem hiding this comment.
The logger.info statement at line 57 is not correctly formatted. It should include a placeholder {} for the variable note.getText(). This will prevent unnecessary string concatenation when logging is disabled and ensure that the note text is properly included in the log message.
- logger.info("createNote note text: ", note.getText());
+ logger.info("createNote note text: {}", note.getText());
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Files selected for processing (1)
- src/main/java/com/salessparrow/api/lib/crmActions/createAccountTask/CreateSalesforceAccountTask.java (2 hunks)
Additional comments (Suppressed): 1
src/main/java/com/salessparrow/api/lib/crmActions/createAccountTask/CreateSalesforceAccountTask.java (1)
- 115-123: The method
getTaskSubjectFromDescriptionnow takes aStringparameter instead ofCreateAccountTaskDto. Ensure that all calls to this method throughout the codebase have been updated to match the new signature.
| public CreateTaskFormatterDto createAccountTask(User User, String accountId, CreateAccountTaskDto task) { | ||
| String salesforceUserId = User.getExternalUserId(); | ||
|
|
||
| String taskSubject = getTaskSubjectFromDescription(task); | ||
| logger.info("createAccountTask task description: ", task.getDescription()); | ||
| Util util = new Util(); | ||
| String unEscapedTaskDescription = util.unEscapeSpecialCharactersForPlainText(task.getDescription()); | ||
| String taskSubject = getTaskSubjectFromDescription(unEscapedTaskDescription); | ||
|
|
||
| logger.info("performing create task in salesforce"); | ||
|
|
||
| Map<String, String> taskBody = new HashMap<String, String>(); | ||
| taskBody.put("Subject", taskSubject); | ||
| taskBody.put("Description", task.getDescription()); | ||
| taskBody.put("Description", unEscapedTaskDescription); | ||
| taskBody.put("OwnerId", task.getCrmOrganizationUserId()); | ||
| taskBody.put("ActivityDate", task.getDueDate()); | ||
| taskBody.put("WhatId", accountId); |
There was a problem hiding this comment.
The Util class is being instantiated directly in the method. This could make testing difficult as you cannot mock this instance for unit tests. Consider injecting Util as a dependency instead.
- Util util = new Util();
+ this.util = util;
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Files selected for processing (1)
- src/main/java/com/salessparrow/api/lib/wrappers/SanitizedRequestWrapper.java (1 hunks)
Additional comments (Suppressed): 1
src/main/java/com/salessparrow/api/lib/wrappers/SanitizedRequestWrapper.java (1)
- 74-76: The
setReadListenermethod throws anUnsupportedOperationException. If this method is called by any part of your application, it will result in a runtime exception. Ensure that this behavior is expected and handled appropriately.
|
|
||
| public SanitizedRequestWrapper(HttpServletRequest request, String sanitizedBody) { | ||
| super(request); | ||
| logger.info("SanitizedRequestWrapper sanitizedBody: ", sanitizedBody); |
There was a problem hiding this comment.
The logger statement is missing the placeholder {} for the variable sanitizedBody. This will result in the log message not displaying the value of sanitizedBody.
- logger.info("SanitizedRequestWrapper sanitizedBody: ", sanitizedBody);
+ logger.info("SanitizedRequestWrapper sanitizedBody: {}", sanitizedBody);
There was a problem hiding this comment.
Review Status
Actionable comments generated: 5
Files selected for processing (3)
- src/main/java/com/salessparrow/api/lib/crmActions/createAccountNote/CreateSalesforceNote.java (2 hunks)
- src/main/java/com/salessparrow/api/lib/crmActions/createAccountTask/CreateSalesforceAccountTask.java (2 hunks)
- src/main/java/com/salessparrow/api/lib/wrappers/SanitizedRequestWrapper.java (1 hunks)
Additional comments (Suppressed): 3
src/main/java/com/salessparrow/api/lib/crmActions/createAccountNote/CreateSalesforceNote.java (1)
- 54-60: Ensure that the logging of sensitive information like note text is compliant with your company's data privacy policy. If the note text contains any sensitive information, it should not be logged.
src/main/java/com/salessparrow/api/lib/crmActions/createAccountTask/CreateSalesforceAccountTask.java (1)
- 115-123: The method
getTaskSubjectFromDescriptionnow takes aStringparameter instead of aCreateAccountTaskDtoobject. Ensure that all calls to this method throughout the codebase have been updated to match the new signature.src/main/java/com/salessparrow/api/lib/wrappers/SanitizedRequestWrapper.java (1)
- 34-40: The
sanitizedParamsandsanitizedHeadersmaps are initialized in the constructor but not populated with sanitized data. If the intention is to sanitize parameters and headers, ensure that these maps are populated with sanitized data.
| public CreateTaskFormatterDto createAccountTask(User User, String accountId, CreateAccountTaskDto task) { | ||
| String salesforceUserId = User.getExternalUserId(); | ||
|
|
||
| String taskSubject = getTaskSubjectFromDescription(task); | ||
| logger.info("createAccountTask task description: {}", task.getDescription()); | ||
| Util util = new Util(); | ||
| String unEscapedTaskDescription = util.unEscapeSpecialCharactersForPlainText(task.getDescription()); | ||
| String taskSubject = getTaskSubjectFromDescription(unEscapedTaskDescription); | ||
|
|
||
| logger.info("performing create task in salesforce"); | ||
|
|
||
| Map<String, String> taskBody = new HashMap<String, String>(); | ||
| taskBody.put("Subject", taskSubject); | ||
| taskBody.put("Description", task.getDescription()); | ||
| taskBody.put("Description", unEscapedTaskDescription); | ||
| taskBody.put("OwnerId", task.getCrmOrganizationUserId()); | ||
| taskBody.put("ActivityDate", task.getDueDate()); | ||
| taskBody.put("WhatId", accountId); |
There was a problem hiding this comment.
The Util class is instantiated directly in the method. This could lead to difficulties when testing this method, as you cannot mock the Util class. Consider injecting the Util class as a dependency instead.
- Util util = new Util();
+ this.util = util;| public void setReadListener(ReadListener readListener) { | ||
| throw new UnsupportedOperationException("Not implemented"); | ||
| } |
There was a problem hiding this comment.
This method throws an UnsupportedOperationException. If this method is not intended to be used, consider documenting this behavior in the method's Javadoc comment to inform users of this class about it.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Files selected for processing (3)
- repo-docs/CHANGELOG.md (2 hunks)
- src/main/java/com/salessparrow/api/lib/ErrorEmailService.java (1 hunks)
- src/main/java/com/salessparrow/api/lib/crmActions/createAccountNote/CreateSalesforceNote.java (2 hunks)
Files skipped from review due to trivial changes (1)
- src/main/java/com/salessparrow/api/lib/ErrorEmailService.java
Additional comments (Suppressed): 2
repo-docs/CHANGELOG.md (2)
- 20-25: The new hunk seems to remove a bug fix entry from the changelog. Please verify if this is intentional or an oversight.
- - Fix: Updated status enum values [#27](https://github.com/TrueSparrowSystems/AI-SalesSparrow-API/pull/27)
- 45-51: The new hunk moves the "Updated status enum values" entry from the Bug Fixes section to a different section. This change seems appropriate if the update was not actually a bug fix but rather a feature or enhancement. However, please ensure that this change accurately reflects the nature of the update.
+ - Updated status enum values [#27](https://github.com/TrueSparrowSystems/AI-SalesSparrow-API/pull/27)
| public CreateNoteFormatterDto createNote(SalesforceUser user, String accountId, NoteDto note) { | ||
| String salesforceUserId = user.getExternalUserId(); | ||
|
|
||
| logger.info("createNote note text: {}", note.getText()); | ||
| String noteTitle = getNoteTitleFromContent(note); | ||
| String encodedNoteContent = base64Helper.base64Encode(note.getText()); | ||
| logger.info("---- encodedNoteContent: {}", encodedNoteContent); | ||
|
|
||
| Map<String, String> createNoteBody = new HashMap<String, String>(); | ||
| createNoteBody.put("Title", noteTitle); |
There was a problem hiding this comment.
The logging of sensitive information such as note text and encodedNoteContent should be avoided due to security reasons. It can lead to potential data leaks.
- logger.info("createNote note text: {}", note.getText());
- String noteTitle = getNoteTitleFromContent(note);
- String encodedNoteContent = base64Helper.base64Encode(note.getText());
- logger.info("---- encodedNoteContent: {}", encodedNoteContent);
+ String noteTitle = getNoteTitleFromContent(note);
+ String encodedNoteContent = base64Helper.base64Encode(note.getText());| private String getNoteTitleFromContent(NoteDto note) { | ||
| if (note.getText().length() < 50) { | ||
| return note.getText(); | ||
| String noteText = note.getText(); | ||
|
|
||
| Util util = new Util(); | ||
| noteText = util.unEscapeSpecialCharactersForPlainText(noteText); | ||
|
|
||
| if (noteText.length() < 50) { | ||
| return noteText; | ||
| } | ||
|
|
||
| return note.getText().substring(0, 50); | ||
| return noteText.substring(0, 50); | ||
| } |
There was a problem hiding this comment.
Creating a new instance of Util class every time the method getNoteTitleFromContent is called can be inefficient. Consider making unEscapeSpecialCharactersForPlainText a static method, so you don't need to create an instance of Util each time.
- Util util = new Util();
- noteText = util.unEscapeSpecialCharactersForPlainText(noteText);
+ noteText = Util.unEscapeSpecialCharactersForPlainText(noteText);
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Files selected for processing (3)
- src/main/java/com/salessparrow/api/SalesSparrowApi.java (1 hunks)
- src/main/java/com/salessparrow/api/lib/Util.java (2 hunks)
- src/main/java/com/salessparrow/api/lib/crmActions/createAccountNote/CreateSalesforceNote.java (2 hunks)
Files skipped from review due to trivial changes (1)
- src/main/java/com/salessparrow/api/SalesSparrowApi.java
Additional comments (Suppressed): 1
src/main/java/com/salessparrow/api/lib/Util.java (1)
- 66-70: The method description and parameters are clear and concise. The changes made in this hunk look good.
| public CreateNoteFormatterDto createNote(SalesforceUser user, String accountId, NoteDto note) { | ||
| String salesforceUserId = user.getExternalUserId(); | ||
|
|
||
| String noteTitle = getNoteTitleFromContent(note); | ||
| String encodedNoteContent = base64Helper.base64Encode(note.getText()); | ||
| Util util = new Util(); | ||
| String noteContent = note.getText(); | ||
| logger.info("createNote noteContent: {}", noteContent); | ||
|
|
||
| String noteTitle = getNoteTitleFromContent(noteContent); | ||
| logger.info("createNote noteTitle: {}", noteTitle); | ||
|
|
||
| noteContent = util.replaceNewLineWithBreak(noteContent); | ||
| logger.info("---- noteContent after replacing \\n: {}", noteContent); | ||
|
|
||
| String encodedNoteContent = base64Helper.base64Encode(noteContent); | ||
| logger.info("---- encodedNoteContent: {}", encodedNoteContent); | ||
|
|
||
| Map<String, String> createNoteBody = new HashMap<String, String>(); | ||
| createNoteBody.put("Title", noteTitle); |
There was a problem hiding this comment.
The Util class is being instantiated directly in the method. This could lead to issues with testability and maintainability. Consider using dependency injection for this class.
- Util util = new Util();
+ @Autowired
+ private Util util;| return null; | ||
| } | ||
|
|
||
| /** | ||
| * Unescape special characters in a string intended for plaintext contexts only. | ||
| * | ||
| * This function is intended for use only with strings that will be rendered as | ||
| * plaintext. It is NOT suitable for HTML strings, XML strings, or any other context | ||
| * where special characters may have syntactic meaning. | ||
| * | ||
| * Current Implementation: - The ampersand ("&") is unescaped to "&" | ||
| * | ||
| * Future versions may include additional un-escaping rules as needed. | ||
| * @param input The original string containing escaped special characters. | ||
| * @return A new string where certain special characters have been unescaped. | ||
| */ | ||
| public String unEscapeSpecialCharactersForPlainText(String input) { | ||
| return input.replace("&", "&"); | ||
| } | ||
|
|
||
| public String replaceNewLineWithBreak(String input) { | ||
| return input.replace("\n", "<br>"); | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
Two new methods unEscapeSpecialCharactersForPlainText and replaceNewLineWithBreak have been added.
The unEscapeSpecialCharactersForPlainText method is well-documented, explaining its purpose, current implementation, and potential future enhancements. However, it currently only handles the unescaping of "&" to "&". If there are other special characters that need to be unescaped for plaintext contexts, they should be handled as well.
The replaceNewLineWithBreak method replaces newline characters with HTML break tags. This could potentially introduce an XSS vulnerability if the input string comes from an untrusted source and is rendered directly in an HTML context without further sanitization. It's recommended to sanitize the output of this method before rendering it in an HTML context.
- public String replaceNewLineWithBreak(String input) {
- return input.replace("\n", "<br>");
- }
+ public String replaceNewLineWithBreak(String input) {
+ return sanitizeHtml(input.replace("\n", "<br>"));
+ }Where sanitizeHtml is a method that applies proper HTML sanitization to prevent XSS attacks.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Files selected for processing (2)
- src/main/java/com/salessparrow/api/config/FilterConfig.java (2 hunks)
- src/main/java/com/salessparrow/api/filter/SanitizationFilter.java (1 hunks)
Additional comments (Suppressed): 3
src/main/java/com/salessparrow/api/config/FilterConfig.java (3)
8-8: Ensure that the
SanitizationFilterclass is correctly implemented and it sanitizes all necessary parts of the HTTP request. Also, verify that it does not remove or alter any data that might be needed further in the processing pipeline.21-22: The order of filters is important. Verify that the
SanitizationFiltershould indeed be executed before theSameSiteCookieFilter. If there are other filters in the application, ensure that this order does not interfere with their operation.- registrationBean.setOrder(1); + registrationBean.setOrder(0); // if it should be the first filter to execute
- 35-35: The order of filters is important. Verify that the
SameSiteCookieFiltershould indeed be executed after theSanitizationFilter. If there are other filters in the application, ensure that this order does not interfere with their operation.- registrationBean.setOrder(2); + registrationBean.setOrder(1); // if it should be the second filter to execute
| try { | ||
| BufferedReader reader = request.getReader(); | ||
| String line; | ||
| // TODO: Consider using a more efficient way to read the request body, such as | ||
| // streaming | ||
| StringBuilder requestBody = new StringBuilder(); | ||
| while ((line = reader.readLine()) != null) { | ||
| requestBody.append(line); | ||
| } | ||
| return requestBody.toString(); | ||
| } | ||
| catch (IOException e) { | ||
| throw new RuntimeException("Error reading request body: ", e.getCause()); | ||
| } | ||
| } |
There was a problem hiding this comment.
The method getRequestBody reads the entire request body into memory which can lead to OutOfMemoryError for large requests. Consider using a more efficient way to read the request body, such as streaming or limiting the size of the request body that can be read.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Files selected for processing (1)
- src/main/java/com/salessparrow/api/filter/SanitizationFilter.java (1 hunks)
Additional comments (Suppressed): 3
src/main/java/com/salessparrow/api/filter/SanitizationFilter.java (3)
73-78: This code modifies the original request parameters by sanitizing them. However, this might break other parts of the application that rely on the original values of these parameters. Ensure that all parts of the application can handle these changes.
85-97: Similar to the above comment, this code modifies the original request headers by sanitizing them. This might break other parts of the application that rely on the original values of these headers. Ensure that all parts of the application can handle these changes.
138-152: This method removes all HTML tags and attributes from the input string. While this is generally a good practice for preventing XSS attacks, there might be cases where some HTML tags are needed. Make sure that this behavior is acceptable in all use cases of this method.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 9
Files selected for processing (3)
- src/main/java/com/salessparrow/api/filter/SanitizationFilter.java (1 hunks)
- src/main/java/com/salessparrow/api/lib/crmActions/createAccountNote/CreateSalesforceNote.java (2 hunks)
- src/main/java/com/salessparrow/api/lib/wrappers/SanitizedRequestWrapper.java (1 hunks)
Additional comments (Suppressed): 1
src/main/java/com/salessparrow/api/lib/wrappers/SanitizedRequestWrapper.java (1)
- 73-75: The
setReadListenermethod is not implemented and throws anUnsupportedOperationException. This could lead to issues if a ReadListener is set on the ServletInputStream. Consider implementing this method or provide a clear documentation stating that this operation is not supported.
| public SanitizedRequestWrapper(HttpServletRequest request, String sanitizedBody) { | ||
| super(request); | ||
| this.sanitizedBody = sanitizedBody; | ||
| this.sanitizedParams = new HashMap<>(); | ||
| this.sanitizedHeaders = new HashMap<>(); | ||
| } |
There was a problem hiding this comment.
The SanitizedRequestWrapper constructor initializes sanitizedParams and sanitizedHeaders as empty HashMaps. If the original request had parameters or headers, these would not be included in the sanitized version. Consider initializing sanitizedParams and sanitizedHeaders with the sanitized versions of the original request's parameters and headers.
- this.sanitizedParams = new HashMap<>();
- this.sanitizedHeaders = new HashMap<>();
+ this.sanitizedParams = sanitizeParameters(request.getParameterMap());
+ this.sanitizedHeaders = sanitizeHeaders(Collections.list(request.getHeaderNames()));| public void setParameter(String key, String value) { | ||
| List<String> values = this.sanitizedParams.getOrDefault(key, new ArrayList<String>()); | ||
| values.add(value); | ||
| this.sanitizedParams.put(key, values); | ||
| } |
There was a problem hiding this comment.
The setParameter method allows setting multiple values for a single parameter key. However, the getParameter method only returns the first value of the list. If the intention is to support multiple values per key, consider implementing a getParameters method that returns all values for a given key.
| public void setHeader(String key, String value) { | ||
| List<String> headerValues = this.sanitizedHeaders.getOrDefault(key, new ArrayList<String>()); | ||
| headerValues.add(value); | ||
| this.sanitizedHeaders.put(key, headerValues); | ||
| } |
There was a problem hiding this comment.
Similar to the setParameter method, the setHeader method allows setting multiple values for a single header key. However, the getHeader method only returns the first value of the list. If the intention is to support multiple values per key, consider implementing a getHeaders method that returns all values for a given key.
| public CreateNoteFormatterDto createNote(SalesforceUser user, String accountId, NoteDto note) { | ||
| String salesforceUserId = user.getExternalUserId(); | ||
|
|
||
| String noteTitle = getNoteTitleFromContent(note); | ||
| String encodedNoteContent = base64Helper.base64Encode(note.getText()); | ||
| Util util = new Util(); | ||
| String noteContent = note.getText(); | ||
| String noteTitle = getNoteTitleFromContent(noteContent); | ||
| noteContent = util.replaceNewLineWithBreak(noteContent); | ||
| String encodedNoteContent = base64Helper.base64Encode(noteContent); | ||
|
|
||
| Map<String, String> createNoteBody = new HashMap<String, String>(); | ||
| createNoteBody.put("Title", noteTitle); |
There was a problem hiding this comment.
The Util class is being instantiated directly in the method. This could lead to issues with testability and maintainability. Consider using dependency injection to manage instances of Util. Also, it's better to sanitize the note title as well.
- Util util = new Util();
+ Util util = this.util; // Assuming 'util' is autowired or passed through constructor
String noteContent = note.getText();
- String noteTitle = getNoteTitleFromContent(noteContent);
+ String noteTitle = util.unEscapeSpecialCharactersForPlainText(getNoteTitleFromContent(noteContent));
noteContent = util.replaceNewLineWithBreak(noteContent);| private String getNoteTitleFromContent(String noteContent) { | ||
| Util util = new Util(); | ||
| noteContent = util.unEscapeSpecialCharactersForPlainText(noteContent); | ||
|
|
||
| if (noteContent.length() < 50) { | ||
| return noteContent; | ||
| } | ||
|
|
||
| return note.getText().substring(0, 50); | ||
| return noteContent.substring(0, 50); | ||
| } |
There was a problem hiding this comment.
Similar to the previous comment, consider using dependency injection for the Util class. Also, it's better to sanitize the note title before returning it.
- Util util = new Util();
+ Util util = this.util; // Assuming 'util' is autowired or passed through constructor
noteContent = util.unEscapeSpecialCharactersForPlainText(noteContent);
if (noteContent.length() < 50) {
- return noteContent;
+ return util.sanitizeHtml(noteContent);
}
- return noteContent.substring(0, 50);
+ return util.sanitizeHtml(noteContent.substring(0, 50));| if (!(servletRequest instanceof HttpServletRequest)) { | ||
| throw new ServletException("Can only process HttpServletRequest"); | ||
| } |
There was a problem hiding this comment.
The filter should be able to handle all types of ServletRequest. Instead of throwing an exception when the request is not an instance of HttpServletRequest, consider skipping the sanitization process and just pass the request to the next filter in the chain.
- if (!(servletRequest instanceof HttpServletRequest)) {
- throw new ServletException("Can only process HttpServletRequest");
- }
+ if (!(servletRequest instanceof HttpServletRequest)) {
+ chain.doFilter(servletRequest, servletResponse);
+ return;
+ }| private SanitizedRequestWrapper sanitizeRequestBody(HttpServletRequest request) { | ||
| String originalBody = getRequestBody(request); | ||
| String sanitizedBody = sanitizeHtml(originalBody); | ||
|
|
||
| return new SanitizedRequestWrapper(request, sanitizedBody); | ||
| } |
There was a problem hiding this comment.
The method sanitizeRequestBody reads the entire request body into memory which can lead to OutOfMemoryError for large requests. Consider using a streaming approach to read and sanitize the request body.
| try { | ||
| BufferedReader reader = request.getReader(); | ||
| String line; | ||
| // TODO: Consider using a more efficient way to read the request body, such as | ||
| // streaming | ||
| StringBuilder requestBody = new StringBuilder(); | ||
| while ((line = reader.readLine()) != null) { | ||
| requestBody.append(line); | ||
| } | ||
| return requestBody.toString(); | ||
| } | ||
| catch (IOException e) { | ||
| throw new RuntimeException("Error reading request body: ", e.getCause()); | ||
| } |
There was a problem hiding this comment.
The BufferedReader is not closed after use which can lead to resource leaks. Always close resources in a finally block or use try-with-resources statement.
- BufferedReader reader = request.getReader();
- String line;
- StringBuilder requestBody = new StringBuilder();
- while ((line = reader.readLine()) != null) {
- requestBody.append(line);
- }
- return requestBody.toString();
+ try (BufferedReader reader = request.getReader()) {
+ String line;
+ StringBuilder requestBody = new StringBuilder();
+ while ((line = reader.readLine()) != null) {
+ requestBody.append(line);
+ }
+ return requestBody.toString();
+ } catch (IOException e) {
+ throw new RuntimeException("Error reading request body: ", e.getCause());
+ }| private String sanitizeHtml(String input) { | ||
| if (input == null || input.isEmpty()) { | ||
| return ""; | ||
| } | ||
|
|
||
| Safelist safelist = Safelist.none(); | ||
|
|
||
| Document.OutputSettings outputSettings = new Document.OutputSettings(); | ||
| outputSettings.prettyPrint(false); | ||
| outputSettings.escapeMode(EscapeMode.base); | ||
|
|
||
| String sanitizedInput = Jsoup.clean(input, "", safelist, outputSettings); | ||
|
|
||
| return sanitizedInput; | ||
| } |
There was a problem hiding this comment.
The method sanitizeHtml creates a new Safelist and Document.OutputSettings object for each call. This can be inefficient for large number of requests. Consider making these objects as class level constants.
Summary by CodeRabbit
SanitizationFilterto sanitize incoming HTTP requests, enhancing the security of the application by removing potentially harmful HTML elements and attributes from request body, parameters, and headers.sendEmailmethod inErrorEmailService.SanitizationFilter.test.secrets.jsonfile.CHANGELOG.mdandRELEASE_PROCESS.mdwith relevant changes.