Pylint Exception logging

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

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,79 @@ 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):
+                                if "." and "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 +2976,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

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