-
Notifications
You must be signed in to change notification settings - Fork 21
DEP-161: Mask sensitive information in API logs #2733
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
c539ac2
8464ae0
b909772
f7a38cd
1cbf4d7
09d3135
2743558
577e012
4be700e
f3eee22
8df18b9
ef4d4ea
7043399
76fd449
c652127
dad93f6
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 |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| # Pylint Plugins | ||
|
|
||
| This directory contains custom Pylint checkers for the MET API. | ||
|
|
||
| ## no_print_checker | ||
|
|
||
| **Purpose:** Prevents use of `print()` statements which bypass sensitive data masking. | ||
|
|
||
| **Why:** Print statements output directly to stdout/stderr and bypass the logging system's `SensitiveDataFilter`. This creates a security risk where passwords, tokens, API keys, and other credentials could be exposed in logs. | ||
|
|
||
| **What it does:** | ||
| - Flags any use of `print()` function with error code **W9001** | ||
| - Exception: Allows print in `config.py` for startup messages before logging is initialized | ||
|
|
||
| **Usage:** | ||
| Instead of: | ||
| ```python | ||
| print(f'User ID: {user_id}') | ||
| print(f'Database URL: {db_url}') | ||
| ``` | ||
|
|
||
| Use: | ||
| ```python | ||
| from flask import current_app | ||
| current_app.logger.info('User ID: %s', user_id) | ||
|
|
||
| # or | ||
| import logging | ||
| logger = logging.getLogger(__name__) | ||
| logger.info('Database URL: %s', db_url) | ||
| ``` | ||
|
|
||
| **Configuration:** | ||
| The checker is automatically loaded via `.pylintrc`: | ||
| ```ini | ||
| load-plugins=pylint_plugins.no_print_checker | ||
| ``` | ||
|
|
||
| **Testing:** | ||
| ```bash | ||
| # Test with a file containing print statements | ||
| pylint --rcfile=.pylintrc path/to/file.py | ||
|
|
||
| # Should show: | ||
| # W9001: Print statement found - use logging to ensure sensitive data masking | ||
| ``` | ||
|
|
||
| ## Adding New Checkers | ||
|
|
||
| 1. Create a new file in this directory (e.g., `my_checker.py`) | ||
| 2. Implement a class that inherits from `pylint.checkers.BaseChecker` | ||
| 3. Define your messages and visit methods | ||
| 4. Add a `register()` function | ||
| 5. Update `.pylintrc` to load your plugin: | ||
| ```ini | ||
| load-plugins=pylint_plugins.no_print_checker,pylint_plugins.my_checker | ||
| ``` | ||
|
|
||
| See [Pylint documentation](https://pylint.readthedocs.io/en/latest/development_guide/how_tos/custom_checkers.html) for more details. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| """Pylint plugins for MET API code quality checks.""" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| """Pylint plugin to check for print statements. | ||
|
|
||
| Print statements bypass logging masking and pose a security risk by potentially | ||
| exposing sensitive data (passwords, tokens, API keys, etc.) in logs. | ||
|
|
||
| Use current_app.logger or logging.getLogger(__name__) instead. | ||
| """ | ||
|
|
||
| from astroid import nodes | ||
| from pylint.checkers import BaseChecker | ||
|
|
||
|
|
||
| class NoPrintChecker(BaseChecker): | ||
| """Check for print statements which bypass logging masking.""" | ||
|
|
||
| name = "no-print" | ||
| msgs = { | ||
| "W9001": ( | ||
| "Print statement found - use logging to ensure sensitive data masking", | ||
| "print-statement-found", | ||
| "Print statements bypass the logging masking system and may expose " | ||
| "sensitive data (passwords, tokens, API keys). Use current_app.logger " | ||
| "or logging.getLogger(__name__) instead.", | ||
| ), | ||
| } | ||
|
|
||
| def visit_call(self, node: nodes.Call) -> None: | ||
| """Check if the node is a print function call.""" | ||
| if isinstance(node.func, nodes.Name) and node.func.name == "print": | ||
| # Allow print in config.py for startup messages before logging is initialized | ||
|
Contributor
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. Nice caveat |
||
| if node.root().file and "config.py" in node.root().file: | ||
| return | ||
| self.add_message("print-statement-found", node=node) | ||
|
|
||
|
|
||
| def register(linter): | ||
| """Register the checker.""" | ||
| linter.register_checker(NoPrintChecker(linter)) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ | |
|
|
||
| from __future__ import annotations | ||
|
|
||
| import logging | ||
| from datetime import datetime | ||
| from typing import List, Optional | ||
|
|
||
|
|
@@ -197,7 +198,7 @@ def close_engagements_due(cls) -> List[Engagement]: | |
| def publish_scheduled_engagements_due(cls) -> List[Engagement]: | ||
| """Update scheduled engagements to published.""" | ||
| datetime_due = datetime.utcnow() | ||
| print('Publish due date (UTC) ------------------------', datetime_due) | ||
| logging.getLogger(__name__).debug('Publish due date (UTC): %s', datetime_due) | ||
|
Contributor
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. Specifiers bring me back to php <3 |
||
| update_fields = { | ||
| 'status_id': Status.Published.value, | ||
| 'published_date': datetime.utcnow(), | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -69,15 +69,13 @@ def upload_template(self, template_file_path): | |
| template = {'template': ('template', file_handle, 'multipart/form-data')} | ||
|
|
||
| current_app.logger.info('Uploading template %s', template_file_path) | ||
| print('Uploading template %s', template_file_path) | ||
|
Contributor
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. Sometimes logging is just too verbose. I'm guessing it's intentional to have no replacement logging here.
Collaborator
Author
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. Yes; plus it exposed file path data which I couldn't think of a good way to parse separately from URL paths. |
||
| response = self._post_upload_template(headers, url, template) | ||
|
|
||
| if response.status_code == HTTPStatus.OK: | ||
| if response.headers.get('X-Template-Hash') is None: | ||
| raise ValueError('Data not found') | ||
|
|
||
| current_app.logger.info('Returning new hash %s', response.headers['X-Template-Hash']) | ||
| print('Returning new hash %s', response.headers['X-Template-Hash']) | ||
| return response.headers['X-Template-Hash'] | ||
|
|
||
| response_json = json.loads(response.content) | ||
|
|
@@ -86,7 +84,6 @@ def upload_template(self, template_file_path): | |
| match = re.findall(r"Hash '(.*?)'", response_json['detail']) | ||
| if match: | ||
| current_app.logger.info('Template already hashed with code %s', match[0]) | ||
| print('Template already hashed with code %s', match[0]) | ||
| return match[0] | ||
|
|
||
| raise ValueError('Data not found') | ||
|
|
||
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.
Here is a crazy idea, maybe we could wrap the print function in a user defined function that is deprecated, so the coder sees a pretty little on-hover suggestion to use current_app.logger when they try to use print.
import builtins
builtins.print = deprecated_print