diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 7cdb6113675..917b1389b7d 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -26,6 +26,17 @@ jobs: - uses: psf/black@stable - uses: pre-commit/action@v3.0.0 + bandit: + name: Bandit + needs: style + runs-on: ubuntu-latest + steps: + - uses: davidslusser/actions_python_bandit@v1.0.0 + with: + src: "mne" + options: "-c pyproject.toml -ll -r" + pip_install_command: "pip install bandit[toml]" + pytest: name: '${{ matrix.os }} / ${{ matrix.kind }} / ${{ matrix.python }}' needs: style diff --git a/README.rst b/README.rst index bb34d9da365..c601e318b51 100644 --- a/README.rst +++ b/README.rst @@ -1,6 +1,6 @@ .. -*- mode: rst -*- -|PyPI|_ |conda-forge|_ |Zenodo|_ |Discourse|_ |Codecov|_ +|PyPI|_ |conda-forge|_ |Zenodo|_ |Discourse|_ |Codecov|_ |Bandit|_ |OpenSSF|_ |MNE|_ @@ -19,6 +19,12 @@ .. |Codecov| image:: https://img.shields.io/codecov/c/github/mne-tools/mne-python?label=Coverage .. _Codecov: https://codecov.io/gh/mne-tools/mne-python +.. |Bandit| image:: https://img.shields.io/badge/security-bandit-yellow.svg +.. _Bandit: https://github.com/PyCQA/bandit + +.. |OpenSSF| image:: https://www.bestpractices.dev/projects/7783/badge +.. _OpenSSF: https://www.bestpractices.dev/projects/7783 + .. |MNE| image:: https://mne.tools/stable/_static/mne_logo.svg .. _MNE: https://mne.tools/dev/ diff --git a/doc/changes/devel.rst b/doc/changes/devel.rst index 1556884a518..514b243c6b9 100644 --- a/doc/changes/devel.rst +++ b/doc/changes/devel.rst @@ -46,3 +46,4 @@ Bugs API changes ~~~~~~~~~~~ - ``mne.preprocessing.apply_maxfilter`` and ``mne maxfilter`` have been deprecated and will be removed in 1.7. Use :func:`mne.preprocessing.maxwell_filter` (see :ref:`this tutorial `) in Python or the command-line utility from MEGIN ``maxfilter`` and :func:`mne.bem.fit_sphere_to_headshape` instead (:gh:`11938` by `Eric Larson`_) +- :func:`mne.io.kit.read_mrk` reading pickled files is deprecated using something like ``np.savetxt(fid, pts, delimiter="\t", newline="\n")`` to save your points instead (:gh:`11937` by `Eric Larson`_) diff --git a/environment.yml b/environment.yml index a41433d53e6..ecabd80c37a 100644 --- a/environment.yml +++ b/environment.yml @@ -31,7 +31,7 @@ dependencies: - imageio-ffmpeg>=0.4.1 - vtk>=9.2 - traitlets - - pyvista>=0.32,!=0.35.2,!=0.38.0,!=0.38.1,!=0.38.2,!=0.38.3,!=0.38.4,!=0.38.5,!=0.38.6 + - pyvista>=0.32,!=0.35.2,!=0.38.0,!=0.38.1,!=0.38.2,!=0.38.3,!=0.38.4,!=0.38.5,!=0.38.6,!=0.42.0 - pyvistaqt>=0.4 - qdarkstyle - darkdetect @@ -58,3 +58,4 @@ dependencies: - pybv - mamba - lazy_loader + - defusedxml diff --git a/mne/_fiff/tests/test_meas_info.py b/mne/_fiff/tests/test_meas_info.py index 0e15572d2d8..58f8659860c 100644 --- a/mne/_fiff/tests/test_meas_info.py +++ b/mne/_fiff/tests/test_meas_info.py @@ -3,7 +3,6 @@ # # License: BSD-3-Clause -import hashlib import pickle from datetime import datetime, timedelta, timezone, date from pathlib import Path @@ -70,7 +69,7 @@ from mne._fiff import meas_info from mne._fiff._digitization import _make_dig_points, DigPoint from mne.transforms import Transform -from mne.utils import catch_logging, assert_object_equal, _record_warnings +from mne.utils import catch_logging, assert_object_equal, _empty_hash, _record_warnings root_dir = Path(__file__).parent.parent.parent fiducials_fname = root_dir / "data" / "fsaverage" / "fsaverage-fiducials.fif" @@ -314,14 +313,14 @@ def test_read_write_info(tmp_path): assert_array_equal(info["meas_id"]["machid"], meas_id["machid"]) # Test that writing twice produces the same file - m1 = hashlib.md5() + m1 = _empty_hash() with open(temp_file, "rb") as fid: m1.update(fid.read()) m1 = m1.hexdigest() temp_file_2 = tmp_path / "info2.fif" assert temp_file_2 != temp_file write_info(temp_file_2, info) - m2 = hashlib.md5() + m2 = _empty_hash() with open(str(temp_file_2), "rb") as fid: m2.update(fid.read()) m2 = m2.hexdigest() @@ -1073,7 +1072,7 @@ def test_pickle(fname_info, unlocked): assert not info._unlocked info._unlocked = unlocked data = pickle.dumps(info) - info_un = pickle.loads(data) + info_un = pickle.loads(data) # nosec B301 assert isinstance(info_un, Info) assert_object_equal(info, info_un) assert info_un._unlocked == unlocked diff --git a/mne/channels/_dig_montage_utils.py b/mne/channels/_dig_montage_utils.py index d03cbc1fcbe..c9fae5a7f9c 100644 --- a/mne/channels/_dig_montage_utils.py +++ b/mne/channels/_dig_montage_utils.py @@ -11,7 +11,7 @@ # # License: Simplified BSD -import xml.etree.ElementTree as ElementTree +from defusedxml import ElementTree import numpy as np diff --git a/mne/channels/_standard_montage_utils.py b/mne/channels/_standard_montage_utils.py index fe3c0e0e975..726d1a810a9 100644 --- a/mne/channels/_standard_montage_utils.py +++ b/mne/channels/_standard_montage_utils.py @@ -9,7 +9,7 @@ import numpy as np from functools import partial -import xml.etree.ElementTree as ElementTree +from defusedxml import ElementTree from .montage import make_dig_montage from .._freesurfer import get_mni_fiducials diff --git a/mne/datasets/_fetch.py b/mne/datasets/_fetch.py index 37802f817b8..6dbfe41c2bf 100644 --- a/mne/datasets/_fetch.py +++ b/mne/datasets/_fetch.py @@ -297,13 +297,12 @@ def fetch_dataset( if check_version and ( _compare_version(data_version, "<", mne_version.strip(".git")) ): + # OK to `nosec` because it's false positive (misidentified as SQL) warn( - "The {name} dataset (version {current}) is older than " - "mne-python (version {newest}). If the examples fail, " - "you may need to update the {name} dataset by using " - "mne.datasets.{name}.data_path(force_update=True)".format( - name=name, current=data_version, newest=mne_version - ) + f"The {name} dataset (version {data_version}) is older than " + f"mne-python (version {mne_version}). If the examples fail, " + f"you may need to update the {name} dataset by using " + f"mne.datasets.{name}.data_path(force_update=True)" # nosec B608 ) _log_time_size(t0, sz) return (final_path, data_version) if return_version else final_path diff --git a/mne/html_templates/_templates.py b/mne/html_templates/_templates.py index f01eb7400eb..dff9c6b6c18 100644 --- a/mne/html_templates/_templates.py +++ b/mne/html_templates/_templates.py @@ -7,12 +7,11 @@ def _get_html_templates_env(kind): assert kind in ("repr", "report"), kind import jinja2 - autoescape = jinja2.select_autoescape(default=True, default_for_string=True) templates_env = jinja2.Environment( loader=jinja2.PackageLoader( package_name="mne.html_templates", package_path=kind ), - autoescape=autoescape, + autoescape=jinja2.select_autoescape(default=True, default_for_string=True), ) if kind == "report": templates_env.filters["zip"] = zip diff --git a/mne/io/egi/egimff.py b/mne/io/egi/egimff.py index 41e53e7eb3e..5e8baa5d89b 100644 --- a/mne/io/egi/egimff.py +++ b/mne/io/egi/egimff.py @@ -5,7 +5,7 @@ import math import os.path as op import re -from xml.dom.minidom import parse +from defusedxml.minidom import parse from pathlib import Path import numpy as np diff --git a/mne/io/egi/events.py b/mne/io/egi/events.py index d9450913aa6..9e5088d7617 100644 --- a/mne/io/egi/events.py +++ b/mne/io/egi/events.py @@ -4,7 +4,7 @@ from datetime import datetime from glob import glob from os.path import basename, join, splitext -from xml.etree.ElementTree import parse +from defusedxml.ElementTree import parse import numpy as np diff --git a/mne/io/egi/general.py b/mne/io/egi/general.py index 6b8829ca4e6..a82f087b59a 100644 --- a/mne/io/egi/general.py +++ b/mne/io/egi/general.py @@ -2,7 +2,7 @@ # License: BSD-3-Clause import os -from xml.dom.minidom import parse +from defusedxml.minidom import parse import re import numpy as np diff --git a/mne/io/kit/coreg.py b/mne/io/kit/coreg.py index 470a0130d4f..881e63fb87a 100644 --- a/mne/io/kit/coreg.py +++ b/mne/io/kit/coreg.py @@ -40,7 +40,7 @@ def read_mrk(fname): ---------- fname : path-like Absolute path to Marker file. - File formats allowed: \*.sqd, \*.mrk, \*.txt, \*.pickled. + File formats allowed: \*.sqd, \*.mrk, \*.txt. Returns ------- @@ -49,9 +49,9 @@ def read_mrk(fname): """ from .kit import _read_dirs - fname = Path(fname) - _check_option("file extension", fname.suffix, (".sqd", ".mrk", ".txt", ".pickled")) - _check_fname(fname, "read", must_exist=True, name="mrk file") + fname = Path(_check_fname(fname, "read", must_exist=True, name="mrk file")) + if fname.suffix != ".pickled": + _check_option("file extension", fname.suffix, (".sqd", ".mrk", ".txt")) if fname.suffix in (".sqd", ".mrk"): with open(fname, "rb", buffering=0) as fid: dirs = _read_dirs(fid) @@ -70,13 +70,18 @@ def read_mrk(fname): elif fname.suffix == ".txt": mrk_points = _read_dig_kit(fname, unit="m") elif fname.suffix == ".pickled": + warn( + "Reading pickled files is unsafe and not future compatible, save " + "to a standard format (text or FIF) instea, e.g. with:\n" + r"np.savetxt(fid, pts, delimiter=\"\\t\", newline=\"\\n\")", + FutureWarning, + ) with open(fname, "rb") as fid: - food = pickle.load(fid) + food = pickle.load(fid) # nosec B301 try: mrk_points = food["mrk"] except Exception: - err = "%r does not contain marker points." % fname - raise ValueError(err) + raise ValueError(f"{fname} does not contain marker points.") from None # check output mrk_points = np.asarray(mrk_points) diff --git a/mne/io/kit/tests/test_coreg.py b/mne/io/kit/tests/test_coreg.py index 32d82f1d57d..330e51471bb 100644 --- a/mne/io/kit/tests/test_coreg.py +++ b/mne/io/kit/tests/test_coreg.py @@ -27,15 +27,23 @@ def test_io_mrk(tmp_path): pts_2 = read_mrk(path) assert_array_equal(pts, pts_2, "read/write mrk to text") - # pickle + # pickle (deprecated) fname = tmp_path / "mrk.pickled" with open(fname, "wb") as fid: pickle.dump(dict(mrk=pts), fid) - pts_2 = read_mrk(fname) + with pytest.warns(FutureWarning, match="unsafe"): + pts_2 = read_mrk(fname) assert_array_equal(pts_2, pts, "pickle mrk") with open(fname, "wb") as fid: pickle.dump(dict(), fid) - pytest.raises(ValueError, read_mrk, fname) + with pytest.warns(FutureWarning, match="unsafe"): + with pytest.raises(ValueError, match="does not contain"): + read_mrk(fname) # unsupported extension - pytest.raises(ValueError, read_mrk, "file.ext") + fname = tmp_path / "file.ext" + with pytest.raises(FileNotFoundError, match="does not exist"): + read_mrk(fname) + fname.write_text("") + with pytest.raises(ValueError, match="file extension"): + read_mrk(fname) diff --git a/mne/io/nedf/nedf.py b/mne/io/nedf/nedf.py index a21e2edffde..3b87986cf51 100644 --- a/mne/io/nedf/nedf.py +++ b/mne/io/nedf/nedf.py @@ -2,7 +2,7 @@ from copy import deepcopy from datetime import datetime, timezone -from xml.etree import ElementTree +from defusedxml import ElementTree import numpy as np diff --git a/mne/preprocessing/maxfilter.py b/mne/preprocessing/maxfilter.py index a1de2092058..886e3f13637 100644 --- a/mne/preprocessing/maxfilter.py +++ b/mne/preprocessing/maxfilter.py @@ -217,7 +217,8 @@ def apply_maxfilter( logger.info("Running MaxFilter: %s " % cmd) if os.getenv("_MNE_MAXFILTER_TEST", "") != "true": # fake maxfilter - st = os.system(cmd) + # OK to `nosec` because it's deprecated / will be removed + st = os.system(cmd) # nosec B605 else: print(cmd) # we can check the output st = 0 diff --git a/mne/report/tests/test_report.py b/mne/report/tests/test_report.py index e269bcc7c9f..1e79b097315 100644 --- a/mne/report/tests/test_report.py +++ b/mne/report/tests/test_report.py @@ -865,7 +865,7 @@ def test_survive_pickle(tmp_path): # Pickle report object to simulate multiprocessing with joblib report = Report(info_fname=raw_fname_new) pickled_report = pickle.dumps(report) - report = pickle.loads(pickled_report) + report = pickle.loads(pickled_report) # nosec B301 # Just test if no errors occur report.parse_folder(tmp_path, render_bem=False) diff --git a/mne/tests/test_label.py b/mne/tests/test_label.py index 360db08b3af..2e2abfa4295 100644 --- a/mne/tests/test_label.py +++ b/mne/tests/test_label.py @@ -352,7 +352,7 @@ def test_label_io(tmp_path): with open(dest, "wb") as fid: pickle.dump(label, fid, pickle.HIGHEST_PROTOCOL) with open(dest, "rb") as fid: - label2 = pickle.load(fid) + label2 = pickle.load(fid) # nosec B301 assert_labels_equal(label, label2) diff --git a/mne/time_frequency/tests/test_csd.py b/mne/time_frequency/tests/test_csd.py index ecdece4469c..8fc3d0008dc 100644 --- a/mne/time_frequency/tests/test_csd.py +++ b/mne/time_frequency/tests/test_csd.py @@ -264,7 +264,7 @@ def test_csd_pickle(tmp_path): with open(fname, "wb") as f: pickle.dump(csd, f) with open(fname, "rb") as f: - csd2 = pickle.load(f) + csd2 = pickle.load(f) # nosec B301 assert_array_equal(csd._data, csd2._data) assert csd.tmin == csd2.tmin assert csd.tmax == csd2.tmax diff --git a/mne/utils/__init__.py b/mne/utils/__init__.py index ea8d01a9563..76e52f378fe 100644 --- a/mne/utils/__init__.py +++ b/mne/utils/__init__.py @@ -107,6 +107,7 @@ "_clean_names", "pformat", "_file_like", + "_empty_hash", "_explain_exception", "_get_argvalues", "sizeof_fmt", diff --git a/mne/utils/_testing.py b/mne/utils/_testing.py index 02f6a0e8a7f..da907bb52c9 100644 --- a/mne/utils/_testing.py +++ b/mne/utils/_testing.py @@ -3,7 +3,7 @@ # # License: BSD-3-Clause -from functools import partial, wraps +from functools import wraps import os import inspect from io import StringIO @@ -19,6 +19,7 @@ from ._logging import warn, ClosingStringIO from .check import check_version +from .misc import run_subprocess from .numerics import object_diff @@ -55,33 +56,9 @@ def __del__(self): # noqa: D105 rmtree(self._path, ignore_errors=True) -def _requires_module(function, name, *, call): - import pytest - - call = ("import %s" % name) if call is None else call - reason = "Test %s skipped, requires %s." % (function.__name__, name) - try: - exec(call) in globals(), locals() - except Exception as exc: - if len(str(exc)) > 0 and str(exc) != "No module named %s" % name: - reason += " Got exception (%s)" % (exc,) - skip = True - else: - skip = False - return pytest.mark.skipif(skip, reason=reason)(function) - - -_mne_call = """ -if not has_mne_c(): - raise ImportError -""" - -_fs_call = """ -if not has_freesurfer(): - raise ImportError -""" - -requires_mne = partial(_requires_module, name="MNE-C", call=_mne_call) +def requires_mne(func): + """Decorate a function as requiring MNE.""" + return requires_mne_mark()(func) def requires_mne_mark(): @@ -102,28 +79,35 @@ def requires_openmeeg_mark(): def requires_freesurfer(arg): """Require Freesurfer.""" + import pytest + + reason = "Requires Freesurfer" if isinstance(arg, str): # Calling as @requires_freesurfer('progname'): return decorator # after checking for progname existence - call = """ -from . import run_subprocess -run_subprocess([%r, '--version']) -""" % ( - arg, - ) - return partial(_requires_module, name="Freesurfer (%s)" % (arg,), call=call) + reason += f" command: {arg}" + try: + run_subprocess([arg, "--version"]) + except Exception: + skip = True + else: + skip = False + return pytest.mark.skipif(skip, reason=reason) else: # Calling directly as @requires_freesurfer: return decorated function # and just check env var existence - return _requires_module(arg, name="Freesurfer", call=_fs_call) + return pytest.mark.skipif(not has_freesurfer(), reason="Requires Freesurfer")( + arg + ) -requires_good_network = partial( - _requires_module, - name="good network connection", - call='if int(os.environ.get("MNE_SKIP_NETWORK_TESTS", 0)):\n' - " raise ImportError", -) +def requires_good_network(func): + import pytest + + return pytest.mark.skipif( + int(os.environ.get("MNE_SKIP_NETWORK_TESTS", 0)), + reason="MNE_SKIP_NETWORK_TESTS is set", + )(func) def run_command_if_main(): diff --git a/mne/utils/misc.py b/mne/utils/misc.py index 73988840c2b..79a23e9a0e6 100644 --- a/mne/utils/misc.py +++ b/mne/utils/misc.py @@ -6,6 +6,7 @@ from contextlib import contextmanager, ExitStack import fnmatch import gc +import hashlib import inspect from math import log import os @@ -31,6 +32,15 @@ from importlib_resources import files +# TODO: no longer needed when py3.9 is minimum supported version +def _empty_hash(kind="md5"): + func = getattr(hashlib, kind) + if "usedforsecurity" in inspect.signature(func).parameters: + return func(usedforsecurity=False) + else: + return func() + + def _pl(x, non_pl="", pl="s"): """Determine if plural should be used.""" len_x = x if isinstance(x, (int, np.generic)) else len(x) diff --git a/mne/utils/numerics.py b/mne/utils/numerics.py index ddabefeeb73..67a15d789e4 100644 --- a/mne/utils/numerics.py +++ b/mne/utils/numerics.py @@ -4,7 +4,6 @@ # # License: BSD-3-Clause -import hashlib import inspect import numbers import operator @@ -22,6 +21,7 @@ from ._logging import logger, warn, verbose from .check import check_random_state, _ensure_int, _validate_type +from .misc import _empty_hash from ..fixes import ( _infer_dimension_, svd_flip, @@ -419,10 +419,7 @@ def hashfunc(fname, block_size=1048576, hash_type="md5"): # 2 ** 20 hash_ : str The hexadecimal digest of the hash. """ - if hash_type == "md5": - hasher = hashlib.md5() - elif hash_type == "sha1": - hasher = hashlib.sha1() + hasher = _empty_hash(kind=hash_type) with open(fname, "rb") as fid: while True: data = fid.read(block_size) @@ -652,7 +649,7 @@ def object_hash(x, h=None): The digest resulting from the hash. """ if h is None: - h = hashlib.md5() + h = _empty_hash() if hasattr(x, "keys"): # dict-like types keys = _sort_keys(x) diff --git a/mne/utils/tests/test_bunch.py b/mne/utils/tests/test_bunch.py index 7819b641fb4..757fa8e3072 100644 --- a/mne/utils/tests/test_bunch.py +++ b/mne/utils/tests/test_bunch.py @@ -20,5 +20,5 @@ def test_pickle(): assert isinstance(b1.y, NamedFloat) assert repr(b1.y) == "2.12 (y)" - b2 = pickle.loads(pickle.dumps(b1)) + b2 = pickle.loads(pickle.dumps(b1)) # nosec B301 assert b1 == b2 diff --git a/mne/viz/_brain/_scraper.py b/mne/viz/_brain/_scraper.py index 173f97b89fd..8c2aa7ed96f 100644 --- a/mne/viz/_brain/_scraper.py +++ b/mne/viz/_brain/_scraper.py @@ -44,7 +44,7 @@ def __call__(self, block, block_vars, gallery_conf): else: break assert line.startswith("(") and line.endswith(")") - kwargs.update(eval(f"dict{line}")) + kwargs.update(eval(f"dict{line}")) # nosec B307 for key, default in [ ("time_dilation", 4), ("framerate", 24), diff --git a/pyproject.toml b/pyproject.toml index f87b8aba971..ac247de714a 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -50,3 +50,6 @@ junit_family = "xunit2" [tool.black] exclude = "(dist/)|(build/)|(.*\\.ipynb)" + +[tool.bandit.assert_used] +skips = ["*/test_*.py"] # assert statements are good practice with pytest diff --git a/requirements.txt b/requirements.txt index 37d7f9c8c9b..abaf4ebfb0e 100644 --- a/requirements.txt +++ b/requirements.txt @@ -32,7 +32,7 @@ xlrd imageio>=2.6.1 imageio-ffmpeg>=0.4.1 traitlets -pyvista>=0.32,!=0.35.2,!=0.38.0,!=0.38.1,!=0.38.2,!=0.38.3,!=0.38.4,!=0.38.5,!=0.38.6 +pyvista>=0.32,!=0.35.2,!=0.38.0,!=0.38.1,!=0.38.2,!=0.38.3,!=0.38.4,!=0.38.5,!=0.38.6,!=0.42.0 pyvistaqt>=0.4 mffpy>=0.5.7 ipywidgets diff --git a/requirements_base.txt b/requirements_base.txt index c97bf844a1e..551156522c3 100644 --- a/requirements_base.txt +++ b/requirements_base.txt @@ -9,3 +9,4 @@ packaging jinja2 importlib_resources>=5.10.2; python_version<'3.9' lazy_loader>=0.3 +defusedxml