Dlp #1909
Dlp #1909
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis change introduces a new Data Loss Prevention (DLP) interceptor framework for JSON-based HTTP message processing. It adds a configurable interceptor capable of masking, filtering, and reporting sensitive fields based on JSON paths and risk levels, with support for field classification via a CSV file. Supporting classes for risk analysis, configuration, and actions are included, along with comprehensive tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DLPInterceptor
participant DLPAnalyzer
participant Action[Mask/Filter/Report]
participant RiskReport
Client->>DLPInterceptor: Send HTTP request (JSON body)
DLPInterceptor->>DLPAnalyzer: Analyze JSON for risk (analyze)
DLPAnalyzer->>RiskReport: Record risk levels/categories
DLPAnalyzer-->>DLPInterceptor: Return RiskReport
loop For each configured Action
DLPInterceptor->>Action: Apply action (mask/filter/report) with DLPContext
Action-->>DLPInterceptor: Return transformed JSON
end
DLPInterceptor-->>Client: Forward (possibly modified) request/response
Estimated code review effort4 (60–120 minutes) Poem
✨ Finishing touches🧪 Generate unit tests
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 |
|
This pull request needs "/ok-to-test" from an authorized committer. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (8)
core/src/main/java/com/predic8/membrane/core/DLP.java (5)
63-65: Consider using more specific exception types.Throwing a generic
RuntimeExceptionmakes it harder for callers to handle specific error conditions. Consider using a more specific exception type or creating a custom exception for DLP-related errors.- } catch (IOException e) { - throw new RuntimeException("Error extracting JSON field names", e); + } catch (IOException e) { + throw new IllegalArgumentException("Invalid JSON format in message body", e);
95-98: Consider using structured logging instead of String.format.For better performance and structured logging, consider using parameterized logging.
- logger.info(String.format( - "High: %d, Medium: %d, Low: %d, Unclassified: %d", - high, medium, low, unclassified - )); + logger.info("DLP analysis results - High: {0}, Medium: {1}, Low: {2}, Unclassified: {3}", + new Object[]{high, medium, low, unclassified});
76-108: Consider using enum for risk levels and improve logging.The risk evaluation logic is sound but could benefit from stronger typing and more structured logging.
Consider creating a risk level enum:
public enum RiskLevel { HIGH("high"), MEDIUM("medium"), LOW("low"), UNCLASSIFIED("unclassified"); private final String value; RiskLevel(String value) { this.value = value; } public String getValue() { return value; } public static RiskLevel fromString(String value) { for (RiskLevel level : values()) { if (level.value.equalsIgnoreCase(value)) return level; } return UNCLASSIFIED; } }This would make the switch statement type-safe and eliminate magic strings.
31-31: Consider resource management best practices.While the
JsonParserwill be closed when the underlyingInputStreamis closed, consider using try-with-resources for explicit resource management to make the code more robust and clear.- try { - JsonParser parser = JSON_FACTORY.createParser(new InputStreamReader(msg.getBodyAsStreamDecoded(), msg.getCharset())); + try (JsonParser parser = JSON_FACTORY.createParser(new InputStreamReader(msg.getBodyAsStreamDecoded(), msg.getCharset()))) {
95-98: Enhance logging with field details.Consider adding more detailed logging that includes which specific fields were classified at each risk level for better debugging and monitoring capabilities.
- logger.info(String.format( - "High: %d, Medium: %d, Low: %d, Unclassified: %d", - high, medium, low, unclassified - )); + logger.info(String.format( + "DLP Analysis completed - High: %d, Medium: %d, Low: %d, Unclassified: %d. " + + "High-risk fields: %s", + high, medium, low, unclassified, + matchedFields.entrySet().stream() + .filter(e -> "high".equals(e.getValue().toLowerCase())) + .map(Map.Entry::getKey) + .collect(java.util.stream.Collectors.toList()) + ));core/src/test/java/com/predic8/membrane/core/DLPTest.java (3)
44-45: Remove unused risk dictionary entry.The risk dictionary contains an entry for "nothing" which doesn't exist in the test JSON. This creates unnecessary noise in the test setup.
riskDict.put("user.email", "High"); riskDict.put("user.profile", "High"); riskDict.put("active", "Low"); - riskDict.put("nothing", "Low");
48-63: Add test coverage for edge cases and error conditions.The current test only covers the happy path. Consider adding tests for:
- Invalid JSON input
- Empty JSON objects
- Arrays in JSON structure
- Case sensitivity issues
- Empty risk dictionary
Consider adding these additional test methods:
@Test void analyzeWithInvalidJson() { when(mockMessage.getBodyAsStreamDecoded()) .thenReturn(new ByteArrayInputStream("invalid json".getBytes(StandardCharsets.UTF_8))); DLP dlp = new DLP(riskDict); assertThrows(RuntimeException.class, () -> dlp.analyze(mockMessage)); } @Test void analyzeWithEmptyRiskDictionary() { DLP dlp = new DLP(new HashMap<>()); Map<String, Object> result = dlp.analyze(mockMessage); assertEquals(0, result.get("high_risk")); assertEquals(0, result.get("medium_risk")); assertEquals(0, result.get("low_risk")); assertEquals(6, result.get("unclassified")); // All fields should be unclassified }
45-45: Consider removing unused risk dictionary entry.The risk dictionary entry
"nothing" -> "Low"doesn't correspond to any field in the test JSON and won't affect the test outcome. Consider removing it to avoid confusion.- riskDict.put("nothing", "Low");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/src/main/java/com/predic8/membrane/core/DLP.java(1 hunks)core/src/test/java/com/predic8/membrane/core/DLPTest.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (10)
core/src/test/java/com/predic8/membrane/core/DLPTest.java (6)
54-57: Verify the expected field count calculations.Let me verify the expected counts based on the test JSON structure. The JSON contains these fields:
user(root level)user.emailuser.profileuser.profile.firstnameuser.profile.lastnameactiveWith the risk dictionary mappings:
- High:
user.email,user.profile= 2 ✓- Medium: none = 0 ✓
- Low:
active= 1 ✓- Unclassified:
user,user.profile.firstname,user.profile.lastname= 3 ✓The assertions appear correct.
37-38: I couldn’t locate thegetCharsetsignature with the previous query—let’s search across the codebase to confirm its return type:#!/bin/bash # Find all occurrences of getCharset to inspect its signature rg "getCharset" -n core/src
19-30: LGTM! Good test data structure.The JSON test data covers nested objects appropriately and provides a realistic test scenario.
32-46: LGTM! Proper test setup.The mock setup correctly provides the necessary dependencies for the DLP class to function.
54-57: Verify test assertion accuracy.Based on the JSON structure and risk dictionary, let me verify the expected counts:
- Extracted fields:
user,user.email,user.profile,user.profile.firstname,user.profile.lastname,active(6 total)- High risk:
user.email,user.profile(2)- Low risk:
active(1)- Unclassified:
user,user.profile.firstname,user.profile.lastname(3)The assertions appear correct.
19-30: Good use of text blocks for JSON test data.The use of Java text blocks for the JSON test data improves readability and maintainability. This is a good practice for multi-line string literals in tests.
core/src/main/java/com/predic8/membrane/core/DLP.java (4)
24-26: LGTM! Clean method design.The method provides a clear interface and delegates appropriately to private helper methods.
38-61: LGTM! Solid JSON parsing logic.The token-based parsing with context stack correctly handles nested JSON structures and builds proper field paths. The logic appropriately manages the parsing state.
69-73: LGTM! Clean helper method.The path building logic is straightforward and correct.
35-62: Verify array handling logic.The current implementation treats array field names the same as object field names. For JSON arrays containing objects, this might lead to unexpected path construction. Consider if this behavior aligns with your DLP requirements.
Please verify that the current array handling meets your requirements. For example, given JSON like
{"users": [{"name": "John"}, {"name": "Jane"}]}, the current logic will extractusersas a field name but won't distinguish between array elements or extract nested object fields within arrays.
| switch (token) { | ||
| case START_OBJECT: | ||
| case START_ARRAY: | ||
| if (currentFieldName != null) { | ||
| contextStack.push(currentFieldName); | ||
| currentFieldName = null; | ||
| } | ||
| break; | ||
|
|
||
| case END_OBJECT: | ||
| case END_ARRAY: | ||
| if (!contextStack.isEmpty()) { | ||
| contextStack.pop(); | ||
| } | ||
| break; | ||
|
|
||
| case FIELD_NAME: | ||
| currentFieldName = parser.currentName(); | ||
| fieldNames.add(buildFullPath(contextStack, currentFieldName).toLowerCase()); | ||
| break; | ||
|
|
||
| default: | ||
| break; | ||
| } |
There was a problem hiding this comment.
Fix array handling in JSON parsing logic.
The current implementation treats arrays the same as objects, which can lead to incorrect path building. Arrays don't have field names, so pushing currentFieldName onto the context stack for arrays may create malformed paths.
Consider this problematic JSON:
{
"users": [
{"name": "John"},
{"name": "Jane"}
]
}The current logic would incorrectly handle the array structure. Here's a suggested fix:
switch (token) {
case START_OBJECT:
- case START_ARRAY:
if (currentFieldName != null) {
contextStack.push(currentFieldName);
currentFieldName = null;
}
break;
+ case START_ARRAY:
+ if (currentFieldName != null) {
+ // For arrays, we want to include the array field name in the path
+ // but not treat array indices as field names
+ contextStack.push(currentFieldName);
+ currentFieldName = null;
+ }
+ break;
case END_OBJECT:
case END_ARRAY:🤖 Prompt for AI Agents
In core/src/main/java/com/predic8/membrane/core/DLP.java between lines 38 and
61, the JSON parsing logic incorrectly pushes the currentFieldName onto the
contextStack for both objects and arrays, which causes malformed paths since
arrays do not have field names. To fix this, modify the switch case so that only
START_OBJECT tokens push currentFieldName onto the contextStack, while
START_ARRAY tokens do not. This ensures that array contexts are handled
correctly without adding invalid field names to the path.
| private Set<String> extractFieldNames(Message msg) { | ||
| Set<String> fieldNames = new HashSet<>(); | ||
| try { | ||
| JsonParser parser = JSON_FACTORY.createParser(new InputStreamReader(msg.getBodyAsStreamDecoded(), msg.getCharset())); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure proper resource management for JsonParser.
The JsonParser should be closed properly to avoid resource leaks. Consider using try-with-resources.
- try {
- JsonParser parser = JSON_FACTORY.createParser(new InputStreamReader(msg.getBodyAsStreamDecoded(), msg.getCharset()));
+ try (JsonParser parser = JSON_FACTORY.createParser(new InputStreamReader(msg.getBodyAsStreamDecoded(), msg.getCharset()))) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| JsonParser parser = JSON_FACTORY.createParser(new InputStreamReader(msg.getBodyAsStreamDecoded(), msg.getCharset())); | |
| try (JsonParser parser = JSON_FACTORY.createParser( | |
| new InputStreamReader(msg.getBodyAsStreamDecoded(), msg.getCharset()))) { | |
| // existing logic using parser |
🤖 Prompt for AI Agents
In core/src/main/java/com/predic8/membrane/core/DLP.java at line 31, the
JsonParser instance created is not closed, which can cause resource leaks.
Refactor the code to use a try-with-resources statement to automatically close
the JsonParser after use, ensuring proper resource management.
| int unclassified = 0; | ||
|
|
||
| for (String field : fieldNames) { | ||
| String risk = riskDict.getOrDefault(field, "unclassified"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure consistent case handling between field extraction and dictionary lookup.
Field names are normalized to lowercase during extraction (line 56), but the risk dictionary lookup uses the field as-is. This could lead to case-sensitivity issues if the risk dictionary contains mixed-case keys.
- String risk = riskDict.getOrDefault(field, "unclassified");
+ String risk = riskDict.getOrDefault(field.toLowerCase(), "unclassified");Alternatively, normalize the risk dictionary keys during construction:
public DLP(Map<String, String> riskDict) {
- this.riskDict = riskDict;
+ this.riskDict = new HashMap<>();
+ riskDict.forEach((key, value) -> this.riskDict.put(key.toLowerCase(), value));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| String risk = riskDict.getOrDefault(field, "unclassified"); | |
| String risk = riskDict.getOrDefault(field.toLowerCase(), "unclassified"); |
🤖 Prompt for AI Agents
In core/src/main/java/com/predic8/membrane/core/DLP.java at line 84, the risk
dictionary lookup uses the field variable without normalizing its case, while
field names are converted to lowercase during extraction at line 56. To fix
this, convert the field variable to lowercase before using it as a key in
riskDict.getOrDefault to ensure consistent case handling and avoid lookup
failures due to case differences.
|
|
||
| private static final Logger logger = Logger.getLogger(DLP.class.getName()); | ||
|
|
||
| private final Map<String, String> riskDict; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making the risk dictionary case-insensitive.
The risk dictionary lookup uses exact string matching, but field names are normalized to lowercase (line 56). This could cause mismatches if the dictionary contains mixed-case keys.
Consider normalizing dictionary keys in the constructor:
public DLP(Map<String, String> riskDict) {
+ this.riskDict = new HashMap<>();
+ riskDict.forEach((key, value) -> this.riskDict.put(key.toLowerCase(), value));
- this.riskDict = riskDict;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private final Map<String, String> riskDict; | |
| public DLP(Map<String, String> riskDict) { | |
| this.riskDict = new HashMap<>(); | |
| riskDict.forEach((key, value) -> this.riskDict.put(key.toLowerCase(), value)); | |
| } |
🤖 Prompt for AI Agents
In core/src/main/java/com/predic8/membrane/core/DLP.java at line 17, the
riskDict map is currently case-sensitive, which can cause lookup mismatches
since field names are normalized to lowercase elsewhere. To fix this, modify the
constructor to normalize all keys in riskDict to lowercase when initializing the
map, ensuring consistent case-insensitive lookups.
| private Set<String> extractFieldNames(Message msg) { | ||
| Set<String> fieldNames = new HashSet<>(); | ||
| try { | ||
| JsonParser parser = JSON_FACTORY.createParser(new InputStreamReader(msg.getBodyAsStreamDecoded(), msg.getCharset())); | ||
| Deque<String> contextStack = new ArrayDeque<>(); | ||
| String currentFieldName = null; | ||
|
|
||
| while (parser.nextToken() != null) { | ||
| JsonToken token = parser.getCurrentToken(); | ||
|
|
||
| switch (token) { | ||
| case START_OBJECT: | ||
| case START_ARRAY: | ||
| if (currentFieldName != null) { | ||
| contextStack.push(currentFieldName); | ||
| currentFieldName = null; | ||
| } | ||
| break; | ||
|
|
||
| case END_OBJECT: | ||
| case END_ARRAY: | ||
| if (!contextStack.isEmpty()) { | ||
| contextStack.pop(); | ||
| } | ||
| break; | ||
|
|
||
| case FIELD_NAME: | ||
| currentFieldName = parser.currentName(); | ||
| fieldNames.add(buildFullPath(contextStack, currentFieldName).toLowerCase()); | ||
| break; | ||
|
|
||
| default: | ||
| break; | ||
| } | ||
| } | ||
| } catch (IOException e) { | ||
| throw new RuntimeException("Error extracting JSON field names", e); | ||
| } | ||
| return fieldNames; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and improve exception handling.
The method lacks validation for null inputs and the message content type. Additionally, the generic RuntimeException could be more specific.
private Set<String> extractFieldNames(Message msg) {
+ if (msg == null) {
+ throw new IllegalArgumentException("Message cannot be null");
+ }
+
Set<String> fieldNames = new HashSet<>();
try {
JsonParser parser = JSON_FACTORY.createParser(new InputStreamReader(msg.getBodyAsStreamDecoded(), msg.getCharset()));
// ... existing parsing logic
} catch (IOException e) {
- throw new RuntimeException("Error extracting JSON field names", e);
+ throw new IllegalStateException("Failed to parse JSON content", e);
}
return fieldNames;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private Set<String> extractFieldNames(Message msg) { | |
| Set<String> fieldNames = new HashSet<>(); | |
| try { | |
| JsonParser parser = JSON_FACTORY.createParser(new InputStreamReader(msg.getBodyAsStreamDecoded(), msg.getCharset())); | |
| Deque<String> contextStack = new ArrayDeque<>(); | |
| String currentFieldName = null; | |
| while (parser.nextToken() != null) { | |
| JsonToken token = parser.getCurrentToken(); | |
| switch (token) { | |
| case START_OBJECT: | |
| case START_ARRAY: | |
| if (currentFieldName != null) { | |
| contextStack.push(currentFieldName); | |
| currentFieldName = null; | |
| } | |
| break; | |
| case END_OBJECT: | |
| case END_ARRAY: | |
| if (!contextStack.isEmpty()) { | |
| contextStack.pop(); | |
| } | |
| break; | |
| case FIELD_NAME: | |
| currentFieldName = parser.currentName(); | |
| fieldNames.add(buildFullPath(contextStack, currentFieldName).toLowerCase()); | |
| break; | |
| default: | |
| break; | |
| } | |
| } | |
| } catch (IOException e) { | |
| throw new RuntimeException("Error extracting JSON field names", e); | |
| } | |
| return fieldNames; | |
| } | |
| private Set<String> extractFieldNames(Message msg) { | |
| if (msg == null) { | |
| throw new IllegalArgumentException("Message cannot be null"); | |
| } | |
| Set<String> fieldNames = new HashSet<>(); | |
| try { | |
| JsonParser parser = JSON_FACTORY.createParser( | |
| new InputStreamReader(msg.getBodyAsStreamDecoded(), msg.getCharset())); | |
| Deque<String> contextStack = new ArrayDeque<>(); | |
| String currentFieldName = null; | |
| while (parser.nextToken() != null) { | |
| JsonToken token = parser.getCurrentToken(); | |
| switch (token) { | |
| case START_OBJECT: | |
| case START_ARRAY: | |
| if (currentFieldName != null) { | |
| contextStack.push(currentFieldName); | |
| currentFieldName = null; | |
| } | |
| break; | |
| case END_OBJECT: | |
| case END_ARRAY: | |
| if (!contextStack.isEmpty()) { | |
| contextStack.pop(); | |
| } | |
| break; | |
| case FIELD_NAME: | |
| currentFieldName = parser.currentName(); | |
| fieldNames.add(buildFullPath(contextStack, currentFieldName).toLowerCase()); | |
| break; | |
| default: | |
| break; | |
| } | |
| } | |
| } catch (IOException e) { | |
| throw new IllegalStateException("Failed to parse JSON content", e); | |
| } | |
| return fieldNames; | |
| } |
🤖 Prompt for AI Agents
In core/src/main/java/com/predic8/membrane/core/DLP.java between lines 28 and
67, add validation to check if the input Message object is null and verify that
its content type is JSON before processing. Replace the generic RuntimeException
with a more specific exception, such as IOException or a custom exception, to
better indicate JSON parsing errors. Ensure that these validations and exception
handling improve robustness and clarity of error reporting.
| @Test | ||
| @SuppressWarnings("unchecked") | ||
| void analyze() { | ||
| DLP dlp = new DLP(riskDict); | ||
| Map<String, Object> result = dlp.analyze(mockMessage); | ||
|
|
||
| assertEquals(2, result.get("high_risk")); | ||
| assertEquals(0, result.get("medium_risk")); | ||
| assertEquals(1, result.get("low_risk")); | ||
| assertEquals(3, result.get("unclassified")); | ||
|
|
||
| Map<String, String> matchedFields = (Map<String, String>) result.get("matched_fields"); | ||
| assertEquals("High", matchedFields.get("user.email")); | ||
| assertEquals("High", matchedFields.get("user.profile")); | ||
| assertEquals("Low", matchedFields.get("active")); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add edge case testing.
The current test only covers the happy path. Consider adding tests for error scenarios and edge cases.
Add these additional test methods:
@Test
void analyzeWithEmptyJson() {
when(mockMessage.getBodyAsStreamDecoded())
.thenReturn(new ByteArrayInputStream("{}".getBytes(StandardCharsets.UTF_8)));
DLP dlp = new DLP(riskDict);
Map<String, Object> result = dlp.analyze(mockMessage);
assertEquals(0, result.get("high_risk"));
assertEquals(0, result.get("medium_risk"));
assertEquals(0, result.get("low_risk"));
assertEquals(0, result.get("unclassified"));
}
@Test
void analyzeWithInvalidJson() {
when(mockMessage.getBodyAsStreamDecoded())
.thenReturn(new ByteArrayInputStream("invalid json".getBytes(StandardCharsets.UTF_8)));
DLP dlp = new DLP(riskDict);
assertThrows(RuntimeException.class, () -> dlp.analyze(mockMessage));
}
@Test
void analyzeWithNullMessage() {
DLP dlp = new DLP(riskDict);
assertThrows(IllegalArgumentException.class, () -> dlp.analyze(null));
}🤖 Prompt for AI Agents
In core/src/test/java/com/predic8/membrane/core/DLPTest.java around lines 48 to
63, the analyze() test only covers the normal case. Add new test methods to
cover edge cases: one that tests analyze() with an empty JSON string and asserts
all risk counts are zero, another that tests analyze() with invalid JSON and
asserts it throws a RuntimeException, and a third that tests analyze() with a
null message and asserts it throws an IllegalArgumentException. Mock the message
body appropriately for these tests.
🛠️ Refactor suggestion
Expand test coverage for edge cases.
The current test only covers the happy path scenario. Consider adding tests for edge cases such as:
- Empty JSON objects
- Malformed JSON
- Null message input
- Empty risk dictionary
- Case sensitivity scenarios
Would you like me to generate additional test methods to cover these edge cases?
🤖 Prompt for AI Agents
In core/src/test/java/com/predic8/membrane/core/DLPTest.java around lines 48 to
63, the analyze() test only covers the happy path. Add new test methods to cover
edge cases including empty JSON objects, malformed JSON input, null message
input, empty risk dictionary, and case sensitivity scenarios. Each test should
create appropriate inputs to simulate these conditions and assert that the
analyze method handles them gracefully without errors or unexpected results.
| public Map<String, Object> analyze(Message msg) { | ||
| return evaluateRisk(extractFieldNames(msg)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null safety checks.
The analyze method should validate the input Message parameter to prevent NullPointerException during processing.
public Map<String, Object> analyze(Message msg) {
+ if (msg == null) {
+ throw new IllegalArgumentException("Message cannot be null");
+ }
return evaluateRisk(extractFieldNames(msg));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public Map<String, Object> analyze(Message msg) { | |
| return evaluateRisk(extractFieldNames(msg)); | |
| } | |
| public Map<String, Object> analyze(Message msg) { | |
| if (msg == null) { | |
| throw new IllegalArgumentException("Message cannot be null"); | |
| } | |
| return evaluateRisk(extractFieldNames(msg)); | |
| } |
🤖 Prompt for AI Agents
In core/src/main/java/com/predic8/membrane/core/DLP.java around lines 24 to 26,
the analyze method lacks null safety checks for the input Message parameter. Add
a null check at the start of the method to verify if msg is null, and handle
this case appropriately, such as returning an empty map or throwing an
IllegalArgumentException, to prevent NullPointerException during processing.
| } catch (IOException e) { | ||
| throw new RuntimeException("Error extracting JSON field names", e); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve exception handling strategy.
Throwing a RuntimeException for JSON parsing errors may not be the best approach. Consider returning an empty set, using a more specific exception type, or allowing the IOException to propagate to let callers handle it appropriately.
- } catch (IOException e) {
- throw new RuntimeException("Error extracting JSON field names", e);
- }
+ } catch (IOException e) {
+ logger.warning("Failed to parse JSON content: " + e.getMessage());
+ return new HashSet<>(); // Return empty set for malformed JSON
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (IOException e) { | |
| throw new RuntimeException("Error extracting JSON field names", e); | |
| } | |
| } catch (IOException e) { | |
| logger.warning("Failed to parse JSON content: " + e.getMessage()); | |
| return new HashSet<>(); // Return empty set for malformed JSON | |
| } |
🤖 Prompt for AI Agents
In core/src/main/java/com/predic8/membrane/core/DLP.java around lines 63 to 65,
the current code catches IOException and throws a RuntimeException, which is not
ideal for JSON parsing errors. Modify the exception handling to either return an
empty set when an IOException occurs or declare the method to throw IOException
so the caller can handle it properly. Avoid wrapping the IOException in a
RuntimeException to improve error handling clarity.
| String risk = riskDict.getOrDefault(field, "unclassified"); | ||
| matchedFields.put(field, risk); | ||
|
|
||
| switch (risk.toLowerCase()) { |
There was a problem hiding this comment.
Fix case sensitivity inconsistency.
Field names are normalized to lowercase during extraction (line 56), but risk dictionary values are compared using toLowerCase() without normalizing the dictionary keys. This creates an inconsistency where the dictionary lookup uses original casing but the switch uses lowercase.
- switch (risk.toLowerCase()) {
+ switch (risk) {Additionally, consider normalizing the risk dictionary keys in the constructor:
public DLP(Map<String, String> riskDict) {
- this.riskDict = riskDict;
+ this.riskDict = new HashMap<>();
+ riskDict.forEach((key, value) -> this.riskDict.put(key.toLowerCase(), value));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| switch (risk.toLowerCase()) { | |
| public class DLP { | |
| private final Map<String, String> riskDict; | |
| public DLP(Map<String, String> riskDict) { | |
| // normalize keys to lowercase so we can compare risk values directly | |
| this.riskDict = new HashMap<>(); | |
| riskDict.forEach((key, value) -> this.riskDict.put(key.toLowerCase(), value)); | |
| } | |
| // … | |
| private void evaluateRisk(String risk) { | |
| // risk is already normalized to lowercase earlier, so no need to call toLowerCase() here | |
| switch (risk) { | |
| case "low": | |
| // … | |
| break; | |
| case "medium": | |
| // … | |
| break; | |
| case "high": | |
| // … | |
| break; | |
| default: | |
| // … | |
| } | |
| } | |
| // … | |
| } |
🤖 Prompt for AI Agents
In core/src/main/java/com/predic8/membrane/core/DLP.java at line 87, the switch
statement uses risk.toLowerCase() but the risk dictionary keys are not
normalized to lowercase, causing case sensitivity issues. To fix this, normalize
all keys in the risk dictionary to lowercase in the constructor so lookups are
consistent. Then ensure the switch statement uses the lowercase risk value to
match the normalized keys.
| when(mockMessage.getCharset()) | ||
| .thenReturn(String.valueOf(StandardCharsets.UTF_8)); |
There was a problem hiding this comment.
Fix charset mocking.
The getCharset() method should return a charset name string, but String.valueOf(StandardCharsets.UTF_8) returns the charset object's string representation, which may not be the expected format.
- when(mockMessage.getCharset())
- .thenReturn(String.valueOf(StandardCharsets.UTF_8));
+ when(mockMessage.getCharset())
+ .thenReturn(StandardCharsets.UTF_8.name());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| when(mockMessage.getCharset()) | |
| .thenReturn(String.valueOf(StandardCharsets.UTF_8)); | |
| when(mockMessage.getCharset()) | |
| .thenReturn(StandardCharsets.UTF_8.name()); |
🤖 Prompt for AI Agents
In core/src/test/java/com/predic8/membrane/core/DLPTest.java around lines 37 to
38, the mocking of getCharset() returns String.valueOf(StandardCharsets.UTF_8),
which gives the charset object's string representation instead of the charset
name. Fix this by returning StandardCharsets.UTF_8.name() to provide the correct
charset name string expected by getCharset().
| assertEquals("High", matchedFields.get("user.profile")); | ||
| assertEquals("Low", matchedFields.get("active")); | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
src/main/resources/dlp-fields.csv
DLPIntercept
init() load fields
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/CsvFieldConfiguration.java (1)
21-24: Consider using IllegalArgumentException instead of NullPointerException.While the error handling is good, throwing
IllegalArgumentExceptionwould be more semantically appropriate for invalid input parameters.- throw new NullPointerException("InputStream is null. File not found: " + fileName); + throw new IllegalArgumentException("Configuration file not found: " + fileName);core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLP.java (2)
72-76: Consider handling array indices in JSON path construction.The current implementation doesn't include array indices in the path, which might miss array-specific field configurations.
For complete path tracking, consider including array indices:
private String buildFullPath(Deque<String> stack, String field) { List<String> path = new ArrayList<>(stack); path.add(field); return String.join(".", path); } + + // Alternative with array index support + private String buildFullPathWithIndices(Deque<String> stack, String field, int arrayIndex) { + List<String> path = new ArrayList<>(stack); + if (arrayIndex >= 0) { + path.add("[" + arrayIndex + "]"); + } + path.add(field); + return String.join(".", path); + }
52-56: Simplify complex risk level lookup logic.The nested Optional.ofNullable chain is hard to read and understand. Consider breaking it down for better maintainability.
if (currentField != null) { String fullPath = buildFullPath(contextStack, currentField).toLowerCase(); - report.recordField(fullPath, Optional.ofNullable(riskDict.get(fullPath)) - .orElse(riskDict.getOrDefault(currentField.toLowerCase(), "unclassified")) - .toLowerCase()); + String riskLevel = determineRiskLevel(fullPath, currentField.toLowerCase()); + report.recordField(fullPath, riskLevel); currentField = null; } + + private String determineRiskLevel(String fullPath, String fieldName) { + // Try full path first, then field name, default to unclassified + String riskLevel = riskDict.get(fullPath); + if (riskLevel == null) { + riskLevel = riskDict.getOrDefault(fieldName, "unclassified"); + } + return riskLevel.toLowerCase(); + }core/src/main/java/com/predic8/membrane/core/interceptor/dlp/RiskReport.java (1)
22-23: Consider using diamond operator for type inference.For consistency with modern Java practices and reduced verbosity.
- Map<String, Object> out = new LinkedHashMap<String, Object>(); + Map<String, Object> out = new LinkedHashMap<>();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/CsvFieldConfiguration.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLP.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPInterceptor.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Field.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/FieldConfiguration.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Fields.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/RiskReport.java(1 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/dlp/DLPTest.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Fields.java
🚧 Files skipped from review as they are similar to previous changes (2)
- core/src/test/java/com/predic8/membrane/core/interceptor/dlp/DLPTest.java
- core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPInterceptor.java
🧰 Additional context used
🧬 Code Graph Analysis (1)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Field.java (2)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Fields.java (1)
MCElement(9-23)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPInterceptor.java (1)
MCElement(15-90)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (3)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/FieldConfiguration.java (1)
5-7: LGTM! Clean interface design.The interface provides a clean abstraction for loading field configurations, allowing for different implementations (CSV, database, etc.) while maintaining a consistent contract.
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLP.java (1)
26-66: LGTM! Efficient streaming JSON analysis.The implementation uses Jackson's streaming parser effectively, maintaining proper context tracking for nested structures. The error handling and logging are appropriate.
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/RiskReport.java (1)
22-32: LGTM! Well-structured reporting with proper data aggregation.The implementation effectively manages risk analysis results with clear separation of concerns. The use of
LinkedHashMapmaintains order for consistent reporting, and the lambda-based map operations are clean and efficient.
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/CsvFieldConfiguration.java
Show resolved
Hide resolved
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Field.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Field.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Field.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Field.java (2)
24-24: Consider a more restrictive default pattern.The default pattern
".*"matches everything, which could lead to unintended behavior if a field name is not properly set. Consider using a pattern that matches nothing by default or require explicit field name configuration.-private Pattern compiled = Pattern.compile(".*"); +private Pattern compiled = Pattern.compile("$^"); // matches nothing by default
58-58: Preserve original Content-Type when possible.Hardcoding the Content-Type to "application/json; charset=UTF-8" might override important Content-Type parameters from the original message (e.g., custom charset, JSON variants).
-msg.getHeader().setContentType("application/json; charset=UTF-8"); +if (msg.getHeader().getContentType() == null || !msg.getHeader().getContentType().toLowerCase().contains("json")) { + msg.getHeader().setContentType("application/json; charset=UTF-8"); +}core/src/test/java/com/predic8/membrane/core/interceptor/dlp/DLPAnalyzerTest.java (1)
56-57: Consider using English comments in test code.The German comments ("Pfad-Match", "Leaf-Match") might cause confusion for international development teams.
-riskDict.put("user.email", "High"); // Pfad-Match -riskDict.put("active", "Low"); // Leaf-Match +riskDict.put("user.email", "High"); // Path-based match +riskDict.put("active", "Low"); // Leaf-based matchcore/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPAnalyzer.java (2)
69-71: Consider more specific exception handling.The generic
RuntimeExceptionwrapper loses important context about the type of failure (JSON parsing, I/O, etc.). Consider preserving the original exception type or providing more specific error messages.} catch (Exception e) { - throw new RuntimeException(e); + throw new RuntimeException("Failed to analyze message for DLP risks: " + e.getMessage(), e); }
74-79: Verify case-insensitive matching performance.The method calls
toLowerCase()on every field lookup. For high-throughput scenarios, consider pre-processing the risk dictionary keys to lowercase during initialization to avoid repeated string operations.public DLPAnalyzer(Map<String, String> riskDict) { - this.riskDict = Map.copyOf(riskDict); // immutable defensive copy + // Pre-process keys to lowercase for efficient lookup + Map<String, String> normalizedDict = new HashMap<>(); + riskDict.forEach((k, v) -> normalizedDict.put(k.toLowerCase(), v.toLowerCase())); + this.riskDict = Map.copyOf(normalizedDict); } private String classify(String fullPath, String simpleName) { - return Optional.ofNullable(riskDict.get(fullPath.toLowerCase())) - .or(() -> Optional.ofNullable(riskDict.get(simpleName.toLowerCase()))) + return Optional.ofNullable(riskDict.get(fullPath)) + .or(() -> Optional.ofNullable(riskDict.get(simpleName))) .orElse("unclassified") - .toLowerCase(); + ; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPAnalyzer.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPInterceptor.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Field.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/JsonUtils.java(1 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/dlp/DLPAnalyzerTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPInterceptor.java
🧰 Additional context used
🧬 Code Graph Analysis (1)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Field.java (4)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPInterceptor.java (1)
MCElement(15-90)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Fields.java (1)
MCElement(9-23)core/src/main/java/com/predic8/membrane/core/graphql/model/Field.java (1)
Field(28-120)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/JsonUtils.java (1)
JsonUtils(10-43)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (5)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/JsonUtils.java (1)
10-43: Excellent implementation addressing previous regex concerns.The utility class properly uses Jackson's JsonNode API for safe JSON manipulation, replacing the problematic regex-based string operations mentioned in previous reviews. The recursive traversal correctly handles both objects and arrays, and the iterator-based removal is safe for concurrent modification.
core/src/test/java/com/predic8/membrane/core/interceptor/dlp/DLPAnalyzerTest.java (2)
22-42: Well-structured test data using text blocks.The use of Java text blocks for JSON test data makes the tests highly readable and maintainable. The test scenarios cover both nested objects and arrays effectively.
55-61: Comprehensive risk dictionary setup.The test setup properly covers both path-based matching ("user.email") and leaf-based matching ("active", "foo", "bar"), which validates the analyzer's dual classification strategy.
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPAnalyzer.java (2)
18-28: Excellent security-conscious JSON parser configuration.The implementation includes proper security constraints (max depth, max string length) and allows trailing commas while disabling potentially dangerous features like unescaped control characters. This provides good protection against JSON-based attacks.
32-34: Good defensive programming with immutable copy.Creating an immutable defensive copy of the risk dictionary prevents external modifications and ensures thread safety.
| switch (action) { | ||
| case FILTER -> JsonUtils.filter(root, compiled); | ||
| case MASK -> JsonUtils.mask(root, compiled); | ||
| case REPORT -> {/* no?op */} | ||
| } | ||
|
|
||
| byte[] out = MAPPER.writeValueAsBytes(root); | ||
| msg.setBodyContent(out); | ||
| msg.getHeader().setContentLength(out.length); | ||
| msg.getHeader().setContentType("application/json; charset=UTF-8"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize REPORT action to avoid unnecessary body modification.
The current implementation updates the message body and headers even for the REPORT action, which is a no-op. This creates unnecessary overhead and could potentially alter message characteristics when only reporting is intended.
switch (action) {
case FILTER -> JsonUtils.filter(root, compiled);
case MASK -> JsonUtils.mask(root, compiled);
case REPORT -> {/* no-op */}
}
- byte[] out = MAPPER.writeValueAsBytes(root);
- msg.setBodyContent(out);
- msg.getHeader().setContentLength(out.length);
- msg.getHeader().setContentType("application/json; charset=UTF-8");
+ if (action != Action.REPORT) {
+ byte[] out = MAPPER.writeValueAsBytes(root);
+ msg.setBodyContent(out);
+ msg.getHeader().setContentLength(out.length);
+ msg.getHeader().setContentType("application/json; charset=UTF-8");
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| switch (action) { | |
| case FILTER -> JsonUtils.filter(root, compiled); | |
| case MASK -> JsonUtils.mask(root, compiled); | |
| case REPORT -> {/* no?op */} | |
| } | |
| byte[] out = MAPPER.writeValueAsBytes(root); | |
| msg.setBodyContent(out); | |
| msg.getHeader().setContentLength(out.length); | |
| msg.getHeader().setContentType("application/json; charset=UTF-8"); | |
| switch (action) { | |
| case FILTER -> JsonUtils.filter(root, compiled); | |
| case MASK -> JsonUtils.mask(root, compiled); | |
| case REPORT -> { /* no-op */ } | |
| } | |
| if (action != Action.REPORT) { | |
| byte[] out = MAPPER.writeValueAsBytes(root); | |
| msg.setBodyContent(out); | |
| msg.getHeader().setContentLength(out.length); | |
| msg.getHeader().setContentType("application/json; charset=UTF-8"); | |
| } |
🤖 Prompt for AI Agents
In core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Field.java
around lines 49 to 58, the code updates the message body and headers even when
the action is REPORT, which does nothing and causes unnecessary processing.
Modify the code to skip updating the message body and headers when the action is
REPORT, so that no changes or overhead occur for this no-op case.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPInterceptor.java(1 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/dlp/DLPAnalyzerTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/test/java/com/predic8/membrane/core/interceptor/dlp/DLPAnalyzerTest.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (java)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPInterceptor.java
Show resolved
Hide resolved
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPInterceptor.java
Outdated
Show resolved
Hide resolved
| @MCAttribute | ||
| public void setAction(String action) { | ||
| this.action = action; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Verification correct
Add validation for the action parameter.
The setter should validate that the action is one of the supported values.
Apply this diff to add validation:
@MCAttribute
public void setAction(String action) {
+ if (action != null && !action.matches("(?i)filter|mask|report")) {
+ throw new IllegalArgumentException("Invalid action: " + action + ". Must be one of: filter, mask, report");
+ }
this.action = action;
}Run the following script to verify the valid actions in the Field class:
🏁 Script executed:
#!/bin/bash
# Description: Verify the valid actions supported by the Field class
# Search for action handling in Field class
ast-grep --pattern 'class Field {
$$$
}'
# Search for action-related constants or enums
rg -A 5 "action.*=.*\"(filter|mask|report)\""Length of output: 703
Add validation for the action parameter.
The setter should validate that the action is one of the supported values.
Apply this diff to add validation:
@MCAttribute
public void setAction(String action) {
+ if (action != null && !action.matches("(?i)filter|mask|report")) {
+ throw new IllegalArgumentException("Invalid action: " + action + ". Must be one of: filter, mask, report");
+ }
this.action = action;
}Run the following script to verify the valid actions in the Field class:
#!/bin/bash
# Description: Verify the valid actions supported by the Field class
# Search for action handling in Field class
ast-grep --pattern 'class Field {
$$$
}'
# Search for action-related constants or enums
rg -A 5 "action.*=.*\"(filter|mask|report)\""🤖 Prompt for AI Agents
In
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPInterceptor.java
around lines 77 to 80, the setAction method lacks validation for the action
parameter. Modify the setter to check if the provided action string matches one
of the supported values (e.g., "filter", "mask", "report") before assigning it.
If the action is invalid, throw an IllegalArgumentException or handle the error
appropriately to prevent unsupported actions from being set.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPAnalyzer.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Field.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (4)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Field.java (1)
13-50: Excellent refactoring addresses previous security and design concerns.The Field class has been significantly improved:
- Strategy pattern: Replaced fragile regex-based JSON manipulation with proper delegation to
FieldActionStrategy- Type safety: Using enum for actions provides compile-time validation
- Robust validation:
Action.valueOf()automatically handles invalid action values- Clean error handling: Exceptions are properly caught and logged
This addresses all the critical issues from previous reviews regarding regex vulnerabilities, validation, and the REPORT action handling.
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPAnalyzer.java (3)
16-23: Well-configured JSON factory with appropriate security constraints.The JSON factory configuration includes important security measures:
- Nesting depth limit: 64 levels prevents stack overflow attacks
- String length limit: 16KB prevents memory exhaustion
- Disabled unescaped control chars: Prevents injection attacks
- Trailing commas allowed: Provides flexibility for malformed JSON
29-31: Good defensive programming with immutable copy.Using
Map.copyOf()creates an immutable defensive copy of the risk dictionary, preventing external modifications after construction.
60-65: Robust classification logic with proper fallback handling.The classify method effectively handles both full path and simple name lookups with appropriate fallback to "unclassified". The use of Optional chaining is clean and null-safe.
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPAnalyzer.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/MaskField.java (1)
20-35: Add input validation for null parameters.The method lacks validation for null parameters, which could lead to NullPointerExceptions during execution.
public void apply(Message msg, Pattern pattern) { + if (msg == null || pattern == null) { + log.warn("MaskField.apply called with null parameters"); + return; + } + try { JsonNode root = OBJECT_MAPPER.readTree(msg.getBodyAsStringDecoded());
🧹 Nitpick comments (1)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/MaskField.java (1)
32-34: Improve error logging specificity.Consider providing more specific error information to aid in debugging, especially distinguishing between JSON parsing errors and other processing errors.
} catch (Exception e) { - log.error("MaskAction failed", e); + log.error("Failed to mask JSON fields in message body: {}", e.getMessage(), e); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/CsvFieldConfiguration.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPAnalyzer.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPInterceptor.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/FilterField.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/MaskField.java(1 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/dlp/DLPInterceptorTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- core/src/main/java/com/predic8/membrane/core/interceptor/dlp/CsvFieldConfiguration.java
- core/src/main/java/com/predic8/membrane/core/interceptor/dlp/FilterField.java
- core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPAnalyzer.java
- core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPInterceptor.java
- core/src/test/java/com/predic8/membrane/core/interceptor/dlp/DLPInterceptorTest.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (3)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/MaskField.java (3)
28-31: LGTM! Content-type preservation implemented correctly.Good improvement from previous feedback - the code now preserves the original content-type when it exists and includes charset information.
45-46: LGTM! Pattern matching consistency resolved.Excellent fix! The pattern matching now checks both the full path and individual segments, ensuring consistent behavior with other field action strategies.
56-60: LGTM! Array index tracking properly implemented.Good improvement - array indices are now properly tracked in the path, ensuring accurate pattern matching for array elements.
| try { | ||
| JsonNode root = OBJECT_MAPPER.readTree(msg.getBodyAsStringDecoded()); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add JSON validation before processing.
The code should validate that the message body contains valid JSON before attempting to parse it, to provide better error handling and logging.
try {
+ String body = msg.getBodyAsStringDecoded();
+ if (body == null || body.trim().isEmpty()) {
+ log.debug("Empty or null message body, skipping masking");
+ return;
+ }
- JsonNode root = OBJECT_MAPPER.readTree(msg.getBodyAsStringDecoded());
+ JsonNode root = OBJECT_MAPPER.readTree(body);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| JsonNode root = OBJECT_MAPPER.readTree(msg.getBodyAsStringDecoded()); | |
| try { | |
| String body = msg.getBodyAsStringDecoded(); | |
| if (body == null || body.trim().isEmpty()) { | |
| log.debug("Empty or null message body, skipping masking"); | |
| return; | |
| } | |
| JsonNode root = OBJECT_MAPPER.readTree(body); |
🤖 Prompt for AI Agents
In core/src/main/java/com/predic8/membrane/core/interceptor/dlp/MaskField.java
around lines 22 to 23, add a validation step before parsing the message body as
JSON. Implement a check to confirm the message body is valid JSON, and if not,
handle the error gracefully by logging an appropriate message and avoiding the
parse attempt. This will improve error handling and prevent exceptions from
invalid JSON input.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Filter.java (3)
3-5: Remove unused Jackson imports.The Jackson imports (
JsonNode,ObjectMapper,ObjectNode) are not used in this class and should be removed to keep the imports clean.-import com.fasterxml.jackson.databind.JsonNode; -import com.fasterxml.jackson.databind.ObjectMapper; -import com.fasterxml.jackson.databind.node.ObjectNode;
30-32: Consider more specific exception handling.The broad exception catch might mask specific issues. Consider handling
JsonPathExceptionandInvalidJsonExceptionseparately if you need different behaviors for different error types.} catch (Exception e) { - throw new RuntimeException("DLP Filter failed to apply JSONPath deletions", e); + log.error("DLP Filter failed to apply JSONPath deletions to JSON", e); + throw new RuntimeException("DLP Filter failed to apply JSONPath deletions", e);Adding logging before rethrowing helps with debugging while maintaining the current error propagation behavior.
37-40: Consider making the setter more defensive.The setter accepts a null list which could cause NullPointerExceptions later. Consider adding null checking or defensive copying.
@MCChildElement public Filter setPaths(List<Path> paths) { - this.paths = paths; + this.paths = paths != null ? new ArrayList<>(paths) : new ArrayList<>(); return this; }This ensures the internal list is never null and provides isolation from external modifications.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPInterceptor.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Filter.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Path.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Path.java
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPInterceptor.java
🧰 Additional context used
🧬 Code Graph Analysis (1)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Filter.java (2)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Path.java (1)
MCElement(6-19)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPInterceptor.java (1)
MCElement(17-91)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (java)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Filter.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Filter.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Filter.java (2)
22-23: Address the empty catch block to avoid silencing errors.The empty catch block silently ignores exceptions during JSONPath deletion, which can hide legitimate issues and make debugging difficult.
25-25: Replace System.out.println with proper logging.Using
System.out.printlnin production code bypasses the application's logging framework and configuration.
🧹 Nitpick comments (4)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Filter.java (2)
32-36: Consider renaming the method to better reflect its purpose.The method name
setPathsdoesn't clearly indicate that it's settingFieldobjects containing JSONPath expressions. Consider renaming tosetFieldsfor consistency with the field name and better clarity.@MCChildElement -public Filter setPaths(List<Field> fields) { +public Filter setFields(List<Field> fields) { this.fields = fields; return this; }
38-40: Rename getter method for consistency.The getter method name should match the setter method name for consistency and clarity.
-public List<Field> getPaths() { +public List<Field> getFields() { return fields; }core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Field.txt (2)
42-47: Improve error handling in handleAction method.The current error handling logs the exception but continues processing. Consider whether this is the intended behavior or if the exception should be propagated to allow the caller to handle it appropriately.
If the intention is to fail fast on field action errors, consider:
public void handleAction(Message msg) { try { FieldActionStrategy.of(action.name().toLowerCase(Locale.ROOT)).apply(msg, compiled); } catch (Exception e) { log.error("DLP field action '{}' failed: {}", name, e); + throw new RuntimeException("Failed to apply DLP field action for field: " + name, e); } }Otherwise, consider logging at warn level instead of error if this is expected behavior:
- log.error("DLP field action '{}' failed: {}", name, e); + log.warn("DLP field action '{}' failed, continuing with next field: {}", name, e.getMessage());
20-20: Consider making the default pattern more restrictive.The default pattern ".*" matches everything, which might not be the intended behavior for a DLP field. Consider using a more restrictive default or requiring explicit configuration.
-private Pattern compiled = Pattern.compile(".*"); +private Pattern compiled = null; // Require explicit configurationThen add validation in the handleAction method:
public void handleAction(Message msg) { + if (compiled == null) { + log.warn("Field '{}' has no pattern configured, skipping action", name); + return; + } try {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Field.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Field.txt(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Filter.java(1 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/dlp/DLPInterceptorTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Field.java
- core/src/test/java/com/predic8/membrane/core/interceptor/dlp/DLPInterceptorTest.java
🧰 Additional context used
🧬 Code Graph Analysis (1)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Filter.java (2)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Field.java (1)
MCElement(6-19)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPInterceptor.java (1)
MCElement(17-91)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (1)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Field.txt (1)
24-26: Good null validation and pattern compilation.The null check with a descriptive message and immediate pattern compilation is well-implemented. This ensures early failure with clear error messages.
| public void setAction(String action) { | ||
| this.action = Action.valueOf(action.toUpperCase(Locale.ROOT)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate action string before conversion.
The method doesn't validate if the provided action string is a valid enum value before conversion, which could lead to IllegalArgumentException.
@MCAttribute
public void setAction(String action) {
+ Objects.requireNonNull(action, "action must not be null");
+ try {
this.action = Action.valueOf(action.toUpperCase(Locale.ROOT));
+ } catch (IllegalArgumentException e) {
+ throw new IllegalArgumentException("Invalid action: " + action + ". Valid actions are: " +
+ String.join(", ", Arrays.stream(Action.values()).map(Enum::name).toArray(String[]::new)), e);
+ }
}You'll need to add this import:
+import java.util.Arrays;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public void setAction(String action) { | |
| this.action = Action.valueOf(action.toUpperCase(Locale.ROOT)); | |
| } | |
| import java.util.Arrays; | |
| @MCAttribute | |
| public void setAction(String action) { | |
| Objects.requireNonNull(action, "action must not be null"); | |
| try { | |
| this.action = Action.valueOf(action.toUpperCase(Locale.ROOT)); | |
| } catch (IllegalArgumentException e) { | |
| throw new IllegalArgumentException( | |
| "Invalid action: " + action + | |
| ". Valid actions are: " + | |
| String.join(", ", | |
| Arrays.stream(Action.values()) | |
| .map(Enum::name) | |
| .toArray(String[]::new) | |
| ), | |
| e | |
| ); | |
| } | |
| } |
🤖 Prompt for AI Agents
In core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Field.txt around
lines 29 to 31, the setAction method converts the input string to an enum
without validation, risking IllegalArgumentException. To fix this, validate the
input string against the Action enum values before conversion. If the string is
invalid, handle it gracefully by throwing a custom exception or logging an
error. Also, add the necessary import for Locale if not already present.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (2)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPInterceptor.java (2)
69-72: Add validation for the action parameter.The setter should validate that the action is one of the supported values as mentioned in the previous review.
@MCAttribute public void setAction(String action) { + if (action != null && !action.matches("(?i)filter|mask|report")) { + throw new IllegalArgumentException("Invalid action: " + action + ". Must be one of: filter, mask, report"); + } this.action = action; }
69-72: Add validation for the action parameter.The setter should validate that the action is one of the supported values, as mentioned in previous review comments but not yet implemented.
Apply this diff to add validation:
@MCAttribute public void setAction(String action) { + if (action != null && !action.matches("(?i)filter|mask|report")) { + throw new IllegalArgumentException("Invalid action: " + action + ". Must be one of: filter, mask, report"); + } this.action = action; }
🧹 Nitpick comments (1)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPInterceptor.java (1)
29-35: Consider adding validation for fieldsConfig parameter.The init method should validate that the fieldsConfig points to a valid file path if provided, to fail fast with a clear error message.
Apply this diff to add validation:
@Override public void init() { + super.init(); if (fieldsConfig != null) { + // Validate that the config file exists and is readable + try { dlpAnalyzer = new DLPAnalyzer(new CsvFieldConfiguration().getFields(fieldsConfig)); + } catch (Exception e) { + throw new RuntimeException("Failed to load DLP fields configuration from: " + fieldsConfig, e); + } } else { dlpAnalyzer = new DLPAnalyzer(java.util.Map.of()); } - super.init(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPInterceptor.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Filter.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Mask.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Filter.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (1)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPInterceptor.java (1)
29-36: Good improvement on analyzer initialization.The initialization of
DLPAnalyzerin theinit()method addresses the performance concern from previous reviews. This ensures the analyzer is created once rather than on every request.
| @MCAttribute | ||
| public void setField(String field) { | ||
| this.field = field; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for field parameter.
The setter should validate that the field is a valid JSONPath expression.
@MCAttribute
public void setField(String field) {
+ if (field != null && field.trim().isEmpty()) {
+ throw new IllegalArgumentException("field cannot be empty");
+ }
this.field = field;
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Mask.java around
lines 55 to 58, the setField method lacks validation for the field parameter.
Add validation logic to check that the provided field string is a valid JSONPath
expression before assigning it to this.field. If the validation fails, throw an
appropriate exception or handle the error to prevent invalid JSONPath
expressions from being set.
| public String apply(String json) { | ||
| try { | ||
| DocumentContext context = JsonPath.parse(json); | ||
| context.set(field, maskKeepRight(context.read(field, String.class), Integer.parseInt(keepRight))); | ||
| return context.jsonString(); | ||
| } catch (Exception e) { | ||
| throw new RuntimeException("DLP Filter failed to apply JSONPath deletions", e); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add input validation and fix error handling issues.
The apply method has several issues:
- No null/empty validation for the
jsonparameter Integer.parseInt(keepRight)can throwNumberFormatExceptionifkeepRightis invalidcontext.read(field, String.class)can fail if the field doesn't exist or isn't a string- Misleading error message mentions "Filter" instead of "Mask"
public String apply(String json) {
+ if (json == null || json.trim().isEmpty()) {
+ return json;
+ }
+ if (field == null || keepRight == null) {
+ throw new IllegalStateException("Both field and keepRight must be configured");
+ }
+
try {
+ int keepRightValue = Integer.parseInt(keepRight);
DocumentContext context = JsonPath.parse(json);
- context.set(field, maskKeepRight(context.read(field, String.class), Integer.parseInt(keepRight)));
+ String fieldValue = context.read(field, String.class);
+ if (fieldValue != null) {
+ context.set(field, maskKeepRight(fieldValue, keepRightValue));
+ }
return context.jsonString();
+ } catch (NumberFormatException e) {
+ throw new IllegalArgumentException("keepRight must be a valid integer: " + keepRight, e);
} catch (Exception e) {
- throw new RuntimeException("DLP Filter failed to apply JSONPath deletions", e);
+ throw new RuntimeException("DLP Mask failed to apply JSONPath masking", e);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public String apply(String json) { | |
| try { | |
| DocumentContext context = JsonPath.parse(json); | |
| context.set(field, maskKeepRight(context.read(field, String.class), Integer.parseInt(keepRight))); | |
| return context.jsonString(); | |
| } catch (Exception e) { | |
| throw new RuntimeException("DLP Filter failed to apply JSONPath deletions", e); | |
| } | |
| } | |
| public String apply(String json) { | |
| if (json == null || json.trim().isEmpty()) { | |
| return json; | |
| } | |
| if (field == null || keepRight == null) { | |
| throw new IllegalStateException("Both field and keepRight must be configured"); | |
| } | |
| try { | |
| int keepRightValue = Integer.parseInt(keepRight); | |
| DocumentContext context = JsonPath.parse(json); | |
| String fieldValue = context.read(field, String.class); | |
| if (fieldValue != null) { | |
| context.set(field, maskKeepRight(fieldValue, keepRightValue)); | |
| } | |
| return context.jsonString(); | |
| } catch (NumberFormatException e) { | |
| throw new IllegalArgumentException("keepRight must be a valid integer: " + keepRight, e); | |
| } catch (Exception e) { | |
| throw new RuntimeException("DLP Mask failed to apply JSONPath masking", e); | |
| } | |
| } |
🤖 Prompt for AI Agents
In core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Mask.java around
lines 28 to 36, add validation to check if the input json string is null or
empty and handle it appropriately. Wrap the Integer.parseInt(keepRight) call in
a try-catch to handle NumberFormatException and provide a clear error message.
Before calling context.read(field, String.class), verify that the field exists
and is a string to avoid runtime exceptions. Finally, update the error message
in the catch block to correctly reference "Mask" instead of "Filter" for
clarity.
Add input validation and fix misleading error message.
The method has several issues:
- No validation for null or empty JSON input
- The error message mentions "DLP Filter" but this is a masking operation
- No validation that
keepRightis a valid integer
Apply this diff to improve the method:
public String apply(String json) {
+ if (json == null || json.trim().isEmpty()) {
+ return json;
+ }
+
try {
DocumentContext context = JsonPath.parse(json);
- context.set(field, maskKeepRight(context.read(field, String.class), Integer.parseInt(keepRight)));
+ int keepRightValue = keepRight != null ? Integer.parseInt(keepRight) : 0;
+ context.set(field, maskKeepRight(context.read(field, String.class), keepRightValue));
return context.jsonString();
} catch (Exception e) {
- throw new RuntimeException("DLP Filter failed to apply JSONPath deletions", e);
+ throw new RuntimeException("DLP Mask failed to apply JSONPath masking", e);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public String apply(String json) { | |
| try { | |
| DocumentContext context = JsonPath.parse(json); | |
| context.set(field, maskKeepRight(context.read(field, String.class), Integer.parseInt(keepRight))); | |
| return context.jsonString(); | |
| } catch (Exception e) { | |
| throw new RuntimeException("DLP Filter failed to apply JSONPath deletions", e); | |
| } | |
| } | |
| public String apply(String json) { | |
| if (json == null || json.trim().isEmpty()) { | |
| return json; | |
| } | |
| try { | |
| DocumentContext context = JsonPath.parse(json); | |
| int keepRightValue = keepRight != null ? Integer.parseInt(keepRight) : 0; | |
| context.set(field, maskKeepRight(context.read(field, String.class), keepRightValue)); | |
| return context.jsonString(); | |
| } catch (Exception e) { | |
| throw new RuntimeException("DLP Mask failed to apply JSONPath masking", e); | |
| } | |
| } |
🤖 Prompt for AI Agents
In core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Mask.java around
lines 28 to 36, add validation to check if the input JSON string is null or
empty and throw an appropriate exception if so. Validate that the keepRight
field can be parsed as an integer before using it, throwing a clear exception if
parsing fails. Also, update the error message in the catch block to accurately
reflect that this is a masking operation, not a DLP filter deletion.
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPInterceptor.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Mask.java (2)
14-22: Fix error handling and add input validation.The method has multiple issues that need to be addressed:
- No validation for null/empty JSON input
- RuntimeException with no error message
- No validation that the field exists in the JSON
@Override public String apply(String json) { + if (json == null || json.trim().isEmpty()) { + return json; + } + if (getField() == null) { + throw new IllegalStateException("field must be configured"); + } try { DocumentContext context = JsonPath.parse(json); - context.set(getField(), maskKeepRight(context.read(getField(), String.class), Integer.parseInt(keepRight))); + String fieldValue = context.read(getField(), String.class); + if (fieldValue != null) { + context.set(getField(), maskKeepRight(fieldValue, Integer.parseInt(keepRight))); + } return context.jsonString(); } catch (Exception e) { - throw new RuntimeException(); + throw new RuntimeException("DLP Mask failed to apply masking", e); } }
41-44: Add validation for keepRight parameter.The setter should validate that keepRight is a valid non-negative integer.
@MCAttribute public void setKeepRight(String keepRight) { + if (keepRight != null) { + try { + int value = Integer.parseInt(keepRight); + if (value < 0) { + throw new IllegalArgumentException("keepRight must be non-negative: " + keepRight); + } + } catch (NumberFormatException e) { + throw new IllegalArgumentException("keepRight must be a valid integer: " + keepRight, e); + } + } this.keepRight = keepRight; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Action.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPInterceptor.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Filter.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Mask.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Report.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/RiskReport.java(1 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/dlp/DLPInterceptorTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Filter.java
- core/src/main/java/com/predic8/membrane/core/interceptor/dlp/RiskReport.java
- core/src/test/java/com/predic8/membrane/core/interceptor/dlp/DLPInterceptorTest.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). (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (1)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPInterceptor.java (1)
30-37: No action needed: DLPAnalyzer is already thread-safe
TheDLPAnalyzerholds only immutable state (riskDictviaMap.copyOf) and uses a shared, thread-safeObjectMapperfor JSON parsing. All traversal state (Deque,RiskReport) is method-local per invocation. You can safely reuse a singleDLPAnalyzerinstance across concurrent requests.
| @MCAttribute | ||
| public void setField(String field) { | ||
| this.field = field; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for the field parameter.
The setter should validate that the field is not empty and is a valid JSONPath expression to prevent runtime errors.
@MCAttribute
public void setField(String field) {
+ if (field != null && field.trim().isEmpty()) {
+ throw new IllegalArgumentException("field cannot be empty");
+ }
this.field = field;
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Action.java
around lines 15 to 18, the setField method lacks validation for the input
parameter. Add validation to ensure the field parameter is not null or empty and
verify it is a valid JSONPath expression before assigning it to the field
variable. If validation fails, throw an appropriate exception to prevent runtime
errors.
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Report.java
Outdated
Show resolved
Hide resolved
| public String apply(String json, RiskReport risk) { | ||
| try { | ||
| DocumentContext context = JsonPath.parse(json); | ||
| context.put("$", "risk", Map.of("category", risk.getCategory().name(), "counts", risk.getLogReport())); | ||
| return context.jsonString(); | ||
| } catch (Exception e) { | ||
| log.error("Failed to inject DLP report", e); | ||
| throw new RuntimeException("DLP Report injection failed", e); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and improve error handling.
The method should validate inputs and provide more specific error handling.
public String apply(String json, RiskReport risk) {
+ if (json == null || json.trim().isEmpty()) {
+ return json;
+ }
+ if (risk == null) {
+ throw new IllegalArgumentException("RiskReport cannot be null");
+ }
try {
DocumentContext context = JsonPath.parse(json);
context.put("$", "risk", Map.of("category", risk.getCategory().name(), "counts", risk.getLogReport()));
return context.jsonString();
} catch (Exception e) {
- log.error("Failed to inject DLP report", e);
+ log.error("Failed to inject DLP risk report into JSON", e);
throw new RuntimeException("DLP Report injection failed", e);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public String apply(String json, RiskReport risk) { | |
| try { | |
| DocumentContext context = JsonPath.parse(json); | |
| context.put("$", "risk", Map.of("category", risk.getCategory().name(), "counts", risk.getLogReport())); | |
| return context.jsonString(); | |
| } catch (Exception e) { | |
| log.error("Failed to inject DLP report", e); | |
| throw new RuntimeException("DLP Report injection failed", e); | |
| } | |
| } | |
| public String apply(String json, RiskReport risk) { | |
| if (json == null || json.trim().isEmpty()) { | |
| return json; | |
| } | |
| if (risk == null) { | |
| throw new IllegalArgumentException("RiskReport cannot be null"); | |
| } | |
| try { | |
| DocumentContext context = JsonPath.parse(json); | |
| context.put("$", "risk", Map.of( | |
| "category", risk.getCategory().name(), | |
| "counts", risk.getLogReport() | |
| )); | |
| return context.jsonString(); | |
| } catch (Exception e) { | |
| log.error("Failed to inject DLP risk report into JSON", e); | |
| throw new RuntimeException("DLP Report injection failed", e); | |
| } | |
| } |
🤖 Prompt for AI Agents
In core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Report.java
around lines 27 to 36, the apply method lacks input validation and uses a
generic exception catch. Add checks to ensure the json string and risk object
are not null or empty before processing. Replace the generic Exception catch
with more specific exceptions related to JSON parsing or manipulation. Provide
clear error messages for each validation failure and exception type to improve
error handling and debugging.
| public Outcome handleInternal(Message msg) { | ||
| try { | ||
| log.info("DLP Risk Analysis: {}", dlpAnalyzer.analyze(msg).getLogReport()); | ||
|
|
||
| if (!masks.isEmpty()) { | ||
| for (Mask mask : masks) { | ||
| msg.setBodyContent(mask.apply(msg.getBodyAsStringDecoded()) | ||
| .getBytes(StandardCharsets.UTF_8)); | ||
| } | ||
| } | ||
|
|
||
| if (!filters.isEmpty()) { | ||
| for (Filter filter : filters) { | ||
| msg.setBodyContent(filter.apply(msg.getBodyAsStringDecoded()) | ||
| .getBytes(StandardCharsets.UTF_8)); | ||
| } | ||
| } | ||
|
|
||
| if (!reports.isEmpty()) { | ||
| for (Report report : reports) { | ||
| msg.setBodyContent(report.apply(msg.getBodyAsStringDecoded(), dlpAnalyzer.analyze(msg)).getBytes(StandardCharsets.UTF_8)); | ||
| } | ||
| } | ||
|
|
||
| return CONTINUE; | ||
| } catch (Exception e) { | ||
| log.error("Exception in DLPInterceptor handleInternal: ", e); | ||
| return Outcome.ABORT; | ||
| } | ||
| } |
There was a problem hiding this comment.
Optimize analyzer usage and add input validation.
The method has performance and validation issues:
- The analyzer is called multiple times (lines 51 and 69)
- No validation of message content before processing
- Each transformation re-encodes the body which is inefficient
public Outcome handleInternal(Message msg) {
+ if (msg == null || msg.getBodyAsStringDecoded() == null || msg.getBodyAsStringDecoded().isEmpty()) {
+ return CONTINUE;
+ }
+
try {
- log.info("DLP Risk Analysis: {}", dlpAnalyzer.analyze(msg).getLogReport());
+ RiskReport riskReport = dlpAnalyzer.analyze(msg);
+ log.info("DLP Risk Analysis: {}", riskReport.getLogReport());
+
+ String body = msg.getBodyAsStringDecoded();
if (!masks.isEmpty()) {
for (Mask mask : masks) {
- msg.setBodyContent(mask.apply(msg.getBodyAsStringDecoded())
- .getBytes(StandardCharsets.UTF_8));
+ body = mask.apply(body);
}
}
if (!filters.isEmpty()) {
for (Filter filter : filters) {
- msg.setBodyContent(filter.apply(msg.getBodyAsStringDecoded())
- .getBytes(StandardCharsets.UTF_8));
+ body = filter.apply(body);
}
}
if (!reports.isEmpty()) {
for (Report report : reports) {
- msg.setBodyContent(report.apply(msg.getBodyAsStringDecoded(), dlpAnalyzer.analyze(msg)).getBytes(StandardCharsets.UTF_8));
+ body = report.apply(body, riskReport);
}
}
+
+ msg.setBodyContent(body.getBytes(StandardCharsets.UTF_8));
return CONTINUE;
} catch (Exception e) {
log.error("Exception in DLPInterceptor handleInternal: ", e);
return Outcome.ABORT;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public Outcome handleInternal(Message msg) { | |
| try { | |
| log.info("DLP Risk Analysis: {}", dlpAnalyzer.analyze(msg).getLogReport()); | |
| if (!masks.isEmpty()) { | |
| for (Mask mask : masks) { | |
| msg.setBodyContent(mask.apply(msg.getBodyAsStringDecoded()) | |
| .getBytes(StandardCharsets.UTF_8)); | |
| } | |
| } | |
| if (!filters.isEmpty()) { | |
| for (Filter filter : filters) { | |
| msg.setBodyContent(filter.apply(msg.getBodyAsStringDecoded()) | |
| .getBytes(StandardCharsets.UTF_8)); | |
| } | |
| } | |
| if (!reports.isEmpty()) { | |
| for (Report report : reports) { | |
| msg.setBodyContent(report.apply(msg.getBodyAsStringDecoded(), dlpAnalyzer.analyze(msg)).getBytes(StandardCharsets.UTF_8)); | |
| } | |
| } | |
| return CONTINUE; | |
| } catch (Exception e) { | |
| log.error("Exception in DLPInterceptor handleInternal: ", e); | |
| return Outcome.ABORT; | |
| } | |
| } | |
| public Outcome handleInternal(Message msg) { | |
| + // input validation | |
| + if (msg == null || msg.getBodyAsStringDecoded() == null || msg.getBodyAsStringDecoded().isEmpty()) { | |
| + return CONTINUE; | |
| + } | |
| try { | |
| - log.info("DLP Risk Analysis: {}", dlpAnalyzer.analyze(msg).getLogReport()); | |
| + // analyze once | |
| + RiskReport riskReport = dlpAnalyzer.analyze(msg); | |
| + log.info("DLP Risk Analysis: {}", riskReport.getLogReport()); | |
| + | |
| + // work off a single mutable body string | |
| + String body = msg.getBodyAsStringDecoded(); | |
| if (!masks.isEmpty()) { | |
| for (Mask mask : masks) { | |
| - msg.setBodyContent(mask.apply(msg.getBodyAsStringDecoded()) | |
| - .getBytes(StandardCharsets.UTF_8)); | |
| + body = mask.apply(body); | |
| } | |
| } | |
| if (!filters.isEmpty()) { | |
| for (Filter filter : filters) { | |
| - msg.setBodyContent(filter.apply(msg.getBodyAsStringDecoded()) | |
| - .getBytes(StandardCharsets.UTF_8)); | |
| + body = filter.apply(body); | |
| } | |
| } | |
| if (!reports.isEmpty()) { | |
| for (Report report : reports) { | |
| - msg.setBodyContent(report.apply(msg.getBodyAsStringDecoded(), dlpAnalyzer.analyze(msg)).getBytes(StandardCharsets.UTF_8)); | |
| + body = report.apply(body, riskReport); | |
| } | |
| } | |
| + | |
| + // apply all transformations at once | |
| + msg.setBodyContent(body.getBytes(StandardCharsets.UTF_8)); | |
| return CONTINUE; | |
| } catch (Exception e) { | |
| log.error("Exception in DLPInterceptor handleInternal: ", e); | |
| return Outcome.ABORT; | |
| } | |
| } |
🤖 Prompt for AI Agents
In
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPInterceptor.java
between lines 49 and 78, optimize by calling dlpAnalyzer.analyze(msg) once and
reusing its result to avoid redundant processing. Add validation to check if the
message body is non-null and non-empty before applying masks, filters, or
reports. Instead of repeatedly decoding and re-encoding the message body for
each transformation, decode once, apply all transformations sequentially on the
decoded string, then encode and set the body content once at the end.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Action.java (1)
13-16: Add validation for the field parameter.The setter should validate that the field is not empty and is a valid JSONPath expression to prevent runtime errors.
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/CsvFieldConfiguration.java (1)
39-39: Consider using a proper CSV parsing library for robustness.The current
line.split(",", -1)approach doesn't handle quoted fields containing commas, which could lead to parsing errors with more complex CSV files.Consider using Apache Commons CSV or OpenCSV for robust CSV parsing:
-String[] parts = line.split(",", -1); +// Use proper CSV parsing library to handle quoted fieldscore/src/main/java/com/predic8/membrane/core/interceptor/dlp/Mask.java (2)
14-24: Add input validation and improve error handling.The method has several issues:
- No validation for null or empty JSON input
Integer.parseInt(keepRight)can throwNumberFormatException- No validation that the field exists before reading
Apply this diff to improve the method:
public String apply(String json, DLPContext context) { + if (json == null || json.trim().isEmpty()) { + return json; + } + try { + int keepRightValue = Integer.parseInt(keepRight); DocumentContext doc = JsonPath.parse(json); - String original = doc.read(getField(), String.class); + String original = doc.read(getField(), String.class); + if (original == null) { + return doc.jsonString(); + } - String masked = maskKeepRight(original, Integer.parseInt(keepRight)); + String masked = maskKeepRight(original, keepRightValue); doc.set(getField(), masked); return doc.jsonString(); + } catch (NumberFormatException e) { + throw new IllegalArgumentException("keepRight must be a valid integer: " + keepRight, e); } catch (Exception e) { throw new RuntimeException("Failed to apply mask on field: " + getField(), e); } }
35-38: Add validation for keepRight parameter.The setter should validate that
keepRightis a valid non-negative integer to prevent runtime errors.Apply this diff to add validation:
@MCAttribute public void setKeepRight(String keepRight) { + if (keepRight != null) { + try { + int value = Integer.parseInt(keepRight); + if (value < 0) { + throw new IllegalArgumentException("keepRight must be non-negative: " + keepRight); + } + } catch (NumberFormatException e) { + throw new IllegalArgumentException("keepRight must be a valid integer: " + keepRight, e); + } + } this.keepRight = keepRight; }
🧹 Nitpick comments (3)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPAnalyzer.java (2)
42-51: Exception handling could be more specific.The broad exception catch wraps all exceptions as RuntimeException, which makes debugging difficult and loses specific error context.
public RiskReport analyze(Message msg) { - try (InputStream is = msg.getBodyAsStreamDecoded()) { + try (InputStream is = msg.getBodyAsStreamDecoded()) { JsonNode root = MAPPER.readTree(is); RiskReport report = new RiskReport(); traverse(root, new ArrayDeque<>(), report); return report; + } catch (IOException e) { + throw new RuntimeException("Failed to parse JSON message", e); + } catch (Exception e) { + throw new RuntimeException("Failed to analyze message", e); - } catch (Exception e) { - throw new RuntimeException("Failed to analyze message", e); } }
68-69: Risk classification should use enum value directly.The code converts the Category enum back to string with
.name()when callingreport.recordField(), but RiskReport could accept the enum directly for better type safety.Consider updating RiskReport.recordField to accept Category enum directly:
RiskReport.Category level = classify(fullPath, lastSegment); -report.recordField(fullPath, level.name()); +report.recordField(fullPath, level);core/src/main/java/com/predic8/membrane/core/interceptor/dlp/RiskReport.java (1)
7-7: Unused constant should be removed.The
LEVELSconstant is declared but never used in the code.-private static final List<String> LEVELS = List.of("high", "medium", "low", "unclassified");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Action.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/CsvFieldConfiguration.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPAction.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPAnalyzer.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPContext.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPInterceptor.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Filter.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Mask.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Report.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/RiskReport.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPContext.java
🚧 Files skipped from review as they are similar to previous changes (2)
- core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Filter.java
- core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPInterceptor.java
🧰 Additional context used
🧬 Code Graph Analysis (2)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Report.java (3)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPInterceptor.java (1)
MCElement(20-113)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Filter.java (1)
MCElement(11-35)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Mask.java (1)
MCElement(8-39)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPAnalyzer.java (1)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/RiskReport.java (1)
RiskReport(5-71)
⏰ 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). (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (10)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPAction.java (1)
3-5: Well-designed interface following good practices.The interface provides a clean contract for DLP actions with a clear method signature and follows the single responsibility principle.
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Report.java (1)
18-54: Well-implemented risk metadata injection with proper validation.The implementation correctly:
- Validates DLPContext and field parameters with appropriate warning logs
- Handles JSONPath operations safely
- Provides clear error messages and exception handling
- Follows good logging practices
The current implementation addresses validation and error handling concerns effectively.
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPAnalyzer.java (4)
16-23: JSON parsing configuration is well-configured for security.The JSON factory configuration appropriately restricts nesting depth (64) and string length (16KB) to prevent DoS attacks, while sensibly allowing trailing commas but disallowing unescaped control characters.
33-40: Risk dictionary mapping handles invalid values gracefully.The mapping correctly normalizes keys to lowercase and uses
RiskReport.Category.fromString()which defaults to UNCLASSIFIED for invalid values.
65-69: Critical NoSuchElementException bug has been properly fixed.The previous issue with
path.getLast()causing NoSuchElementException for root-level arrays/primitives has been correctly resolved by usingpath.peekLast()with null-safe handling.
73-77: Classification logic is efficient and correct.The two-level lookup (full path first, then simple name) with Optional chaining provides good flexibility while maintaining performance.
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/RiskReport.java (4)
9-11: Efficient data structure choices for risk tracking.Using LinkedHashMap for field order preservation and EnumMap for efficient enum-based storage is a good design choice.
27-48: Category enum implementation is well-designed.The enum with severity levels, proper string conversion, and switch expression provides a clean API for risk level handling.
50-62: Structured report generation is comprehensive.The method properly handles all risk categories and provides both counts and detailed field information in a well-structured format.
64-70: Accessor methods provide proper encapsulation.The methods return unmodifiable views of internal collections, preventing external modification while allowing safe access.
| public DLPAnalyzer(Map<String, String> rawRiskMap) { | ||
| this.riskDict = mapToEnumRiskLevels(rawRiskMap); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Constructor should validate input parameter.
The constructor accepts a raw risk map but doesn't validate for null input, which could cause a NullPointerException during processing.
public DLPAnalyzer(Map<String, String> rawRiskMap) {
+ if (rawRiskMap == null) {
+ throw new IllegalArgumentException("Risk map cannot be null");
+ }
this.riskDict = mapToEnumRiskLevels(rawRiskMap);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public DLPAnalyzer(Map<String, String> rawRiskMap) { | |
| this.riskDict = mapToEnumRiskLevels(rawRiskMap); | |
| } | |
| public DLPAnalyzer(Map<String, String> rawRiskMap) { | |
| if (rawRiskMap == null) { | |
| throw new IllegalArgumentException("Risk map cannot be null"); | |
| } | |
| this.riskDict = mapToEnumRiskLevels(rawRiskMap); | |
| } |
🤖 Prompt for AI Agents
In core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPAnalyzer.java
around lines 29 to 31, the constructor accepts a rawRiskMap parameter but does
not validate it for null, risking a NullPointerException. Add a null check at
the start of the constructor to verify rawRiskMap is not null, and if it is,
throw an IllegalArgumentException or handle it appropriately to prevent
downstream errors.
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/RiskReport.java
Outdated
Show resolved
Hide resolved
| public void recordField(String field, String riskLevel) { | ||
| matchedFields.put(field, riskLevel); | ||
| Category category = Category.fromString(riskLevel); | ||
| riskCounts.merge(category, 1, Integer::sum); | ||
| riskDetails.computeIfAbsent(category, r -> new LinkedHashMap<>()).merge(field, 1, Integer::sum); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
recordField method should validate input parameters.
The method doesn't validate for null inputs, which could cause NullPointerException during processing.
public void recordField(String field, String riskLevel) {
+ if (field == null) {
+ throw new IllegalArgumentException("Field cannot be null");
+ }
+ if (riskLevel == null) {
+ throw new IllegalArgumentException("Risk level cannot be null");
+ }
matchedFields.put(field, riskLevel);
Category category = Category.fromString(riskLevel);
riskCounts.merge(category, 1, Integer::sum);
riskDetails.computeIfAbsent(category, r -> new LinkedHashMap<>()).merge(field, 1, Integer::sum);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public void recordField(String field, String riskLevel) { | |
| matchedFields.put(field, riskLevel); | |
| Category category = Category.fromString(riskLevel); | |
| riskCounts.merge(category, 1, Integer::sum); | |
| riskDetails.computeIfAbsent(category, r -> new LinkedHashMap<>()).merge(field, 1, Integer::sum); | |
| } | |
| public void recordField(String field, String riskLevel) { | |
| if (field == null) { | |
| throw new IllegalArgumentException("Field cannot be null"); | |
| } | |
| if (riskLevel == null) { | |
| throw new IllegalArgumentException("Risk level cannot be null"); | |
| } | |
| matchedFields.put(field, riskLevel); | |
| Category category = Category.fromString(riskLevel); | |
| riskCounts.merge(category, 1, Integer::sum); | |
| riskDetails.computeIfAbsent(category, r -> new LinkedHashMap<>()).merge(field, 1, Integer::sum); | |
| } |
🤖 Prompt for AI Agents
In core/src/main/java/com/predic8/membrane/core/interceptor/dlp/RiskReport.java
around lines 13 to 18, the recordField method lacks validation for null inputs,
risking NullPointerException. Add checks at the start of the method to verify
that both field and riskLevel are not null, and handle null cases appropriately,
such as throwing IllegalArgumentException or returning early to prevent further
processing with invalid inputs.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Action.java (1)
13-16: Add validation for the field parameter.The setter should validate that the field is not empty and is a valid JSONPath expression to prevent runtime errors.
@MCAttribute public void setField(String field) { + if (field != null && field.trim().isEmpty()) { + throw new IllegalArgumentException("field cannot be empty"); + } this.field = field; }
🧹 Nitpick comments (2)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Report.java (1)
24-27: Consider using more efficient string building for matched fields.The current approach creates a new StringBuilder and appends in a lambda, which is less readable than a traditional loop.
if (!report.getMatchedFields().isEmpty()) { StringBuilder sb = new StringBuilder("Matched fields:"); - report.getMatchedFields().forEach((field, category) -> - sb.append("\n - ").append(field).append(" -> ").append(category)); + for (Map.Entry<String, Object> entry : report.getMatchedFields().entrySet()) { + sb.append("\n - ").append(entry.getKey()).append(" -> ").append(entry.getValue()); + } log.info(sb.toString()); }core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPContext.java (1)
3-24: Consider adding builder pattern for future extensibility.As the DLP framework evolves, you may need to add more context fields. A builder pattern would make the class more maintainable and extensible.
public class DLPContext { private final String body; private final RiskReport riskReport; + public static class Builder { + private String body; + private RiskReport riskReport; + + public Builder body(String body) { + this.body = body; + return this; + } + + public Builder riskReport(RiskReport riskReport) { + this.riskReport = riskReport; + return this; + } + + public DLPContext build() { + return new DLPContext(body, riskReport); + } + } + + public static Builder builder() { + return new Builder(); + } + public DLPContext(String body, RiskReport riskReport) { this.body = Objects.requireNonNull(body, "body cannot be null"); this.riskReport = riskReport; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Action.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPAction.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPContext.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPInterceptor.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Filter.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Mask.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Report.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Filter.java
- core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Mask.java
- core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPInterceptor.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). (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (7)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPAction.java (3)
3-5: Clean and well-designed interface.The interface follows good design principles with a single, focused responsibility and clear method signature. The contract is simple and effective for the DLP action pattern.
3-5: Clean interface design following command pattern.The interface is well-designed with a clear contract. The single
applymethod withDLPContextparameter andStringreturn type provides a consistent contract for all DLP actions.
3-5: Well-designed interface following the Strategy pattern.The interface provides a clean contract for DLP actions with a focused single responsibility. The method signature appropriately accepts context and returns transformed content.
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Report.java (2)
20-30: Well-implemented reporting logic.The risk report logging is well-structured with clear formatting and appropriate log levels. The method correctly returns the original body without modification, which is the expected behavior for a reporting action.
20-29: Well-implemented logging functionality.The risk report logging is well-structured:
- Appropriate log levels for different information types
- Efficient StringBuilder usage for matched fields
- Clean lambda expression for field iteration
- Correctly returns original body unchanged
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPContext.java (2)
3-24: Well-designed immutable context holder.The class follows good design principles with final fields creating an immutable data structure. The
hasRiskReport()convenience method is a nice touch for null checking.
13-23: Well-implemented data accessor methods.The getter methods and utility method are clean and appropriate:
- Clear method names and return types
- Useful
hasRiskReport()utility for null checking- Simple and focused implementation
| if (context == null || !context.hasRiskReport()) { | ||
| log.warn("No RiskReport provided. Skipping report logging."); | ||
| return Objects.requireNonNull(context).getBody(); | ||
| } |
There was a problem hiding this comment.
Fix potential NullPointerException in error handling.
The null check logic has a flaw - if context is null, Objects.requireNonNull(context) will throw a NullPointerException rather than safely returning a value.
if (context == null || !context.hasRiskReport()) {
log.warn("No RiskReport provided. Skipping report logging.");
- return Objects.requireNonNull(context).getBody();
+ return context != null ? context.getBody() : "";
}Alternatively, consider throwing a more descriptive exception for null context:
if (context == null || !context.hasRiskReport()) {
+ if (context == null) {
+ throw new IllegalArgumentException("DLPContext cannot be null");
+ }
log.warn("No RiskReport provided. Skipping report logging.");
- return Objects.requireNonNull(context).getBody();
+ return context.getBody();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (context == null || !context.hasRiskReport()) { | |
| log.warn("No RiskReport provided. Skipping report logging."); | |
| return Objects.requireNonNull(context).getBody(); | |
| } | |
| if (context == null || !context.hasRiskReport()) { | |
| - return Objects.requireNonNull(context).getBody(); | |
| + if (context == null) { | |
| + throw new IllegalArgumentException("DLPContext cannot be null"); | |
| + } | |
| + log.warn("No RiskReport provided. Skipping report logging."); | |
| + return context.getBody(); | |
| } |
🤖 Prompt for AI Agents
In core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Report.java
around lines 16 to 19, the code calls Objects.requireNonNull(context) after
checking if context is null, which will throw a NullPointerException if context
is null. To fix this, avoid calling Objects.requireNonNull(context) when context
is null; instead, handle the null case explicitly by either returning a default
value or throwing a descriptive exception before accessing context.getBody().
| public DLPContext(String body, RiskReport riskReport) { | ||
| this.riskReport = riskReport; | ||
| this.body = body; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and fix parameter order inconsistency.
The constructor lacks validation and has inconsistent parameter ordering compared to field declaration.
public DLPContext(String body, RiskReport riskReport) {
+ if (body == null) {
+ throw new IllegalArgumentException("body cannot be null");
+ }
- this.riskReport = riskReport;
this.body = body;
+ this.riskReport = riskReport;
}The parameter order should match the field declaration order for consistency and readability.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public DLPContext(String body, RiskReport riskReport) { | |
| this.riskReport = riskReport; | |
| this.body = body; | |
| } | |
| public DLPContext(String body, RiskReport riskReport) { | |
| if (body == null) { | |
| throw new IllegalArgumentException("body cannot be null"); | |
| } | |
| this.body = body; | |
| this.riskReport = riskReport; | |
| } |
🤖 Prompt for AI Agents
In core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPContext.java
around lines 8 to 11, the constructor parameters are in a different order than
the field declarations and lack input validation. To fix this, reorder the
constructor parameters to match the field declaration order, placing the body
parameter first followed by riskReport. Additionally, add validation checks to
ensure that neither parameter is null or invalid before assigning them to the
fields.
🛠️ Refactor suggestion
Consider adding constructor validation for required parameters.
The constructor should validate that the body parameter is not null, as it's likely a required field for DLP processing.
public DLPContext(String body, RiskReport riskReport) {
+ if (body == null) {
+ throw new IllegalArgumentException("body cannot be null");
+ }
this.riskReport = riskReport;
this.body = body;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public DLPContext(String body, RiskReport riskReport) { | |
| this.riskReport = riskReport; | |
| this.body = body; | |
| } | |
| public DLPContext(String body, RiskReport riskReport) { | |
| if (body == null) { | |
| throw new IllegalArgumentException("body cannot be null"); | |
| } | |
| this.riskReport = riskReport; | |
| this.body = body; | |
| } |
🤖 Prompt for AI Agents
In core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPContext.java
around lines 8 to 11, the constructor does not validate the body parameter which
is likely required. Add a null check for the body parameter at the start of the
constructor and throw an IllegalArgumentException if it is null to ensure the
object is always created with valid data.
🛠️ Refactor suggestion
Add null validation in constructor.
The constructor should validate parameters to prevent null pointer exceptions in downstream code.
public DLPContext(String body, RiskReport riskReport) {
+ this.body = Objects.requireNonNull(body, "body cannot be null");
- this.riskReport = riskReport;
- this.body = body;
+ this.riskReport = riskReport; // riskReport can be null as indicated by hasRiskReport()
}🤖 Prompt for AI Agents
In core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPContext.java
around lines 8 to 11, the constructor lacks null checks for its parameters. Add
validation to ensure that neither the body nor the riskReport parameters are
null. If either is null, throw an IllegalArgumentException with a clear message
indicating which parameter is missing. This prevents null pointer exceptions
later in the code.
| if (context == null || !context.hasRiskReport()) { | ||
| log.warn("No RiskReport provided. Skipping report logging."); | ||
| return Objects.requireNonNull(context).getBody(); |
There was a problem hiding this comment.
Fix potential NullPointerException in error handling.
The logic has a flaw: if context is null, the method logs a warning but then calls Objects.requireNonNull(context).getBody() which will throw a NPE.
Apply this fix:
if (context == null || !context.hasRiskReport()) {
log.warn("No RiskReport provided. Skipping report logging.");
- return Objects.requireNonNull(context).getBody();
+ return context != null ? context.getBody() : null;
}Alternatively, handle null context separately:
+if (context == null) {
+ log.warn("DLPContext is null. Skipping report logging.");
+ return null;
+}
if (!context.hasRiskReport()) {
log.warn("No RiskReport provided. Skipping report logging.");
return context.getBody();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (context == null || !context.hasRiskReport()) { | |
| log.warn("No RiskReport provided. Skipping report logging."); | |
| return Objects.requireNonNull(context).getBody(); | |
| if (context == null || !context.hasRiskReport()) { | |
| log.warn("No RiskReport provided. Skipping report logging."); | |
| return context != null ? context.getBody() : null; | |
| } |
🤖 Prompt for AI Agents
In core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Report.java
around lines 16 to 18, the code calls Objects.requireNonNull(context).getBody()
even when context is null, causing a NullPointerException. To fix this, separate
the null check and handle the null context case explicitly by returning null or
an appropriate default value before calling any methods on context. This
prevents the NPE and ensures safe error handling.
| public String apply(DLPContext context) { | ||
| if (context == null || !context.hasRiskReport()) { | ||
| log.warn("No RiskReport provided. Skipping report logging."); | ||
| return Objects.requireNonNull(context).getBody(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve null handling logic.
The null check and Objects.requireNonNull() usage creates a potential NPE risk. If context is null, the method will still try to call context.getBody() after the null check.
if (context == null || !context.hasRiskReport()) {
log.warn("No RiskReport provided. Skipping report logging.");
- return Objects.requireNonNull(context).getBody();
+ return context != null ? context.getBody() : "";
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public String apply(DLPContext context) { | |
| if (context == null || !context.hasRiskReport()) { | |
| log.warn("No RiskReport provided. Skipping report logging."); | |
| return Objects.requireNonNull(context).getBody(); | |
| } | |
| public String apply(DLPContext context) { | |
| if (context == null || !context.hasRiskReport()) { | |
| log.warn("No RiskReport provided. Skipping report logging."); | |
| - return Objects.requireNonNull(context).getBody(); | |
| + return context != null ? context.getBody() : ""; | |
| } | |
| // … rest of method unchanged … |
🤖 Prompt for AI Agents
In core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Report.java
around lines 15 to 19, the method applies a null check on context but then calls
context.getBody() unconditionally, risking a NullPointerException. To fix this,
separate the null check from the call to getBody(): if context is null, return a
safe default value or handle accordingly without calling getBody(); only call
getBody() when context is confirmed non-null. Remove the use of
Objects.requireNonNull() here to avoid the NPE risk.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPAnalyzer.java (1)
30-33: Add input validation for constructor parameters.The constructor lacks null validation for both
rawRiskMapandcategoryMapparameters, which could cause NullPointerException during processing.Add validation to prevent NPE:
public DLPAnalyzer(Map<String, String> rawRiskMap, Map<String, String> categoryMap) { + if (rawRiskMap == null) { + throw new IllegalArgumentException("Risk map cannot be null"); + } + if (categoryMap == null) { + throw new IllegalArgumentException("Category map cannot be null"); + } this.riskDict = mapToEnumRiskLevels(rawRiskMap); this.categoryMap = categoryMap; }core/src/main/java/com/predic8/membrane/core/interceptor/dlp/RiskReport.java (1)
14-21: Add input validation for recordField method parameters.The method lacks null validation for its parameters, which could cause NullPointerException during processing.
Add validation to prevent NPE:
public void recordField(String field, String riskLevel, String category) { + if (field == null) { + throw new IllegalArgumentException("Field cannot be null"); + } + if (riskLevel == null) { + throw new IllegalArgumentException("Risk level cannot be null"); + } + if (category == null) { + throw new IllegalArgumentException("Category cannot be null"); + } matchedFields.put(field, riskLevel); fieldCategories.put(field, category);
🧹 Nitpick comments (1)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/RiskReport.java (1)
7-7: Remove unused LEVELS field.The
LEVELSfield is defined but not used anywhere in the code. Consider removing it to reduce code clutter.- private static final List<String> LEVELS = List.of("high", "medium", "low", "unclassified");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/CsvFieldConfiguration.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPAnalyzer.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPInterceptor.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Filter.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Mask.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Report.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/RiskReport.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Filter.java
- core/src/main/java/com/predic8/membrane/core/interceptor/dlp/CsvFieldConfiguration.java
- core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Mask.java
- core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPInterceptor.java
🧰 Additional context used
🧬 Code Graph Analysis (1)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPAnalyzer.java (1)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/RiskReport.java (1)
RiskReport(5-78)
⏰ 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). (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (5)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Report.java (2)
15-19: LGTM! Null handling has been properly addressed.The null handling logic has been correctly implemented, resolving the previous NPE concerns. The method now safely handles null context by returning
context.getBody()only when context is confirmed non-null.
27-42: Well-implemented field processing with proper error handling.The field processing logic correctly:
- Normalizes JSON paths by removing the
$.prefix and converting to lowercase- Provides sensible defaults ("UNCLASSIFIED" for risk level)
- Includes comprehensive logging for monitoring
- Handles exceptions gracefully without disrupting message flow
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPAnalyzer.java (2)
67-76: LGTM! NoSuchElementException issue has been resolved.The traverse method now correctly uses
path.peekLast()instead ofpath.getLast(), which safely handles empty paths by returning null. This addresses the previous critical bug with root-level arrays and primitives.
16-25: Well-configured JSON factory with appropriate security constraints.The JSON factory configuration includes sensible security constraints:
- Maximum nesting depth (64) prevents stack overflow attacks
- Maximum string length (16KiB) prevents memory exhaustion
- Proper handling of trailing commas and control characters
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/RiskReport.java (1)
27-32: LGTM! Enum key usage has been corrected.The getCategory method now correctly uses Category enum values as keys instead of string literals, resolving the previous bug where conditions were always false.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPInterceptor.java (2)
62-65: Consider whether response processing is needed for comprehensive DLP coverage.The interceptor currently only processes requests. Depending on your DLP requirements, you might also want to process responses to prevent sensitive data from being returned to clients.
If response processing is needed, consider adding:
+@Override +public Outcome handleResponse(Exchange exc) { + return handleInternal(exc.getResponse()); +}
127-130: Fix inconsistent return type for fluent interface.The
setReports()method should returnthisto maintain consistency with the other setter methods and support method chaining.Apply this diff to fix the inconsistency:
@MCChildElement(order = 2) -public void setReports(List<Report> reports) { +public DLPInterceptor setReports(List<Report> reports) { this.reports = reports; + return this; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPInterceptor.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Report.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Report.java
🧰 Additional context used
🧬 Code Graph Analysis (1)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPInterceptor.java (6)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Mask.java (1)
MCElement(23-67)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Filter.java (1)
MCElement(24-53)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Report.java (1)
MCElement(21-58)core/src/main/java/com/predic8/membrane/core/interceptor/dlp/Field.java (1)
MCElement(6-19)core/src/main/java/com/predic8/membrane/core/exchange/Exchange.java (1)
Exchange(30-235)core/src/main/java/com/predic8/membrane/core/interceptor/AbstractInterceptor.java (1)
AbstractInterceptor(29-148)
⏰ 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). (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (6)
core/src/main/java/com/predic8/membrane/core/interceptor/dlp/DLPInterceptor.java (6)
1-18: Package declaration and imports look good.The imports are well-organized and all appear to be necessary for the DLP interceptor functionality.
20-44: Well-structured class declaration and field organization.The class properly follows Membrane framework conventions with appropriate annotations, comprehensive JavaDoc, and well-organized field declarations. The use of
finalfor theactionslist is a good practice for immutability.
46-60: Excellent implementation addressing previous performance concerns.The
init()method properly initializes theDLPAnalyzeronce during startup rather than on each request, which significantly improves performance. The handling of both configured and unconfigured scenarios is well-implemented.
67-88: Excellent implementation addressing all previous concerns.The
handleInternal()method now properly:
- Validates that actions are configured before processing
- Efficiently processes the message body once and applies all transformations
- Includes comprehensive error handling
- Uses proper UTF-8 encoding
- Returns appropriate outcomes
This addresses all the performance and error handling issues raised in previous reviews.
90-101: Clean configuration property implementation.The
fieldsConfiggetter and setter are well-implemented with appropriate annotations and helpful documentation including a usage example.
103-121: Well-implemented child element accessors.The getters and setters for masks and filters properly follow Membrane framework conventions with appropriate annotations. The fluent interface pattern (returning
this) is a nice touch for configuration chaining.
|
/ok-to-test |
Summary by CodeRabbit
New Features
Tests