HDXDSYS-3018 Improve resource download handling

pyproject.toml

@@ -39,8 +39,8 @@ dependencies = [
     "ckanapi>=4.11",
     "defopt>=7.0.0",
     "email_validator",
-    "hdx-python-country>=4.2.4",
-    "hdx-python-utilities>=4.2.3",
+    "hdx-python-country>=4.2.5",
+    "hdx-python-utilities>=4.2.4",
     "makefun",
     "requests",
 ]

src/hdx/data/resource.py

@@ -4,13 +4,15 @@
 import logging
 from collections.abc import Iterator, Sequence
 from datetime import datetime
-from pathlib import Path
+from pathlib import Path, PureWindowsPath
 from typing import Any, Optional
+from urllib.parse import urlsplit
 
 from hdx.utilities.dateparse import now_utc, now_utc_notz, parse_date
 from hdx.utilities.downloader import Download
 from hdx.utilities.file_hashing import get_size_and_hash
 from hdx.utilities.retriever import Retrieve
+from hdx.utilities.session import get_session
 from hdx.utilities.uuid import is_valid_uuid
 
 import hdx.api.utilities.url_utils
@@ -755,6 +757,22 @@ def search_in_hdx(
             logger.debug(result)
         return resources
 
+    @staticmethod
+    def _get_safe_filename(filename: str) -> str:
+        """Strip any directory components (/ or \\) from a filename built from
+        resource metadata so downloads cannot be written outside the folder
+
+        Args:
+            filename: Filename to make safe
+
+        Returns:
+            Filename without directory components
+        """
+        safe_filename = PureWindowsPath(filename).name
+        if safe_filename in ("", ".", ".."):
+            raise HDXError(f"Invalid filename {filename} for download!")
+        return safe_filename
+
     def _download_file(
         self,
         url: str,
@@ -773,7 +791,17 @@ def _download_file(
         Returns:
             Path to downloaded file
         """
-        session = hdx.api.utilities.url_utils.get_ckan_ready_session(self.configuration)
+        # Only send the HDX API key to the HDX site, not to external resource URLs
+        hdx_site_url = self.configuration.get_hdx_site_url()
+        if urlsplit(url)[:2] == urlsplit(hdx_site_url)[:2]:
+            session = hdx.api.utilities.url_utils.get_ckan_ready_session(
+                self.configuration
+            )
+        else:
+            session = get_session(
+                full_agent=self.configuration.get_user_agent(), use_env=False
+            )
+        filename = self._get_safe_filename(filename)
         with Download(session=session) as downloader:
             if retriever:
                 downloader = retriever.clone(downloader)

tests/hdx/data/test_resource.py

@@ -1225,6 +1225,44 @@ def test_download(self, configuration, read):
         with pytest.raises(DownloadError):
             resource2.download()
 
+    def test_download_security(self, configuration, read, monkeypatch):
+        downloads = []
+
+        def download_file(self, url, folder=None, filename=None, path=None, **kwargs):
+            downloads.append((self.session.headers.get("Authorization"), filename))
+            return Path(folder or "tmp") / filename
+
+        monkeypatch.setattr(Download, "download_file", download_file)
+        Configuration.read().set_api_key("12345")
+        resource = Resource.read_from_hdx("74b74ae1-df0c-4716-829f-4f939a046811")
+        resource.download()
+        site_url = resource.configuration.get_hdx_site_url()
+        resource["url"] = f"{site_url}/dataset/x/resource/y/download/a.csv"
+        resource.download()
+        resource["url"] = site_url.replace("https://", "http://")
+        resource.download()
+        assert downloads == [
+            (None, "MyResource1.csv"),
+            ("12345", "MyResource1.csv"),
+            (None, "MyResource1.csv"),
+        ]
+
+        downloads.clear()
+        resource["name"] = "/home/user/.bashrc"
+        resource.download()
+        resource["name"] = "..\\..\\evil"
+        resource.download_datastore()
+        resource["name"] = "evil"
+        resource["format"] = "csv/../../x"
+        resource.download()
+        assert [filename for _, filename in downloads] == [
+            ".bashrc.csv",
+            "evil.csv",
+            "x",
+        ]
+        with pytest.raises(HDXError):
+            Resource._get_safe_filename("a/..")
+
     def test_download_datastore(self, configuration, read, monkeypatch):
         downloads = []