Added config option to the plugin to turn ON/OFF warning R8009

Takishima · Takishima · commit 709b21d99ee3 · 2021-07-25T19:51:12.000+02:00
Also added W8012 to warn about "unsafe" value for the mode argument to
`os.open` (not implemented yet and missing unit tests).
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Added
+
+-   Add plugin option to control whether we favour `os.open` over the builtin `open`
+-   Added W8012 to warn when using `os.open` with unsafe permissions
+
 ### Repository
 
 -   Update pre-commit hooks
diff --git a/README.md b/README.md
@@ -25,8 +25,31 @@ pylint plugin that enforces some secure coding standards.
 | R8009 | Use of builtin `open` for writing is discouraged in favor of `os.open` to allow for setting file permissions |
 | E8010 | Avoid using `os.popen()` as it internally uses `subprocess.Popen` with `shell=True`                          |
 | E8011 | Use of `shlex.quote()` should be avoided on non-POSIX platforms                                              |
+| W8012 | Avoid using `os.open` with unsafe permissions permissions                                                    |
 
 
+## Plugin configuration options
+
+### File permissions when using `os.open`
+
+Since version 1.3.0 you can control whether this plugin favors `os.open` over the builtin `open` function when opening files.
+
+```toml
+    [tool.pylint.plugins]
+    os-open-mode = '0'            # check disabled
+    os-open-mode = '0o000'        # check disabled
+    os-open-mode = '493'          # all modes from 0 to 0o755
+    os-open-mode = '0o755'        # all modes from 0 to 0o755
+    os-open-mode = '0o755,'       # only 0o755
+    os-open-mode = '0o644,0o755'  # only 0o644 and 0o755
+```
+
+You can also specify this option directly on the command line:
+
+```sh
+python3 -m pylint --load-plugins=pylint_secure_coding_standard --os-open-mode='0o755'
+```
+
 ## Pre-commit hook
 
 See [pre-commit](https://github.com/pre-commit/pre-commit) for instructions
diff --git a/pylint_secure_coding_standard.py b/pylint_secure_coding_standard.py
@@ -226,9 +226,24 @@ def _is_shlex_quote_call(node):
 class SecureCodingStandardChecker(BaseChecker):
     """Plugin class."""
 
-    __implements__ = IAstroidChecker
+    DEFAULT_MAX_MODE = 0o755
+    W8012_DISPLAY_MSG = 'Avoid using `os.open` with unsafe permissions permissions'
+
+    __implements__ = (IAstroidChecker,)
 
     name = 'secure-coding-standard'
+    options = (
+        (
+            'os-open-mode',
+            {
+                'default': False,
+                'type': 'string',
+                'metavar': '<os-open-mode>',
+                'help': 'Integer or comma-separated list of integers (octal or decimal) of allowed modes. If set to a '
+                'truthful value (ie. >0 or non-empty list), this checker will prefer `os.open` over the builtin `open`',
+            },
+        ),
+    )
     priority = -1
 
     msg = {
@@ -294,9 +309,18 @@ class SecureCodingStandardChecker(BaseChecker):
             'avoid-shlex-quote-on-non-posix',
             'Use of `shlex.quote()` should be avoided on non-POSIX platforms (such as Windows)',
         ),
+        'W8012': (
+            W8012_DISPLAY_MSG,
+            'os-open-unsafe-permissions',
+            'Avoid using `os.open` with unsafe file permissions (by default 0 <= mode <= 0o755)',
+        ),
     }
 
-    options = {}
+    def __init__(self, *args, **kwargs):
+        """Initialize a SecureCodingStandardChecker object."""
+        super().__init__(*args, **kwargs)
+        self._prefer_os_open = False
+        self._os_open_mode_allowed = []
 
     def visit_call(self, node):
         """Visitor method called for astroid.Call nodes."""
@@ -316,7 +340,7 @@ def visit_call(self, node):
             self.add_message('avoid-shell-true', node=node)
         elif _is_os_popen_call(node):
             self.add_message('avoid-os-popen', node=node)
-        elif _is_builtin_open_for_writing(node):
+        elif _is_builtin_open_for_writing(node) and self._prefer_os_open:
             self.add_message('replace-builtin-open', node=node)
         elif isinstance(node.func, astroid.Name) and (node.func.name in ('eval', 'exec')):
             self.add_message('avoid-eval-exec', node=node)
@@ -354,15 +378,84 @@ def visit_importfrom(self, node):
 
     def visit_with(self, node):
         """Visitor method called for astroid.With nodes."""
-        for item in node.items:
-            if item and isinstance(item[0], astroid.Call) and _is_builtin_open_for_writing(item[0]):
-                self.add_message('replace-builtin-open', node=node)
+        if self._prefer_os_open:
+            for item in node.items:
+                if item and isinstance(item[0], astroid.Call) and _is_builtin_open_for_writing(item[0]):
+                    self.add_message('replace-builtin-open', node=node)
 
     def visit_assert(self, node):
         """Visitor method called for astroid.Assert nodes."""
         self.add_message('avoid-assert', node=node)
 
+    def set_os_open_mode(self, arg):
+        """
+        Control whether we prefer `os.open` over the builtin `open`.
+
+        Args:
+            arg (str): String with with mode value. Can be either of:
+                - 'yes', 'y', 'true' (case-insensitive)
+                    The maximum mode value is then set to self.DEFAULT_MAX_MODE
+                - a single octal or decimal integer
+                    The maximum mode value is then set to that integer value
+                - a comma-separated list of integers (octal or decimal)
+                    The allowed mode values are then those found in the list
+                - anything else will disable the feature
+        """
+
+        def _str_to_int(arg):
+            try:
+                return int(arg, 8)
+            except ValueError:
+                return int(arg)
+
+        def _update_display_msg(suffix=''):
+            self.msg['W8012'] = (self.W8012_DISPLAY_MSG + suffix, self.msg['W8012'][1], self.msg['W8012'][2])
+
+        arg = arg.lower()
+        modes = [mode.strip() for mode in arg.split(',')]
+
+        if len(modes) > 1:
+            # Lists of allowed modes
+            try:
+                self._os_open_mode_allowed = [_str_to_int(mode) for mode in modes if mode]
+                if not self._os_open_mode_allowed:
+                    raise ValueError('Calculated empty value for `os_open_mode`!')
+                self._prefer_os_open = True
+                _update_display_msg(suffix=f' (mode in {modes})')
+            except ValueError as error:
+                raise ValueError(f'Unable to convert {modes} elements to integers!') from error
+        elif modes and modes[0]:
+            # Single values (ie. max allowed value for mode)
+            try:
+                val = _str_to_int(arg)
+                self._prefer_os_open = val > 0
+                if self._prefer_os_open:
+                    self._os_open_mode_allowed = list(range(0, val + 1))
+                    _update_display_msg(suffix=f' (mode <= {arg})')
+                else:
+                    self._os_open_mode_allowed.clear()
+            except ValueError as error:
+                if arg in ('y', 'yes', 'true'):
+                    self._prefer_os_open = True
+                    self._os_open_mode_allowed = list(range(0, self.DEFAULT_MAX_MODE + 1))
+                    _update_display_msg(suffix=f' (mode <= {oct(self.DEFAULT_MAX_MODE)})')
+                elif arg in ('n', 'no', 'false'):
+                    self._prefer_os_open = False
+                    self._os_open_mode_allowed.clear()
+                else:
+                    raise ValueError(f'Invalid value for `os_open_mode`: {arg}!') from error
+        else:
+            raise ValueError(f'Invalid value for `os_open_mode`: {arg}!')
+
 
 def register(linter):  # pragma: no cover
     """Register the plugin to Pylint."""
     linter.register_checker(SecureCodingStandardChecker(linter))
+
+
+def load_configuration(linter):  # pragma: no cover
+    """Load data from the configuration file."""
+    for checker in linter.get_checkers():
+        if isinstance(checker, SecureCodingStandardChecker):
+            checker.set_os_open_mode(checker.config.os_open_mode)
+            break
diff --git a/tests/builtin_open_test.py b/tests/builtin_open_test.py
@@ -41,6 +41,8 @@ def test_builtin_open_ok(self):
         with_nodes = nodes[5:]
 
         with self.assertNoMessages():
+            self.checker.set_os_open_mode('True')
+            assert self.checker._prefer_os_open
             for idx, node in enumerate(call_nodes):
                 self.checker.visit_call(node)
             for idx, node in enumerate(with_nodes):
@@ -70,13 +72,27 @@ def test_builtin_open_ok(self):
     )
 
     @pytest.mark.parametrize('s', _calls_not_ok)
-    def test_builtin_open_call(self, s):
+    @pytest.mark.parametrize('os_open_mode', (False, True))
+    def test_builtin_open_call(self, s, os_open_mode):
         node = astroid.extract_node(s + ' #@')
-        with self.assertAddsMessages(pylint.testutils.Message(msg_id='replace-builtin-open', node=node)):
-            self.checker.visit_call(node)
+        self.checker.set_os_open_mode(str(os_open_mode))
+        assert self.checker._prefer_os_open == os_open_mode
+        if os_open_mode:
+            with self.assertAddsMessages(pylint.testutils.Message(msg_id='replace-builtin-open', node=node)):
+                self.checker.visit_call(node)
+        else:
+            with self.assertNoMessages():
+                self.checker.visit_call(node)
 
     @pytest.mark.parametrize('s', ('with ' + s + ' as fd: fd.read()' for s in _calls_not_ok))
-    def test_builtin_open_with(self, s):
+    @pytest.mark.parametrize('os_open_mode', (False, True))
+    def test_builtin_open_with(self, s, os_open_mode):
         node = astroid.extract_node(s + ' #@')
-        with self.assertAddsMessages(pylint.testutils.Message(msg_id='replace-builtin-open', node=node)):
-            self.checker.visit_with(node)
+        self.checker.set_os_open_mode(str(os_open_mode))
+        assert self.checker._prefer_os_open == os_open_mode
+        if os_open_mode:
+            with self.assertAddsMessages(pylint.testutils.Message(msg_id='replace-builtin-open', node=node)):
+                self.checker.visit_with(node)
+        else:
+            with self.assertNoMessages():
+                self.checker.visit_with(node)
diff --git a/tests/plugin_options_test.py b/tests/plugin_options_test.py
@@ -0,0 +1,75 @@
+# -*- coding: utf-8 -*-
+# Copyright 2021 Damien Nguyen
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import pylint.testutils
+import pytest
+
+import pylint_secure_coding_standard as pylint_scs
+
+_default_modes = list(range(0, pylint_scs.SecureCodingStandardChecker.DEFAULT_MAX_MODE + 1))
+
+
+def _id_func(arg):
+    _max_len = 4
+    if arg == _default_modes:
+        return 'default_modes'
+    if isinstance(arg, list) and len(arg) > _max_len:
+        return '[{}...{}]'.format(
+            ','.join(str(val) for val in arg[:_max_len]),
+            ','.join(str(val) for val in arg[-_max_len:]),
+        )
+    return str(arg)
+
+
+class TestSecureCodingStandardChecker(pylint.testutils.CheckerTestCase):
+    CHECKER_CLASS = pylint_scs.SecureCodingStandardChecker
+
+    @pytest.mark.parametrize(
+        'arg, prefer_os_open, allowed_modes',
+        (
+            ('0', False, []),
+            ('false', False, []),
+            ('False', False, []),
+            ('n', False, []),
+            ('no', False, []),
+            ('No', False, []),
+            ('NO', False, []),
+            ('y', True, _default_modes),
+            ('yes', True, _default_modes),
+            ('Yes', True, _default_modes),
+            ('YES', True, _default_modes),
+            ('true', True, _default_modes),
+            ('True', True, _default_modes),
+            ('1', True, [0, 1]),
+            ('2', True, [0, 1, 2]),
+            ('0o755', True, _default_modes),
+            ('0o755,', True, [0o755]),
+            ('0o644, 0o755,', True, [0o644, 0o755]),
+            ('493', True, _default_modes),
+            ('0o644', True, list(range(0, 0o644 + 1))),
+        ),
+        ids=_id_func,
+    )
+    def test_os_open_mode_option(self, arg, prefer_os_open, allowed_modes):
+        print(f'INFO: allowed_modes: {allowed_modes}')
+        print(self.checker._os_open_mode_allowed)
+        self.checker.set_os_open_mode(arg)
+        assert self.checker._prefer_os_open == prefer_os_open
+        assert self.checker._os_open_mode_allowed == allowed_modes
+
+    @pytest.mark.parametrize('arg', ('', ',', ',,', 'asd', 'a,', '493, a'))
+    def test_os_open_mode_option_invalid(self, arg):
+        with pytest.raises(ValueError):
+            self.checker.set_os_open_mode(arg)
