fix(security): add path traversal and header injection defenses to files and conversations

Signed-off-by: Doug Edgar <dedgar@redhat.com>

Author: rhdedgar

src/llama_stack/core/conversations/conversations.py

@@ -10,6 +10,7 @@
 
 from pydantic import BaseModel, TypeAdapter
 
+from llama_stack.core.conversations.validation import CONVERSATION_ID_PATTERN
 from llama_stack.core.datatypes import AccessRule, StackConfig
 from llama_stack.core.storage.sqlstore.authorized_sqlstore import AuthorizedSqlStore
 from llama_stack.core.storage.sqlstore.sqlstore import sqlstore_impl
@@ -182,9 +183,13 @@ async def openai_delete_conversation(self, request: DeleteConversationRequest) -
         return ConversationDeletedResource(id=request.conversation_id)
 
     def _validate_conversation_id(self, conversation_id: str) -> None:
-        """Validate conversation ID format."""
-        if not conversation_id.startswith("conv_"):
-            raise InvalidParameterError("conversation_id", conversation_id, "Conversation ID must begin with 'conv_'.")
+        """Validate conversation ID format matches ``conv_`` + 48 hex chars."""
+        if not CONVERSATION_ID_PATTERN.fullmatch(conversation_id):
+            raise InvalidParameterError(
+                "conversation_id",
+                conversation_id,
+                "Conversation ID must match format 'conv_' followed by 48 hex characters.",
+            )
 
     def _get_or_generate_item_id(self, item: ConversationItem, item_dict: dict) -> str:
         """Get existing item ID or generate one if missing."""

src/llama_stack/core/conversations/validation.py

@@ -0,0 +1,9 @@
+# Copyright (c) Meta Platforms, Inc. and affiliates.
+# All rights reserved.
+#
+# This source code is licensed under the terms described in the LICENSE file in
+# the root directory of this source tree.
+
+import re
+
+CONVERSATION_ID_PATTERN = re.compile(r"^conv_[0-9a-f]{48}$")

src/llama_stack/providers/inline/agents/meta_reference/responses/openai_responses.py

@@ -12,6 +12,7 @@
 
 from pydantic import BaseModel, TypeAdapter
 
+from llama_stack.core.conversations.validation import CONVERSATION_ID_PATTERN
 from llama_stack.log import get_logger
 from llama_stack.providers.utils.responses.responses_store import (
     ResponsesStore,
@@ -617,8 +618,12 @@ async def create_openai_response(
                     "Provide only one of these parameters.",
                 )
 
-            if not conversation.startswith("conv_"):
-                raise InvalidParameterError("conversation", conversation, "Expected an ID that begins with 'conv_'.")
+            if not CONVERSATION_ID_PATTERN.fullmatch(conversation):
+                raise InvalidParameterError(
+                    "conversation",
+                    conversation,
+                    "Must match format 'conv_' followed by 48 hex characters.",
+                )
 
         if max_tool_calls is not None and max_tool_calls < 1:
             raise ValueError(f"Invalid {max_tool_calls=}; should be >= 1")

src/llama_stack/providers/inline/files/localfs/files.py

@@ -4,6 +4,22 @@
 # This source code is licensed under the terms described in the LICENSE file in
 # the root directory of this source tree.
 
+"""Local-filesystem files provider with defense-in-depth path security.
+
+Security boundaries
+-------------------
+* **Indirect references** -- Clients address files via opaque IDs
+  (``file-<1-64 hex chars>``), never raw paths.
+* **ID format validation** -- ``_validate_file_id`` rejects any ID that does
+  not match ``^file-[0-9a-f]{1,64}$``, blocking path separators, traversal
+  sequences, null bytes, and any non-hex characters.
+* **Path containment** -- ``_validate_path_containment`` resolves symlinks and
+  ``..`` components, then verifies the result stays inside ``storage_dir``.
+* **Filename sanitization** -- ``Content-Disposition`` filenames are stripped of
+  path separators, traversal sequences, null bytes, and non-ASCII-safe chars.
+"""
+
+import re
 import time
 import uuid
 from pathlib import Path
@@ -16,9 +32,11 @@
 from llama_stack.core.storage.sqlstore.authorized_sqlstore import AuthorizedSqlStore
 from llama_stack.core.storage.sqlstore.sqlstore import sqlstore_impl
 from llama_stack.log import get_logger
+from llama_stack.providers.utils.files.sanitize import sanitize_content_disposition_filename
 from llama_stack_api import (
     DeleteFileRequest,
     Files,
+    InvalidParameterError,
     ListFilesRequest,
     ListOpenAIFileResponse,
     OpenAIFileDeleteResponse,
@@ -67,16 +85,46 @@ async def initialize(self) -> None:
     async def shutdown(self) -> None:
         pass
 
+    _FILE_ID_PATTERN = re.compile(r"^file-[0-9a-f]{1,64}$")
+
     def _generate_file_id(self) -> str:
         """Generate a unique file ID for OpenAI API."""
         return generate_object_id("file", lambda: f"file-{uuid.uuid4().hex}")
 
+    def _validate_file_id(self, file_id: str) -> None:
+        """Validate that file_id contains only safe characters (hex digits) after the ``file-`` prefix."""
+        if not self._FILE_ID_PATTERN.fullmatch(file_id):
+            raise InvalidParameterError(
+                "file_id", file_id, "Must match format 'file-' followed by 1-64 hex characters."
+            )
+
+    def _validate_path_containment(self, file_path: Path) -> Path:
+        """Canonicalize *file_path* and verify it resides inside storage_dir.
+
+        Returns the resolved (absolute, symlink-free) path so callers operate
+        on the canonical location.  Raises ``InvalidParameterError`` when the
+        resolved path escapes the storage directory boundary.
+        """
+        resolved = file_path.resolve()
+        storage_dir = Path(self.config.storage_dir).resolve()
+        if not resolved.is_relative_to(storage_dir):
+            raise InvalidParameterError(
+                "file_path",
+                file_path.name,
+                "File path does not resolve to a valid storage location.",
+            )
+        return resolved
+
     def _get_file_path(self, file_id: str) -> Path:
         """Get the filesystem path for a file ID."""
-        return Path(self.config.storage_dir) / file_id
+        self._validate_file_id(file_id)
+        path = Path(self.config.storage_dir) / file_id
+        return self._validate_path_containment(path)
 
     async def _lookup_file_id(self, file_id: str, action: Action = Action.READ) -> tuple[OpenAIFileObject, Path]:
         """Look up a OpenAIFileObject and filesystem path from its ID."""
+        self._validate_file_id(file_id)
+
         if not self.sql_store:
             raise RuntimeError("Files provider not initialized")
 
@@ -85,6 +133,7 @@ async def _lookup_file_id(self, file_id: str, action: Action = Action.READ) -> t
             raise OpenAIFileObjectNotFoundError(file_id)
 
         file_path = Path(row.pop("file_path"))
+        file_path = self._validate_path_containment(file_path)
         return OpenAIFileObject(**row), file_path
 
     # OpenAI Files API Implementation
@@ -224,5 +273,7 @@ async def openai_retrieve_file_content(self, request: RetrieveFileContentRequest
         return Response(
             content=file_path.read_bytes(),
             media_type="application/octet-stream",
-            headers={"Content-Disposition": f'attachment; filename="{file_obj.filename}"'},
+            headers={
+                "Content-Disposition": f'attachment; filename="{sanitize_content_disposition_filename(file_obj.filename)}"'
+            },
         )

src/llama_stack/providers/remote/files/openai/files.py

@@ -13,6 +13,7 @@
 from llama_stack.core.datatypes import AccessRule
 from llama_stack.core.storage.sqlstore.authorized_sqlstore import AuthorizedSqlStore
 from llama_stack.core.storage.sqlstore.sqlstore import sqlstore_impl
+from llama_stack.providers.utils.files.sanitize import sanitize_content_disposition_filename
 from llama_stack_api import (
     DeleteFileRequest,
     ExpiresAfter,
@@ -249,5 +250,7 @@ async def openai_retrieve_file_content(self, request: RetrieveFileContentRequest
         return Response(
             content=file_content,
             media_type="application/octet-stream",
-            headers={"Content-Disposition": f'attachment; filename="{row["filename"]}"'},
+            headers={
+                "Content-Disposition": f'attachment; filename="{sanitize_content_disposition_filename(row["filename"])}"'
+            },
         )

src/llama_stack/providers/remote/files/s3/files.py

@@ -20,6 +20,7 @@
 from llama_stack.core.id_generation import generate_object_id
 from llama_stack.core.storage.sqlstore.authorized_sqlstore import AuthorizedSqlStore
 from llama_stack.core.storage.sqlstore.sqlstore import sqlstore_impl
+from llama_stack.providers.utils.files.sanitize import sanitize_content_disposition_filename
 from llama_stack_api import (
     ExpiresAfter,
     Files,
@@ -331,5 +332,7 @@ async def openai_retrieve_file_content(self, request: RetrieveFileContentRequest
         return Response(
             content=content,
             media_type="application/octet-stream",
-            headers={"Content-Disposition": f'attachment; filename="{row["filename"]}"'},
+            headers={
+                "Content-Disposition": f'attachment; filename="{sanitize_content_disposition_filename(row["filename"])}"'
+            },
         )

src/llama_stack/providers/utils/files/sanitize.py

@@ -0,0 +1,27 @@
+# Copyright (c) Meta Platforms, Inc. and affiliates.
+# All rights reserved.
+#
+# This source code is licensed under the terms described in the LICENSE file in
+# the root directory of this source tree.
+
+import re
+
+_SAFE_FILENAME_PATTERN = re.compile(r"[^a-zA-Z0-9._-]")
+
+
+def sanitize_content_disposition_filename(filename: str) -> str:
+    """Sanitize *filename* for safe inclusion in a ``Content-Disposition`` header.
+
+    The function strips null bytes and path separators, collapses ``..``
+    sequences, prevents hidden-file names (leading dot), and replaces any
+    remaining characters outside an ASCII alphanumeric allowlist.  Returns
+    ``"download"`` when the result would otherwise be empty.
+    """
+    filename = filename.replace("\x00", "")
+    filename = filename.replace('"', "_")  # prevent Content-Disposition header injection
+    filename = filename.replace("/", "_").replace("\\", "_")
+    filename = filename.replace("..", "_")
+    if filename.startswith("."):
+        filename = "_" + filename[1:]
+    filename = _SAFE_FILENAME_PATTERN.sub("_", filename)
+    return filename or "download"

tests/integration/responses/test_conversation_responses.py

@@ -94,12 +94,12 @@ def test_conversation_error_handling(self, openai_client, text_model_id):
             )
         assert any(word in str(exc_info.value).lower() for word in ["conv", "invalid", "bad"])
 
-        # Nonexistent conversation ID
+        # Nonexistent conversation ID (must be valid format to reach the DB lookup)
         with pytest.raises(Exception) as exc_info:
             openai_client.responses.create(
                 model=text_model_id,
                 input=[{"role": "user", "content": "Hello"}],
-                conversation="conv_nonexistent123",
+                conversation="conv_" + "0" * 48,
             )
         assert any(word in str(exc_info.value).lower() for word in ["not found", "404"])
 

tests/integration/responses/test_responses_errors.py

@@ -250,7 +250,7 @@ def test_nonexistent_conversation_raises_not_found_error(self, openai_client, te
         Test that referencing a nonexistent conversation returns 404 and triggers
         openai.NotFoundError in the SDK.
         """
-        conversation_id = "conv_nonexistent123456"
+        conversation_id = "conv_" + "0" * 48
         with pytest.raises(NotFoundError) as exc_info:
             openai_client.responses.create(
                 model=text_model_id,

tests/unit/conversations/test_conversations.py

@@ -108,31 +108,32 @@ async def test_conversation_items(service):
 
 
 async def test_invalid_conversation_id(service):
-    with pytest.raises(InvalidParameterError, match="Conversation ID must begin with 'conv_'"):
+    with pytest.raises(InvalidParameterError, match="Conversation ID must match format"):
         await service.get_conversation(GetConversationRequest(conversation_id="invalid_id"))
 
 
 async def test_invalid_conversation_id_on_retrieve(service):
-    with pytest.raises(InvalidParameterError, match="Conversation ID must begin with 'conv_'"):
+    with pytest.raises(InvalidParameterError, match="Conversation ID must match format"):
         await service.retrieve(RetrieveItemRequest(conversation_id="bad_id", item_id="item_123"))
 
 
 async def test_invalid_conversation_id_on_update(service):
     from llama_stack_api.conversations import UpdateConversationRequest
 
-    with pytest.raises(InvalidParameterError, match="Conversation ID must begin with 'conv_'"):
+    with pytest.raises(InvalidParameterError, match="Conversation ID must match format"):
         await service.update_conversation("bad_id", UpdateConversationRequest(metadata={}))
 
 
 async def test_invalid_conversation_id_on_delete(service):
-    with pytest.raises(InvalidParameterError, match="Conversation ID must begin with 'conv_'"):
+    with pytest.raises(InvalidParameterError, match="Conversation ID must match format"):
         await service.openai_delete_conversation(DeleteConversationRequest(conversation_id="bad_id"))
 
 
 async def test_nonexistent_conversation_raises_conversation_not_found(service):
     """Test that get_conversation raises ConversationNotFoundError for nonexistent ID."""
-    with pytest.raises(ConversationNotFoundError, match="Conversation 'conv_nonexistent' not found"):
-        await service.get_conversation(GetConversationRequest(conversation_id="conv_nonexistent"))
+    nonexistent_id = "conv_" + "0" * 48
+    with pytest.raises(ConversationNotFoundError, match=f"Conversation '{nonexistent_id}' not found"):
+        await service.get_conversation(GetConversationRequest(conversation_id=nonexistent_id))
 
 
 async def test_retrieve_nonexistent_item_raises_conversation_item_not_found(service):