janhoy commented on code in PR #113:
URL: https://github.com/apache/solr-orbit/pull/113#discussion_r4287102646


##########
solrorbit/builder/solr_provisioner.py:
##########
@@ -136,14 +138,29 @@ def download(self, version: str) -> str:
         for base_url in (_DOWNLOADS_URL, _ARCHIVE_9_URL, _ARCHIVE_PRE9_URL):
             url = f"{base_url}/{version}/{tarball}"
             logger.info("Downloading Solr %s from %s", version, url)
+
+            temp_path = None
+
             try:
-                urllib.request.urlretrieve(url, dest)
+                fd, temp_path = tempfile.mkstemp(
+                    dir=self.cache_dir,
+                    prefix=f"{tarball}.",
+                    suffix=".tmp"
+                )
+                os.close(fd)
+                try:
+                    urllib.request.urlretrieve(url, temp_path)
+                except Exception as exc:
+                    logger.warning("Download from %s failed: %s", url, exc)
+                    continue
+                os.replace(temp_path, dest)

Review Comment:
   `tempfile.mkstemp()` creates its file with mode `0600`, whereas the old 
`urlretrieve(url, dest)` wrote through a plain `open(dest, 'wb')` and so got 
`0o666 & ~umask` — `0644` under the usual `umask 022`. Because `os.replace()` 
preserves the source file's mode, this change silently makes every cached 
tarball owner-only.
   
   That's harmless for a per-user `~/.solr-orbit/cache`, but it breaks a cache 
directory shared between users, or one written by a CI job and later read by a 
different account.
   
   An explicit chmod before publishing preserves the previous behaviour:
   
   ```python
   umask = os.umask(0)
   os.umask(umask)
   os.chmod(temp_path, 0o666 & ~umask)
   os.replace(temp_path, dest)
   ```
   
   `0o666 & ~umask` is what `open()` would have applied itself. Reading the 
umask requires that set-and-restore dance because there is no getter, and it is 
not thread-safe, so if you would rather not touch the process umask then a 
plain `os.chmod(temp_path, 0o644)` is a reasonable simpler alternative.
   
   🤖 Assisted by Claude Code



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to