From abcab63a439dfb7f5e7b7fa5321101be4a041471 Mon Sep 17 00:00:00 2001 From: Sujoy Datta Choudhury Date: Mon, 10 Aug 2026 14:37:44 -0700 Subject: [PATCH 1/5] feat(api): add scope-guarded profile write endpoints The HTTP surface can list, read, and validate agent profiles but cannot write them, so creating, editing, or deleting one still requires the CLI. This adds the mutating half of the surface a management UI needs, plus an authoring read that returns a profile exactly as stored. Four routes: POST /agents/profiles create from a supplied document PUT /agents/profiles/{name} replace an existing local profile DELETE /agents/profiles/{name} remove a local profile GET /agents/profiles/{name}/source raw document, placeholders intact Most of the diff sits below the handlers, because three of the four contracts these routes need are properties of the service layer, and each previously failed silently rather than loudly. replace_profile in services/profile_store.py is update-only persistence. write_profile(overwrite=True) is an upsert, so a PUT naming a built-in would have written a local file that shadows a shipped profile on load, which is the condition duplicated_in exists to report. replace_profile raises ProfileNotFoundError instead, and because store_path resolves only inside LOCAL_AGENT_STORE_DIR, that rejection happens at the service boundary under the lock rather than in a handler pre-check. locked_atomic_write gains must_exist, enforced in the same critical section as overwrite. Checking existence outside the lock would be the same TOCTOU shape that #543 removed from write_profile. The contradictory pair overwrite=False with must_exist=True raises ValueError so a caller bug fails fast instead of masquerading as FileExistsError. A shared helper, _validate_profile_for_write, backs both POST and PUT so the two cannot drift apart on either rule. It rejects any error-severity finding with 400 and returns warnings to the caller rather than blocking on them, and it requires the storage key and the frontmatter name to agree: parsing treats the filename stem only as a fallback, so "name: foo" stored as bar.md previously loaded as foo while being addressed as bar. Every rejection carries one detail shape, {"message", "errors"}, so a client iterates errors unconditionally instead of switching on the type of detail. The helper parses the frontmatter once and calls validate_frontmatter, rather than calling validate_profile_text and then parsing a second time for the name check. GET /agents/profiles/{name}/source exists because GET /agents/profiles/{name} applies resolve_env_vars to the raw text before parsing. An editor built on the resolved route would write substituted values back, persisting a resolved secret from the managed environment file into a plaintext profile. DELETE requires cao:admin alone, matching six of the seven existing DELETE routes; POST and PUT take cao:write or cao:admin like the other non-destructive writes. No route is added to the scope-exemption set, because these are real mutations, unlike the validate routes. Also fixes a P3 reported on #575. Three malformed-but-parseable documents raised TypeError out of profile_validator: an unhashable element in allowedTools and a non-string role, both hashed against a set for membership, and mixed-type mcpServers keys, compared while sorting schema errors by path. TypeError is not caught by the route's ValueError handler, so an endpoint whose job is reporting what is wrong with a document answered some invalid documents with HTTP 500. All three now return 200 with valid: false and the schema error attached. The coverage gap was that every malformed-input test used unparseable YAML and none used parseable-but-wrong-typed. Tests: 53 new across five files. Full suite 6,379 passed. --- docs/api.md | 25 ++ src/cli_agent_orchestrator/api/main.py | 265 +++++++++++++- .../services/profile_store.py | 49 +++ .../services/profile_validator.py | 22 +- .../utils/atomic_file.py | 21 +- test/api/test_api_profile_surface.py | 328 ++++++++++++++++++ test/api/test_scope_coverage.py | 49 +++ test/services/test_profile_store.py | 97 ++++++ test/services/test_profile_validator.py | 52 +++ test/utils/test_atomic_file.py | 74 ++++ 10 files changed, 977 insertions(+), 5 deletions(-) diff --git a/docs/api.md b/docs/api.md index 53364abd0..d551b47c0 100644 --- a/docs/api.md +++ b/docs/api.md @@ -60,6 +60,31 @@ See [AG-UI](agui.md) for enablement, event shapes, and privacy boundaries. client can render create and edit forms from the server's definition instead of duplicating the field list. - `POST /agents/profiles/install` installs a profile. +- `POST /agents/profiles` creates a profile in the local store from a supplied + document. Named distinctly from `install`, which takes a bare profile name or + an https:// URL rather than the document itself. The request carries `name` + and `content`; the two identities of a profile, its storage key and its + frontmatter `name`, must agree, so a mismatch is a 400 rather than a silent + rename. A conflicting name returns 409. Requires `cao:write` or `cao:admin`. +- `PUT /agents/profiles/{name}` replaces an existing local-store profile and + never creates one. A request naming a built-in or provider-managed profile + returns 404 rather than writing a local file that would shadow the original. + Requires `cao:write` or `cao:admin`. +- `DELETE /agents/profiles/{name}` removes a profile from the local store. + Requires `cao:admin`, matching the other destructive routes on this service; + a `cao:write` token that may create and edit profiles cannot delete them. + Built-ins are not deletable, for the same reason they are not replaceable. +- Both write routes run the profile validator on the exact submitted document + before persisting anything, so an invalid profile never reaches disk. Errors + reject the request with 400 and the findings attached; warnings do not block + the write and are returned in the response so a client can surface them after + a successful save. +- `GET /agents/profiles/{name}/source` returns a profile's document exactly as + stored. Use this, not `GET /agents/profiles/{name}`, when the document is + going to be edited and written back: that route returns the *resolved* + profile, having applied `${VAR}` substitution from the managed environment + file to the raw text before parsing. Round-tripping a resolved document + through a write would persist substituted values into a plaintext profile. - Template validation and preview require the selected template to include a `schema.json` file. - `/agents/providers` reports provider availability. diff --git a/src/cli_agent_orchestrator/api/main.py b/src/cli_agent_orchestrator/api/main.py index 278fc63bf..970a7d35a 100644 --- a/src/cli_agent_orchestrator/api/main.py +++ b/src/cli_agent_orchestrator/api/main.py @@ -14,7 +14,7 @@ from contextlib import asynccontextmanager from datetime import datetime, timezone from pathlib import Path -from typing import Annotated, Any, Dict, List, Literal, Optional, Tuple, cast +from typing import Annotated, Any, Dict, List, Literal, Optional, Sequence, Tuple, cast from fastapi import ( BackgroundTasks, @@ -675,6 +675,61 @@ class ProfileValidationResponse(BaseModel): messages: List[ProfileValidationMessage] = Field(default_factory=list) +class ProfileCreateRequest(BaseModel): + """Request body for ``POST /agents/profiles``. + + ``name`` is explicit rather than parsed out of ``content`` so the conflict + target is unambiguous even when the document is malformed. When the + frontmatter also declares a ``name`` the two must agree; see + ``_assert_frontmatter_name_matches``. + """ + + name: str = Field(description="Profile name, used as the local-store filename stem") + content: str = Field( + max_length=262_144, + description="Full profile markdown, including YAML frontmatter", + ) + + +class ProfileReplaceRequest(BaseModel): + """Request body for ``PUT /agents/profiles/{name}``. + + No ``name`` field: the path parameter is authoritative. Frontmatter that + declares a different name is rejected rather than silently renaming. + """ + + content: str = Field( + max_length=262_144, + description="Full profile markdown, including YAML frontmatter", + ) + + +class ProfileWriteResponse(BaseModel): + """Outcome of a profile create or replace. + + ``warnings`` carries advisory findings that did not block the write, so a + client can surface them after a successful save. Errors never reach here; + they reject the request with 400. + """ + + name: str + warnings: List[ProfileValidationMessage] = Field(default_factory=list) + + +class ProfileSourceResponse(BaseModel): + """A profile's document exactly as stored, with placeholders intact. + + Distinct from ``GET /agents/profiles/{name}``, which returns the *parsed and + resolved* profile. That response runs ``resolve_env_vars`` over the raw text + before parsing, so managed ``${VAR}`` placeholders come back as their + substituted values. Round-tripping that through a write would persist + resolved secrets into a plaintext profile, so an editor must read from here. + """ + + name: str + content: str + + class MemorySummary(BaseModel): """Memory list entry. Excludes file_path (absolute server filesystem path).""" @@ -1884,9 +1939,91 @@ async def get_agent_profile_schema_endpoint() -> Dict: return load_profile_schema() +def _validate_profile_for_write(name: str, content: str) -> List[ProfileValidationMessage]: + """Validate a submitted profile document and enforce name identity. + + Shared by ``POST /agents/profiles`` and ``PUT /agents/profiles/{name}`` so the + two cannot drift apart on either rule. + + Runs the same validator the CLI and ``POST /agents/profiles/validate`` use, on + the exact document being persisted rather than on a client-side approximation + of it. Error-severity findings reject the write; warnings are returned so a + client can surface them after a successful save. + + A profile has two identities: the storage key (its filename stem) and the + frontmatter ``name``. ``parse_agent_profile_text`` treats the stem only as a + fallback when frontmatter omits ``name``, so the two can diverge and nothing + reconciles them: ``name: foo`` in ``bar.md`` loads as ``foo`` while being + addressed as ``bar``. Requiring them to agree closes that without introducing + a rename operation, which has its own failure semantics. + + Args: + name: The storage name, authoritative. + content: The full profile document. + + Returns: + The warning-severity findings, if any. + + Raises: + HTTPException: 400 if the document is unparseable, carries an + error-severity finding, or declares a conflicting ``name``. Every + rejection uses the same ``detail`` shape, ``{"message", "errors"}``, + so a client parses one thing rather than switching on the type of + ``detail``. ``errors`` is empty for failures that are not per-field. + """ + import frontmatter + + from cli_agent_orchestrator.services.profile_validator import validate_frontmatter + + def _reject(message: str, findings: Sequence[Any] = ()) -> None: + raise HTTPException( + status_code=status.HTTP_400_BAD_REQUEST, + detail={ + "message": message, + "errors": [ + {"severity": f.severity, "message": f.message, "path": f.path} for f in findings + ], + }, + ) + + # Parsed once here, then handed to validate_frontmatter as metadata. + # validate_profile_text would parse it again: its docstring exists precisely + # to keep callers from duplicating the parse, and this function needs the + # metadata anyway for the name check below. + try: + parsed = frontmatter.loads(content) + except Exception as exc: + _reject(f"Profile could not be parsed and was not written: {exc}") + + findings = validate_frontmatter(parsed.metadata) + + errors = [f for f in findings if f.severity == "error"] + if errors: + _reject("Profile failed validation and was not written.", errors) + + declared = parsed.metadata.get("name") + if isinstance(declared, str) and declared != name: + _reject( + f"Frontmatter name '{declared}' does not match the profile name " + f"'{name}'. They must agree; renaming a profile is not supported " + f"through this endpoint." + ) + + return [ + ProfileValidationMessage(severity=f.severity, message=f.message, path=f.path) + for f in findings + if f.severity == "warning" + ] + + @app.get("/agents/profiles/{name}") async def get_agent_profile_endpoint(name: str) -> Dict: - """Return the full parsed content of a named agent profile.""" + """Return the full parsed content of a named agent profile. + + Note this response is *resolved*: ``load_agent_profile`` applies + ``resolve_env_vars`` before parsing. Use ``GET /agents/profiles/{name}/source`` + when the document is going to be edited and written back. + """ try: profile = load_agent_profile(name) return profile.model_dump(exclude_none=True) @@ -1922,6 +2059,130 @@ async def install_agent_profile_endpoint( return result +@app.post("/agents/profiles", status_code=status.HTTP_201_CREATED) +async def create_agent_profile_endpoint( + request: ProfileCreateRequest, + _scopes: List[str] = Depends(require_any_scope(SCOPE_WRITE, SCOPE_ADMIN)), +) -> ProfileWriteResponse: + """Create a profile in the local store from a supplied document. + + Named distinctly from ``POST /agents/profiles/install``, which installs from a + bare name or an https:// URL. This one takes the document itself in the body. + + Validation runs on the exact submitted content before anything is persisted, + so an invalid profile never reaches disk. Conflict detection is delegated to + ``write_profile(overwrite=False)``, which checks for an existing file inside + the write lock; a pre-check here would sit outside that critical section and + let two concurrent creators both succeed. + """ + from cli_agent_orchestrator.services.profile_store import ( + InvalidProfileNameError, + ProfileExistsError, + write_profile, + ) + + warnings = _validate_profile_for_write(request.name, request.content) + + try: + write_profile(request.name, request.content, overwrite=False) + except InvalidProfileNameError as exc: + raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail=str(exc)) + except ProfileExistsError as exc: + raise HTTPException(status_code=status.HTTP_409_CONFLICT, detail=str(exc)) + + return ProfileWriteResponse(name=request.name, warnings=warnings) + + +@app.put("/agents/profiles/{name}") +async def replace_agent_profile_endpoint( + name: str, + request: ProfileReplaceRequest, + _scopes: List[str] = Depends(require_any_scope(SCOPE_WRITE, SCOPE_ADMIN)), +) -> ProfileWriteResponse: + """Replace an existing local-store profile. Never creates one. + + Backed by ``replace_profile``, which requires the target to exist *inside* the + write lock. That is what makes a PUT naming a built-in or provider-managed + profile a 404 rather than a silent create: the local store is the only place + this resolves, so a built-in's name is simply not there. An upsert would + instead write a local file that shadows the built-in on load, manufacturing + exactly the condition ``duplicated_in`` exists to report. + """ + from cli_agent_orchestrator.services.profile_store import ( + InvalidProfileNameError, + ProfileNotFoundError, + replace_profile, + ) + + warnings = _validate_profile_for_write(name, request.content) + + try: + replace_profile(name, request.content) + except InvalidProfileNameError as exc: + raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail=str(exc)) + except ProfileNotFoundError as exc: + raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail=str(exc)) + + return ProfileWriteResponse(name=name, warnings=warnings) + + +@app.delete("/agents/profiles/{name}", status_code=status.HTTP_204_NO_CONTENT) +async def delete_agent_profile_endpoint( + name: str, + _scopes: List[str] = Depends(require_any_scope(SCOPE_ADMIN)), +) -> None: + """Delete a profile from the local store. + + ``SCOPE_ADMIN`` alone rather than write-or-admin, matching every other + destructive route on this service (``/sessions``, ``/workflows``, + ``/terminals``, ``/flows``, ``/memory``). Deletion is the only irreversible + operation in this group, and the asymmetry with POST and PUT is the existing + convention rather than a new one. + + Built-in and provider-managed profiles are not deletable for the same reason + they are not replaceable: ``delete_profile`` resolves only inside the local + store, so their names raise ``ProfileNotFoundError``. + """ + from cli_agent_orchestrator.services.profile_store import ( + InvalidProfileNameError, + ProfileNotFoundError, + delete_profile, + ) + + try: + delete_profile(name) + except InvalidProfileNameError as exc: + raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail=str(exc)) + except ProfileNotFoundError as exc: + raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail=str(exc)) + + +@app.get("/agents/profiles/{name}/source") +async def get_agent_profile_source_endpoint(name: str) -> ProfileSourceResponse: + """Return a profile's document exactly as stored, unresolved. + + The authoring counterpart to ``GET /agents/profiles/{name}``. That route calls + ``load_agent_profile``, which applies ``resolve_env_vars`` to the raw text + *before* parsing, so substitution reaches the Markdown body as well as the + frontmatter, and the substitution source is the managed CAO ``.env`` file. + Using that response to pre-fill an editor and then PUT it back would persist + resolved secret values into a plaintext profile. ``safe_substitute`` leaves + unset variables intact, which would make the damage selective and silent. + + Reads across all configured stores, not only the local one, so a built-in can + be fetched as the starting point for a clone. Writing it back still requires + the local store, which is enforced by the write routes. + """ + from cli_agent_orchestrator.utils.agent_profiles import _read_agent_profile_source + + try: + return ProfileSourceResponse(name=name, content=_read_agent_profile_source(name)) + except FileNotFoundError as exc: + raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail=str(exc)) + except ValueError as exc: + raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail=str(exc)) + + @app.get("/agents/providers") async def list_providers_endpoint() -> List[Dict]: """List available providers with installation status.""" diff --git a/src/cli_agent_orchestrator/services/profile_store.py b/src/cli_agent_orchestrator/services/profile_store.py index 87f5d570a..45b401c6f 100644 --- a/src/cli_agent_orchestrator/services/profile_store.py +++ b/src/cli_agent_orchestrator/services/profile_store.py @@ -140,6 +140,55 @@ def write_profile(name: str, content: str, *, overwrite: bool = False) -> Path: return target +def replace_profile(name: str, content: str) -> Path: + """Replace an existing local-store profile. Never creates one. + + The update-only counterpart to :func:`write_profile`. ``write_profile`` with + ``overwrite=True`` is an upsert, which is wrong for an HTTP ``PUT``: a request + naming a built-in or provider-managed profile would not fail, it would create a + *new* local file that shadows the original, silently changing which profile + wins on load. That is exactly the shadowing the ``duplicated_in`` field exists + to surface, so an upsert would manufacture the condition we warn about. + + Because this module resolves only inside ``LOCAL_AGENT_STORE_DIR``, a built-in's + name is simply not present here, so requiring the target to exist rejects + writes against built-ins at the service boundary rather than by a check in the + caller. + + The existence requirement is enforced *inside* the write lock. A caller testing + for the file first would leave a window where a concurrent delete turns the + intended update back into a create. + + Args: + name: Profile name, used as the filename stem. + content: Full profile text (frontmatter + body). + + Returns: + The path written. + + Raises: + InvalidProfileNameError: If ``name`` is not a safe single segment. + ProfileNotFoundError: If the profile is not in the local store. + """ + # Inline guard: see _PROFILE_NAME_RE. + if not _PROFILE_NAME_RE.fullmatch(name): + raise InvalidProfileNameError(f"Profile name '{name}' must match [A-Za-z0-9_-]{{1,64}}.") + root = LOCAL_AGENT_STORE_DIR.resolve() + target = (LOCAL_AGENT_STORE_DIR / f"{name}.md").resolve() + if not target.is_relative_to(root): + raise InvalidProfileNameError(f"Profile name '{name}' escapes the local store.") + + try: + locked_atomic_write(target, content, overwrite=True, must_exist=True) + except FileNotFoundError as exc: + raise ProfileNotFoundError( + f"Profile '{name}' is not in the local store, so there is nothing to " + f"replace. Built-in and provider-managed profiles are not writable; " + f"copy one into the local store first." + ) from exc + return target + + def delete_profile(name: str) -> None: """Delete the profile ``name`` from the local store. diff --git a/src/cli_agent_orchestrator/services/profile_validator.py b/src/cli_agent_orchestrator/services/profile_validator.py index 2551e6904..0e13c6e64 100644 --- a/src/cli_agent_orchestrator/services/profile_validator.py +++ b/src/cli_agent_orchestrator/services/profile_validator.py @@ -93,15 +93,30 @@ def validate_frontmatter(metadata: dict) -> list[ValidationMessage]: ) # 2. JSON-Schema structural validation. + # + # The sort key stringifies each path component. Raw components are whatever + # the document used as mapping keys, so a profile with mixed-type keys (for + # example ``mcpServers: {1: {}, x: {}}``) yields paths that cannot be ordered + # against each other and would raise TypeError mid-sort. Such a document is + # already schema-invalid; it must be *reported* as invalid rather than crash + # the validator. validator = Draft202012Validator(load_profile_schema()) - for error in sorted(validator.iter_errors(metadata), key=lambda e: list(e.path)): + for error in sorted(validator.iter_errors(metadata), key=lambda e: [str(p) for p in e.path]): path = ".".join(str(p) for p in error.absolute_path) or "(root)" messages.append(ValidationMessage("error", error.message, path)) # 3. allowedTools vocabulary check (advisory, not blocking). + # + # Each entry is type-checked before the membership test. ``_VALID_TOOL_VOCAB`` + # is a set, so ``tool not in`` hashes ``tool``, and an unhashable element + # (``allowedTools: [[Read]]``) would raise TypeError. The schema already + # rejects a non-string entry, so this check only has to avoid crashing on + # input the caller will be told about anyway. allowed = metadata.get("allowedTools") if allowed and isinstance(allowed, list): for tool in allowed: + if not isinstance(tool, str): + continue if tool not in _VALID_TOOL_VOCAB: messages.append( ValidationMessage( @@ -112,8 +127,11 @@ def validate_frontmatter(metadata: dict) -> list[ValidationMessage]: ) # 4. Role check (advisory — custom roles are valid but worth flagging). + # + # Same hashing hazard as above: ``role: [developer]`` is unhashable. The + # schema reports the type error, so this advisory check simply stands aside. role = metadata.get("role") - if role and role not in _BUILTIN_ROLES: + if isinstance(role, str) and role and role not in _BUILTIN_ROLES: messages.append( ValidationMessage( "warning", diff --git a/src/cli_agent_orchestrator/utils/atomic_file.py b/src/cli_agent_orchestrator/utils/atomic_file.py index 2e03ed092..3869e1adb 100644 --- a/src/cli_agent_orchestrator/utils/atomic_file.py +++ b/src/cli_agent_orchestrator/utils/atomic_file.py @@ -248,6 +248,7 @@ def locked_atomic_write( encoding: str = "utf-8", lock_timeout: float = DEFAULT_LOCK_TIMEOUT_SECONDS, overwrite: bool = True, + must_exist: bool = False, ) -> None: """Replace ``target``'s entire contents atomically and safely across processes. @@ -274,22 +275,40 @@ def locked_atomic_write( observe an absent file. Callers must not pre-check with ``target.exists()`` themselves: that test would sit outside this critical section and reintroduce the race. + must_exist: When True, refuse to create a missing target, so the write + is an update and never an insert. Checked inside the same lock and + for the same reason: a caller testing existence beforehand would let + a concurrent delete slip between the check and the write, turning an + intended update back into a create. ``overwrite=False`` with + ``must_exist=True`` is contradictory and raises ``ValueError``. Raises: FileExistsError: If ``target`` exists and ``overwrite`` is False. + FileNotFoundError: If ``target`` is absent and ``must_exist`` is True. + ValueError: If ``overwrite`` is False and ``must_exist`` is True. LockTimeoutError: If the lock is not acquired within ``lock_timeout`` seconds. OSError: Propagated from filesystem operations (write, fsync, replace). """ + if not overwrite and must_exist: + raise ValueError( + "overwrite=False with must_exist=True can never succeed: it demands a " + "target that exists and refuses to replace it." + ) + target.parent.mkdir(parents=True, exist_ok=True) lock_path = _lock_path_for(target) with _file_lock(lock_path, lock_timeout): # Checked HERE, not by the caller: an exists() test outside this # critical section lets two concurrent creators both see "absent" and - # both write, so the second silently clobbers the first. + # both write, so the second silently clobbers the first. The same + # applies in reverse to must_exist, where a concurrent delete between + # an external check and the write would turn an update into a create. if not overwrite and target.exists(): raise FileExistsError(f"{target} already exists") + if must_exist and not target.exists(): + raise FileNotFoundError(f"{target} does not exist") _atomic_publish(target, content, encoding) diff --git a/test/api/test_api_profile_surface.py b/test/api/test_api_profile_surface.py index 340482274..5900e49e1 100644 --- a/test/api/test_api_profile_surface.py +++ b/test/api/test_api_profile_surface.py @@ -474,3 +474,331 @@ def test_is_not_shadowed_by_the_name_route(self, client) -> None: assert response.status_code == 200 assert not mock_load.called assert "properties" in response.json() + + +class TestValidateEndpointOnMalformedInput: + """The endpoint must diagnose bad documents, not 500 on them. + + Regression guard for the P3 finding on #575: these three shapes raised + ``TypeError`` inside the handler, which is not caught by its ``except + ValueError``, so the client received HTTP 500 instead of the findings it + asked for. Asserting the status explicitly is the point of these tests. + """ + + def test_unhashable_allowed_tools_entry_returns_findings(self, client) -> None: + content = "---\nname: x\nallowedTools:\n - [Read]\n---\n\nBody.\n" + + response = client.post("/agents/profiles/validate", json={"content": content}) + + assert response.status_code == 200 + body = response.json() + assert body["valid"] is False + assert any(m["severity"] == "error" for m in body["messages"]) + + def test_unhashable_role_returns_findings(self, client) -> None: + content = "---\nname: x\nrole:\n - developer\n---\n\nBody.\n" + + response = client.post("/agents/profiles/validate", json={"content": content}) + + assert response.status_code == 200 + assert response.json()["valid"] is False + + def test_mixed_type_mapping_keys_return_findings(self, client) -> None: + content = "---\nname: x\nmcpServers:\n 1: {}\n x: {}\n---\n\nBody.\n" + + response = client.post("/agents/profiles/validate", json={"content": content}) + + assert response.status_code == 200 + assert response.json()["valid"] is False + + +# -------------------------------------------------------------------------- +# Write endpoints (POST / PUT / DELETE) and the authoring read +# -------------------------------------------------------------------------- + +VALID_PROFILE = "---\nname: {name}\ndescription: A test profile.\n---\n\nYou are a test agent.\n" + + +@pytest.fixture() +def write_store(tmp_path, monkeypatch): + """Point the local profile store at a tmp dir for the write routes. + + Both module references must be patched. ``profile_store`` and + ``agent_profiles`` each import ``LOCAL_AGENT_STORE_DIR`` by value, so the + write routes read one copy and the source route reads the other. Patching + only ``profile_store`` leaves the source route resolving against the real + store on the developer's machine. + """ + from cli_agent_orchestrator.services import profile_store + from cli_agent_orchestrator.utils import agent_profiles + + target = tmp_path / "agent-store" + monkeypatch.setattr(profile_store, "LOCAL_AGENT_STORE_DIR", target) + monkeypatch.setattr(agent_profiles, "LOCAL_AGENT_STORE_DIR", target) + return target + + +class TestCreateAgentProfileEndpoint: + """POST /agents/profiles -- create from a supplied document.""" + + def test_creates_a_profile_and_returns_201(self, client, write_store) -> None: + response = client.post( + "/agents/profiles", + json={"name": "fresh", "content": VALID_PROFILE.format(name="fresh")}, + ) + + assert response.status_code == 201 + assert response.json()["name"] == "fresh" + assert (write_store / "fresh.md").exists() + + def test_conflicting_name_returns_409(self, client, write_store) -> None: + """Conflict is detected inside the write lock, not by a pre-check.""" + body = {"name": "dupe", "content": VALID_PROFILE.format(name="dupe")} + assert client.post("/agents/profiles", json=body).status_code == 201 + + assert client.post("/agents/profiles", json=body).status_code == 409 + + def test_invalid_profile_is_rejected_and_not_written(self, client, write_store) -> None: + """Validation runs before persistence, so nothing reaches disk.""" + content = "---\nname: bad\nengine: v3\n---\n\nBody.\n" + + response = client.post("/agents/profiles", json={"name": "bad", "content": content}) + + assert response.status_code == 400 + assert not (write_store / "bad.md").exists() + assert response.json()["detail"]["errors"] + + def test_frontmatter_name_mismatch_is_rejected(self, client, write_store) -> None: + """The storage name and the frontmatter name must agree. + + Without this the two silently diverge: the profile loads under its + frontmatter name while being addressed by its filename stem. + """ + content = VALID_PROFILE.format(name="something-else") + + response = client.post("/agents/profiles", json={"name": "declared", "content": content}) + + assert response.status_code == 400 + assert "does not match" in response.json()["detail"]["message"] + assert not (write_store / "declared.md").exists() + + def test_unsafe_name_is_rejected(self, client, write_store) -> None: + content = "---\nname: ok\n---\n\nBody.\n" + + response = client.post("/agents/profiles", json={"name": "../escape", "content": content}) + + assert response.status_code == 400 + + def test_warnings_do_not_block_the_write(self, client, write_store) -> None: + """A warning-only profile is written, with the warnings returned. + + This is the block/allow contract: only errors reject a save. + """ + content = "---\nname: warned\nrole: archaeologist\n---\n\nBody.\n" + + response = client.post("/agents/profiles", json={"name": "warned", "content": content}) + + assert response.status_code == 201 + assert response.json()["warnings"] + assert (write_store / "warned.md").exists() + + def test_oversized_content_is_rejected_by_the_model(self, client, write_store) -> None: + response = client.post("/agents/profiles", json={"name": "big", "content": "x" * 262_145}) + + assert response.status_code == 422 + + +class TestReplaceAgentProfileEndpoint: + """PUT /agents/profiles/{name} -- update only, never insert.""" + + def test_replaces_an_existing_profile(self, client, write_store) -> None: + client.post( + "/agents/profiles", + json={"name": "target", "content": VALID_PROFILE.format(name="target")}, + ) + updated = "---\nname: target\ndescription: Updated.\n---\n\nNew body.\n" + + response = client.put("/agents/profiles/target", json={"content": updated}) + + assert response.status_code == 200 + assert "New body." in (write_store / "target.md").read_text(encoding="utf-8") + + def test_missing_profile_returns_404_and_creates_nothing(self, client, write_store) -> None: + content = VALID_PROFILE.format(name="ghost") + + response = client.put("/agents/profiles/ghost", json={"content": content}) + + assert response.status_code == 404 + assert not (write_store / "ghost.md").exists() + + def test_built_in_profile_cannot_be_shadowed(self, client, write_store) -> None: + """A PUT naming a built-in must 404, not create a shadowing local file. + + ``code_supervisor`` ships with the package. An upsert would write a local + file of the same name that wins on load, which is the condition + ``duplicated_in`` exists to report. + """ + content = VALID_PROFILE.format(name="code_supervisor") + + response = client.put("/agents/profiles/code_supervisor", json={"content": content}) + + assert response.status_code == 404 + assert not (write_store / "code_supervisor.md").exists() + + def test_frontmatter_name_mismatch_is_rejected(self, client, write_store) -> None: + client.post( + "/agents/profiles", + json={"name": "keeper", "content": VALID_PROFILE.format(name="keeper")}, + ) + + response = client.put( + "/agents/profiles/keeper", json={"content": VALID_PROFILE.format(name="renamed")} + ) + + assert response.status_code == 400 + assert "does not match" in response.json()["detail"]["message"] + + def test_invalid_profile_does_not_overwrite(self, client, write_store) -> None: + client.post( + "/agents/profiles", + json={"name": "guarded", "content": VALID_PROFILE.format(name="guarded")}, + ) + original = (write_store / "guarded.md").read_text(encoding="utf-8") + + response = client.put( + "/agents/profiles/guarded", + json={"content": "---\nname: guarded\nengine: v3\n---\n\nBody.\n"}, + ) + + assert response.status_code == 400 + assert (write_store / "guarded.md").read_text(encoding="utf-8") == original + + +class TestDeleteAgentProfileEndpoint: + """DELETE /agents/profiles/{name} -- local store only.""" + + def test_deletes_an_existing_profile(self, client, write_store) -> None: + client.post( + "/agents/profiles", + json={"name": "doomed", "content": VALID_PROFILE.format(name="doomed")}, + ) + + response = client.delete("/agents/profiles/doomed") + + assert response.status_code == 204 + assert not (write_store / "doomed.md").exists() + + def test_missing_profile_returns_404(self, client, write_store) -> None: + assert client.delete("/agents/profiles/never-existed").status_code == 404 + + def test_built_in_profile_cannot_be_deleted(self, client, write_store) -> None: + """Built-ins are not in the local store, so they are not deletable.""" + assert client.delete("/agents/profiles/code_supervisor").status_code == 404 + + def test_unsafe_name_is_rejected(self, client, write_store) -> None: + """A single-segment unsafe name reaches the handler and is rejected there. + + An encoded traversal such as ``..%2Fescape`` never gets this far: the URL + normalises to a different path and routing answers 405, so it does not + exercise the name guard. + """ + assert client.delete("/agents/profiles/bad@name").status_code == 400 + + +class TestAgentProfileSourceEndpoint: + """GET /agents/profiles/{name}/source -- unresolved authoring read.""" + + def test_returns_the_document_as_stored(self, client, write_store) -> None: + content = VALID_PROFILE.format(name="sourced") + client.post("/agents/profiles", json={"name": "sourced", "content": content}) + + response = client.get("/agents/profiles/sourced/source") + + assert response.status_code == 200 + assert response.json()["content"] == content + + def test_placeholders_are_not_resolved(self, client, write_store) -> None: + """The whole point of this route. + + ``GET /agents/profiles/{name}`` runs resolve_env_vars over the raw text + before parsing, so a managed variable would come back substituted and an + edit round-trip would persist the resolved value. Here the placeholder + must survive verbatim. + """ + content = "---\nname: templated\ndescription: Uses a variable.\n---\n\nToken: ${MY_TOKEN}\n" + client.post("/agents/profiles", json={"name": "templated", "content": content}) + + response = client.get("/agents/profiles/templated/source") + + assert "${MY_TOKEN}" in response.json()["content"] + + def test_is_not_shadowed_by_the_name_route(self, client, write_store) -> None: + """Route-ordering guard. + + ``GET /agents/profiles/{name}`` is declared first. It must not capture + ``foo/source`` as a profile named "foo/source", and this route must not be + served by the parsed-profile handler. + """ + client.post( + "/agents/profiles", + json={"name": "distinct", "content": VALID_PROFILE.format(name="distinct")}, + ) + + source = client.get("/agents/profiles/distinct/source").json() + parsed = client.get("/agents/profiles/distinct").json() + + assert set(source) == {"name", "content"} + assert "system_prompt" in parsed + + def test_missing_profile_returns_404(self, client, write_store) -> None: + assert client.get("/agents/profiles/absent/source").status_code == 404 + + +class TestWriteRejectionShape: + """Every 400 from a write route uses one ``detail`` shape. + + A client should not have to switch on ``type(detail)``. Before this was + unified, a schema failure returned a dict while a name mismatch and a parse + failure returned bare strings, from the same endpoint. + """ + + REJECTIONS = { + "schema error": {"name": "x", "content": "---\nname: x\nengine: v3\n---\n\nB.\n"}, + "name mismatch": {"name": "x", "content": "---\nname: other\n---\n\nB.\n"}, + "unparseable": {"name": "x", "content": "---\nname: [unclosed\n b: : y\n---\n\nB.\n"}, + "missing name": {"name": "x", "content": "---\ndescription: none\n---\n\nB.\n"}, + } + + def test_every_rejection_has_the_same_detail_shape(self, client, write_store) -> None: + for label, body in self.REJECTIONS.items(): + response = client.post("/agents/profiles", json=body) + + assert response.status_code == 400, label + detail = response.json()["detail"] + assert isinstance(detail, dict), f"{label}: detail was {type(detail).__name__}" + assert set(detail) == {"message", "errors"}, label + assert isinstance(detail["message"], str), label + assert isinstance(detail["errors"], list), label + + def test_field_level_failures_carry_a_path(self, client, write_store) -> None: + """A schema failure must say which field, so a form can render it.""" + response = client.post( + "/agents/profiles", + json={"name": "x", "content": "---\nname: x\nengine: v3\n---\n\nB.\n"}, + ) + + errors = response.json()["detail"]["errors"] + assert errors + assert any(e["path"] == "engine" for e in errors) + + def test_non_field_failures_carry_an_empty_error_list(self, client, write_store) -> None: + """A parse failure is not attributable to a field, so ``errors`` is empty. + + The key is still present, so a client can iterate it unconditionally. + """ + response = client.post( + "/agents/profiles", + json={"name": "x", "content": "---\nname: [unclosed\n b: : y\n---\n\nB.\n"}, + ) + + assert response.json()["detail"]["errors"] == [] diff --git a/test/api/test_scope_coverage.py b/test/api/test_scope_coverage.py index d636c39a5..d4ce620f0 100644 --- a/test/api/test_scope_coverage.py +++ b/test/api/test_scope_coverage.py @@ -189,3 +189,52 @@ def test_write_token_still_admitted_on_run_list(client, auth_on): app.dependency_overrides[auth.get_current_scopes] = _override_scopes([auth.SCOPE_WRITE]) resp = client.get("/workflows/runs") assert resp.status_code != 403 + + +# -------------------------------------------------------------------------- +# Profile write routes (#510 PR B) +# +# Asserts the deliberate asymmetry: POST and PUT accept cao:write, DELETE does +# not. DELETE is admin-only to match every other destructive route on this +# service (/sessions, /workflows, /terminals, /flows, /memory), since it is the +# only irreversible operation in the profile group. +# -------------------------------------------------------------------------- + + +def test_read_token_forbidden_on_profile_create(client, auth_on): + """A cao:read token cannot create a profile.""" + app.dependency_overrides[auth.get_current_scopes] = _override_scopes([auth.SCOPE_READ]) + resp = client.post("/agents/profiles", json={"name": "x", "content": "---\nname: x\n---\n"}) + assert resp.status_code == 403 + + +def test_write_token_admitted_on_profile_create(client, auth_on): + """A cao:write token passes the create dependency.""" + app.dependency_overrides[auth.get_current_scopes] = _override_scopes([auth.SCOPE_WRITE]) + resp = client.post("/agents/profiles", json={"name": "x", "content": "---\nname: x\n---\n"}) + assert resp.status_code != 403 + + +def test_write_token_admitted_on_profile_replace(client, auth_on): + """A cao:write token passes the replace dependency.""" + app.dependency_overrides[auth.get_current_scopes] = _override_scopes([auth.SCOPE_WRITE]) + resp = client.put("/agents/profiles/x", json={"content": "---\nname: x\n---\n"}) + assert resp.status_code != 403 + + +def test_write_token_forbidden_on_profile_delete(client, auth_on): + """A cao:write token is 403'd on profile deletion. + + This is the point of choosing admin-only for DELETE: a token that may create + and edit profiles must not be able to remove them. + """ + app.dependency_overrides[auth.get_current_scopes] = _override_scopes([auth.SCOPE_WRITE]) + resp = client.delete("/agents/profiles/x") + assert resp.status_code == 403 + + +def test_admin_token_admitted_on_profile_delete(client, auth_on): + """A cao:admin token passes the deletion dependency.""" + app.dependency_overrides[auth.get_current_scopes] = _override_scopes([auth.SCOPE_ADMIN]) + resp = client.delete("/agents/profiles/x") + assert resp.status_code != 403 diff --git a/test/services/test_profile_store.py b/test/services/test_profile_store.py index a613b5054..29dc4b51b 100644 --- a/test/services/test_profile_store.py +++ b/test/services/test_profile_store.py @@ -244,3 +244,100 @@ def test_delete_profile_does_not_follow_a_name_out_of_the_store( with pytest.raises((InvalidProfileNameError, ProfileNotFoundError)): delete_profile("../outsider") assert outsider.exists() + + +# -------------------------------------------------------------------------- +# replace_profile +# -------------------------------------------------------------------------- + + +def test_replace_profile_updates_an_existing_profile(store: Path) -> None: + profile_store.write_profile("agent", "original\n") + + written = profile_store.replace_profile("agent", "updated\n") + + assert written.read_text(encoding="utf-8") == "updated\n" + + +def test_replace_profile_refuses_to_create_a_missing_profile(store: Path) -> None: + """The whole point of the function: update-only, never insert. + + ``write_profile(..., overwrite=True)`` is an upsert, which is wrong for an + HTTP PUT. Requiring the target to exist is what stops a PUT from creating a + file at all. + """ + with pytest.raises(profile_store.ProfileNotFoundError): + profile_store.replace_profile("never-installed", "content\n") + + assert not (store / "never-installed.md").exists() + + +def test_replace_profile_will_not_shadow_a_built_in(store: Path) -> None: + """A built-in's name is not in the local store, so PUT must reject it. + + ``code_supervisor`` ships in ``cli_agent_orchestrator/agent_store``. An upsert + would create a *local* file of the same name that wins on load, silently + shadowing the built-in. That is precisely the condition ``duplicated_in`` + exists to report, so it must not be manufacturable through the write path. + """ + with pytest.raises(profile_store.ProfileNotFoundError): + profile_store.replace_profile("code_supervisor", "hijacked\n") + + assert not (store / "code_supervisor.md").exists() + + +def test_replace_profile_rejects_an_unsafe_name_before_touching_disk(store: Path) -> None: + with pytest.raises(profile_store.InvalidProfileNameError): + profile_store.replace_profile("../escape", "content\n") + + assert not store.exists() + + +def test_replace_profile_can_replace_a_corrupt_store_file(store: Path) -> None: + """Undecodable bytes must not make an existing profile unrepairable. + + Same property ``write_profile`` has, for the same reason: the write path must + not read the old content first. + """ + store.mkdir(parents=True, exist_ok=True) + target = store / "agent.md" + target.write_bytes(b"\xff\xfe not utf-8 at all") + + profile_store.replace_profile("agent", "clean\n") + + assert target.read_text(encoding="utf-8") == "clean\n" + + +def test_replace_profile_lets_exactly_one_concurrent_deleter_or_writer_win(store: Path) -> None: + """The existence requirement holds under contention, not just serially. + + Two threads race to replace the same profile after it is deleted. Neither may + succeed by creating the file, because the check lives inside the lock. + """ + import threading + + profile_store.write_profile("agent", "original\n") + profile_store.delete_profile("agent") + + barrier = threading.Barrier(2) + outcomes: list[str] = [] + lock = threading.Lock() + + def attempt(label: str) -> None: + barrier.wait() + try: + profile_store.replace_profile("agent", f"{label}\n") + with lock: + outcomes.append(f"created:{label}") + except profile_store.ProfileNotFoundError: + with lock: + outcomes.append(f"rejected:{label}") + + threads = [threading.Thread(target=attempt, args=(name,)) for name in ("FIRST", "SECOND")] + for thread in threads: + thread.start() + for thread in threads: + thread.join() + + assert sorted(outcomes) == ["rejected:FIRST", "rejected:SECOND"] + assert not (store / "agent.md").exists() diff --git a/test/services/test_profile_validator.py b/test/services/test_profile_validator.py index 0f76921cf..cfddff3e4 100644 --- a/test/services/test_profile_validator.py +++ b/test/services/test_profile_validator.py @@ -278,3 +278,55 @@ def test_every_schema_property_is_a_model_field(self) -> None: f"The schema declares {sorted(extra)} but AgentProfile has no such " "field, so a client filling them in would have them silently dropped." ) + + +class TestMalformedButParseableInput: + """Schema-invalid values must be *reported*, never raise. + + Regression guard for the P3 finding on #575. The advisory checks test set + membership, which hashes the value, so an unhashable one (a list) raised + ``TypeError``; and the schema-error sort key used raw path components, so + mixed-type mapping keys could not be ordered. Both escaped the endpoint's + ``except ValueError`` and surfaced as HTTP 500 from a route whose entire + purpose is reporting what is wrong with a document. + + Every case below is syntactically valid YAML that the schema already rejects, + so the correct outcome is an error finding rather than an exception. + """ + + def test_unhashable_allowed_tools_entry_is_reported(self) -> None: + findings = validate_frontmatter({"name": "x", "allowedTools": [["Read"]]}) + + assert any(f.severity == "error" for f in findings) + + def test_unhashable_role_is_reported(self) -> None: + findings = validate_frontmatter({"name": "x", "role": ["developer"]}) + + assert any(f.severity == "error" for f in findings) + + def test_mixed_type_mapping_keys_are_reported(self) -> None: + """Path components of different types must not break the error sort.""" + findings = validate_frontmatter({"name": "x", "mcpServers": {1: {}, "x": {}}}) + + assert any(f.severity == "error" for f in findings) + + def test_non_string_role_does_not_produce_a_spurious_warning(self) -> None: + """The advisory role check stands aside; the schema owns the type error.""" + findings = validate_frontmatter({"name": "x", "role": 7}) + + assert any(f.severity == "error" for f in findings) + assert not any(f.severity == "warning" for f in findings) + + def test_non_string_allowed_tool_does_not_produce_a_spurious_warning(self) -> None: + findings = validate_frontmatter({"name": "x", "allowedTools": [{"a": 1}]}) + + assert any(f.severity == "error" for f in findings) + assert not any(f.severity == "warning" for f in findings) + + def test_well_formed_values_still_warn(self) -> None: + """The type guards must not silence the checks they protect.""" + tool_findings = validate_frontmatter({"name": "x", "allowedTools": ["not_a_real_tool"]}) + role_findings = validate_frontmatter({"name": "x", "role": "archaeologist"}) + + assert any(f.severity == "warning" for f in tool_findings) + assert any(f.severity == "warning" for f in role_findings) diff --git a/test/utils/test_atomic_file.py b/test/utils/test_atomic_file.py index c60d9a750..ca4b5596a 100644 --- a/test/utils/test_atomic_file.py +++ b/test/utils/test_atomic_file.py @@ -697,3 +697,77 @@ def test_write_times_out_when_the_lock_is_held(tmp_path: Path) -> None: with _file_lock(lock_path, timeout=5.0): with pytest.raises(LockTimeoutError): locked_atomic_write(target, "never lands", lock_timeout=0.2) + + +def test_locked_atomic_write_must_exist_updates_an_existing_target(tmp_path: Path) -> None: + target = tmp_path / "f.txt" + target.write_text("old\n", encoding="utf-8") + + locked_atomic_write(target, "new\n", must_exist=True) + + assert target.read_text(encoding="utf-8") == "new\n" + + +def test_locked_atomic_write_must_exist_refuses_to_create(tmp_path: Path) -> None: + target = tmp_path / "absent.txt" + + with pytest.raises(FileNotFoundError): + locked_atomic_write(target, "new\n", must_exist=True) + + assert not target.exists() + + +def test_locked_atomic_write_rejects_the_contradictory_flag_pair(tmp_path: Path) -> None: + """``overwrite=False`` with ``must_exist=True`` can never succeed. + + It demands a target that exists and simultaneously refuses to replace one, so + it is a caller bug rather than a runtime condition. Failing fast beats always + raising FileExistsError and letting the caller think the file was in the way. + """ + target = tmp_path / "f.txt" + + with pytest.raises(ValueError, match="never succeed"): + locked_atomic_write(target, "x\n", overwrite=False, must_exist=True) + + +def test_locked_atomic_write_must_exist_leaves_no_temp_debris(tmp_path: Path) -> None: + """A refused update must not leave a partially written temp file behind.""" + target = tmp_path / "absent.txt" + + with pytest.raises(FileNotFoundError): + locked_atomic_write(target, "new\n", must_exist=True) + + assert [p.name for p in tmp_path.iterdir() if p.name.startswith("absent")] == [] + + +def test_locked_atomic_write_must_exist_is_enforced_under_the_lock(tmp_path: Path) -> None: + """Two concurrent updaters of an absent target must both be refused. + + If the existence test sat outside the critical section, a thread could observe + "absent", lose the race, and still publish, converting an update into a create. + """ + import threading + + target = tmp_path / "absent.txt" + barrier = threading.Barrier(2) + outcomes: list[str] = [] + lock = threading.Lock() + + def attempt(label: str) -> None: + barrier.wait() + try: + locked_atomic_write(target, f"{label}\n", must_exist=True) + with lock: + outcomes.append(f"wrote:{label}") + except FileNotFoundError: + with lock: + outcomes.append(f"refused:{label}") + + threads = [threading.Thread(target=attempt, args=(n,)) for n in ("A", "B")] + for thread in threads: + thread.start() + for thread in threads: + thread.join() + + assert sorted(outcomes) == ["refused:A", "refused:B"] + assert not target.exists() From 167d46ce619d65d65f2018fd752858bc60f911f3 Mon Sep 17 00:00:00 2001 From: Sujoy Datta Choudhury Date: Tue, 11 Aug 2026 15:38:32 -0700 Subject: [PATCH 2/5] fix(api): gate profile source read and serialize profile deletion Addresses the review findings on #585 from @haofeif and @fanhongy, who reviewed independently and converged on the same two blocking defects. GET /agents/profiles/{name}/source carried no scope dependency, so enabling OAuth did not protect it. Authorization here is route-dependency based, and the route now takes require_any_scope(SCOPE_READ, SCOPE_WRITE, SCOPE_ADMIN), the same shape the ten already-guarded GET routes use. The pre-existing profile reads beside it stay ungated, following the split the #505 review settled: gate the newly added read, leave shipped routes alone rather than risk breaking an existing unauthenticated reader. Gating matters more on this route than on those siblings because _read_agent_profile_source returns the stored bytes verbatim across the local, provider, extra and built-in stores, including documents that fail to parse, whereas the parsed route can only return what the model accepts. test_scope_coverage.py could not have caught this: _MUTATING_METHODS is {POST, PUT, PATCH, DELETE}, so a new ungated GET is invisible to it. The guard therefore ships with a structural test asserting the dependency exists on the route object, mirroring _NEW_505_READ_ROUTES, plus enforcement tests that a scopeless token is refused and a read-scoped token is admitted. A status-code test would prove nothing, because auth is default-off and require_any_scope returns the full scope set when it is off. delete_profile performed an unlocked exists() then unlink(), which voided the update-only guarantee replace_profile advertises. A delete could land between an update's must_exist check and its os.replace: the delete succeeded, the update republished, and both callers were told they succeeded while a deleted profile was back on disk holding the replacement text. Reproduced deterministically with a barrier. locked_atomic_delete moves the existence check and the unlink inside the same per-target lock the writers use; unlink is already atomic, so the helper adds only the lock. It is safe against the flock-is-per-inode hazard because the lock file is not the target: _lock_path_for keys a file under LOCK_DIR by a hash of the resolved path and those are never unlinked. Two of these were self-inflicted in ways the tests did not catch. The replace_profile docstring described the update-versus-delete hazard accurately and then left deletion as the unlocked side of the pair, so it promised a guarantee the code did not deliver. The concurrency test was named for a concurrent deleter but deleted the file before its barrier and raced two writers, so it never overlapped a delete with a write. The test is renamed to what it exercises and a deterministic DELETE-versus-PUT overlap test is added alongside it. DELETE /agents/profiles/{name} moves from SCOPE_ADMIN alone to SCOPE_WRITE or SCOPE_ADMIN. Scopes are a flat set rather than a hierarchy: require_any_scope tests membership and get_current_scopes returns the token's claims unexpanded, so admin-only does not merely add admin access, it refuses a client holding exactly cao:write. That contradicted the contract published in #510 and would have left a profile-management credential able to create and edit a profile but not remove it. The six-of-seven precedent for admin-only DELETE still holds, but every one of those routes removes running or generated state, while the lone write-or-admin exception is the only content resource among them. A profile is an authored document, so it belongs with that one. Also exports replace_profile from profile_store's __all__, which listed every peer operation except the one this PR added. Tests: 10 new across three files. Full suite 6,389 passed. --- docs/api.md | 13 ++- src/cli_agent_orchestrator/api/main.py | 32 +++++-- .../services/profile_store.py | 18 +++- .../utils/atomic_file.py | 55 ++++++++++++ test/api/test_scope_coverage.py | 88 +++++++++++++++++-- test/services/test_profile_store.py | 75 +++++++++++++++- test/utils/test_atomic_file.py | 74 ++++++++++++++++ 7 files changed, 331 insertions(+), 24 deletions(-) diff --git a/docs/api.md b/docs/api.md index d551b47c0..b6a9a9513 100644 --- a/docs/api.md +++ b/docs/api.md @@ -71,9 +71,11 @@ See [AG-UI](agui.md) for enablement, event shapes, and privacy boundaries. returns 404 rather than writing a local file that would shadow the original. Requires `cao:write` or `cao:admin`. - `DELETE /agents/profiles/{name}` removes a profile from the local store. - Requires `cao:admin`, matching the other destructive routes on this service; - a `cao:write` token that may create and edit profiles cannot delete them. - Built-ins are not deletable, for the same reason they are not replaceable. + Requires `cao:write` or `cao:admin`, the same guard as create and replace, so + one credential covers the whole create/edit/delete cycle. Scopes are a flat + set rather than a hierarchy, so requiring admin here would 403 a caller + holding exactly `cao:write`. Built-ins are not deletable, for the same reason + they are not replaceable. - Both write routes run the profile validator on the exact submitted document before persisting anything, so an invalid profile never reaches disk. Errors reject the request with 400 and the findings attached; warnings do not block @@ -85,6 +87,11 @@ See [AG-UI](agui.md) for enablement, event shapes, and privacy boundaries. profile, having applied `${VAR}` substitution from the managed environment file to the raw text before parsing. Round-tripping a resolved document through a write would persist substituted values into a plaintext profile. + Requires `cao:read`, `cao:write`, or `cao:admin`. The pre-existing profile + reads beside it are ungated and stay that way, since tightening a shipped + route could break an existing unauthenticated reader; this one is gated + because it returns the stored bytes verbatim from every configured store, + including documents that fail to parse. - Template validation and preview require the selected template to include a `schema.json` file. - `/agents/providers` reports provider availability. diff --git a/src/cli_agent_orchestrator/api/main.py b/src/cli_agent_orchestrator/api/main.py index 970a7d35a..9ac99afbd 100644 --- a/src/cli_agent_orchestrator/api/main.py +++ b/src/cli_agent_orchestrator/api/main.py @@ -2129,15 +2129,22 @@ async def replace_agent_profile_endpoint( @app.delete("/agents/profiles/{name}", status_code=status.HTTP_204_NO_CONTENT) async def delete_agent_profile_endpoint( name: str, - _scopes: List[str] = Depends(require_any_scope(SCOPE_ADMIN)), + _scopes: List[str] = Depends(require_any_scope(SCOPE_WRITE, SCOPE_ADMIN)), ) -> None: """Delete a profile from the local store. - ``SCOPE_ADMIN`` alone rather than write-or-admin, matching every other - destructive route on this service (``/sessions``, ``/workflows``, - ``/terminals``, ``/flows``, ``/memory``). Deletion is the only irreversible - operation in this group, and the asymmetry with POST and PUT is the existing - convention rather than a new one. + Write-or-admin, the same guard as POST and PUT, so one credential completes + the whole create/edit/delete cycle that issue #510 specifies. Scopes are a + flat set here, not a hierarchy: ``require_any_scope`` tests membership, so + admin-only would 403 a caller holding exactly ``cao:write`` and leave a + client that can create and edit a profile unable to remove it. + + Most other ``DELETE`` routes on this service do require admin alone, but they + remove *running or generated* state: sessions, terminals, workflows, flows, + and bulk memory. A profile is an authored document, closer to + ``DELETE /memory/relationships/{id}``, which is also write-or-admin. Removing + one stops no in-flight work and destroys nothing that cannot be re-authored, + and the deletion is already gated behind a confirmation in the UI. Built-in and provider-managed profiles are not deletable for the same reason they are not replaceable: ``delete_profile`` resolves only inside the local @@ -2158,7 +2165,10 @@ async def delete_agent_profile_endpoint( @app.get("/agents/profiles/{name}/source") -async def get_agent_profile_source_endpoint(name: str) -> ProfileSourceResponse: +async def get_agent_profile_source_endpoint( + name: str, + _scopes: List[str] = Depends(require_any_scope(SCOPE_READ, SCOPE_WRITE, SCOPE_ADMIN)), +) -> ProfileSourceResponse: """Return a profile's document exactly as stored, unresolved. The authoring counterpart to ``GET /agents/profiles/{name}``. That route calls @@ -2172,6 +2182,14 @@ async def get_agent_profile_source_endpoint(name: str) -> ProfileSourceResponse: Reads across all configured stores, not only the local one, so a built-in can be fetched as the starting point for a clone. Writing it back still requires the local store, which is enforced by the write routes. + + Scope-gated, unlike the pre-existing profile reads beside it, following the + precedent set on #505: a newly added read route carries the gate, while + already-shipped ungated siblings are left alone because tightening them could + break an existing unauthenticated reader. Gating matters more here than on + those siblings because this route returns the stored bytes verbatim from the + local, provider, extra and built-in stores, including documents that fail to + parse, whereas the parsed route can only return what the model accepts. """ from cli_agent_orchestrator.utils.agent_profiles import _read_agent_profile_source diff --git a/src/cli_agent_orchestrator/services/profile_store.py b/src/cli_agent_orchestrator/services/profile_store.py index 45b401c6f..115723a64 100644 --- a/src/cli_agent_orchestrator/services/profile_store.py +++ b/src/cli_agent_orchestrator/services/profile_store.py @@ -19,7 +19,7 @@ from pathlib import Path from cli_agent_orchestrator.constants import LOCAL_AGENT_STORE_DIR -from cli_agent_orchestrator.utils.atomic_file import locked_atomic_write +from cli_agent_orchestrator.utils.atomic_file import locked_atomic_delete, locked_atomic_write # A profile name becomes a single filesystem segment under # LOCAL_AGENT_STORE_DIR. Restricting to [A-Za-z0-9_-] with a 64-char cap @@ -38,6 +38,7 @@ "ProfileExistsError", "ProfileNotFoundError", "delete_profile", + "replace_profile", "store_path", "write_profile", ] @@ -192,6 +193,14 @@ def replace_profile(name: str, content: str) -> Path: def delete_profile(name: str) -> None: """Delete the profile ``name`` from the local store. + The existence check and the unlink both happen inside the target's write + lock, via :func:`locked_atomic_delete`. That lock is shared with + :func:`replace_profile`, and it has to be: an unlocked delete can slip + between an update's ``must_exist`` check and its publish, so the update + recreates the file and both operations report success. Deletion being the + unlocked side of that pair would silently void the update-only guarantee + ``replace_profile`` advertises. + Args: name: Profile name, used as the filename stem. @@ -207,6 +216,7 @@ def delete_profile(name: str) -> None: if not target.is_relative_to(root): raise InvalidProfileNameError(f"Profile name '{name}' escapes the local store.") - if not target.exists(): - raise ProfileNotFoundError(f"Profile '{name}' not found in the local store.") - target.unlink() + try: + locked_atomic_delete(target) + except FileNotFoundError as exc: + raise ProfileNotFoundError(f"Profile '{name}' not found in the local store.") from exc diff --git a/src/cli_agent_orchestrator/utils/atomic_file.py b/src/cli_agent_orchestrator/utils/atomic_file.py index 3869e1adb..4bfa3c93d 100644 --- a/src/cli_agent_orchestrator/utils/atomic_file.py +++ b/src/cli_agent_orchestrator/utils/atomic_file.py @@ -312,6 +312,61 @@ def locked_atomic_write( _atomic_publish(target, content, encoding) +def locked_atomic_delete( + target: Path, + *, + lock_timeout: float = DEFAULT_LOCK_TIMEOUT_SECONDS, + must_exist: bool = True, +) -> None: + """Remove ``target`` while holding the same lock the writers use. + + The deleting sibling of :func:`locked_atomic_write`. It exists because + ``locked_atomic_write``'s ``must_exist`` guarantee is only real if deletion + participates in the same critical section. An unlocked + ``exists()``-then-``unlink()`` can land *between* an update's existence check + and its ``os.replace``: the delete succeeds, the update then republishes the + file, and both callers are told they succeeded while a supposedly-deleted + file is back on disk holding the new content. Serialising here makes one of + the two lose, which is the whole point of ``must_exist``. + + Deletion needs no temp file or ``os.replace`` because ``unlink`` is already + atomic. The lock is what this adds, not atomicity. + + Note: + Safe against the ``fcntl.flock``-is-per-inode hazard because the lock + file is not the target. ``_lock_path_for`` keys a file under + ``LOCK_DIR`` by a hash of the target's *resolved* path, and those lock + files are never unlinked, so removing the target leaves the lock inode + untouched and a concurrent writer keeps contending on the same one. + + Args: + target: The file to remove. + lock_timeout: Seconds to wait for the lock before raising + ``LockTimeoutError``. + must_exist: When True (default), a missing target raises + ``FileNotFoundError``. The check runs inside the lock, so two + concurrent deleters cannot both observe the file as present. + Pass False to make removal idempotent. + + Raises: + FileNotFoundError: If ``target`` is absent and ``must_exist`` is True. + LockTimeoutError: If the lock is not acquired within ``lock_timeout`` + seconds. + OSError: Propagated from the unlink. + """ + lock_path = _lock_path_for(target) + + with _file_lock(lock_path, lock_timeout): + # Inside the lock for the same reason as locked_atomic_write's checks: + # a caller testing existence beforehand would race both a concurrent + # deleter (double unlink) and a concurrent update (resurrected file). + if not target.exists(): + if must_exist: + raise FileNotFoundError(f"{target} does not exist") + return + target.unlink() + + def _atomic_publish(target: Path, content: str, encoding: str) -> None: """Write ``content`` to ``target`` via a unique temp file + ``os.replace``. diff --git a/test/api/test_scope_coverage.py b/test/api/test_scope_coverage.py index d4ce620f0..a8f30ee19 100644 --- a/test/api/test_scope_coverage.py +++ b/test/api/test_scope_coverage.py @@ -194,10 +194,14 @@ def test_write_token_still_admitted_on_run_list(client, auth_on): # -------------------------------------------------------------------------- # Profile write routes (#510 PR B) # -# Asserts the deliberate asymmetry: POST and PUT accept cao:write, DELETE does -# not. DELETE is admin-only to match every other destructive route on this -# service (/sessions, /workflows, /terminals, /flows, /memory), since it is the -# only irreversible operation in the profile group. +# All three mutating routes accept cao:write or cao:admin, so a single credential +# covers the create/edit/delete cycle #510 specifies. Scopes are a flat set, not +# a hierarchy — ``require_any_scope`` tests membership — so gating DELETE on +# admin alone would 403 a caller holding exactly cao:write and leave a client +# able to create and edit a profile unable to remove it. Most other DELETE +# routes here are admin-only, but they remove running or generated state +# (sessions, terminals, workflows, flows, bulk memory); a profile is an authored +# document, like DELETE /memory/relationships/{id}, which is also write-or-admin. # -------------------------------------------------------------------------- @@ -222,15 +226,24 @@ def test_write_token_admitted_on_profile_replace(client, auth_on): assert resp.status_code != 403 -def test_write_token_forbidden_on_profile_delete(client, auth_on): - """A cao:write token is 403'd on profile deletion. +def test_read_token_forbidden_on_profile_delete(client, auth_on): + """A cao:read token cannot delete a profile — deletion is still a mutation.""" + app.dependency_overrides[auth.get_current_scopes] = _override_scopes([auth.SCOPE_READ]) + resp = client.delete("/agents/profiles/x") + assert resp.status_code == 403 + + +def test_write_token_admitted_on_profile_delete(client, auth_on): + """A cao:write token passes the deletion dependency. - This is the point of choosing admin-only for DELETE: a token that may create - and edit profiles must not be able to remove them. + Changed during the PR #585 review. DELETE was briefly admin-only, which + contradicted the contract published in #510 and would have broken the + documented create/edit/delete workflow for a write-scoped client, since + holding cao:write grants no admin privilege under a flat scope set. """ app.dependency_overrides[auth.get_current_scopes] = _override_scopes([auth.SCOPE_WRITE]) resp = client.delete("/agents/profiles/x") - assert resp.status_code == 403 + assert resp.status_code != 403 def test_admin_token_admitted_on_profile_delete(client, auth_on): @@ -238,3 +251,60 @@ def test_admin_token_admitted_on_profile_delete(client, auth_on): app.dependency_overrides[auth.get_current_scopes] = _override_scopes([auth.SCOPE_ADMIN]) resp = client.delete("/agents/profiles/x") assert resp.status_code != 403 + + +# -------------------------------------------------------------------------- +# PR #585 review — the NEW #510 read route carries a read-scope gate. +# +# Scoped deliberately to the one route this PR ADDED, following the precedent +# the #505 block above set. The pre-existing profile reads (``GET +# /agents/profiles``, ``/search``, ``/templates``, ``/schema``, ``/{name}``) are +# equally ungated and are left alone: tightening shipped routes could break an +# existing unauthenticated reader. +# +# The gate matters more on this route than on those siblings because +# ``_read_agent_profile_source`` returns the stored bytes verbatim, across the +# local, provider, extra and built-in stores, including documents that fail to +# parse. The parsed route can only return what the model accepts. +# -------------------------------------------------------------------------- +_NEW_510_READ_ROUTES = [ + ("GET", "/agents/profiles/{name}/source"), +] + + +@pytest.mark.parametrize("method,path", _NEW_510_READ_ROUTES) +def test_new_510_read_routes_declare_a_scope_dependency(method, path): + """Structural guard: the dependency is present on the route object. + + Asserting on the route table rather than on a status code, for the reason the + #505 version of this test spells out: ``is_auth_enabled()`` is default-off and + ``require_any_scope`` hands back the full scope set when auth is off, so a + "the route still returns 200" test passes whether or not the dependency + exists at all. That is exactly how this route shipped ungated. + """ + matches = [ + r + for r in app.routes + if getattr(r, "path", None) == path and method in (getattr(r, "methods", None) or set()) + ] + assert matches, f"{method} {path} is not registered" + assert _has_scope_dependency(matches[0]), f"{method} {path} has no require_any_scope dependency" + + +def test_scopeless_token_forbidden_on_profile_source(client, auth_on): + """Enforcement: a token holding none of read/write/admin is 403'd on the source read.""" + app.dependency_overrides[auth.get_current_scopes] = _override_scopes([]) + resp = client.get("/agents/profiles/x/source") + assert resp.status_code == 403 + + +def test_read_token_admitted_on_profile_source(client, auth_on): + """A cao:read token PASSES the gate — this is an authoring read, not a mutation. + + Guards the over-restriction failure mode: gating on write/admin only would + lock out a read-only client that legitimately needs the unresolved document, + which is the safe one to read since it never returns substituted secrets. + """ + app.dependency_overrides[auth.get_current_scopes] = _override_scopes([auth.SCOPE_READ]) + resp = client.get("/agents/profiles/x/source") + assert resp.status_code != 403 diff --git a/test/services/test_profile_store.py b/test/services/test_profile_store.py index 29dc4b51b..b7005e9f2 100644 --- a/test/services/test_profile_store.py +++ b/test/services/test_profile_store.py @@ -308,11 +308,18 @@ def test_replace_profile_can_replace_a_corrupt_store_file(store: Path) -> None: assert target.read_text(encoding="utf-8") == "clean\n" -def test_replace_profile_lets_exactly_one_concurrent_deleter_or_writer_win(store: Path) -> None: +def test_replace_profile_refuses_every_concurrent_writer_when_the_target_is_absent( + store: Path, +) -> None: """The existence requirement holds under contention, not just serially. Two threads race to replace the same profile after it is deleted. Neither may succeed by creating the file, because the check lives inside the lock. + + Note what this does NOT cover: the file is removed *before* the barrier, so + both racers are writers and no delete overlaps a write. The interleaving that + actually threatened the update-only guarantee is covered by + ``test_delete_profile_cannot_unlink_while_a_replace_holds_the_lock`` below. """ import threading @@ -341,3 +348,69 @@ def attempt(label: str) -> None: assert sorted(outcomes) == ["rejected:FIRST", "rejected:SECOND"] assert not (store / "agent.md").exists() + + +def test_delete_profile_cannot_unlink_while_a_replace_holds_the_lock( + store: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """A concurrent delete cannot resurrect a profile through an in-flight update. + + Reported on PR #585. ``delete_profile`` used to do an unlocked ``exists()`` + then ``unlink()``, so this interleaving was reachable: + + 1. ``replace_profile`` takes the lock and passes its ``must_exist`` check + 2. ``delete_profile`` unlinks the file and reports success + 3. ``replace_profile`` publishes, recreating what was just deleted + + Both callers were told they succeeded and the "deleted" profile was back on + disk holding the replacement text. Pausing inside the publish makes the + window deterministic rather than hoping the scheduler lands in it: the + deleter must still be blocked on the lock while the replace holds it. + """ + from cli_agent_orchestrator.utils import atomic_file + + profile_store.write_profile("agent", "original\n") + target = store / "agent.md" + + publish_entered = threading.Event() + release = threading.Event() + real_publish = atomic_file._atomic_publish + + def paused_publish(t: Path, content: str, encoding: str) -> None: + publish_entered.set() + release.wait(timeout=10) + return real_publish(t, content, encoding) + + monkeypatch.setattr(atomic_file, "_atomic_publish", paused_publish) + + delete_outcome: list[str] = [] + + def deleter() -> None: + try: + profile_store.delete_profile("agent") + delete_outcome.append("deleted") + except Exception as exc: # noqa: BLE001 - recording the class is the point + delete_outcome.append(type(exc).__name__) + + replacer = threading.Thread(target=lambda: profile_store.replace_profile("agent", "REPLACED\n")) + replacer.start() + assert publish_entered.wait(timeout=10), "replace never reached the publish step" + + deleter_thread = threading.Thread(target=deleter) + deleter_thread.start() + deleter_thread.join(timeout=0.5) + + # The assertion that fails on the unlocked implementation: the delete would + # have completed here, having unlinked a file the replace is about to + # republish. + assert deleter_thread.is_alive(), "delete_profile did not wait for the write lock" + assert target.exists() + + release.set() + replacer.join(timeout=10) + deleter_thread.join(timeout=15) + + # Serialised, so the delete lands after the update rather than inside it and + # the file is genuinely gone. + assert delete_outcome == ["deleted"] + assert not target.exists() diff --git a/test/utils/test_atomic_file.py b/test/utils/test_atomic_file.py index ca4b5596a..d79bed3e4 100644 --- a/test/utils/test_atomic_file.py +++ b/test/utils/test_atomic_file.py @@ -31,6 +31,7 @@ LockTimeoutError, _file_lock, _lock_path_for, + locked_atomic_delete, locked_atomic_rewrite, locked_atomic_write, ) @@ -771,3 +772,76 @@ def attempt(label: str) -> None: assert sorted(outcomes) == ["refused:A", "refused:B"] assert not target.exists() + + +# -------------------------------------------------------------------------- +# locked_atomic_delete +# +# Added for the PR #585 review: locked_atomic_write's ``must_exist`` guarantee +# is only real if deletion takes the SAME lock. An unlocked +# exists()-then-unlink() can land between an update's existence check and its +# os.replace, so the delete succeeds, the update republishes, and a deleted +# file is back on disk holding the new content. +# -------------------------------------------------------------------------- + + +def test_delete_removes_an_existing_file(tmp_path: Path) -> None: + target = tmp_path / "AGENTS.md" + target.write_text("bye\n", encoding="utf-8") + + locked_atomic_delete(target) + + assert not target.exists() + + +def test_delete_raises_when_the_target_is_absent(tmp_path: Path) -> None: + """``must_exist`` defaults to True so a caller can map absence to its own error.""" + with pytest.raises(FileNotFoundError): + locked_atomic_delete(tmp_path / "never-existed.md") + + +def test_delete_is_idempotent_when_must_exist_is_false(tmp_path: Path) -> None: + target = tmp_path / "AGENTS.md" + + locked_atomic_delete(target, must_exist=False) # no raise + + assert not target.exists() + + +def test_delete_contends_on_the_same_lock_as_a_write(tmp_path: Path) -> None: + """The guarantee this helper exists for: one lock covers writes AND deletes. + + Holding the target's lock directly, then asserting the delete times out, + proves ``locked_atomic_delete`` computes the same key via ``_lock_path_for`` + that ``locked_atomic_write`` uses. A delete that keyed a different lock (or + took none) would return immediately and the file would be gone. + """ + target = tmp_path / "AGENTS.md" + target.write_text("live\n", encoding="utf-8") + + with _file_lock(_lock_path_for(target), 5.0): + with pytest.raises(LockTimeoutError): + locked_atomic_delete(target, lock_timeout=0.2) + + assert target.exists(), "the delete must not have unlinked while the lock was held" + + +def test_delete_does_not_disturb_the_lock_file_itself(tmp_path: Path) -> None: + """Removing the target must leave the lock inode intact. + + ``fcntl.flock`` is per-inode, so if deleting a target also removed its lock + file, the next writer and the next deleter would lock different inodes and + stop conflicting. Lock files live under ``LOCK_DIR``, keyed by a hash of the + resolved target path, and are never unlinked. + """ + target = tmp_path / "AGENTS.md" + target.write_text("live\n", encoding="utf-8") + locked_atomic_write(target, "live\n") + lock_path = _lock_path_for(target) + assert lock_path.exists() + + locked_atomic_delete(target) + + assert not target.exists() + assert lock_path.exists() + assert LOCK_DIR in lock_path.parents From 741b4702a194e86831fc40c7f16fc97723abd258 Mon Sep 17 00:00:00 2001 From: Sujoy Datta Choudhury Date: Thu, 13 Aug 2026 10:42:07 -0700 Subject: [PATCH 3/5] fix(api): reject unloadable profiles and unify every write rejection Addresses the round-2 findings from @fanhongy on #585. A profile is submitted as YAML, which allows any scalar as a mapping key, but the format is described by JSON Schema, where object keys are strings by definition. jsonschema therefore reported nothing wrong with mcpServers: 1: command: echo so the write returned 201 and created the file, and parse_agent_profile_text then refused to load it with a Pydantic error at mcpServers.1.[key]. The profile saved and could not be read or launched, contradicting the route's guarantee that an invalid profile never reaches disk. Reproduced for both mcpServers and toolAliases before fixing. validate_frontmatter now walks the parsed document and reports any non-string mapping key as an error. Placed in the validator rather than only on the HTTP write path so every consumer agrees: otherwise cao profile validate and POST /agents/profiles/validate would call such a document valid while the write routes rejected it, and a UI that validates before saving would show a contradiction. Checking the key type generally rather than enumerating fields also covers YAML's other auto-typing: an unquoted 2026-01-01 key becomes a datetime.date, which fails the same way and is now caught. The schema limitation predates this PR. What this PR introduced was making that schema the sole gate in front of a new persistence operation, which turned a latent gap into a broken save path. Not fixed by validating through the AgentProfile model, which would catch strictly more. The write path persists unresolved text, so model-validating unresolved content would reject provider_init_timeout: ${TIMEOUT} that the runtime accepts after resolution, while model-validating resolved content would make acceptance depend on the server's environment. That tradeoff needs its own design rather than riding along here. Also unifies the 400 detail shape across the whole profile surface. The {"message", "errors"} dict was previously produced only inside _validate_profile_for_write, while four sites still returned a bare string: the service-raised InvalidProfileNameError on POST, PUT and DELETE, and the source route's ValueError. A caller therefore still had to switch on type(detail), which is what unifying the shape was supposed to remove, and DELETE was the reachable one because it has no body to validate first. The shape moves to a module-level _profile_write_rejection and all four sites use it. 404 and 409 keep FastAPI's conventional bare string: the status code discriminates and there are no findings to attach. TestWriteRejectionShape claimed to cover "every 400 from a write route" while its loop only sent POST and only validation failures, so the contract it documented was false. It is now parameterized across POST, PUT and DELETE, including the service-raised name error, so the gap fails a test instead of merely contradicting a docstring. This is the third assertion in this PR that promised more than the code delivered, after the replace_profile docstring and the concurrency test named for a deleter it never exercised. Tests: 13 new. Full suite 6,402 passed. --- docs/api.md | 12 ++ src/cli_agent_orchestrator/api/main.py | 51 ++++-- .../services/profile_validator.py | 77 ++++++++- test/api/test_api_profile_surface.py | 150 ++++++++++++++++-- 4 files changed, 250 insertions(+), 40 deletions(-) diff --git a/docs/api.md b/docs/api.md index b6a9a9513..501ae9f22 100644 --- a/docs/api.md +++ b/docs/api.md @@ -81,6 +81,18 @@ See [AG-UI](agui.md) for enablement, event shapes, and privacy boundaries. reject the request with 400 and the findings attached; warnings do not block the write and are returned in the response so a client can surface them after a successful save. +- The validator rejects non-string mapping keys. A profile is written as YAML, + which allows any scalar as a key, but the format is described by JSON Schema, + where object keys are strings. Without this rule `mcpServers: {1: {...}}` + validates clean and persists, then fails to load, since the model requires + string keys. Note YAML also auto-types an unquoted date, so `2026-01-01:` is a + date key rather than a string; quote such keys. +- Every 400 from the profile write and source routes uses one `detail` shape, + `{"message", "errors"}`, so a client never has to switch on the type of + `detail`. `errors` is empty for a failure that is not attributable to a field, + but the key is always present. This covers rejected names as well as schema + findings. 404 and 409 keep FastAPI's conventional bare-string `detail`, since + the status code already discriminates and there are no findings to attach. - `GET /agents/profiles/{name}/source` returns a profile's document exactly as stored. Use this, not `GET /agents/profiles/{name}`, when the document is going to be edited and written back: that route returns the *resolved* diff --git a/src/cli_agent_orchestrator/api/main.py b/src/cli_agent_orchestrator/api/main.py index 9ac99afbd..380130403 100644 --- a/src/cli_agent_orchestrator/api/main.py +++ b/src/cli_agent_orchestrator/api/main.py @@ -1939,6 +1939,33 @@ async def get_agent_profile_schema_endpoint() -> Dict: return load_profile_schema() +def _profile_write_rejection(message: str, findings: Sequence[Any] = ()) -> HTTPException: + """Build the one 400 a profile write route may return. + + Every 400 from the profile write and source routes carries this shape, + ``{"message", "errors"}``, so a client parses one thing rather than switching + on ``type(detail)``. ``errors`` is empty for a failure that is not + attributable to a field, but the key is always present so a caller can + iterate it unconditionally. + + Deliberately covers the service-raised ``InvalidProfileNameError`` paths too, + not only schema findings. An unsafe name is a rejected input just like a + schema violation, and returning a bare string for one and a dict for the + other reintroduces exactly the type-switching this removes. The 404 and 409 + mappings keep FastAPI's conventional bare-string ``detail``: the status code + already tells a client what happened and there are no findings to attach. + """ + return HTTPException( + status_code=status.HTTP_400_BAD_REQUEST, + detail={ + "message": message, + "errors": [ + {"severity": f.severity, "message": f.message, "path": f.path} for f in findings + ], + }, + ) + + def _validate_profile_for_write(name: str, content: str) -> List[ProfileValidationMessage]: """Validate a submitted profile document and enforce name identity. @@ -1966,25 +1993,15 @@ def _validate_profile_for_write(name: str, content: str) -> List[ProfileValidati Raises: HTTPException: 400 if the document is unparseable, carries an - error-severity finding, or declares a conflicting ``name``. Every - rejection uses the same ``detail`` shape, ``{"message", "errors"}``, - so a client parses one thing rather than switching on the type of - ``detail``. ``errors`` is empty for failures that are not per-field. + error-severity finding, or declares a conflicting ``name``. See + :func:`_profile_write_rejection` for the shared ``detail`` shape. """ import frontmatter from cli_agent_orchestrator.services.profile_validator import validate_frontmatter def _reject(message: str, findings: Sequence[Any] = ()) -> None: - raise HTTPException( - status_code=status.HTTP_400_BAD_REQUEST, - detail={ - "message": message, - "errors": [ - {"severity": f.severity, "message": f.message, "path": f.path} for f in findings - ], - }, - ) + raise _profile_write_rejection(message, findings) # Parsed once here, then handed to validate_frontmatter as metadata. # validate_profile_text would parse it again: its docstring exists precisely @@ -2086,7 +2103,7 @@ async def create_agent_profile_endpoint( try: write_profile(request.name, request.content, overwrite=False) except InvalidProfileNameError as exc: - raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail=str(exc)) + raise _profile_write_rejection(str(exc)) except ProfileExistsError as exc: raise HTTPException(status_code=status.HTTP_409_CONFLICT, detail=str(exc)) @@ -2119,7 +2136,7 @@ async def replace_agent_profile_endpoint( try: replace_profile(name, request.content) except InvalidProfileNameError as exc: - raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail=str(exc)) + raise _profile_write_rejection(str(exc)) except ProfileNotFoundError as exc: raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail=str(exc)) @@ -2159,7 +2176,7 @@ async def delete_agent_profile_endpoint( try: delete_profile(name) except InvalidProfileNameError as exc: - raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail=str(exc)) + raise _profile_write_rejection(str(exc)) except ProfileNotFoundError as exc: raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail=str(exc)) @@ -2198,7 +2215,7 @@ async def get_agent_profile_source_endpoint( except FileNotFoundError as exc: raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail=str(exc)) except ValueError as exc: - raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail=str(exc)) + raise _profile_write_rejection(str(exc)) @app.get("/agents/providers") diff --git a/src/cli_agent_orchestrator/services/profile_validator.py b/src/cli_agent_orchestrator/services/profile_validator.py index 0e13c6e64..ba6e2737a 100644 --- a/src/cli_agent_orchestrator/services/profile_validator.py +++ b/src/cli_agent_orchestrator/services/profile_validator.py @@ -71,12 +71,71 @@ def load_profile_schema() -> dict: return json.loads(schema_path.read_text(encoding="utf-8")) +def _non_string_key_findings( + value: object, path: str = "", _depth: int = 0 +) -> list["ValidationMessage"]: + """Report every mapping key in ``value`` that is not a string. + + Closes a gap between the two formats in play. A profile arrives as **YAML**, + which allows any scalar as a mapping key, but the format is described by + **JSON Schema**, where object keys are strings by definition. jsonschema + therefore does not flag ``mcpServers: {1: {command: echo}}`` at all, while + ``AgentProfile`` refuses to load it later because ``Dict[str, ...]`` rejects + the integer key. + + Reported here rather than only on the HTTP write path so that every consumer + agrees. Otherwise ``cao profile validate`` and + ``POST /agents/profiles/validate`` would call such a document valid and the + write routes would then reject it, and a UI that validates before saving + would show a contradiction. + + The same mismatch reaches further than integer keys: YAML also auto-types + unquoted dates, so ``2026-01-01:`` becomes a ``datetime.date`` key. Checking + the key type generally covers those without enumerating them. + + Args: + value: Any parsed YAML value. Only mappings and sequences are descended. + path: Dotted path of ``value`` within the document, for the finding. + _depth: Recursion guard. YAML aliases can build deeply nested or + self-referential structures, and a validator must not hang on input + whose whole problem is that it is malformed. + + Returns: + One error finding per offending key, in document order. + """ + if _depth > 32: + return [] + + findings: list[ValidationMessage] = [] + + if isinstance(value, dict): + for key, child in value.items(): + child_path = f"{path}.{key}" if path else str(key) + if not isinstance(key, str): + findings.append( + ValidationMessage( + "error", + f"Mapping key {key!r} is a {type(key).__name__}, not a string. " + f"Profile fields are string-keyed; quote it as '{key}'.", + child_path, + ) + ) + findings.extend(_non_string_key_findings(child, child_path, _depth + 1)) + elif isinstance(value, list): + for index, child in enumerate(value): + child_path = f"{path}.{index}" if path else str(index) + findings.extend(_non_string_key_findings(child, child_path, _depth + 1)) + + return findings + + def validate_frontmatter(metadata: dict) -> list[ValidationMessage]: """Validate a frontmatter dict against the schema and CAO conventions. - Returns findings in a stable order: deprecated fields, then JSON-Schema - errors sorted by path, then ``allowedTools`` vocabulary warnings, then the - role check. An empty list means the profile is valid with no advisories. + Returns findings in a stable order: deprecated fields, then non-string + mapping keys, then JSON-Schema errors sorted by path, then ``allowedTools`` + vocabulary warnings, then the role check. An empty list means the profile is + valid with no advisories. """ messages: list[ValidationMessage] = [] @@ -92,7 +151,13 @@ def validate_frontmatter(metadata: dict) -> list[ValidationMessage]: ) ) - # 2. JSON-Schema structural validation. + # 2. Non-string mapping keys, which JSON Schema cannot see. Reported before + # the schema errors because a document with a non-string key is outside + # the format entirely, and because the schema's own findings for such a + # document tend to be confusing. + messages.extend(_non_string_key_findings(metadata)) + + # 3. JSON-Schema structural validation. # # The sort key stringifies each path component. Raw components are whatever # the document used as mapping keys, so a profile with mixed-type keys (for @@ -105,7 +170,7 @@ def validate_frontmatter(metadata: dict) -> list[ValidationMessage]: path = ".".join(str(p) for p in error.absolute_path) or "(root)" messages.append(ValidationMessage("error", error.message, path)) - # 3. allowedTools vocabulary check (advisory, not blocking). + # 4. allowedTools vocabulary check (advisory, not blocking). # # Each entry is type-checked before the membership test. ``_VALID_TOOL_VOCAB`` # is a set, so ``tool not in`` hashes ``tool``, and an unhashable element @@ -126,7 +191,7 @@ def validate_frontmatter(metadata: dict) -> list[ValidationMessage]: ) ) - # 4. Role check (advisory — custom roles are valid but worth flagging). + # 5. Role check (advisory — custom roles are valid but worth flagging). # # Same hashing hazard as above: ``role: [developer]`` is unhashable. The # schema reports the type error, so this advisory check simply stands aside. diff --git a/test/api/test_api_profile_surface.py b/test/api/test_api_profile_surface.py index 5900e49e1..5fbdb6cef 100644 --- a/test/api/test_api_profile_surface.py +++ b/test/api/test_api_profile_surface.py @@ -754,31 +754,147 @@ def test_missing_profile_returns_404(self, client, write_store) -> None: assert client.get("/agents/profiles/absent/source").status_code == 404 +class TestNonStringMappingKeysAreRejected: + """A profile the runtime cannot load must not reach disk. + + Reported as a P2 in round 1 of review on #585. The request body is YAML, + which allows any scalar as a mapping key, but the gate was JSON Schema, where + object keys are strings by definition. jsonschema saw nothing wrong with + ``mcpServers: {1: {command: echo}}``, so the write returned 201 and created + the file, and ``parse_agent_profile_text`` then refused to load it with a + Pydantic error at ``mcpServers.1.[key]``. The profile saved and could not be + read or launched, which contradicts the route's guarantee. + + The check lives in the validator rather than only on this path, so + ``cao profile validate`` and ``POST /agents/profiles/validate`` agree with + the write routes. A UI that validates before saving would otherwise be told + the document is fine and then handed a 400. + """ + + CASES = { + "mcpServers integer key": "---\nname: probe\nmcpServers:\n 1:\n command: echo\n---\n\nB.\n", + "toolAliases integer key": "---\nname: probe\ntoolAliases:\n 1: Read\n---\n\nB.\n", + # YAML auto-types an unquoted date, so this key is a datetime.date. The + # same mismatch, reached without anyone writing a number. + "unquoted date key": "---\nname: probe\ntoolAliases:\n 2026-01-01: Read\n---\n\nB.\n", + } + + @pytest.mark.parametrize("label", list(CASES)) + def test_create_rejects_and_writes_nothing(self, client, write_store, label) -> None: + response = client.post( + "/agents/profiles", json={"name": "probe", "content": self.CASES[label]} + ) + + assert response.status_code == 400, label + assert not (write_store / "probe.md").exists(), f"{label}: profile was persisted" + + @pytest.mark.parametrize("label", list(CASES)) + def test_replace_rejects_and_leaves_the_original(self, client, write_store, label) -> None: + """The existing document must survive a rejected update byte-for-byte.""" + write_store.mkdir(parents=True, exist_ok=True) + original = VALID_PROFILE.format(name="probe") + (write_store / "probe.md").write_text(original, encoding="utf-8") + + response = client.put("/agents/profiles/probe", json={"content": self.CASES[label]}) + + assert response.status_code == 400, label + assert (write_store / "probe.md").read_text(encoding="utf-8") == original, label + + def test_the_rejected_document_is_indeed_unloadable(self, write_store) -> None: + """Anchors the reason for rejecting: the runtime cannot parse these. + + Without this, the rule above reads as an arbitrary restriction. Asserting + the load failure keeps the justification in the suite rather than only in + a commit message. + """ + import pytest as _pytest + + from cli_agent_orchestrator.utils.agent_profiles import parse_agent_profile_text + + with _pytest.raises(Exception) as excinfo: + parse_agent_profile_text(self.CASES["mcpServers integer key"], "probe") + + assert "mcpServers" in str(excinfo.value) + + class TestWriteRejectionShape: - """Every 400 from a write route uses one ``detail`` shape. + """Every 400 from the profile write and source routes uses one ``detail`` shape. A client should not have to switch on ``type(detail)``. Before this was unified, a schema failure returned a dict while a name mismatch and a parse failure returned bare strings, from the same endpoint. - """ - REJECTIONS = { - "schema error": {"name": "x", "content": "---\nname: x\nengine: v3\n---\n\nB.\n"}, - "name mismatch": {"name": "x", "content": "---\nname: other\n---\n\nB.\n"}, - "unparseable": {"name": "x", "content": "---\nname: [unclosed\n b: : y\n---\n\nB.\n"}, - "missing name": {"name": "x", "content": "---\ndescription: none\n---\n\nB.\n"}, - } + Two kinds of 400 reach the client and both are covered below: validation + findings raised inside ``_validate_profile_for_write``, and the + service-raised ``InvalidProfileNameError``, which ``DELETE`` reaches because + it has no body to validate first. Round 1 of review on #585 caught that this + class asserted the suite-wide contract while exercising only ``POST``, and + that ``DELETE``'s name error was in fact still a bare string. The parameters + below exist so that gap fails a test rather than merely contradicting a + docstring. + + 404 and 409 deliberately keep FastAPI's conventional bare-string ``detail`` + and are not covered here: the status code already discriminates and there are + no findings to attach. + """ - def test_every_rejection_has_the_same_detail_shape(self, client, write_store) -> None: - for label, body in self.REJECTIONS.items(): - response = client.post("/agents/profiles", json=body) + # (label, client method, url, json body or None) + REJECTIONS = [ + ( + "post: schema error", + "post", + "/agents/profiles", + {"name": "x", "content": "---\nname: x\nengine: v3\n---\n\nB.\n"}, + ), + ( + "post: name mismatch", + "post", + "/agents/profiles", + {"name": "x", "content": "---\nname: other\n---\n\nB.\n"}, + ), + ( + "post: unparseable", + "post", + "/agents/profiles", + {"name": "x", "content": "---\nname: [unclosed\n b: : y\n---\n\nB.\n"}, + ), + ( + "post: missing name", + "post", + "/agents/profiles", + {"name": "x", "content": "---\ndescription: none\n---\n\nB.\n"}, + ), + ( + "post: non-string mapping key", + "post", + "/agents/profiles", + { + "name": "x", + "content": "---\nname: x\nmcpServers:\n 1:\n command: echo\n---\n\nB.\n", + }, + ), + ( + "put: name mismatch", + "put", + "/agents/profiles/x", + {"content": "---\nname: other\n---\n\nB.\n"}, + ), + ("delete: unsafe name", "delete", "/agents/profiles/bad@name", None), + ] - assert response.status_code == 400, label - detail = response.json()["detail"] - assert isinstance(detail, dict), f"{label}: detail was {type(detail).__name__}" - assert set(detail) == {"message", "errors"}, label - assert isinstance(detail["message"], str), label - assert isinstance(detail["errors"], list), label + @pytest.mark.parametrize("label,method,url,body", REJECTIONS, ids=[r[0] for r in REJECTIONS]) + def test_every_rejection_has_the_same_detail_shape( + self, client, write_store, label, method, url, body + ) -> None: + call = getattr(client, method) + response = call(url) if body is None else call(url, json=body) + + assert response.status_code == 400, label + detail = response.json()["detail"] + assert isinstance(detail, dict), f"{label}: detail was {type(detail).__name__}" + assert set(detail) == {"message", "errors"}, label + assert isinstance(detail["message"], str), label + assert isinstance(detail["errors"], list), label def test_field_level_failures_carry_a_path(self, client, write_store) -> None: """A schema failure must say which field, so a form can render it.""" From c5added051339b79edf3a784a4c5f4e5eaba49d5 Mon Sep 17 00:00:00 2001 From: Sujoy Datta Choudhury Date: Fri, 14 Aug 2026 12:05:09 -0700 Subject: [PATCH 4/5] fix(api): bound the key walk and accept url-based MCP servers Two findings from @haofeif on the write gate, in opposite directions: it could be stalled by a valid document, and it rejected a valid one. The non-string mapping key check added last round walked the parsed document with only a recursion depth cap. That bounded the wrong dimension. yaml.safe_load resolves every alias to another reference to the *same* object, so N chained anchors that each reference the previous one twice leave memory linear while an unmemoized walk traverses the graph 2**N times. Depth was never the problem; revisiting shared objects was. A 640-byte, schema-valid body took ~1s locally and doubled per added level, against ~0s for the jsonschema step beside it, so the amplification was introduced entirely by that walk. It was reachable without credentials. POST /agents/profiles/validate is in the scope-exemption set, so it answers even when OAuth is configured, and it is declared async, so a synchronous CPU-bound walk on its thread stalls the event loop for every other request rather than only the caller's own. The walk now skips any container it has already visited, keyed on id(). That removes the amplification at its source and costs no coverage: a shared subtree cannot hold a different set of keys on a second visit, so one finding per offending key is the correct output, reported at the first path reaching it. Comparing identity is sound here specifically because every value stays reachable from the document for the duration of the walk, so nothing can be collected and no id recycled midway; the code says so rather than leaving it as a trap for a later reader. Identity memoization does not bound a document that is merely enormous, so explicit ceilings on total values and nesting depth remain, both ~1000x the largest bundled profile. Exceeding either now yields an error finding. Previously the depth cap returned silently, which reported an unchecked document as valid. Separately, agent_profile.schema.json required "command" on every mcpServers entry, while resolve_mcp_server_config documents command-less entries shaped {"type": "http", "url": ...} as passing through untouched, and providers forward them to their own MCP config. Because this PR made that schema the blocking gate in front of persistence, an incomplete description became a broken save path: POST and PUT returned 400 for a form CAO supports. Entries now require command or url via anyOf, with url declared so the field is described rather than merely tolerated by an absent additionalProperties: false, and so a wrong type is a finding. An entry defining neither is still rejected. That is the mirror image of the round-1 finding on the same decision: that one let unloadable profiles reach disk, this one blocked loadable ones. Both came from treating an incomplete schema as a blocking gate. The anyOf rejection message is jsonschema's generic "is not valid under any of the given schemas". It names the exact entry and path, but not which key is missing. Left as is rather than adding message-rewriting machinery to the validator; noted in the PR's known gaps. Tests: 20 new. A 40-level bomb (2**40 paths, under 1500 bytes) now validates in ~0.0001s, and a bad key inside a shared subtree is asserted to appear exactly once, which pins the memoization without depending on a clock. Legitimate anchor reuse still validates clean, both ceilings are asserted to reject rather than fall silent, and a url-based profile is asserted to survive the write, the profile parse, and MCP resolution with its transport intact. Full suite 6,711 passed. --- docs/agent-profile.md | 5 +- docs/api.md | 15 ++ .../schemas/agent_profile.schema.json | 9 +- .../services/profile_validator.py | 128 ++++++++++--- test/api/test_api_profile_surface.py | 119 ++++++++++++ test/services/test_profile_validator.py | 177 ++++++++++++++++++ 6 files changed, 420 insertions(+), 33 deletions(-) diff --git a/docs/agent-profile.md b/docs/agent-profile.md index bc830b52a..17f895f96 100644 --- a/docs/agent-profile.md +++ b/docs/agent-profile.md @@ -51,7 +51,10 @@ portable and make profile listings useful. ### Provider configuration -- `mcpServers` (object): MCP server definitions. +- `mcpServers` (object): MCP server definitions. Each entry defines either + `command` (with optional `args`, `env`, `timeout`) for a server CAO launches, + or `url` for a remote one, with `type` naming its transport (for example + `http` or `sse`). An entry defining neither is invalid. - `tools` (array), `toolAliases` (object), and `toolsSettings` (object): provider tool configuration. - `resources` (array), `hooks` (object), and `useLegacyMcpJson` (boolean): diff --git a/docs/api.md b/docs/api.md index cc10df805..7886fd132 100644 --- a/docs/api.md +++ b/docs/api.md @@ -87,6 +87,21 @@ See [AG-UI](agui.md) for enablement, event shapes, and privacy boundaries. validates clean and persists, then fails to load, since the model requires string keys. Note YAML also auto-types an unquoted date, so `2026-01-01:` is a date key rather than a string; quote such keys. +- That key check walks the parsed document, and the walk is bounded, because YAML + anchors make a document's value graph arbitrarily larger than its bytes: each + alias resolves to another reference to the same object, so chained anchors give + a sub-kilobyte body an exponential number of paths. Containers already visited + are skipped, which removes the amplification and still reports each offending + key once, at the first path that reaches it. Separate ceilings on total values + (20,000) and nesting depth (64) bound a document that is merely enormous rather + than aliased; exceeding either is itself an error, so such a document is + rejected rather than reported clean on a partial walk. Both bounds are ~1000x + the largest bundled profile. +- An `mcpServers` entry must define either `command`, for a server CAO launches, + or `url`, for a remote one whose `type` names its transport. The schema + previously required `command` unconditionally, which made the write routes + reject url-based servers that the runtime accepts and passes through to the + provider unchanged. An entry defining neither is still rejected. - Every 400 from the profile write and source routes uses one `detail` shape, `{"message", "errors"}`, so a client never has to switch on the type of `detail`. `errors` is empty for a failure that is not attributable to a field, diff --git a/src/cli_agent_orchestrator/schemas/agent_profile.schema.json b/src/cli_agent_orchestrator/schemas/agent_profile.schema.json index f52295e35..ee2e4e11a 100644 --- a/src/cli_agent_orchestrator/schemas/agent_profile.schema.json +++ b/src/cli_agent_orchestrator/schemas/agent_profile.schema.json @@ -50,14 +50,19 @@ "description": "MCP server configurations for additional tools.", "additionalProperties": { "type": "object", + "description": "One MCP server. A command-launched server sets 'command'; a remote server sets 'url', with 'type' naming its transport (for example 'http' or 'sse'). At least one of the two is required.", "properties": { "type": {"type": "string"}, "command": {"type": "string"}, "args": {"type": "array", "items": {"type": "string"}}, "env": {"type": "object", "additionalProperties": {"type": "string"}}, - "timeout": {"type": "integer"} + "timeout": {"type": "integer"}, + "url": {"type": "string"} }, - "required": ["command"] + "anyOf": [ + {"required": ["command"]}, + {"required": ["url"]} + ] } }, "tools": { diff --git a/src/cli_agent_orchestrator/services/profile_validator.py b/src/cli_agent_orchestrator/services/profile_validator.py index ba6e2737a..30cd1a800 100644 --- a/src/cli_agent_orchestrator/services/profile_validator.py +++ b/src/cli_agent_orchestrator/services/profile_validator.py @@ -71,10 +71,16 @@ def load_profile_schema() -> dict: return json.loads(schema_path.read_text(encoding="utf-8")) -def _non_string_key_findings( - value: object, path: str = "", _depth: int = 0 -) -> list["ValidationMessage"]: - """Report every mapping key in ``value`` that is not a string. +# Ceilings for the key walk below, on a document that is genuinely huge or +# deeply nested rather than aliased. Generous by ~1000x: the largest bundled +# profile's frontmatter parses to 23 values and nests 3 deep, so no legitimate +# profile approaches either bound. +_MAX_WALK_VALUES = 20_000 +_MAX_WALK_DEPTH = 64 + + +def _non_string_key_findings(metadata: object) -> list["ValidationMessage"]: + """Report every mapping key reachable from ``metadata`` that is not a string. Closes a gap between the two formats in play. A profile arrives as **YAML**, which allows any scalar as a mapping key, but the format is described by @@ -93,38 +99,100 @@ def _non_string_key_findings( unquoted dates, so ``2026-01-01:`` becomes a ``datetime.date`` key. Checking the key type generally covers those without enumerating them. + **Why the walk is bounded.** YAML anchors make a document's value *graph* + arbitrarily larger than its bytes: ``yaml.safe_load`` resolves each alias to + another reference to the *same* object, so N chained anchors that each + reference the previous one twice build a graph an unmemoized walk traverses + 2**N times while memory stays linear. The first version of this function + carried only a depth cap, which bounded the wrong dimension -- depth was + never the problem, revisiting shared objects was -- and a 640-byte, + schema-valid document with 20 anchor levels took ~1s here against ~0s in + jsonschema, doubling per added level. That is reachable unauthenticated: + ``POST /agents/profiles/validate`` is scope-exempt and declared ``async``, + so a synchronous walk on its thread stalls the whole event loop. + + Two bounds, each covering what the other does not: + + - ``seen`` skips any container already walked, keyed on ``id()``. This + removes the amplification at its source and costs no coverage: a shared + subtree cannot hold a different set of keys on a second visit, so one + finding per offending key is the correct output, reported at the first + path that reaches it. Comparing identity is sound here specifically + because every value stays reachable from ``metadata`` for the duration of + the walk, so nothing can be collected and no id can be recycled midway. + - ``_MAX_WALK_VALUES`` and ``_MAX_WALK_DEPTH`` bound a document that is + merely enormous, which memoizing identity does not. Exceeding either adds + an **error** finding, so such a document is rejected rather than quietly + called valid on the strength of a partial walk. + Args: - value: Any parsed YAML value. Only mappings and sequences are descended. - path: Dotted path of ``value`` within the document, for the finding. - _depth: Recursion guard. YAML aliases can build deeply nested or - self-referential structures, and a validator must not hang on input - whose whole problem is that it is malformed. + metadata: Any parsed YAML value. Only mappings and sequences are + descended into. Returns: - One error finding per offending key, in document order. + Error findings in document order: one per offending key, plus a final + one if a bound was reached. """ - if _depth > 32: - return [] - findings: list[ValidationMessage] = [] - - if isinstance(value, dict): - for key, child in value.items(): - child_path = f"{path}.{key}" if path else str(key) - if not isinstance(key, str): - findings.append( - ValidationMessage( - "error", - f"Mapping key {key!r} is a {type(key).__name__}, not a string. " - f"Profile fields are string-keyed; quote it as '{key}'.", - child_path, + seen: set[int] = set() + remaining = _MAX_WALK_VALUES + limit_reached: Optional[str] = None + + def walk(value: object, path: str, depth: int) -> None: + nonlocal remaining, limit_reached + + if not isinstance(value, (dict, list)): + return # A scalar has no keys, and nothing to descend into. + if limit_reached is not None: + return + if depth > _MAX_WALK_DEPTH: + limit_reached = f"is nested more than {_MAX_WALK_DEPTH} levels deep" + return + if id(value) in seen: + return + seen.add(id(value)) + + children: list[tuple[str, object]] + if isinstance(value, dict): + children = [] + for key, child in value.items(): + child_path = f"{path}.{key}" if path else str(key) + if not isinstance(key, str): + findings.append( + ValidationMessage( + "error", + f"Mapping key {key!r} is a {type(key).__name__}, not a string. " + f"Profile fields are string-keyed; quote it as '{key}'.", + child_path, + ) ) - ) - findings.extend(_non_string_key_findings(child, child_path, _depth + 1)) - elif isinstance(value, list): - for index, child in enumerate(value): - child_path = f"{path}.{index}" if path else str(index) - findings.extend(_non_string_key_findings(child, child_path, _depth + 1)) + children.append((child_path, child)) + else: + children = [ + (f"{path}.{index}" if path else str(index), child) + for index, child in enumerate(value) + ] + + for child_path, child in children: + remaining -= 1 + if remaining <= 0: + limit_reached = f"holds more than {_MAX_WALK_VALUES} values" + return + walk(child, child_path, depth + 1) + if limit_reached is not None: + return + + walk(metadata, "", 0) + + if limit_reached is not None: + findings.append( + ValidationMessage( + "error", + f"Frontmatter {limit_reached}, past the bound this validator will " + f"traverse, so its mapping keys cannot be fully checked. Simplify " + f"the document.", + ) + ) return findings diff --git a/test/api/test_api_profile_surface.py b/test/api/test_api_profile_surface.py index 5fbdb6cef..62de6a74a 100644 --- a/test/api/test_api_profile_surface.py +++ b/test/api/test_api_profile_surface.py @@ -6,6 +6,7 @@ paths as the CLI instead of reimplementing them. """ +import time from unittest.mock import patch import pytest @@ -918,3 +919,121 @@ def test_non_field_failures_carry_an_empty_error_list(self, client, write_store) ) assert response.json()["detail"]["errors"] == [] + + +class TestValidateEndpointResistsAliasAmplification: + """The unauthenticated validate route must not be stallable by its body. + + Reported as a P2 in round 2 of review on #585. The non-string mapping key + check added in round 1 walked the parsed document with only a depth cap, but + YAML anchors resolve to repeated references to one object, so a walk that + does not remember where it has been revisits shared subtrees exponentially. + A sub-kilobyte body reached seconds of CPU and doubled per anchor level. + + This route is the exposed one: it is in the scope-exemption set, so it + answers without credentials even when OAuth is configured, and it is declared + ``async``, so a synchronous walk on its thread blocks the event loop for every + other request rather than just the attacker's own. That exemption is pinned in + ``test/api/test_scope_coverage.py::_EXEMPT``, which is the one place it is + asserted; if it is ever removed, the reasoning here changes. + """ + + @staticmethod + def _bomb(levels: int) -> str: + lines = [ + "---", + "name: bomb", + "description: A profile.", + "toolsSettings:", + " a0: &a0 {k: v}", + ] + for level in range(1, levels + 1): + lines.append(f" a{level}: &a{level} {{x: *a{level - 1}, y: *a{level - 1}}}") + return "\n".join(lines) + "\n---\n\nBody.\n" + + def test_an_anchor_bomb_is_answered_promptly(self, client) -> None: + content = self._bomb(40) + assert len(content) < 1500 + + started = time.perf_counter() + response = client.post("/agents/profiles/validate", json={"content": content}) + elapsed = time.perf_counter() - started + + assert response.status_code == 200 + assert response.json()["valid"] is True + assert elapsed < 10.0, f"validate took {elapsed:.2f}s on a {len(content)}-byte body" + + +class TestUrlBasedMcpServersAreWritable: + """A url-based MCP entry is a supported form and must survive the write gate. + + Reported as a P2 in round 2 of review on #585. ``agent_profile.schema.json`` + required ``command`` on every ``mcpServers`` entry, while + ``resolve_mcp_server_config`` documents command-less entries shaped + ``{"type": "http", "url": ...}`` as passing through untouched. Making that + schema the blocking gate in front of persistence turned an incomplete + description into a rejected save, the mirror image of the round-1 P2: that one + let unloadable profiles through, this one blocked loadable ones. + """ + + URL_PROFILE = ( + "---\nname: {name}\ndescription: A test profile.\nmcpServers:\n" + " docs:\n type: http\n url: https://example.test/mcp\n---\n\nYou are a test agent.\n" + ) + + def test_create_accepts_a_url_based_server(self, client, write_store) -> None: + response = client.post( + "/agents/profiles", + json={"name": "remote", "content": self.URL_PROFILE.format(name="remote")}, + ) + + assert response.status_code == 201, response.json() + assert (write_store / "remote.md").exists() + + def test_the_written_profile_loads_and_keeps_its_transport(self, client, write_store) -> None: + """Accepting it is only correct if the runtime can then use it. + + Guards against fixing the gate by loosening it past what CAO supports: + the entry has to survive both the profile parse and MCP resolution with + its ``type``/``url`` intact. + """ + from cli_agent_orchestrator.utils.agent_profiles import parse_agent_profile_text + from cli_agent_orchestrator.utils.mcp_resolution import resolve_mcp_server_config + + client.post( + "/agents/profiles", + json={"name": "remote", "content": self.URL_PROFILE.format(name="remote")}, + ) + stored = (write_store / "remote.md").read_text(encoding="utf-8") + + profile = parse_agent_profile_text(stored, "remote") + resolved = resolve_mcp_server_config(dict(profile.mcpServers["docs"])) + + assert resolved == {"type": "http", "url": "https://example.test/mcp"} + + def test_replace_accepts_a_url_based_server(self, client, write_store) -> None: + write_store.mkdir(parents=True, exist_ok=True) + (write_store / "remote.md").write_text( + VALID_PROFILE.format(name="remote"), encoding="utf-8" + ) + + response = client.put( + "/agents/profiles/remote", + json={"content": self.URL_PROFILE.format(name="remote")}, + ) + + assert response.status_code == 200, response.json() + assert "url: https://example.test/mcp" in (write_store / "remote.md").read_text() + + def test_an_entry_naming_no_transport_is_still_rejected(self, client, write_store) -> None: + """The rule gained a branch; it was not removed.""" + content = ( + "---\nname: broken\ndescription: A test profile.\nmcpServers:\n" + " docs:\n type: http\n---\n\nBody.\n" + ) + + response = client.post("/agents/profiles", json={"name": "broken", "content": content}) + + assert response.status_code == 400 + assert not (write_store / "broken.md").exists() + assert response.json()["detail"]["errors"] diff --git a/test/services/test_profile_validator.py b/test/services/test_profile_validator.py index cfddff3e4..223261bb0 100644 --- a/test/services/test_profile_validator.py +++ b/test/services/test_profile_validator.py @@ -9,6 +9,8 @@ Ref: https://github.com/awslabs/cli-agent-orchestrator/issues/510 """ +import time + import pytest from cli_agent_orchestrator.models.agent_profile import AgentProfile @@ -330,3 +332,178 @@ def test_well_formed_values_still_warn(self) -> None: assert any(f.severity == "warning" for f in tool_findings) assert any(f.severity == "warning" for f in role_findings) + + +def _alias_amplified_yaml(levels: int, leaf: str = "{k: v}") -> str: + """A schema-valid profile whose value graph is 2**``levels`` paths. + + Each anchor references the previous one twice, so ``yaml.safe_load`` returns + ``levels + 1`` dicts while an unmemoized walk sees an exponential number of + paths through them. Nested under ``toolsSettings`` because that field is a + free-form object, which keeps the document *valid* -- the point being that a + rejected document would never reach a full traversal anyway. + """ + lines = ["---", "name: bomb", "description: A profile.", "toolsSettings:", f" a0: &a0 {leaf}"] + for level in range(1, levels + 1): + lines.append(f" a{level}: &a{level} {{x: *a{level - 1}, y: *a{level - 1}}}") + return "\n".join(lines) + "\n---\n\nBody.\n" + + +class TestAliasAmplificationIsBounded: + """A YAML-anchor bomb must not stall the key walk. + + Round 2 of review on #585 added a non-string mapping key check whose only + bound was a recursion depth cap. That bounded the wrong dimension: YAML + aliases resolve to repeated references to the *same* object, so the walk + revisited shared subtrees exponentially while the document stayed tiny. A + 640-byte, schema-valid body took ~1s, doubling per added anchor level, on a + scope-exempt ``async`` route -- a denial of service reachable without + credentials. Reported by @haofeif. + + The fix skips containers already walked, keyed on identity, so these tests + pin both halves of that: the traversal terminates, and skipping repeats does + not lose a finding. + """ + + def test_a_forty_level_bomb_validates_promptly(self) -> None: + """Forty levels is 2**40 paths: unbounded, this never returns.""" + document = _alias_amplified_yaml(40) + assert len(document) < 1500 # the whole point: tiny input, huge graph + + started = time.perf_counter() + findings = validate_profile_text(document) + elapsed = time.perf_counter() - started + + assert findings == [] + # Measured at ~0.0001s. The bound is loose enough to survive a loaded + # CI runner while still being unreachable for an exponential walk. + assert elapsed < 5.0, f"walk took {elapsed:.2f}s; the traversal bound is not holding" + + def test_a_self_referential_document_terminates(self) -> None: + """An anchor that contains itself is a cycle, not merely deep nesting.""" + document = ( + "---\nname: cyc\ndescription: A profile.\ntoolsSettings: &c {self: *c}\n---\n\nB.\n" + ) + + started = time.perf_counter() + findings = validate_profile_text(document) + + assert findings == [] + assert time.perf_counter() - started < 5.0 + + def test_a_bad_key_in_a_shared_subtree_is_reported_exactly_once(self) -> None: + """Deterministic proof of the memoization, with no reliance on a clock. + + The offending key sits in the one node every alias resolves to. Reported + once, it confirms shared nodes are visited once; the pre-fix walk would + have emitted 2**20 copies of the same finding. + """ + document = _alias_amplified_yaml(20, leaf="{1: one}") + + findings = validate_profile_text(document) + key_errors = [f for f in findings if "not a string" in f.message] + + assert len(key_errors) == 1 + assert key_errors[0].severity == "error" + assert key_errors[0].path == "toolsSettings.a0.1" + + def test_legitimate_anchor_reuse_still_validates_clean(self) -> None: + """Anchors are a normal YAML convenience, not inherently suspect.""" + document = ( + "---\nname: shared\ndescription: A profile.\ntoolsSettings:\n" + " common: &common {timeout: 30}\n fs: *common\n web: *common\n---\n\nBody.\n" + ) + + findings = validate_profile_text(document) + + assert findings == [] + + def test_exceeding_a_bound_is_an_error_not_silence(self) -> None: + """A document too large to traverse is rejected, not called valid. + + Identity memoization bounds an *aliased* document; these bounds cover one + that is merely enormous. Returning no findings there would report an + unchecked document as clean, which is the failure mode being avoided. + """ + deep: dict = {"name": "deep", "description": "A profile."} + node = deep + for _ in range(70): + node["toolsSettings"] = {} + node = node["toolsSettings"] + wide = { + "name": "wide", + "description": "A profile.", + "toolsSettings": {f"k{index}": index for index in range(25_000)}, + } + + for metadata, expected in ((deep, "nested more than"), (wide, "holds more than")): + errors = [f for f in validate_frontmatter(metadata) if f.severity == "error"] + assert len(errors) == 1 + assert expected in errors[0].message + + +class TestMcpServerTransports: + """``mcpServers`` entries may be command-launched *or* url-based. + + The schema required ``command`` unconditionally, which made the write routes + reject a form CAO supports: ``resolve_mcp_server_config`` documents entries + without a ``command`` (``{"type": "http", "url": ...}``) as passing through + untouched, and providers forward them to their own MCP config. Because + #585 made this schema the blocking gate in front of persistence, a latent + description gap became a broken save path. Reported by @haofeif. + """ + + ACCEPTED = { + "http url": {"docs": {"type": "http", "url": "https://example.test/mcp"}}, + "sse url": {"docs": {"type": "sse", "url": "https://example.test/sse"}}, + "url with headers": { + "docs": {"type": "http", "url": "https://example.test/mcp", "headers": {"A": "b"}} + }, + "command": {"fs": {"command": "npx", "args": ["-y", "server"]}}, + "bundled cao server": {"cao-mcp-server": {"command": "cao-mcp-server", "args": []}}, + "command and url together": {"z": {"command": "npx", "url": "https://example.test/mcp"}}, + } + + @pytest.mark.parametrize("label", sorted(ACCEPTED)) + def test_supported_forms_validate(self, label: str) -> None: + findings = validate_frontmatter( + {"name": "x", "description": "d", "mcpServers": self.ACCEPTED[label]} + ) + + assert [f for f in findings if f.severity == "error"] == [] + + @pytest.mark.parametrize( + "entry", [{"type": "http"}, {}, {"args": ["-y"]}], ids=["type only", "empty", "args only"] + ) + def test_an_entry_with_neither_command_nor_url_is_rejected(self, entry: dict) -> None: + """Widening the rule must not widen it into accepting anything. + + An entry naming no transport cannot be launched or reached, so the gate + still has to catch it -- the fix is a second permitted shape, not the + removal of the requirement. + """ + findings = validate_frontmatter( + {"name": "x", "description": "d", "mcpServers": {"broken": entry}} + ) + errors = [f for f in findings if f.severity == "error"] + + assert len(errors) == 1 + assert errors[0].path == "mcpServers.broken" + + def test_url_is_described_rather_than_merely_tolerated(self) -> None: + """The field is typed, so a form generator can render it and catch a typo. + + The inner object does not set ``additionalProperties: false``, so a url + entry would pass even with no ``url`` property declared. Declaring it is + what makes ``GET /agents/profiles/schema`` describe the shape, and what + makes a wrong type a finding. + """ + inner = load_profile_schema()["properties"]["mcpServers"]["additionalProperties"] + assert inner["properties"]["url"] == {"type": "string"} + assert inner["anyOf"] == [{"required": ["command"]}, {"required": ["url"]}] + + findings = validate_frontmatter( + {"name": "x", "description": "d", "mcpServers": {"docs": {"url": 7}}} + ) + + assert any(f.severity == "error" and f.path == "mcpServers.docs.url" for f in findings) From 6fe1f8f4927dc0af778002674a95ca1804f38240 Mon Sep 17 00:00:00 2001 From: Sujoy Datta Choudhury Date: Sat, 15 Aug 2026 11:39:40 -0700 Subject: [PATCH 5/5] fix(api): bound profile expansion before validating, not mid-traversal A strawman of the previous commit found that its headline claim was only half true. It closed a CPU amplification in this module's own key walk and left a larger allocation vector open on the same unauthenticated route. jsonschema builds every error message eagerly, interpolating repr of the offending instance. YAML aliases resolve to repeated references to one object, so a document whose expansion is exponential in its byte count produces an error message that is too: a 651-byte body with 20 anchor levels that trips a single type error yielded a 25 MB message, 101 MB at 22 levels, doubling per level, which puts ~26 levels in the gigabytes. That string is then serialised into the response. Allocation is the ceiling, not CPU, and no care taken in this module's own traversal avoids it. Confirmed pre-existing rather than introduced here: byte-identical numbers on pristine origin/main, where the route was already scope-exempt and already async. Fixed here anyway. It is the same route and the same class of bug as the finding it sits behind, and shipping "the traversal is bounded" while a larger vector remains on that endpoint would be the same overstatement this PR has now corrected four times. _structural_bound_finding counts the values a fully expanded rendering would contain, memoized on id() so the count stays linear in distinct objects and capped so an enormous document costs no more to reject than a borderline one. It runs before the key walk and before jsonschema, and validate_frontmatter returns as soon as it reports, because continuing would pay exactly the cost the ceiling exists to avoid. Expressing the ceiling as expanded size rather than traversal steps also fixes a message that lied. The previous commit decremented a budget per edge traversed while naming it _MAX_WALK_VALUES and reporting "holds more than 20000 values", so an 84 KB document holding four values, one of them aliased 21,000 times, was rejected for holding twenty thousand. It now reports what is actually counted: that document does expand to ~21,004 values. Reachable inside the 256 KB content cap, so not hypothetical. The walk loses its own size budget as redundant. With identity memoization it is linear in the document's distinct containers, and it now only runs on documents the ceiling accepted. Two ratios stated separately, because they are not the same: against the largest bundled profile's 23 expanded values and depth of 3, the ceilings sit ~870x and ~21x above it. The previous commit's comment said "~1000x" of both, which was wrong by ~50x for depth. Also documents, rather than leaves implicit, that the two halves of validate_frontmatter report shared values differently. A shared value that is schema-invalid yields one finding per referencing path, since jsonschema does not memoize; a shared non-string key yields exactly one. Both are right, but a client rendering findings should not assume one convention. Tests: the anchor bomb is now rejected rather than accepted, so the assertions become deterministic. Rejection is asserted on the error message and on response size (under 2 KB for a body that previously returned 25 MB) rather than on elapsed time, which means a regression fails instead of hanging until CI's job timeout, as the earlier timing-only assertions would have. The memoization proof moves to 10 anchor levels, 1024 paths to one bad key, which stays under the ceiling so the walk still runs and one finding still proves dedup. 98 net new tests. Full suite 6,795 passed. --- docs/api.md | 35 +-- .../services/profile_validator.py | 199 ++++++++++++------ test/api/test_api_profile_surface.py | 77 +++++-- test/services/test_profile_validator.py | 128 ++++++----- 4 files changed, 288 insertions(+), 151 deletions(-) diff --git a/docs/api.md b/docs/api.md index 7886fd132..4d06fc545 100644 --- a/docs/api.md +++ b/docs/api.md @@ -87,21 +87,25 @@ See [AG-UI](agui.md) for enablement, event shapes, and privacy boundaries. validates clean and persists, then fails to load, since the model requires string keys. Note YAML also auto-types an unquoted date, so `2026-01-01:` is a date key rather than a string; quote such keys. -- That key check walks the parsed document, and the walk is bounded, because YAML - anchors make a document's value graph arbitrarily larger than its bytes: each - alias resolves to another reference to the same object, so chained anchors give - a sub-kilobyte body an exponential number of paths. Containers already visited - are skipped, which removes the amplification and still reports each offending - key once, at the first path that reaches it. Separate ceilings on total values - (20,000) and nesting depth (64) bound a document that is merely enormous rather - than aliased; exceeding either is itself an error, so such a document is - rejected rather than reported clean on a partial walk. Both bounds are ~1000x - the largest bundled profile. +- That key check walks the parsed document, and a document is rejected up front if + it is too large to inspect, because YAML anchors make a document's value graph + arbitrarily larger than its bytes: each alias resolves to another reference to + the same object, so chained anchors give a sub-kilobyte body an exponential + expansion. Two ceilings, both far above real input, which for the largest + bundled profile is 23 expanded values nested 3 deep: a document may expand to at + most 20,000 values (~870x) and nest at most 64 levels (~21x). Exceeding either + is itself an error, and nothing further runs, since the later steps are what + such a document is expensive in. Within the ceilings, containers already visited + are skipped, so each offending key is reported once, at the first path that + reaches it. Note that differs from the schema step, which does not memoize and + so reports a shared invalid value once per referencing path. - An `mcpServers` entry must define either `command`, for a server CAO launches, or `url`, for a remote one whose `type` names its transport. The schema previously required `command` unconditionally, which made the write routes reject url-based servers that the runtime accepts and passes through to the - provider unchanged. An entry defining neither is still rejected. + provider unchanged. An entry defining neither is still rejected. `url` is the + spelling `resolve_mcp_server_config` documents; an entry naming its endpoint + under any other key satisfies neither branch and is rejected. - Every 400 from the profile write and source routes uses one `detail` shape, `{"message", "errors"}`, so a client never has to switch on the type of `detail`. `errors` is empty for a failure that is not attributable to a field, @@ -114,11 +118,10 @@ See [AG-UI](agui.md) for enablement, event shapes, and privacy boundaries. profile, having applied `${VAR}` substitution from the managed environment file to the raw text before parsing. Round-tripping a resolved document through a write would persist substituted values into a plaintext profile. - Requires `cao:read`, `cao:write`, or `cao:admin`. The pre-existing profile - reads beside it are ungated and stay that way, since tightening a shipped - route could break an existing unauthenticated reader; this one is gated - because it returns the stored bytes verbatim from every configured store, - including documents that fail to parse. + Requires `cao:read`, `cao:write`, or `cao:admin`, the same guard the profile + reads beside it now carry. Gating matters at least as much here as on the parsed + route, because this one returns the stored bytes verbatim from every configured + store, including documents that fail to parse. - Template validation and preview require the selected template to include a `schema.json` file. - `/agents/providers` reports provider availability. diff --git a/src/cli_agent_orchestrator/services/profile_validator.py b/src/cli_agent_orchestrator/services/profile_validator.py index 30cd1a800..b6c3775ea 100644 --- a/src/cli_agent_orchestrator/services/profile_validator.py +++ b/src/cli_agent_orchestrator/services/profile_validator.py @@ -71,12 +71,91 @@ def load_profile_schema() -> dict: return json.loads(schema_path.read_text(encoding="utf-8")) -# Ceilings for the key walk below, on a document that is genuinely huge or -# deeply nested rather than aliased. Generous by ~1000x: the largest bundled -# profile's frontmatter parses to 23 values and nests 3 deep, so no legitimate -# profile approaches either bound. -_MAX_WALK_VALUES = 20_000 -_MAX_WALK_DEPTH = 64 +# Structural ceilings, applied before anything else walks or validates a +# document. Both sit far above real input: the largest bundled profile's +# frontmatter expands to 23 values and nests 3 deep, which these clear by ~870x +# and ~21x respectively. +_MAX_EXPANDED_VALUES = 20_000 +_MAX_DEPTH = 64 + + +def _structural_bound_finding(metadata: object) -> Optional["ValidationMessage"]: + """Reject a document too large to hand to the rest of the validator. + + Size here is the number of values a *fully expanded* rendering would contain, + not the number of distinct objects the parser produced. The two diverge + without limit: ``yaml.safe_load`` resolves every alias to another reference to + the *same* object, so N chained anchors that each reference the previous one + twice give ~N distinct objects whose expansion is 2**N values, out of a + sub-kilobyte body. + + That expansion, not the parsed size, is what the steps downstream pay for: + + - jsonschema builds every error message eagerly, interpolating ``repr`` of the + offending instance. A 651-byte document with 20 anchor levels that trips one + ``type`` error produced a single 25 MB message here, doubling per added + level, so ~26 levels reaches gigabytes. Allocation is the ceiling there, not + CPU, and no amount of care in this module's own traversal avoids it. + - the key walk below visits each distinct container once, so it is already + linear in the parsed structure. It is bounded here only in the sense that it + is *reached* on documents this function accepted. + + Both land on ``POST /agents/profiles/validate`` in particular: it is + scope-exempt, so it answers without credentials even when OAuth is + configured, and it is declared ``async``, so work on its thread delays every + other request rather than only the caller's own. + + Counted with memoization on ``id()``, which keeps the count itself linear in + distinct objects, and capped so an enormous document costs no more to reject + than one sitting just under the ceiling. Comparing identity is sound here + because every value stays reachable from ``metadata`` throughout, so nothing + can be collected and no id recycled midway. A container reached again while + still being counted is a cycle and contributes 1. + + Returns: + An error finding naming the ceiling that was exceeded, or ``None`` when + the document is within both. + """ + memo: dict[int, int] = {} + ceiling = _MAX_EXPANDED_VALUES + 1 + too_deep = False + + def expanded(value: object, depth: int) -> int: + nonlocal too_deep + if not isinstance(value, (dict, list)): + return 1 + if depth > _MAX_DEPTH: + too_deep = True + return 1 + identity = id(value) + if identity in memo: + return memo[identity] + memo[identity] = 1 # Cycle guard, in force while this container counts. + total = 1 + for child in value.values() if isinstance(value, dict) else value: + total += expanded(child, depth + 1) + if total >= ceiling: + total = ceiling + break + memo[identity] = total + return total + + size = expanded(metadata, 0) + + if too_deep: + return ValidationMessage( + "error", + f"Frontmatter nests more than {_MAX_DEPTH} levels deep, past what this " + f"validator will inspect. Flatten the document.", + ) + if size >= ceiling: + return ValidationMessage( + "error", + f"Frontmatter expands to more than {_MAX_EXPANDED_VALUES} values, past " + f"what this validator will inspect. If it uses YAML anchors, note that " + f"each alias expands in full. Simplify the document.", + ) + return None def _non_string_key_findings(metadata: object) -> list["ValidationMessage"]: @@ -99,55 +178,47 @@ def _non_string_key_findings(metadata: object) -> list["ValidationMessage"]: unquoted dates, so ``2026-01-01:`` becomes a ``datetime.date`` key. Checking the key type generally covers those without enumerating them. - **Why the walk is bounded.** YAML anchors make a document's value *graph* + **Why the walk memoizes.** YAML anchors make a document's value *graph* arbitrarily larger than its bytes: ``yaml.safe_load`` resolves each alias to another reference to the *same* object, so N chained anchors that each reference the previous one twice build a graph an unmemoized walk traverses 2**N times while memory stays linear. The first version of this function - carried only a depth cap, which bounded the wrong dimension -- depth was - never the problem, revisiting shared objects was -- and a 640-byte, - schema-valid document with 20 anchor levels took ~1s here against ~0s in - jsonschema, doubling per added level. That is reachable unauthenticated: - ``POST /agents/profiles/validate`` is scope-exempt and declared ``async``, - so a synchronous walk on its thread stalls the whole event loop. - - Two bounds, each covering what the other does not: - - - ``seen`` skips any container already walked, keyed on ``id()``. This - removes the amplification at its source and costs no coverage: a shared - subtree cannot hold a different set of keys on a second visit, so one - finding per offending key is the correct output, reported at the first - path that reaches it. Comparing identity is sound here specifically - because every value stays reachable from ``metadata`` for the duration of - the walk, so nothing can be collected and no id can be recycled midway. - - ``_MAX_WALK_VALUES`` and ``_MAX_WALK_DEPTH`` bound a document that is - merely enormous, which memoizing identity does not. Exceeding either adds - an **error** finding, so such a document is rejected rather than quietly - called valid on the strength of a partial walk. + carried only a depth cap, which bounded the wrong dimension: depth was never + the problem, revisiting shared objects was. A 640-byte, schema-valid document + with 20 anchor levels took ~1s here against ~0s in jsonschema, doubling per + added level, on a route that is scope-exempt and ``async``. + + ``seen`` skips any container already walked, keyed on ``id()``. That removes + the amplification at its source and needs no size ceiling of its own: the walk + is linear in the document's *distinct* containers, and + :func:`_structural_bound_finding` has already rejected anything whose expansion + is large before this runs. Comparing identity is sound because every value + stays reachable from ``metadata`` for the duration of the walk, so nothing can + be collected and no id recycled midway. + + Skipping repeats costs no coverage: a shared subtree cannot hold a different + set of keys on a second visit, so one finding per offending key is the correct + output. Worth knowing that this makes the two halves of + :func:`validate_frontmatter` report shared values differently. A shared value + that is *schema*-invalid yields one finding per referencing path, because + jsonschema does not memoize, while a shared *non-string key* yields exactly + one, at whichever path reached it first. Both are defensible; a client + highlighting findings against a document should not assume one convention. Args: - metadata: Any parsed YAML value. Only mappings and sequences are + metadata: Any parsed YAML value, already accepted by + :func:`_structural_bound_finding`. Only mappings and sequences are descended into. Returns: - Error findings in document order: one per offending key, plus a final - one if a bound was reached. + One error finding per offending key, in document order. """ findings: list[ValidationMessage] = [] seen: set[int] = set() - remaining = _MAX_WALK_VALUES - limit_reached: Optional[str] = None - - def walk(value: object, path: str, depth: int) -> None: - nonlocal remaining, limit_reached + def walk(value: object, path: str) -> None: if not isinstance(value, (dict, list)): return # A scalar has no keys, and nothing to descend into. - if limit_reached is not None: - return - if depth > _MAX_WALK_DEPTH: - limit_reached = f"is nested more than {_MAX_WALK_DEPTH} levels deep" - return if id(value) in seen: return seen.add(id(value)) @@ -174,25 +245,9 @@ def walk(value: object, path: str, depth: int) -> None: ] for child_path, child in children: - remaining -= 1 - if remaining <= 0: - limit_reached = f"holds more than {_MAX_WALK_VALUES} values" - return - walk(child, child_path, depth + 1) - if limit_reached is not None: - return - - walk(metadata, "", 0) - - if limit_reached is not None: - findings.append( - ValidationMessage( - "error", - f"Frontmatter {limit_reached}, past the bound this validator will " - f"traverse, so its mapping keys cannot be fully checked. Simplify " - f"the document.", - ) - ) + walk(child, child_path) + + walk(metadata, "") return findings @@ -204,6 +259,10 @@ def validate_frontmatter(metadata: dict) -> list[ValidationMessage]: mapping keys, then JSON-Schema errors sorted by path, then ``allowedTools`` vocabulary warnings, then the role check. An empty list means the profile is valid with no advisories. + + A document outside the structural ceilings is the one exception to that + order: it is reported and nothing further runs, because the later steps are + exactly what such a document is expensive in. """ messages: list[ValidationMessage] = [] @@ -219,13 +278,27 @@ def validate_frontmatter(metadata: dict) -> list[ValidationMessage]: ) ) - # 2. Non-string mapping keys, which JSON Schema cannot see. Reported before + # 2. Structural ceilings, before anything traverses or validates the + # document. + # + # Returning here rather than continuing is the whole point of the check. + # Step 3 is linear in distinct containers, but step 4 hands the document + # to jsonschema, which interpolates ``repr`` of an offending instance into + # every error message it builds -- so on an alias-amplified document, + # reporting the ceiling and then running the remaining steps anyway would + # pay the exact cost the ceiling exists to avoid. + structural = _structural_bound_finding(metadata) + if structural is not None: + messages.append(structural) + return messages + + # 3. Non-string mapping keys, which JSON Schema cannot see. Reported before # the schema errors because a document with a non-string key is outside # the format entirely, and because the schema's own findings for such a # document tend to be confusing. messages.extend(_non_string_key_findings(metadata)) - # 3. JSON-Schema structural validation. + # 4. JSON-Schema structural validation. # # The sort key stringifies each path component. Raw components are whatever # the document used as mapping keys, so a profile with mixed-type keys (for @@ -238,7 +311,7 @@ def validate_frontmatter(metadata: dict) -> list[ValidationMessage]: path = ".".join(str(p) for p in error.absolute_path) or "(root)" messages.append(ValidationMessage("error", error.message, path)) - # 4. allowedTools vocabulary check (advisory, not blocking). + # 5. allowedTools vocabulary check (advisory, not blocking). # # Each entry is type-checked before the membership test. ``_VALID_TOOL_VOCAB`` # is a set, so ``tool not in`` hashes ``tool``, and an unhashable element @@ -259,7 +332,7 @@ def validate_frontmatter(metadata: dict) -> list[ValidationMessage]: ) ) - # 5. Role check (advisory — custom roles are valid but worth flagging). + # 6. Role check (advisory — custom roles are valid but worth flagging). # # Same hashing hazard as above: ``role: [developer]`` is unhashable. The # schema reports the type error, so this advisory check simply stands aside. diff --git a/test/api/test_api_profile_surface.py b/test/api/test_api_profile_surface.py index 62de6a74a..39cc065ee 100644 --- a/test/api/test_api_profile_surface.py +++ b/test/api/test_api_profile_surface.py @@ -6,7 +6,6 @@ paths as the CLI instead of reimplementing them. """ -import time from unittest.mock import patch import pytest @@ -922,46 +921,86 @@ def test_non_field_failures_carry_an_empty_error_list(self, client, write_store) class TestValidateEndpointResistsAliasAmplification: - """The unauthenticated validate route must not be stallable by its body. - - Reported as a P2 in round 2 of review on #585. The non-string mapping key - check added in round 1 walked the parsed document with only a depth cap, but - YAML anchors resolve to repeated references to one object, so a walk that - does not remember where it has been revisits shared subtrees exponentially. - A sub-kilobyte body reached seconds of CPU and doubled per anchor level. - - This route is the exposed one: it is in the scope-exemption set, so it - answers without credentials even when OAuth is configured, and it is declared - ``async``, so a synchronous walk on its thread blocks the event loop for every - other request rather than just the attacker's own. That exemption is pinned in + """The unauthenticated validate route must not be stallable or OOM'd by its body. + + Two findings, both on this route. Round 2 of review on #585 added a + non-string mapping key check bounded only by a depth cap, so the walk + revisited alias-shared subtrees exponentially. Round 3's own strawman then + found the larger half: jsonschema interpolates ``repr`` of an offending + instance into every error message it builds, so an amplified value that trips + one ``type`` error yielded a 25 MB message at 20 anchor levels and 101 MB at + 22, which the route would then serialise into its response body. + + This route is the exposed one: it is in the scope-exemption set, so it answers + without credentials even when OAuth is configured, and it is declared + ``async``, so work on its thread delays every other request rather than only + the caller's own. That exemption is pinned in ``test/api/test_scope_coverage.py::_EXEMPT``, which is the one place it is asserted; if it is ever removed, the reasoning here changes. + + Both are closed by rejecting a document whose expansion exceeds a ceiling, + ahead of either step. The assertions are on status and response size rather + than elapsed time, so a regression fails rather than hanging to CI's timeout. """ @staticmethod - def _bomb(levels: int) -> str: + def _bomb(levels: int, tail: str = "") -> str: lines = [ "---", "name: bomb", "description: A profile.", - "toolsSettings:", + "hooks:", " a0: &a0 {k: v}", ] for level in range(1, levels + 1): lines.append(f" a{level}: &a{level} {{x: *a{level - 1}, y: *a{level - 1}}}") + if tail: + lines.append(tail) return "\n".join(lines) + "\n---\n\nBody.\n" - def test_an_anchor_bomb_is_answered_promptly(self, client) -> None: + def test_an_anchor_bomb_is_rejected_with_a_small_response(self, client) -> None: content = self._bomb(40) assert len(content) < 1500 - started = time.perf_counter() response = client.post("/agents/profiles/validate", json={"content": content}) - elapsed = time.perf_counter() - started + + assert response.status_code == 200 + body = response.json() + assert body["valid"] is False + assert any("expands to more than" in m["message"] for m in body["messages"]) + assert ( + len(response.content) < 2000 + ), f"a {len(content)}-byte body produced a {len(response.content)}-byte response" + + def test_a_bomb_that_trips_a_schema_error_does_not_render_itself(self, client) -> None: + """The response must not grow with the bomb. + + Before the ceiling, the offending instance here was the amplified node, so + the schema error's message was its full expansion: 25 MB of JSON out of a + sub-kilobyte request. + """ + for levels in (20, 22, 30): + content = self._bomb(levels, tail="toolsSettings: [*a%d]" % levels) + + response = client.post("/agents/profiles/validate", json={"content": content}) + + assert response.status_code == 200, levels + assert response.json()["valid"] is False, levels + assert ( + len(response.content) < 2000 + ), f"{levels} levels produced a {len(response.content)}-byte response" + + def test_a_profile_using_anchors_normally_is_still_valid(self, client) -> None: + """The ceiling is on expansion, not on anchors.""" + content = ( + "---\nname: shared\ndescription: A profile.\ntoolsSettings:\n" + " common: &common {timeout: 30}\n fs: *common\n web: *common\n---\n\nBody.\n" + ) + + response = client.post("/agents/profiles/validate", json={"content": content}) assert response.status_code == 200 assert response.json()["valid"] is True - assert elapsed < 10.0, f"validate took {elapsed:.2f}s on a {len(content)}-byte body" class TestUrlBasedMcpServersAreWritable: diff --git a/test/services/test_profile_validator.py b/test/services/test_profile_validator.py index 223261bb0..4bd108c21 100644 --- a/test/services/test_profile_validator.py +++ b/test/services/test_profile_validator.py @@ -9,8 +9,6 @@ Ref: https://github.com/awslabs/cli-agent-orchestrator/issues/510 """ -import time - import pytest from cli_agent_orchestrator.models.agent_profile import AgentProfile @@ -334,96 +332,120 @@ def test_well_formed_values_still_warn(self) -> None: assert any(f.severity == "warning" for f in role_findings) -def _alias_amplified_yaml(levels: int, leaf: str = "{k: v}") -> str: - """A schema-valid profile whose value graph is 2**``levels`` paths. +def _alias_amplified_yaml(levels: int, leaf: str = "{k: v}", tail: str = "") -> str: + """A profile whose *expanded* value count is exponential in ``levels``. Each anchor references the previous one twice, so ``yaml.safe_load`` returns - ``levels + 1`` dicts while an unmemoized walk sees an exponential number of - paths through them. Nested under ``toolsSettings`` because that field is a - free-form object, which keeps the document *valid* -- the point being that a - rejected document would never reach a full traversal anyway. + ``levels + 1`` dicts while a full expansion of them contains ~2**levels + values. Nested under ``toolsSettings``/``hooks`` because those fields are + free-form objects, which keeps the document otherwise *valid*: a document + rejected on its own merits would never reach the expensive steps anyway. + + ``tail`` appends a final line, used to plant a schema error whose offending + instance is the amplified node. """ - lines = ["---", "name: bomb", "description: A profile.", "toolsSettings:", f" a0: &a0 {leaf}"] + lines = ["---", "name: bomb", "description: A profile.", "hooks:", f" a0: &a0 {leaf}"] for level in range(1, levels + 1): lines.append(f" a{level}: &a{level} {{x: *a{level - 1}, y: *a{level - 1}}}") + if tail: + lines.append(tail) return "\n".join(lines) + "\n---\n\nBody.\n" class TestAliasAmplificationIsBounded: - """A YAML-anchor bomb must not stall the key walk. - - Round 2 of review on #585 added a non-string mapping key check whose only - bound was a recursion depth cap. That bounded the wrong dimension: YAML - aliases resolve to repeated references to the *same* object, so the walk - revisited shared subtrees exponentially while the document stayed tiny. A - 640-byte, schema-valid body took ~1s, doubling per added anchor level, on a - scope-exempt ``async`` route -- a denial of service reachable without - credentials. Reported by @haofeif. - - The fix skips containers already walked, keyed on identity, so these tests - pin both halves of that: the traversal terminates, and skipping repeats does - not lose a finding. + """A YAML-anchor bomb must not reach anything that pays for its expansion. + + Two rounds of review on #585 landed here. Round 2 added a non-string mapping + key check whose only bound was a recursion depth cap, which bounded the wrong + dimension: aliases resolve to repeated references to the *same* object, so the + walk revisited shared subtrees exponentially while the document stayed tiny. A + 640-byte body took ~1s, doubling per anchor level. Reported by @haofeif. + + Round 3's own strawman then found the larger half. jsonschema builds each error + message eagerly, interpolating ``repr`` of the offending instance, so an + amplified value that trips one ``type`` error produced a 25 MB message at 20 + levels and 101 MB at 22, which is an allocation ceiling rather than a stall and + was reachable on merged ``main`` independently of this PR. + + Both are now closed ahead of either step, by rejecting a document whose + expansion exceeds a ceiling. Every assertion below is deterministic: a + regression fails on a count or a length rather than hanging until CI's job + timeout, which is what the earlier timing-only assertions would have done. """ - def test_a_forty_level_bomb_validates_promptly(self) -> None: - """Forty levels is 2**40 paths: unbounded, this never returns.""" + def test_an_anchor_bomb_is_rejected_rather_than_traversed(self) -> None: document = _alias_amplified_yaml(40) - assert len(document) < 1500 # the whole point: tiny input, huge graph + assert len(document) < 1500 # the whole point: tiny input, huge expansion - started = time.perf_counter() findings = validate_profile_text(document) - elapsed = time.perf_counter() - started + errors = [f for f in findings if f.severity == "error"] + + assert len(errors) == 1 + assert "expands to more than" in errors[0].message + + def test_the_rejection_does_not_grow_with_the_bomb(self) -> None: + """Rejecting must not itself render the document. + + This is the regression guard for the jsonschema message vector: the + offending instance below is the amplified node, so before the ceiling + existed the returned message *was* its full ``repr``. Asserting a bound on + the response size catches that without measuring time. + """ + for levels in (20, 22, 30): + document = _alias_amplified_yaml(levels, tail="toolsSettings: [*a%d]" % levels) - assert findings == [] - # Measured at ~0.0001s. The bound is loose enough to survive a loaded - # CI runner while still being unreachable for an exponential walk. - assert elapsed < 5.0, f"walk took {elapsed:.2f}s; the traversal bound is not holding" + findings = validate_profile_text(document) - def test_a_self_referential_document_terminates(self) -> None: - """An anchor that contains itself is a cycle, not merely deep nesting.""" + assert len(findings) == 1, levels + assert len(findings[0].message) < 1000, ( + f"{levels} levels produced a {len(findings[0].message)}-char message; " + f"the offending instance is being rendered" + ) + + def test_a_self_referential_document_is_accepted(self) -> None: + """An anchor containing itself is a cycle, and expands to almost nothing.""" document = ( "---\nname: cyc\ndescription: A profile.\ntoolsSettings: &c {self: *c}\n---\n\nB.\n" ) - started = time.perf_counter() - findings = validate_profile_text(document) - - assert findings == [] - assert time.perf_counter() - started < 5.0 + assert validate_profile_text(document) == [] def test_a_bad_key_in_a_shared_subtree_is_reported_exactly_once(self) -> None: """Deterministic proof of the memoization, with no reliance on a clock. - The offending key sits in the one node every alias resolves to. Reported - once, it confirms shared nodes are visited once; the pre-fix walk would - have emitted 2**20 copies of the same finding. + Ten levels is 1024 paths to the single node every alias resolves to, and + expands to well under the ceiling so the walk still runs. One finding + confirms shared nodes are visited once; the unmemoized walk emitted 1024 + copies of it. """ - document = _alias_amplified_yaml(20, leaf="{1: one}") + document = _alias_amplified_yaml(10, leaf="{1: one}") findings = validate_profile_text(document) key_errors = [f for f in findings if "not a string" in f.message] assert len(key_errors) == 1 assert key_errors[0].severity == "error" - assert key_errors[0].path == "toolsSettings.a0.1" + assert key_errors[0].path == "hooks.a0.1" def test_legitimate_anchor_reuse_still_validates_clean(self) -> None: - """Anchors are a normal YAML convenience, not inherently suspect.""" + """Anchors are a normal YAML convenience, not inherently suspect. + + The ceiling is on expansion, not on aliasing, so ordinary reuse has to + pass. Without this, satisfying the bound by rejecting anchors outright + would look like a fix. + """ document = ( "---\nname: shared\ndescription: A profile.\ntoolsSettings:\n" " common: &common {timeout: 30}\n fs: *common\n web: *common\n---\n\nBody.\n" ) - findings = validate_profile_text(document) - - assert findings == [] + assert validate_profile_text(document) == [] - def test_exceeding_a_bound_is_an_error_not_silence(self) -> None: - """A document too large to traverse is rejected, not called valid. + def test_exceeding_a_ceiling_is_an_error_not_silence(self) -> None: + """A document past a ceiling is rejected, not called valid. - Identity memoization bounds an *aliased* document; these bounds cover one - that is merely enormous. Returning no findings there would report an - unchecked document as clean, which is the failure mode being avoided. + Reporting nothing would present an uninspected document as clean, which is + the failure mode of the depth cap this replaced: it returned an empty list. """ deep: dict = {"name": "deep", "description": "A profile."} node = deep @@ -436,7 +458,7 @@ def test_exceeding_a_bound_is_an_error_not_silence(self) -> None: "toolsSettings": {f"k{index}": index for index in range(25_000)}, } - for metadata, expected in ((deep, "nested more than"), (wide, "holds more than")): + for metadata, expected in ((deep, "nests more than"), (wide, "expands to more than")): errors = [f for f in validate_frontmatter(metadata) if f.severity == "error"] assert len(errors) == 1 assert expected in errors[0].message