From 4bc054f3155bf43199884ac4323605dcf298eaf6 Mon Sep 17 00:00:00 2001 From: Chaim Date: Sat, 25 Apr 2026 10:48:44 +0000 Subject: [PATCH] Add: download_decisive_appraisal_pdf MCP tool The PDF host (free-justice.openapi.gov.il) requires the same x-client-id header as the search API, so a normal browser click on the URL fails. This tool carries the auth header automatically and saves the PDF to a configurable local directory. Safety properties: - SSRF guard: only download from free-justice.openapi.gov.il and pub-justice.openapi.gov.il. - Path-traversal guard: filename is reduced to its basename; arbitrary paths are stripped. - Content-type guard: rejects 200-OK responses whose body is not a real PDF (gateway sometimes returns JSON-error 200s). - Atomic write via .tmp + rename so partial downloads never replace the cached copy. - Caches by destination path; re-downloads are no-ops unless overwrite=True. DECISIVE_APPRAISER_DOWNLOAD_DIR env var configures the output dir (default: ./downloads/decisive_appraisals). 6 new unit tests cover the happy path, caching, and all four guards. End-to-end live test confirmed a 1.4MB real PDF lands on disk with valid `%PDF-1.7` header. Co-Authored-By: Claude Opus 4.7 (1M context) --- nadlan_mcp/config.py | 12 +++ nadlan_mcp/fastmcp_server.py | 49 +++++++++++ nadlan_mcp/govil/client.py | 109 +++++++++++++++++++++++- tests/test_decisive_appraiser_client.py | 85 ++++++++++++++++++ 4 files changed, 254 insertions(+), 1 deletion(-) diff --git a/nadlan_mcp/config.py b/nadlan_mcp/config.py index 3d45391..a437216 100644 --- a/nadlan_mcp/config.py +++ b/nadlan_mcp/config.py @@ -176,6 +176,18 @@ class GovmapConfig: os.getenv("TOOL_SEARCH_DECISIVE_APPRAISALS_ENABLED", "true").lower() == "true" ) ) + tool_download_decisive_appraisal_pdf_enabled: bool = field( + default_factory=lambda: ( + os.getenv("TOOL_DOWNLOAD_DECISIVE_APPRAISAL_PDF_ENABLED", "true").lower() == "true" + ) + ) + + # Decisive Appraiser download settings + decisive_appraiser_download_dir: str = field( + default_factory=lambda: os.getenv( + "DECISIVE_APPRAISER_DOWNLOAD_DIR", "./downloads/decisive_appraisals" + ) + ) def __post_init__(self): """Validate configuration after initialization.""" diff --git a/nadlan_mcp/fastmcp_server.py b/nadlan_mcp/fastmcp_server.py index 53d3345..627399c 100644 --- a/nadlan_mcp/fastmcp_server.py +++ b/nadlan_mcp/fastmcp_server.py @@ -1420,6 +1420,55 @@ def search_decisive_appraisals( return f"Error searching decisive appraisals: {str(e)}" +@conditional_tool("tool_download_decisive_appraisal_pdf_enabled") +def download_decisive_appraisal_pdf( + url: str, + filename: Optional[str] = None, + output_dir: Optional[str] = None, + overwrite: bool = False, +) -> str: + """Download the PDF of a decisive-appraiser (שמאי מכריע) decision. + + The PDF host requires the same `x-client-id` header as the search API, + so a normal browser click on the URL will fail. This tool carries the + auth header automatically. + + Args: + url: Direct PDF URL, as returned in `decisions[].pdf_url` from + search_decisive_appraisals. Must be on free-justice.openapi.gov.il. + filename: Optional filename for the saved PDF. Path components are + stripped; only the basename is used. If omitted, a hash-based + filename is generated from the URL. + output_dir: Optional override for the output directory. Defaults to + the configured `DECISIVE_APPRAISER_DOWNLOAD_DIR` + (./downloads/decisive_appraisals). + overwrite: If False (default), returns the cached file when it + already exists at the destination. + + Returns: + JSON string with `path` (absolute local file path), `size_bytes`, + `from_cache` (true if file already existed), `content_type`. + """ + log_mcp_call( + "download_decisive_appraisal_pdf", + url=url, + filename=filename, + output_dir=output_dir, + overwrite=overwrite, + ) + try: + result = decisive_appraiser_client.download_pdf( + url=url, + output_dir=output_dir, + filename=filename, + overwrite=overwrite, + ) + return json.dumps(result, ensure_ascii=False, indent=None) + except Exception as e: + logger.error(f"Error in download_decisive_appraisal_pdf: {e}", exc_info=True) + return f"Error downloading appraisal PDF: {str(e)}" + + # Health check endpoint for HTTP deployments @mcp.custom_route("/health", methods=["GET"]) async def health_check(request): diff --git a/nadlan_mcp/govil/client.py b/nadlan_mcp/govil/client.py index 3bee89e..801173a 100644 --- a/nadlan_mcp/govil/client.py +++ b/nadlan_mcp/govil/client.py @@ -9,9 +9,14 @@ generic 500 "General Error". """ from datetime import datetime +import hashlib import logging +import os +from pathlib import Path +import re import time -from typing import Any, Dict, List, Optional +from typing import Any, Dict, List, Optional, Union +from urllib.parse import urlparse from curl_cffi import requests as cf_requests @@ -35,6 +40,11 @@ DEFAULT_REFERER = "https://www.gov.il/he/departments/dynamiccollectors/decisive_ # DevTools session. Not a secret, but required by the gateway. DEFAULT_CLIENT_ID = "149a5bad-edde-49a6-9fb9-188bd17d4788" +# PDFs are hosted on a sibling host (`free-justice.openapi.gov.il`) and +# also require the same x-client-id header. SSRF guard: refuse to download +# anything outside this host. +ALLOWED_PDF_HOSTS = ("free-justice.openapi.gov.il", "pub-justice.openapi.gov.il") + # Filters the upstream API understands. Anything else is silently dropped. _VALID_FILTER_KEYS = { "SearchText", @@ -261,3 +271,100 @@ class DecisiveAppraiserClient: results=collected[:max_results], total_results=total, ) + + def download_pdf( + self, + url: str, + output_dir: Optional[Union[str, Path]] = None, + filename: Optional[str] = None, + overwrite: bool = False, + ) -> Dict[str, Any]: + """ + Download a decisive-appraiser PDF to the local filesystem. + + The PDF host requires the same `x-client-id` header as the search + API; a normal browser click on the URL will fail. This method + carries the auth header automatically. + + Args: + url: Direct PDF URL, as returned in `decision.documents[].file_url`. + Must be on `free-justice.openapi.gov.il` or + `pub-justice.openapi.gov.il` (SSRF guard). + output_dir: Directory to save into. Defaults to the configured + `decisive_appraiser_download_dir`. + filename: Optional filename. If omitted, derived from a hash of + the URL plus `.pdf`. Path components in the filename are + stripped — only the basename is kept. + overwrite: If False (default), aborts when the destination file + already exists; returns its existing size unchanged. + + Returns: + Dict with `path` (str), `size_bytes` (int), `from_cache` (bool), + `content_type` (str). Raises ValueError on host/url issues and + curl_cffi.RequestsError on network failures. + """ + parsed = urlparse(url) + if parsed.scheme not in ("http", "https"): + raise ValueError(f"Refusing non-http(s) URL: {url}") + if parsed.hostname not in ALLOWED_PDF_HOSTS: + raise ValueError( + f"Refusing to download from untrusted host '{parsed.hostname}'. " + f"Allowed: {ALLOWED_PDF_HOSTS}" + ) + + # Resolve destination + if output_dir is None: + output_dir = getattr( + self.config, "decisive_appraiser_download_dir", "./downloads/decisive_appraisals" + ) + out_dir = Path(output_dir).expanduser().resolve() + out_dir.mkdir(parents=True, exist_ok=True) + + if filename: + # Strip any path components — only basename allowed. + safe_name = Path(filename).name + # Sanitise a bit: keep alnum, dot, hyphen, underscore, Hebrew. + safe_name = re.sub(r'[<>:"|?*\x00-\x1f]', "_", safe_name) + if not safe_name: + safe_name = None # type: ignore[assignment] + + if not filename: + digest = hashlib.sha256(url.encode("utf-8")).hexdigest()[:16] + safe_name = f"decisive_appraisal_{digest}.pdf" + + dest = out_dir / safe_name + if dest.exists() and not overwrite: + return { + "path": str(dest), + "size_bytes": dest.stat().st_size, + "from_cache": True, + "content_type": "application/pdf", + } + + self._rate_limit() + response = self._session.get( + url, + timeout=(self.config.connect_timeout, self.config.read_timeout * 4), + headers={"Accept": "application/pdf,*/*"}, + ) + if response.status_code != 200: + raise ValueError(f"PDF download returned {response.status_code}: {response.text[:200]}") + + content = response.content + if not content.startswith(b"%PDF"): + # Gateway sometimes returns JSON error with 200 — guard against it. + raise ValueError( + f"Response is not a PDF (first bytes: {content[:8]!r}, size={len(content)})" + ) + + # Atomic write: write to .tmp then rename. + tmp = dest.with_suffix(dest.suffix + ".tmp") + tmp.write_bytes(content) + os.replace(tmp, dest) + + return { + "path": str(dest), + "size_bytes": len(content), + "from_cache": False, + "content_type": response.headers.get("content-type", "application/pdf"), + } diff --git a/tests/test_decisive_appraiser_client.py b/tests/test_decisive_appraiser_client.py index 076ba44..957e387 100644 --- a/tests/test_decisive_appraiser_client.py +++ b/tests/test_decisive_appraiser_client.py @@ -222,3 +222,88 @@ class TestDecisiveAppraiserClient: assert "x-client-id" in headers assert headers["x-client-id"] == "149a5bad-edde-49a6-9fb9-188bd17d4788" assert headers["Origin"] == "https://www.gov.il" + + +class TestDownloadPdf: + @patch("nadlan_mcp.govil.client.cf_requests.Session") + def test_download_writes_pdf_to_disk(self, mock_session_class, tmp_path): + pdf_bytes = b"%PDF-1.7\n" + b"x" * 1000 + mock_response = Mock() + mock_response.status_code = 200 + mock_response.content = pdf_bytes + mock_response.headers = {"content-type": "application/pdf"} + mock_session = Mock() + mock_session.get.return_value = mock_response + mock_session_class.return_value = mock_session + + client = DecisiveAppraiserClient() + url = "https://free-justice.openapi.gov.il/free/moj/portal/rest/searchpredefinedapi/v1/SearchPredefinedApi/Documents/DecisiveAppraiser/abc123=" + result = client.download_pdf(url, output_dir=str(tmp_path), filename="decision.pdf") + + assert result["from_cache"] is False + assert result["size_bytes"] == len(pdf_bytes) + assert result["content_type"] == "application/pdf" + saved = tmp_path / "decision.pdf" + assert saved.exists() + assert saved.read_bytes() == pdf_bytes + + @patch("nadlan_mcp.govil.client.cf_requests.Session") + def test_download_caches_existing_file(self, mock_session_class, tmp_path): + # Pre-create the destination + existing = tmp_path / "decision.pdf" + existing.write_bytes(b"%PDF-1.7\nold") + + mock_session = Mock() + mock_session_class.return_value = mock_session + + client = DecisiveAppraiserClient() + url = "https://free-justice.openapi.gov.il/free/moj/portal/rest/searchpredefinedapi/v1/SearchPredefinedApi/Documents/DecisiveAppraiser/abc123=" + result = client.download_pdf(url, output_dir=str(tmp_path), filename="decision.pdf") + + assert result["from_cache"] is True + # Network call should NOT have happened. + mock_session.get.assert_not_called() + + def test_download_rejects_external_host(self, tmp_path): + client = DecisiveAppraiserClient() + with pytest.raises(ValueError, match="untrusted host"): + client.download_pdf("https://evil.com/foo.pdf", output_dir=str(tmp_path)) + + def test_download_rejects_non_https(self, tmp_path): + client = DecisiveAppraiserClient() + with pytest.raises(ValueError, match="non-http"): + client.download_pdf("file:///etc/passwd", output_dir=str(tmp_path)) + + @patch("nadlan_mcp.govil.client.cf_requests.Session") + def test_download_rejects_non_pdf_response(self, mock_session_class, tmp_path): + # Gateway sometimes returns 200 with a JSON error body — we must catch it. + mock_response = Mock() + mock_response.status_code = 200 + mock_response.content = b'{"error":"forbidden"}' + mock_response.headers = {"content-type": "application/json"} + mock_session = Mock() + mock_session.get.return_value = mock_response + mock_session_class.return_value = mock_session + + client = DecisiveAppraiserClient() + url = "https://free-justice.openapi.gov.il/free/moj/portal/rest/searchpredefinedapi/v1/SearchPredefinedApi/Documents/DecisiveAppraiser/abc=" + with pytest.raises(ValueError, match="not a PDF"): + client.download_pdf(url, output_dir=str(tmp_path)) + + @patch("nadlan_mcp.govil.client.cf_requests.Session") + def test_download_strips_path_components_from_filename(self, mock_session_class, tmp_path): + """Path-traversal guard: filename must not escape the output dir.""" + mock_response = Mock() + mock_response.status_code = 200 + mock_response.content = b"%PDF-1.7\n" + mock_response.headers = {"content-type": "application/pdf"} + mock_session = Mock() + mock_session.get.return_value = mock_response + mock_session_class.return_value = mock_session + + client = DecisiveAppraiserClient() + url = "https://free-justice.openapi.gov.il/free/moj/portal/rest/searchpredefinedapi/v1/SearchPredefinedApi/Documents/DecisiveAppraiser/abc=" + result = client.download_pdf(url, output_dir=str(tmp_path), filename="../../../escape.pdf") + # File should land inside tmp_path, not outside. + assert (tmp_path / "escape.pdf").exists() + assert str(tmp_path) in result["path"]