diff --git a/.flake8 b/.flake8 deleted file mode 100644 index 9314234..0000000 --- a/.flake8 +++ /dev/null @@ -1,4 +0,0 @@ -[flake8] -extend-ignore = E203,E701 -max-line-length = 88 -exclude = [".eggs"] diff --git a/.gitignore b/.gitignore index 81f0228..0ef8fbb 100644 --- a/.gitignore +++ b/.gitignore @@ -2,7 +2,6 @@ .* !.gitignore !.readthedocs.yaml -!.flake8 !.github # Byte / compiled / optimized diff --git a/pyproject.toml b/pyproject.toml index 6ffac0c..7a9fd41 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -33,7 +33,7 @@ test = [ ] dev = [ "ewoksutils[test]", - "ruff", + "ruff>=0.16.0", ] doc = [ "sphinx >=4.5", @@ -56,12 +56,21 @@ module = "setuptools.*" ignore_missing_imports = true [tool.ruff.lint] -select = ["E", "F", "I"] -ignore = ["E501"] +select = [ + "E", # pycodestyle errors + "F", # pyflakes + "I", # isort + "S", # flake8-bandit +] +ignore = [ + "E501", # line too long +] [tool.ruff.lint.per-file-ignores] -# Ignore `S101` (assert used violations) in all test files -"src/ewoks/tests/*.py" = ["S101"] +"src/ewoksutils/tests/*.py" = [ + "S101" # allow asserts +] [tool.ruff.lint.isort] force-single-line = true +known-first-party = ["ewoksutils"] diff --git a/src/ewoksutils/sqlite3_utils.py b/src/ewoksutils/sqlite3_utils.py index 1d44c20..ecf0d5d 100644 --- a/src/ewoksutils/sqlite3_utils.py +++ b/src/ewoksutils/sqlite3_utils.py @@ -1,4 +1,5 @@ import json +import re import sqlite3 from contextlib import closing from contextlib import contextmanager @@ -60,13 +61,6 @@ def serialize(value: Any, sql_type: Optional[str] = None): return json.dumps(value).encode() -def _select_serialize(value: Any, sql_type: Optional[str] = None): - sql_value = serialize(value, sql_type) - if isinstance(sql_value, str): - return f"'{sql_value}'" - return sql_value - - def deserialize(sql_value, field_type: Optional[str] = None): if isinstance(sql_value, bytes): sql_value = sql_value.decode() @@ -91,35 +85,39 @@ def select( endtime: Optional[Union[str, datetime]] = None, **is_equal_filter, ) -> Iterator[dict]: + table = _validate_identifier(table) + conditions = [] + params = [] + if is_equal_filter: if sql_types is None: sql_types = python_to_sql_types(field_types) - conditions = [ - f"{k} = {_select_serialize(v, sql_types.get(k))}" - for k, v in is_equal_filter.items() - ] - else: - conditions = list() + for k, v in is_equal_filter.items(): + conditions.append(f"{_validate_identifier(k)} = ?") + params.append(serialize(v, sql_types.get(k))) if starttime: if isinstance(starttime, str): starttime = fromisoformat(starttime) - conditions.append(f"time >= '{starttime.isoformat()}'") + conditions.append("time >= ?") + params.append(starttime.isoformat()) if endtime: if isinstance(endtime, str): endtime = fromisoformat(endtime) - conditions.append(f"time <= '{endtime.isoformat()}'") + conditions.append("time <= ?") + params.append(endtime.isoformat()) if conditions: search_condition = " AND ".join(conditions) - query = f"SELECT * FROM {table} WHERE {search_condition}" + # table/columns are validated by _validate_identifier(); values are parameterized + query = f"SELECT * FROM {table} WHERE {search_condition}" # noqa: S608 else: - query = f"SELECT * FROM {table}" + query = f"SELECT * FROM {table}" # noqa: S608 with closing(conn.cursor()) as cursor: try: - cursor.execute(query) + cursor.execute(query, params) except sqlite3.OperationalError as e: if "no such table" in str(e): return @@ -154,3 +152,14 @@ def _ensure_directory_exists(uri: str) -> None: if parsed.scheme == "file": path = uri_utils.path_from_uri(uri) path.parent.mkdir(parents=True, exist_ok=True) + + +# Table/column names cannot be parameterized in DBAPI queries, so restrict +# them to simple identifiers before interpolating them into SQL strings. +_IDENTIFIER_RE = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$") + + +def _validate_identifier(name: str) -> str: + if not _IDENTIFIER_RE.match(name): + raise ValueError(f"{name!r} is not a valid SQL identifier") + return name diff --git a/src/ewoksutils/tests/test_cli.py b/src/ewoksutils/tests/test_cli.py index bcfb26a..926be11 100644 --- a/src/ewoksutils/tests/test_cli.py +++ b/src/ewoksutils/tests/test_cli.py @@ -5,13 +5,14 @@ from ..cli_utils import cli_submit_utils -def test_cli_execute_no_parameters(cli_interface): +def test_cli_execute_no_parameters(cli_interface, tmp_path): + workflow_dir = str(tmp_path) argv = [ "acyclic1", "acyclic2", "--test", "--workflow-dir", - "/tmp", + workflow_dir, ] cli_args = cli_interface( argv, @@ -27,7 +28,7 @@ def test_cli_execute_no_parameters(cli_interface): "outputs": [], "task_options": {}, "varinfo": {"root_uri": "", "scheme": "nexus"}, - "load_options": {"representation": "test_core", "root_dir": "/tmp"}, + "load_options": {"representation": "test_core", "root_dir": workflow_dir}, "execinfo": {}, } assert cli_args.execute_options == execute_options @@ -201,7 +202,8 @@ def test_cli_execute_deprecated_inputs_all(cli_interface): assert cli_args.execute_options == execute_options -def test_cli_submit(cli_interface): +def test_cli_submit(cli_interface, tmp_path): + workflow_dir = str(tmp_path) argv = [ "acyclic1", "acyclic2", @@ -211,7 +213,7 @@ def test_cli_submit(cli_interface): "-pn", "node1:b=test", "--workflow-dir", - "/tmp", + workflow_dir, "--wait=inf", ] cli_args = cli_interface( @@ -231,7 +233,7 @@ def test_cli_submit(cli_interface): "outputs": [], "task_options": {}, "varinfo": {"root_uri": "", "scheme": "nexus"}, - "load_options": {"representation": "test_core", "root_dir": "/tmp"}, + "load_options": {"representation": "test_core", "root_dir": workflow_dir}, "execinfo": {}, } assert cli_args.execute_options == execute_options diff --git a/src/ewoksutils/tests/test_sqlite3_utils.py b/src/ewoksutils/tests/test_sqlite3_utils.py index ececc41..5d748a2 100644 --- a/src/ewoksutils/tests/test_sqlite3_utils.py +++ b/src/ewoksutils/tests/test_sqlite3_utils.py @@ -1,5 +1,7 @@ import datetime +import pytest + from .. import sqlite3_utils @@ -134,3 +136,52 @@ def test_sqlite3_types(): ) ) assert len(rows) == 0 + + +def test_sqlite3_select_value_is_not_sql_injectable(): + """A filter value that looks like SQL must be treated as literal data, + not executable SQL (values are passed as parameters, not interpolated).""" + field_types = {"name": ""} + sql_types = sqlite3_utils.python_to_sql_types(field_types) + + with sqlite3_utils.connect(":memory:") as conn: + conn.execute(sqlite3_utils.ensure_table_query("test", sql_types)) + conn.commit() + + insert_query = sqlite3_utils.insert_query("test", len(field_types)) + conn.execute( + insert_query, [sqlite3_utils.serialize("alice", sql_types["name"])] + ) + conn.commit() + + malicious = "x' OR '1'='1" + rows = list( + sqlite3_utils.select( + conn, + "test", + field_types=field_types, + sql_types=sql_types, + name=malicious, + ) + ) + assert rows == [] + + # The table must still exist and be queryable normally afterwards. + rows = list( + sqlite3_utils.select( + conn, "test", field_types=field_types, sql_types=sql_types, name="alice" + ) + ) + assert rows == [{"name": "alice"}] + + +def test_sqlite3_select_rejects_invalid_table_name(): + with sqlite3_utils.connect(":memory:") as conn: + with pytest.raises(ValueError): + list(sqlite3_utils.select(conn, "test; DROP TABLE test; --")) + + +def test_sqlite3_select_rejects_invalid_filter_key(): + with sqlite3_utils.connect(":memory:") as conn: + with pytest.raises(ValueError): + list(sqlite3_utils.select(conn, "test", **{"bad name; --": "value"}))