fix(security): redact sensitive query params in request logs #805
Conversation
📝 WalkthroughWalkthroughAdds query-parameter redaction to request logging via LoggingMiddleware (new class attributes and helper methods). Updates docs and SECURITY-REVIEW.md to describe and record the change. Adds unit tests covering redaction behavior and edge-case query inputs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant LoggingMiddleware as Middleware
participant AppHandler as Handler
participant Logger
Client->>Middleware: HTTP request (method, path, query)
alt query present
Middleware->>Middleware: parse_qsl(query) / _redact_query()
Note right of Middleware `#EFEFEF`: Sensitive params replaced\nwith "REDACTED"
end
Middleware->>Handler: forward request (unchanged)
Handler-->>Middleware: response
Middleware->>Logger: log method and path (uses redacted query if applied)
Logger-->>Middleware: write log
Middleware-->>Client: return response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
Comment |
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/middleware/logging_middleware.py (1)
46-67: Solid redaction logic with good edge case handling.The implementation correctly handles:
- Empty queries (Line 48-49)
- Unparseable queries (Lines 52-53)
- Mixed sensitive/non-sensitive parameters
- Performance optimization by only rebuilding when needed (Lines 64-65)
Minor nitpick: Line 67 uses
doseq=True, which is designed for dict values that are sequences (e.g.,{'key': ['v1', 'v2']}). Since we're passing a list of tuples, this parameter has no effect—repeated keys are already handled correctly. It's harmless but unnecessary.Optional: Remove unnecessary parameter
- return urlencode(redacted_params, doseq=True) + return urlencode(redacted_params)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
SECURITY-REVIEW.mdapp/middleware/logging_middleware.pydocs/usage/configuration/environment.mdtests/unit/test_logging.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/unit/test_logging.py (1)
app/middleware/logging_middleware.py (3)
LoggingMiddleware(21-99)dispatch(69-99)_redact_query(47-67)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: test (3.14)
- GitHub Check: test (3.10)
- GitHub Check: test (3.12)
- GitHub Check: test (3.11)
- GitHub Check: test (3.13)
🔇 Additional comments (5)
app/middleware/logging_middleware.py (3)
24-36: LGTM! Well-designed redaction configuration.The class attributes provide a clean, centralized way to define sensitive keys. The comment on Line 25 about key ordering for performance shows good awareness. The tuple contains appropriate sensitive parameter names commonly used in authentication flows.
38-44: LGTM! Case-insensitive matching is appropriate.The case-insensitive comparison (Line 40) correctly handles variations like
code,Code, andCODE, which is the right security posture since attackers might try case variations.
90-92: LGTM! Clean integration of redaction into request logging.The redaction is applied at the right layer—after request processing but before logging—ensuring the application can still access sensitive parameters while keeping them out of logs.
tests/unit/test_logging.py (2)
517-552: Excellent integration test coverage!This test thoroughly validates the redaction behavior:
- ✅ Sensitive parameters redacted (
code,token,API_KEY)- ✅ Non-sensitive parameters preserved (
page=2)- ✅ Case-insensitive matching verified (Line 535:
API_KEYuppercase)- ✅ Original sensitive values confirmed absent from logs (Lines 549-551)
- ✅ Response unchanged (Line 552)
554-560: Good edge case coverage for the helper method.These unit tests cover important edge cases:
- Empty query string handling
- Unparseable query strings (e.g., lone
&)
Summary
SENSITIVE_QUERY_KEYSinapp/middleware/logging_middleware.py)Testing
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.