Address PR #180 review feedback: Complete exception sanitization coverage#181
Conversation
|
🚫 This pull request was requested to be canceled by Abhi Mehrotra (a GitHub user), so it was removed from the merge queue. See more details here.
|
…ve tests Co-authored-by: abhimehro <84992105+abhimehro@users.noreply.github.com>
|
👋 Development Partner is reviewing this PR. Will provide feedback shortly. |
There was a problem hiding this comment.
Pull request overview
This PR completes exception-message sanitization in a few remaining log paths (to prevent leaking tokens/secrets into logs) and expands the exception logging test suite intended to regress those cases.
Changes:
- Apply
sanitize_for_log()to remaining exception-handling log statements in_retry_request(),warm_up_cache(), andcreate_folder(). - Expand
tests/test_exception_logging.pywith additional scenarios meant to cover sanitization paths across URL validation, rule fetching, folder creation, and retry warnings.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
main.py |
Ensures exceptions are sanitized before being interpolated into warning/debug logs in remaining handlers. |
tests/test_exception_logging.py |
Adds/extends tests intended to prevent regressions in exception sanitization across multiple code paths. |
tests/test_exception_logging.py
Outdated
| # Mock the API response to return data but cause an exception | ||
| # when trying to extract the ID | ||
| mock_response = MagicMock() | ||
| mock_response.json.return_value = { | ||
| "body": { | ||
| "groups": "invalid_format_causes_exception_with_token=ABC123" | ||
| } | ||
| } | ||
| mock_api_post.return_value = mock_response |
There was a problem hiding this comment.
test_create_folder_redacts_exception doesn't actually verify sanitization: the mocked response shape triggers an AttributeError whose message won't include the embedded "ABC123" substring, so the test can pass even if secrets are logged. Consider making the exception message itself contain a sensitive URL/query param (e.g. have response.json() raise with ?token=...) so the redaction behavior is meaningfully exercised.
tests/test_exception_logging.py
Outdated
| # Assertion - if debug was called, verify redaction | ||
| if mock_log.debug.called: | ||
| debug_calls = mock_log.debug.call_args_list | ||
|
|
||
| for call in debug_calls: | ||
| logged_message = str(call) | ||
| # The message should not contain raw exception data | ||
| # that might have tokens. Note: This is checking the | ||
| # general pattern. In real scenarios, the exception | ||
| # might contain URLs with tokens | ||
| pass # Debug logging happened with sanitization |
There was a problem hiding this comment.
test_create_folder_redacts_exception contains no assertions and ends with a pass, so it will succeed even if sanitization regresses. Please assert that log.debug was called and that the logged message contains [REDACTED] and does not contain the secret value.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
👋 Development Partner is reviewing this PR. Will provide feedback shortly. |
1 similar comment
|
👋 Development Partner is reviewing this PR. Will provide feedback shortly. |
There was a problem hiding this comment.
Hi @copilot,
Thanks for your help with this! Could you please address the following review suggestions for this pull request? Once these changes are implemented, the PR will be ready to merge.
I’ve included the specific context for the two suggestions below:
File: tests/test_exception_logging.py
Lines 158–166
test_create_folder_redacts_exception currently fails to verify sanitization because the mocked response triggers an AttributeError. Since that error message doesn't include the "ABC123" substring, the test passes even if secrets are logged. Please update the test so the exception message itself contains a sensitive URL or query parameter (e.g., by having response.json() raise an error with ?token=...) to properly exercise the redaction behavior.
File: tests/test_exception_logging.py
Lines 178–188
This test currently contains no assertions and ends with pass, meaning it will succeed even if sanitization fails. Please add assertions to verify that log.debug was called, that the logged message contains [REDACTED], and that it does not contain the actual secret value.
Thanks again!
Co-authored-by: abhimehro <84992105+abhimehro@users.noreply.github.com>
|
👋 Development Partner is reviewing this PR. Will provide feedback shortly. |
abhimehro
merged commit fa5152f
into
sentinel-log-sanitization-231878360127877934
Apply PR #180 Review Feedback
Security & Code Quality Fixes
Fix remaining unsanitized exception logs in main.py:
_retry_request()- line 609: Sanitize exception in warning logwarm_up_cache()- line 951: Sanitize exception in warning logcreate_folder()- line 1016: Sanitize exception in debug logImprove test_exception_logging.py:
Add comprehensive test coverage:
validate_folder_url()exception redaction_fetch_folder_rules()exception redactioncreate_folder()exception redaction - IMPROVED_retry_request()exception redactionAddress review feedback:
test_create_folder_redacts_exceptionto create exception with sensitive token in message[REDACTED]placeholder_api_getandtime.sleepto prevent test hangsValidate all changes with linting and tests
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.