fix: remove DEBUG log statements left at INFO/WARNING/ERROR level#668
Open
arnavp27 wants to merge 1 commit intopotpie-ai:mainfrom
Open
fix: remove DEBUG log statements left at INFO/WARNING/ERROR level#668arnavp27 wants to merge 1 commit intopotpie-ai:mainfrom
arnavp27 wants to merge 1 commit intopotpie-ai:mainfrom
Conversation
Removes 15 debug log statements from auth_service.py and user_service.py that were incorrectly logged at INFO/WARNING/ERROR level instead of DEBUG, including a security issue where the first 20 chars of a Firebase auth token were leaked to logs at INFO level. Fixes potpie-ai#633
Contributor
WalkthroughRemoved DEBUG log statements and diagnostic logging from authentication and user service modules that were running at INFO/ERROR production levels. Includes elimination of a statement logging partial auth tokens, reducing security exposure and log noise. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/modules/users/user_service.py (2)
129-131: LGTM - PII correctly omitted from error message.The error logging correctly avoids including the email address, aligning with the PR objective to prevent PII exposure.
Optional: Per static analysis (TRY400), consider using
logger.exceptioninstead oflogger.errorto automatically include the stack trace, which aids debugging while still reducing log verbosity compared to the removed DEBUG statements.♻️ Optional fix
except Exception as e: - logger.error(f"Error fetching user ID by email: {e}") + logger.exception("Error fetching user ID by email") return None,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/modules/users/user_service.py` around lines 129 - 131, Replace the logger.error call in the except block that handles failures when fetching a user ID by email with logger.exception so the stack trace is captured while still avoiding logging the email; locate the except Exception as e in the function that fetches user ID by email (e.g., get_user_id_by_email / fetch_user_id_by_email) in user_service.py and change the logging call to logger.exception("Error fetching user ID by email", exc_info=True) or simply logger.exception("Error fetching user ID by email") and keep the return None unchanged.
157-159: LGTM - PII correctly omitted from error message.The error logging correctly avoids including the email addresses, consistent with the other changes in this PR.
Optional: Same as above, consider using
logger.exceptionfor automatic stack trace inclusion.♻️ Optional fix
except Exception as e: - logger.error(f"Error fetching user IDs by emails: {e}") + logger.exception("Error fetching user IDs by emails") return None,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/modules/users/user_service.py` around lines 157 - 159, The except block catching Exception in user_service.py currently calls logger.error(f"Error fetching user IDs by emails: {e}") without a stack trace; replace that call with logger.exception("Error fetching user IDs by emails") (or add logger.exception after the logger.error) so the exception handler (the except Exception as e: block) logs the full stack trace automatically while still avoiding PII in the message and then returns None as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/modules/users/user_service.py`:
- Around line 129-131: Replace the logger.error call in the except block that
handles failures when fetching a user ID by email with logger.exception so the
stack trace is captured while still avoiding logging the email; locate the
except Exception as e in the function that fetches user ID by email (e.g.,
get_user_id_by_email / fetch_user_id_by_email) in user_service.py and change the
logging call to logger.exception("Error fetching user ID by email",
exc_info=True) or simply logger.exception("Error fetching user ID by email") and
keep the return None unchanged.
- Around line 157-159: The except block catching Exception in user_service.py
currently calls logger.error(f"Error fetching user IDs by emails: {e}") without
a stack trace; replace that call with logger.exception("Error fetching user IDs
by emails") (or add logger.exception after the logger.error) so the exception
handler (the except Exception as e: block) logs the full stack trace
automatically while still avoiding PII in the message and then returns None as
before.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/modules/auth/auth_service.pyapp/modules/users/user_service.py
💤 Files with no reviewable changes (1)
- app/modules/auth/auth_service.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #633
What
Removes 15
DEBUG:prefixed log statements fromauth_service.pyanduser_service.pythat were accidentally logged atINFO,WARNING, andERRORlevel instead ofDEBUG, causing them to appear in production logs.Changes
app/modules/auth/auth_service.py— removed 7 statements, includingthe security issue on line 104 where the first 20 characters of a Firebase
auth token were being written to logs at INFO level on every authenticated
request
app/modules/users/user_service.py— removed 8 statements that wereleaking user emails and UIDs (PII) into production logs on every user lookup
Why
These were debug statements using
logging.info/warning/error("DEBUG: ...")instead of
logging.debug(...), so they bypassed log level filtering and ranin production. Standard auth and user lookup operations don't need verbose
logging.
Summary by CodeRabbit