-
Notifications
You must be signed in to change notification settings - Fork 1
🛡️ Sentinel: Redact sensitive query parameters in logs #157
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -149,8 +149,21 @@ | |||||
| def sanitize_for_log(text: Any) -> str: | ||||||
| """Sanitize text for logging, ensuring TOKEN is redacted and control chars are escaped.""" | ||||||
| s = str(text) | ||||||
|
|
||||||
| # 1. Redact common sensitive query parameters in URLs (Defense in Depth) | ||||||
| # Matches ?param=value or ¶m=value | ||||||
| # Stops at &, whitespace, or quotes | ||||||
| s = re.sub( | ||||||
Check warningCode scanning / Pylint (reported by Codacy) Variable name "s" doesn't conform to snake_case naming style Warning
Variable name "s" doesn't conform to snake_case naming style
Check warningCode scanning / Pylintpython3 (reported by Codacy) Variable name "s" doesn't conform to snake_case naming style Warning
Variable name "s" doesn't conform to snake_case naming style
|
||||||
| r"([?&](?:token|key|secret|password|auth|access_token|api_key)=)([^&\s\"']+)", | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The regular expression
Suggested change
|
||||||
| r"\1[REDACTED]", | ||||||
| s, | ||||||
| flags=re.IGNORECASE, | ||||||
| ) | ||||||
|
Comment on lines
+156
to
+161
|
||||||
|
|
||||||
| # 2. Redact the specific global TOKEN if known | ||||||
| if TOKEN and TOKEN in s: | ||||||
| s = s.replace(TOKEN, "[REDACTED]") | ||||||
|
|
||||||
| # repr() safely escapes control characters (e.g., \n -> \\n, \x1b -> \\x1b) | ||||||
| # This prevents log injection and terminal hijacking. | ||||||
| safe = repr(s) | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,56 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||
| import unittest | ||||||||||||||||||||||||||||||||||||||||||||||||
Check warningCode scanning / Pylint (reported by Codacy) Missing module docstring Warning test
Missing module docstring
Check warningCode scanning / Pylintpython3 (reported by Codacy) Missing module docstring Warning test
Missing module docstring
|
||||||||||||||||||||||||||||||||||||||||||||||||
| from main import sanitize_for_log | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| class TestSecurityLog(unittest.TestCase): | ||||||||||||||||||||||||||||||||||||||||||||||||
Check warningCode scanning / Pylint (reported by Codacy) Missing class docstring Warning test
Missing class docstring
Check warningCode scanning / Pylintpython3 (reported by Codacy) Missing class docstring Warning test
Missing class docstring
|
||||||||||||||||||||||||||||||||||||||||||||||||
| def test_redact_query_params(self): | ||||||||||||||||||||||||||||||||||||||||||||||||
Check warningCode scanning / Pylint (reported by Codacy) Missing method docstring Warning test
Missing method docstring
Check warningCode scanning / Pylintpython3 (reported by Codacy) Missing function or method docstring Warning test
Missing function or method docstring
|
||||||||||||||||||||||||||||||||||||||||||||||||
| # Test cases for URL query parameter redaction | ||||||||||||||||||||||||||||||||||||||||||||||||
| test_cases = [ | ||||||||||||||||||||||||||||||||||||||||||||||||
| ( | ||||||||||||||||||||||||||||||||||||||||||||||||
| "https://example.com?token=secret123", | ||||||||||||||||||||||||||||||||||||||||||||||||
| "https://example.com?token=[REDACTED]" | ||||||||||||||||||||||||||||||||||||||||||||||||
| ), | ||||||||||||||||||||||||||||||||||||||||||||||||
| ( | ||||||||||||||||||||||||||||||||||||||||||||||||
| "https://example.com?key=my_key&foo=bar", | ||||||||||||||||||||||||||||||||||||||||||||||||
| "https://example.com?key=[REDACTED]&foo=bar" | ||||||||||||||||||||||||||||||||||||||||||||||||
| ), | ||||||||||||||||||||||||||||||||||||||||||||||||
| ( | ||||||||||||||||||||||||||||||||||||||||||||||||
| "Error fetching https://api.com?auth=xyz failed", | ||||||||||||||||||||||||||||||||||||||||||||||||
| "Error fetching https://api.com?auth=[REDACTED] failed" | ||||||||||||||||||||||||||||||||||||||||||||||||
| ), | ||||||||||||||||||||||||||||||||||||||||||||||||
| ( | ||||||||||||||||||||||||||||||||||||||||||||||||
| "https://site.com?access_token=token&api_key=key", | ||||||||||||||||||||||||||||||||||||||||||||||||
| "https://site.com?access_token=[REDACTED]&api_key=[REDACTED]" | ||||||||||||||||||||||||||||||||||||||||||||||||
| ), | ||||||||||||||||||||||||||||||||||||||||||||||||
| ( | ||||||||||||||||||||||||||||||||||||||||||||||||
| "https://safe.com?public=data", | ||||||||||||||||||||||||||||||||||||||||||||||||
| "https://safe.com?public=data" | ||||||||||||||||||||||||||||||||||||||||||||||||
| ), | ||||||||||||||||||||||||||||||||||||||||||||||||
| ( | ||||||||||||||||||||||||||||||||||||||||||||||||
| "'https://quoted.com?password=pass'", | ||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The test case at this line uses an input string that is already quoted:
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||
| "https://quoted.com?password=[REDACTED]" | ||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+27
to
+30
|
||||||||||||||||||||||||||||||||||||||||||||||||
| ), | |
| ( | |
| "'https://quoted.com?password=pass'", | |
| "https://quoted.com?password=[REDACTED]" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sanitize_for_log function in main.py now includes logic (lines 170-172) to remove the outermost quotes added by repr(). Therefore, the result_content extraction logic within this test is redundant. The result returned by sanitize_for_log should already be the unquoted string.
# The sanitize_for_log function now handles stripping repr() quotes.
# So, result should be directly comparable to expected.
result_content = resultCheck warning
Code scanning / Pylint (reported by Codacy)
Line too long (105/100) Warning test
Check warning
Code scanning / Pylintpython3 (reported by Codacy)
Line too long (105/100) Warning test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot
AI
Feb 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The inline comments here describe sanitize_for_log as returning a quoted repr() string, but the current implementation strips matching outer quotes before returning. This makes the comments misleading and the extra quote-stripping logic below unnecessary for most cases; consider simplifying the test to compare sanitize_for_log(input_str) directly to expected (and update comments accordingly).
| # sanitize_for_log uses repr() which adds quotes and escapes. | |
| # We need to handle that in our expectation or strip it. | |
| # The current implementation of sanitize_for_log returns a repr() string (quoted). | |
| # If our expected string is the *content* inside the quotes, we should match that. | |
| result = sanitize_for_log(input_str) | |
| # Remove surrounding quotes for easier comparison if present | |
| if len(result) >= 2 and result[0] == result[-1] and result[0] in ("'", '"'): | |
| result_content = result[1:-1] | |
| else: | |
| result_content = result | |
| # Also repr() escapes things. | |
| # Our expected strings don't have special chars that repr escapes (except maybe quotes). | |
| # But the proposed implementation applies redaction BEFORE repr. | |
| # So sanitizing "url?token=s" -> "url?token=[REDACTED]" -> repr() -> "'url?token=[REDACTED]'" | |
| self.assertEqual(result_content, expected, f"Failed for input: {input_str}") | |
| # sanitize_for_log is expected to return the sanitized string directly, | |
| # with any sensitive query parameters redacted. | |
| result = sanitize_for_log(input_str) | |
| self.assertEqual(result, expected, f"Failed for input: {input_str}") |
Check warning
Code scanning / Prospector (reported by Codacy)
expected 2 blank lines after class or function definition, found 1 (E305) Warning test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sanitize_for_lognow redacts common sensitive URL query parameters in addition to the globalTOKEN, but the docstring still only mentionsTOKENredaction and control-char escaping. Please update the docstring to reflect the expanded behavior so callers/tests have an accurate contract.