Skip to content

fix: address AI review findings on staging promotion (#582) #583

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
barakb merged 2 commits into from
May 31, 2026
Merged

fix: address AI review findings on staging promotion (#582) #583

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d0f36e4
fix: address AI review findings on staging promotion (#582)
barakb May 31, 2026
92905fe
fix: make _sanitize_for_log non-fatal on str() conversion (overcut-ai…
barakb May 31, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,10 +111,10 @@ jobs:
--health-retries 5

steps:
- uses: actions/checkout@v6
- uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6

- name: Set up Python
uses: actions/setup-python@v6
uses: actions/setup-python@a309ff8b426b58ec0e2a45f0f869d46889d02405 # v6
with:
python-version: '3.12'

Expand Down
8 changes: 6 additions & 2 deletions api/routes/auth.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,8 +116,12 @@ def _verify_password(password: str, stored_password_hex: str) -> bool:
def _sanitize_for_log(value: str) -> str:

@coderabbitai coderabbitai Bot May 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Update type hint to accept any input type.

The function signature declares value: str, but the implementation accepts any type and coerces non-strings via str(value) on line 119. The type hint should reflect the actual contract.

🔧 Proposed fix 
-def _sanitize_for_log(value: str) -> str:
+def _sanitize_for_log(value) -> str:
     """Sanitize user input for logging by removing newlines and carriage returns."""

Or, if you prefer explicit typing:

+from typing import Any
+
-def _sanitize_for_log(value: str) -> str:
+def _sanitize_for_log(value: Any) -> str:
     """Sanitize user input for logging by removing newlines and carriage returns."""

As per coding guidelines, Python code in api/**/*.py should include type hints throughout.

🤖 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 `@api/routes/auth.py` at line 116, The function _sanitize_for_log currently
types its parameter as str but accepts any input and coerces with str(value);
update the type hint to reflect that by changing the parameter type to
typing.Any (or object) and add the corresponding import (from typing import Any)
so the signature becomes _sanitize_for_log(value: Any) -> str; keep the existing
implementation that handles non-strings via str(value).

"""Sanitize user input for logging by removing newlines and carriage returns."""
if not isinstance(value, str):
return str(value)
return value.replace('\r\n', '').replace('\n', '').replace('\r', '')
value = str(value)
# Strip carriage returns first, then newlines. The final ``.replace('\n', '')``

@overcut-ai overcut-ai Bot May 31, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[major]: Changing the non-string branch to value = str(value) can now raise and break auth flows if a passed object implements __str__ that throws, whereas the previous early return isolated this behavior from subsequent sanitizer operations. Consider keeping a guarded conversion to avoid unexpected exceptions in logging sanitization paths.

@barakb barakb May 31, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 92905fe. Wrapped the non-string str(value) conversion in try/except with a safe '<unprintable>' fallback, so log sanitisation can never raise into the auth flow if a custom __str__ throws.

Note: the previous code already called str(value) in the non-string branch (return str(value)), so this exception path existed before this PR rather than being newly introduced — but making sanitisation fail-safe is a worthwhile hardening, so the guard is added. The CodeQL-recognised line-break sanitizer is preserved (the normal return still comes from .replace('\n', '')).

# must be the outermost (returned) call so that CodeQL's log-injection sanitizer
# (ReplaceLineBreaksSanitizer, which only recognises a first argument of "\n" or
# "\r\n") treats the returned value as sanitised.
return value.replace('\r', '').replace('\n', '')

def _validate_email(email: str) -> bool:
"""Basic email validation."""
Expand Down
Loading