Bug 1651731: [lint] Only allow files that are typically executable to…

… have shebang lines override permission check; r=linter-reviewers,sylvestre

Differential Revision: https://phabricator.services.mozilla.com/D82949

.clang-format-ignore

@@ -52,10 +52,7 @@ config/windows-h-.*.h
 tools/clang-tidy/test/.*
 
 # We are testing the incorrect formatting.
-tools/lint/test/files/file-whitespace/
-
-# Test reformatting
-tools/lint/test/files/clang-format/
+tools/lint/test/files/
 
 # Contains an XML definition and formatting would break the layout
 widget/gtk/MPRISInterfaceDescription.h

docs/code-quality/lint/linters/file-perm.rst

@@ -5,7 +5,9 @@ This linter verifies if a file has unnecessary permissions.
 If a file has execution permissions (+x), file-perm will
 generate a warning.
 
-It will ignore files starting with ``#!`` (Python or node scripts).
+It will ignore files starting with ``#!`` for types of files
+that typically have shebang lines (such as python, node or
+shell scripts).
 
 This linter does not have any affect on Windows.
 

tools/lint/file-perm.yml

@@ -9,7 +9,6 @@ file-perm:
         - .cpp
         - .h
         - .html
-        - .js
         - .jsm
         - .jsx
         - .m
@@ -22,3 +21,15 @@ file-perm:
         - 'tools/lint/file-perm/**'
     type: external
     payload: file-perm:lint
+
+maybe-shebang-file-perm:
+    description: "File permission check for files that might have `#!` header."
+    include:
+        - .
+    allow-shebang: true
+    extensions:
+        - .js
+    support-files:
+        - 'tools/lint/file-perm/**'
+    type: external
+    payload: file-perm:lint

tools/lint/file-perm/__init__.py

@@ -8,10 +8,9 @@
 from mozlint import result
 from mozlint.pathutils import expand_exclusions
 
-results = []
-
 
 def lint(paths, config, fix=None, **lintargs):
+    results = []
 
     if platform.system() == "Windows":
         # Windows doesn't have permissions in files
@@ -21,13 +20,14 @@ def lint(paths, config, fix=None, **lintargs):
     files = list(expand_exclusions(paths, config, lintargs["root"]))
     for f in files:
         if os.access(f, os.X_OK):
-            with open(f, "r+") as content:
-                # Some source files have +x permissions
-                line = content.readline()
-                if line.startswith("#!"):
-                    # Check if the file doesn't start with a shebang
-                    # if it does, not a warning
-                    continue
+            if config.get("allow-shebang"):
+                with open(f, "r+") as content:
+                    # Some source files have +x permissions
+                    line = content.readline()
+                    if line.startswith("#!"):
+                        # Check if the file doesn't start with a shebang
+                        # if it does, not a warning
+                        continue
 
             if fix:
                 # We want to fix it, do it and leave

tools/lint/test/conftest.py

@@ -20,7 +20,39 @@
 logger = logging.getLogger("mozlint")
 
 
-@pytest.fixture(scope="module")
+def pytest_generate_tests(metafunc):
+    """Finds, loads and returns the config for the linter name specified by the
+    LINTER global variable in the calling module.
+
+    This implies that each test file (that uses this fixture) should only be
+    used to test a single linter. If no LINTER variable is defined, the test
+    will fail.
+    """
+    if "config" in metafunc.fixturenames:
+        if not hasattr(metafunc.module, "LINTER"):
+            pytest.fail(
+                "'config' fixture used from a module that didn't set the LINTER variable"
+            )
+
+        name = metafunc.module.LINTER
+        config_path = os.path.join(lintdir, "{}.yml".format(name))
+        parser = Parser(build.topsrcdir)
+        configs = parser.parse(config_path)
+        config_names = {config["name"] for config in configs}
+
+        marker = metafunc.definition.get_closest_marker("lint_config")
+        if marker:
+            config_name = marker.kwargs["name"]
+            if config_name not in config_names:
+                pytest.fail(f"lint config {config_name} not present in {name}.yml")
+            configs = [
+                config for config in configs if config["name"] == marker.kwargs["name"]
+            ]
+
+        ids = [config["name"] for config in configs]
+        metafunc.parametrize("config", configs, ids=ids)
+
+
 def root(request):
     """Return the root directory for the files of the linter under test.
 
@@ -50,27 +82,6 @@ def _inner(*paths):
     return _inner
 
 
-@pytest.fixture
-def config(request):
-    """Finds, loads and returns the config for the linter name specified by the
-    LINTER global variable in the calling module.
-
-    This implies that each test file (that uses this fixture) should only be
-    used to test a single linter. If no LINTER variable is defined, the test
-    will fail.
-    """
-    if not hasattr(request.module, "LINTER"):
-        pytest.fail(
-            "'config' fixture used from a module that didn't set the LINTER variable"
-        )
-
-    name = request.module.LINTER
-    config_path = os.path.join(lintdir, "{}.yml".format(name))
-    parser = Parser(build.topsrcdir)
-    # TODO Don't assume one linter per yaml file
-    return parser.parse(config_path)[0]
-
-
 @pytest.fixture(autouse=True)
 def run_setup(config):
     """Make sure that if the linter named in the LINTER global variable has a
@@ -111,7 +122,7 @@ def wrapper(paths, config=config, root=root, collapse_results=False, **lintargs)
 
         ret = defaultdict(list)
         for r in results:
-            ret[r.path].append(r)
+            ret[r.relpath].append(r)
         return ret
 
     return wrapper

tools/lint/test/files/file-perm/no-shebang/bad-shebang.c

@@ -0,0 +1,2 @@
+#!/bin/bash
+int main() { return 0; }

tools/lint/test/test_file_perm.py

@@ -1,22 +1,38 @@
 from __future__ import absolute_import, print_function
 
+import pytest
+
 import mozunit
 
 LINTER = "file-perm"
 
 
+@pytest.mark.lint_config(name="file-perm")
 def test_lint_file_perm(lint, paths):
-    results = lint(paths())
+    results = lint(paths("no-shebang"), collapse_results=True)
     print(results)
     assert len(results) == 2
 
+    assert results.keys() == {
+        "no-shebang/bad.c",
+        "no-shebang/bad-shebang.c",
+    }
+
+    for path, issues in results.items():
+        for issue in issues:
+            assert "permissions on a source" in issue.message
+            assert issue.level == "error"
+
+
+@pytest.mark.lint_config(name="maybe-shebang-file-perm")
+def test_lint_shebang_file_perm(config, lint, paths):
+    results = lint(paths("maybe-shebang"))
+    print(results)
+    assert len(results) == 1
+
     assert "permissions on a source" in results[0].message
     assert results[0].level == "error"
-    assert results[0].relpath == "bad.c"
-
-    assert "permissions on a source" in results[1].message
-    assert results[1].level == "error"
-    assert results[1].relpath == "bad.js"
+    assert results[0].relpath == "maybe-shebang/bad.js"
 
 
 if __name__ == "__main__":