Repository: huggingface/huggingface_hub
Pull request #1999 (2024-01-24 14:52:59)

Fix download from private renamed repo
This PR fixes a bug when trying to download a regular file (non-LFS) from a private renamed repo. The problem came from the fact that:
1. renaming a repo induce a redirect
2. we were considering that any redirect means that we are downloading from a CDN
3. when we download from a CDN, we remove the `authorization` header since the URL is supposed to be signed
4. when making the actual GET call, the `authorization` header was not there + the url is not signed because it is still a relative redirect to huggingface.co

**=> this PR fixes this issue by keeping the `authorization` header when the redirected URL is from the same domain**


cc @LysandreJik @osanseviero who investigated the bug
cc @coyotte508 @XciD can you please review this PR as well? Just want to make sure I did not break anything (especially when downloading from a Space)

HuggingFaceDocBuilderDev: The docs for this PR live [here](https://moon-ci-docs.huggingface.co/docs/huggingface_hub/pr_1999). All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.
Wauplin: Thanks everyone for the quick review!
Wauplin: (failing tests are unrelated, closing this PR)

Patch:
diff --git a/src/huggingface_hub/file_download.py b/src/huggingface_hub/file_download.py
index d2c3d613..1baac40e 100644
--- a/src/huggingface_hub/file_download.py
+++ b/src/huggingface_hub/file_download.py
@@ -1300,11 +1300,14 @@ def hf_hub_download(
             # In case of a redirect, save an extra redirect on the request.get call,
             # and ensure we download the exact atomic version even if it changed
             # between the HEAD and the GET (unlikely, but hey).
-            # Useful for lfs blobs that are stored on a CDN.
+            #
+            # If url domain is different => we are downloading from a CDN => url is signed => don't send auth
+            # If url domain is the same => redirect due to repo rename AND downloading a regular file => keep auth
             if metadata.location != url:
                 url_to_download = metadata.location
-                # Remove authorization header when downloading a LFS blob
-                headers.pop("authorization", None)
+                if urlparse(url).netloc != urlparse(url_to_download).netloc:
+                    # Remove authorization header when downloading a LFS blob
+                    headers.pop("authorization", None)
         except (requests.exceptions.SSLError, requests.exceptions.ProxyError):
             # Actually raise for those subclasses of ConnectionError
             raise


Test patch:
diff --git a/tests/test_file_download.py b/tests/test_file_download.py
index 28211efd..9cfbd22f 100644
--- a/tests/test_file_download.py
+++ b/tests/test_file_download.py
@@ -164,6 +164,28 @@ class StagingDownloadTests(unittest.TestCase):
                     repo_id=repo_url.repo_id, filename=".gitattributes", token=OTHER_TOKEN, cache_dir=tmpdir
                 )
 
+    @use_tmp_repo()
+    def test_download_regular_file_from_private_renamed_repo(self, repo_url: RepoUrl) -> None:
+        """Regression test for #1999.
+
+        See https://github.com/huggingface/huggingface_hub/pull/1999.
+        """
+        repo_id_before = repo_url.repo_id
+        repo_id_after = repo_url.repo_id + "_renamed"
+
+        # Make private + rename + upload regular file
+        self._api.update_repo_visibility(repo_id_before, private=True)
+        self._api.upload_file(repo_id=repo_id_before, path_in_repo="file.txt", path_or_fileobj=b"content")
+        self._api.move_repo(repo_id_before, repo_id_after)
+
+        # Download from private renamed repo
+        path = self._api.hf_hub_download(repo_id_before, filename="file.txt")
+        with open(path) as f:
+            self.assertEqual(f.read(), "content")
+
+        # Move back (so that auto-cleanup works)
+        self._api.move_repo(repo_id_after, repo_id_before)
+
 
 @with_production_testing
 class CachedDownloadTests(unittest.TestCase):