diff --git a/mcp_server/README.md b/mcp_server/README.md index 6855f135..ecffd870 100644 --- a/mcp_server/README.md +++ b/mcp_server/README.md @@ -45,7 +45,7 @@ When you start an asynchronous mobile test via `mobile_run_task`, Artemis MCP ru | **Cursor / Windsurf / VS Code / Cline / Roo Code** | **Native Desktop Toast (`DesktopNotifier`) + File Audit (`FileNotifier`)** | Enabled **by default** across macOS (`osascript`), Linux (`notify-send`), and Windows (`powershell`). Pops up a system notification banner informing the developer whether the task completed or failed. The AI agent inspects `/status.json` or uses `mobile_manage_task(action="status")`. Can be silenced via `ARTEMIS_DESKTOP_NOTIFY=false`. | | **Claude Code / Desktop** | **Native Desktop Toast + File Audit** | Alerts the user via system desktop notification banner when background execution concludes, while maintaining a complete JSONL audit log in `notifications.jsonl`. | | **OpenClaw / Slack / Discord / CI/CD** | **HTTP Webhook (`WebhookNotifier`)** | Sends structured JSON POST payloads to custom endpoints configured via `OPENCLAW_WEBHOOK_URL`, `MCP_NOTIFICATION_WEBHOOK`, or `ARTEMIS_WEBHOOK_URL`. | -| **Universal Custom IDE / CLI Hooks** | **Custom Script Hook (`ScriptNotifier`)** | Set `ARTEMIS_NOTIFY_CMD="my-script --title '{title}' --message '{message}' --trace-id '{trace_id}'"` in your environment to execute any custom command or script upon event completion. | +| **Universal Custom IDE / CLI Hooks** | **Custom Script Hook (`ScriptNotifier`)** | Set `ARTEMIS_NOTIFY_CMD="my-script --title '{title}' --message '{message}' --trace-id '{trace_id}'"` in your environment to execute any custom command or script upon event completion. The template is run **as an argument vector, not through a shell** — task data substituted into the placeholders is never interpreted as shell syntax. If you need pipes, redirection, or `$VAR` expansion, put that logic in the script itself and reference the script as a single command. | ## 🧠 AI Agent Behavioral Rules (`rules.md`) diff --git a/mcp_server/notifiers/script.py b/mcp_server/notifiers/script.py index 6d88fd8f..9553d5a7 100644 --- a/mcp_server/notifiers/script.py +++ b/mcp_server/notifiers/script.py @@ -16,6 +16,7 @@ import logging import os +import shlex import subprocess from typing import Any @@ -31,6 +32,17 @@ class ScriptNotifier(BaseNotifier): or automation platform by allowing users to define ARTEMIS_NOTIFY_CMD or MCP_NOTIFY_COMMAND. Placeholders like {title}, {message}, {conversation_id}, {event_type}, and {trace_id} are automatically replaced before execution. + + The template is executed as an argument vector, not through a shell: it is parsed with + ``shlex.split`` and launched with ``shell=False``. Substituted values are task data (a + task's ``goal`` reaches {title}/{message}), so they are never interpreted as shell + syntax. Two consequences to be aware of: + + - Shell features in the template (pipes, ``&&``, redirection, ``$VAR`` expansion, + globbing) are not interpreted. Put that logic in the script itself and reference the + script as a single command, e.g. ``my-notify.sh --title '{title}'``. + - Quotes around a placeholder are no longer significant: ``--title '{title}'`` and + ``--title {title}`` both pass the value as a single argument. """ ENV_VARS = [ @@ -67,23 +79,29 @@ def notify( trace_id = (payload or {}).get("trace_id", "") formatted_title = title or f"Artemis Task {event_type.capitalize()}" - # Replace template placeholders safely + # The template is operator-supplied, but every substituted value is + # attacker-influenced task data: a task's `goal` reaches {title}/{message} + # (see task_queue_service). Run the template as an argument vector instead of + # through a shell, so task data can never be reinterpreted as shell syntax + # (CWE-78). shlex.split() resolves the operator's own quoting, so a template such + # as `my-script --title '{title}'` still passes the title as a single argument. try: - cmd = ( - cmd_template.replace("{title}", str(formatted_title)) + argv = [ + token.replace("{title}", str(formatted_title)) .replace("{message}", str(message)) .replace("{conversation_id}", str(conversation_id)) .replace("{event_type}", str(event_type)) .replace("{trace_id}", str(trace_id)) - ) + for token in shlex.split(cmd_template) + ] subprocess.run( - cmd, - shell=True, + argv, + shell=False, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, timeout=10, ) - logger.info(f"Custom script notification command executed: {cmd[:60]}...") + logger.info(f"Custom script notification executed: {shlex.join(argv)[:60]}...") return True except Exception as e: logger.warning(f"Failed to execute custom script notification: {e}") diff --git a/tests/unit/mcp/test_notifiers.py b/tests/unit/mcp/test_notifiers.py index b429bc59..fea8b91b 100644 --- a/tests/unit/mcp/test_notifiers.py +++ b/tests/unit/mcp/test_notifiers.py @@ -132,6 +132,46 @@ def test_script_notifier(monkeypatch): assert res is True +def test_script_notifier_passes_substituted_values_as_arguments(monkeypatch): + """Substituted values are task data, not shell syntax. + + The notification text embeds single quotes around a task's goal, so letting a shell + reinterpret the command line would let that data inject commands of its own. + """ + import subprocess as subprocess_module + + from mcp_server.notifiers.script import ScriptNotifier + + captured: list[list[str]] = [] + monkeypatch.setattr(subprocess_module, "run", lambda cmd, **kwargs: captured.append(cmd)) + monkeypatch.setenv("ARTEMIS_NOTIFY_CMD", "true --title '{title}' --message '{message}'") + + goal = "check the battery level ; echo injected ; true" + message = f"Artemis autonomous task '{goal}' finished with status 'failed'." + title = f"Task Failed: {goal[:40]}" + + assert ScriptNotifier().notify("conv-1", message, title=title) is True + # Each substituted value must arrive as exactly one literal argument. + assert captured == [["true", "--title", title, "--message", message]] + + +def test_script_notifier_does_not_execute_injected_task_data(monkeypatch, tmp_path): + """End-to-end check: task data containing shell metacharacters must not execute.""" + from mcp_server.notifiers.script import ScriptNotifier + + marker = tmp_path / "injected" + monkeypatch.setenv( + "ARTEMIS_NOTIFY_CMD", + "true --title '{title}' --message '{message}' --trace-id '{trace_id}'", + ) + goal = f"check the battery level and report back now ; touch {marker} ; true" + message = f"Artemis autonomous task '{goal}' finished with status 'failed'." + title = f"Task Failed: {goal[:40]}" + + assert ScriptNotifier().notify("conv-1", message, title=title) is True + assert not marker.exists() + + def test_composite_notifier_dispatch(): d1 = DummyNotifier(available=True, return_val=True) d2 = DummyNotifier(available=False, return_val=False)