From 03b278aae6ffad5d26dde54ebd6205bd5d269a84 Mon Sep 17 00:00:00 2001 From: Roshan Sharma Date: Tue, 22 Sep 2026 15:06:17 -0400 Subject: [PATCH 1/2] fix(mcp): stop interpolating notification text into AppleScript/PowerShell DesktopNotifier.notify() built the macOS `osascript` and Windows `powershell` commands by f-string interpolating the notification title and body directly into the AppleScript/PowerShell source. A `"` or newline in that text terminates the string literal early and runs as its own statement, giving arbitrary command execution. The notifier is enabled by default (no opt-in), and the text comes from the task goal and the agent's result/explanation, which can include content read directly off the automated app's screen. Fix: keep the script text constant and pass the title/body through the subprocess environment instead, reading them back with `system attribute` (AppleScript) and `$env:` (PowerShell). Since the script source no longer varies with the input, there is no quoting/escaping question left to get wrong. Fixes #139 --- mcp_server/notifiers/desktop.py | 36 ++++++++++++---- tests/unit/mcp/test_notifiers.py | 70 ++++++++++++++++++++++++++++++++ 2 files changed, 98 insertions(+), 8 deletions(-) diff --git a/mcp_server/notifiers/desktop.py b/mcp_server/notifiers/desktop.py index 423508b3..32e2d757 100644 --- a/mcp_server/notifiers/desktop.py +++ b/mcp_server/notifiers/desktop.py @@ -78,26 +78,46 @@ 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 via the subprocess environment and have the fixed + # script text read them back with `system attribute`. + script = ( + 'display notification (system attribute "ARTEMIS_NOTIFY_BODY") ' + 'with title (system attribute "ARTEMIS_NOTIFY_TITLE")' + ) subprocess.run( ["osascript", "-e", script], + env={ + **os.environ, + "ARTEMIS_NOTIFY_TITLE": header, + "ARTEMIS_NOTIFY_BODY": 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..117ddd14 100644 --- a/tests/unit/mcp/test_notifiers.py +++ b/tests/unit/mcp/test_notifiers.py @@ -118,6 +118,76 @@ 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 + + script = captured["cmd"][2] + assert payload not in script + assert "do shell script" not in script + assert captured["env"]["ARTEMIS_NOTIFY_BODY"] == payload + assert captured["env"]["ARTEMIS_NOTIFY_TITLE"] == payload + + +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 From 99dd74b37a67b7d51466c2a6369dbf62fcb516f4 Mon Sep 17 00:00:00 2001 From: Roshan Sharma Date: Tue, 22 Sep 2026 21:32:45 -0400 Subject: [PATCH 2/2] fix(mcp): pass notifier text as osascript argv, not env + system attribute MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The initial version of this fix passed title/body through the subprocess environment and read them back in AppleScript with `system attribute`. That closes the injection (the script text is a fixed constant either way), but `system attribute` re-decodes its value through the wrong text encoding and corrupts non-ASCII input, including the default title `"☕ Artemis Task {event_type}"`. Switch to passing title/body as `on run argv` arguments after a `--` separator instead. Argv values are passed through as literal UTF-8, so this preserves non-ASCII text exactly while keeping the same injection-proof property: the script source never varies with the input. `--` prevents a title/body that happens to equal an osascript flag (e.g. "-e") from being misparsed. Verified against real osascript: 14 payloads (injection attempts, edge-case flag values, non-ASCII/emoji text) all execute with returncode 0 and empty stderr, and a byte-for-byte round-trip confirms no corruption of legitimate non-ASCII text. Windows branch is unchanged (env + $env:) since Windows environment variables are natively UTF-16 and PowerShell doesn't re-decode them. --- mcp_server/notifiers/desktop.py | 22 ++++++++------- tests/unit/mcp/test_notifiers.py | 47 ++++++++++++++++++++++++++++++-- 2 files changed, 56 insertions(+), 13 deletions(-) diff --git a/mcp_server/notifiers/desktop.py b/mcp_server/notifiers/desktop.py index 32e2d757..3773699c 100644 --- a/mcp_server/notifiers/desktop.py +++ b/mcp_server/notifiers/desktop.py @@ -80,19 +80,21 @@ def notify( if shutil.which("osascript"): # header/clean_body are untrusted (can be derived from on-screen # app content). Never interpolate them into the script source - - # pass them via the subprocess environment and have the fixed - # script text read them back with `system attribute`. + # 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 = ( - 'display notification (system attribute "ARTEMIS_NOTIFY_BODY") ' - 'with title (system attribute "ARTEMIS_NOTIFY_TITLE")' + "on run argv\n" + " display notification (item 2 of argv) with title (item 1 of argv)\n" + "end run" ) subprocess.run( - ["osascript", "-e", script], - env={ - **os.environ, - "ARTEMIS_NOTIFY_TITLE": header, - "ARTEMIS_NOTIFY_BODY": clean_body, - }, + ["osascript", "-e", script, "--", header, clean_body], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, timeout=3, diff --git a/tests/unit/mcp/test_notifiers.py b/tests/unit/mcp/test_notifiers.py index 117ddd14..0c1488d7 100644 --- a/tests/unit/mcp/test_notifiers.py +++ b/tests/unit/mcp/test_notifiers.py @@ -147,11 +147,52 @@ class Result: notifier = DesktopNotifier() assert notifier.notify("conv-1", payload, title=payload) is True - script = captured["cmd"][2] + cmd = captured["cmd"] + script = cmd[2] assert payload not in script assert "do shell script" not in script - assert captured["env"]["ARTEMIS_NOTIFY_BODY"] == payload - assert captured["env"]["ARTEMIS_NOTIFY_TITLE"] == payload + # 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):