feat: attribute requests to clients and API keys in logs - #265
Conversation
Parse the X-Spoo-Client header and bind client/client_version to the request log context, bind key_id/key_prefix on API key auth, exempt derived analytics fields like has_password and key_id from log redaction, classify access_token cookie auth as jwt_cookie, and allow X-Spoo-Client in CORS preflight.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe changes extend structured logging with API-key identifiers and request client metadata, preserve approved fields during redaction, classify cookie-based JWT authentication, allow the client header through CORS, and add unit coverage. ChangesStructured logging
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Request
participant LoggingMiddleware
participant Structlog
Request->>LoggingMiddleware: Send X-Spoo-Client and authentication headers/cookies
LoggingMiddleware->>LoggingMiddleware: Parse client tag and classify authentication
LoggingMiddleware->>Structlog: Bind client metadata and auth context
Structlog-->>Request: Continue request with inherited logging context
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@middleware/logging.py`:
- Around line 51-52: Update the bearer detection helper in middleware/logging.py
to match dependencies/auth.py: recognize the Authorization scheme
case-insensitively and normalize surrounding whitespace before selecting the
API-key/bearer classification. Ensure a valid bearer header takes precedence
over the access_token cookie so such requests are not labeled jwt_cookie.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d4926a79-eb6e-4b34-a9b4-8cfa00aad5e6
📒 Files selected for processing (6)
dependencies/auth.pyinfrastructure/logging.pymiddleware/logging.pymiddleware/security.pytests/unit/infrastructure/test_logging_redaction.pytests/unit/middleware/test_logging.py
Zingzy
left a comment
Zingzy
left a comment
There was a problem hiding this comment.
Mergeable, nothing blocking. Traced the redaction and CORS paths rather than trusting the description.
- Redaction: the safe list is exact-match and checked before the substring heuristic, so every secret-bearing name stays covered (
jwt_secret,client_secret,device_tokenstill redact; the new tests pin this). The unredacted fields are derived booleans, display prefixes, and identifiers only. key_id/key_prefixbound in the auth dependency ride service-layer events but notrequest_completed(the BaseHTTPMiddleware task boundary). #267 in this stack closes that; landing this first is still worth it for the service events alone.- Keeping
auth_kind(header shape, middleware) andauth_method(resolved identity, auth dep) as separate fields is right: a mismatch between the two is itself a useful signal. - Hot path cost is one regex match on a usually empty header. Negligible, including on redirects.
- CORS:
_ALLOWED_HEADERSis shared by the public and private classes, so the header is preflight-allowed everywhere. Harmless header, fine.
CI matrix is green here. I also ran the full unit suite at the stack tip locally, 2001 pass.
Aligns the log tag with dependencies/auth.py so a lowercase bearer API key with a cookie present is not misclassified as jwt_cookie.
|
Re-verified at eb1f53f. The For the record: my first pass cleared this file without catching the case-sensitive scheme check, and the mismatch was real. A Full test matrix is green on the new head; the unit suite passes at the chain tip locally (2010). |
What
Adds request source attribution to the structured logs.
X-Spoo-Clientrequest header, parsed in the logging middleware and bound to the log context asclientandclient_version. The value is<slug>or<slug>/<version>(for examplesnap/2.1.0). First party clients will senddashboard,landing,snap,raycast,cli, andbot. Values that do not match the shape are treated as absent, never rejected.key_idandkey_prefixto the log context, so traffic and service events can be attributed to a specific key instead of just a user.has_password,password_protected,key_id,key_prefix,token_prefix,query_keys). Previously these were logged as***REDACTED***, which made password usage and key identifiers unqueryable._auth_kindnow recognizes theaccess_tokencookie asjwt_cookie. Cookie authenticated browser sessions were previously classified asanonymous.X-Spoo-Clientadded to CORS allowed headers so browser clients can send it.Why
Log queries can currently tell API key traffic from JWT traffic, but not which client an anonymous or authenticated request came from, nor which specific key produced it. This makes questions like where links are created from and which key is responsible for a traffic spike answerable with a group by.
Notes
_auth_kind, and the redaction safe list.Summary by CodeRabbit