-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
fix: disable workspace skills in group sessions #9082
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -873,6 +873,63 @@ async def test_ensure_skills_includes_workspace_skills( | |
| in req.system_prompt | ||
| ) | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_ensure_skills_skips_workspace_skills_for_group_sessions( | ||
| self, | ||
| monkeypatch, | ||
| tmp_path, | ||
| mock_event, | ||
| mock_context, | ||
| ): | ||
| module = ama | ||
| data_dir = tmp_path / "data" | ||
| global_skills_dir = tmp_path / "global_skills" | ||
| plugins_dir = tmp_path / "plugins" | ||
| workspaces_dir = tmp_path / "workspaces" | ||
| for path in (data_dir, global_skills_dir, plugins_dir): | ||
| path.mkdir(parents=True, exist_ok=True) | ||
|
|
||
| mock_event.get_group_id.return_value = "group123" | ||
| mock_event.message_obj.group_id = "group123" | ||
| mock_event.unified_msg_origin = "test_platform:GroupMessage:group123" | ||
| workspace_root = workspaces_dir / module.normalize_umo_for_workspace( | ||
| mock_event.unified_msg_origin | ||
| ) | ||
| workspace_skill_dir = workspace_root / "skills" / "workspace-skill" | ||
| workspace_skill_dir.mkdir(parents=True) | ||
| workspace_skill_dir.joinpath("SKILL.md").write_text( | ||
| "---\ndescription: Workspace scoped skill.\n---\n", | ||
| encoding="utf-8", | ||
| ) | ||
|
|
||
| monkeypatch.setattr( | ||
| module, | ||
| "get_astrbot_workspaces_path", | ||
| lambda: str(workspaces_dir), | ||
| ) | ||
| monkeypatch.setattr( | ||
| "astrbot.core.skills.skill_manager.get_astrbot_data_path", | ||
| lambda: str(data_dir), | ||
| ) | ||
| monkeypatch.setattr( | ||
| "astrbot.core.skills.skill_manager.get_astrbot_skills_path", | ||
| lambda: str(global_skills_dir), | ||
| ) | ||
| monkeypatch.setattr( | ||
| "astrbot.core.skills.skill_manager.get_astrbot_plugin_path", | ||
| lambda: str(plugins_dir), | ||
| ) | ||
|
|
||
| req = ProviderRequest() | ||
| req.conversation = MagicMock(persona_id=None) | ||
|
|
||
| await module._ensure_persona_and_skills( | ||
| req, {"computer_use_runtime": "local"}, mock_context, mock_event | ||
| ) | ||
|
|
||
| assert "Workspace scoped skill." not in req.system_prompt | ||
| assert "## Skills" not in req.system_prompt | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The current test asserts that To make this regression test more robust, we should also set up a global skill (similar to
global_skill_dir = global_skills_dir / "workspace-skill"
global_skill_dir.mkdir(parents=True)
global_skill_dir.joinpath("SKILL.md").write_text(
"---\ndescription: Global scoped skill.\n---\n",
encoding="utf-8",
)
workspace_skill_dir = workspace_root / "skills" / "workspace-skill"
workspace_skill_dir.mkdir(parents=True)
workspace_skill_dir.joinpath("SKILL.md").write_text(
"---\ndescription: Workspace scoped skill.\n---\n",
encoding="utf-8",
)
monkeypatch.setattr(
module,
"get_astrbot_workspaces_path",
lambda: str(workspaces_dir),
)
monkeypatch.setattr(
"astrbot.core.skills.skill_manager.get_astrbot_data_path",
lambda: str(data_dir),
)
monkeypatch.setattr(
"astrbot.core.skills.skill_manager.get_astrbot_skills_path",
lambda: str(global_skills_dir),
)
monkeypatch.setattr(
"astrbot.core.skills.skill_manager.get_astrbot_plugin_path",
lambda: str(plugins_dir),
)
req = ProviderRequest()
req.conversation = MagicMock(persona_id=None)
await module._ensure_persona_and_skills(
req, {"computer_use_runtime": "local"}, mock_context, mock_event
)
assert "Global scoped skill." in req.system_prompt
assert "Workspace scoped skill." not in req.system_prompt |
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_ensure_skills_respects_empty_persona_skills_for_workspace( | ||
| self, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While disabling workspace-scoped skills in group sessions is an excellent security improvement to prevent privilege-boundary vulnerabilities, a similar vulnerability exists with
EXTRA_PROMPT.md.In
_apply_workspace_extra_prompt(called during_decorate_llm_request), the fileEXTRA_PROMPT.mdis loaded from the workspace path and appended to the system prompt. Since group sessions share the same workspace directory (derived fromevent.unified_msg_origin), any non-admin user in the group could plant or modifyEXTRA_PROMPT.mdto perform a prompt injection attack against other users (including admins) in the same group.To fully mitigate this privilege-boundary vulnerability, please consider also disabling
EXTRA_PROMPT.mdloading for group sessions. For example, you can update_apply_workspace_extra_promptas follows: