From 5fa3e5a7506f599d41f1ac39924e03447d534875 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Ahlert?= Date: Sun, 4 Oct 2026 13:21:14 -0700 Subject: [PATCH 1/4] fix(tracking): validate identifiers and keep server paths inside the storage directory Add an allowlist check for project and app identifiers in LocalBackend and a join helper that resolves paths and verifies they stay inside the configured storage directory. Bind the standalone server to 127.0.0.1 by default. --- burr/tracking/server/backend.py | 49 +++++++- burr/tracking/server/run.py | 2 +- tests/tracking/test_local_backend_security.py | 112 ++++++++++++++++++ 3 files changed, 159 insertions(+), 4 deletions(-) create mode 100644 tests/tracking/test_local_backend_security.py diff --git a/burr/tracking/server/backend.py b/burr/tracking/server/backend.py index 5e9e1c771..9c4e39b26 100644 --- a/burr/tracking/server/backend.py +++ b/burr/tracking/server/backend.py @@ -20,6 +20,7 @@ import importlib import json import os.path +import re import sys from datetime import datetime from typing import Any, Optional, Sequence, Tuple, Type, TypeVar @@ -292,6 +293,44 @@ def get_uri(project_id: str) -> str: DEFAULT_PATH = os.path.expanduser("~/.burr") +# Regex for valid project/app identifiers — no path separators or traversal. +_VALID_IDENTIFIER_RE = re.compile(r"^[a-zA-Z0-9_\-:]+$") + + +def _validate_identifier(value: str, name: str = "identifier") -> str: + """Validate that a project/app identifier does not contain path traversal characters. + + :param value: the identifier to validate + :param name: human-readable name for error messages + :return: the identifier if valid + :raises fastapi.HTTPException: 400 if the identifier contains invalid characters + """ + if not _VALID_IDENTIFIER_RE.match(value): + raise fastapi.HTTPException( + status_code=400, + detail=f"Invalid {name}: '{value}'. Only alphanumeric, underscore, hyphen and colon are allowed.", + ) + return value + + +def _safe_join(base: str, *parts: str) -> str: + """Safely join path components and ensure the result stays within ``base``. + + :param base: the allowed base directory + :param parts: path components to join + :return: the resolved path + :raises fastapi.HTTPException: 400 if the resolved path escapes the base directory + """ + target = os.path.realpath(os.path.join(base, *parts)) + base_real = os.path.realpath(base) + # Ensure target is either the base directory or a subdirectory of it + if target != base_real and not target.startswith(base_real + os.sep): + raise fastapi.HTTPException( + status_code=400, + detail="Path traversal detected: attempted to escape the base directory.", + ) + return target + class LocalBackend(BackendBase, AnnotationsBackendMixin): """Quick implementation of a local backend for testing purposes. This is not a production backend. @@ -303,7 +342,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 +504,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") @@ -506,7 +547,9 @@ 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" diff --git a/burr/tracking/server/run.py b/burr/tracking/server/run.py index da5b8b965..ef7e1c018 100644 --- a/burr/tracking/server/run.py +++ b/burr/tracking/server/run.py @@ -450,4 +450,4 @@ def mount_burr_ui( if __name__ == "__main__": port = int(os.getenv("PORT", 8000)) # Default to 8000 if no PORT environment variable is set - uvicorn.run(app, host="0.0.0.0", port=port) + uvicorn.run(app, host="127.0.0.1", port=port) diff --git a/tests/tracking/test_local_backend_security.py b/tests/tracking/test_local_backend_security.py new file mode 100644 index 000000000..8f640d374 --- /dev/null +++ b/tests/tracking/test_local_backend_security.py @@ -0,0 +1,112 @@ +# 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. + +import pytest +from fastapi import HTTPException + +from burr.tracking.server.backend import LocalBackend, _safe_join, _validate_identifier + + +class TestValidateIdentifier: + def test_valid_identifiers(self): + assert _validate_identifier("hello_world") == "hello_world" + assert _validate_identifier("hello-world") == "hello-world" + assert _validate_identifier("Hello:World_123") == "Hello:World_123" + + def test_invalid_identifiers(self): + with pytest.raises(HTTPException) as exc: + _validate_identifier("../etc/passwd") + assert exc.value.status_code == 400 + + with pytest.raises(HTTPException) as exc: + _validate_identifier("hello/world") + assert exc.value.status_code == 400 + + with pytest.raises(HTTPException) as exc: + _validate_identifier("hello\\world") + assert exc.value.status_code == 400 + + with pytest.raises(HTTPException) as exc: + _validate_identifier("hello..world") + assert exc.value.status_code == 400 + + +class TestSafeJoin: + def test_safe_join_within_base(self, tmp_path): + base = str(tmp_path) + assert _safe_join(base, "project1") == str(tmp_path / "project1") + assert _safe_join(base, "project1", "app1") == str(tmp_path / "project1" / "app1") + + def test_safe_join_blocks_traversal(self, tmp_path): + base = str(tmp_path) + with pytest.raises(HTTPException) as exc: + _safe_join(base, "..", "etc") + assert exc.value.status_code == 400 + + with pytest.raises(HTTPException) as exc: + _safe_join(base, "project", "..", "..", "etc") + assert exc.value.status_code == 400 + + def test_safe_join_allows_exact_base(self, tmp_path): + base = str(tmp_path) + # Joining with nothing should return the base itself + assert _safe_join(base) == base + + +class TestLocalBackendPathTraversal: + def test_get_annotation_path_rejects_traversal(self, tmp_path): + backend = LocalBackend(path=str(tmp_path)) + with pytest.raises(HTTPException) as exc: + backend._get_annotation_path("../etc") + assert exc.value.status_code == 400 + + @pytest.mark.asyncio + async def test_list_apps_rejects_traversal(self, tmp_path): + backend = LocalBackend(path=str(tmp_path)) + with pytest.raises(HTTPException) as exc: + await backend.list_apps(None, "../../../etc", None) + assert exc.value.status_code == 400 + + @pytest.mark.asyncio + async def test_get_application_logs_rejects_traversal_project(self, tmp_path): + backend = LocalBackend(path=str(tmp_path)) + with pytest.raises(HTTPException) as exc: + await backend.get_application_logs(None, "../etc", "app1", None) + assert exc.value.status_code == 400 + + @pytest.mark.asyncio + async def test_get_application_logs_rejects_traversal_app(self, tmp_path): + backend = LocalBackend(path=str(tmp_path)) + with pytest.raises(HTTPException) as exc: + await backend.get_application_logs(None, "project1", "../etc", None) + assert exc.value.status_code == 400 + + @pytest.mark.asyncio + async def test_get_application_logs_allows_valid(self, tmp_path): + backend = LocalBackend(path=str(tmp_path)) + # Create the expected directory structure + app_dir = tmp_path / "project1" / "app1" + app_dir.mkdir(parents=True) + # Use a minimal valid graph.json matching ApplicationModel schema + (app_dir / "graph.json").write_text( + '{"entrypoint": "counter", "actions": [{"name": "counter", "reads": [], "writes": ["counter"], "code": "pass"}], "transitions": []}' + ) + (app_dir / "log.jsonl").write_text("") + (app_dir / "metadata.json").write_text("{}") + + result = await backend.get_application_logs(None, "project1", "app1", None) + assert result is not None From 4a43a57c021a7a76e68ad5233d55800c07467e77 Mon Sep 17 00:00:00 2001 From: Elijah Ben Izzy Date: Sun, 4 Oct 2026 13:32:47 -0700 Subject: [PATCH 2/4] fix(tracking): share the identifier rule with the client and cover the server endpoints Move the project/app identifier rule and the path join helper into burr.tracking.common.identifiers (shared with LocalTrackingClient), keep the LocalBackend wrappers that report failures as HTTP 400, honour BURR_SERVER_HOST for the standalone server, and extend the tests to the FastAPI endpoints, symlinked directories and client-style identifiers. --- burr/cli/__main__.py | 5 +- burr/tracking/common/identifiers.py | 92 +++++ burr/tracking/server/backend.py | 46 ++- burr/tracking/server/run.py | 10 +- tests/tracking/test_local_backend_paths.py | 324 ++++++++++++++++++ tests/tracking/test_local_backend_security.py | 112 ------ 6 files changed, 450 insertions(+), 139 deletions(-) create mode 100644 burr/tracking/common/identifiers.py create mode 100644 tests/tracking/test_local_backend_paths.py delete mode 100644 tests/tracking/test_local_backend_security.py diff --git a/burr/cli/__main__.py b/burr/cli/__main__.py index 8967602d9..9205a95d2 100644 --- a/burr/cli/__main__.py +++ b/burr/cli/__main__.py @@ -212,8 +212,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", diff --git a/burr/tracking/common/identifiers.py b/burr/tracking/common/identifiers.py new file mode 100644 index 000000000..090231e27 --- /dev/null +++ b/burr/tracking/common/identifiers.py @@ -0,0 +1,92 @@ +# 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 + +# Letters, digits, underscore, hyphen, colon and dot. This covers uuid4 (hex and +# hyphens) as well as the session/conversation style ids applications tend to pass +# through as app ids. +_ALLOWED_CHARACTERS = r"A-Za-z0-9_\-:." +IDENTIFIER_PATTERN = re.compile(f"^[{_ALLOWED_CHARACTERS}]+$") +MAX_IDENTIFIER_LENGTH = 255 + + +def validate_identifier(value: str, what: str, *, on_windows: Optional[bool] = None) -> str: + """Checks that ``value`` can be used as a single path component under the storage directory. + + The rule: a non-empty string of at most 255 characters drawn from letters, digits, ``_``, + ``-``, ``:`` and ``.``, and not the special directory names ``.`` or ``..``. On Windows + ``:`` is also refused, as it is a drive/stream separator there. + + :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 ValueError: if the identifier does not meet the rule + """ + if on_windows is None: + on_windows = system.IS_WINDOWS + if not isinstance(value, str): + raise ValueError(f"{what} must be a string, got {type(value).__name__}: {value!r}") + if not value: + raise ValueError(f"{what} must not be empty") + if len(value) > MAX_IDENTIFIER_LENGTH: + raise ValueError( + f"{what} must be at most {MAX_IDENTIFIER_LENGTH} characters, got {len(value)}" + ) + if value in (".", ".."): + raise ValueError(f"{what} must not be '.' or '..', got {value!r}") + if not IDENTIFIER_PATTERN.match(value) or (on_windows and ":" in value): + allowed = "letters, digits, '_', '-', '.'" + ("" if on_windows else " and ':'") + raise ValueError(f"{what} may only contain {allowed}, 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 ValueError: 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 ValueError(f"Path {joined!r} is not inside the storage directory {base!r}") + return joined diff --git a/burr/tracking/server/backend.py b/burr/tracking/server/backend.py index 9c4e39b26..5dc250cc9 100644 --- a/burr/tracking/server/backend.py +++ b/burr/tracking/server/backend.py @@ -20,7 +20,6 @@ import importlib import json import os.path -import re import sys from datetime import datetime from typing import Any, Optional, Sequence, Tuple, Type, TypeVar @@ -32,6 +31,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 ( @@ -293,43 +293,41 @@ def get_uri(project_id: str) -> str: DEFAULT_PATH = os.path.expanduser("~/.burr") -# Regex for valid project/app identifiers — no path separators or traversal. -_VALID_IDENTIFIER_RE = re.compile(r"^[a-zA-Z0-9_\-:]+$") - def _validate_identifier(value: str, name: str = "identifier") -> str: - """Validate that a project/app identifier does not contain path traversal characters. + """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: human-readable name for error messages + :param name: label for the error message, e.g. ``"project_id"`` :return: the identifier if valid - :raises fastapi.HTTPException: 400 if the identifier contains invalid characters + :raises fastapi.HTTPException: 400 if the identifier does not meet the rule """ - if not _VALID_IDENTIFIER_RE.match(value): - raise fastapi.HTTPException( - status_code=400, - detail=f"Invalid {name}: '{value}'. Only alphanumeric, underscore, hyphen and colon are allowed.", - ) - return value + 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: - """Safely join path components and ensure the result stays within ``base``. + """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 resolved path - :raises fastapi.HTTPException: 400 if the resolved path escapes the base directory + :return: the joined path + :raises fastapi.HTTPException: 400 if the resolved path is not inside the base directory """ - target = os.path.realpath(os.path.join(base, *parts)) - base_real = os.path.realpath(base) - # Ensure target is either the base directory or a subdirectory of it - if target != base_real and not target.startswith(base_real + os.sep): + try: + return join_within(base, *parts) + except ValueError as e: raise fastapi.HTTPException( - status_code=400, - detail="Path traversal detected: attempted to escape the base directory.", - ) - return target + status_code=400, detail="Invalid path: must be inside the storage directory." + ) from e class LocalBackend(BackendBase, AnnotationsBackendMixin): diff --git a/burr/tracking/server/run.py b/burr/tracking/server/run.py index ef7e1c018..7860f1859 100644 --- a/burr/tracking/server/run.py +++ b/burr/tracking/server/run.py @@ -195,6 +195,10 @@ def create_burr_ui_app(serve_static: bool = SERVE_STATIC) -> FastAPI: This factory creates a new FastAPI instance with all Burr UI routes, demo routers, and (optionally) static file serving configured. + The app does not authenticate requests. Serve it on localhost (the default for the + ``burr`` CLI and for ``python -m burr.tracking.server.run``) or put it behind an + authenticating reverse proxy when exposing it beyond the local machine. + :param serve_static: Whether to serve the React UI static files. Defaults to the BURR_SERVE_STATIC environment variable (true by default). :return: A fully-configured FastAPI application. @@ -450,4 +454,8 @@ def mount_burr_ui( if __name__ == "__main__": port = int(os.getenv("PORT", 8000)) # Default to 8000 if no PORT environment variable is set - uvicorn.run(app, host="127.0.0.1", port=port) + # Bind to localhost unless BURR_SERVER_HOST says otherwise. The server has no built-in + # authentication, so only expose it beyond the local machine (e.g. BURR_SERVER_HOST=0.0.0.0) + # behind an authenticating reverse proxy. + host = os.getenv("BURR_SERVER_HOST", "127.0.0.1") + uvicorn.run(app, host=host, port=port) diff --git a/tests/tracking/test_local_backend_paths.py b/tests/tracking/test_local_backend_paths.py new file mode 100644 index 000000000..58efdcbb6 --- /dev/null +++ b/tests/tracking/test_local_backend_paths.py @@ -0,0 +1,324 @@ +# 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. + +"""Identifier validation and path containment for the local tracking backend. + +Project and app identifiers arrive as URL path parameters and are used as path +components under the storage directory. These tests check that only plain +identifiers are accepted, that every resolved path stays inside the storage +directory, and that the server reports rejected identifiers as HTTP 400. +""" + +import importlib +import os +import uuid +from pathlib import Path + +import pytest +from fastapi import HTTPException + +from burr import system +from burr.tracking.server.backend import LocalBackend, _safe_join, _validate_identifier + +# Minimal valid graph.json matching the ApplicationModel schema +GRAPH_JSON = ( + '{"entrypoint": "counter", "actions": [{"name": "counter", "reads": [], ' + '"writes": ["counter"], "code": "pass"}], "transitions": []}' +) + +ANNOTATION = { + "span_id": None, + "step_name": "counter", + "tags": ["review"], + "observations": [ + {"data_fields": {"note": "looks fine"}, "thumbs_up_thumbs_down": True, "data_pointers": []} + ], +} + +# ".." sent percent-encoded: the HTTP client collapses a literal ".." segment before the +# request leaves, while the server decodes %2E%2E back to ".." and hands it to the handler. +PARENT = "%2E%2E" + + +def _make_app(storage: Path, project_id: str, app_id: str) -> Path: + """Creates the on-disk layout the local tracking client writes for one app.""" + app_dir = storage / project_id / app_id + app_dir.mkdir(parents=True) + (app_dir / "graph.json").write_text(GRAPH_JSON) + (app_dir / "log.jsonl").write_text("") + (app_dir / "metadata.json").write_text("{}") + return app_dir + + +class TestValidateIdentifier: + def test_valid_identifiers(self): + assert _validate_identifier("hello_world") == "hello_world" + assert _validate_identifier("hello-world") == "hello-world" + assert _validate_identifier("Hello:World_123") == "Hello:World_123" + + def test_accepts_everything_the_client_produces(self): + # default app ids are uuid4; project names are [a-zA-Z0-9_-] plus ":" off Windows + app_id = str(uuid.uuid4()) + assert _validate_identifier(app_id, "app_id") == app_id + assert _validate_identifier("my-project_1", "project_id") == "my-project_1" + assert _validate_identifier("a" * 255) == "a" * 255 + # dots are ordinary filename characters as long as the value is not "." or ".." + assert _validate_identifier("v1.2.3", "app_id") == "v1.2.3" + assert _validate_identifier("hello..world", "app_id") == "hello..world" + if not system.IS_WINDOWS: + assert _validate_identifier("demo:chatbot", "project_id") == "demo:chatbot" + + def test_invalid_identifiers(self): + with pytest.raises(HTTPException) as exc: + _validate_identifier("../etc/passwd") + assert exc.value.status_code == 400 + + with pytest.raises(HTTPException) as exc: + _validate_identifier("hello/world") + assert exc.value.status_code == 400 + + with pytest.raises(HTTPException) as exc: + _validate_identifier("hello\\world") + assert exc.value.status_code == 400 + + @pytest.mark.parametrize("value", ["", ".", "..", " ", "a b", "/etc/passwd", "C:\\Windows"]) + def test_rejects_empty_dot_and_absolute_identifiers(self, value): + with pytest.raises(HTTPException) as exc: + _validate_identifier(value, "project_id") + assert exc.value.status_code == 400 + assert "project_id" in exc.value.detail + + def test_rejects_over_long_and_non_string_identifiers(self): + with pytest.raises(HTTPException) as exc: + _validate_identifier("a" * 256) + assert exc.value.status_code == 400 + + with pytest.raises(HTTPException) as exc: + _validate_identifier(None) # type: ignore[arg-type] + assert exc.value.status_code == 400 + + +class TestSafeJoin: + def test_safe_join_within_base(self, tmp_path): + base = str(tmp_path) + assert _safe_join(base, "project1") == str(tmp_path / "project1") + assert _safe_join(base, "project1", "app1") == str(tmp_path / "project1" / "app1") + + def test_safe_join_rejects_parent_directory(self, tmp_path): + base = str(tmp_path) + with pytest.raises(HTTPException) as exc: + _safe_join(base, "..", "etc") + assert exc.value.status_code == 400 + + with pytest.raises(HTTPException) as exc: + _safe_join(base, "project", "..", "..", "etc") + assert exc.value.status_code == 400 + + def test_safe_join_rejects_absolute_parts(self, tmp_path): + # os.path.join discards everything before an absolute component + with pytest.raises(HTTPException) as exc: + _safe_join(str(tmp_path), "/etc/passwd") + assert exc.value.status_code == 400 + + with pytest.raises(HTTPException) as exc: + _safe_join(str(tmp_path), "project", os.sep + "etc") + assert exc.value.status_code == 400 + + def test_safe_join_refuses_exact_base(self, tmp_path): + # identifier paths must be strictly inside the storage directory, never the root itself + with pytest.raises(HTTPException) as exc: + _safe_join(str(tmp_path)) + assert exc.value.status_code == 400 + + def test_safe_join_follows_symlinks(self, tmp_path): + storage = tmp_path / "storage" + storage.mkdir() + outside = tmp_path / "outside" + outside.mkdir() + (storage / "inside").mkdir() + (storage / "to-outside").symlink_to(outside, target_is_directory=True) + (storage / "to-inside").symlink_to(storage / "inside", target_is_directory=True) + + # a link under the storage directory that resolves elsewhere is rejected ... + with pytest.raises(HTTPException) as exc: + _safe_join(str(storage), "to-outside") + assert exc.value.status_code == 400 + with pytest.raises(HTTPException) as exc: + _safe_join(str(storage), "to-outside", "annotations.jsonl") + assert exc.value.status_code == 400 + + # ... while one that stays inside is accepted (the plain join is returned) + assert _safe_join(str(storage), "to-inside", "app") == str(storage / "to-inside" / "app") + + def test_safe_join_accepts_symlinked_base(self, tmp_path): + real_base = tmp_path / "real" + real_base.mkdir() + linked_base = tmp_path / "linked" + linked_base.symlink_to(real_base, target_is_directory=True) + assert _safe_join(str(linked_base), "project") == str(linked_base / "project") + + +class TestLocalBackendPathContainment: + def test_get_annotation_path_rejects_parent_directory(self, tmp_path): + backend = LocalBackend(path=str(tmp_path)) + with pytest.raises(HTTPException) as exc: + backend._get_annotation_path("../etc") + assert exc.value.status_code == 400 + + @pytest.mark.asyncio + async def test_list_apps_rejects_parent_directory(self, tmp_path): + backend = LocalBackend(path=str(tmp_path)) + with pytest.raises(HTTPException) as exc: + await backend.list_apps(None, "../../../etc", None) + assert exc.value.status_code == 400 + + @pytest.mark.asyncio + async def test_get_application_logs_rejects_parent_directory_in_project(self, tmp_path): + backend = LocalBackend(path=str(tmp_path)) + with pytest.raises(HTTPException) as exc: + await backend.get_application_logs(None, "../etc", "app1", None) + assert exc.value.status_code == 400 + + @pytest.mark.asyncio + async def test_get_application_logs_rejects_parent_directory_in_app(self, tmp_path): + backend = LocalBackend(path=str(tmp_path)) + with pytest.raises(HTTPException) as exc: + await backend.get_application_logs(None, "project1", "../etc", None) + assert exc.value.status_code == 400 + + @pytest.mark.asyncio + async def test_get_application_logs_allows_valid(self, tmp_path): + backend = LocalBackend(path=str(tmp_path)) + _make_app(tmp_path, "project1", "app1") + result = await backend.get_application_logs(None, "project1", "app1", None) + assert result is not None + + +@pytest.fixture +def server(tmp_path, monkeypatch): + """A TestClient for the tracking server with a LocalBackend on a fresh directory. + + ``burr.tracking.server.run`` builds its backend and FastAPI app at import time from the + environment, so the environment is prepared before the import and the module-level + backend is swapped afterwards (the endpoints look it up on every call). + """ + storage = tmp_path / "storage" + storage.mkdir() + monkeypatch.setenv("BURR_SERVE_STATIC", "false") + monkeypatch.setenv("BURR_BACKEND_IMPL", "burr.tracking.server.backend.LocalBackend") + monkeypatch.setenv("burr_path", str(storage)) + run = importlib.import_module("burr.tracking.server.run") + monkeypatch.setattr(run, "backend", LocalBackend(path=str(storage))) + + from fastapi.testclient import TestClient + + return TestClient(run.app), storage + + +class TestServerEndpoints: + def test_list_apps_rejects_parent_directory(self, server): + client, _ = server + response = client.get(f"/api/v0/{PARENT}/__none__/apps") + assert response.status_code == 400 + assert "project_id" in response.json()["detail"] + + def test_application_logs_reject_parent_directory(self, server): + client, storage = server + _make_app(storage, "proj", "app-1") + assert client.get(f"/api/v0/{PARENT}/app-1/__none__/apps").status_code == 400 + assert client.get(f"/api/v0/proj/{PARENT}/__none__/apps").status_code == 400 + + @pytest.mark.parametrize("project_id", ["..%5C..%5Cetc", "my%20project", "a" * 256]) + def test_rejects_other_malformed_identifiers(self, server, project_id): + client, _ = server + assert client.get(f"/api/v0/{project_id}/__none__/apps").status_code == 400 + + def test_separators_never_reach_a_handler(self, server): + # The router splits on "/" whether or not it is percent-encoded, so an identifier + # containing one cannot match any identifier route; the function-level tests above + # cover these values directly. + client, _ = server + assert client.get("/api/v0/..%2F..%2Fetc/__none__/apps").status_code == 404 + assert client.get("/api/v0/%2Fetc%2Fpasswd/__none__/apps").status_code == 404 + + def test_annotation_endpoints_reject_parent_directory(self, server): + client, storage = server + assert client.get(f"/api/v0/{PARENT}/annotations").status_code == 400 + response = client.post(f"/api/v0/{PARENT}/app-1/__none__/0/annotations", json=ANNOTATION) + assert response.status_code == 400 + response = client.put(f"/api/v0/{PARENT}/0/update_annotations", json=ANNOTATION) + assert response.status_code == 400 + # nothing was written next to the storage directory + assert not (storage.parent / "annotations.jsonl").exists() + + def test_symlinked_project_outside_storage_is_rejected(self, server, tmp_path): + client, storage = server + outside = tmp_path / "outside" + outside.mkdir() + (storage / "linked").symlink_to(outside, target_is_directory=True) + + response = client.get("/api/v0/linked/__none__/apps") + assert response.status_code == 400 + # the error does not reveal where the storage directory lives + assert str(storage) not in response.json()["detail"] + assert client.get("/api/v0/linked/app-1/__none__/apps").status_code == 400 + assert client.get("/api/v0/linked/annotations").status_code == 400 + response = client.post("/api/v0/linked/app-1/__none__/0/annotations", json=ANNOTATION) + assert response.status_code == 400 + assert not (outside / "annotations.jsonl").exists() + + def test_accepts_identifiers_the_client_produces(self, server): + client, storage = server + project_id = "my-project_1" + app_id = str(uuid.uuid4()) + _make_app(storage, project_id, app_id) + + response = client.get(f"/api/v0/{project_id}/__none__/apps") + assert response.status_code == 200 + assert [a["app_id"] for a in response.json()["applications"]] == [app_id] + + response = client.get(f"/api/v0/{project_id}/{app_id}/__none__/apps") + assert response.status_code == 200 + assert response.json()["application"]["entrypoint"] == "counter" + + # ":" is used by the demo projects on non-Windows hosts; an unknown project is an + # empty listing rather than a rejection + if not system.IS_WINDOWS: + response = client.get("/api/v0/demo:chatbot/__none__/apps") + assert response.status_code == 200 + assert response.json()["applications"] == [] + + def test_valid_but_missing_app_is_not_found(self, server): + client, storage = server + _make_app(storage, "proj", "app-1") + assert client.get(f"/api/v0/proj/{uuid.uuid4()}/__none__/apps").status_code == 404 + + def test_annotation_round_trip_with_valid_identifiers(self, server): + client, storage = server + app_id = str(uuid.uuid4()) + _make_app(storage, "proj", app_id) + + response = client.post(f"/api/v0/proj/{app_id}/__none__/0/annotations", json=ANNOTATION) + assert response.status_code == 200 + assert (storage / "proj" / "annotations.jsonl").exists() + + response = client.put("/api/v0/proj/0/update_annotations", json=ANNOTATION) + assert response.status_code == 200 + + response = client.get(f"/api/v0/proj/annotations?app_id={app_id}") + assert response.status_code == 200 + assert [a["app_id"] for a in response.json()] == [app_id] diff --git a/tests/tracking/test_local_backend_security.py b/tests/tracking/test_local_backend_security.py deleted file mode 100644 index 8f640d374..000000000 --- a/tests/tracking/test_local_backend_security.py +++ /dev/null @@ -1,112 +0,0 @@ -# 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. - -import pytest -from fastapi import HTTPException - -from burr.tracking.server.backend import LocalBackend, _safe_join, _validate_identifier - - -class TestValidateIdentifier: - def test_valid_identifiers(self): - assert _validate_identifier("hello_world") == "hello_world" - assert _validate_identifier("hello-world") == "hello-world" - assert _validate_identifier("Hello:World_123") == "Hello:World_123" - - def test_invalid_identifiers(self): - with pytest.raises(HTTPException) as exc: - _validate_identifier("../etc/passwd") - assert exc.value.status_code == 400 - - with pytest.raises(HTTPException) as exc: - _validate_identifier("hello/world") - assert exc.value.status_code == 400 - - with pytest.raises(HTTPException) as exc: - _validate_identifier("hello\\world") - assert exc.value.status_code == 400 - - with pytest.raises(HTTPException) as exc: - _validate_identifier("hello..world") - assert exc.value.status_code == 400 - - -class TestSafeJoin: - def test_safe_join_within_base(self, tmp_path): - base = str(tmp_path) - assert _safe_join(base, "project1") == str(tmp_path / "project1") - assert _safe_join(base, "project1", "app1") == str(tmp_path / "project1" / "app1") - - def test_safe_join_blocks_traversal(self, tmp_path): - base = str(tmp_path) - with pytest.raises(HTTPException) as exc: - _safe_join(base, "..", "etc") - assert exc.value.status_code == 400 - - with pytest.raises(HTTPException) as exc: - _safe_join(base, "project", "..", "..", "etc") - assert exc.value.status_code == 400 - - def test_safe_join_allows_exact_base(self, tmp_path): - base = str(tmp_path) - # Joining with nothing should return the base itself - assert _safe_join(base) == base - - -class TestLocalBackendPathTraversal: - def test_get_annotation_path_rejects_traversal(self, tmp_path): - backend = LocalBackend(path=str(tmp_path)) - with pytest.raises(HTTPException) as exc: - backend._get_annotation_path("../etc") - assert exc.value.status_code == 400 - - @pytest.mark.asyncio - async def test_list_apps_rejects_traversal(self, tmp_path): - backend = LocalBackend(path=str(tmp_path)) - with pytest.raises(HTTPException) as exc: - await backend.list_apps(None, "../../../etc", None) - assert exc.value.status_code == 400 - - @pytest.mark.asyncio - async def test_get_application_logs_rejects_traversal_project(self, tmp_path): - backend = LocalBackend(path=str(tmp_path)) - with pytest.raises(HTTPException) as exc: - await backend.get_application_logs(None, "../etc", "app1", None) - assert exc.value.status_code == 400 - - @pytest.mark.asyncio - async def test_get_application_logs_rejects_traversal_app(self, tmp_path): - backend = LocalBackend(path=str(tmp_path)) - with pytest.raises(HTTPException) as exc: - await backend.get_application_logs(None, "project1", "../etc", None) - assert exc.value.status_code == 400 - - @pytest.mark.asyncio - async def test_get_application_logs_allows_valid(self, tmp_path): - backend = LocalBackend(path=str(tmp_path)) - # Create the expected directory structure - app_dir = tmp_path / "project1" / "app1" - app_dir.mkdir(parents=True) - # Use a minimal valid graph.json matching ApplicationModel schema - (app_dir / "graph.json").write_text( - '{"entrypoint": "counter", "actions": [{"name": "counter", "reads": [], "writes": ["counter"], "code": "pass"}], "transitions": []}' - ) - (app_dir / "log.jsonl").write_text("") - (app_dir / "metadata.json").write_text("{}") - - result = await backend.get_application_logs(None, "project1", "app1", None) - assert result is not None From d162a414bf2be1d1ce6dbfe910810a8ab5fabcb8 Mon Sep 17 00:00:00 2001 From: Elijah Ben Izzy Date: Sat, 17 Oct 2026 08:16:36 -0700 Subject: [PATCH 3/4] fix(tracking): keep final file paths inside the storage directory and mount demos only on request --- burr/cli/__main__.py | 13 +- burr/tracking/common/identifiers.py | 54 ++++++--- burr/tracking/server/backend.py | 29 +++-- burr/tracking/server/run.py | 47 +++++-- tests/tracking/test_local_backend_paths.py | 78 +++++++++++- tests/tracking/test_server_demos.py | 135 +++++++++++++++++++++ 6 files changed, 314 insertions(+), 42 deletions(-) create mode 100644 tests/tracking/test_server_demos.py diff --git a/burr/cli/__main__.py b/burr/cli/__main__.py index 9205a95d2..a45e03df1 100644 --- a/burr/cli/__main__.py +++ b/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) @@ -225,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") diff --git a/burr/tracking/common/identifiers.py b/burr/tracking/common/identifiers.py index 090231e27..4d5fe1a3e 100644 --- a/burr/tracking/common/identifiers.py +++ b/burr/tracking/common/identifiers.py @@ -25,42 +25,58 @@ from burr import system -# Letters, digits, underscore, hyphen, colon and dot. This covers uuid4 (hex and -# hyphens) as well as the session/conversation style ids applications tend to pass -# through as app ids. -_ALLOWED_CHARACTERS = r"A-Za-z0-9_\-:." -IDENTIFIER_PATTERN = re.compile(f"^[{_ALLOWED_CHARACTERS}]+$") +# 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 a single path component under the storage directory. + """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 drawn from letters, digits, ``_``, - ``-``, ``:`` and ``.``, and not the special directory names ``.`` or ``..``. On Windows - ``:`` is also refused, as it is a drive/stream separator there. + 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 ValueError: if the identifier does not meet the rule + :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 ValueError(f"{what} must be a string, got {type(value).__name__}: {value!r}") + raise InvalidIdentifier(f"{what} must be a string, got {type(value).__name__}: {value!r}") if not value: - raise ValueError(f"{what} must not be empty") + raise InvalidIdentifier(f"{what} must not be empty") if len(value) > MAX_IDENTIFIER_LENGTH: - raise ValueError( + raise InvalidIdentifier( f"{what} must be at most {MAX_IDENTIFIER_LENGTH} characters, got {len(value)}" ) if value in (".", ".."): - raise ValueError(f"{what} must not be '.' or '..', got {value!r}") - if not IDENTIFIER_PATTERN.match(value) or (on_windows and ":" in value): - allowed = "letters, digits, '_', '-', '.'" + ("" if on_windows else " and ':'") - raise ValueError(f"{what} may only contain {allowed}, got {value!r}") + 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 @@ -74,7 +90,7 @@ def join_within(base: str, *parts: str) -> str: :param base: the directory the result must stay inside :param parts: path components to join onto ``base`` :return: ``os.path.join(base, *parts)`` - :raises ValueError: if the joined path would land outside ``base`` + :raises InvalidIdentifier: if the joined path would land outside ``base`` """ joined = os.path.join(base, *parts) base_resolved = os.path.realpath(base) @@ -88,5 +104,5 @@ def join_within(base: str, *parts: str) -> str: # commonpath refuses to compare paths on different drives (Windows); treat as outside inside = False if not inside: - raise ValueError(f"Path {joined!r} is not inside the storage directory {base!r}") + raise InvalidIdentifier(f"Path {joined!r} is not inside the storage directory {base!r}") return joined diff --git a/burr/tracking/server/backend.py b/burr/tracking/server/backend.py index 5dc250cc9..5781132e8 100644 --- a/burr/tracking/server/backend.py +++ b/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 @@ -45,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: @@ -512,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 @@ -552,10 +565,12 @@ async def get_application_logs( 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( diff --git a/burr/tracking/server/run.py b/burr/tracking/server/run.py index 7860f1859..6f3bb66a8 100644 --- a/burr/tracking/server/run.py +++ b/burr/tracking/server/run.py @@ -15,6 +15,14 @@ # specific language governing permissions and limitations # under the License. +"""FastAPI application for the Burr UI server (``burr`` CLI, ``python -m burr.tracking.server.run``). + +The demo routers (counter, chatbot, email assistant, streaming chatbot, deep researcher) +each build their own local tracking client, and are not mounted unless the +``BURR_SERVER_DEMOS`` environment variable is set to ``1``, ``true`` or ``yes`` +(case-insensitive). The ``burr-demo`` CLI entry point sets it; a plain ``burr`` does not. +""" + import asyncio import importlib import logging @@ -82,6 +90,12 @@ SERVE_STATIC = os.getenv("BURR_SERVE_STATIC", "true").lower() == "true" + +def _demos_enabled() -> bool: + """Whether the demo routers should be mounted: ``BURR_SERVER_DEMOS`` set to a truthy value.""" + return os.getenv("BURR_SERVER_DEMOS", "").strip().lower() in ("1", "true", "yes") + + SENTINEL_PARTITION_KEY = "__none__" backend = BackendBase.create_from_env() @@ -189,10 +203,12 @@ def _get_app_spec() -> BackendSpec: )(save_snapshot) -def create_burr_ui_app(serve_static: bool = SERVE_STATIC) -> FastAPI: +def create_burr_ui_app( + serve_static: bool = SERVE_STATIC, mount_demos: Optional[bool] = None +) -> FastAPI: """Create a fully-configured Burr UI FastAPI application. - This factory creates a new FastAPI instance with all Burr UI routes, + This factory creates a new FastAPI instance with all Burr UI routes, (optionally) the demo routers, and (optionally) static file serving configured. The app does not authenticate requests. Serve it on localhost (the default for the @@ -201,6 +217,9 @@ def create_burr_ui_app(serve_static: bool = SERVE_STATIC) -> FastAPI: :param serve_static: Whether to serve the React UI static files. Defaults to the BURR_SERVE_STATIC environment variable (true by default). + :param mount_demos: Whether to mount the demo routers (counter, chatbot, ...). Defaults + to the BURR_SERVER_DEMOS environment variable, read when the app is created; the + demos are off unless it is set to 1, true or yes. :return: A fully-configured FastAPI application. """ ui_app = FastAPI(lifespan=lifespan) @@ -363,12 +382,19 @@ async def version() -> dict: burr_version = "unknown" return {"version": burr_version} - # Examples -- todo -- put them behind `if` statements - ui_app.include_router(chatbot.router, prefix="/api/v0/chatbot") - ui_app.include_router(email_assistant.router, prefix="/api/v0/email_assistant") - ui_app.include_router(streaming_chatbot.router, prefix="/api/v0/streaming_chatbot") - ui_app.include_router(deep_researcher.router, prefix="/api/v0/deep_researcher") - ui_app.include_router(counter.router, prefix="/api/v0/counter") + # The demo routers are opt-in: each builds its own local tracking client on ~/.burr, + # so a plain `burr` server only mounts them when BURR_SERVER_DEMOS asks for it + # (`burr-demo` sets it). + if mount_demos is None: + mount_demos = _demos_enabled() + if mount_demos: + ui_app.include_router(chatbot.router, prefix="/api/v0/chatbot") + ui_app.include_router(email_assistant.router, prefix="/api/v0/email_assistant") + ui_app.include_router(streaming_chatbot.router, prefix="/api/v0/streaming_chatbot") + ui_app.include_router(deep_researcher.router, prefix="/api/v0/deep_researcher") + ui_app.include_router(counter.router, prefix="/api/v0/counter") + else: + logger.info("Demo routers are not mounted; set BURR_SERVER_DEMOS=true to enable them.") if serve_static: base_asset_directory = str(files("burr").joinpath("tracking/server/build")) @@ -433,6 +459,7 @@ def mount_burr_ui( path: str = "/burr", name: str = "burr-ui", serve_static: bool = SERVE_STATIC, + mount_demos: Optional[bool] = None, ) -> FastAPI: """Mount the Burr UI inside another FastAPI app. @@ -441,9 +468,11 @@ def mount_burr_ui( :param name: Name for the mounted sub-application. Defaults to "burr-ui". :param serve_static: Whether to serve the React UI static files. Defaults to the BURR_SERVE_STATIC environment variable (true by default). + :param mount_demos: Whether to mount the demo routers. Defaults to the + BURR_SERVER_DEMOS environment variable (off unless set to 1, true or yes). :return: The mounted Burr UI FastAPI app instance. """ - ui_app = create_burr_ui_app(serve_static=serve_static) + ui_app = create_burr_ui_app(serve_static=serve_static, mount_demos=mount_demos) parent_app.mount(path, ui_app, name=name) return ui_app diff --git a/tests/tracking/test_local_backend_paths.py b/tests/tracking/test_local_backend_paths.py index 58efdcbb6..c1f3974a9 100644 --- a/tests/tracking/test_local_backend_paths.py +++ b/tests/tracking/test_local_backend_paths.py @@ -18,15 +18,17 @@ """Identifier validation and path containment for the local tracking backend. Project and app identifiers arrive as URL path parameters and are used as path -components under the storage directory. These tests check that only plain -identifiers are accepted, that every resolved path stays inside the storage -directory, and that the server reports rejected identifiers as HTTP 400. +components under the storage directory. These tests check that an identifier has to +be a single path segment, that every resolved path (including the fixed filenames +inside an app directory) stays inside the storage directory, and that the server +reports rejected identifiers and paths as HTTP 400. """ import importlib import os import uuid from pathlib import Path +from urllib.parse import quote import pytest from fastapi import HTTPException @@ -82,6 +84,13 @@ def test_accepts_everything_the_client_produces(self): if not system.IS_WINDOWS: assert _validate_identifier("demo:chatbot", "project_id") == "demo:chatbot" + @pytest.mark.parametrize( + "value", ["user@example.com", "chat session 1", "héllo-wörld", "日本語", "a*b$c(d)"] + ) + def test_accepts_any_single_path_segment(self, value): + # ids like these already exist on disk for some projects; they are ordinary filenames + assert _validate_identifier(value, "app_id") == value + def test_invalid_identifiers(self): with pytest.raises(HTTPException) as exc: _validate_identifier("../etc/passwd") @@ -95,8 +104,11 @@ def test_invalid_identifiers(self): _validate_identifier("hello\\world") assert exc.value.status_code == 400 - @pytest.mark.parametrize("value", ["", ".", "..", " ", "a b", "/etc/passwd", "C:\\Windows"]) - def test_rejects_empty_dot_and_absolute_identifiers(self, value): + @pytest.mark.parametrize( + "value", + ["", ".", "..", "/etc/passwd", "C:\\Windows", "nul\x00byte", "tab\tbed", "trailing\n"], + ) + def test_rejects_empty_dot_absolute_and_control_identifiers(self, value): with pytest.raises(HTTPException) as exc: _validate_identifier(value, "project_id") assert exc.value.status_code == 400 @@ -242,7 +254,9 @@ def test_application_logs_reject_parent_directory(self, server): assert client.get(f"/api/v0/{PARENT}/app-1/__none__/apps").status_code == 400 assert client.get(f"/api/v0/proj/{PARENT}/__none__/apps").status_code == 400 - @pytest.mark.parametrize("project_id", ["..%5C..%5Cetc", "my%20project", "a" * 256]) + @pytest.mark.parametrize( + "project_id", ["..%5C..%5Cetc", "my%0Aproject", "my%00project", "a" * 256] + ) def test_rejects_other_malformed_identifiers(self, server, project_id): client, _ = server assert client.get(f"/api/v0/{project_id}/__none__/apps").status_code == 400 @@ -302,6 +316,58 @@ def test_accepts_identifiers_the_client_produces(self, server): assert response.status_code == 200 assert response.json()["applications"] == [] + @pytest.mark.parametrize("app_id", ["user@example.com", "chat session 1", "héllo-wörld"]) + def test_accepts_ids_that_are_any_single_path_segment(self, server, app_id): + client, storage = server + _make_app(storage, "proj", app_id) + + response = client.get("/api/v0/proj/__none__/apps") + assert response.status_code == 200 + assert [a["app_id"] for a in response.json()["applications"]] == [app_id] + + response = client.get(f"/api/v0/proj/{quote(app_id)}/__none__/apps") + assert response.status_code == 200 + assert response.json()["application"]["entrypoint"] == "counter" + + @pytest.mark.parametrize( + "filename", ["graph.json", "log.jsonl", "metadata.json", "children.jsonl"] + ) + def test_app_file_linked_outside_storage_is_rejected(self, server, tmp_path, filename): + client, storage = server + app_dir = _make_app(storage, "proj", "app-1") + outside = tmp_path / "outside.txt" + outside.write_text("not for the server") + target = app_dir / filename + if target.exists(): + target.unlink() + target.symlink_to(outside) + + response = client.get("/api/v0/proj/app-1/__none__/apps") + assert response.status_code == 400 + assert "not for the server" not in response.text + assert str(storage) not in response.json()["detail"] + + def test_app_files_inside_storage_are_served(self, server): + client, storage = server + app_dir = _make_app(storage, "proj", "app-1") + (app_dir / "children.jsonl").write_text("") + response = client.get("/api/v0/proj/app-1/__none__/apps") + assert response.status_code == 200 + assert response.json()["application"]["entrypoint"] == "counter" + + def test_listing_skips_app_whose_files_link_outside_storage(self, server, tmp_path): + client, storage = server + _make_app(storage, "proj", "good-app") + bad_dir = _make_app(storage, "proj", "bad-app") + outside = tmp_path / "outside.json" + outside.write_text("{}") + (bad_dir / "metadata.json").unlink() + (bad_dir / "metadata.json").symlink_to(outside) + + response = client.get("/api/v0/proj/__none__/apps") + assert response.status_code == 200 + assert [a["app_id"] for a in response.json()["applications"]] == ["good-app"] + def test_valid_but_missing_app_is_not_found(self, server): client, storage = server _make_app(storage, "proj", "app-1") diff --git a/tests/tracking/test_server_demos.py b/tests/tracking/test_server_demos.py new file mode 100644 index 000000000..cf0d4fc3c --- /dev/null +++ b/tests/tracking/test_server_demos.py @@ -0,0 +1,135 @@ +# 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. + +"""Mounting of the demo routers. + +The demo routers (counter, chatbot, ...) each build a local tracking client on ``~/.burr``, +so they are mounted only when ``BURR_SERVER_DEMOS`` asks for them: a plain ``burr`` server +exposes just the UI routes, and the ``burr-demo`` entry point turns the demos on. +""" + +import importlib + +import pytest +from click.testing import CliRunner + +COUNTER_STATE_ROUTE = "/api/v0/counter/state/{project_id}/{app_id}" + + +@pytest.fixture +def run(monkeypatch): + """The server module, imported with static serving off and a local backend configured.""" + monkeypatch.setenv("BURR_SERVE_STATIC", "false") + monkeypatch.setenv("BURR_BACKEND_IMPL", "burr.tracking.server.backend.LocalBackend") + return importlib.import_module("burr.tracking.server.run") + + +def _paths(app) -> set: + return {route.path for route in app.routes} + + +@pytest.mark.parametrize("value", ["1", "true", "TRUE", "yes", " Yes "]) +def test_demos_mounted_when_env_is_truthy(run, monkeypatch, value): + monkeypatch.setenv("BURR_SERVER_DEMOS", value) + app = run.create_burr_ui_app(serve_static=False) + assert COUNTER_STATE_ROUTE in _paths(app) + + +@pytest.mark.parametrize("value", [None, "", "0", "false", "no", "off"]) +def test_demos_not_mounted_otherwise(run, monkeypatch, value): + if value is None: + monkeypatch.delenv("BURR_SERVER_DEMOS", raising=False) + else: + monkeypatch.setenv("BURR_SERVER_DEMOS", value) + app = run.create_burr_ui_app(serve_static=False) + assert not any(path.startswith("/api/v0/counter") for path in _paths(app)) + assert "/api/v0/projects" in _paths(app) # the UI's own routes are unaffected + + +def test_explicit_argument_overrides_env(run, monkeypatch): + monkeypatch.delenv("BURR_SERVER_DEMOS", raising=False) + assert COUNTER_STATE_ROUTE in _paths( + run.create_burr_ui_app(serve_static=False, mount_demos=True) + ) + monkeypatch.setenv("BURR_SERVER_DEMOS", "true") + assert COUNTER_STATE_ROUTE not in _paths( + run.create_burr_ui_app(serve_static=False, mount_demos=False) + ) + + +def test_demo_route_is_404_without_env_and_present_with_it(run, monkeypatch): + from fastapi.testclient import TestClient + + monkeypatch.delenv("BURR_SERVER_DEMOS", raising=False) + client = TestClient(run.create_burr_ui_app(serve_static=False)) + assert client.get("/api/v0/counter/state/proj/app").status_code == 404 + assert client.get("/api/v0/projects").status_code == 200 + + monkeypatch.setenv("BURR_SERVER_DEMOS", "true") + client = TestClient(run.create_burr_ui_app(serve_static=False)) + # presence is checked through the schema so the demo's own storage is never touched + assert COUNTER_STATE_ROUTE in client.get("/openapi.json").json()["paths"] + + +def test_mount_burr_ui_passes_the_flag_through(run, monkeypatch): + from fastapi import FastAPI + + monkeypatch.delenv("BURR_SERVER_DEMOS", raising=False) + parent = FastAPI() + ui_app = run.mount_burr_ui(parent, serve_static=False) + assert COUNTER_STATE_ROUTE not in _paths(ui_app) + ui_app = run.mount_burr_ui(parent, path="/with-demos", serve_static=False, mount_demos=True) + assert COUNTER_STATE_ROUTE in _paths(ui_app) + + +class TestCli: + """``burr`` leaves the demos off; ``burr-demo`` turns them on through the environment.""" + + def test_run_server_sets_the_flag_only_on_request(self, monkeypatch): + cli_main = importlib.import_module("burr.cli.__main__") + captured = {} + + def fake_command(command, capture_output, addl_env=None): + captured.clear() + captured.update(addl_env or {}) + + monkeypatch.setattr(cli_main, "_command", fake_command) + # no_open and no_copy_demo_data keep the call from opening a browser or copying data + cli_main._run_server(7241, False, True, True) + assert "BURR_SERVER_DEMOS" not in captured + cli_main._run_server(7241, False, True, True, demos=True) + assert captured["BURR_SERVER_DEMOS"] == "true" + + def test_commands_are_wired_to_the_flag(self, monkeypatch): + cli_main = importlib.import_module("burr.cli.__main__") + calls = [] + monkeypatch.setattr(cli_main, "_run_server", lambda *args, **kwargs: calls.append(kwargs)) + + result = CliRunner().invoke(cli_main.cli, ["run-server", "--no-open"]) + assert result.exit_code == 0, result.output + assert calls[-1].get("demos", False) is False + + result = CliRunner().invoke(cli_main.cli, ["demo-server"]) + assert result.exit_code == 0, result.output + assert calls[-1]["demos"] is True + + def test_help_mentions_the_flag(self): + cli_main = importlib.import_module("burr.cli.__main__") + for command in ["run-server", "demo-server"]: + result = CliRunner().invoke(cli_main.cli, [command, "--help"]) + assert result.exit_code == 0, result.output + assert "BURR_SERVER_DEMOS" in result.output From 71f9ffab4016185bf84a9a198e472bfb771a28da Mon Sep 17 00:00:00 2001 From: Elijah Ben Izzy Date: Sat, 17 Oct 2026 08:25:02 -0700 Subject: [PATCH 4/4] test(tracking): make demo-router tests independent of FastAPI internals and the cli extra --- tests/tracking/test_server_demos.py | 78 +++++++++++++++++------------ 1 file changed, 47 insertions(+), 31 deletions(-) diff --git a/tests/tracking/test_server_demos.py b/tests/tracking/test_server_demos.py index cf0d4fc3c..8b5bda401 100644 --- a/tests/tracking/test_server_demos.py +++ b/tests/tracking/test_server_demos.py @@ -20,14 +20,21 @@ The demo routers (counter, chatbot, ...) each build a local tracking client on ``~/.burr``, so they are mounted only when ``BURR_SERVER_DEMOS`` asks for them: a plain ``burr`` server exposes just the UI routes, and the ``burr-demo`` entry point turns the demos on. + +Whether a demo router is mounted is checked over HTTP rather than by inspecting +``app.routes``, whose entries for included routers differ between FastAPI versions. """ import importlib import pytest -from click.testing import CliRunner +from fastapi.testclient import TestClient -COUNTER_STATE_ROUTE = "/api/v0/counter/state/{project_id}/{app_id}" +# A POST-only demo route. A GET on it is 404 when the router is not mounted and 405 (method +# not allowed) when it is, so the check never reaches the handler or touches the demo's +# storage directory. +COUNTER_CREATE = "/api/v0/counter/create/proj/app" +MOUNTED, NOT_MOUNTED = 405, 404 @pytest.fixture @@ -38,15 +45,14 @@ def run(monkeypatch): return importlib.import_module("burr.tracking.server.run") -def _paths(app) -> set: - return {route.path for route in app.routes} +def _demo_status(app) -> int: + return TestClient(app).get(COUNTER_CREATE).status_code @pytest.mark.parametrize("value", ["1", "true", "TRUE", "yes", " Yes "]) def test_demos_mounted_when_env_is_truthy(run, monkeypatch, value): monkeypatch.setenv("BURR_SERVER_DEMOS", value) - app = run.create_burr_ui_app(serve_static=False) - assert COUNTER_STATE_ROUTE in _paths(app) + assert _demo_status(run.create_burr_ui_app(serve_static=False)) == MOUNTED @pytest.mark.parametrize("value", [None, "", "0", "false", "no", "off"]) @@ -56,33 +62,35 @@ def test_demos_not_mounted_otherwise(run, monkeypatch, value): else: monkeypatch.setenv("BURR_SERVER_DEMOS", value) app = run.create_burr_ui_app(serve_static=False) - assert not any(path.startswith("/api/v0/counter") for path in _paths(app)) - assert "/api/v0/projects" in _paths(app) # the UI's own routes are unaffected + assert _demo_status(app) == NOT_MOUNTED + # the UI's own routes are unaffected + assert TestClient(app).get("/api/v0/projects").status_code == 200 def test_explicit_argument_overrides_env(run, monkeypatch): monkeypatch.delenv("BURR_SERVER_DEMOS", raising=False) - assert COUNTER_STATE_ROUTE in _paths( - run.create_burr_ui_app(serve_static=False, mount_demos=True) - ) + assert _demo_status(run.create_burr_ui_app(serve_static=False, mount_demos=True)) == MOUNTED monkeypatch.setenv("BURR_SERVER_DEMOS", "true") - assert COUNTER_STATE_ROUTE not in _paths( - run.create_burr_ui_app(serve_static=False, mount_demos=False) - ) - + app = run.create_burr_ui_app(serve_static=False, mount_demos=False) + assert _demo_status(app) == NOT_MOUNTED -def test_demo_route_is_404_without_env_and_present_with_it(run, monkeypatch): - from fastapi.testclient import TestClient +def test_demo_route_is_absent_without_env_and_listed_with_it(run, monkeypatch): monkeypatch.delenv("BURR_SERVER_DEMOS", raising=False) client = TestClient(run.create_burr_ui_app(serve_static=False)) - assert client.get("/api/v0/counter/state/proj/app").status_code == 404 - assert client.get("/api/v0/projects").status_code == 200 + assert client.get(COUNTER_CREATE).status_code == NOT_MOUNTED + assert ( + "/api/v0/counter/create/{project_id}/{app_id}" + not in client.get("/openapi.json").json()["paths"] + ) monkeypatch.setenv("BURR_SERVER_DEMOS", "true") client = TestClient(run.create_burr_ui_app(serve_static=False)) - # presence is checked through the schema so the demo's own storage is never touched - assert COUNTER_STATE_ROUTE in client.get("/openapi.json").json()["paths"] + assert client.get(COUNTER_CREATE).status_code == MOUNTED + assert ( + "/api/v0/counter/create/{project_id}/{app_id}" + in client.get("/openapi.json").json()["paths"] + ) def test_mount_burr_ui_passes_the_flag_through(run, monkeypatch): @@ -90,17 +98,23 @@ def test_mount_burr_ui_passes_the_flag_through(run, monkeypatch): monkeypatch.delenv("BURR_SERVER_DEMOS", raising=False) parent = FastAPI() - ui_app = run.mount_burr_ui(parent, serve_static=False) - assert COUNTER_STATE_ROUTE not in _paths(ui_app) - ui_app = run.mount_burr_ui(parent, path="/with-demos", serve_static=False, mount_demos=True) - assert COUNTER_STATE_ROUTE in _paths(ui_app) + run.mount_burr_ui(parent, path="/plain", serve_static=False) + run.mount_burr_ui(parent, path="/with-demos", serve_static=False, mount_demos=True) + client = TestClient(parent) + assert client.get("/plain" + COUNTER_CREATE).status_code == NOT_MOUNTED + assert client.get("/with-demos" + COUNTER_CREATE).status_code == MOUNTED class TestCli: """``burr`` leaves the demos off; ``burr-demo`` turns them on through the environment.""" - def test_run_server_sets_the_flag_only_on_request(self, monkeypatch): - cli_main = importlib.import_module("burr.cli.__main__") + @pytest.fixture + def cli_main(self): + # the CLI module needs the `cli` extra, which not every test environment installs + pytest.importorskip("loguru", reason="the cli extra is not installed") + return importlib.import_module("burr.cli.__main__") + + def test_run_server_sets_the_flag_only_on_request(self, cli_main, monkeypatch): captured = {} def fake_command(command, capture_output, addl_env=None): @@ -114,8 +128,9 @@ def fake_command(command, capture_output, addl_env=None): cli_main._run_server(7241, False, True, True, demos=True) assert captured["BURR_SERVER_DEMOS"] == "true" - def test_commands_are_wired_to_the_flag(self, monkeypatch): - cli_main = importlib.import_module("burr.cli.__main__") + def test_commands_are_wired_to_the_flag(self, cli_main, monkeypatch): + from click.testing import CliRunner + calls = [] monkeypatch.setattr(cli_main, "_run_server", lambda *args, **kwargs: calls.append(kwargs)) @@ -127,8 +142,9 @@ def test_commands_are_wired_to_the_flag(self, monkeypatch): assert result.exit_code == 0, result.output assert calls[-1]["demos"] is True - def test_help_mentions_the_flag(self): - cli_main = importlib.import_module("burr.cli.__main__") + def test_help_mentions_the_flag(self, cli_main): + from click.testing import CliRunner + for command in ["run-server", "demo-server"]: result = CliRunner().invoke(cli_main.cli, [command, "--help"]) assert result.exit_code == 0, result.output