Add default masking for request and response body fields #22
Conversation
WalkthroughThe changes introduce a new data transfer object for request logging, centralize and enhance masking logic for request and response data, and defer serialization until log writing. Configuration now supports specifying JSON body field masking patterns. Test coverage is expanded to verify exclusion and masking behavior, with new helper methods and more granular assertions. Changes
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #22 +/- ##
============================================
+ Coverage 75.44% 78.08% +2.63%
- Complexity 281 313 +32
============================================
Files 30 31 +1
Lines 1006 1054 +48
Branches 128 133 +5
============================================
+ Hits 759 823 +64
+ Misses 167 157 -10
+ Partials 80 74 -6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/main/java/io/apitally/common/RequestLogger.java (1)
464-479: Fix compilation error inskipEmptyValuesmethod.Same issue with
properties()method here.Apply this fix:
private ObjectNode skipEmptyValues(ObjectNode node) { ObjectNode result = objectMapper.createObjectNode(); - node.properties().forEach(entry -> { + node.fields().forEachRemaining(entry -> { if (entry.getValue().isNull()) { return; } if (entry.getValue().isArray() && entry.getValue().size() == 0) { return; } if (entry.getValue().isTextual() && entry.getValue().asText().isEmpty()) { return; } result.set(entry.getKey(), entry.getValue()); }); return result; }
🧹 Nitpick comments (2)
src/test/java/io/apitally/common/RequestLoggerTest.java (1)
361-428: Excellent test coverage for JSON body field masking.The test thoroughly validates the masking behavior with nested objects and arrays. Good attention to detail in verifying both masked and non-masked fields.
Consider extracting the large JSON strings to constants or a test fixture file for better readability:
+ private static final String TEST_REQUEST_BODY_JSON = "{\"username\":\"john_doe\",\"password\":\"secret123\"...}"; + private static final String TEST_RESPONSE_BODY_JSON = "{\"status\":\"success\",\"secret\":\"response_secret\"...}"; + @Test void testMaskBodyFields() { // ... setup code ... - String requestBodyJson = "{\"username\":\"john_doe\",\"password\":\"secret123\",\"token\":\"abc123\",\"custom\":\"xyz789\",\"user_id\":42,\"api_key\":123,\"normal_field\":\"value\",\"nested\":{\"password\":\"nested_secret\",\"count\":5,\"deeper\":{\"auth\":\"deep_token\"}},\"array\":[{\"password\":\"array_secret\",\"id\":1},{\"normal\":\"text\",\"token\":\"array_token\"}]}"; - String responseBodyJson = "{\"status\":\"success\",\"secret\":\"response_secret\",\"data\":{\"pwd\":\"response_pwd\"}}"; + String requestBodyJson = TEST_REQUEST_BODY_JSON; + String responseBodyJson = TEST_RESPONSE_BODY_JSON;src/main/java/io/apitally/common/RequestLogger.java (1)
70-78: Good addition of default body field masking patterns.The default patterns cover common sensitive field names including passwords, tokens, authentication fields, and financial data.
Consider adding a few more common sensitive field patterns:
private static final List<String> MASK_BODY_FIELD_PATTERNS = Arrays.asList( "password", "pwd", "token", "secret", "auth", "card[-_ ]?number", "ccv", - "ssn"); + "ssn", + "api[-_ ]?key", + "private[-_ ]?key", + "access[-_ ]?token", + "refresh[-_ ]?token");Also applies to: 111-112, 387-390
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/io/apitally/common/RequestLogger.java(11 hunks)src/main/java/io/apitally/common/RequestLoggingConfig.java(2 hunks)src/main/java/io/apitally/common/dto/RequestLogItem.java(1 hunks)src/test/java/io/apitally/common/RequestLoggerTest.java(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/java/io/apitally/common/RequestLogger.java (4)
src/main/java/io/apitally/common/dto/ExceptionDto.java (1)
ExceptionDto(3-25)src/main/java/io/apitally/common/dto/Header.java (1)
Header(3-19)src/main/java/io/apitally/common/dto/Request.java (1)
Request(3-43)src/main/java/io/apitally/common/dto/RequestLogItem.java (1)
RequestLogItem(7-39)
🪛 GitHub Actions: Tests
src/main/java/io/apitally/common/RequestLogger.java
[error] 433-433: Compilation error: cannot find symbol method 'properties()' in variable 'objectNode' of type com.fasterxml.jackson.databind.node.ObjectNode
[error] 466-466: Compilation error: cannot find symbol method 'properties()' in variable 'node' of type com.fasterxml.jackson.databind.node.ObjectNode
🔇 Additional comments (5)
src/main/java/io/apitally/common/RequestLoggingConfig.java (1)
16-16: LGTM! Clean implementation of body field masking configuration.The new
bodyFieldMaskPatternsfield and its accessor methods follow the established pattern used for other masking configurations in this class.Also applies to: 92-98
src/main/java/io/apitally/common/dto/RequestLogItem.java (1)
1-39: LGTM! Well-structured DTO for request logging.The implementation follows best practices:
- Immutable fields with proper encapsulation
- UUID generation for unique identification
- Correct use of Jackson annotations for JSON serialization
src/test/java/io/apitally/common/RequestLoggerTest.java (2)
45-107: Good refactoring of the end-to-end test.The test now properly validates the structure of logged data and uses the helper method for file reading. The assertions are comprehensive and check all relevant fields.
430-452: Clean helper method implementation.The helper method properly handles file operations and JSON parsing.
src/main/java/io/apitally/common/RequestLogger.java (1)
178-244: Well-structured masking implementation.The
applyMaskingmethod effectively centralizes all masking logic with proper handling of:
- User-provided masking callbacks
- Size limits with clear messages
- JSON body field masking
- Header and query parameter masking
The deferred masking approach improves performance by avoiding unnecessary processing for requests that may be dropped from the queue.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/main/java/io/apitally/common/RequestLogger.java (1)
433-433: Compilation error fix confirmed.The use of
fields().forEachRemaining()correctly addresses the previous compilation error. The ObjectNode API providesfields()method that returns an Iterator of Map.Entry objects.
🧹 Nitpick comments (3)
src/main/java/io/apitally/common/RequestLogger.java (3)
70-78: Consider making body field masking patterns configurable.The default patterns are comprehensive and security-focused. However, consider adding these as defaults to the configuration class to allow runtime customization while maintaining backward compatibility.
- private static final List<String> MASK_BODY_FIELD_PATTERNS = Arrays.asList( - "password", - "pwd", - "token", - "secret", - "auth", - "card[-_ ]?number", - "ccv", - "ssn"); + private static final List<String> DEFAULT_MASK_BODY_FIELD_PATTERNS = Arrays.asList( + "password", + "pwd", + "token", + "secret", + "auth", + "card[-_ ]?number", + "ccv", + "ssn");Then update the pattern compilation to use
DEFAULT_MASK_BODY_FIELD_PATTERNSas the default list.
189-191: Consider extracting duplicate body size validation.The body size checking logic is duplicated for both request and response bodies. Consider extracting this into a helper method for better maintainability.
+ private void validateAndTruncateBodySize(RequestResponseBase requestOrResponse) { + if (requestOrResponse.getBody() != null && requestOrResponse.getBody().length > MAX_BODY_SIZE) { + requestOrResponse.setBody(BODY_TOO_LARGE); + } + } if (request.getBody() != null) { // Apply user-provided masking callback for request body if (config.getCallbacks() != null) { byte[] maskedBody = config.getCallbacks().maskRequestBody(request); request.setBody(maskedBody != null ? maskedBody : BODY_MASKED); } - if (request.getBody().length > MAX_BODY_SIZE) { - request.setBody(BODY_TOO_LARGE); - } + validateAndTruncateBodySize(request);Also applies to: 207-209
238-242: Handle URL reconstruction exceptions more explicitly.While the malformed URL catch is good, consider logging the exception for debugging purposes, especially in development environments.
} catch (MalformedURLException e) { - // Keep original URL if malformed + logger.debug("Failed to reconstruct URL for query param masking: {}", e.getMessage()); + // Keep original URL if malformed }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/io/apitally/common/RequestLogger.java(10 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/java/io/apitally/common/RequestLogger.java (4)
src/main/java/io/apitally/common/dto/ExceptionDto.java (1)
ExceptionDto(3-25)src/main/java/io/apitally/common/dto/Header.java (1)
Header(3-19)src/main/java/io/apitally/common/dto/Request.java (1)
Request(3-43)src/main/java/io/apitally/common/dto/RequestLogItem.java (1)
RequestLogItem(7-39)
🪛 GitHub Check: codecov/patch
src/main/java/io/apitally/common/RequestLogger.java
[warning] 190-190: src/main/java/io/apitally/common/RequestLogger.java#L190
Added line #L190 was not covered by tests
[warning] 208-208: src/main/java/io/apitally/common/RequestLogger.java#L208
Added line #L208 was not covered by tests
[warning] 240-240: src/main/java/io/apitally/common/RequestLogger.java#L240
Added line #L240 was not covered by tests
[warning] 425-426: src/main/java/io/apitally/common/RequestLogger.java#L425-L426
Added lines #L425 - L426 were not covered by tests
🔇 Additional comments (8)
src/main/java/io/apitally/common/RequestLogger.java (8)
44-44: LGTM: Improved JSON content type detection.The regex pattern provides more flexible JSON content type detection compared to exact string matching.
148-153: LGTM: Improved exclusion logic structure.The early return pattern for exclusion checks is clean and efficient. The callback-based exclusion check provides good extensibility.
162-168: LGTM: Clean separation of concerns.Creating the RequestLogItem upfront and deferring masking to
applyMasking()improves the architecture by separating logging from masking concerns.
178-244: Excellent centralization of masking logic.The
applyMasking()method successfully centralizes all masking operations, improving maintainability. The logic flow is clear: user callbacks → size limits → field-level masking → headers → query params.
419-443: LGTM: Robust JSON masking implementation.The JSON masking logic correctly handles the compilation error from the past review by using
fields().forEachRemaining(). The recursive approach properly handles nested objects and arrays, and the exception handling gracefully falls back to the original body.
451-454: LGTM: Focused JSON content type detection.The dedicated method with regex pattern matching provides more accurate JSON detection compared to the broader
hasSupportedContentType()method.
255-268: LGTM: Clean serialization with proper object composition.The writeToFile method now properly applies masking before serialization and uses Jackson's ObjectNode for clean JSON structure composition. The
skipEmptyValues()call helps reduce log size.
111-112: ```shell
#!/bin/bashSearch for the getBodyFieldMaskPatterns method in the codebase
rg -n "getBodyFieldMaskPatterns" -C3 .
</details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Summary by CodeRabbit
New Features
Refactor
Tests