fix(telemetry): filter sensitive kwargs in error path of track_usage decorator#826
Conversation
|
PR author is not in the allowed authors list. |
WalkthroughThis pull request modifies the Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
|
2027 auto-runs evals against preview deployments of your docs. To enable this, install one of:
Once a preview is deployed, open a new PR and we'll run the eval automatically. Evaluating agent experience using 2027.dev · View dashboard |
|
Thanks for the review! You're right — the formatting was the only thing blocking CI. I've run |
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 `@packages/notte-core/src/notte_core/common/telemetry.py`:
- Around line 241-249: The current try/except captures success telemetry inside
the try and error telemetry inside except; refactor to use try/except/else in
the wrapper around func so the success-path capture_event(event_name,
properties={...}) runs in an else block (executed only if no exception), keep
the error capture in except using the same properties plus "error": str(e), and
re-raise the exception with plain raise (not raise e) to preserve the traceback;
update references to func, event_name, capture_event, and filtered_kwargs
accordingly.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b9bde868-e12b-4fa2-b83e-e6b6760ee8c0
📒 Files selected for processing (1)
packages/notte-core/src/notte_core/common/telemetry.py
| try: | ||
| result = func(*args, **kwargs) | ||
| filtered_kwargs = {k: v for k, v in kwargs.items() if k not in exclude_kwargs} | ||
| capture_event(event_name, properties={"input": {"args": args, "kwargs": filtered_kwargs}}) | ||
| return result | ||
| except Exception as e: | ||
| capture_event(event_name, properties={"input": {"args": args, "kwargs": kwargs}, "error": str(e)}) | ||
| capture_event( | ||
| event_name, properties={"input": {"args": args, "kwargs": filtered_kwargs}, "error": str(e)} | ||
| ) | ||
| raise e |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consider using try/except/else for clearer control flow.
The success-path telemetry on line 243 could be moved to an else block to make it explicit that it only runs when no exception occurs. This follows Python best practices and addresses the Ruff TRY300 hint.
♻️ Proposed refactor using try/except/else
try:
result = func(*args, **kwargs)
- capture_event(event_name, properties={"input": {"args": args, "kwargs": filtered_kwargs}})
- return result
except Exception as e:
capture_event(
event_name, properties={"input": {"args": args, "kwargs": filtered_kwargs}, "error": str(e)}
)
raise e
+else:
+ capture_event(event_name, properties={"input": {"args": args, "kwargs": filtered_kwargs}})
+ return result📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| result = func(*args, **kwargs) | |
| filtered_kwargs = {k: v for k, v in kwargs.items() if k not in exclude_kwargs} | |
| capture_event(event_name, properties={"input": {"args": args, "kwargs": filtered_kwargs}}) | |
| return result | |
| except Exception as e: | |
| capture_event(event_name, properties={"input": {"args": args, "kwargs": kwargs}, "error": str(e)}) | |
| capture_event( | |
| event_name, properties={"input": {"args": args, "kwargs": filtered_kwargs}, "error": str(e)} | |
| ) | |
| raise e | |
| try: | |
| result = func(*args, **kwargs) | |
| except Exception as e: | |
| capture_event( | |
| event_name, properties={"input": {"args": args, "kwargs": filtered_kwargs}, "error": str(e)} | |
| ) | |
| raise e | |
| else: | |
| capture_event(event_name, properties={"input": {"args": args, "kwargs": filtered_kwargs}}) | |
| return result |
🧰 Tools
🪛 Ruff (0.15.12)
[warning] 244-244: Consider moving this statement to an else block
(TRY300)
[warning] 249-249: Use raise without specifying exception name
Remove exception name
(TRY201)
🤖 Prompt for 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.
In `@packages/notte-core/src/notte_core/common/telemetry.py` around lines 241 -
249, The current try/except captures success telemetry inside the try and error
telemetry inside except; refactor to use try/except/else in the wrapper around
func so the success-path capture_event(event_name, properties={...}) runs in an
else block (executed only if no exception), keep the error capture in except
using the same properties plus "error": str(e), and re-raise the exception with
plain raise (not raise e) to preserve the traceback; update references to func,
event_name, capture_event, and filtered_kwargs accordingly.
There was a problem hiding this comment.
LGTM
Previous formatting comment addressed. The fix is correct and complete. CI failures are pre-existing on the base branch and unrelated to this change.
Tag @mendral-app with feedback or questions. View session
Vulnerability: Sensitive credentials leaked to external telemetry on errors (CWE-200)
Affected file:
packages/notte-core/src/notte_core/common/telemetry.pyAffected function:
track_usagedecorator (line ~237)Severity: High — credentials sent to third-party services
Summary
The
track_usagedecorator filters sensitive kwargs (password,email,username,mfa_secret) before sending telemetry events on the success path, but the error/exception path bypasses this filtering entirely. When a decorated function raises an exception, the raw unfilteredkwargsdict — including plaintext passwords and secrets — is sent to external telemetry services (PostHog, Scarf).This directly affects the vault credential endpoints in
packages/notte-sdk/src/notte_sdk/endpoints/vaults.py, where functions likeadd_or_update_credentialsaccept sensitive fields (password,email,username,mfa_secret) as kwargs and are decorated with@track_usage.Data flow
vault_client.add_or_update_credentials(vault_id, password="s3cret", email="user@example.com")track_usagedecorator wraps the callexceptblock firescapture_event()is called with rawkwargscontainingpasswordandemailcapture_event()forwards to PostHog and Scarf — external third-party servicesPoC
To confirm leakage, you can temporarily patch
capture_eventto print its properties argument — you'll see the raw password in the error-path payload but not in the success-path payload.Fix
The fix hoists the
filtered_kwargscomputation above thetryblock so both the success and error paths use the same filtered dict. This is a 2-line change: move the filtering line up by 1 line, and replacekwargswithfiltered_kwargsin theexceptblock.Before:
After:
Testing
password,email,username, andmfa_secreton the success pathfiltered_kwargsinstead of rawkwargsexclude_kwargsset matches the sensitive fields used by vault credential endpointsAdversarial review
Before submitting, we considered whether this exposure is mitigated by other controls. It is not — telemetry is enabled by default (
NOTTE_DISABLE_TELEMETRYmust be explicitly set), thecapture_eventfunction sends data to external PostHog and Scarf endpoints over HTTPS, and there is no secondary filtering or redaction layer between the decorator and the HTTP calls. Any exception in a vault credential operation (network timeout, 401, server 500) triggers the leak. The filtering logic already existed for the success path, confirming the project's intent to exclude these fields from telemetry — the error path was simply missed.Submitted by Sebastion — autonomous open-source security research from Foundation Machines. Free for public repos via the Sebastion AI GitHub App.
Note
Fixes a security bug in the
track_usagedecorator where sensitive kwargs (password,email,username,mfa_secret) were correctly filtered on the success path but sent raw to external telemetry (PostHog, Scarf) on the exception path. The fix hoistsfiltered_kwargscomputation above thetryblock so both paths use the same filtered dict. The second commit appliesruff formatto address the previous review comment.Written by Mendral for commit 88d9732.