diff --git a/cwmscli/commands/blob.py b/cwmscli/commands/blob.py index 756c0f5..b058266 100644 --- a/cwmscli/commands/blob.py +++ b/cwmscli/commands/blob.py @@ -11,10 +11,12 @@ from cwmscli.utils import ( colors, + format_local_download_error, get_api_key, has_invalid_chars, init_cwms_session, log_scoped_read_hint, + validate_default_download_dest, ) from cwmscli.utils.click_help import DOCS_BASE_URL from cwmscli.utils.deps import requires @@ -109,6 +111,14 @@ def _save_blob_content( return dest +def _default_download_dest(blob_id: str) -> str: + return validate_default_download_dest( + blob_id, + resource_name="Blob", + docs_url=BLOB_DOCS_URL, + ) + + def _blob_media_type(cwms_module, office: str, blob_id: str) -> Optional[str]: try: result = cwms_module.get_blobs(office_id=office, blob_id_like=blob_id) @@ -602,7 +612,7 @@ def download_cmd( try: blob_content = cwms.get_blob(office_id=office, blob_id=bid) - target = dest or bid + target = dest or _default_download_dest(bid) _save_blob_content( blob_content, dest=target, @@ -621,14 +631,15 @@ def download_cmd( ) sys.exit(1) except Exception as e: - logging.error(f"Failed to download: {e}") - log_scoped_read_hint( - credential_kind=credential_kind, - anonymous=anonymous, - office=office, - action="download", - resource="blob content", - ) + logging.error(format_local_download_error(e, BLOB_DOCS_URL)) + if not isinstance(e, (OSError, ValueError)): + log_scoped_read_hint( + credential_kind=credential_kind, + anonymous=anonymous, + office=office, + action="download", + resource="blob content", + ) sys.exit(1) diff --git a/cwmscli/commands/clob.py b/cwmscli/commands/clob.py index a2584b9..613467f 100644 --- a/cwmscli/commands/clob.py +++ b/cwmscli/commands/clob.py @@ -9,7 +9,13 @@ import requests from cwms import api as cwms_api -from cwmscli.utils import get_api_key, has_invalid_chars, log_scoped_read_hint +from cwmscli.utils import ( + format_local_download_error, + get_api_key, + has_invalid_chars, + log_scoped_read_hint, + validate_default_download_dest, +) def _join_api_url(api_root: str, path: str) -> str: @@ -29,6 +35,10 @@ def _write_clob_content(content: str, dest: str) -> str: return dest +def _default_download_dest(clob_id: str) -> str: + return validate_default_download_dest(clob_id, resource_name="Clob") + + def _clob_endpoint_id(clob_id: str) -> tuple[str, Optional[str]]: normalized = clob_id.upper() if has_invalid_chars(normalized): @@ -198,7 +208,7 @@ def download_cmd( content = str(payload) else: content = _get_special_clob_text(office=office, clob_id=query_id) - target = dest or bid + target = dest or _default_download_dest(bid) _write_clob_content(content, target) logging.info(f"Downloaded clob to: {target}") except requests.HTTPError as e: @@ -213,14 +223,7 @@ def download_cmd( ) sys.exit(1) except Exception as e: - logging.error(f"Failed to download: {e}") - log_scoped_read_hint( - api_key=resolved_api_key, - anonymous=anonymous, - office=office, - action="download", - resource="clob content", - ) + logging.error(format_local_download_error(e, "")) sys.exit(1) diff --git a/cwmscli/utils/__init__.py b/cwmscli/utils/__init__.py index a540cbd..462b8ab 100644 --- a/cwmscli/utils/__init__.py +++ b/cwmscli/utils/__init__.py @@ -1,4 +1,5 @@ import logging as py_logging +import re import time from pathlib import Path from typing import Optional, Union @@ -235,6 +236,66 @@ def log_scoped_read_hint( ) +def format_local_download_error(error: Exception, docs_url: str) -> str: + if isinstance(error, (OSError, ValueError)): + message = ( + f"{colors.c('Failed to download:', 'red', bright=True)} {error}. " + f"If this is a local destination/path issue, pass " + f"{colors.c('--dest', 'cyan', bright=True)} explicitly." + ) + if docs_url: + message = ( + f"{message} {colors.c('Docs:', 'blue', bright=True)} " + f"{colors.c(docs_url, 'blue', bright=True)}" + ) + return message + return f"{colors.c('Failed to download:', 'red', bright=True)} {error}" + + +def validate_default_download_dest( + raw_id: str, + *, + resource_name: str, + docs_url: str = "", +) -> str: + if raw_id is None: + raise ValueError( + f"{resource_name} ID must include a non-root destination name. " + f"Pass --dest explicitly if needed." + ) + + if raw_id.startswith("//") or raw_id.startswith("\\\\"): + raise ValueError( + f"{resource_name} ID must resolve to a relative local path. " + f"Pass --dest explicitly if needed." + ) + + target = raw_id.lstrip("/\\") + if not target: + message = ( + f"{resource_name} ID must include a non-root destination name. " + f"Pass --dest explicitly if needed." + ) + if docs_url: + message = f"{message} Docs: {docs_url}" + raise ValueError(message) + + if re.match(r"^[A-Za-z]:", target): + raise ValueError( + f"{resource_name} ID must resolve to a relative local path. " + f"Pass --dest explicitly if needed." + ) + + parts = re.split(r"[\\/]", target) + if any(part in {"", ".", ".."} for part in parts): + raise ValueError( + f"{resource_name} ID must resolve to a relative local path. " + f"Pass --dest explicitly if needed." + ) + + return target + + def common_api_options(f): f = log_level_option(f) f = office_option(f) diff --git a/tests/commands/test_blob_upload.py b/tests/commands/test_blob_upload.py index c70696d..f9c037e 100644 --- a/tests/commands/test_blob_upload.py +++ b/tests/commands/test_blob_upload.py @@ -1,3 +1,4 @@ +import logging import sys import types @@ -6,6 +7,7 @@ from cwmscli.commands.blob import ( _blob_id_for_path, + _default_download_dest, _find_blob_id_collisions, _list_matching_files, _save_blob_content, @@ -17,7 +19,7 @@ @pytest.fixture(autouse=True) -def no_saved_login(monkeypatch): +def no_saved_login(monkeypatch: pytest.MonkeyPatch): monkeypatch.setattr( "cwmscli.utils.get_saved_login_token", lambda *args, **kwargs: None ) @@ -44,7 +46,9 @@ def test_blob_id_for_path_uses_prefix_and_relative_path(): assert blob_id == "OPS_REPORTS_JAN_FINAL" -def test_upload_cmd_continues_on_error_for_directory(tmp_path, monkeypatch): +def test_upload_cmd_continues_on_error_for_directory( + tmp_path, monkeypatch: pytest.MonkeyPatch +): file_a = tmp_path / "a.txt" file_b = tmp_path / "b.txt" file_a.write_text("a") @@ -112,7 +116,7 @@ def test_find_blob_id_collisions_detects_same_stem_and_path_collisions(): def test_upload_cmd_aborts_before_upload_when_generated_ids_collide( - tmp_path, monkeypatch + tmp_path, monkeypatch: pytest.MonkeyPatch ): (tmp_path / "a.txt").write_text("a") (tmp_path / "a.json").write_text("{}") @@ -168,7 +172,14 @@ def test_save_blob_content_writes_raw_text(tmp_path): assert dest.read_text(encoding="utf-8") == "plain text payload" -def test_download_cmd_uses_media_type_to_write_text(tmp_path, monkeypatch): +def test_default_download_dest_strips_leading_path_separators(): + assert _default_download_dest("/REPORTS/REL-BLB") == "REPORTS/REL-BLB" + assert _default_download_dest("\\REPORTS\\REL-BLB") == "REPORTS\\REL-BLB" + + +def test_download_cmd_uses_media_type_to_write_text( + tmp_path, monkeypatch: pytest.MonkeyPatch +): dest = tmp_path / "downloaded" class FakeBlobListing: @@ -216,7 +227,63 @@ class FakeHTTPError(Exception): assert saved.read_text(encoding="utf-8") == "retrieved text" -def test_download_cmd_initializes_session_with_api_key(tmp_path, monkeypatch): +def test_download_cmd_default_dest_stays_relative_for_leading_slash_id( + tmp_path, monkeypatch: pytest.MonkeyPatch +): + class FakeBlobListing: + df = pd.DataFrame( + [ + { + "id": "/REPORTS/REL-BLB", + "media-type-id": "text/plain", + "description": "x", + } + ] + ) + + class FakeCwms: + @staticmethod + def init_session(api_root, api_key=None): + return None + + @staticmethod + def get_blob(office_id, blob_id): + assert blob_id == "/REPORTS/REL-BLB" + return "retrieved text" + + @staticmethod + def get_blobs(office_id, blob_id_like): + assert blob_id_like == "/REPORTS/REL-BLB" + return FakeBlobListing() + + monkeypatch.setitem(sys.modules, "cwms", FakeCwms) + + class FakeHTTPError(Exception): + pass + + monkeypatch.setitem( + sys.modules, "requests", types.SimpleNamespace(HTTPError=FakeHTTPError) + ) + + monkeypatch.chdir(tmp_path) + + download_cmd( + blob_id="/reports/rel-blb", + dest=None, + office="SWT", + api_root="https://example.test/", + api_key="x", + dry_run=False, + ) + + saved = tmp_path / "REPORTS" / "REL-BLB.txt" + assert saved.exists() + assert saved.read_text(encoding="utf-8") == "retrieved text" + + +def test_download_cmd_initializes_session_with_api_key( + tmp_path, monkeypatch: pytest.MonkeyPatch +): dest = tmp_path / "downloaded" calls = [] @@ -260,7 +327,62 @@ class FakeHTTPError(Exception): assert calls == [("init_session", "https://example.test/", "apikey 123")] -def test_list_cmd_initializes_session_with_api_key(monkeypatch): +def test_download_cmd_local_error_skips_scope_hint_and_logs_docs( + monkeypatch: pytest.MonkeyPatch, caplog +): + class FakeBlobListing: + df = pd.DataFrame( + [{"id": "TEST_TXT", "media-type-id": "text/plain", "description": "x"}] + ) + + class FakeCwms: + @staticmethod + def init_session(api_root, api_key=None): + return None + + @staticmethod + def get_blob(office_id, blob_id): + return "retrieved text" + + @staticmethod + def get_blobs(office_id, blob_id_like): + return FakeBlobListing() + + monkeypatch.setitem(sys.modules, "cwms", FakeCwms) + + class FakeHTTPError(Exception): + pass + + monkeypatch.setitem( + sys.modules, "requests", types.SimpleNamespace(HTTPError=FakeHTTPError) + ) + monkeypatch.setattr( + "cwmscli.commands.blob.log_scoped_read_hint", + lambda **kwargs: (_ for _ in ()).throw( + AssertionError("scope hint should not run") + ), + ) + monkeypatch.setattr( + "cwmscli.commands.blob._save_blob_content", + lambda *args, **kwargs: (_ for _ in ()).throw(PermissionError("denied")), + ) + + with caplog.at_level(logging.ERROR), pytest.raises(SystemExit) as exc: + download_cmd( + blob_id="test_txt", + dest="downloaded", + office="SWT", + api_root="https://example.test/", + api_key="apikey 123", + dry_run=False, + ) + + assert exc.value.code == 1 + assert "pass --dest explicitly" in caplog.text + assert "/cli/blob.html" in caplog.text + + +def test_list_cmd_initializes_session_with_api_key(monkeypatch: pytest.MonkeyPatch): calls = [] class FakeCwms: @@ -297,7 +419,7 @@ def get_blobs(office_id, blob_id_like, page_size=None): ] -def test_list_cmd_uses_limit_as_fetch_page_size(monkeypatch): +def test_list_cmd_uses_limit_as_fetch_page_size(monkeypatch: pytest.MonkeyPatch): calls = [] class FakeCwms: @@ -333,7 +455,7 @@ def get_blobs(office_id, blob_id_like, page_size=None): assert calls == [("SWT", "TEST_.*", 25)] -def test_list_cmd_page_size_overrides_limit_for_fetch(monkeypatch): +def test_list_cmd_page_size_overrides_limit_for_fetch(monkeypatch: pytest.MonkeyPatch): calls = [] class FakeCwms: @@ -366,7 +488,9 @@ def get_blobs(office_id, blob_id_like, page_size=None): assert calls == [("SWT", "TEST_.*", 200)] -def test_delete_cmd_uses_query_override_for_special_char_ids(monkeypatch): +def test_delete_cmd_uses_query_override_for_special_char_ids( + monkeypatch: pytest.MonkeyPatch, +): calls = [] class FakeApi: @@ -412,7 +536,9 @@ class FakeHTTPError(Exception): ] -def test_download_cmd_anonymous_skips_api_key(tmp_path, monkeypatch): +def test_download_cmd_anonymous_skips_api_key( + tmp_path, monkeypatch: pytest.MonkeyPatch +): dest = tmp_path / "downloaded" calls = [] @@ -457,7 +583,7 @@ class FakeHTTPError(Exception): assert calls == [("init_session", "https://example.test/", None)] -def test_list_cmd_anonymous_skips_api_key(monkeypatch): +def test_list_cmd_anonymous_skips_api_key(monkeypatch: pytest.MonkeyPatch): calls = [] class FakeCwms: @@ -495,7 +621,9 @@ def get_blobs(office_id, blob_id_like, page_size=None): ] -def test_download_cmd_prefers_saved_token_over_api_key(tmp_path, monkeypatch): +def test_download_cmd_prefers_saved_token_over_api_key( + tmp_path, monkeypatch: pytest.MonkeyPatch +): dest = tmp_path / "downloaded" calls = [] diff --git a/tests/commands/test_clob.py b/tests/commands/test_clob.py index d32ef8d..1f36d82 100644 --- a/tests/commands/test_clob.py +++ b/tests/commands/test_clob.py @@ -1,11 +1,14 @@ +import logging import sys import types import pandas as pd +import pytest from cwmscli.commands import commands_cwms from cwmscli.commands.clob import ( _clob_endpoint_id, + _default_download_dest, delete_cmd, download_cmd, list_cmd, @@ -36,7 +39,14 @@ def test_clob_endpoint_id_uses_ignored_path_for_special_chars(): assert _clob_endpoint_id("path/id") == ("ignored", "PATH/ID") -def test_download_cmd_uses_default_dest_and_writes_text(tmp_path, monkeypatch): +def test_default_download_dest_strips_leading_path_separators(): + assert _default_download_dest("/REPORTS/REL-CLOB") == "REPORTS/REL-CLOB" + assert _default_download_dest("\\REPORTS\\REL-CLOB") == "REPORTS\\REL-CLOB" + + +def test_download_cmd_uses_default_dest_and_writes_text( + tmp_path, monkeypatch: pytest.MonkeyPatch +): calls = [] class FakeClobResponse: @@ -87,7 +97,51 @@ class FakeHTTPError(Exception): ] -def test_download_cmd_uses_query_override_for_special_char_ids(tmp_path, monkeypatch): +def test_download_cmd_default_dest_stays_relative_for_leading_slash_id( + tmp_path, monkeypatch: pytest.MonkeyPatch +): + class FakeCwms: + @staticmethod + def init_session(api_root, api_key): + return None + + monkeypatch.setitem(sys.modules, "cwms", FakeCwms) + monkeypatch.setattr("cwmscli.commands.clob.cwms", FakeCwms) + monkeypatch.setattr( + "cwmscli.commands.clob._get_special_clob_text", + lambda office, clob_id: "retrieved clob text", + ) + + class FakeHTTPError(Exception): + pass + + monkeypatch.setitem( + sys.modules, "requests", types.SimpleNamespace(HTTPError=FakeHTTPError) + ) + monkeypatch.setattr( + "cwmscli.commands.clob.requests", + types.SimpleNamespace(HTTPError=FakeHTTPError), + ) + + monkeypatch.chdir(tmp_path) + + download_cmd( + clob_id="/reports/rel-clob", + dest=None, + office="SWT", + api_root="https://example.test/", + api_key="apikey 123", + dry_run=False, + ) + + saved = tmp_path / "REPORTS" / "REL-CLOB" + assert saved.exists() + assert saved.read_text(encoding="utf-8") == "retrieved clob text" + + +def test_download_cmd_uses_query_override_for_special_char_ids( + tmp_path, monkeypatch: pytest.MonkeyPatch +): calls = [] class FakeCwms: @@ -154,7 +208,9 @@ class FakeHTTPError(Exception): ] -def test_download_cmd_anonymous_skips_api_key(tmp_path, monkeypatch): +def test_download_cmd_anonymous_skips_api_key( + tmp_path, monkeypatch: pytest.MonkeyPatch +): calls = [] class FakeClobResponse: @@ -197,7 +253,61 @@ class FakeHTTPError(Exception): assert calls == [("init_session", "https://example.test/", None)] -def test_list_cmd_initializes_session_with_api_key(monkeypatch): +def test_download_cmd_local_error_skips_scope_hint( + monkeypatch: pytest.MonkeyPatch, caplog +): + class FakeClobResponse: + json = {"value": "retrieved clob text"} + + class FakeCwms: + @staticmethod + def init_session(api_root, api_key): + return None + + @staticmethod + def get_clob(office_id, clob_id): + return FakeClobResponse() + + monkeypatch.setitem(sys.modules, "cwms", FakeCwms) + monkeypatch.setattr("cwmscli.commands.clob.cwms", FakeCwms) + + class FakeHTTPError(Exception): + pass + + monkeypatch.setitem( + sys.modules, "requests", types.SimpleNamespace(HTTPError=FakeHTTPError) + ) + monkeypatch.setattr( + "cwmscli.commands.clob.requests", + types.SimpleNamespace(HTTPError=FakeHTTPError), + ) + monkeypatch.setattr( + "cwmscli.commands.clob.log_scoped_read_hint", + lambda **kwargs: (_ for _ in ()).throw( + AssertionError("scope hint should not run") + ), + ) + monkeypatch.setattr( + "cwmscli.commands.clob._write_clob_content", + lambda *args, **kwargs: (_ for _ in ()).throw(PermissionError("denied")), + ) + + with caplog.at_level(logging.ERROR), pytest.raises(SystemExit) as exc: + download_cmd( + clob_id="test_clob", + dest="downloaded.txt", + office="SWT", + api_root="https://example.test/", + api_key="apikey 123", + dry_run=False, + ) + + assert exc.value.code == 1 + assert "pass --dest explicitly" in caplog.text + assert "/cli/blob.html" not in caplog.text + + +def test_list_cmd_initializes_session_with_api_key(monkeypatch: pytest.MonkeyPatch): calls = [] class FakeCwms: @@ -233,7 +343,7 @@ def get_clobs(office_id, clob_id_like, page_size=None): ] -def test_list_cmd_uses_limit_as_fetch_page_size(monkeypatch): +def test_list_cmd_uses_limit_as_fetch_page_size(monkeypatch: pytest.MonkeyPatch): calls = [] class FakeCwms: @@ -270,7 +380,7 @@ def get_clobs(office_id, clob_id_like, page_size=None): assert calls == [("SWT", "TEST_.*", 25)] -def test_list_cmd_page_size_overrides_limit_for_fetch(monkeypatch): +def test_list_cmd_page_size_overrides_limit_for_fetch(monkeypatch: pytest.MonkeyPatch): calls = [] class FakeCwms: @@ -302,7 +412,7 @@ def get_clobs(office_id, clob_id_like, page_size=None): assert calls == [("SWT", "TEST_.*", 200)] -def test_list_cmd_anonymous_skips_api_key(monkeypatch): +def test_list_cmd_anonymous_skips_api_key(monkeypatch: pytest.MonkeyPatch): calls = [] class FakeCwms: @@ -339,7 +449,9 @@ def get_clobs(office_id, clob_id_like, page_size=None): ] -def test_delete_cmd_uses_query_override_for_special_char_ids(monkeypatch): +def test_delete_cmd_uses_query_override_for_special_char_ids( + monkeypatch: pytest.MonkeyPatch, +): calls = [] class FakeCwms: @@ -379,7 +491,9 @@ def delete(endpoint, params=None): ] -def test_update_cmd_uses_query_override_for_special_char_ids(tmp_path, monkeypatch): +def test_update_cmd_uses_query_override_for_special_char_ids( + tmp_path, monkeypatch: pytest.MonkeyPatch +): calls = [] file_path = tmp_path / "updated.txt" file_path.write_text("updated clob text", encoding="utf-8") diff --git a/tests/commands/test_download_dest_safety.py b/tests/commands/test_download_dest_safety.py new file mode 100644 index 0000000..a9f9d5c --- /dev/null +++ b/tests/commands/test_download_dest_safety.py @@ -0,0 +1,52 @@ +import pytest + +from cwmscli.commands.blob import _default_download_dest as blob_default_download_dest +from cwmscli.commands.clob import _default_download_dest as clob_default_download_dest + +SAFE_CASES = [ + ("REPORTS/REL-BLB", "REPORTS/REL-BLB"), + ("REPORTS\\REL-BLB", "REPORTS\\REL-BLB"), + ("/REPORTS/REL-BLB", "REPORTS/REL-BLB"), + ("\\REPORTS\\REL-BLB", "REPORTS\\REL-BLB"), +] + + +UNSAFE_CASES = [ + "", + "/", + "\\", + "../REPORTS/REL-BLB", + "..\\REPORTS\\REL-BLB", + "./REPORTS/REL-BLB", + ".\\REPORTS\\REL-BLB", + "REPORTS/../REL-BLB", + "REPORTS\\..\\REL-BLB", + "REPORTS/./REL-BLB", + "REPORTS\\.\\REL-BLB", + "C:/REPORTS/REL-BLB", + "C:\\REPORTS\\REL-BLB", + "//server/share/file", + "\\\\server\\share\\file", +] + + +@pytest.mark.parametrize("blob_id,expected", SAFE_CASES) +def test_blob_default_download_dest_allows_safe_relative_paths(blob_id, expected): + assert blob_default_download_dest(blob_id) == expected + + +@pytest.mark.parametrize("clob_id,expected", SAFE_CASES) +def test_clob_default_download_dest_allows_safe_relative_paths(clob_id, expected): + assert clob_default_download_dest(clob_id) == expected + + +@pytest.mark.parametrize("blob_id", UNSAFE_CASES) +def test_blob_default_download_dest_rejects_unsafe_paths(blob_id): + with pytest.raises(ValueError): + blob_default_download_dest(blob_id) + + +@pytest.mark.parametrize("clob_id", UNSAFE_CASES) +def test_clob_default_download_dest_rejects_unsafe_paths(clob_id): + with pytest.raises(ValueError): + clob_default_download_dest(clob_id)