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..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,23 @@ 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") + 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..617c53f 100644 --- a/tests/test_cli_metadata.py +++ b/tests/test_cli_metadata.py @@ -104,6 +104,23 @@ 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") + 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",