fix: only allow basenames for embedded files

src/openjd/sessions/_embedded_files.py

@@ -66,6 +66,34 @@ def _convert_line_endings(data: str, end_of_line: Optional[str]) -> str:
     return data
 
 
+def _validate_embedded_filename(filename: str) -> None:
+    """Validate that an embedded file's ``filename`` is a single path component
+    (a basename) with no directory pathing, as required by the OpenJD
+    specification (the ``<Filename>`` type).
+
+    This rejects path-traversal vectors -- parent references (``..``), path
+    separators, absolute paths, and (on Windows) drive-letter or UNC prefixes --
+    so that the file is always materialized inside the session directory. The
+    check uses the host operating system's path rules: ``Path(...).name`` is
+    host-flavored, and this runtime always runs on the host that materializes
+    the file. The OS-agnostic, fail-closed check (which cannot know the target
+    fleet's OS) is performed earlier, at job-submission time, by openjd-model;
+    this is the last line of defense before the write.
+
+    Note: pathlib treats ``..`` as a valid single path component (its ``.name``
+    is ``".."``), so it must be rejected explicitly.
+
+    Raises:
+        ValueError: if ``filename`` is not a valid basename.
+    """
+    if not filename or filename in (os.curdir, os.pardir) or Path(filename).name != filename:
+        raise ValueError(
+            f"Embedded file filename {filename!r} must be a basename with no "
+            "directory path components (for example 'script.sh', not "
+            "'dir/script.sh', '../script.sh', or '/abs/script.sh')"
+        )
+
+
 def write_file_for_user(
     filename: Path,
     data: str,
@@ -81,14 +109,25 @@ def write_file_for_user(
     #  O_TRUNC - truncate the file. If we overwrite an existing file, then we
     #            need to clear its contents.
     #  O_BINARY - (Windows only) prevent automatic \n to \r\n conversion
+    #  O_NOFOLLOW - (POSIX only) refuse to open the final path component if it
+    #            is a symbolic link. This prevents a symlink planted at the
+    #            destination (e.g. by the queue-configured job user, which
+    #            shares the session directory) from redirecting this write to a
+    #            file outside the session directory. O_NOFOLLOW only guards the
+    #            final component, so it is paired with the basename validation
+    #            and containment check performed before the write.
     #  O_EXCL (intentionally not present) - fail if file exists
     #    - We exclude this 'cause we expect to be writing the same embedded file
     #      into the same location repeatedly with different contents as we run
     #      multiple Tasks in the same Session.
+    #    - O_NOFOLLOW is used instead of O_EXCL to block symlink redirection
+    #      while still allowing the file to be rewritten across Tasks.
     flags = os.O_WRONLY | os.O_CREAT | os.O_TRUNC
     # On Windows, use O_BINARY to prevent automatic line ending conversion
     # since we handle line endings explicitly via _convert_line_endings
     flags |= getattr(os, "O_BINARY", 0)
+    # O_NOFOLLOW is not defined on Windows; getattr(..., 0) makes this a no-op there.
+    flags |= getattr(os, "O_NOFOLLOW", 0)
     # mode:
     #  S_IRUSR - Read by owner
     #  S_IWUSR - Write by owner
@@ -197,13 +236,15 @@ def materialize(self, files: EmbeddedFilesListType, symtab: SymbolTable) -> None
             for record in records:
                 # Raises: OSError
                 self._materialize_file(record.filename, record.file, symtab)
-        except OSError as err:
-            raise RuntimeError(f"Could not write embedded file: {err}")
         except FormatStringError as err:
             # This should *never* happen. All format string contents are
             # checked when building the Job Template model. If we get here,
             # then something is broken with our model validation.
+            # Note: FormatStringError subclasses ValueError, so it must be
+            # caught before the general (OSError, ValueError) clause below.
             raise RuntimeError(f"Error resolving format string: {str(err)}")
+        except (OSError, ValueError) as err:
+            raise RuntimeError(f"Could not write embedded file: {err}")
 
     def _find_value_prefix(self, file: EmbeddedFileType) -> str:
         """Figure out what prefix to use when referencing the file in format strings.
@@ -245,7 +286,23 @@ def _get_symtab_entry(self, file: EmbeddedFileType) -> tuple[str, Path]:
             os.close(fd)
             filename = Path(fname)
         else:
+            # Validate that the caller-supplied filename is a basename with no
+            # directory pathing, as required by the OpenJD specification. This
+            # prevents path-traversal writes outside of the session directory
+            # via '..' components, path separators, or absolute/rooted paths.
+            # Raises: ValueError
+            _validate_embedded_filename(file.filename)
             filename = self._target_directory / file.filename
+            # Defense in depth: confirm that the fully-resolved path is still
+            # contained within the target directory. This also rejects the case
+            # where the destination is an existing symlink that resolves to a
+            # location outside of the session directory.
+            # Raises: ValueError, OSError
+            if not filename.resolve().is_relative_to(self._target_directory.resolve()):
+                raise ValueError(
+                    f"Embedded file filename {file.filename!r} resolves to a "
+                    "path outside of the session directory"
+                )
 
         return (f"{self._find_value_prefix(file)}.{file.name}", filename)
 

test/openjd/sessions_v0/test_embedded_files.py

@@ -22,7 +22,12 @@
 from openjd.model.v2023_09 import (
     EndOfLine as EndOfLine_2023_09,
 )
-from openjd.sessions._embedded_files import EmbeddedFiles, EmbeddedFilesScope
+from openjd.sessions._embedded_files import (
+    EmbeddedFiles,
+    EmbeddedFilesScope,
+    _validate_embedded_filename,
+    write_file_for_user,
+)
 from openjd.sessions._session_user import PosixSessionUser, WindowsSessionUser
 
 from .conftest import (
@@ -120,6 +125,115 @@ def test_generate_filename(
             assert os.path.exists(result_filename)
             assert result_filename.parent == tmp_path
 
+        def test_allows_dotfile_basename(self, tmp_path: Path) -> None:
+            # A leading-dot basename (e.g. ".bashrc") is a legitimate filename
+            # and must NOT be rejected by the traversal guard.
+
+            # GIVEN
+            test_obj = EmbeddedFiles(
+                logger=MagicMock(),
+                scope=EmbeddedFilesScope.STEP,
+                session_files_directory=tmp_path,
+            )
+            test_file = EmbeddedFileText_2023_09(
+                name="Foo",
+                type=EmbeddedFileTypes_2023_09.TEXT,
+                filename=".bashrc",
+                data=DataString_2023_09("some data"),
+            )
+
+            # WHEN
+            _symbol, result_filename = test_obj._get_symtab_entry(test_file)
+
+            # THEN
+            assert result_filename == tmp_path / ".bashrc"
+
+        @pytest.mark.skipif(not is_posix(), reason="symlink semantics are posix-specific")
+        def test_rejects_symlink_escape(self, tmp_path: Path) -> None:
+            # If the destination basename already exists as a symlink that
+            # resolves outside the session directory (e.g. planted by the
+            # queue-configured job user between Tasks), the containment check
+            # must reject it even though the filename itself is a valid basename.
+
+            # GIVEN
+            outside = tmp_path / "outside"
+            outside.mkdir()
+            session_dir = tmp_path / "session"
+            session_dir.mkdir()
+            target = outside / "secret.txt"
+            target.write_text("original")
+            (session_dir / "foo.txt").symlink_to(target)
+            test_obj = EmbeddedFiles(
+                logger=MagicMock(),
+                scope=EmbeddedFilesScope.STEP,
+                session_files_directory=session_dir,
+            )
+            test_file = EmbeddedFileText_2023_09(
+                name="Foo",
+                type=EmbeddedFileTypes_2023_09.TEXT,
+                filename="foo.txt",
+                data=DataString_2023_09("some data"),
+            )
+
+            # WHEN / THEN
+            with pytest.raises(ValueError):
+                test_obj._get_symtab_entry(test_file)
+
+        @pytest.mark.skipif(not is_posix(), reason="symlink semantics are posix-specific")
+        def test_materialize_surfaces_rejection_as_runtimeerror(self, tmp_path: Path) -> None:
+            # materialize() must convert the embedded-file ValueError into a
+            # RuntimeError (the contract the script runner catches to fail the
+            # action). Triggered via a model-valid basename ("foo.txt") that is a
+            # symlink escaping the session dir, so it exercises the sessions guard
+            # independently of the openjd-model version, and must not write the
+            # symlink target outside the session directory.
+
+            # GIVEN
+            outside = tmp_path / "outside"
+            outside.mkdir()
+            session_dir = tmp_path / "session"
+            session_dir.mkdir()
+            target = outside / "secret.txt"
+            target.write_text("original")
+            (session_dir / "foo.txt").symlink_to(target)
+            test_obj = EmbeddedFiles(
+                logger=MagicMock(),
+                scope=EmbeddedFilesScope.STEP,
+                session_files_directory=session_dir,
+            )
+            test_file = EmbeddedFileText_2023_09(
+                name="Foo",
+                type=EmbeddedFileTypes_2023_09.TEXT,
+                filename="foo.txt",
+                data=DataString_2023_09("payload"),
+            )
+
+            # WHEN / THEN
+            with pytest.raises(RuntimeError):
+                test_obj.materialize([test_file], SymbolTable())
+            assert target.read_text() == "original"
+
+    @pytest.mark.skipif(not is_posix(), reason="O_NOFOLLOW is posix-specific")
+    class TestWriteFileForUserPosix:
+        """Tests that write_file_for_user() refuses to follow symlinks."""
+
+        def test_refuses_to_follow_symlink(self, tmp_path: Path) -> None:
+            # write_file_for_user must not follow a symlink at the destination
+            # (O_NOFOLLOW), so it cannot be tricked into truncating/overwriting
+            # a file outside the intended location.
+
+            # GIVEN
+            target = tmp_path / "target.txt"
+            target.write_text("original-contents")
+            link = tmp_path / "link.txt"
+            link.symlink_to(target)
+
+            # WHEN / THEN
+            with pytest.raises(OSError):
+                write_file_for_user(link, "overwritten", user=None)
+            # AND the symlink target is untouched
+            assert target.read_text() == "original-contents"
+
     @pytest.mark.skipif(not is_posix(), reason="posix-specific test")
     class TestMaterializeFilePosix:
         """Tests for EmbeddedFiles._materialize_file() on posix systems.
@@ -777,3 +891,59 @@ class Datum:
                 with open(filename, "r") as file:
                     result_contents = file.read()
                 assert result_contents == expected_file_data, "File contents are as expected"
+
+
+class TestValidateEmbeddedFilename:
+    """Unit tests for the _validate_embedded_filename() basename guard.
+
+    The guard is host-flavored, so backslash separators and drive specifiers are
+    only rejected on Windows (a backslash is a legal filename character on
+    POSIX). The OS-agnostic rejection happens earlier, in openjd-model.
+    """
+
+    @pytest.mark.parametrize(
+        "filename",
+        [
+            pytest.param("foo.txt", id="simple"),
+            pytest.param(".bashrc", id="dotfile"),
+            pytest.param("a_b-c.1", id="punctuation"),
+            pytest.param("café.txt", id="non-ascii"),
+            pytest.param("foo.bar.baz", id="multi-dot"),
+        ],
+    )
+    def test_accepts_valid_basenames(self, filename: str) -> None:
+        # WHEN / THEN (must not raise)
+        _validate_embedded_filename(filename)
+
+    @pytest.mark.parametrize(
+        "filename",
+        [
+            pytest.param("", id="empty"),
+            pytest.param(".", id="dot"),
+            pytest.param("..", id="parent"),
+            pytest.param("../x", id="parent-rel"),
+            pytest.param("a/b", id="sep"),
+            pytest.param("foo/", id="trailing-sep"),
+            pytest.param("/abs", id="absolute"),
+        ],
+    )
+    def test_rejects_non_basenames(self, filename: str) -> None:
+        # WHEN / THEN
+        with pytest.raises(ValueError):
+            _validate_embedded_filename(filename)
+
+    @pytest.mark.skipif(not is_windows(), reason="backslash/drive are separators only on Windows")
+    @pytest.mark.parametrize(
+        "filename",
+        [
+            pytest.param("..\\x", id="windows-parent"),
+            pytest.param("a\\b", id="windows-sep"),
+            pytest.param("C:x", id="drive-relative"),
+            pytest.param("C:\\x", id="drive-absolute"),
+            pytest.param("\\\\srv\\share", id="unc"),
+        ],
+    )
+    def test_rejects_windows_non_basenames(self, filename: str) -> None:
+        # WHEN / THEN
+        with pytest.raises(ValueError):
+            _validate_embedded_filename(filename)