fix: redact secret tokens from URLs before logging#197
Conversation
Login/OAuth tokens (login_token, access_token, refresh_token) appear in URL query strings and fragments during the auth flow. The httpx event hooks (log_request, log_response, raise_for_status_handler) logged these URLs verbatim at DEBUG and ERROR levels, leaking live credentials to log files. Add sanitize_url() to log_sanitizer.py that strips sensitive query and fragment parameters. Apply it to all URL logging in authentication.py (SmartLoginClient, SmartLoginRetry, SmartAuthentication.async_auth_flow) and client.py (SmartClient). Also extend _sanitize_string to catch param=value and glt_<key>=<token> patterns in arbitrary text. https://claude.ai/code/session_01TFLXNPgy1T5kvNaBB6VUx7
📝 WalkthroughWalkthroughThis PR adds URL-based log sanitization across authentication and HTTP client logging. A new ChangesURL Sanitization Across Logging
🎯 2 (Simple) | ⏱️ ~12 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pysmarthashtag/api/authentication.py (1)
248-250:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSanitize the non-rate-limit login failure log too.
This branch still logs
excraw. Forhttpx.HTTPStatusError, that string includes the request URL, socontext/login_tokencan still hit logs through this path even after the new sanitization changes.Suggested fix
else: self._state.quiet_until = now + self._OTHER_FAILURE_BACKOFF _LOGGER.info( "Smart API login failed (%s). Short retry-suppress until %s", - exc, + sanitize_log_data(str(exc)), self._state.quiet_until.isoformat(timespec="seconds"), )As per coding guidelines, "Use sanitize_log_data() from
api.log_sanitizerwhen logging sensitive data".🤖 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 `@pysmarthashtag/api/authentication.py` around lines 248 - 250, The info log in the login-failure branch currently interpolates exc directly (in the _LOGGER.info call), which may leak request URLs for httpx.HTTPStatusError; update that call to sanitize the exception data using sanitize_log_data() from api.log_sanitizer before logging (e.g., wrap or convert exc via sanitize_log_data(exc) or extract and sanitize only non-sensitive fields), ensuring the _LOGGER.info invocation that references exc uses the sanitized value; touch the code around the login retry-suppress branch (where _LOGGER.info is called) and keep the existing message text/format but pass the sanitized exception instead.
🤖 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 `@pysmarthashtag/api/log_sanitizer.py`:
- Around line 58-63: The regex _URL_PARAM_PATTERN currently only matches params
preceded by ? or &, so fragment-leading (`#access_token`=...) and standalone
"access_token=..." cases slip through; update _URL_PARAM_PATTERN to also accept
start-of-string, fragment '#' or word-boundary prefixes (so keys preceded by ^,
#, ?, &, or a word boundary are matched) while preserving the same capture
groups and flags, then run/update tests and ensure _sanitize_string uses the
updated pattern to redact those additional cases.
---
Outside diff comments:
In `@pysmarthashtag/api/authentication.py`:
- Around line 248-250: The info log in the login-failure branch currently
interpolates exc directly (in the _LOGGER.info call), which may leak request
URLs for httpx.HTTPStatusError; update that call to sanitize the exception data
using sanitize_log_data() from api.log_sanitizer before logging (e.g., wrap or
convert exc via sanitize_log_data(exc) or extract and sanitize only
non-sensitive fields), ensuring the _LOGGER.info invocation that references exc
uses the sanitized value; touch the code around the login retry-suppress branch
(where _LOGGER.info is called) and keep the existing message text/format but
pass the sanitized exception instead.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
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: c1c2e8dd-d635-4ba4-bf74-ec77d16ad71f
📒 Files selected for processing (3)
pysmarthashtag/api/authentication.pypysmarthashtag/api/client.pypysmarthashtag/api/log_sanitizer.py
| # Matches sensitive key=value pairs in query strings embedded in text (e.g. login_token=abc123&) | ||
| _URL_PARAM_PATTERN = re.compile( | ||
| r"(?<=[?&])((?:access_token|refresh_token|login_token|token|code|context)" | ||
| r")=([^&#\s]+)", | ||
| re.IGNORECASE, | ||
| ) |
There was a problem hiding this comment.
Handle fragment-leading and standalone secret params in _sanitize_string.
_URL_PARAM_PATTERN only matches keys preceded by ? or &. Strings like #access_token=... or access_token=... still pass through unchanged, so the fallback sanitize_log_data(str(exc)) path can leak the first OAuth token in fragment/query-like text.
Suggested fix
_URL_PARAM_PATTERN = re.compile(
- r"(?<=[?&])((?:access_token|refresh_token|login_token|token|code|context)"
- r")=([^&#\s]+)",
+ r"(^|[?#&])((?:access_token|refresh_token|login_token|token|code|context))=([^&#\s]+)",
re.IGNORECASE,
)- result = _URL_PARAM_PATTERN.sub(r"\1=***", result)
+ result = _URL_PARAM_PATTERN.sub(r"\1\2=***", result)🤖 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 `@pysmarthashtag/api/log_sanitizer.py` around lines 58 - 63, The regex
_URL_PARAM_PATTERN currently only matches params preceded by ? or &, so
fragment-leading (`#access_token`=...) and standalone "access_token=..." cases
slip through; update _URL_PARAM_PATTERN to also accept start-of-string, fragment
'#' or word-boundary prefixes (so keys preceded by ^, #, ?, &, or a word
boundary are matched) while preserving the same capture groups and flags, then
run/update tests and ensure _sanitize_string uses the updated pattern to redact
those additional cases.
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. An unexpected error occurred while generating fixes: Not Found - https://docs.github.com/rest/git/refs#get-a-reference |
|
@ |
3 participants
Login/OAuth tokens (login_token, access_token, refresh_token) appear in URL query strings and fragments during the auth flow. The httpx event hooks (log_request, log_response, raise_for_status_handler) logged these URLs verbatim at DEBUG and ERROR levels, leaking live credentials to log files.
Add sanitize_url() to log_sanitizer.py that strips sensitive query and fragment parameters. Apply it to all URL logging in authentication.py (SmartLoginClient, SmartLoginRetry, SmartAuthentication.async_auth_flow) and client.py (SmartClient). Also extend sanitize_string to catch param=value and glt= patterns in arbitrary text.
https://claude.ai/code/session_01TFLXNPgy1T5kvNaBB6VUx7
Summary by CodeRabbit
Release Notes