fix(tracking): validate identifiers and keep server paths inside the storage directory

burr/cli/__main__.py

@@ -164,6 +164,7 @@ def _run_server(
     initial_page="",
     host: str = "127.0.0.1",
     backend: str = "local",
+    demos: bool = False,
 ):
     # TODO: Implement server running logic here
     # Example: Start a web server, configure ports, etc.
@@ -201,6 +202,9 @@ def _run_server(
     env = {
         "BURR_BACKEND_IMPL": BACKEND_MODULES[backend],
     }
+    if demos:
+        # The demo routers are off unless this is set; see burr.tracking.server.run.
+        env["BURR_SERVER_DEMOS"] = "true"
     _command(cmd, capture_output=False, addl_env=env)
 
 
@@ -212,8 +216,9 @@ def _run_server(
 @click.option(
     "--host",
     default="127.0.0.1",
-    help="Host to run the server on -- use 0.0.0.0 if you want "
-    "to expose it to the network (E.G. in a docker image)",
+    help="Host to run the server on -- defaults to 127.0.0.1 (local only). Use 0.0.0.0 if you "
+    "want to expose it to the network (E.G. in a docker image); the server has no built-in "
+    "authentication, so put it behind an authenticating proxy when doing so.",
 )
 @click.option(
     "--backend",
@@ -224,13 +229,20 @@ def _run_server(
 def run_server(
     port: int, dev_mode: bool, no_open: bool, no_copy_demo_data: bool, host: str, backend: str
 ):
+    """Run the Burr UI server.
+
+    The demo apps (counter, chatbot, email assistant, ...) are not mounted unless the
+    BURR_SERVER_DEMOS environment variable is set to 1, true or yes; `burr-demo` runs the
+    server with them on.
+    """
     _run_server(port, dev_mode, no_open, no_copy_demo_data, host=host, backend=backend)
 
 
 @cli.command()
 @click.option("--port", default=7241, help="Port to run the server on")
 def demo_server(port: int):
-    _run_server(port, True, False, False, "demos/chatbot")
+    """Run the Burr UI server with the demo apps mounted (sets BURR_SERVER_DEMOS=true)."""
+    _run_server(port, True, False, False, "demos/chatbot", demos=True)
 
 
 @cli.command(help="Publishes the package to a repository")

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/backend.py

@@ -19,6 +19,7 @@
 import collections
 import importlib
 import json
+import logging
 import os.path
 import sys
 from datetime import datetime
@@ -31,6 +32,7 @@
 from pydantic_settings import BaseSettings, SettingsConfigDict
 
 from burr.tracking.common import models
+from burr.tracking.common.identifiers import join_within, validate_identifier
 from burr.tracking.common.models import ChildApplicationModel
 from burr.tracking.server import schema
 from burr.tracking.server.schema import (
@@ -44,6 +46,8 @@
 
 T = TypeVar("T")
 
+logger = logging.getLogger(__name__)
+
 # The following is a backend for the server.
 # Note this is not a fixed API yet, and thus not documented (in Burr's documentation)
 # Specifically, this does not have:
@@ -293,6 +297,42 @@ def get_uri(project_id: str) -> str:
 DEFAULT_PATH = os.path.expanduser("~/.burr")
 
 
+def _validate_identifier(value: str, name: str = "identifier") -> str:
+    """Validate a project/app identifier that is used as a path component under the storage
+    directory. The rule itself lives in :mod:`burr.tracking.common.identifiers` and is shared
+    with the tracking client; this wrapper reports failures as HTTP 400.
+
+    :param value: the identifier to validate
+    :param name: label for the error message, e.g. ``"project_id"``
+    :return: the identifier if valid
+    :raises fastapi.HTTPException: 400 if the identifier does not meet the rule
+    """
+    try:
+        return validate_identifier(value, name)
+    except ValueError as e:
+        raise fastapi.HTTPException(status_code=400, detail=str(e)) from e
+
+
+def _safe_join(base: str, *parts: str) -> str:
+    """Join path components and ensure the result stays inside ``base``.
+
+    Delegates to :func:`burr.tracking.common.identifiers.join_within`, which resolves symlinks
+    on both sides before comparing, so a link under the storage directory that points elsewhere
+    is rejected too. Failures are reported as HTTP 400 without echoing the storage location.
+
+    :param base: the allowed base directory
+    :param parts: path components to join
+    :return: the joined path
+    :raises fastapi.HTTPException: 400 if the resolved path is not inside the base directory
+    """
+    try:
+        return join_within(base, *parts)
+    except ValueError as e:
+        raise fastapi.HTTPException(
+            status_code=400, detail="Invalid path: must be inside the storage directory."
+        ) from e
+
+
 class LocalBackend(BackendBase, AnnotationsBackendMixin):
     """Quick implementation of a local backend for testing purposes. This is not a production backend.
 
@@ -303,7 +343,8 @@ def __init__(self, path: str = DEFAULT_PATH):
         self.path = path
 
     def _get_annotation_path(self, project_id: str) -> str:
-        return os.path.join(self.path, project_id, "annotations.jsonl")
+        _validate_identifier(project_id, "project_id")
+        return _safe_join(self.path, project_id, "annotations.jsonl")
 
     async def _load_project_annotations(self, project_id: str):
         annotations_path = self._get_annotation_path(project_id)
@@ -464,7 +505,8 @@ async def list_apps(
         limit: int = 100,
         offset: int = 0,
     ) -> Tuple[Sequence[ApplicationSummary], int]:
-        project_filepath = os.path.join(self.path, project_id)
+        _validate_identifier(project_id, "project_id")
+        project_filepath = _safe_join(self.path, project_id)
         if not os.path.exists(project_filepath):
             return [], 0
             # raise fastapi.HTTPException(status_code=404, detail=f"Project: {project_id} not found")
@@ -473,9 +515,19 @@ async def list_apps(
             if entry.startswith("."):
                 # skip hidden files/directories
                 continue
-            full_path = os.path.join(project_filepath, entry)
-            metadata_path = os.path.join(full_path, "metadata.json")
-            log_path = os.path.join(full_path, "log.jsonl")
+            try:
+                full_path = _safe_join(project_filepath, entry)
+                metadata_path = _safe_join(full_path, "metadata.json")
+                log_path = _safe_join(full_path, "log.jsonl")
+            except fastapi.HTTPException:
+                # An entry (or one of its files) that resolves outside the storage directory
+                # is left out of the listing rather than failing the whole page.
+                logger.warning(
+                    "Skipping %r in project %r: it does not resolve inside the storage directory",
+                    entry,
+                    project_id,
+                )
+                continue
             if os.path.isdir(full_path):
                 metadata = await self._load_metadata(metadata_path)
                 app_partition_key = metadata.partition_key
@@ -506,15 +558,19 @@ async def get_application_logs(
     ) -> ApplicationLogs:
         # TODO -- handle partition key here
         # This currently assumes uniqueness
-        app_filepath = os.path.join(self.path, project_id, app_id)
+        _validate_identifier(project_id, "project_id")
+        _validate_identifier(app_id, "app_id")
+        app_filepath = _safe_join(self.path, project_id, app_id)
         if not os.path.exists(app_filepath):
             raise fastapi.HTTPException(
                 status_code=404, detail=f"App: {app_id} from project: {project_id} not found"
             )
-        log_file = os.path.join(app_filepath, "log.jsonl")
-        graph_file = os.path.join(app_filepath, "graph.json")
-        metadata_file = os.path.join(app_filepath, "metadata.json")
-        children_file = os.path.join(app_filepath, "children.jsonl")
+        # The fixed filenames go through the same containment check as the identifiers, so a
+        # link under the app directory that points elsewhere is refused before it is opened.
+        log_file = _safe_join(app_filepath, "log.jsonl")
+        graph_file = _safe_join(app_filepath, "graph.json")
+        metadata_file = _safe_join(app_filepath, "metadata.json")
+        children_file = _safe_join(app_filepath, "children.jsonl")
         metadata = await self._load_metadata(metadata_file)
         if not os.path.exists(graph_file):
             raise fastapi.HTTPException(