securedrop-workstation icon indicating copy to clipboard operation
securedrop-workstation copied to clipboard

Configure ruff

Open micahflee opened this issue 1 month ago • 0 comments

Status

Ready for review

Description of Changes

Fixes #1022.

This replaces flake8, bandit, black, and isort with ruff!

Many of the things that ruff flagged were assert statements in validate_config.py, and I already fixed those in #1025. So rather than fixing them again here, I merged that branch into this branch. So please review and merge #1025 first.

I started copying the ruff config from https://github.com/freedomofpress/securedrop-client/pull/1885, but then I paired it down to only include what's needed for this repo.

I added S108 (probably insecure usage of /tmp) to the ignore list, as it was being triggered just on this code in sdw_updater/Updater.py:

# We use a hardcoded temporary directory path in dom0. As dom0 is not
# a multi-user environment, we can safely assume that only the Updater is
# managing that filepath. Later on, we should consider porting the check-migration
# logic to leverage the Qubes Python API.
MIGRATION_DIR = "/tmp/sdw-migrations"  # nosec

I also added a per-file-ignore for SIM115 (use context handler for opening files) for sdw_util/Util.py specifically for the obtain_lock and can_obtain_lock functions. This check wants you to open files using the with statement so they automatically close, but in this case these functions open files and then return the file handle. So I think making an exception is fine.

Though I did notice a potential small bug. In can_obtain_lock, the lock file is opened temporarily, just to see if it can be opened, but I don't think it's ever closed since it returns a bool instead of the file handle -- though maybe this is fine and I just don't understand how fcntl.lockf works:

def can_obtain_lock(basename):
    """
    We temporarily obtain a shared, nonblocking lock to a lockfile to determine
    whether the associated process is currently running. Returns True if it is
    safe to continue execution (no lock conflict), False if not.

    `basename` is the basename of a lockfile situated in the LOCK_DIRECTORY.
    """
    lock_file = os.path.join(LOCK_DIRECTORY, basename)
    try:
        lh = open(lock_file)
    except FileNotFoundError:
        # Process may not have run during this session, safe to continue
        return True

    try:
        # Obtain a nonblocking, shared lock
        fcntl.lockf(lh, fcntl.LOCK_SH | fcntl.LOCK_NB)
    except OSError:
        sdlog.error(LOCK_ERROR.format(lock_file))
        return False

    return True

micahflee avatar May 08 '24 18:05 micahflee