From 9e23b81535e769946610c82b19d12e5922abcaf0 Mon Sep 17 00:00:00 2001
From: Gahyun Suh <132245153+gahyusuh@users.noreply.github.com>
Date: Mon, 25 Mar 2024 15:45:17 -0500
Subject: [PATCH] feat(job_attachment): reject files on non-Windows systems
 that do not support O_NOFOLLOW (#242)

Signed-off-by: Gahyun Suh <132245153+gahyusuh@users.noreply.github.com>
---
 src/deadline/job_attachments/upload.py            | 14 ++++++++++++--
 test/unit/deadline_job_attachments/test_upload.py |  5 ++++-
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/src/deadline/job_attachments/upload.py b/src/deadline/job_attachments/upload.py
index d0d75053..42f4376f 100644
--- a/src/deadline/job_attachments/upload.py
+++ b/src/deadline/job_attachments/upload.py
@@ -450,10 +450,20 @@ def _open_non_symlink_file_binary(
             # Make sure the file isn’t following a symlink to a different path.
             if hasattr(os, "O_NOFOLLOW"):
                 open_flags |= os.O_NOFOLLOW
+            elif sys.platform != "win32" and not os.path.islink(path):
+                # We are on a non-Windows system that does not support O_NOFOLLOW. When we encounter
+                # symbolic link, we cannot guarantee security here, so log a warning and reject the file.
+                logger.warning(
+                    f"Job Attachments does not support files referenced by symbolic links on this system ({sys.platform}). "
+                    "Please refrain from using symbolic links in Job Attachment asset roots and use real files instead. "
+                    f"The following file will be skipped: {path}."
+                )
+                yield None
 
             fd = os.open(path, open_flags)
             if sys.platform == "win32":
-                # On Windows, check the file handle with GetFinalPathNameByHandle
+                # Windows does not support O_NOFOLLOW. So, check the file handle with GetFinalPathNameByHandle
+                # to verify it is actually pointing to the path that we verified to be safe to open.
                 if not self._is_path_win32_final_path_of_file_descriptor(path, fd):
                     # ELOOP is the error code that open with NOFOLLOW will return
                     # if the path is a symlink.  We raise the same error here for
@@ -466,7 +476,7 @@ def _open_non_symlink_file_binary(
             with os.fdopen(fd, "rb", closefd=False) as file_obj:
                 yield file_obj
         except OSError as e:
-            logger.warning(f"Failed to open file {path}: {e}")
+            logger.warning(f"Failed to open file. The following file will be skipped: {path}: {e}")
             yield None
         finally:
             if fd is not None:
diff --git a/test/unit/deadline_job_attachments/test_upload.py b/test/unit/deadline_job_attachments/test_upload.py
index b83b3a2a..89d48836 100644
--- a/test/unit/deadline_job_attachments/test_upload.py
+++ b/test/unit/deadline_job_attachments/test_upload.py
@@ -2420,7 +2420,10 @@ def test_open_non_symlink_file_binary_posix_fail(self, tmp_path: Path, caplog):
         with a3_asset_uploader._open_non_symlink_file_binary(str(symlink_path)) as file_obj:
             # THEN
             assert file_obj is None
-            assert f"Failed to open file {symlink_path}" in caplog.text
+            assert (
+                f"Failed to open file. The following file will be skipped: {symlink_path}"
+                in caplog.text
+            )
             if hasattr(os, "O_NOFOLLOW") is False:
                 # Windows or other platforms that don't support O_NOFOLLOW
                 assert "Mismatch between path and its final path" in caplog.text