diff --git a/mcp_server/notifiers/desktop.py b/mcp_server/notifiers/desktop.py index 423508b3..3773699c 100644 --- a/mcp_server/notifiers/desktop.py +++ b/mcp_server/notifiers/desktop.py @@ -78,26 +78,48 @@ def notify( return True elif sys.platform == "darwin": if shutil.which("osascript"): - script = f'display notification "{clean_body}" with title "{header}"' + # header/clean_body are untrusted (can be derived from on-screen + # app content). Never interpolate them into the script source - + # pass them as `on run argv` arguments instead of splicing them + # into the AppleScript text. (An earlier version of this fix used + # `system attribute` + env vars, but `system attribute` re-decodes + # its value through the wrong text encoding and corrupts non-ASCII + # input, including the default "☕ Artemis Task ..." title - + # `argv` values are passed through as literal UTF-8 unchanged.) + # `--` stops osascript from treating a title/body that happens to + # equal "-e" or "--" as its own flag. + script = ( + "on run argv\n" + " display notification (item 2 of argv) with title (item 1 of argv)\n" + "end run" + ) subprocess.run( - ["osascript", "-e", script], + ["osascript", "-e", script, "--", header, clean_body], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, timeout=3, ) return True elif sys.platform == "win32": + # Same untrusted-content constraint as the macOS branch above: the + # script text is fixed and reads title/body from the environment + # instead of having them interpolated into the source. ps_cmd = ( - f"[Windows.UI.Notifications.ToastNotificationManager, Windows.UI.Notifications, ContentType = WindowsRuntime] > $null; " - f"$template = [Windows.UI.Notifications.ToastNotificationManager]::GetTemplateContent([Windows.UI.Notifications.ToastTemplateType]::ToastText02); " - f'$textNodes = $template.GetElementsByTagName("text"); ' - f'$textNodes.Item(0).AppendChild($template.CreateTextNode("{header}")) > $null; ' - f'$textNodes.Item(1).AppendChild($template.CreateTextNode("{clean_body}")) > $null; ' - f"$toast = [Windows.UI.Notifications.ToastNotification]::new($template); " - f'[Windows.UI.Notifications.ToastNotificationManager]::CreateToastNotifier("Artemis").Show($toast);' + "[Windows.UI.Notifications.ToastNotificationManager, Windows.UI.Notifications, ContentType = WindowsRuntime] > $null; " + "$template = [Windows.UI.Notifications.ToastNotificationManager]::GetTemplateContent([Windows.UI.Notifications.ToastTemplateType]::ToastText02); " + '$textNodes = $template.GetElementsByTagName("text"); ' + "$textNodes.Item(0).AppendChild($template.CreateTextNode($env:ARTEMIS_NOTIFY_TITLE)) > $null; " + "$textNodes.Item(1).AppendChild($template.CreateTextNode($env:ARTEMIS_NOTIFY_BODY)) > $null; " + "$toast = [Windows.UI.Notifications.ToastNotification]::new($template); " + '[Windows.UI.Notifications.ToastNotificationManager]::CreateToastNotifier("Artemis").Show($toast);' ) subprocess.run( ["powershell", "-NoProfile", "-Command", ps_cmd], + env={ + **os.environ, + "ARTEMIS_NOTIFY_TITLE": header, + "ARTEMIS_NOTIFY_BODY": clean_body, + }, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, timeout=5, diff --git a/tests/unit/mcp/test_notifiers.py b/tests/unit/mcp/test_notifiers.py index b429bc59..0c1488d7 100644 --- a/tests/unit/mcp/test_notifiers.py +++ b/tests/unit/mcp/test_notifiers.py @@ -118,6 +118,117 @@ def test_desktop_notifier_enabled_by_default(monkeypatch): assert notifier.is_available() is True +def test_desktop_notifier_darwin_does_not_interpolate_untrusted_text(monkeypatch): + import shutil + import subprocess + import sys + + monkeypatch.delenv("ARTEMIS_DESKTOP_NOTIFY", raising=False) + monkeypatch.delenv("CI", raising=False) + monkeypatch.setattr(sys, "platform", "darwin") + monkeypatch.setattr( + shutil, "which", lambda cmd: "/usr/bin/osascript" if cmd == "osascript" else None + ) + + captured = {} + + def fake_run(cmd, **kwargs): + captured["cmd"] = cmd + captured["env"] = kwargs.get("env") + + class Result: + returncode = 0 + + return Result() + + monkeypatch.setattr(subprocess, "run", fake_run) + + payload = 'pwned" \ndo shell script "touch /tmp/artemis_pwned"\n--' + notifier = DesktopNotifier() + assert notifier.notify("conv-1", payload, title=payload) is True + + cmd = captured["cmd"] + script = cmd[2] + assert payload not in script + assert "do shell script" not in script + # payload must reach osascript only as an `on run argv` argument, after + # the `--` separator, never spliced into the script text itself + assert cmd[3] == "--" + assert cmd[4] == payload + assert cmd[5] == payload + + +def test_desktop_notifier_darwin_passes_argv_after_separator(monkeypatch): + """Title/body reach osascript as `on run argv` arguments after a literal + `--`, so a value that happens to equal an osascript flag (e.g. "-e") can't + be mistaken for one, and non-ASCII text isn't routed through `system + attribute` (which re-decodes through the wrong text encoding and corrupts + it - see the default "☕ Artemis Task ..." title).""" + import shutil + import subprocess + import sys + + monkeypatch.delenv("ARTEMIS_DESKTOP_NOTIFY", raising=False) + monkeypatch.delenv("CI", raising=False) + monkeypatch.setattr(sys, "platform", "darwin") + monkeypatch.setattr( + shutil, "which", lambda cmd: "/usr/bin/osascript" if cmd == "osascript" else None + ) + + captured = {} + + def fake_run(cmd, **kwargs): + captured["cmd"] = cmd + + class Result: + returncode = 0 + + return Result() + + monkeypatch.setattr(subprocess, "run", fake_run) + + notifier = DesktopNotifier() + assert notifier.notify("conv-1", "-e", title="-e") is True + cmd = captured["cmd"] + assert cmd[-3] == "--" + assert cmd[-2:] == ["-e", "-e"] + assert "-e" not in cmd[2] + + +def test_desktop_notifier_windows_does_not_interpolate_untrusted_text(monkeypatch): + import shutil + import subprocess + import sys + + monkeypatch.delenv("ARTEMIS_DESKTOP_NOTIFY", raising=False) + monkeypatch.delenv("CI", raising=False) + monkeypatch.setattr(sys, "platform", "win32") + monkeypatch.setattr(shutil, "which", lambda cmd: "/usr/bin/" + cmd) + + captured = {} + + def fake_run(cmd, **kwargs): + captured["cmd"] = cmd + captured["env"] = kwargs.get("env") + + class Result: + returncode = 0 + + return Result() + + monkeypatch.setattr(subprocess, "run", fake_run) + + payload = '"); Start-Process calc.exe; ("' + notifier = DesktopNotifier() + assert notifier.notify("conv-1", payload, title=payload) is True + + script = captured["cmd"][2] + assert payload not in script + assert "Start-Process" not in script + assert captured["env"]["ARTEMIS_NOTIFY_BODY"] == payload + assert captured["env"]["ARTEMIS_NOTIFY_TITLE"] == payload + + def test_script_notifier(monkeypatch): from mcp_server.notifiers.script import ScriptNotifier