diff --git a/tools/pylint-extensions/azure-pylint-guidelines-checker/README.md b/tools/pylint-extensions/azure-pylint-guidelines-checker/README.md index afb92720126..bf5bbe505b3 100644 --- a/tools/pylint-extensions/azure-pylint-guidelines-checker/README.md +++ b/tools/pylint-extensions/azure-pylint-guidelines-checker/README.md @@ -54,7 +54,7 @@ In the case of a false positive, use the disable command to remove the pylint er | Pylint checker name | How to fix this | How to disable this rule | Link to python guideline | -|----------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------|---------------------------------------------------------------------| ------------------------------------------------------------------------------------------------------- | +|----------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------|---------------------------------------------------------------------|---------------------------------------------------------------------------------------------------------| | client-method-should-not-use-static-method | Use module level functions instead. | # pylint:disable=client-method-should-not-use-static-method | [link](https://azure.github.io/azure-sdk/python_implementation.html#method-signatures) | | missing-client-constructor-parameter-credential | Add a credential parameter to the client constructor. Do not use plural form "credentials". | # pylint:disable=missing-client-constructor-parameter-credential | [link](https://azure.github.io/azure-sdk/python_design.html#client-configuration) | | missing-client-constructor-parameter-kwargs | Add a \*\*kwargs parameter to the client constructor. | # pylint:disable=missing-client-constructor-parameter-kwargs | [link](https://azure.github.io/azure-sdk/python_design.html#client-configuration) | @@ -95,4 +95,5 @@ In the case of a false positive, use the disable command to remove the pylint er | no-typing-import-in-type-check | Do not import typing under TYPE_CHECKING. | pylint:disable=no-typing-import-in-type-check | No Link. | | do-not-log-raised-errors | Do not log errors at `error` or `warning` level when error is raised in an exception block. | pylint:disable=do-not-log-raised-errors | No Link. | | do-not-use-legacy-typing | Do not use legacy (<Python 3.8) type hinting comments | pylint:disable=do-not-use-legacy-typing | No Link. -| do-not-import-asyncio | Do not import asyncio directly. | pylint:disable=do-not-import-asyncio | No Link. | \ No newline at end of file +| do-not-import-asyncio | Do not import asyncio directly. | pylint:disable=do-not-import-asyncio | No Link. | +| do-not-log-exceptions | Do not log exceptions in levels other than debug, otherwise it can reveal sensitive information | pylint:disable=do-not-log-exceptions | [link](https://azure.github.io/azure-sdk/python_implementation.html#python-logging-sensitive-info) | \ No newline at end of file diff --git a/tools/pylint-extensions/azure-pylint-guidelines-checker/pylint_guidelines_checker.py b/tools/pylint-extensions/azure-pylint-guidelines-checker/pylint_guidelines_checker.py index 3520efc6f00..da7dcfde095 100644 --- a/tools/pylint-extensions/azure-pylint-guidelines-checker/pylint_guidelines_checker.py +++ b/tools/pylint-extensions/azure-pylint-guidelines-checker/pylint_guidelines_checker.py @@ -2614,7 +2614,7 @@ def visit_functiondef(self, node): try: if node.returns.as_string() == "None": # If there are residual comment typehints or no return value, - # we dont want to throw an error + # we don't want to throw an error return if node.name.startswith("delete") and node.parent.name.endswith("Client"): if node.returns.as_string() != "None": @@ -2865,10 +2865,81 @@ def visit_import(self, node): # [Pylint] custom linter check for invalid use of @overload #3229 -# [Pylint] Custom Linter check for Exception Logging #3227 + + +class DoNotLogExceptions(BaseChecker): + + """Rule to check that exceptions aren't logged""" + + name = "do-not-log-exceptions" + priority = -1 + msgs = {"C4766": ( + "Do not log exceptions. See Details:" + " https://azure.github.io/azure-sdk/python_implementation.html#python-logging-sensitive-info", + "do-not-log-exceptions", + "Do not log exceptions in levels other than debug, it can otherwise reveal sensitive information", + ), + } + + def visit_try(self, node): + """Check that exceptions aren't logged in exception blocks. + Go through exception block and branches and ensure error hasn't been logged. + """ + # Return a list of exception blocks + except_block = node.handlers + # Iterate through each exception block + for nod in except_block: + # Get exception blocks with an exception name + if nod.name is not None: + exception_name = nod.name.name + self.check_for_logging(nod.body, exception_name) + + def check_for_logging(self, node, exception_name): + """ Helper function - checks nodes to see if logging has occurred at all + levels. + """ + levels_matches = [".warning", ".error", ".info", ".debug"] + for j in node: + if isinstance(j, astroid.Expr): + expression = j.as_string().lower() + if any(x in expression for x in levels_matches) and "logger" in expression: + # Check for variables after strings + end_finder = expression.rfind("'") + delimiters = ["(", "{", "}", ")", "\"", ",", "'"] + if end_finder != -1: + expression_a = expression[end_finder + 1:] + # If there are variables after a string + if len(expression_a) > 1: + expression = expression_a + for delimiter in delimiters: + expression = " ".join(expression.split(delimiter)) + expression1 = expression.split() + # Check for presence of exception name + for i in range(len(expression1)): + if exception_name == expression1[i]: + if i+1 < len(expression1): + # TODO: Investigate whether there are any other cases we don't want to raise a Pylint + # error + if ".__name__" not in expression1[i+1]: + self.add_message( + msgid=f"do-not-log-exceptions", + node=j, + confidence=None, + ) + else: + self.add_message( + msgid=f"do-not-log-exceptions", + node=j, + confidence=None, + ) + if isinstance(j, astroid.If): + self.check_for_logging(j.body, exception_name) + # Check any 'elif' or 'else' branches + self.check_for_logging(j.orelse, exception_name) + + # [Pylint] Address Commented out Pylint Custom Plugin Checkers #3228 # [Pylint] Add a check for connection_verify hardcoded settings #35355 -# [Pylint] Refactor test suite for custom pylint checkers to use files instead of docstrings #3233 # [Pylint] Investigate pylint rule around missing dependency #3231 @@ -2907,12 +2978,11 @@ def register(linter): linter.register_checker(NoImportTypingFromTypeCheck(linter)) linter.register_checker(DoNotUseLegacyTyping(linter)) linter.register_checker(DoNotLogErrorsEndUpRaising(linter)) + linter.register_checker(DoNotLogExceptions(linter)) # [Pylint] custom linter check for invalid use of @overload #3229 - # [Pylint] Custom Linter check for Exception Logging #3227 # [Pylint] Address Commented out Pylint Custom Plugin Checkers #3228 # [Pylint] Add a check for connection_verify hardcoded settings #35355 - # [Pylint] Refactor test suite for custom pylint checkers to use files instead of docstrings #3233 # [Pylint] Investigate pylint rule around missing dependency #3231 # disabled by default, use pylint --enable=check-docstrings if you want to use it diff --git a/tools/pylint-extensions/azure-pylint-guidelines-checker/tests/test_files/do_not_log_exceptions.py b/tools/pylint-extensions/azure-pylint-guidelines-checker/tests/test_files/do_not_log_exceptions.py new file mode 100644 index 00000000000..84865fe6344 --- /dev/null +++ b/tools/pylint-extensions/azure-pylint-guidelines-checker/tests/test_files/do_not_log_exceptions.py @@ -0,0 +1,96 @@ +from plugin import logger + + +# test_logging_levels_logged_str_exception +def test_logging_levels_logged_str_exception(): + try: # @ + add = 1 + 2 + except Exception as ex: + logger.error("Error" + str(ex)) # @ + logger.warning(str(ex)) + logger.info(str(ex)) + logger.debug(str(ex)) + + +# test_logging_levels_logged_repr_exception +def test_logging_levels_logged_repr_exception(): + try: # @ + add = 1 + 2 + except Exception as ex: + logger.error(repr(ex)) # @ + logger.warning(repr(ex)) + logger.info(repr(ex)) + logger.debug(repr(ex)) + + +# test_regular_logging_ok +def test_regular_logging_ok(): + try: # @ + add = 1 + 2 + except Exception as ex: + logger.error("Example 1") # @ + logger.warning("This is another example") + logger.info("Random logging") + logger.debug("Logging") + + +# test_logging_str_exception_branches +def test_logging_str_exception_branches(): + try: # @ + add = 1 + 2 + except Exception as ex: + if ex.code == "Retryable": + logger.error(str(ex)) + return True + elif Exception != BaseException: + logger.warning(repr(ex)) + return False + else: + logger.info(str(ex)) # @ + + +# test_other_logging_fails +def test_other_logging_fails(): + try: # @ + add = 1 + 2 + except Exception as ex: + if ex.code == "Retryable": + logger.error("Something went wrong: {ex}. Try again") + return True + else: + logger.warning(ex) + return False + + +# test_no_logging_and_no_exception_name_ok +def test_no_logging_and_no_exception_name_ok(): + try: + add = 1 + 2 + except Exception as ex: + self.errors.appendleft(ex) + except Exception as ex: # pylint: disable=broad-except + _logger.warning( + "Exception occurred when instrumenting: %s.", + lib_name, + exc_info=ex, + ) + except (OSError, PermissionError) as e: + logger.warning( + "Failed to read on-disk cache for component due to %s. " + "Please check if the file %s is in use or current user doesn't have the permission.", + type(e).__name__, + on_disk_cache_path.as_posix(), + ) + + +# test_logging_without_exception_name +def test_logging_without_exception_name(): + try: + add = 1 + 2 + except Exception as exception: + if exception.code == "Retryable": + _LOGGER.info( + "%r returns an exception %r", self._container_id, last_exception + ) + else: + module_logger.debug("Skipping file upload, reason: %s", str(e.reason)) diff --git a/tools/pylint-extensions/azure-pylint-guidelines-checker/tests/test_pylint_custom_plugins.py b/tools/pylint-extensions/azure-pylint-guidelines-checker/tests/test_pylint_custom_plugins.py index ca1b73a443c..2cd88fbf13f 100644 --- a/tools/pylint-extensions/azure-pylint-guidelines-checker/tests/test_pylint_custom_plugins.py +++ b/tools/pylint-extensions/azure-pylint-guidelines-checker/tests/test_pylint_custom_plugins.py @@ -3598,7 +3598,195 @@ def test_extra_nested_branches_exception_logged(self, setup): self.checker.visit_try(try_node) # [Pylint] custom linter check for invalid use of @overload #3229 -# [Pylint] Custom Linter check for Exception Logging #3227 + + +class TestDoNotLogExceptions(pylint.testutils.CheckerTestCase): + + """Test that any errors are not logged in exception blocks.""" + + CHECKER_CLASS = checker.DoNotLogExceptions + + @pytest.fixture(scope="class") + def setup(self): + file = open( + os.path.join(TEST_FOLDER, "test_files", "do_not_log_exceptions.py") + ) + node = astroid.parse(file.read()) + file.close() + return node + + def test_logging_levels_logged_str_exception(self, setup): + """Check that exceptions aren't logged at all logging levels in the exception block.""" + try_node = setup.body[1].body[0] + error_node = setup.body[1].body[0].handlers[0].body[0] + warning_node = setup.body[1].body[0].handlers[0].body[1] + info_node = setup.body[1].body[0].handlers[0].body[2] + debug_node = setup.body[1].body[0].handlers[0].body[3] + with self.assertAddsMessages( + pylint.testutils.MessageTest( + msg_id="do-not-log-exceptions", + line=9, + node=error_node, + col_offset=8, + end_line=9, + end_col_offset=39, + ), + pylint.testutils.MessageTest( + msg_id="do-not-log-exceptions", + line=10, + node=warning_node, + col_offset=8, + end_line=10, + end_col_offset=31, + ), + pylint.testutils.MessageTest( + msg_id="do-not-log-exceptions", + line=11, + node=info_node, + col_offset=8, + end_line=11, + end_col_offset=28, + ), + pylint.testutils.MessageTest( + msg_id="do-not-log-exceptions", + line=12, + node=debug_node, + col_offset=8, + end_line=12, + end_col_offset=29, + ) + ): + self.checker.visit_try(try_node) + + def test_logging_levels_logged_repr_exception(self, setup): + """Check that exceptions aren't logged at all logging levels in the exception block.""" + try_node = setup.body[2].body[0] + error_node = setup.body[2].body[0].handlers[0].body[0] + warning_node = setup.body[2].body[0].handlers[0].body[1] + info_node = setup.body[2].body[0].handlers[0].body[2] + debug_node = setup.body[2].body[0].handlers[0].body[3] + with self.assertAddsMessages( + pylint.testutils.MessageTest( + msg_id="do-not-log-exceptions", + line=20, + node=error_node, + col_offset=8, + end_line=20, + end_col_offset=30, + ), + pylint.testutils.MessageTest( + msg_id="do-not-log-exceptions", + line=21, + node=warning_node, + col_offset=8, + end_line=21, + end_col_offset=32, + ), + pylint.testutils.MessageTest( + msg_id="do-not-log-exceptions", + line=22, + node=info_node, + col_offset=8, + end_line=22, + end_col_offset=29, + ), + pylint.testutils.MessageTest( + msg_id="do-not-log-exceptions", + line=23, + node=debug_node, + col_offset=8, + end_line=23, + end_col_offset=30, + ) + ): + self.checker.visit_try(try_node) + + def test_regular_logging_ok(self, setup): + """Check that normal logging is ok in the exception block.""" + try_node = setup.body[3].body[0] + with self.assertNoMessages(): + self.checker.visit_try(try_node) + + def test_logging_str_exception_branches(self, setup): + """Check that exceptions aren't logged at all logging levels in the exception block.""" + try_node = setup.body[4].body[0] + error_node = setup.body[4].body[0].handlers[0].body[0].body[0] + warning_node = setup.body[4].body[0].handlers[0].body[0].orelse[0].body[0] + info_node = setup.body[4].body[0].handlers[0].body[0].orelse[0].orelse[0] + with self.assertAddsMessages( + pylint.testutils.MessageTest( + msg_id="do-not-log-exceptions", + line=43, + node=error_node, + col_offset=12, + end_line=43, + end_col_offset=33, + ), + pylint.testutils.MessageTest( + msg_id="do-not-log-exceptions", + line=46, + node=warning_node, + col_offset=12, + end_line=46, + end_col_offset=36, + ), + pylint.testutils.MessageTest( + msg_id="do-not-log-exceptions", + line=49, + node=info_node, + col_offset=12, + end_line=49, + end_col_offset=32, + ) + ): + self.checker.visit_try(try_node) + + def test_other_logging_fails(self, setup): + """Check that exceptions aren't logged at all logging levels in the exception block.""" + try_node = setup.body[5].body[0] + error_node = setup.body[5].body[0].handlers[0].body[0].body[0] + warning_node = setup.body[5].body[0].handlers[0].body[0].orelse[0] + with self.assertAddsMessages( + pylint.testutils.MessageTest( + msg_id="do-not-log-exceptions", + line=58, + node=error_node, + col_offset=12, + end_line=58, + end_col_offset=65, + ), + pylint.testutils.MessageTest( + msg_id="do-not-log-exceptions", + line=61, + node=warning_node, + col_offset=12, + end_line=61, + end_col_offset=30, + ) + ): + self.checker.visit_try(try_node) + + def test_no_logging_and_no_exception_name_ok(self, setup): + """Check that no logging is ok in the exception block.""" + try_node = setup.body[6].body[0] + with self.assertNoMessages(): + self.checker.visit_try(try_node) + + def test_logging_without_exception_name(self, setup): + """Check that logging without exception name is ok in the exception block.""" + try_node = setup.body[7].body[0] + with self.assertNoMessages(): + self.checker.visit_try(try_node) + + def test_guidelines_link_active(self): + url = "https://azure.github.io/azure-sdk/python_implementation.html#python-logging-sensitive-info" + config = Configuration() + client = PipelineClient(url, config=config) + request = client.get(url) + response = client._pipeline.run(request) + assert response.http_response.status_code == 200 + + # [Pylint] Address Commented out Pylint Custom Plugin Checkers #3228 # [Pylint] Add a check for connection_verify hardcoded settings #35355 # [Pylint] Investigate pylint rule around missing dependency #3231