From f70f306e1a0fcdd309e851da478366c2bb5ab107 Mon Sep 17 00:00:00 2001 From: Morgan Epp <60796713+epmog@users.noreply.github.com> Date: Fri, 3 Jul 2026 10:31:02 -0500 Subject: [PATCH] fix: only allow basenames for embedded files Signed-off-by: Morgan Epp <60796713+epmog@users.noreply.github.com> --- src/openjd/sessions/_embedded_files.py | 61 ++++++- .../openjd/sessions_v0/test_embedded_files.py | 172 +++++++++++++++++- 2 files changed, 230 insertions(+), 3 deletions(-) diff --git a/src/openjd/sessions/_embedded_files.py b/src/openjd/sessions/_embedded_files.py index ca099579..b5444d9a 100644 --- a/src/openjd/sessions/_embedded_files.py +++ b/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 ```` 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) diff --git a/test/openjd/sessions_v0/test_embedded_files.py b/test/openjd/sessions_v0/test_embedded_files.py index 9d70c878..09b671b8 100644 --- a/test/openjd/sessions_v0/test_embedded_files.py +++ b/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)