From 93f64085a1cc8bb4ebd5d81f5a95ba9decee668c Mon Sep 17 00:00:00 2001 From: Dan LaManna Date: Mon, 25 May 2026 12:43:49 -0400 Subject: [PATCH 1/2] Fix permission denied errors This also unifies the logic behind the symmetrical file and directory problems. --- isic_cli/cli/image.py | 4 +- isic_cli/cli/types.py | 75 ++++++++++++++++++++++++++++++++++---- tests/test_cli_image.py | 16 ++++++++ tests/test_cli_metadata.py | 16 ++++++++ 4 files changed, 102 insertions(+), 9 deletions(-) diff --git a/isic_cli/cli/image.py b/isic_cli/cli/image.py index 9f80262..f3dc34a 100644 --- a/isic_cli/cli/image.py +++ b/isic_cli/cli/image.py @@ -19,7 +19,7 @@ from rich.console import Console from rich.progress import Progress -from isic_cli.cli.types import CommaSeparatedCollectionIds, SearchString +from isic_cli.cli.types import CommaSeparatedCollectionIds, SearchString, WritableDirectoryPath from isic_cli.cli.utils import _extract_metadata, get_attributions, suggest_guest_login from isic_cli.io.http import ( download_image, @@ -114,7 +114,7 @@ def image(ctx): ) @click.argument( "outdir", - type=click.Path(file_okay=False, dir_okay=True, path_type=Path), + type=WritableDirectoryPath(file_okay=False, dir_okay=True, path_type=Path), ) @click.pass_obj @suggest_guest_login diff --git a/isic_cli/cli/types.py b/isic_cli/cli/types.py index 88ff534..0891543 100644 --- a/isic_cli/cli/types.py +++ b/isic_cli/cli/types.py @@ -1,5 +1,6 @@ from __future__ import annotations +import contextlib import re import sys @@ -135,17 +136,77 @@ def __init__(self, *args, **kwargs): def convert(self, value, param, ctx): value = super().convert(value, param, ctx) - # writeable checks on click.Path only apply to already existing paths, see - # https://github.com/pallets/click/issues/2495. - # check if the final path is writable before going to the effort of downloading the data. + # click.Path(writable=True) only validates existing paths, so it can't catch + # the case where the file must be created. See + # https://github.com/pallets/click/issues/2495. Attempt to open the file so we + # exercise the real OS check, then remove it so this stays a pure probe — + # a subsequent argument failing to parse must not leave a stray file behind. + # The callback recreates it for real. + # + # Actually performing the operation is more reliable than heuristics like + # os.access, which only checks POSIX mode bits and misses ACLs, read-only + # mounts, immutable flags, etc. — it can say "writable" for a path the + # kernel will still reject. If we want to know whether the write will work, + # the most truthful test is to try it. if value is not None and str(value) != "-": try: + # .exists() can itself raise (e.g. a filename that's too long), so + # it has to live inside the try alongside the write probe. + existed_before = value.exists() value.parent.mkdir(parents=True, exist_ok=True) with value.open("w", newline="", encoding="utf8"): pass - except (PermissionError, OSError): - # a user can end up here from lacking permissions, a read only filesystem, - # filenames that are too long or have invalid chars, etc. - self.fail(f"Permission denied - cannot write to '{value}'.", param, ctx) + except OSError as e: + msg = f"Permission denied - cannot write to '{value}': {e.strerror}." + self.fail(msg, param, ctx) + + # remove what we created — a later argument could still fail to parse + if not existed_before: + with contextlib.suppress(OSError): + value.unlink() + + return value + + +class WritableDirectoryPath(click.Path): + name = "writable_directory_path" + + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + + if self.file_okay: + raise ValueError("file_okay must be False") + elif not self.dir_okay: + raise ValueError("dir_okay must be True") + + def convert(self, value, param, ctx): + value = super().convert(value, param, ctx) + + # click.Path(writable=True) only validates existing paths, so it can't catch + # the case where the directory must be created. See + # https://github.com/pallets/click/issues/2495. Attempt the mkdir so we + # exercise the real OS check, then remove it so this stays a pure probe — + # a subsequent argument failing to parse must not leave a stray directory + # behind. The callback recreates it for real. + # + # Actually performing the operation is more reliable than heuristics like + # os.access, which only checks POSIX mode bits and misses ACLs, read-only + # mounts, immutable flags, etc. — it can say "writable" for a path the + # kernel will still reject. If we want to know whether mkdir will work, + # the most truthful test is to try it. + if value is not None: + try: + # .exists() can itself raise (e.g. a filename that's too long), so + # it has to live inside the try alongside the write probe. + existed_before = value.exists() + value.mkdir(parents=True, exist_ok=True) + except OSError as e: + msg = f"Permission denied - cannot write to '{value}': {e.strerror}." + self.fail(msg, param, ctx) + + # remove what we created — a later argument could still fail to parse + if not existed_before: + with contextlib.suppress(OSError): + value.rmdir() return value diff --git a/tests/test_cli_image.py b/tests/test_cli_image.py index 760f0ae..0b93a89 100644 --- a/tests/test_cli_image.py +++ b/tests/test_cli_image.py @@ -125,6 +125,22 @@ def test_image_download_legacy_diagnosis_unsupported(cli_run, outdir): assert "no longer supported" in result.output +@pytest.mark.usefixtures("_isolated_filesystem") +def test_image_download_unwritable_outdir(cli_run): + readonly = Path("readonly") + readonly.mkdir() + readonly.chmod(0o555) + try: + result = cli_run(["image", "download", str(readonly / "child")]) + finally: + readonly.chmod(0o755) + + assert result.exit_code == 2 + assert "Permission denied" in result.output + # probe must be side-effect-free: a failed validation leaves no stray dir + assert not (readonly / "child").exists() + + @pytest.mark.usefixtures("_isolated_filesystem", "_mock_images") def test_image_download_shows_size_info(cli_run, outdir): result = cli_run(["image", "download", outdir]) diff --git a/tests/test_cli_metadata.py b/tests/test_cli_metadata.py index 25b0b9a..c3ad29b 100644 --- a/tests/test_cli_metadata.py +++ b/tests/test_cli_metadata.py @@ -104,6 +104,22 @@ def test_metadata_download_permission_denied(cli_run, output_file): assert re.search(r"Permission denied", result.output), result.output +@pytest.mark.usefixtures("_mock_image_metadata", "_isolated_filesystem") +def test_metadata_download_unwritable_output(cli_run): + readonly = Path("readonly") + readonly.mkdir() + readonly.chmod(0o555) + try: + result = cli_run(["metadata", "download", "-o", str(readonly / "child.csv")]) + finally: + readonly.chmod(0o755) + + assert result.exit_code == 2 + assert re.search(r"Permission denied", result.output), result.output + # probe must be side-effect-free: a failed validation leaves no stray file + assert not (readonly / "child.csv").exists() + + @pytest.mark.usefixtures("_mock_image_metadata") @pytest.mark.parametrize( "cli_runner", From af79e4401ac06cc854735c83f3ef5ed4efbd4b8b Mon Sep 17 00:00:00 2001 From: Dan LaManna Date: Mon, 25 May 2026 13:14:18 -0400 Subject: [PATCH 2/2] Skip directory-write permission tests on Windows chmod(0o555) doesn't restrict writes inside a directory on Windows, so the write probe succeeds and the command exits 1 instead of 2. This mirrors the existing pytest.skip in test_metadata_download_permission_denied. --- tests/test_cli_image.py | 2 ++ tests/test_cli_metadata.py | 1 + 2 files changed, 3 insertions(+) diff --git a/tests/test_cli_image.py b/tests/test_cli_image.py index 0b93a89..69d684e 100644 --- a/tests/test_cli_image.py +++ b/tests/test_cli_image.py @@ -3,6 +3,7 @@ import logging import os from pathlib import Path +import sys import pytest from requests import HTTPError @@ -125,6 +126,7 @@ def test_image_download_legacy_diagnosis_unsupported(cli_run, outdir): assert "no longer supported" in result.output +@pytest.mark.skipif(sys.platform == "win32", reason="chmod doesn't restrict directory writes") @pytest.mark.usefixtures("_isolated_filesystem") def test_image_download_unwritable_outdir(cli_run): readonly = Path("readonly") diff --git a/tests/test_cli_metadata.py b/tests/test_cli_metadata.py index c3ad29b..617c53f 100644 --- a/tests/test_cli_metadata.py +++ b/tests/test_cli_metadata.py @@ -104,6 +104,7 @@ def test_metadata_download_permission_denied(cli_run, output_file): assert re.search(r"Permission denied", result.output), result.output +@pytest.mark.skipif(sys.platform == "win32", reason="chmod doesn't restrict directory writes") @pytest.mark.usefixtures("_mock_image_metadata", "_isolated_filesystem") def test_metadata_download_unwritable_output(cli_run): readonly = Path("readonly")