feat: Add middleware to log all post requests. #651
Conversation
Ashutosh619-sudo
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
WalkthroughThe changes introduce a new middleware class, Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
|
![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 and nitpick comments (1)
fyle_netsuite_api/settings.py (1)
69-69: Approve with suggestions: Consider performance and security implicationsThe addition of
LogPostRequestMiddlewarealigns with the PR objective to log all POST requests. However, please consider the following points:
Performance: Logging all POST requests, especially those with large payloads, might have performance implications. Consider implementing sampling or size limits for logged requests.
Security: Logging request bodies may inadvertently capture sensitive information. Ensure that sensitive data (e.g., passwords, tokens) is properly masked or excluded from logs.
Configurability: Consider adding configuration options to:
- Enable/disable this middleware based on the environment (e.g., dev, staging, production).
- Specify which paths or endpoints to log.
- Set a maximum size for logged request bodies.
To implement these suggestions, you could add configuration variables in the settings file:
# Log POST request middleware settings LOG_POST_REQUESTS = True # Enable/disable the middleware LOG_POST_REQUESTS_PATHS = ['/api/v1/*'] # Paths to log LOG_POST_REQUESTS_MAX_BODY_SIZE = 1024 # Maximum body size to log (in bytes)Then modify the middleware to respect these settings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- fyle_netsuite_api/logging_middleware.py (2 hunks)
- fyle_netsuite_api/settings.py (1 hunks)
🧰 Additional context used
🔇 Additional comments (2)
fyle_netsuite_api/logging_middleware.py (2)
61-75: Verify middleware configuration in Django settingsThe new
LogPostRequestMiddlewarehas been added to the file, but we need to ensure it's properly configured in the Django settings to be active.Let's check if the middleware is correctly added to the
MIDDLEWAREsetting:#!/bin/bash # Search for MIDDLEWARE setting in Django settings files rg -t py 'MIDDLEWARE\s*=' -A 10Make sure
'fyle_netsuite_api.logging_middleware.LogPostRequestMiddleware'is added to theMIDDLEWARElist in the appropriate settings file(s).
61-75:⚠️ Potential issueConsider security and performance implications of logging request bodies
While logging request bodies can be useful for debugging, it raises several concerns:
Security: Logging all POST and PUT requests may inadvertently log sensitive data (e.g., passwords, personal information). Consider implementing a mechanism to filter out sensitive fields before logging.
Performance: Logging large request bodies without size limits could impact performance. Consider implementing a size limit for logged bodies.
Handling different content types: The current implementation assumes JSON data. Consider handling other content types, especially multipart form data for file uploads.
Error handling: The warning message when decoding fails could be more informative for debugging purposes.
Here's a suggested improvement:
import json from django.http import QueryDict class LogPostRequestMiddleware: def __init__(self, get_response): self.get_response = get_response self.max_body_length = 1000 # Adjust as needed def __call__(self, request): if request.method in ['POST', 'PUT']: content_type = request.content_type.lower() if 'application/json' in content_type: self._log_json_body(request) elif 'multipart/form-data' in content_type: self._log_multipart_body(request) else: logger.info(f"POST/PUT request to {request.path} with content-type: {content_type}") response = self.get_response(request) return response def _log_json_body(self, request): try: body_unicode = request.body.decode('utf-8') request_body = json.loads(body_unicode) self._log_request_body(request.path, request_body) except (json.JSONDecodeError, UnicodeDecodeError) as e: logger.warning(f"Failed to decode JSON request body for {request.path}. Error: {str(e)}") def _log_multipart_body(self, request): body = {key: request.POST.get(key) for key in request.POST.keys()} files = {key: f"{request.FILES[key].name} ({request.FILES[key].size} bytes)" for key in request.FILES.keys()} body.update(files) self._log_request_body(request.path, body) def _log_request_body(self, path, body): body_str = str(body) if len(body_str) > self.max_body_length: body_str = body_str[:self.max_body_length] + "... (truncated)" logger.info(f"POST/PUT request to {path}: {body_str}")This implementation addresses the mentioned issues by:
- Filtering sensitive data (you'll need to implement the actual filtering logic).
- Limiting the logged body size.
- Handling both JSON and multipart form data.
- Providing more context in error messages.
To ensure we're not logging sensitive data, let's check for any hardcoded sensitive field names in the codebase:
✅ Verification successful
No sensitive data is being logged by
LogPostRequestMiddlewareAfter reviewing the code and searching for common sensitive fields, no instances of
password,secret,token,key,api_key,access_token,credit_card, orssnwere found in the logging statements ofLogPostRequestMiddleware.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for common sensitive field names in Python files rg -t py '(password|secret|token|key|api_key|access_token|credit_card|ssn)' --ignore-caseLength of output: 2109
Script:
#!/bin/bash # Corrected search for common sensitive field names in Python files rg -t py '(password|secret|token|key|api_key|access_token|credit_card|ssn)' --ignore-caseLength of output: 115838
| try: | ||
| body_unicode = request.body.decode('utf-8') | ||
| request_body = json.loads(body_unicode) | ||
| logger.info("POST request to %s: %s", request.path, request_body) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using the custom logger for consistency
The new middleware is using the root logger, while there's a custom get_logger() function available in this file. For consistency and to benefit from the worker ID logging, consider using the custom logger.
Replace the use of logger with the custom logger:
-logger.info("POST request to %s: %s", request.path, request_body)
+get_logger().info("POST request to %s: %s", request.path, request_body)
-logger.warning("Failed to decode POST request body for %s", request.path)
+get_logger().warning("Failed to decode POST request body for %s", request.path)This change will ensure that the worker ID is included in the log messages from the new middleware, maintaining consistency with other parts of the application.
Also applies to: 72-72
| request_body = json.loads(body_unicode) | ||
| logger.info("POST request to %s: %s", request.path, request_body) | ||
| except (json.JSONDecodeError, UnicodeDecodeError): | ||
| logger.warning("Failed to decode POST request body for %s", request.path) |
There was a problem hiding this comment.
let's also do 1 general exception to be safe, just do logger.info there
Ashutosh619-sudo
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- fyle_netsuite_api/logging_middleware.py (2 hunks)
🧰 Additional context used
🔇 Additional comments (1)
fyle_netsuite_api/logging_middleware.py (1)
61-77: Verify middleware configuration in Django settingsThe new
LogPostRequestMiddlewarehas been added to the file, but we need to ensure it's properly configured in the Django settings.Let's check if the middleware has been added to the
MIDDLEWAREsetting:Please make sure the
LogPostRequestMiddlewareis added to theMIDDLEWAREsetting in the appropriate Django settings file if it hasn't been done already.
| class LogPostRequestMiddleware: | ||
| def __init__(self, get_response): | ||
| self.get_response = get_response | ||
|
|
||
| def __call__(self, request): | ||
| if request.method in ['POST', 'PUT'] : | ||
| try: | ||
| body_unicode = request.body.decode('utf-8') | ||
| request_body = json.loads(body_unicode) | ||
| logger.info("POST request to %s: %s", request.path, request_body) | ||
| except (json.JSONDecodeError, UnicodeDecodeError): | ||
| logger.warning("Failed to decode POST request body for %s", request.path) | ||
| except Exception as e: | ||
| logger.info('Something went wrong when logging post call - %s', e) | ||
|
|
||
| response = self.get_response(request) | ||
| return response |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Implement Data Masking or Filtering for Sensitive Information in Logs
The current implementation logs all POST and PUT request bodies without masking or filtering sensitive information. This can lead to exposure of sensitive data such as PII.
Recommended Actions:
- Implement Data Masking:
- Use the custom
get_logger()function to ensure consistency. - Introduce a mechanism to identify and mask sensitive fields before logging. For example:
class LogPostRequestMiddleware: def __init__(self, get_response): self.get_response = get_response self.logger = get_logger() def __call__(self, request): if request.method in ['POST', 'PUT']: try: body_unicode = request.body.decode('utf-8') request_body = json.loads(body_unicode) masked_body = mask_sensitive_data(request_body) self.logger.info("POST request to %s: %s", request.path, masked_body) except (json.JSONDecodeError, UnicodeDecodeError): self.logger.warning("Failed to decode POST request body for %s", request.path) except Exception as e: self.logger.error('Unexpected error when logging post call - %s', str(e), exc_info=True) response = self.get_response(request) return response def mask_sensitive_data(data): sensitive_fields = ['password', 'ssn', 'credit_card'] for field in sensitive_fields: if field in data: data[field] = '****' return data
- Use the custom
- Review Log Levels:
- Change
logger.infotologger.errorfor unexpected exceptions to improve issue visibility.
- Change
- Audit Logging Practices:
- Regularly review logs to ensure no sensitive information is inadvertently captured.
🔗 Analysis chain
Implement custom logger and review exception handling
-
As suggested in the previous review, please use the custom
get_logger()function instead of the root logger for consistency and to benefit from worker ID logging. -
The general exception handler has been added as requested, but consider using
logger.errorinstead oflogger.infofor unexpected exceptions. This will make it easier to identify and troubleshoot issues. -
Be cautious about logging all POST and PUT request bodies, as they may contain sensitive information. Consider implementing a mechanism to filter out or mask sensitive data before logging.
Here's a suggested implementation addressing points 1 and 2:
class LogPostRequestMiddleware:
def __init__(self, get_response):
self.get_response = get_response
self.logger = get_logger()
def __call__(self, request):
if request.method in ['POST', 'PUT']:
try:
body_unicode = request.body.decode('utf-8')
request_body = json.loads(body_unicode)
self.logger.info("POST request to %s: %s", request.path, request_body)
except (json.JSONDecodeError, UnicodeDecodeError):
self.logger.warning("Failed to decode POST request body for %s", request.path)
except Exception as e:
self.logger.error('Unexpected error when logging post call - %s', str(e), exc_info=True)
response = self.get_response(request)
return responseTo address point 3, let's check if there are any existing mechanisms for handling sensitive data in the project:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing data masking or filtering mechanisms
rg -i "mask|filter|sensitive|pii" --type python
Length of output: 152
Script:
#!/bin/bash
# Search for existing data masking or filtering mechanisms
rg -i "mask|filter|sensitive|pii" --type py
Length of output: 101832
Ashutosh619-sudo
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
Description
Please add PR description here, add screenshots if needed
Clickup
Please add link here
https://app.clickup.com/1864988/v/l/6-901603904304-1
Summary by CodeRabbit
New Features
Chores