fix(webui): harden skill marketplace lifecycle
This commit is contained in:
@@ -765,6 +765,151 @@ async def test_webui_skills_marketplace_routes_search_and_install(
|
||||
await server_task
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_webui_skill_install_rejects_overlapping_requests(
|
||||
bus: MagicMock,
|
||||
tmp_path: Path,
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
started = asyncio.Event()
|
||||
finish = asyncio.Event()
|
||||
|
||||
async def install(
|
||||
source: str,
|
||||
skill_id: str,
|
||||
workspace: Path,
|
||||
*,
|
||||
provider: str,
|
||||
version: str,
|
||||
) -> dict[str, Any]:
|
||||
started.set()
|
||||
await finish.wait()
|
||||
skill_dir = workspace / "skills" / skill_id
|
||||
skill_dir.mkdir(parents=True)
|
||||
(skill_dir / "SKILL.md").write_text(
|
||||
"---\nname: react-testing\ndescription: Test React apps.\n---\n",
|
||||
encoding="utf-8",
|
||||
)
|
||||
return {"installed": True, "already_installed": False, "name": skill_id}
|
||||
|
||||
install_mock = AsyncMock(side_effect=install)
|
||||
monkeypatch.setattr("nanobot.webui.ws_http.install_marketplace_skill", install_mock)
|
||||
channel = _ch(
|
||||
bus,
|
||||
session_manager=_seed_session(tmp_path),
|
||||
workspace_path=tmp_path,
|
||||
port=_free_port(),
|
||||
)
|
||||
token = channel.gateway.tokens.issue_api_token(300)
|
||||
path = (
|
||||
"/api/webui/skills/install"
|
||||
"?source=acme%2Fagent-skills&skill=react-testing"
|
||||
)
|
||||
request = _FakeReq(
|
||||
{
|
||||
"Authorization": f"Bearer {token}",
|
||||
"Host": "127.0.0.1:8765",
|
||||
},
|
||||
path=path,
|
||||
)
|
||||
|
||||
first = asyncio.create_task(channel.gateway.http.dispatch(_LOCAL, request))
|
||||
await started.wait()
|
||||
overlapping = await channel.gateway.http.dispatch(_LOCAL, request)
|
||||
|
||||
assert overlapping.status_code == 409
|
||||
assert "already in progress" in overlapping.body.decode()
|
||||
assert install_mock.await_count == 1
|
||||
|
||||
finish.set()
|
||||
completed = await first
|
||||
assert completed.status_code == 200
|
||||
assert install_mock.await_count == 1
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_webui_skill_delete_remains_local_only(
|
||||
bus: MagicMock,
|
||||
tmp_path: Path,
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
delete = MagicMock()
|
||||
policy = MagicMock()
|
||||
policy.tools.webui_allow_remote_package_install = True
|
||||
monkeypatch.setattr("nanobot.config.loader.load_config", lambda: policy)
|
||||
monkeypatch.setattr("nanobot.webui.ws_http.delete_webui_skill", delete)
|
||||
channel = _ch(
|
||||
bus,
|
||||
session_manager=_seed_session(tmp_path),
|
||||
workspace_path=tmp_path,
|
||||
port=_free_port(),
|
||||
)
|
||||
token = channel.gateway.tokens.issue_api_token(300)
|
||||
response = await channel.gateway.http.dispatch(
|
||||
_REMOTE,
|
||||
_FakeReq(
|
||||
{"Authorization": f"Bearer {token}"},
|
||||
path="/api/webui/skills/delete?name=custom-skill",
|
||||
),
|
||||
)
|
||||
|
||||
assert response.status_code == 403
|
||||
assert "remote skill deletion is disabled" in response.body.decode()
|
||||
delete.assert_not_called()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_webui_skill_install_honors_remote_install_opt_in(
|
||||
bus: MagicMock,
|
||||
tmp_path: Path,
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
policy = MagicMock()
|
||||
policy.tools.webui_allow_remote_package_install = True
|
||||
monkeypatch.setattr("nanobot.config.loader.load_config", lambda: policy)
|
||||
|
||||
async def install(
|
||||
source: str,
|
||||
skill_id: str,
|
||||
workspace: Path,
|
||||
*,
|
||||
provider: str,
|
||||
version: str,
|
||||
) -> dict[str, Any]:
|
||||
skill_dir = workspace / "skills" / skill_id
|
||||
skill_dir.mkdir(parents=True)
|
||||
(skill_dir / "SKILL.md").write_text(
|
||||
"---\nname: react-testing\ndescription: Test React apps.\n---\n",
|
||||
encoding="utf-8",
|
||||
)
|
||||
return {"installed": True, "already_installed": False, "name": skill_id}
|
||||
|
||||
monkeypatch.setattr(
|
||||
"nanobot.webui.ws_http.install_marketplace_skill",
|
||||
AsyncMock(side_effect=install),
|
||||
)
|
||||
channel = _ch(
|
||||
bus,
|
||||
session_manager=_seed_session(tmp_path),
|
||||
workspace_path=tmp_path,
|
||||
port=_free_port(),
|
||||
)
|
||||
token = channel.gateway.tokens.issue_api_token(300)
|
||||
response = await channel.gateway.http.dispatch(
|
||||
_REMOTE,
|
||||
_FakeReq(
|
||||
{"Authorization": f"Bearer {token}"},
|
||||
path=(
|
||||
"/api/webui/skills/install"
|
||||
"?source=acme%2Fagent-skills&skill=react-testing"
|
||||
),
|
||||
),
|
||||
)
|
||||
|
||||
assert response.status_code == 200
|
||||
assert json.loads(response.body.decode())["last_action"]["name"] == "react-testing"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_cli_apps_routes_require_token_and_return_payload(
|
||||
bus: MagicMock,
|
||||
|
||||
@@ -403,7 +403,7 @@ class ToolsConfig(Base):
|
||||
"webuiAllowRemotePackageInstall",
|
||||
"webui_allow_remote_package_install",
|
||||
),
|
||||
) # allow non-local WebUI clients to install optional Python packages
|
||||
) # allow non-local WebUI clients to install optional packages and agent skills
|
||||
mcp_servers: dict[str, MCPServerConfig] = Field(default_factory=dict)
|
||||
ssrf_whitelist: list[str] = Field(default_factory=list) # CIDR ranges to exempt from SSRF blocking (e.g. ["100.64.0.0/10"] for Tailscale)
|
||||
|
||||
|
||||
@@ -4,7 +4,7 @@ from __future__ import annotations
|
||||
|
||||
import json
|
||||
import shlex
|
||||
import shutil
|
||||
import tempfile
|
||||
from pathlib import Path
|
||||
from typing import Any
|
||||
|
||||
@@ -113,19 +113,25 @@ def delete_webui_skill(
|
||||
target = skills_root / name
|
||||
if target.parent != skills_root:
|
||||
raise SkillManagementError("invalid skill name")
|
||||
if target.is_symlink():
|
||||
target.unlink()
|
||||
elif target.is_dir():
|
||||
shutil.rmtree(target)
|
||||
else:
|
||||
if not target.is_symlink() and not target.is_dir():
|
||||
raise SkillManagementError("skill directory was not found", status=404)
|
||||
|
||||
config = load_config()
|
||||
next_disabled = set(config.agents.defaults.disabled_skills)
|
||||
original_disabled = list(config.agents.defaults.disabled_skills)
|
||||
next_disabled = set(original_disabled)
|
||||
if name in next_disabled:
|
||||
next_disabled.remove(name)
|
||||
config.agents.defaults.disabled_skills = sorted(next_disabled)
|
||||
save_config(config)
|
||||
with tempfile.TemporaryDirectory(prefix=".nanobot-delete-", dir=skills_root) as staging:
|
||||
staged_target = Path(staging) / name
|
||||
target.replace(staged_target)
|
||||
try:
|
||||
if next_disabled != set(original_disabled):
|
||||
config.agents.defaults.disabled_skills = sorted(next_disabled)
|
||||
save_config(config)
|
||||
except Exception:
|
||||
config.agents.defaults.disabled_skills = original_disabled
|
||||
staged_target.replace(target)
|
||||
raise
|
||||
disabled_skills.clear()
|
||||
disabled_skills.update(next_disabled)
|
||||
return {"name": name, "enabled": False, "deleted": True}
|
||||
|
||||
@@ -530,7 +530,6 @@ async def _install_skillhub_skill(
|
||||
"name": skill_id,
|
||||
"provider": _PROVIDER_SKILLHUB,
|
||||
"version": version,
|
||||
"verified": bool(signature.get("signed")),
|
||||
}
|
||||
|
||||
|
||||
|
||||
+18
-14
@@ -199,6 +199,7 @@ class GatewayHTTPHandler:
|
||||
self.skills_workspace_path = skills_workspace_path
|
||||
self.disabled_skills = disabled_skills if disabled_skills is not None else set()
|
||||
self.skill_state_action = skill_state_action
|
||||
self._skill_install_lock = asyncio.Lock()
|
||||
self.cron_service = cron_service
|
||||
self.local_trigger_store = local_trigger_store
|
||||
self.cron_pending_job_ids = cron_pending_job_ids
|
||||
@@ -912,25 +913,28 @@ class GatewayHTTPHandler:
|
||||
return _http_error(401, "Unauthorized")
|
||||
if not self._allow_webui_package_install(connection, request):
|
||||
return _http_error(403, "remote skill installation is disabled")
|
||||
if self._skill_install_lock.locked():
|
||||
return _http_error(409, "another skill installation is already in progress")
|
||||
|
||||
query = _parse_query(request.path)
|
||||
provider = _query_first(query, "provider") or "skills_sh"
|
||||
source = _query_first(query, "source") or ""
|
||||
skill_id = _query_first(query, "skill") or ""
|
||||
version = _query_first(query, "version") or ""
|
||||
try:
|
||||
action = await install_marketplace_skill(
|
||||
source,
|
||||
skill_id,
|
||||
self.skills_workspace_path,
|
||||
provider=provider,
|
||||
version=version,
|
||||
)
|
||||
except SkillsMarketplaceError as exc:
|
||||
return _http_error(exc.status, exc.message)
|
||||
except Exception:
|
||||
self._log.exception("skill installation failed")
|
||||
return _http_error(500, "skill installation failed")
|
||||
async with self._skill_install_lock:
|
||||
try:
|
||||
action = await install_marketplace_skill(
|
||||
source,
|
||||
skill_id,
|
||||
self.skills_workspace_path,
|
||||
provider=provider,
|
||||
version=version,
|
||||
)
|
||||
except SkillsMarketplaceError as exc:
|
||||
return _http_error(exc.status, exc.message)
|
||||
except Exception:
|
||||
self._log.exception("skill installation failed")
|
||||
return _http_error(500, "skill installation failed")
|
||||
return _http_json_response({
|
||||
**webui_skills_payload(
|
||||
self.skills_workspace_path,
|
||||
@@ -983,7 +987,7 @@ class GatewayHTTPHandler:
|
||||
) -> Response:
|
||||
if not self.check_api_token(request):
|
||||
return _http_error(401, "Unauthorized")
|
||||
if not self._allow_webui_package_install(connection, request):
|
||||
if not _is_local_browser_request(connection, request.headers):
|
||||
return _http_error(403, "remote skill deletion is disabled")
|
||||
name = _query_first(_parse_query(request.path), "name") or ""
|
||||
try:
|
||||
|
||||
Reference in New Issue
Block a user