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) <noreply@anthropic.com>
2026-04-25 10:48:44 +00:00
parent 8d6639bc4c
commit 4bc054f315
4 changed files with 254 additions and 1 deletions
@@ -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"]