[NAE-2310] Elasticsearch fulltext query input sanitation #401
Conversation
- Introduced `ElasticsearchQuerySanitizer` for escaping and removing reserved characters during query sanitization. - Updated relevant request parsing classes to apply sanitization on `fullText` fields. - Added unit tests for `ElasticsearchQuerySanitizer` to ensure robust functionality.
- Ensures proper contextual deserialization based on the type of the target property.
- Added detailed Javadoc for `ElasticsearchQuerySanitizer` methods. - Updated custom deserializers to sanitize `fullText` fields in `TaskSearchRequest` and `CaseSearchRequest` using `ElasticsearchQuerySanitizer`.
tuplle
requested review from
Retoocs,
machacjozef,
mazarijuraj and
renczesstefan
tuplle
added
bugfix
WalkthroughAdds a new ElasticsearchQuerySanitizer and applies it to full-text fields during request construction and deserialization; introduces case/task-specific single-item-as-list deserializers, enhances SingleItemAsListDeserializer helpers for contextual typing and wrapper detection, updates JsonDeserialize annotations, and adds unit tests for the sanitizer. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Jackson as "Jackson\n(JsonDeserializer)"
participant Sanitizer as "ElasticsearchQuerySanitizer"
participant RequestObj as "SearchRequest\n(Case/Task)"
participant ES as "Elasticsearch"
Client->>Jackson: POST JSON (single item or list) with fullText
Jackson->>Jackson: parse JSON -> produce wrapper or list of items
alt item(s) contain fullText
Jackson->>Sanitizer: sanitize(fullText)
Sanitizer-->>Jackson: sanitized fullText
end
Jackson->>RequestObj: instantiate item(s) with sanitized fullText
RequestObj->>ES: execute search using RequestObj.fullText
ES-->>Client: return results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (3)📓 Common learnings📚 Learning: 2025-12-18T19:20:32.658ZApplied to files:
📚 Learning: 2025-12-18T19:20:32.658ZApplied to files:
🧬 Code graph analysis (1)src/main/java/com/netgrif/application/engine/workflow/utils/TaskSearchRequestSingleItemAsListDeserializer.java (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
🔇 Additional comments (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
src/main/java/com/netgrif/application/engine/elastic/service/ElasticsearchQuerySanitizer.java(1 hunks)src/main/java/com/netgrif/application/engine/elastic/web/requestbodies/CaseSearchRequest.java(3 hunks)src/main/java/com/netgrif/application/engine/elastic/web/requestbodies/ElasticTaskSearchRequest.java(3 hunks)src/main/java/com/netgrif/application/engine/elastic/web/requestbodies/singleaslist/SingleCaseSearchRequestAsList.java(1 hunks)src/main/java/com/netgrif/application/engine/workflow/utils/CaseSearchRequestSingleItemAsListDeserializer.java(1 hunks)src/main/java/com/netgrif/application/engine/workflow/utils/TaskSearchRequestSingleItemAsListDeserializer.java(1 hunks)src/main/java/com/netgrif/application/engine/workflow/web/requestbodies/singleaslist/SingleTaskSearchRequestAsList.java(1 hunks)src/test/java/com/netgrif/application/engine/elastic/ElasticsearchQuerySanitizerTest.java(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: machacjozef
Repo: netgrif/application-engine PR: 367
File: application-engine/src/main/resources/application.yaml:24-24
Timestamp: 2025-10-20T11:44:44.907Z
Learning: In the netgrif/application-engine project, the correction of the Elasticsearch task index name from "_taks" to "_task" in application.yaml was approved by maintainer machacjozef, indicating that any data migration concerns for this typo fix are handled separately or not applicable to their deployment scenario.
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/CaseField.java:16-16
Timestamp: 2025-08-19T20:13:40.087Z
Learning: In CaseField.java, fulltextValue is mapped as a keyword field type in Elasticsearch (for exact matches, filtering, aggregations), while the separate caseValue field serves different Elasticsearch query requirements, allowing the system to support multiple query patterns on the same data through different field mappings.
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovy:341-341
Timestamp: 2025-08-19T20:07:15.621Z
Learning: In the Elasticsearch migration, the stringId property was removed from ElasticCase and ElasticTask, with the id property now containing the MongoDB case/task object ID directly. This consolidation eliminates redundancy and simplifies the data model.
📚 Learning: 2025-08-19T20:07:43.748Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/CaseField.java:16-16
Timestamp: 2025-08-19T20:07:43.748Z
Learning: In CaseField.java, the separate caseValue field (List<String>) is intentionally maintained alongside fulltextValue for specific Elasticsearch query requirements, rather than being derived on-the-fly from fulltextValue.
Applied to files:
src/main/java/com/netgrif/application/engine/elastic/web/requestbodies/CaseSearchRequest.java
📚 Learning: 2025-08-19T20:13:40.087Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/CaseField.java:16-16
Timestamp: 2025-08-19T20:13:40.087Z
Learning: In CaseField.java, fulltextValue is mapped as a keyword field type in Elasticsearch (for exact matches, filtering, aggregations), while the separate caseValue field serves different Elasticsearch query requirements, allowing the system to support multiple query patterns on the same data through different field mappings.
Applied to files:
src/main/java/com/netgrif/application/engine/elastic/web/requestbodies/CaseSearchRequest.java
📚 Learning: 2025-08-19T20:07:15.621Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovy:341-341
Timestamp: 2025-08-19T20:07:15.621Z
Learning: In the Elasticsearch migration, the stringId property was removed from ElasticCase and ElasticTask, with the id property now containing the MongoDB case/task object ID directly. This consolidation eliminates redundancy and simplifies the data model.
Applied to files:
src/main/java/com/netgrif/application/engine/elastic/web/requestbodies/ElasticTaskSearchRequest.java
📚 Learning: 2025-10-20T11:44:44.907Z
Learnt from: machacjozef
Repo: netgrif/application-engine PR: 367
File: application-engine/src/main/resources/application.yaml:24-24
Timestamp: 2025-10-20T11:44:44.907Z
Learning: In the netgrif/application-engine project, the correction of the Elasticsearch task index name from "_taks" to "_task" in application.yaml was approved by maintainer machacjozef, indicating that any data migration concerns for this typo fix are handled separately or not applicable to their deployment scenario.
Applied to files:
src/main/java/com/netgrif/application/engine/elastic/web/requestbodies/ElasticTaskSearchRequest.java
🧬 Code graph analysis (2)
src/main/java/com/netgrif/application/engine/elastic/web/requestbodies/singleaslist/SingleCaseSearchRequestAsList.java (2)
src/main/java/com/netgrif/application/engine/workflow/utils/CaseSearchRequestSingleItemAsListDeserializer.java (1)
CaseSearchRequestSingleItemAsListDeserializer(32-88)src/main/java/com/netgrif/application/engine/workflow/web/requestbodies/singleaslist/SingleTaskSearchRequestAsList.java (1)
JsonDeserialize(8-10)
src/main/java/com/netgrif/application/engine/workflow/web/requestbodies/singleaslist/SingleTaskSearchRequestAsList.java (2)
src/main/java/com/netgrif/application/engine/workflow/utils/TaskSearchRequestSingleItemAsListDeserializer.java (1)
TaskSearchRequestSingleItemAsListDeserializer(28-84)src/main/java/com/netgrif/application/engine/elastic/web/requestbodies/singleaslist/SingleCaseSearchRequestAsList.java (1)
JsonDeserialize(8-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: Build
🔇 Additional comments (8)
src/main/java/com/netgrif/application/engine/elastic/web/requestbodies/ElasticTaskSearchRequest.java (2)
23-33: Good catch on the missing stream termination.The addition of
.collect(Collectors.toList())fixes a bug where the stream was not being collected into theuseCaselist. Without this terminal operation, the mapping would not have been executed anduseCasewould remain uninitialized.
47-50: Sanitization applied correctly and consistently.The fullText sanitization matches the approach in
CaseSearchRequest. The logic is consistent and correct.src/main/java/com/netgrif/application/engine/workflow/web/requestbodies/singleaslist/SingleTaskSearchRequestAsList.java (1)
5-10: Deserializer reference updated correctly.The change from
SingleItemAsListDeserializertoTaskSearchRequestSingleItemAsListDeserializerenables fullText sanitization during deserialization. This is consistent with the PR's objective.src/main/java/com/netgrif/application/engine/elastic/web/requestbodies/singleaslist/SingleCaseSearchRequestAsList.java (1)
6-10: Deserializer reference updated correctly.The change from
SingleItemAsListDeserializertoCaseSearchRequestSingleItemAsListDeserializeris consistent with the pattern used inSingleTaskSearchRequestAsList.src/main/java/com/netgrif/application/engine/workflow/utils/TaskSearchRequestSingleItemAsListDeserializer.java (1)
64-72: Verify null safety in sanitization.Similar to
CaseSearchRequestSingleItemAsListDeserializer, this code sanitizesrequest.fullTextwithout an explicit null check. While the sanitizer handles null inputs, consider adding a defensive check or documenting the null-safety contract.src/main/java/com/netgrif/application/engine/workflow/utils/CaseSearchRequestSingleItemAsListDeserializer.java (1)
68-76: No defensive null check needed—sanitize()already handles nulls safely.The
ElasticsearchQuerySanitizer.sanitize()method explicitly checks for null at line 58 (if (query == null || query.isBlank())), making the code null-safe as written. No additional check in the deserializer is required.src/main/java/com/netgrif/application/engine/elastic/service/ElasticsearchQuerySanitizer.java (1)
74-78: Verify if character-by-character escaping is the intended behavior for Elasticsearch query sanitization.The code escapes each character in reserved strings individually (e.g., "AND" → "\A\N\D"). This character-by-character decomposition approach differs from Elasticsearch's standard escaping model, where reserved characters are typically escaped individually within the complete term (e.g., "AND" → "\AND" or handled by quoting/replacement). Verify whether this approach is intentional or if the sanitizer should instead escape each character only once or replace reserved keywords entirely.
src/main/java/com/netgrif/application/engine/elastic/web/requestbodies/CaseSearchRequest.java (1)
78-81: Sanitization applied correctly.The fullText field is properly sanitized using
ElasticsearchQuerySanitizer.sanitize(). The intermediate variableoriginalFullTextis good for debugging purposes. Escaped characters in Elasticsearch query_string queries are treated literally rather than as operators, so sanitization prevents special characters from being interpreted as query syntax operators while still allowing users to search for those literal values.The
queryfield already exists on line 44 and receives unsanitized input, providing users an alternative path for advanced Elasticsearch query syntax when needed.
src/main/java/com/netgrif/application/engine/elastic/service/ElasticsearchQuerySanitizer.java
Show resolved
Hide resolved
src/main/java/com/netgrif/application/engine/elastic/service/ElasticsearchQuerySanitizer.java
Show resolved
Hide resolved
src/main/java/com/netgrif/application/engine/elastic/service/ElasticsearchQuerySanitizer.java
Outdated
Show resolved
Hide resolved
src/main/java/com/netgrif/application/engine/elastic/service/ElasticsearchQuerySanitizer.java
Outdated
Show resolved
Hide resolved
...netgrif/application/engine/workflow/utils/CaseSearchRequestSingleItemAsListDeserializer.java
Outdated
Show resolved
Hide resolved
...netgrif/application/engine/workflow/utils/TaskSearchRequestSingleItemAsListDeserializer.java
Show resolved
Hide resolved
src/test/java/com/netgrif/application/engine/elastic/ElasticsearchQuerySanitizerTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/netgrif/application/engine/elastic/ElasticsearchQuerySanitizerTest.java
Show resolved
Hide resolved
src/main/java/com/netgrif/application/engine/elastic/service/ElasticsearchQuerySanitizer.java
Outdated
Show resolved
Hide resolved
src/main/java/com/netgrif/application/engine/elastic/service/ElasticsearchQuerySanitizer.java
Show resolved
Hide resolved
- Enhanced `ElasticsearchQuerySanitizerTest` with additional unit tests for various edge cases. - Simplified `ElasticsearchQuerySanitizer` implementation for better readability and efficiency.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/main/java/com/netgrif/application/engine/workflow/utils/CaseSearchRequestSingleItemAsListDeserializer.java (1)
44-67: Code duplication with TaskSearchRequestSingleItemAsListDeserializer persists.This class remains structurally identical to TaskSearchRequestSingleItemAsListDeserializer, differing only in type references. While the current implementation is correct and functional, the duplication increases maintenance burden and risk of inconsistencies.
Consider extracting the common sanitization logic into a generic base class or using a parameterized approach as suggested in previous reviews.
src/main/java/com/netgrif/application/engine/workflow/utils/TaskSearchRequestSingleItemAsListDeserializer.java (1)
40-63: Code duplication with CaseSearchRequestSingleItemAsListDeserializer persists.This class is structurally identical to CaseSearchRequestSingleItemAsListDeserializer, differing only in type references. The duplication concern raised in previous reviews remains unaddressed.
While functionally correct, consider refactoring to reduce maintenance burden.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
src/main/java/com/netgrif/application/engine/elastic/service/ElasticsearchQuerySanitizer.java(1 hunks)src/main/java/com/netgrif/application/engine/utils/SingleItemAsListDeserializer.java(3 hunks)src/main/java/com/netgrif/application/engine/workflow/utils/CaseSearchRequestSingleItemAsListDeserializer.java(1 hunks)src/main/java/com/netgrif/application/engine/workflow/utils/TaskSearchRequestSingleItemAsListDeserializer.java(1 hunks)src/test/java/com/netgrif/application/engine/elastic/ElasticsearchQuerySanitizerTest.java(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: machacjozef
Repo: netgrif/application-engine PR: 367
File: application-engine/src/main/resources/application.yaml:24-24
Timestamp: 2025-10-20T11:44:44.907Z
Learning: In the netgrif/application-engine project, the correction of the Elasticsearch task index name from "_taks" to "_task" in application.yaml was approved by maintainer machacjozef, indicating that any data migration concerns for this typo fix are handled separately or not applicable to their deployment scenario.
📚 Learning: 2025-12-18T19:20:32.658Z
Learnt from: Smotanka
Repo: netgrif/application-engine PR: 401
File: src/main/java/com/netgrif/application/engine/elastic/service/ElasticsearchQuerySanitizer.java:57-66
Timestamp: 2025-12-18T19:20:32.658Z
Learning: When performing multiple string replacements in Java files (e.g., sanitization/escaping logic like ElasticsearchQuerySanitizer), prefer using StringUtils.replaceEach() over chaining multiple StringUtils.replace() calls. This avoids cascading replacements where the result of one replacement triggers another replacement and potentially corrupts the final output. Apply this pattern to other similar sanitization code across the codebase as appropriate.
Applied to files:
src/main/java/com/netgrif/application/engine/utils/SingleItemAsListDeserializer.javasrc/main/java/com/netgrif/application/engine/workflow/utils/TaskSearchRequestSingleItemAsListDeserializer.javasrc/main/java/com/netgrif/application/engine/workflow/utils/CaseSearchRequestSingleItemAsListDeserializer.javasrc/main/java/com/netgrif/application/engine/elastic/service/ElasticsearchQuerySanitizer.java
📚 Learning: 2025-12-18T19:20:32.658Z
Learnt from: Smotanka
Repo: netgrif/application-engine PR: 401
File: src/main/java/com/netgrif/application/engine/elastic/service/ElasticsearchQuerySanitizer.java:57-66
Timestamp: 2025-12-18T19:20:32.658Z
Learning: In the netgrif/application-engine project, when performing multiple string replacements for sanitization or escaping, use `StringUtils.replaceEach()` instead of chained `StringUtils.replace()` calls to prevent cascading replacements where the output of one replacement gets replaced again, which can corrupt the result.
Applied to files:
src/test/java/com/netgrif/application/engine/elastic/ElasticsearchQuerySanitizerTest.java
📚 Learning: 2025-10-20T11:44:44.907Z
Learnt from: machacjozef
Repo: netgrif/application-engine PR: 367
File: application-engine/src/main/resources/application.yaml:24-24
Timestamp: 2025-10-20T11:44:44.907Z
Learning: In the netgrif/application-engine project, the correction of the Elasticsearch task index name from "_taks" to "_task" in application.yaml was approved by maintainer machacjozef, indicating that any data migration concerns for this typo fix are handled separately or not applicable to their deployment scenario.
Applied to files:
src/main/java/com/netgrif/application/engine/elastic/service/ElasticsearchQuerySanitizer.java
📚 Learning: 2025-08-19T20:13:40.087Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/CaseField.java:16-16
Timestamp: 2025-08-19T20:13:40.087Z
Learning: In CaseField.java, fulltextValue is mapped as a keyword field type in Elasticsearch (for exact matches, filtering, aggregations), while the separate caseValue field serves different Elasticsearch query requirements, allowing the system to support multiple query patterns on the same data through different field mappings.
Applied to files:
src/main/java/com/netgrif/application/engine/elastic/service/ElasticsearchQuerySanitizer.java
📚 Learning: 2025-08-19T20:07:43.748Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/CaseField.java:16-16
Timestamp: 2025-08-19T20:07:43.748Z
Learning: In CaseField.java, the separate caseValue field (List<String>) is intentionally maintained alongside fulltextValue for specific Elasticsearch query requirements, rather than being derived on-the-fly from fulltextValue.
Applied to files:
src/main/java/com/netgrif/application/engine/elastic/service/ElasticsearchQuerySanitizer.java
🧬 Code graph analysis (2)
src/main/java/com/netgrif/application/engine/workflow/utils/TaskSearchRequestSingleItemAsListDeserializer.java (1)
src/main/java/com/netgrif/application/engine/utils/SingleItemAsListDeserializer.java (1)
SingleItemAsListDeserializer(17-84)
src/main/java/com/netgrif/application/engine/workflow/utils/CaseSearchRequestSingleItemAsListDeserializer.java (1)
src/main/java/com/netgrif/application/engine/utils/SingleItemAsListDeserializer.java (1)
SingleItemAsListDeserializer(17-84)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Test
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
🔇 Additional comments (13)
src/test/java/com/netgrif/application/engine/elastic/ElasticsearchQuerySanitizerTest.java (1)
1-9: LGTM - Test setup simplified.The test class now uses plain JUnit 5 without Spring Boot test context, which is appropriate for testing static utility methods. This addresses the previous feedback about unnecessary test infrastructure.
src/main/java/com/netgrif/application/engine/utils/SingleItemAsListDeserializer.java (2)
12-15: LGTM - Imports support new reflection-based wrapper detection.The new imports (ParameterizedType, Type, Objects) are required for the isWrapperClass utility method added at lines 74-83.
32-43: LGTM - Clean extraction of item class resolution.The new getItemClass method properly extracts the raw class from the contextual JavaType. This refactoring improves clarity and allows subclasses to customize type resolution if needed.
src/main/java/com/netgrif/application/engine/workflow/utils/CaseSearchRequestSingleItemAsListDeserializer.java (2)
1-29: LGTM - Clear documentation and appropriate imports.The class documentation clearly explains its purpose: handling single-item-or-list deserialization for CaseSearchRequest and ensuring fullText sanitization.
31-42: LGTM - Standard contextual deserializer pattern.The constructors and createContextual method follow the correct pattern for Jackson contextual deserializers, properly delegating to the parent class and creating type-specific instances.
src/main/java/com/netgrif/application/engine/workflow/utils/TaskSearchRequestSingleItemAsListDeserializer.java (2)
1-25: LGTM - Clear documentation and structure.The class documentation clearly explains its purpose for handling TaskSearchRequest deserialization and sanitization.
27-38: LGTM - Standard contextual deserializer pattern.The constructors and createContextual method correctly follow the Jackson contextual deserializer pattern.
src/main/java/com/netgrif/application/engine/elastic/service/ElasticsearchQuerySanitizer.java (6)
1-25: LGTM - Appropriate dependencies and clear documentation.The class uses appropriate dependencies (Lombok for logging, Apache Commons Lang for string utilities) and provides clear documentation of its purpose.
27-29: LGTM - Appropriate reserved character list for query_string queries.The reserved characters and keywords correctly cover Elasticsearch query_string syntax elements. Notable behaviors:
- Space escaping: Spaces are escaped to
\, which is necessary to prevent unintended term splitting in query_string queries.- Angle bracket handling:
>and<are replaced with escaped space to prevent range query injection.This aligns with the confirmed usage in query_string queries (ElasticCaseService line 398, ElasticTaskService line 356).
31-44: LGTM - Clean delegation pattern.The single-argument sanitize method appropriately delegates to the full implementation with null exclusions.
46-67: LGTM - Correct sanitization implementation with proper replacement strategy.The sanitization logic correctly:
- Returns null/blank inputs unchanged
- Uses
StringUtils.replaceEach()to avoid cascading replacement issues (per learning)- Applies exclusions when needed
- Logs at trace level (appropriate for detailed debugging)
This implementation properly prevents query injection in Elasticsearch query_string queries.
Based on learnings: Uses StringUtils.replaceEach() to prevent cascading replacements.
69-82: LGTM - Correct escaping logic for Elasticsearch syntax.The method correctly:
- Escapes each character in reserved strings (e.g., "AND" → "\A\N\D")
- Replaces angle brackets with escaped space to prevent range queries
- Returns an immutable map to prevent accidental modification
The inline initialization of arrays at lines 27-28 ensures they cannot be null, so no null-checks are needed here.
84-95: LGTM - Safe exclusion logic with proper validation.The method correctly:
- Returns the full keyword map when no exclusions are provided
- Creates a mutable copy for filtering
- Only removes keywords that actually exist in the reserved map (line 90)
- Returns an immutable view of the filtered result
This prevents issues with invalid exclusion keys while allowing legitimate structured query building.
src/main/java/com/netgrif/application/engine/utils/SingleItemAsListDeserializer.java
Show resolved
Hide resolved
src/test/java/com/netgrif/application/engine/elastic/ElasticsearchQuerySanitizerTest.java
Show resolved
Hide resolved
- Updated `TaskSearchRequestSingleItemAsListDeserializer` to support `SingleElasticTaskSearchRequestAsList`. - Applied `ElasticsearchQuerySanitizer` to sanitize `fullText` in elastic task search requests during deserialization.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/main/java/com/netgrif/application/engine/elastic/web/requestbodies/singleaslist/SingleElasticTaskSearchRequestAsList.java(1 hunks)src/main/java/com/netgrif/application/engine/workflow/utils/TaskSearchRequestSingleItemAsListDeserializer.java(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: machacjozef
Repo: netgrif/application-engine PR: 367
File: application-engine/src/main/resources/application.yaml:24-24
Timestamp: 2025-10-20T11:44:44.907Z
Learning: In the netgrif/application-engine project, the correction of the Elasticsearch task index name from "_taks" to "_task" in application.yaml was approved by maintainer machacjozef, indicating that any data migration concerns for this typo fix are handled separately or not applicable to their deployment scenario.
📚 Learning: 2025-12-18T19:20:32.658Z
Learnt from: Smotanka
Repo: netgrif/application-engine PR: 401
File: src/main/java/com/netgrif/application/engine/elastic/service/ElasticsearchQuerySanitizer.java:57-66
Timestamp: 2025-12-18T19:20:32.658Z
Learning: When performing multiple string replacements in Java files (e.g., sanitization/escaping logic like ElasticsearchQuerySanitizer), prefer using StringUtils.replaceEach() over chaining multiple StringUtils.replace() calls. This avoids cascading replacements where the result of one replacement triggers another replacement and potentially corrupts the final output. Apply this pattern to other similar sanitization code across the codebase as appropriate.
Applied to files:
src/main/java/com/netgrif/application/engine/workflow/utils/TaskSearchRequestSingleItemAsListDeserializer.javasrc/main/java/com/netgrif/application/engine/elastic/web/requestbodies/singleaslist/SingleElasticTaskSearchRequestAsList.java
📚 Learning: 2025-10-20T11:44:44.907Z
Learnt from: machacjozef
Repo: netgrif/application-engine PR: 367
File: application-engine/src/main/resources/application.yaml:24-24
Timestamp: 2025-10-20T11:44:44.907Z
Learning: In the netgrif/application-engine project, the correction of the Elasticsearch task index name from "_taks" to "_task" in application.yaml was approved by maintainer machacjozef, indicating that any data migration concerns for this typo fix are handled separately or not applicable to their deployment scenario.
Applied to files:
src/main/java/com/netgrif/application/engine/elastic/web/requestbodies/singleaslist/SingleElasticTaskSearchRequestAsList.java
📚 Learning: 2025-08-19T20:07:15.621Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovy:341-341
Timestamp: 2025-08-19T20:07:15.621Z
Learning: In the Elasticsearch migration, the stringId property was removed from ElasticCase and ElasticTask, with the id property now containing the MongoDB case/task object ID directly. This consolidation eliminates redundancy and simplifies the data model.
Applied to files:
src/main/java/com/netgrif/application/engine/elastic/web/requestbodies/singleaslist/SingleElasticTaskSearchRequestAsList.java
🧬 Code graph analysis (2)
src/main/java/com/netgrif/application/engine/workflow/utils/TaskSearchRequestSingleItemAsListDeserializer.java (1)
src/main/java/com/netgrif/application/engine/utils/SingleItemAsListDeserializer.java (1)
SingleItemAsListDeserializer(17-84)
src/main/java/com/netgrif/application/engine/elastic/web/requestbodies/singleaslist/SingleElasticTaskSearchRequestAsList.java (1)
src/main/java/com/netgrif/application/engine/workflow/utils/TaskSearchRequestSingleItemAsListDeserializer.java (1)
TaskSearchRequestSingleItemAsListDeserializer(28-73)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: task-list-completed
- GitHub Check: Build
- GitHub Check: task-list-completed
🔇 Additional comments (2)
src/main/java/com/netgrif/application/engine/workflow/utils/TaskSearchRequestSingleItemAsListDeserializer.java (1)
67-68: No null check needed—sanitizer handles null input gracefully.The
ElasticsearchQuerySanitizer.sanitize()method already handles null input with an explicit null check at line 58:if (query == null || query.isBlank()) { return query; }. No additional null checking at the call site is required.Likely an incorrect or invalid review comment.
src/main/java/com/netgrif/application/engine/elastic/web/requestbodies/singleaslist/SingleElasticTaskSearchRequestAsList.java (1)
6-8: Approve the deserializer change.The new
TaskSearchRequestSingleItemAsListDeserializercorrectly enablesfullTextsanitization during deserialization while maintaining full backward compatibility. The deserializer preserves base deserialization functionality and applies sanitization as an additive security layer that is transparent to valid API inputs.
...netgrif/application/engine/workflow/utils/TaskSearchRequestSingleItemAsListDeserializer.java
Show resolved
Hide resolved
...netgrif/application/engine/workflow/utils/TaskSearchRequestSingleItemAsListDeserializer.java
Outdated
Show resolved
Hide resolved
- Added new test cases in `ElasticsearchQuerySanitizerTest` to cover additional scenarios.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/test/java/com/netgrif/application/engine/elastic/ElasticsearchQuerySanitizerTest.java(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: machacjozef
Repo: netgrif/application-engine PR: 367
File: application-engine/src/main/resources/application.yaml:24-24
Timestamp: 2025-10-20T11:44:44.907Z
Learning: In the netgrif/application-engine project, the correction of the Elasticsearch task index name from "_taks" to "_task" in application.yaml was approved by maintainer machacjozef, indicating that any data migration concerns for this typo fix are handled separately or not applicable to their deployment scenario.
📚 Learning: 2025-12-18T19:20:32.658Z
Learnt from: Smotanka
Repo: netgrif/application-engine PR: 401
File: src/main/java/com/netgrif/application/engine/elastic/service/ElasticsearchQuerySanitizer.java:57-66
Timestamp: 2025-12-18T19:20:32.658Z
Learning: In the netgrif/application-engine project, when performing multiple string replacements for sanitization or escaping, use `StringUtils.replaceEach()` instead of chained `StringUtils.replace()` calls to prevent cascading replacements where the output of one replacement gets replaced again, which can corrupt the result.
Applied to files:
src/test/java/com/netgrif/application/engine/elastic/ElasticsearchQuerySanitizerTest.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Test
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
🔇 Additional comments (2)
src/test/java/com/netgrif/application/engine/elastic/ElasticsearchQuerySanitizerTest.java (2)
1-203: Excellent test coverage and structure!The test suite comprehensively covers the sanitization logic with appropriate edge cases, null handling, and exclusion scenarios. The removal of the Spring Boot test context (as addressed in previous feedback) makes these tests lightweight and fast.
94-98: No changes needed—blank query handling is intentional.The sanitizer explicitly preserves blank or whitespace-only queries without modification (line 58-60:
if (query == null || query.isBlank()) { return query; }). This design choice means blank queries bypass sanitization entirely, which is why" "returns unchanged. The space escaping shown inshouldEscapeSpacesapplies only to non-blank queries that proceed through the sanitization logic. The test expectation is correct.
src/test/java/com/netgrif/application/engine/elastic/ElasticsearchQuerySanitizerTest.java
Show resolved
Hide resolved
- Corrected method name typo in `ElasticsearchQuerySanitizerTest`. - Refactored `TaskSearchRequestSingleItemAsListDeserializer` for better readability and initialization clarity.
src/main/java/com/netgrif/application/engine/elastic/service/ElasticsearchQuerySanitizer.java
Show resolved
Hide resolved
src/main/java/com/netgrif/application/engine/elastic/service/ElasticsearchQuerySanitizer.java
Show resolved
Hide resolved
renczesstefan
left a comment
renczesstefan
left a comment
There was a problem hiding this comment.
Please review my comment.
Description
Implements the sanitation of the fullText value for Elasticsearch queries. Search request objects have sanitation implemented in their deserialization.
Implements NAE-2310
Dependencies
No new dependencies were introduced.
Blocking Pull requests
There are no dependencies on other PR
How Has Been This Tested?
Manualy testing with calling request of case and task search.
The sanitation function has a test:
Test Configuration
Checklist:
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.