Skip to content

Commit

Permalink
Core: Reviews silenced bandit errors and increases robustness
Browse files Browse the repository at this point in the history
LINK: SEA-1010
TYPE: Bugfix
  • Loading branch information
Daverball authored Oct 31, 2024
1 parent a117d6b commit aac58ba
Show file tree
Hide file tree
Showing 13 changed files with 204 additions and 74 deletions.
17 changes: 4 additions & 13 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down
3 changes: 2 additions & 1 deletion src/onegov/core/cache.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand All @@ -121,6 +121,7 @@ def __init__(
redis_url: str,
):
super().__init__(
# FIXME: OGC-1893
serializer=dill_serialize,
deserializer=dill_deserialize
)
Expand Down
199 changes: 162 additions & 37 deletions src/onegov/core/cli/commands.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import click
import os
import platform
import shlex
import shutil
import smtplib
import ssl
Expand Down Expand Up @@ -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:
Expand All @@ -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(
Expand All @@ -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,
Expand All @@ -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))
Expand All @@ -459,33 +531,75 @@ 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
)

# 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

Expand Down Expand Up @@ -554,16 +668,27 @@ 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) "
f"VALUES ('generic', '{id_}', '[email protected]', "
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
)
Expand Down
4 changes: 3 additions & 1 deletion src/onegov/core/crypto/token.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
7 changes: 6 additions & 1 deletion src/onegov/core/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
5 changes: 3 additions & 2 deletions src/onegov/file/filters.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import shlex
import subprocess

from depot.fields.interfaces import FileFilter
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down
5 changes: 3 additions & 2 deletions src/onegov/file/integration.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Loading

0 comments on commit aac58ba

Please sign in to comment.