diff --git a/pyproject.toml b/pyproject.toml index e8f9ca4b31..51eeacd00e 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -72,22 +72,13 @@ exclude_dirs = [ skips = [ # ignore asserts "B101", - # ignore pickle (SEA-1030) - "B301", - "B403", - # ignore XML (SEA-1031) - "B314", + # ignore lxml.etree.fromstring (we may want to revisit this in the future) "B320", + # ignore meta-codes, we are aware of the implications + "B403", + "B404", "B405", "B410", - # ignore subprocesses (SEA-1032) - "B404", - "B602", - "B603", - "B605", - "B607", - # ignore temp directory (SEA-1033) - "B108" ] [tool.ruff] diff --git a/src/onegov/core/cache.py b/src/onegov/core/cache.py index 5d27010901..8d9ae78437 100644 --- a/src/onegov/core/cache.py +++ b/src/onegov/core/cache.py @@ -103,7 +103,7 @@ def dill_serialize(value: Any) -> bytes: def dill_deserialize(value: 'bytes | NoValue') -> Any: if value is NO_VALUE: return value - return dill.loads(value) + return dill.loads(value) # nosec:B301 class RedisCacheRegion(CacheRegion): @@ -121,6 +121,7 @@ def __init__( redis_url: str, ): super().__init__( + # FIXME: OGC-1893 serializer=dill_serialize, deserializer=dill_deserialize ) diff --git a/src/onegov/core/cli/commands.py b/src/onegov/core/cli/commands.py index 22dd18a565..8e15e9e61c 100644 --- a/src/onegov/core/cli/commands.py +++ b/src/onegov/core/cli/commands.py @@ -1,6 +1,7 @@ import click import os import platform +import shlex import shutil import smtplib import ssl @@ -376,8 +377,11 @@ def transfer( try: remote_cfg = Config.from_yaml_string( - subprocess.check_output([ - 'ssh', server, '-C', "sudo cat '{}'".format(remote_config) + # NOTE: Using an absolute path is more trouble than it's worth + subprocess.check_output([ # nosec + 'ssh', server, '-C', 'sudo cat {}'.format( + shlex.quote(remote_config) + ) ]) ) except subprocess.CalledProcessError: @@ -397,15 +401,45 @@ def transfer_storage(remote: str, local: str, glob: str = '*') -> None: count = remote.count('/') count += platform.system() == 'Darwin' and 1 or 0 - send = f"ssh {server} -C 'sudo nice -n 10 tar cz {remote}/{glob}'" - send = f'{send} --absolute-names' - recv = f'tar xz --strip-components {count} -C {local}' + send = shlex.join([ + 'ssh', + server, + '-C', + shlex.join([ + 'sudo', + 'nice', + '-n', + '10', + 'tar', + 'cz', + f'{remote}/{glob}', + '--absolute-names', + ]), + ]) + recv = shlex.join([ + 'tar', + 'xz', + '--strip-components', + str(count), + '-C', + local, + ]) if shutil.which('pv'): - recv = f'pv -L 5m --name "{remote}/{glob}" -r -b | {recv}' + track_progress = shlex.join([ + 'pv', + '-L', + '5m', + '--name', + f'{remote}/{glob}', + '-r', + '-b', + ]) + recv = f'{track_progress} | {recv}' click.echo(f'Copying {remote}/{glob}') - subprocess.check_output(f'{send} | {recv}', shell=True) + # NOTE: We took extra care that this is safe with shlex.join + subprocess.check_output(f'{send} | {recv}', shell=True) # nosec:B602 @lru_cache(maxsize=None) def transfer_delta_storage( @@ -419,22 +453,44 @@ def transfer_delta_storage( # not it's contents. glob += '/***' - dry_run = ( - f"rsync -a --include='*/' --include='{glob}' --exclude='*' " - f"--dry-run --itemize-changes " - f"{server}:{remote}/ {local}/" - ) - subprocess.run(dry_run, shell=True, capture_output=False) - - send = ( - f"rsync -av --include='*/' --include='{glob}' --exclude='*' " - f"{server}:{remote}/ {local}/" - ) + dry_run = shlex.join([ + 'rsync', + '-a', + "--include='*/'", + '--include={}'.format(shlex.quote(glob)), + "--exclude='*'", + '--dry-run', + '--itemize-changes', + f'{server}:{remote}/', + f'{local}/', + ]) + # NOTE: We took extra care that this is safe with shlex.join + subprocess.run(dry_run, shell=True, capture_output=False) # nosec:B602 + + send = shlex.join([ + 'rsync', + '-av', + "--include='*/'", + '--include={}'.format(shlex.quote(glob)), + "--exclude='*'", + f'{server}:{remote}/', + f'{local}/', + ]) if shutil.which('pv'): - send = f"{send} | pv -L 5m --name '{remote}/{glob}' -r -b" + track_progress = shlex.join([ + 'pv', + '-L', + '5m', + '--name', + f'{remote}/{glob}', + '-r', + '-b', + ]) + send = f'{send} | {track_progress}' click.echo(f'Copying {remote}/{glob}') - subprocess.check_output(send, shell=True) + # NOTE: We took extra care that this is safe with shlex.join + subprocess.check_output(send, shell=True) # nosec:B602 def transfer_database( remote_db: str, @@ -445,10 +501,26 @@ def transfer_database( # Get available schemas query = 'SELECT schema_name FROM information_schema.schemata' - lst = f'sudo -u postgres psql {remote_db} -t -c "{query}"' - lst = f"ssh {server} '{lst}'" - - schemas_str = subprocess.check_output(lst, shell=True).decode('utf-8') + lst = shlex.join([ + 'sudo', + '-u', + 'postgres', + 'psql', + remote_db, + '-t', + '-c', + query, + ]) + lst = shlex.join([ + 'ssh', + server, + lst, + ]) + + # NOTE: We took extra care that this is safe with shlex.join + schemas_str = subprocess.check_output( + lst, shell=True # nosec:B602 + ).decode('utf-8') schemas_iter = (s.strip() for s in schemas_str.splitlines()) schemas_iter = (s for s in schemas_iter if s) schemas_iter = (s for s in schemas_iter if fnmatch(s, schema_glob)) @@ -459,24 +531,58 @@ def transfer_database( return schemas # Prepare send command - send = f'ssh {server} sudo -u postgres nice -n 10 pg_dump {remote_db}' - send = f'{send} --no-owner --no-privileges' - send = f'{send} --quote-all-identifiers --no-sync' - send = f'{send} --schema {" --schema ".join(schemas)}' + send_parts = [ + 'sudo', + '-u', + 'postgres', + 'nice', + '-n', + '10', + 'pg_dump', + remote_db, + '--no-owner', + '--no-privileges', + '--quote-all-identifiers', + '--no-sync', + ] + for schema in schemas: + send_parts.extend(('--schema', schema)) + + send = shlex.join(send_parts) + send = shlex.join([ + 'ssh', + server, + send, + ]) # Prepare receive command - recv = f'psql -d {local_db} -v ON_ERROR_STOP=1' + recv_parts = [ + 'psql', + '-d', + local_db, + '-v', + 'ON_ERROR_STOP=1', + ] if platform.system() == 'Linux': - recv = f'sudo -u postgres {recv}' + recv_parts = [ + 'sudo', + '-u', + 'postgres', + *recv_parts, + ] + + recv = shlex.join(recv_parts) # Drop existing schemas for schema in schemas: click.echo(f'Drop local database schema {schema}') + assert "'" not in schema and '"' not in schema drop = f'DROP SCHEMA IF EXISTS "{schema}" CASCADE' drop = f"echo '{drop}' | {recv}" - subprocess.check_call( - drop, shell=True, + subprocess.check_call( # nosec:B602 + drop, + shell=True, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL ) @@ -484,8 +590,16 @@ def transfer_database( # Transfer click.echo('Transfering database') if shutil.which('pv'): - recv = f'pv --name "{remote_db}@postgres" -r -b | {recv}' - subprocess.check_output(f'{send} | {recv}', shell=True) + track_progress = shlex.join([ + 'pv', + '--name', + f'{remote_db}@postgres', + '-r', + '-b', + ]) + recv = f'{track_progress} | {recv}' + # NOTE: We took extra care that this is safe with shlex.join + subprocess.check_output(f'{send} | {recv}', shell=True) # nosec:B602 return schemas @@ -554,6 +668,7 @@ def transfer_database_of_app( def do_add_admins(local_cfg: 'ApplicationConfig', schema: str) -> None: id_ = str(uuid4()) password_hash = hash_password('test').replace('$', '\\$') + assert '"' not in schema and "'" not in schema query = ( f'INSERT INTO \\"{schema}\\".users ' # nosec: B608 f"(type, id, username, password_hash, role, active, realname) " @@ -561,9 +676,19 @@ def do_add_admins(local_cfg: 'ApplicationConfig', schema: str) -> None: f"'{password_hash}', 'admin', true, 'John Doe');" ) local_db = local_cfg.configuration['dsn'].split('/')[-1] - command = f'sudo -u postgres psql {local_db} -c "{query}"' - subprocess.check_call( - command, shell=True, + command = shlex.join([ + 'sudo', + '-u', + 'postgres', + 'psql', + local_db, + '-c', + query, + ]) + # NOTE: We took extra care that this is safe with shlex.join + subprocess.check_call( # nosec:B602 + command, + shell=True, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL ) diff --git a/src/onegov/core/crypto/token.py b/src/onegov/core/crypto/token.py index 71c07082f8..fe231fafdd 100644 --- a/src/onegov/core/crypto/token.py +++ b/src/onegov/core/crypto/token.py @@ -48,7 +48,9 @@ def stored_random_token(namespace: str, name: str) -> str: general use! """ - namespace_dir = os.path.join('/tmp/onegov-secrets', namespace) + # NOTE: Since this is only used for development: + # The hardcoded path is a feature, not a bug + namespace_dir = os.path.join('/tmp/onegov-secrets', namespace) # nosec:B108 os.makedirs(namespace_dir, mode=0o700, exist_ok=True) path = os.path.join(namespace_dir, name) diff --git a/src/onegov/core/utils.py b/src/onegov/core/utils.py index 4364f66294..3b3e8797a1 100644 --- a/src/onegov/core/utils.py +++ b/src/onegov/core/utils.py @@ -109,7 +109,12 @@ def local_lock(namespace: str, key: str) -> 'Iterator[None]': """ name = f'{namespace}-{key}'.replace('/', '-') - with open(f'/tmp/{name}', 'w+') as f: + # NOTE: hardcoding /tmp is a bit piggy, but on the other hand we + # don't want different processes to miss each others locks + # just because one of them has a different TMPDIR, can we + # come up with a more robust way of doing this, e.g. with + # named semaphores? + with open(f'/tmp/{name}', 'w+') as f: # nosec:B108 try: fcntl.flock(f, fcntl.LOCK_EX | fcntl.LOCK_NB) yield diff --git a/src/onegov/file/filters.py b/src/onegov/file/filters.py index 968dafa839..ff40a510df 100644 --- a/src/onegov/file/filters.py +++ b/src/onegov/file/filters.py @@ -1,3 +1,4 @@ +import shlex import subprocess from depot.fields.interfaces import FileFilter @@ -145,7 +146,7 @@ def generate_preview(self, fp: 'SupportsRead[bytes]') -> BytesIO: with pdf_input.open('wb') as pdf: pdf.write(fp.read()) - process = subprocess.run(( + process = subprocess.run(( # nosec:B603 'gs', # disable read/writes outside of the given files @@ -173,7 +174,7 @@ def generate_preview(self, fp: 'SupportsRead[bytes]') -> BytesIO: # output to png '-sDEVICE=png16m', - f'-sOutputFile={png_output}', + '-sOutputFile={}'.format(shlex.quote(str(png_output))), # from pdf str(pdf_input) diff --git a/src/onegov/file/integration.py b/src/onegov/file/integration.py index af371d83dd..18ef82a39c 100644 --- a/src/onegov/file/integration.py +++ b/src/onegov/file/integration.py @@ -244,11 +244,12 @@ def bust_frontend_cache(self, file_id: str) -> None: if not self.frontend_cache_buster: return + delay = shlex.quote(str(self.frontend_cache_bust_delay)) bin = shlex.quote(self.frontend_cache_buster) fid = shlex.quote(file_id) - cmd = f'sleep {self.frontend_cache_bust_delay} && {bin} {fid}' + cmd = f'sleep {delay} && {bin} {fid}' - subprocess.Popen(cmd, close_fds=True, shell=True) + subprocess.Popen(cmd, close_fds=True, shell=True) # nosec:B602 def sign_file( # nosec: B105,B107 self, diff --git a/src/onegov/foundation6/cli.py b/src/onegov/foundation6/cli.py index 4a6e975c72..73cd1a6961 100644 --- a/src/onegov/foundation6/cli.py +++ b/src/onegov/foundation6/cli.py @@ -11,7 +11,7 @@ def pre_checks() -> None: - node_version = check_output('node --version', shell=True) + node_version = check_output('node --version', shell=True) # nosec if 'v10' not in node_version.decode('utf-8'): click.secho('Foundation CLI currently works with node version 10') sys.exit() @@ -50,15 +50,15 @@ def update() -> None: foundation_js_files = ('foundation.min.js', 'foundation.js') - os.chdir('/tmp') - if not os.path.exists('/tmp/foundation-update'): - os.system( + os.chdir('/tmp') # nosec:B108 + if not os.path.exists('/tmp/foundation-update'): # nosec:B108 + os.system( # nosec 'echo foundation-update | ' 'foundation new --framework sites --template zurb' ) os.chdir('foundation-update') - node_root = Path('/tmp/foundation-update/node_modules/foundation-sites') + node_root = Path('/tmp/foundation-update/node_modules/foundation-sites') # nosec:B108 # click.secho('Create a backup', fg='green') # shutil.copytree(src, src_bckp) diff --git a/src/onegov/org/formats/digirez.py b/src/onegov/org/formats/digirez.py index d64bbbb3e3..ff0cb54afa 100644 --- a/src/onegov/org/formats/digirez.py +++ b/src/onegov/org/formats/digirez.py @@ -28,7 +28,8 @@ def __init__(self, accessdb_path: str) -> None: @property def tables(self) -> list[str]: - output = subprocess.check_output(('mdb-tables', self.accessdb_path)) + output = subprocess.check_output( # nosec:B603 + ('mdb-tables', self.accessdb_path)) output_str = output.decode('utf-8').rstrip('\n ') return output_str.split(' ') @@ -50,7 +51,7 @@ def open(self) -> None: output_path = os.path.join(self.csv_path, f'{table}.csv') with open(output_path, 'w') as output_file: - subprocess.check_call( + subprocess.check_call( # nosec:B603 args=( 'mdb-export', '-D', '%Y-%m-%dT%T', self.accessdb_path, table diff --git a/src/onegov/server/cli.py b/src/onegov/server/cli.py index fd440ab7d2..8878e3df41 100644 --- a/src/onegov/server/cli.py +++ b/src/onegov/server/cli.py @@ -459,7 +459,7 @@ def run(self) -> None: # reset the tty every time, fixing problems that might occur if # the process is restarted during a pdb session - os.system('stty sane') + os.system('stty sane') # nosec try: if sys.platform == 'darwin': diff --git a/src/onegov/swissvotes/external_resources/posters.py b/src/onegov/swissvotes/external_resources/posters.py index 48cbd8b02b..ea8a9a1d48 100644 --- a/src/onegov/swissvotes/external_resources/posters.py +++ b/src/onegov/swissvotes/external_resources/posters.py @@ -1,7 +1,7 @@ +import lxml.etree import requests from onegov.swissvotes import log from onegov.swissvotes.models import SwissVote -from xml.etree import ElementTree from typing import Any @@ -24,7 +24,7 @@ def meta_data_url(self, url: str) -> str: raise NotImplementedError() def parse_xml(self, response: requests.Response) -> str: - tree = ElementTree.fromstring(response.content) + tree = lxml.etree.fromstring(response.content) element = tree.find("./field[@name='primaryMedia']/value") if element is None or not element.text: raise ValueError('No primary media found') diff --git a/src/onegov/winterthur/app.py b/src/onegov/winterthur/app.py index 6204936c42..a99626544b 100644 --- a/src/onegov/winterthur/app.py +++ b/src/onegov/winterthur/app.py @@ -1,4 +1,5 @@ import re +import shlex import subprocess from functools import cached_property @@ -137,7 +138,7 @@ def get_shift_schedule_image(self) -> BytesIO | None: with (path / 'input.pdf').open('wb') as pdf: pdf.write(file.reference.file.read()) - process = subprocess.run(( + process = subprocess.run(( # nosec:B603 'gs', # disable read/writes outside of the given files @@ -157,15 +158,17 @@ def get_shift_schedule_image(self) -> BytesIO | None: '-dPDFFitPage', # render in high resolution before downscaling to 300 dpi - f'-r{300}', - f'-dDownScaleFactor={1}', + '-r300', + '-dDownScaleFactor=1', # only use the first page '-dLastPage=1', # output to png '-sDEVICE=png16m', - f'-sOutputFile={path / "preview.png"}', + '-sOutputFile={}'.format( + shlex.quote(str(path / 'preview.png')) + ), # force landscape orientation in postscript '-c', diff --git a/tests/onegov/swissvotes/test_external_resources.py b/tests/onegov/swissvotes/test_external_resources.py index 927f191a12..fee4b1d3b5 100644 --- a/tests/onegov/swissvotes/test_external_resources.py +++ b/tests/onegov/swissvotes/test_external_resources.py @@ -1,6 +1,6 @@ from datetime import date from decimal import Decimal -from xml.etree.ElementTree import ParseError +from lxml.etree import XMLSyntaxError from onegov.core.utils import Bunch from onegov.swissvotes.collections import SwissVoteCollection @@ -194,9 +194,9 @@ def meta_data_url(self, url): # parse xml posters = MyPosters() - with raises(TypeError): + with raises(ValueError): posters.parse_xml(Bunch(content=None)) - with raises(ParseError): + with raises(XMLSyntaxError): posters.parse_xml(Bunch(content='')) with raises(ValueError): posters.parse_xml(Bunch(content=''))