fix: broaden telemetry kwarg redaction and _ctx filter atomically (BE-992)#459
Conversation
|
Warning Review limit reached
More reviews will be available in 1 hour, 53 minutes, and 7 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #459 +/- ##
==========================================
+ Coverage 83.32% 83.36% +0.04%
==========================================
Files 45 45
Lines 6831 6848 +17
==========================================
+ Hits 5692 5709 +17
Misses 1139 1139
🚀 New features to boost your workflow:
|
…elemetry Match sensitive kwargs by suffix (_token/_api_key/_secret/_password) and exact name instead of a fixed allowlist, drop ctx/underscore-prefixed and non-serializable values, and strip query strings from URL values. Add a drift-gate test so a new credential flag cannot ship unredacted. Original telemetry-redaction fix by Matt Miller; ported onto the dual-provider tracking path and extended. Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
fa5b17c to
e29536c
Compare
Summary
Fixes a latent CWE-200 vulnerability (BE-992) where two interacting allowlist gaps in
track_command()could leak credentials to Mixpanel if either gap were fixed independently.The Problem
Gap 1 — Credential redaction:
SENSITIVE_TRACKING_KEYSonly matched"api_key"by exact name, missing kwargs likeset_civitai_api_tokenandset_hf_api_token.Gap 2 — Context-object filter: The kwarg filter dropped
"ctx"and"context"but not"_ctx"(underscore-prefixed Click Context used by thedownloadcommand).These gaps accidentally cancelled each other out: the non-JSON-serializable
_ctxobject causedmp.track()to raise aTypeErrorbefore any credential reached Mixpanel. Fixing Gap 2 alone would silently activate the credential leak.The Fix
Both gaps are fixed atomically in a single commit:
Suffix-based sensitive key detection — replaces the exact-match
SENSITIVE_TRACKING_KEYSwith_is_sensitive()that matches suffixes (_token,_api_key,_secret,_password) and exact names (api_key,token,password,secret).Broadened kwarg filter —
_is_trackable()now:ctxandcontext(as before)_ctxand any future private params)Historical context comment explaining the interaction so future maintainers don't undo this in pieces.
Tests Added (6 new)
test_set_civitai_api_token_is_redacted<redacted>test_set_hf_api_token_is_redacted<redacted>test_bare_token_kwarg_is_redactedtokenkwarg is redactedtest_underscore_ctx_is_excluded_ctx(Click Context) never reaches tracked propertiestest_non_serializable_value_is_excludedRisk Assessment
_ctxserialization) will now succeed with properly redacted properties.Linear Issue: BE-992