mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-08-01 19:06:01 +00:00
* fix(gateway): require admin for global skills management endpoints The skills router exposed its management endpoints with no authorization check (only Depends(get_config)), while the MCP router guards the equivalent global extensions_config mutations with require_admin_user (added in the #3425 security hardening). Skills storage is global/shared across users (no per-user path), lives in the same extensions_config.json as MCP servers, and custom skill SKILL.md content is injected into every user's agent system prompt. Any authenticated non-admin user could mutate or read global skills that affect all tenants: - POST /api/skills/install: install from an arbitrary thread_id, writing into the global custom skills tree - PUT /api/skills/custom/{skill_name}: rewrite a global skill, injecting instructions into all users' agent prompts (cross-tenant prompt injection) - DELETE /api/skills/custom/{skill_name} and rollback: tamper with global skills - GET /api/skills/custom, GET .../{skill_name}, GET .../history: read raw global custom skill bodies/history - PUT /api/skills/{skill_name} (enable toggle): writes the shared extensions_config.json and refreshes the system prompt for every tenant, so a non-admin could enable/disable any skill globally. There is no per-user skill state, so this is a global mutation, not a preference. Add require_admin_user to every endpoint above, mirroring the MCP router. The shared read path used internally by update/rollback was extracted into a non-auth helper (_read_custom_skill_response) so internal reuse does not double-check auth. Only the read-only GET /api/skills and GET /api/skills/{skill_name} stay open to normal users: they return just name/description/enabled and back the user-facing settings UI. Tests: - New tests/test_skills_router_authz.py: a non-admin user gets 403 on every guarded endpoint (including the enable toggle); basic listing stays open; admins can still toggle. - Update tests/test_skills_custom_router.py to authenticate as admin. - pytest tests/ -k skill -> 350 passed, 1 skipped; ruff clean. Signed-off-by: DengY11 <151997860+DengY11@users.noreply.github.com> * fix(frontend): gate skill enable/install UI behind admin + handle 403 Follow-up to the backend change that made the global skills mutations admin-only. Without a matching UI change, a non-admin user would hit a silent 403 when toggling a skill in Settings -> Skills or clicking "Install skill" on a .skill artifact. - core/skills/api.ts: add SkillRequestError (status + isAdminRequired), throw it from loadSkills/enableSkill on non-ok responses and from installSkill on 403 (other install errors keep the soft-failure contract). - core/skills/hooks.ts: useSkills no longer retries on SkillRequestError. - skill-settings-page.tsx: show an "admin required" message on 403, and disable the enable toggle for non-admins (mirrors the MCP tools page). - artifact-file-detail.tsx / artifact-file-list.tsx: only render the "Install skill" action for admins, and surface an admin-required toast if a 403 still occurs. - i18n: add settings.skills.adminRequired / installAdminRequired (en + zh). Auth/no-auth and static-website modes synthesize an admin user, so these gates do not affect single-user/local deployments. Verified locally: pnpm check (eslint + tsc) passes with no new errors, pnpm build succeeds, and the dev server renders / and /login (200) with no compile/runtime errors. Signed-off-by: DengY11 <151997860+DengY11@users.noreply.github.com> * style(frontend): format skills hook for prettier Keep the admin-guard skills hook aligned with Prettier output so the frontend format check passes in CI. --------- Signed-off-by: DengY11 <151997860+DengY11@users.noreply.github.com>
142 lines
5.8 KiB
Python
142 lines
5.8 KiB
Python
"""Authorization regression tests for the skills router.
|
|
|
|
Skills storage is global/shared across all users, and custom skill SKILL.md
|
|
content is injected into every user's agent system prompt. The mutating skills
|
|
endpoints (and the endpoints that expose raw custom-skill content/history) must
|
|
therefore be admin-only, matching the MCP router which guards the equivalent
|
|
global extensions_config mutations with ``require_admin_user``.
|
|
|
|
These tests pin the access-control boundary: a normal authenticated
|
|
(non-admin) user must receive 403 on every guarded endpoint.
|
|
"""
|
|
|
|
from __future__ import annotations
|
|
|
|
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.
|
|
# Every entry here writes/reads global shared state (the custom skills tree,
|
|
# the shared extensions_config.json, or raw global skill content), so all are
|
|
# admin-only. PUT /api/skills/{name} is included: toggling enabled writes the
|
|
# shared extensions_config.json and changes every tenant's injected skill set.
|
|
_GUARDED_ENDPOINTS = [
|
|
("post", "/api/skills/install", {"thread_id": "t1", "path": "mnt/user-data/outputs/x.skill"}),
|
|
("get", "/api/skills/custom", 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.
|
|
"""
|
|
|
|
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_or_new_skill_storage", lambda **kw: SimpleNamespace(load_skills=_load_skills))
|
|
with TestClient(app) as client:
|
|
assert client.get("/api/skills").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"
|
|
|
|
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")
|
|
monkeypatch.setattr(skills_router, "get_or_new_skill_storage", lambda **kw: SimpleNamespace(load_skills=_load_skills))
|
|
monkeypatch.setattr(skills_router, "get_extensions_config", lambda: SimpleNamespace(mcp_servers={}, skills={}))
|
|
monkeypatch.setattr(skills_router, "reload_extensions_config", lambda: None)
|
|
monkeypatch.setattr(skills_router.ExtensionsConfig, "resolve_config_path", staticmethod(lambda: config_path))
|
|
|
|
async def _refresh():
|
|
return None
|
|
|
|
monkeypatch.setattr(skills_router, "refresh_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}"
|