diff --git a/burr/cli/__main__.py b/burr/cli/__main__.py index 8967602d9..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) @@ -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") diff --git a/burr/tracking/common/identifiers.py b/burr/tracking/common/identifiers.py new file mode 100644 index 000000000..4d5fe1a3e --- /dev/null +++ b/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 diff --git a/burr/tracking/server/backend.py b/burr/tracking/server/backend.py index 5e9e1c771..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 @@ -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( diff --git a/burr/tracking/server/run.py b/burr/tracking/server/run.py index da5b8b965..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,14 +203,23 @@ 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 + ``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). + :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) @@ -359,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")) @@ -429,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. @@ -437,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 @@ -450,4 +483,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="0.0.0.0", 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..c1f3974a9 --- /dev/null +++ b/tests/tracking/test_local_backend_paths.py @@ -0,0 +1,390 @@ +# 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 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 + +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" + + @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") + 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", + ["", ".", "..", "/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 + 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%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 + + 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"] == [] + + @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") + 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_server_demos.py b/tests/tracking/test_server_demos.py new file mode 100644 index 000000000..8b5bda401 --- /dev/null +++ b/tests/tracking/test_server_demos.py @@ -0,0 +1,151 @@ +# 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. + +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 fastapi.testclient import TestClient + +# 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 +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 _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) + assert _demo_status(run.create_burr_ui_app(serve_static=False)) == MOUNTED + + +@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 _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 _demo_status(run.create_burr_ui_app(serve_static=False, mount_demos=True)) == MOUNTED + monkeypatch.setenv("BURR_SERVER_DEMOS", "true") + app = run.create_burr_ui_app(serve_static=False, mount_demos=False) + assert _demo_status(app) == NOT_MOUNTED + + +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(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)) + 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): + from fastapi import FastAPI + + monkeypatch.delenv("BURR_SERVER_DEMOS", raising=False) + parent = FastAPI() + 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.""" + + @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): + 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, cli_main, monkeypatch): + from click.testing import CliRunner + + 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): + 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 + assert "BURR_SERVER_DEMOS" in result.output