Add R8009 for builtin open() vs os.open() for file permissions

Author: Takishima

CHANGELOG.md

@@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Added
+
+-   Added R8009 to prefer `os.open()` to the builtin `open` when in writing mode
+
 ### Repository
 
 - Update pre-commit configuration
@@ -15,6 +19,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 Initial release
 
-[Unreleased]: https://github.com/Takishima/pylint-secure-coding-standard/compare/1.0.0...HEAD
+[Unreleased]: https://github.com/Takishima/pylint-secure-coding-standard/compare/v1.0.0...HEAD
 
-[1.0.0]: https://github.com/Takishima/pylint-secure-coding-standard/compare/375145a3dec096ff4e33901ef749a1a9a6f4edc6...1.0.0
+[1.0.0]: https://github.com/Takishima/pylint-secure-coding-standard/compare/375145a3dec096ff4e33901ef749a1a9a6f4edc6...v1.0.0

README.md

@@ -11,17 +11,18 @@ pylint plugin that enforces some secure coding standards.
 
 ## Pylint codes
 
-| Code  | Description                                                                     |
-|-------|---------------------------------------------------------------------------------|
-| R8000 | Use `os.path.realpath()` instead of `os.path.abspath()` and `os.path.relpath()` |
-| E8001 | Avoid using `exec()` and `eval()`                                               |
-| E8002 | Avoid using `os.sytem()`                                                        |
-| E8003 | Avoid using `shell=True` when calling `subprocess` functions                    |
-| R8004 | Avoid using `tempfile.mktemp()`, prefer `tempfile.mkstemp()` instead            |
-| E8005 | Avoid using unsafe PyYAML loading functions                                     |
-| E8006 | Avoid using `jsonpickle.decode()`                                               |
-| C8007 | Avoid debug statement in production code                                        |
-| C8008 | Avoid `assert` statements in production code                                    |
+| Code  | Description                                                                                                  |
+|-------|--------------------------------------------------------------------------------------------------------------|
+| R8000 | Use `os.path.realpath()` instead of `os.path.abspath()` and `os.path.relpath()`                              |
+| E8001 | Avoid using `exec()` and `eval()`                                                                            |
+| E8002 | Avoid using `os.sytem()`                                                                                     |
+| E8003 | Avoid using `shell=True` when calling `subprocess` functions                                                 |
+| R8004 | Avoid using `tempfile.mktemp()`, prefer `tempfile.mkstemp()` instead                                         |
+| E8005 | Avoid using unsafe PyYAML loading functions                                                                  |
+| E8006 | Avoid using `jsonpickle.decode()`                                                                            |
+| C8007 | Avoid debug statement in production code                                                                     |
+| C8008 | Avoid `assert` statements in production code                                                                 |
+| R8009 | Use of builtin `open` for writing is discouraged in favor of `os.open` to allow for setting file permissions |
 
 
 ## Pre-commit hook
@@ -31,7 +32,7 @@ See [pre-commit](https://github.com/pre-commit/pre-commit) for instructions
 Sample `.pre-commit-config.yaml`:
 
 ```yaml
-  - repo: https://github.com/pycqa/pylint
+  - repo: https://github.com/PyCQA/pylint/
     rev: pylint-2.6.0
     hooks:
     -   id: pylint

pylint_secure_coding_standard.py

@@ -49,6 +49,31 @@ def _is_os_path_call(node):
     )
 
 
+def _is_builtin_open_for_writing(node):
+    if isinstance(node.func, astroid.Name) and node.func.name == 'open':
+        mode = ''
+        if len(node.args) > 1:
+            if isinstance(node.args[1], astroid.Name):
+                return True  # variable -> to be on the safe side, flag as inappropriate
+            if isinstance(node.args[1], astroid.Const):
+                mode = node.args[1].value
+        elif node.keywords:
+            for keyword in node.keywords:
+                if keyword.arg == 'mode':
+                    if not isinstance(keyword.value, astroid.Const):
+                        return True  # variable -> to be on the safe side, flag as inappropriate
+                    mode = keyword.value.value
+                    break
+
+        if any(m in mode for m in 'awx'):
+            # open(..., "w")
+            # open(..., "wb")
+            # open(..., "a")
+            # open(..., "x")
+            return True
+    return False
+
+
 def _is_subprocess_shell_true_call(node):
     if (
         isinstance(node.func, astroid.Attribute)
@@ -190,6 +215,12 @@ class SecureCodingStandardChecker(BaseChecker):
             'avoid-assert',
             '`assert` statements should not be present in production code',
         ),
+        'R8005': (
+            'Avoid builtin open() when writing to files, prefer os.open()',
+            'replace-builtin-open',
+            'Use of builtin `open` for writing is discouraged in favor of `os.open` to allow for setting file '
+            'permissions',
+        ),
     }
 
     options = {}
@@ -212,6 +243,8 @@ def visit_call(self, node):
             self.add_message('replace-os-relpath-abspath', node=node)
         elif _is_subprocess_shell_true_call(node):
             self.add_message('avoid-shell-true', node=node)
+        elif _is_builtin_open_for_writing(node):
+            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)
 
@@ -238,6 +271,14 @@ def visit_importfrom(self, node):
         elif node.modname == 'os' and [name for (name, _) in node.names if name == 'system']:
             self.add_message('avoid-os-system', node=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)
+
     def visit_assert(self, node):
         """
         Visitor method called for astroid.Assert nodes

tests/builtin_open_test.py

@@ -0,0 +1,82 @@
+# -*- 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 astroid
+import pytest
+
+import pylint_secure_coding_standard as pylint_scs
+import pylint.testutils
+
+
+class TestSecureCodingStandardChecker(pylint.testutils.CheckerTestCase):
+    CHECKER_CLASS = pylint_scs.SecureCodingStandardChecker
+
+    def test_builtin_open_ok(self):
+        nodes = astroid.extract_node(
+            """
+            int(0) #@
+            foo() #@
+            open("file.txt") #@
+            bla.open("file.txt") #@
+            bla.open("file.txt", "w") #@
+            with open("file.txt") as fd: fd.read() #@
+            with bla.open("file.txt") as fd: fd.read() #@
+            with bla.open("file.txt", "w") as fd: fd.read() #@
+            """
+        )
+
+        call_nodes = nodes[:5]
+        with_nodes = nodes[5:]
+
+        with self.assertNoMessages():
+            for idx, node in enumerate(call_nodes):
+                self.checker.visit_call(node)
+            for idx, node in enumerate(with_nodes):
+                self.checker.visit_with(node)
+
+    _calls_not_ok = (
+        'open("file.txt", "w")',
+        'open("file.txt", "wb")',
+        'open("file.txt", "bw")',
+        'open("file.txt", "a")',
+        'open("file.txt", "ab")',
+        'open("file.txt", "ba")',
+        'open("file.txt", "x")',
+        'open("file.txt", "xb")',
+        'open("file.txt", "bx")',
+        'open("file.txt", mode)',
+        'open("file.txt", mode="w")',
+        'open("file.txt", mode="wb")',
+        'open("file.txt", mode="bw")',
+        'open("file.txt", mode="a")',
+        'open("file.txt", mode="ab")',
+        'open("file.txt", mode="ba")',
+        'open("file.txt", mode="x")',
+        'open("file.txt", mode="xb")',
+        'open("file.txt", mode="bx")',
+        'open("file.txt", mode=mode)',
+    )
+
+    @pytest.mark.parametrize('s', _calls_not_ok)
+    def test_builtin_open_call(self, s):
+        node = astroid.extract_node(s + ' #@')
+        with self.assertAddsMessages(pylint.testutils.Message(msg_id='replace-builtin-open', node=node)):
+            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):
+        node = astroid.extract_node(s + ' #@')
+        with self.assertAddsMessages(pylint.testutils.Message(msg_id='replace-builtin-open', node=node)):
+            self.checker.visit_with(node)