🛡️ Sentinel: Redact sensitive query parameters in logs#157
🛡️ Sentinel: Redact sensitive query parameters in logs#157
Conversation
Co-authored-by: abhimehro <84992105+abhimehro@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Merging to
|
Summary of ChangesHello @abhimehro, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant security enhancement by automatically redacting sensitive query parameters from URLs before they are written to logs. This measure prevents the inadvertent exposure of confidential information such as API keys or tokens, thereby strengthening the application's security posture and ensuring log integrity. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| self.assertEqual(result_content, expected, f"Failed for input: {input_str}") | ||
|
|
||
| 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
| # 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 warning
Code scanning / Pylint (reported by Codacy)
Variable name "s" doesn't conform to snake_case naming style Warning
| @@ -0,0 +1,56 @@ | |||
| import unittest | |||
Check warning
Code scanning / Pylint (reported by Codacy)
Missing module docstring Warning test
| import unittest | ||
| from main import sanitize_for_log | ||
|
|
||
| class TestSecurityLog(unittest.TestCase): |
Check warning
Code scanning / Pylint (reported by Codacy)
Missing class docstring Warning test
| from main import sanitize_for_log | ||
|
|
||
| class TestSecurityLog(unittest.TestCase): | ||
| def test_redact_query_params(self): |
Check warning
Code scanning / Pylint (reported by Codacy)
Missing method docstring Warning test
| # 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
| # 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 warning
Code scanning / Pylintpython3 (reported by Codacy)
Variable name "s" doesn't conform to snake_case naming style Warning
| @@ -0,0 +1,56 @@ | |||
| import unittest | |||
Check warning
Code scanning / Pylintpython3 (reported by Codacy)
Missing module docstring Warning test
| import unittest | ||
| from main import sanitize_for_log | ||
|
|
||
| class TestSecurityLog(unittest.TestCase): |
Check warning
Code scanning / Pylintpython3 (reported by Codacy)
Missing class docstring Warning test
| from main import sanitize_for_log | ||
|
|
||
| class TestSecurityLog(unittest.TestCase): | ||
| def test_redact_query_params(self): |
Check warning
Code scanning / Pylintpython3 (reported by Codacy)
Missing function or method docstring Warning test
| # 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 / Pylintpython3 (reported by Codacy)
Line too long (105/100) Warning test
There was a problem hiding this comment.
Pull request overview
Adds defense-in-depth log sanitization by redacting common sensitive URL query parameters (e.g., token/key/secret/password) before emitting log messages.
Changes:
- Extend
sanitize_for_logto redact sensitive query parameter values via regex substitution. - Add a new unit test module covering expected query-parameter redaction behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
main.py |
Adds regex-based query-parameter redaction in sanitize_for_log prior to existing TOKEN redaction and control-char escaping. |
tests/test_security_log.py |
Introduces tests for URL query parameter redaction behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -149,8 +149,21 @@ def check_env_permissions(env_path: str = ".env") -> None: | |||
| def sanitize_for_log(text: Any) -> str: | |||
| """Sanitize text for logging, ensuring TOKEN is redacted and control chars are escaped.""" | |||
There was a problem hiding this comment.
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.
| """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. | |
| """ |
| ), | ||
| ( | ||
| "'https://quoted.com?password=pass'", | ||
| "https://quoted.com?password=[REDACTED]" |
There was a problem hiding this comment.
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.
| ), | |
| ( | |
| "'https://quoted.com?password=pass'", | |
| "https://quoted.com?password=[REDACTED]" |
| # 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}") | ||
|
|
There was a problem hiding this comment.
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}") |
| s = re.sub( | ||
| r"([?&](?:token|key|secret|password|auth|access_token|api_key)=)([^&\s\"']+)", | ||
| r"\1[REDACTED]", | ||
| s, | ||
| flags=re.IGNORECASE, | ||
| ) |
There was a problem hiding this comment.
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.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request introduces a valuable security enhancement by redacting sensitive query parameters in logs, which is a good defense-in-depth measure. The addition of a new test file tests/test_security_log.py is also a positive step towards ensuring the correctness of this feature. However, there are critical issues identified related to the completeness of sensitive data redaction due to an overly restrictive regular expression, and an incorrect test case input that misrepresents expected behavior. Additionally, there is redundant logic in the new test file that can be streamlined for better maintainability.
| # Matches ?param=value or ¶m=value | ||
| # Stops at &, whitespace, or quotes | ||
| s = re.sub( | ||
| r"([?&](?:token|key|secret|password|auth|access_token|api_key)=)([^&\s\"']+)", |
There was a problem hiding this comment.
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.
| r"([?&](?:token|key|secret|password|auth|access_token|api_key)=)([^&\s\"']+)", | |
| r"([?&](?:token|key|secret|password|auth|access_token|api_key)=)([^&]+)", |
| "https://safe.com?public=data" | ||
| ), | ||
| ( | ||
| "'https://quoted.com?password=pass'", |
There was a problem hiding this comment.
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.
| "'https://quoted.com?password=pass'", | |
| "https://quoted.com?password=pass'", |
| if len(result) >= 2 and result[0] == result[-1] and result[0] in ("'", '"'): | ||
| result_content = result[1:-1] | ||
| else: | ||
| result_content = result | ||
|
|
There was a problem hiding this comment.
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| # 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}") |
There was a problem hiding this comment.
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}")…itization, add dry-run plan details Incorporates the best changes from 36 Jules PRs, addressing review feedback: Bolt (Performance) - from PR #173: - Pre-compile PROFILE_ID_PATTERN and RULE_PATTERN at module level - Use compiled patterns in is_valid_profile_id_format, validate_profile_id, and is_valid_rule - Supersedes PRs: #140, #143, #152, #155, #158, #161, #167, #170, #173 Sentinel (Security) - from PR #172 with review feedback: - Enhance sanitize_for_log to redact Basic Auth credentials in URLs - Redact sensitive query parameters (token, key, secret, password, etc.) - Handle fragment separators (#) per Gemini Code Assist review - Use [^&#\s]* pattern per Copilot reviewer suggestion - Update docstring per reviewer suggestion - Supersedes PRs: #142, #145, #148, #151, #154, #157, #160, #169, #172 Palette (UX) - from PR #174 with lint fixes: - Add print_plan_details function for dry-run visibility - Fix duplicate render_progress_bar definition bug - Supersedes PRs: #139, #141, #144, #147, #150, #153, #156, #159, #162, #165, #168, #171, #174 Also: #146, #149, #164 (parallel folder deletion) and #166 (auto-fix .env perms) are independent features not consolidated here. Co-authored-by: abhimehro <84992105+abhimehro@users.noreply.github.com>
abhimehro
deleted the
sentinel/redact-sensitive-logs-17711217631418324444
branch
…itization, add dry-run plan details Incorporates the best changes from 36 Jules PRs, addressing review feedback: Bolt (Performance) - from PR #173: - Pre-compile PROFILE_ID_PATTERN and RULE_PATTERN at module level - Use compiled patterns in is_valid_profile_id_format, validate_profile_id, and is_valid_rule - Supersedes PRs: #140, #143, #152, #155, #158, #161, #167, #170, #173 Sentinel (Security) - from PR #172 with review feedback: - Enhance sanitize_for_log to redact Basic Auth credentials in URLs - Redact sensitive query parameters (token, key, secret, password, etc.) - Handle fragment separators (#) per Gemini Code Assist review - Use [^&#\s]* pattern per Copilot reviewer suggestion - Update docstring per reviewer suggestion - Supersedes PRs: #142, #145, #148, #151, #154, #157, #160, #169, #172 Palette (UX) - from PR #174 with lint fixes: - Add print_plan_details function for dry-run visibility - Fix duplicate render_progress_bar definition bug - Supersedes PRs: #139, #141, #144, #147, #150, #153, #156, #159, #162, #165, #168, #171, #174 Also: #146, #149, #164 (parallel folder deletion) and #166 (auto-fix .env perms) are independent features not consolidated here. Co-authored-by: abhimehro <84992105+abhimehro@users.noreply.github.com>
Redacts common sensitive query parameters (token, key, secret, etc.) from URLs in log messages to prevent accidental information disclosure. This is a defense-in-depth measure.
PR created automatically by Jules for task 17711217631418324444 started by @abhimehro