CMM-1217 Redact sensitive data from mobile network logs#22572
CMM-1217 Redact sensitive data from mobile network logs#22572
Conversation
|
Claude finished @adalpari's task in 1m 23s —— View job Code Review Complete ✓Todo List:
SummaryOverall, this is a well-designed and secure implementation that effectively addresses the security concern of redacting sensitive credentials from network logs. The approach is clean, maintainable, and follows good architectural patterns. I've identified a few areas for improvement below. ✅ Strengths
🔍 Issues & Suggestions1. Security: Path Matching Could Be Bypassed (Medium Priority)Location: The current implementation uses private fun shouldRedactRequestBody(request: Request): Boolean {
val path = request.url.encodedPath
return REDACTED_BODY_PATHS.any { path.endsWith(it) }
}Concerns:
Recommendation: Consider using private fun shouldRedactRequestBody(request: Request): Boolean {
val path = request.url.encodedPath
return REDACTED_BODY_PATHS.any { sensitivePath ->
path.contains("/$sensitivePath") || path.endsWith(sensitivePath)
}
}Or even better, use the path segments: private fun shouldRedactRequestBody(request: Request): Boolean {
val pathSegments = request.url.pathSegments
return REDACTED_BODY_PATHS.any { it in pathSegments }
}2. Missing Unit Tests (High Priority)Location: No test file exists for This security-critical code lacks unit test coverage. Tests should verify:
Recommendation: Add unit tests at: 3. Potential NPE in RedactedBodyChain (Low Priority)Location: val contentType = original.body?.contentType()
?: "text/plain".toMediaType()While the fallback handles a null body gracefully, if Recommendation: Add a clarifying comment: // Use plain text content type if original has no body
// (the redacted placeholder still needs a content type)
val contentType = original.body?.contentType()
?: "text/plain".toMediaType()4. Documentation: Clarify Logging vs Network Behavior (Low Priority)Location: The class-level KDoc is excellent, but the Current: /**
* A [Interceptor.Chain] wrapper that presents a redacted request body
* to Chucker while ensuring the actual network call uses the original
* request with the real body.
*/Suggested Enhancement: /**
* A [Interceptor.Chain] wrapper that presents a redacted request body
* to Chucker for logging purposes, while ensuring the actual network call
* uses the original request with the real body intact.
*
* This achieves selective logging redaction without affecting the
* actual HTTP request/response.
*/5. Consider: HTTPS-Only Validation (Optional Enhancement)While not strictly required for this PR, consider validating that sensitive endpoints are only accessed over HTTPS. This could be a separate enhancement: private fun shouldRedactRequestBody(request: Request): Boolean {
val path = request.url.encodedPath
val isSensitive = REDACTED_BODY_PATHS.any { path.endsWith(it) }
// Optional: Log warning if sensitive endpoint accessed over HTTP
if (isSensitive && request.url.scheme != "https") {
// Log warning about insecure connection to sensitive endpoint
}
return isSensitive
}📋 Testing VerificationThe PR description provides comprehensive testing instructions. When testing, pay special attention to:
🎯 Recommendations Priority
ConclusionThis is solid work that addresses a real security concern. With unit tests and slightly more robust path matching, this will be production-ready. The core design pattern is excellent and the code is clean and maintainable. |
Generated by 🚫 Danger |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
libs/fluxc/src/main/java/org/wordpress/android/fluxc/network/TrackNetworkRequestsInterceptor.kt
Outdated
Show resolved
Hide resolved
libs/fluxc/src/main/java/org/wordpress/android/fluxc/network/TrackNetworkRequestsInterceptor.kt
Show resolved
Hide resolved
|
|
|
|
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #22572 +/- ##
==========================================
- Coverage 38.23% 38.22% -0.01%
==========================================
Files 2237 2237
Lines 111111 111138 +27
Branches 15544 15546 +2
==========================================
Hits 42486 42486
- Misses 65087 65114 +27
Partials 3538 3538 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
adalpari
deleted the
internal/CMM-1217-Redact-sensitive-data-from-mobile-network-logs
branch
4 participants
Description
Redact request bodies for sensitive endpoints (
xmlrpc.phpandwp-login.php) in the network request tracking feature (Chucker). These endpoints carry credentials in their request bodies, which should not be stored in the on-device log.The requests are still fully logged (URL, method, headers, status code, response) — only the request body is replaced with a
[Body redacted]placeholder. The actual network call is unaffected.This is achieved via a
RedactedBodyChainwrapper that presents a redacted request to Chucker while delegatingproceed()to the original chain with the real body intact.Adding more sensitive endpoints in the future only requires appending to
REDACTED_BODY_PATHS.Testing instructions
Redacted body for sensitive endpoints:
Network Debuggingexperimental featureTrack Network Requestsxmlrpc.phporwp-login.phprequest (e.g. open a self-hosted site and go to posts)View Network Requeststo open Chuckerxmlrpc.php/wp-login.phpentry[Body redacted — contains sensitive information]Non-sensitive endpoints unaffected:
View Network RequestsNetwork calls work correctly: