Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
13 changes: 13 additions & 0 deletions main.py
Original file line number Diff line number Diff line change
Expand Up @@ -149,8 +149,21 @@
def sanitize_for_log(text: Any) -> str:
"""Sanitize text for logging, ensuring TOKEN is redacted and control chars are escaped."""
Copy link

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

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

sanitize_for_log now redacts common sensitive URL query parameters in addition to the global TOKEN, but the docstring still only mentions TOKEN redaction and control-char escaping. Please update the docstring to reflect the expanded behavior so callers/tests have an accurate contract.

Suggested change
"""Sanitize text for logging, ensuring TOKEN is redacted and control chars are escaped."""
"""
Sanitize text for logging.
This helper:
- Redacts values of common sensitive URL query parameters
(e.g. token, key, secret, password, auth, access_token, api_key).
- Redacts the global TOKEN value, if present.
- Escapes control characters to reduce log-injection and terminal issues.
"""

Copilot uses AI. Check for mistakes.
s = str(text)

# 1. Redact common sensitive query parameters in URLs (Defense in Depth)
# Matches ?param=value or &param=value
# Stops at &, whitespace, or quotes
s = re.sub(

Check warning

Code 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 warning

Code 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\"']+)",

Choose a reason for hiding this comment

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

critical

The regular expression r"([?&](?:token|key|secret|password|auth|access_token|api_key)=)([^&\s\"']+)" used for redacting sensitive query parameters is too restrictive. The character class [^&\s\"'] explicitly excludes single quotes ('), double quotes ("), and whitespace. This means if a sensitive query parameter's value contains any of these characters (e.g., token=abc'def or token=abc def), the redaction will stop prematurely, leaving a portion of the sensitive information exposed in the logs. This defeats the purpose of redacting sensitive data and poses a significant security risk. A more robust approach would be to redact the entire value until the next URL parameter delimiter (&) or the end of the string.

Suggested change
r"([?&](?:token|key|secret|password|auth|access_token|api_key)=)([^&\s\"']+)",
r"([?&](?:token|key|secret|password|auth|access_token|api_key)=)([^&]+)",

r"\1[REDACTED]",
s,
flags=re.IGNORECASE,
)
Comment on lines +156 to +161
Copy link

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

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

The new query-param redaction regex does not stop at URL fragments ('#'). Because # is allowed in the value character class, an input like ...?token=abc#section will redact the entire abc#section and drop the fragment text. Consider treating # as a terminator (and potentially other common delimiters) so only the parameter value is replaced and the rest of the message is preserved.

Copilot uses AI. Check for mistakes.

# 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)
Expand Down
56 changes: 56 additions & 0 deletions tests/test_security_log.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
import unittest

Check warning

Code scanning / Pylint (reported by Codacy)

Missing module docstring Warning test

Missing module docstring

Check warning

Code scanning / Pylintpython3 (reported by Codacy)

Missing module docstring Warning test

Missing module docstring
from main import sanitize_for_log

class TestSecurityLog(unittest.TestCase):

Check warning

Code scanning / Pylint (reported by Codacy)

Missing class docstring Warning test

Missing class docstring

Check warning

Code scanning / Pylintpython3 (reported by Codacy)

Missing class docstring Warning test

Missing class docstring
def test_redact_query_params(self):

Check warning

Code scanning / Pylint (reported by Codacy)

Missing method docstring Warning test

Missing method docstring

Check warning

Code 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'",

Choose a reason for hiding this comment

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

high

The test case at this line uses an input string that is already quoted: "'https://quoted.com?password=pass'". This format implies that the input text to sanitize_for_log is a string literal containing quotes, rather than a raw URL string. The sanitize_for_log function is designed to apply repr() for escaping control characters and then strip the outermost quotes. When the input itself is already quoted, repr() will escape these inner quotes, leading to an output that still contains escaped quotes (e.g., "'https://quoted.com?password=[REDACTED]'") which does not match the expected value of "https://quoted.com?password=[REDACTED]". To properly test the redaction of a URL containing a sensitive parameter with a value that includes a single quote, the input should be a raw URL string.

Suggested change
"'https://quoted.com?password=pass'",
"https://quoted.com?password=pass'",

"https://quoted.com?password=[REDACTED]"
Comment on lines +27 to +30
Copy link

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

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

This test case expects sanitize_for_log to strip the surrounding quotes from the input URL. With the current implementation, the outer quotes become escaped by repr() (e.g., \'...\') and will not be removed by the helper logic below, so this assertion will fail. Either remove this case, or change the expected value to match the actual sanitized output, or adjust sanitize_for_log to normalize/strip surrounding quotes before applying repr() if that’s desired behavior.

Suggested change
),
(
"'https://quoted.com?password=pass'",
"https://quoted.com?password=[REDACTED]"

Copilot uses AI. Check for mistakes.
)
]

for input_str, expected in test_cases:
# 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

Comment on lines +43 to +47

Choose a reason for hiding this comment

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

medium

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 = 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]'"

Check warning

Code scanning / Pylint (reported by Codacy)

Line too long (105/100) Warning test

Line too long (105/100)

Check warning

Code scanning / Pylintpython3 (reported by Codacy)

Line too long (105/100) Warning test

Line too long (105/100)

self.assertEqual(result_content, expected, f"Failed for input: {input_str}")

Choose a reason for hiding this comment

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

medium

Following the removal of redundant repr() handling logic, the comparison in the assertion should directly use the result from sanitize_for_log, as it already returns the unquoted string.

            self.assertEqual(result, expected, f"Failed for input: {input_str}")


Comment on lines +35 to +54
Copy link

Copilot AI Feb 3, 2026

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).

Suggested change
# 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}")

Copilot uses AI. Check for mistakes.
if __name__ == "__main__":

Check warning

Code scanning / Prospector (reported by Codacy)

expected 2 blank lines after class or function definition, found 1 (E305) Warning test

expected 2 blank lines after class or function definition, found 1 (E305)
unittest.main()
Loading