fix: address follow-up Copilot review (error typing, docs, tests)

- __init__.py preset/extension URL installs: give read_response_limited a
  domain error_type (PresetError / ExtensionError) and catch that instead
  of a blanket ValueError, so an oversized body is reported cleanly while
  unrelated ValueErrors surface as real errors. The extension catch now
  also covers install_from_zip's ValidationError (an ExtensionError).
- _utils.run_command: rewrite the misleading docstring — shell=False is the
  only honoured mode; shell=True is rejected with ValueError, the parameter
  is retained only so existing keyword callers don't hit TypeError.
- _download_security: document that the loopback allowance is an exact-string
  match (not an IP-range check), that read_response_limited's max_bytes
  default is the 50 MiB ceiling (callers with tighter budgets should pass an
  explicit value), and how _safe_zip_name handles single trailing-slash
  directory markers vs malformed empty segments.
- authentication/http: comment the empty-hosts _StripAuthOnRedirect use as the
  HTTPS-downgrade guard on the unauthenticated path.
- check_security_requirements: document the HEAD^ fallback failing safe (audit
  anyway) on shallow / single-commit checkouts.
- security.yml: document the universal committed snapshot vs per-Python
  scheduled compile distinction.
- tests: add a regression test that a symlink alongside benign members is
  rejected with no partial extraction to disk.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: PascalThuet

.github/scripts/check_security_requirements.py

@@ -18,6 +18,11 @@ def _dependency_diff_refs() -> tuple[str, str]:
     head_ref = os.environ.get("DEPENDENCY_DIFF_HEAD", "").strip() or "HEAD"
     if base_ref and not set(base_ref) <= {"0"}:
         return base_ref, head_ref
+    # Fallback when no usable base is supplied (push with an all-zero
+    # ``github.event.before``, manual dispatch, etc.). ``HEAD^`` fails on a
+    # shallow checkout or a single-commit repo; that ``git diff`` error is
+    # caught by the caller and deliberately treated as "inputs changed" so the
+    # audit runs anyway — failing safe (audit) rather than skipping silently.
     return "HEAD^", "HEAD"
 
 

.github/workflows/security.yml

@@ -34,6 +34,13 @@ jobs:
         with:
           python-version: ${{ matrix.python-version }}
 
+      # The committed .github/security-audit-requirements.txt is generated with
+      # --universal (resolves across all interpreters/platforms) and is what
+      # push/PR runs audit. The scheduled job instead compiles per matrix
+      # entry with --python-version so it can surface advisories in wheels that
+      # only resolve on a specific interpreter (e.g. 3.11-only) — coverage the
+      # universal file may not exercise. This broadening is intentional; PR runs
+      # trade that depth for determinism against the committed snapshot.
       - name: Compile scheduled audit requirements
         if: ${{ github.event_name == 'schedule' }}
         run: |

src/specify_cli/__init__.py

@@ -2114,10 +2114,15 @@ def preset_add(
                         zip_path.write_bytes(
                             read_response_limited(
                                 response,
+                                error_type=PresetError,
                                 label=f"preset {from_url}",
                             )
                         )
-                except (urllib.error.URLError, ValueError) as e:
+                # The URL scheme is validated above, so the only failures here
+                # are network errors and an oversized body (raised as PresetError
+                # via error_type). Catching those specifically lets unrelated
+                # ValueErrors surface instead of masquerading as download errors.
+                except (urllib.error.URLError, PresetError) as e:
                     console.print(f"[red]Error:[/red] Failed to download: {e}")
                     raise typer.Exit(1)
 
@@ -3047,13 +3052,18 @@ def extension_add(
                     ) as response:
                         zip_data = read_response_limited(
                             response,
+                            error_type=ExtensionError,
                             label=f"extension {from_url}",
                         )
                     zip_path.write_bytes(zip_data)
 
                     # Install from downloaded ZIP
                     manifest = manager.install_from_zip(zip_path, speckit_version, priority=priority)
-                except (urllib.error.URLError, ValueError) as e:
+                # ExtensionError covers an oversized body (via error_type) and the
+                # ValidationError/ExtensionError raised by install_from_zip; URL
+                # scheme is validated above. Catching these instead of a blanket
+                # ValueError lets unrelated ValueErrors surface as real errors.
+                except (urllib.error.URLError, ExtensionError) as e:
                     console.print(f"[red]Error:[/red] Failed to download from {from_url}: {e}")
                     raise typer.Exit(1)
                 finally:

src/specify_cli/_download_security.py

@@ -26,6 +26,12 @@ def is_https_or_localhost_http(url: str) -> bool:
 
     Shared redirect-safety predicate used by the GitHub and auth HTTP redirect
     handlers so the rule (and any future tightening of it) lives in one place.
+
+    The loopback allowance is a deliberate *exact-string* match on
+    ``localhost`` / ``127.0.0.1`` / ``::1``, not an IP-range check: other
+    loopback addresses (e.g. ``127.0.0.2``) are intentionally not covered.
+    ``urlparse`` already lower-cases the hostname, so the comparison is
+    case-insensitive.
     """
     parsed = urlparse(url)
     is_localhost = parsed.hostname in ("localhost", "127.0.0.1", "::1")
@@ -53,6 +59,12 @@ def read_response_limited(
     return fewer even when more data is pending (e.g. chunked transfer encoding),
     so a single ``read(max_bytes + 1)`` cannot enforce the bound on its own. Read
     in a loop until EOF or until one byte past the limit has been accumulated.
+
+    *max_bytes* is keyword-only. It defaults to the module-wide
+    ``MAX_DOWNLOAD_BYTES`` (50 MiB) ceiling for archive/payload downloads;
+    callers with a tighter budget (e.g. small JSON responses) should pass an
+    explicit value so the intended bound is pinned at the call site rather than
+    tracking changes to the shared default.
     """
     chunks: list[bytes] = []
     total = 0
@@ -111,6 +123,10 @@ def _safe_zip_name(name: str, *, error_type: type[ErrorT]) -> str:
     normalized = name.replace("\\", "/")
     path = PurePosixPath(normalized)
     raw_parts = normalized.split("/")
+    # Strip a single trailing empty segment, i.e. the one-slash directory
+    # marker that legitimate ZIPs use ("mydir/", "mydir/subdir/"). Anything
+    # else that produces an empty segment — consecutive slashes ("a//b") or a
+    # second trailing slash — is left in place and rejected below as malformed.
     if raw_parts and raw_parts[-1] == "":
         raw_parts = raw_parts[:-1]
     has_windows_drive = re.match(r"^[A-Za-z]:", normalized) is not None

src/specify_cli/_utils.py

@@ -24,8 +24,11 @@ def run_command(
 ) -> str | None:
     """Run a command without invoking a shell and optionally capture output.
 
-    ``shell`` remains accepted for public API compatibility, but shell
-    execution is intentionally unsupported.
+    The ``shell`` parameter is kept in the signature so existing keyword
+    callers (and the re-export from ``specify_cli``) don't raise ``TypeError``,
+    but only the default ``shell=False`` is honoured. ``shell=True`` is
+    rejected with ``ValueError`` rather than silently ignored, so the
+    unsupported mode fails loudly instead of running with a different meaning.
     """
     if shell:
         raise ValueError(

src/specify_cli/authentication/http.py

@@ -161,6 +161,9 @@ def _make_req(auth_headers: dict[str, str]) -> urllib.request.Request:
     # No entry worked (or none matched) — unauthenticated fallback
     req = _make_req({})
     if strict_redirects:
+        # No auth is attached on this path, so the handler's host list is empty:
+        # here it acts purely as the HTTPS-downgrade guard (its redirect_request
+        # rejects non-HTTPS, non-loopback targets), not as an auth-stripper.
         opener = urllib.request.build_opener(_StripAuthOnRedirect(()))
         return opener.open(req, timeout=timeout)
     return urllib.request.urlopen(req, timeout=timeout)  # noqa: S310

tests/test_download_security.py

@@ -177,6 +177,27 @@ def test_safe_extract_zip_rejects_symlinks(tmp_path):
         safe_extract_zip(zip_path, tmp_path / "out")
 
 
+def test_safe_extract_zip_rejects_symlink_without_partial_extraction(tmp_path):
+    # A symlink sitting next to benign members must be rejected before ANY
+    # file is written: validation runs over the whole member list first, so an
+    # unsafe member cannot leak a partially-extracted tree to disk.
+    zip_path = tmp_path / "mixed.zip"
+    link = zipfile.ZipInfo("evil-link")
+    link.external_attr = (stat.S_IFLNK | 0o777) << 16
+    with zipfile.ZipFile(zip_path, "w") as zf:
+        zf.writestr("safe/first.txt", "hello")
+        zf.writestr(link, "target")
+        zf.writestr("safe/second.txt", "world")
+
+    out_dir = tmp_path / "out"
+    with pytest.raises(ValueError, match="Unsafe symlink"):
+        safe_extract_zip(zip_path, out_dir)
+
+    # Nothing should have been written — not even the benign member that
+    # precedes the symlink in the archive.
+    assert not out_dir.exists() or not any(out_dir.rglob("*"))
+
+
 def test_safe_extract_zip_rejects_oversized_member(tmp_path):
     zip_path = tmp_path / "bad.zip"
     with zipfile.ZipFile(zip_path, "w") as zf: