fix(tracking): validate app ids in the local tracking client

burr/tracking/client.py

@@ -46,7 +46,6 @@ def flock(*args, **kwargs):
 import json
 import logging
 import os
-import re
 import traceback
 from abc import ABC
 from typing import Any, Dict, Optional, Tuple
@@ -68,6 +67,7 @@ def flock(*args, **kwargs):
     PreRunStepHook,
     PreStartSpanHook,
 )
+from burr.tracking.common.identifiers import join_within, validate_identifier
 from burr.tracking.common.models import (
     ApplicationMetadataModel,
     ApplicationModel,
@@ -109,13 +109,15 @@ def _filter_inputs(d: dict) -> dict:
 
 
 def _allowed_project_name(project_name: str, on_windows: bool) -> bool:
-    allowed_chars = r"a-zA-Z0-9_\-"
-    if not on_windows:
-        allowed_chars += ":"
-    pattern = f"^[{allowed_chars}]+$"
+    """Whether ``project_name`` passes the identifier rule shared with app ids.
 
-    # Use regular expression to check if the string is valid
-    return bool(re.match(pattern, project_name))
+    Kept as a boolean wrapper around :func:`validate_identifier` for existing callers.
+    """
+    try:
+        validate_identifier(project_name, "project", on_windows=on_windows)
+    except ValueError:
+        return False
+    return True
 
 
 @dataclasses.dataclass
@@ -190,11 +192,7 @@ def __init__(
         """
 
         self.f = None
-        if not _allowed_project_name(project, on_windows=system.IS_WINDOWS):
-            raise ValueError(
-                f"Project: {project} is not valid. Project name cannot contain non-alphanumeric (except _ and -) characters."
-                "We will be relaxing this restriction later but for now please rename your project!"
-            )
+        validate_identifier(project, "project")
         self.raw_storage_dir = storage_dir
         self.storage_dir = LocalTrackingClient.get_storage_path(project, storage_dir)
         self.project_id = project
@@ -255,7 +253,7 @@ def _log_child_relationships(
                 )
             )
         for parent_id, child_of in parent_relationships:
-            parent_path = os.path.join(self.storage_dir, parent_id)
+            parent_path = self._application_directory(parent_id)
             if not os.path.exists(parent_path):
                 # This currently makes the parent directory so that it does not fail
                 # If the parent directory exists we'll just use that
@@ -285,7 +283,22 @@ def copy(self) -> "LocalTrackingClient":
 
     @classmethod
     def get_storage_path(cls, project, storage_dir) -> str:
-        return str(os.path.join(os.path.expanduser(storage_dir), project))
+        return str(join_within(os.path.expanduser(storage_dir), project))
+
+    @classmethod
+    def _application_log_path(cls, project: str, app_id: str, storage_dir: str) -> str:
+        """Path to an application's log file, with both identifiers validated and the
+        result kept inside the storage directory."""
+        validate_identifier(project, "project")
+        validate_identifier(app_id, "app_id")
+        application_path = join_within(cls.get_storage_path(project, storage_dir), app_id)
+        return os.path.join(application_path, cls.LOG_FILENAME)
+
+    def _application_directory(self, app_id: str) -> str:
+        """Directory for one application run, validated to sit inside this project's
+        storage directory. Nothing is created on disk."""
+        validate_identifier(app_id, "app_id")
+        return join_within(self.storage_dir, app_id)
 
     @classmethod
     def app_log_exists(
@@ -301,7 +314,7 @@ def app_log_exists(
         :param storage_dir: the storage directory.
         :return: True if state exists, False otherwise.
         """
-        path = os.path.join(cls.get_storage_path(project, storage_dir), app_id, cls.LOG_FILENAME)
+        path = cls._application_log_path(project, app_id, storage_dir)
         if not os.path.exists(path):
             return False
         lines = open(path, "r", errors="replace", encoding="utf-8").readlines()
@@ -336,7 +349,7 @@ def load_state(
         """
         if sequence_id is None:
             sequence_id = -1  # get the last one
-        path = os.path.join(cls.get_storage_path(project, storage_dir), app_id, cls.LOG_FILENAME)
+        path = cls._application_log_path(project, app_id, storage_dir)
         if not os.path.exists(path):
             raise ValueError(f"No logs found for {project}/{app_id} under {storage_dir}")
         with open(path, "r", errors="replace", encoding="utf-8") as f:
@@ -370,14 +383,16 @@ def load_state(
         prior_state["__SEQUENCE_ID"] = line_seq  # add the sequence id back
         return prior_state, entry_point
 
-    def _ensure_dir_structure(self, app_id: str):
+    def _ensure_dir_structure(self, app_id: str) -> str:
+        # Validate the id before anything is created on disk
+        application_path = self._application_directory(app_id)
         if not os.path.exists(self.storage_dir):
             logger.info(f"Creating storage directory: {self.storage_dir}")
             os.makedirs(self.storage_dir)
-        application_path = os.path.join(self.storage_dir, app_id)
         if not os.path.exists(application_path):
             logger.info(f"Creating application directory: {application_path}")
             os.makedirs(application_path)
+        return application_path
 
     def __setstate__(self, state):
         self.__dict__.update(state)
@@ -402,15 +417,20 @@ def post_application_create(
         spawning_parent_pointer: Optional[burr_types.ParentPointer],
         **future_kwargs: Any,
     ):
-        self._ensure_dir_structure(app_id)
+        # Every identifier that becomes a path is checked before the first write. A pointer
+        # with no app_id (plain resume, not a fork) never becomes a path, so it is skipped.
+        for pointer in (parent_pointer, spawning_parent_pointer):
+            if pointer is not None and pointer.app_id is not None:
+                validate_identifier(pointer.app_id, "app_id")
+        application_path = self._ensure_dir_structure(app_id)
         self.f = open(
-            os.path.join(self.storage_dir, app_id, self.LOG_FILENAME),
+            os.path.join(application_path, self.LOG_FILENAME),
             "a",
             encoding="utf-8",
             errors="replace",
         )
 
-        graph_path = os.path.join(self.storage_dir, app_id, self.GRAPH_FILENAME)
+        graph_path = os.path.join(application_path, self.GRAPH_FILENAME)
         if os.path.exists(graph_path):
             logger.info(f"Graph already exists at {graph_path}. Not overwriting.")
             return
@@ -420,7 +440,7 @@ def post_application_create(
         with open(graph_path, "w", encoding="utf-8", errors="replace") as f:
             json.dump(graph, f)
 
-        metadata_path = os.path.join(self.storage_dir, app_id, self.METADATA_FILENAME)
+        metadata_path = os.path.join(application_path, self.METADATA_FILENAME)
         if os.path.exists(metadata_path):
             logger.info(f"Metadata already exists at {metadata_path}. Not overwriting.")
             return
@@ -617,7 +637,7 @@ def load(
         # TODO:
         if app_id is None:
             return  # no application ID
-        path = os.path.join(self.storage_dir, app_id, self.LOG_FILENAME)
+        path = os.path.join(self._application_directory(app_id), self.LOG_FILENAME)
         if not os.path.exists(path):
             return None
         with open(path, "r", errors="replace", encoding="utf-8") as f:

burr/tracking/common/identifiers.py

@@ -0,0 +1,108 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+"""Validation for the identifiers (project names, application ids) that the tracking
+layer turns into filesystem paths, plus a helper that keeps the resulting paths inside
+the storage directory."""
+
+import os
+import re
+from typing import Optional
+
+from burr import system
+
+# Any character except the path separators ("/" and "\") and the control characters
+# (NUL through unit separator, and DEL). Checked with ``fullmatch`` so the whole value
+# has to satisfy it; a trailing newline is a control character and is refused like any
+# other.
+IDENTIFIER_PATTERN = re.compile(r"[^/\\\x00-\x1f\x7f]+")
+MAX_IDENTIFIER_LENGTH = 255
+
+
+class InvalidIdentifier(ValueError):
+    """An identifier cannot be used as a path segment under the storage directory, or a
+    joined path would not stay inside it.
+
+    Subclasses :class:`ValueError` so existing ``except ValueError`` handlers keep working;
+    callers that want to report the problem specifically (for instance as an HTTP 400) can
+    catch this class instead.
+    """
+
+
+def validate_identifier(value: str, what: str, *, on_windows: Optional[bool] = None) -> str:
+    """Checks that ``value`` can be used as any single path segment under the storage directory.
+
+    The rule: a non-empty string of at most 255 characters that is not the special directory
+    name ``.`` or ``..`` and contains no ``/``, ``\\``, NUL or other control character. On
+    Windows ``:`` is also refused, as it is a drive/stream separator there. Everything else --
+    spaces, ``@``, dots, unicode letters -- is an ordinary filename character and is accepted,
+    so ids such as ``user@example.com`` or ``chat session 1`` keep working.
+
+    :param value: the identifier to check
+    :param what: short label for the error message, e.g. ``"app_id"`` or ``"project"``
+    :param on_windows: platform override, defaults to the current platform; exposed for tests
+    :return: ``value`` unchanged, so the call can be used inline
+    :raises InvalidIdentifier: if the identifier does not meet the rule
+    """
+    if on_windows is None:
+        on_windows = system.IS_WINDOWS
+    if not isinstance(value, str):
+        raise InvalidIdentifier(f"{what} must be a string, got {type(value).__name__}: {value!r}")
+    if not value:
+        raise InvalidIdentifier(f"{what} must not be empty")
+    if len(value) > MAX_IDENTIFIER_LENGTH:
+        raise InvalidIdentifier(
+            f"{what} must be at most {MAX_IDENTIFIER_LENGTH} characters, got {len(value)}"
+        )
+    if value in (".", ".."):
+        raise InvalidIdentifier(f"{what} must not be '.' or '..', got {value!r}")
+    if IDENTIFIER_PATTERN.fullmatch(value) is None:
+        raise InvalidIdentifier(
+            f"{what} must be a single path segment: it may not contain '/', '\\' or control "
+            f"characters, got {value!r}"
+        )
+    if on_windows and ":" in value:
+        raise InvalidIdentifier(f"{what} may not contain ':' on Windows, got {value!r}")
+    return value
+
+
+def join_within(base: str, *parts: str) -> str:
+    """Joins ``parts`` onto ``base`` and checks that the result stays strictly inside ``base``.
+
+    Both sides are resolved with :func:`os.path.realpath` before comparing, so ``..`` segments
+    and symlinks are accounted for. The path returned is the plain join (not the resolved
+    form) so callers see the same spelling they would get from :func:`os.path.join`.
+
+    :param base: the directory the result must stay inside
+    :param parts: path components to join onto ``base``
+    :return: ``os.path.join(base, *parts)``
+    :raises InvalidIdentifier: if the joined path would land outside ``base``
+    """
+    joined = os.path.join(base, *parts)
+    base_resolved = os.path.realpath(base)
+    target_resolved = os.path.realpath(joined)
+    try:
+        inside = (
+            target_resolved != base_resolved
+            and os.path.commonpath([base_resolved, target_resolved]) == base_resolved
+        )
+    except ValueError:
+        # commonpath refuses to compare paths on different drives (Windows); treat as outside
+        inside = False
+    if not inside:
+        raise InvalidIdentifier(f"Path {joined!r} is not inside the storage directory {base!r}")
+    return joined

burr/tracking/server/run.py

@@ -24,9 +24,11 @@
 from typing import Optional, Sequence
 
 from starlette import status
+from starlette.responses import JSONResponse
 
 # TODO -- remove this, just for testing
 from burr.log_setup import setup_logging
+from burr.tracking.common.identifiers import InvalidIdentifier
 from burr.tracking.server.backend import (
     AnnotationsBackendMixin,
     BackendBase,
@@ -201,6 +203,13 @@ def create_burr_ui_app(serve_static: bool = SERVE_STATIC) -> FastAPI:
     """
     ui_app = FastAPI(lifespan=lifespan)
 
+    @ui_app.exception_handler(InvalidIdentifier)
+    async def invalid_identifier_to_400(request: Request, exc: InvalidIdentifier):
+        # An identifier that cannot be used as a path segment under the storage directory
+        # (the tracking client behind the demo routes raises this) is a client error, so
+        # report it as 400 with the reason rather than letting it surface as a 500.
+        return JSONResponse(status_code=status.HTTP_400_BAD_REQUEST, content={"detail": str(exc)})
+
     @ui_app.get("/ready")
     def is_ready():
         if not initialized: