deer-flow/backend/tests/test_skills_router_authz.py
Xinmin Zeng f2e832330e
fix(sandbox): enforce disabled skills in filesystem views (#4178)
* fix(sandbox): project enabled skills into sandbox views

* fix(skills): keep projection mutations consistent

* fix(skills): fail closed on projection errors

* fix(skills): isolate per-scope failures during boot projection rebuild

rebuild_all_skill_projections() propagated any exception from the public
rebuild or from a single user's rebuild straight out of the gateway
lifespan startup, uncaught. A single broken user directory (bad
permissions, corrupted _skill_states.json, unreadable content) would
therefore abort gateway boot for every user, not just that one -
_rebuild_*_locked already fails closed internally (clears the view and
re-raises), so the boot loop only needed to stop treating that re-raise
as fatal.

Each scope's rebuild now fails closed independently and boot continues;
a scope left empty by a boot failure self-heals on the next sandbox
acquire via ensure_skill_projections().

Also patches deerflow.skills.projection.rebuild_all_skill_projections in
the memory-flush lifespan test fixture, matching the two sibling
fixtures in the same file — this call is now on the lifespan startup
path and the fixture's minimal SimpleNamespace config predates it.

* test(skills): update authz test for the projection-aware public toggle

_persist_shared_skill_state (introduced earlier in this branch) reads
the shared extensions_config.json fresh from disk under the projection
lock instead of through the cached get_extensions_config() singleton -
that's the whole point of the fix (stale worker caches must not clobber
another worker's concurrent update). The name no longer exists on the
skills router module, so the test's monkeypatch of it started raising
AttributeError instead of exercising the endpoint.

The mock storage in this test isn't a real LocalSkillStorage instance,
so _persist_shared_skill_state's projection-mutation branch is already
skipped (nullcontext) and it falls back to a fresh ExtensionsConfig()
for the nonexistent tmp config_path - no replacement monkeypatch needed.

* fix(sandbox): make skill projection ensure best-effort in acquire

acquire() called _ensure_skills_projection() directly, outside any
try/except, in both LocalSandboxProvider and AioSandboxProvider. Every
other skill-mount setup path in these providers has always caught
exceptions and logged a warning rather than failing sandbox acquire
outright (e.g. when config.yaml can't be resolved) - these two new call
sites broke that contract, so any projection failure (including simply
not having a config.yaml, as in CI's test environment) now failed
acquire() itself instead of just leaving skill mounts off.

_ensure_skills_projection now catches its own exceptions and returns
None; both providers' callers already tolerate that (a None projection
skips the skill-specific mounts, matching the existing degrade path)
after making _append_public_skill_mapping and the custom/legacy mount
block in LocalSandboxProvider explicitly None-safe.

Caught by running the full suite with config.yaml removed, matching
CI's environment - not caught locally because a real config.yaml was
present, masking the failure.

* fix(sandbox): make E2B skill projection mounts best-effort

_skill_projection_mounts called ensure_skill_projections with no guard,
unlike Local/AIO's _ensure_skills_projection. A raise propagated out of
_apply_mounts before the configured-mounts loop ran, so a skills
projection failure dropped the operator's own configured mounts too -
only caught by create()'s outer warning, with nothing applied at all.

Swallow here and return an empty mount list on failure, matching the
Local/AIO pattern: still fail-closed for skills, but no longer widens
the blast radius to unrelated configured mounts.

Review feedback from PR #4178.

* docs(skills): document projection trade-offs flagged in review

- _update_tree_digest: note the metadata-only (not content) hashing
  trade-off and why runtime writes through this codebase are still
  covered regardless (rebuild-under-lock + rename always changes inode).
- LocalSandboxProvider.acquire: note the acquire-time self-heal cost
  (cheap on a fresh manifest, ~400ms rebuild under lock on stale/drift).
- skill_projection_mutation: drop the no-op except-Exception-then-raise;
  a raise from the mutation already propagates past the yield with the
  view left cleared, no explicit re-raise needed.
- provisioner README: spell out that hostPath skills volumes require
  the gateway and K8s node to share DEER_FLOW_HOST_BASE_DIR (single-node
  or shared storage), and that the custom/legacy volumes' hostPath type
  Directory (not DirectoryOrCreate) makes a violation of that assumption
  a visible Pod-creation failure instead of a silent empty mount.

Review feedback from PR #4178.

* fix(skills): lazily repair user projections

* fix(skills): close projection review gaps

* fix(skills): refresh user projection enable state

* fix(skills): close projection review follow-ups

* fix(skills): preserve state across projection writes

---------

Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
2026-07-31 17:55:24 +08:00

172 lines
7.1 KiB
Python

"""Authorization regression tests for the skills router.
Custom skill SKILL.md content is injected into every user's agent system
prompt. The mutating endpoints that write global shared state (install,
toggle PUBLIC skills, edit/delete custom skill content, and the endpoints
that expose raw custom-skill content/history) must be admin-only, matching
the MCP router which guards the equivalent global extensions_config mutations
with ``require_admin_user``.
Under per-user skill isolation, ``list_custom_skills`` is open to all
authenticated users (they see only their own custom skills), but all other
custom-skill endpoints remain admin-only because they write global state
(install writes to the shared archive, toggle writes extensions_config.json
for PUBLIC skills, and edit/delete modify the on-disk skill tree).
These tests pin the access-control boundary: a normal authenticated
(non-admin) user must receive 403 on every guarded endpoint.
"""
from __future__ import annotations
import json
from types import SimpleNamespace
from uuid import uuid4
from _router_auth_helpers import make_authed_test_app
from fastapi import FastAPI
from fastapi.testclient import TestClient
from app.gateway.auth.models import User
from app.gateway.deps import get_config
from app.gateway.routers import skills as skills_router
def _make_user(system_role: str) -> User:
return User(email=f"{system_role}-test@example.com", password_hash="x", system_role=system_role, id=uuid4())
def _make_app(*, system_role: str) -> FastAPI:
config = SimpleNamespace(
skills=SimpleNamespace(get_skills_path=lambda: "/tmp/skills", container_path="/mnt/skills", use="deerflow.skills.storage.local_skill_storage:LocalSkillStorage"),
skill_evolution=SimpleNamespace(enabled=True, moderation_model_name=None),
)
app = make_authed_test_app(user_factory=lambda: _make_user(system_role))
app.state.config = config
app.dependency_overrides[get_config] = lambda: config
app.include_router(skills_router.router)
return app
# (method, path, json_body) for every endpoint that must require admin.
# Under per-user skill isolation, list_custom_skills is open to normal users
# (they see only their own skills), so it is NOT in this list.
# All other mutating endpoints write/read global shared state and must be
# admin-only. PUT /api/skills/{name} is included: toggling enabled writes
# the shared extensions_config.json (for PUBLIC skills) and changes every
# tenant's injected skill set.
_GUARDED_ENDPOINTS = [
("post", "/api/skills/install", {"thread_id": "t1", "path": "mnt/user-data/outputs/x.skill"}),
("post", "/api/skills/reload", None),
("get", "/api/skills/custom/demo", None),
("put", "/api/skills/custom/demo", {"content": "---\nname: demo\ndescription: hijacked\n---\n"}),
("delete", "/api/skills/custom/demo", None),
("get", "/api/skills/custom/demo/history", None),
("post", "/api/skills/custom/demo/rollback", {"history_index": -1}),
("put", "/api/skills/demo", {"enabled": False}),
]
def test_non_admin_is_forbidden_on_all_mutating_skills_endpoints():
"""A normal (non-admin) authenticated user must get 403, never 200/500.
403 proves the admin guard fired before any business logic ran. If the
guard were missing the request would instead reach the handler and return
200 or a 4xx/5xx from the storage layer.
"""
app = _make_app(system_role="user")
with TestClient(app) as client:
for method, path, body in _GUARDED_ENDPOINTS:
resp = getattr(client, method)(path, json=body) if body is not None else getattr(client, method)(path)
assert resp.status_code == 403, f"{method.upper()} {path} expected 403 for non-admin, got {resp.status_code}"
def test_basic_skill_listing_stays_open_to_normal_users(monkeypatch):
"""The basic list/detail endpoints expose only name/description and are
needed by the normal-user UI, so they must NOT be admin-gated.
Under per-user skill isolation, ``list_custom_skills`` (GET /api/skills/custom)
is also open to normal users — they see only their own custom skills.
"""
def _load_skills(*, enabled_only: bool):
from pathlib import Path
from deerflow.skills.types import Skill
return [
Skill(
name="demo",
description="d",
license="MIT",
skill_dir=Path("/tmp/demo"),
skill_file=Path("/tmp/demo/SKILL.md"),
relative_path=Path("demo"),
category="public",
enabled=True,
)
]
app = _make_app(system_role="user")
app.dependency_overrides[get_config] = lambda: SimpleNamespace()
monkeypatch.setattr(skills_router, "_get_user_skill_storage", lambda cfg: SimpleNamespace(load_skills=_load_skills))
with TestClient(app) as client:
assert client.get("/api/skills").status_code == 200
assert client.get("/api/skills/custom").status_code == 200
assert client.get("/api/skills/demo").status_code == 200
def test_enable_toggle_allowed_for_admin(monkeypatch, tmp_path):
"""`PUT /api/skills/{name}` writes the shared extensions_config.json, so it
is admin-only. This confirms the guard does not block a legitimate admin.
"""
from pathlib import Path
from deerflow.skills.types import Skill
config_path = tmp_path / "extensions_config.json"
config_path.write_text(
json.dumps(
{
"mcpServers": {},
"skills": {},
"middlewares": ["pkg:Middleware"],
"mcpInterceptors": ["pkg.interceptor:build"],
}
),
encoding="utf-8",
)
def _load_skills(*, enabled_only: bool):
return [
Skill(
name="demo",
description="d",
license="MIT",
skill_dir=Path("/tmp/demo"),
skill_file=Path("/tmp/demo/SKILL.md"),
relative_path=Path("demo"),
category="public",
enabled=True,
)
]
app = _make_app(system_role="admin")
# Not a real LocalSkillStorage instance, so _write_extensions_skill_state's
# projection-mutation branch is skipped (nullcontext) and it reads the
# config_path fresh via ExtensionsConfig.from_file.
monkeypatch.setattr(skills_router, "_get_user_skill_storage", lambda cfg: SimpleNamespace(load_skills=_load_skills))
monkeypatch.setattr(skills_router, "reload_extensions_config", lambda: None)
monkeypatch.setattr(skills_router.ExtensionsConfig, "resolve_config_path", staticmethod(lambda _config_path=None: config_path))
async def _refresh(_user_id: str):
return None
monkeypatch.setattr(skills_router, "refresh_user_skills_system_prompt_cache_async", _refresh)
with TestClient(app) as client:
resp = client.put("/api/skills/demo", json={"enabled": False})
assert resp.status_code == 200, f"admin toggle should succeed, got {resp.status_code}"
written = json.loads(config_path.read_text(encoding="utf-8"))
assert written["middlewares"] == ["pkg:Middleware"]
assert written["mcpInterceptors"] == ["pkg.interceptor:build"]