diff --git a/docs/examples/50-mcdbus.rules b/docs/examples/50-mcdbus.rules new file mode 100644 index 0000000..bf4de48 --- /dev/null +++ b/docs/examples/50-mcdbus.rules @@ -0,0 +1,88 @@ +// polkit rules for mcdbus +// +// Drop into /etc/polkit-1/rules.d/50-mcdbus.rules and restart polkit: +// sudo systemctl restart polkit.service +// +// Users in the "mcdbus" group get read-only access to common system +// services. Everything else falls through to system defaults (which +// typically require admin authentication or deny). +// +// Create the group: +// sudo groupadd mcdbus +// sudo usermod -aG mcdbus youruser +// +// Test with: +// pkcheck --action-id org.freedesktop.systemd1.manage-units \ +// --process $$ --allow-user-interaction +// +// List all actions: +// pkaction | grep -E 'systemd|NetworkManager|UPower|udisks' + +polkit.addRule(function(action, subject) { + + // Only apply to users in the mcdbus group + if (!subject.isInGroup("mcdbus")) { + return polkit.Result.NOT_HANDLED; + } + + // Read-only systemd operations + // ListUnits, GetUnit, GetUnitProperties -- these do not change state. + var systemdReadActions = [ + "org.freedesktop.systemd1.manage-units", + "org.freedesktop.systemd1.reload-daemon" + ]; + + // NetworkManager: allow reading connection and device info + var nmReadActions = [ + "org.freedesktop.NetworkManager.network-control", + "org.freedesktop.NetworkManager.settings.modify.system" + ]; + + // UPower: allow reading battery status + var upowerActions = [ + "org.freedesktop.upower.get-history", + "org.freedesktop.upower.get-statistics" + ]; + + // UDisks2: allow reading disk info but not mounting/formatting + var udisksReadActions = [ + "org.freedesktop.udisks2.ata-smart-selftest" + ]; + + // Allow read-only systemd access + // NOTE: systemd1.manage-units covers both read and write operations. + // If you only want list/status (no start/stop/restart), you need + // Layer 2 (D-Bus bus policy) to restrict which methods can be called. + // polkit alone cannot distinguish between "list units" and "stop unit" + // under the same action ID. + if (systemdReadActions.indexOf(action.id) >= 0) { + // Only allow for local, active sessions + if (subject.local && subject.active) { + return polkit.Result.YES; + } + } + + // Allow NetworkManager reads for local sessions + if (nmReadActions.indexOf(action.id) >= 0) { + if (subject.local && subject.active) { + return polkit.Result.YES; + } + } + + // Allow UPower reads + if (upowerActions.indexOf(action.id) >= 0) { + return polkit.Result.YES; + } + + // Allow UDisks2 reads + if (udisksReadActions.indexOf(action.id) >= 0) { + if (subject.local && subject.active) { + return polkit.Result.YES; + } + } + + // Everything else: fall through to system defaults. + // Do NOT return polkit.Result.NO here -- that would override rules + // with lower priority numbers. NOT_HANDLED lets the chain continue. + return polkit.Result.NOT_HANDLED; +}); diff --git a/docs/examples/mcdbus-proxy.sh b/docs/examples/mcdbus-proxy.sh new file mode 100755 index 0000000..71adbfa --- /dev/null +++ b/docs/examples/mcdbus-proxy.sh @@ -0,0 +1,91 @@ +#!/usr/bin/env bash +# +# mcdbus-proxy.sh -- run mcdbus through a filtered D-Bus proxy +# +# Uses xdg-dbus-proxy to create a restricted session bus socket. +# mcdbus connects to the proxy socket and can only see/talk to +# the services listed below. Everything else is invisible. +# +# Install xdg-dbus-proxy: +# Arch: pacman -S xdg-dbus-proxy +# Debian/Ubuntu: apt install xdg-dbus-proxy +# Fedora: dnf install xdg-dbus-proxy +# +# Usage: +# ./mcdbus-proxy.sh +# +# Or with a custom mcdbus command: +# MCDBUS_CMD="uv run mcdbus" ./mcdbus-proxy.sh +# +# To integrate with systemd: +# ExecStart=/usr/local/bin/mcdbus-proxy.sh + +set -euo pipefail + +MCDBUS_CMD="${MCDBUS_CMD:-mcdbus}" + +# Bail out if xdg-dbus-proxy is not installed +if ! command -v xdg-dbus-proxy &>/dev/null; then + echo "error: xdg-dbus-proxy not found. Install it first." >&2 + exit 1 +fi + +# Need a session bus to proxy +if [ -z "${DBUS_SESSION_BUS_ADDRESS:-}" ]; then + echo "error: DBUS_SESSION_BUS_ADDRESS is not set." >&2 + exit 1 +fi + +# Create a temporary directory for the proxy socket +PROXY_DIR="$(mktemp -d /tmp/mcdbus-proxy.XXXXXX)" +PROXY_SOCKET="${PROXY_DIR}/bus" + +cleanup() { + if [ -n "${PROXY_PID:-}" ]; then + kill "$PROXY_PID" 2>/dev/null || true + wait "$PROXY_PID" 2>/dev/null || true + fi + rm -rf "$PROXY_DIR" +} +trap cleanup EXIT INT TERM + +# Start the proxy. +# +# --talk= Full bidirectional access (method calls, property writes, signals). +# --see= Read-only visibility (appears in ListNames, introspectable, but +# method calls are blocked). +# +# The bus daemon itself is always accessible through the proxy. +# +# Adjust these to match your deployment needs. The set below covers +# the services that mcdbus's built-in shortcut tools use. + +xdg-dbus-proxy "$DBUS_SESSION_BUS_ADDRESS" "$PROXY_SOCKET" \ + --filter \ + --talk=org.freedesktop.Notifications \ + --talk=org.mpris.MediaPlayer2 \ + --see=org.freedesktop.UPower \ + --see=org.freedesktop.NetworkManager \ + --see=org.bluez \ + --see=org.freedesktop.systemd1 \ + --talk=org.kde.KWin \ + & + +PROXY_PID=$! + +# Wait for the proxy socket to appear (up to 5 seconds) +for i in $(seq 1 50); do + if [ -e "$PROXY_SOCKET" ]; then + break + fi + sleep 0.1 +done + +if [ ! -e "$PROXY_SOCKET" ]; then + echo "error: proxy socket did not appear at $PROXY_SOCKET" >&2 + exit 1 +fi + +# Point mcdbus at the proxy socket and exec +export DBUS_SESSION_BUS_ADDRESS="unix:path=${PROXY_SOCKET}" +exec $MCDBUS_CMD diff --git a/docs/examples/mcdbus.conf b/docs/examples/mcdbus.conf new file mode 100644 index 0000000..06a8f0d --- /dev/null +++ b/docs/examples/mcdbus.conf @@ -0,0 +1,129 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/docs/examples/mcdbus.service b/docs/examples/mcdbus.service new file mode 100644 index 0000000..6be9a34 --- /dev/null +++ b/docs/examples/mcdbus.service @@ -0,0 +1,76 @@ +[Unit] +Description=mcdbus D-Bus MCP server +Documentation=https://github.com/supported-systems/mcdbus +After=dbus.service + +[Service] +Type=simple +ExecStart=/usr/bin/env mcdbus + +# --- Identity --- +# Ephemeral user, no persistent UID, no home directory. +# If you need supplementary groups (e.g. for polkit rules), switch +# to User=mcdbus with a real system account instead. +DynamicUser=yes + +# --- Filesystem --- +ProtectSystem=strict +ProtectHome=yes +PrivateTmp=yes +PrivateDevices=yes +ProtectKernelTunables=yes +ProtectKernelModules=yes +ProtectKernelLogs=yes +ProtectControlGroups=yes +ProtectHostname=yes +ProtectClock=yes +ProtectProc=invisible +ProcSubset=pid +ReadWritePaths= + +# --- Network --- +# D-Bus uses AF_UNIX only. Block everything else. +RestrictAddressFamilies=AF_UNIX +PrivateNetwork=no + +# --- Capabilities --- +# Empty = drop all capabilities. +CapabilityBoundingSet= +AmbientCapabilities= +NoNewPrivileges=yes + +# --- Syscalls --- +SystemCallFilter=@system-service +SystemCallArchitectures=native +SystemCallErrorNumber=EPERM + +# --- Memory --- +MemoryDenyWriteExecute=yes +LockPersonality=yes +RestrictRealtime=yes +RestrictSUIDSGID=yes +RemoveIPC=yes + +# --- Namespaces --- +RestrictNamespaces=yes + +# --- Environment --- +Environment=MCDBUS_TIMEOUT=30 +# Uncomment to require user confirmation for system bus calls: +# Environment=MCDBUS_REQUIRE_ELICITATION=1 + +# --- Logging --- +StandardOutput=journal +StandardError=journal +SyslogIdentifier=mcdbus + +# --- Resource limits --- +MemoryMax=256M +TasksMax=64 + +# --- Restart --- +Restart=on-failure +RestartSec=5 + +[Install] +WantedBy=default.target diff --git a/docs/permissions.md b/docs/permissions.md new file mode 100644 index 0000000..c9fb577 --- /dev/null +++ b/docs/permissions.md @@ -0,0 +1,332 @@ +# D-Bus Permission Model + +mcdbus does not implement its own access control. By design, it delegates +all security decisions to the operating system's existing D-Bus permission +stack. This means the same mechanisms that protect D-Bus from any other +client also protect it from mcdbus. + +This guide covers four independent layers that can be composed to build +the level of isolation your deployment requires. They are all optional, +all independent, and all stack on top of each other. + +## Layer 1: systemd Service Sandboxing + +The single most effective thing you can do. Running mcdbus as a systemd +service gives you process-level isolation using Linux namespaces, seccomp, +and capability dropping -- all with a handful of declarative directives. + +An example unit file is provided at [docs/examples/mcdbus.service](examples/mcdbus.service). + +### What it does + +The unit file uses `DynamicUser=yes`, which creates an ephemeral Unix user +for each service invocation. No home directory, no persistent UID, no +leftover state. Combined with filesystem and network restrictions, the +mcdbus process gets a minimal sandbox: + +- **Filesystem:** `ProtectSystem=strict` makes `/usr`, `/boot`, and `/efi` + read-only. `ProtectHome=yes` hides `/home`, `/root`, and `/run/user` + entirely. `PrivateTmp=yes` gives mcdbus its own `/tmp` that is invisible + to other processes. +- **Network:** `RestrictAddressFamilies=AF_UNIX` limits socket creation to + Unix domain sockets. D-Bus only needs `AF_UNIX`, so this blocks any + TCP/UDP/raw socket activity. mcdbus cannot open network connections. +- **Capabilities:** `CapabilityBoundingSet=` (empty) drops every Linux + capability. mcdbus cannot change file ownership, load kernel modules, + bind to privileged ports, or do anything else that requires elevated + privileges. +- **Syscalls:** `SystemCallFilter=@system-service` restricts the process to + the set of syscalls that a typical well-behaved service needs. Things + like `mount`, `reboot`, and `kexec_load` are blocked. +- **Memory:** `MemoryDenyWriteExecute=yes` prevents mapping memory as both + writable and executable. Stops most classes of code injection. + +### Deploying it + +```bash +sudo cp docs/examples/mcdbus.service /etc/systemd/system/ +sudo systemctl daemon-reload +sudo systemctl enable --now mcdbus.service +``` + +### Testing it + +systemd ships a built-in security auditor: + +```bash +systemd-analyze security mcdbus.service +``` + +This produces a score from 0.0 (fully exposed) to 10.0 (fully locked down). +The example unit file should score above 7.0. The output also lists each +directive and its impact, so you can see exactly what is and is not restricted. + +Check the journal for any sandbox-related denials: + +```bash +journalctl -u mcdbus.service -f +``` + +### Environment variables + +The unit file sets `MCDBUS_TIMEOUT=30` by default. You can override this +and add other mcdbus environment variables via a drop-in: + +```bash +sudo systemctl edit mcdbus.service +``` + +```ini +[Service] +Environment=MCDBUS_TIMEOUT=60 +Environment=MCDBUS_REQUIRE_ELICITATION=1 +``` + + +## Layer 2: D-Bus Bus Policy + +D-Bus itself has a message filtering layer that operates at the bus daemon +(or bus broker) level. This is independent of what mcdbus does -- it +controls which messages any client can send or receive based on the +sender's Unix identity. + +An example policy file is provided at [docs/examples/mcdbus.conf](examples/mcdbus.conf). + +### How it works + +D-Bus policy files are XML documents dropped into `/etc/dbus-1/system.d/` +(for the system bus) or `/etc/dbus-1/session.d/` (for the session bus). +Both `dbus-daemon` and `dbus-broker` read them. + +The policy engine evaluates rules top to bottom. The last matching rule +wins. The general pattern is: + +1. Default deny everything for the mcdbus user/group. +2. Selectively allow specific destinations, interfaces, and methods. + +### Filter attributes + +Each `` or `` element can filter on: + +- `send_destination` -- the bus name being called (e.g. `org.freedesktop.Notifications`) +- `send_interface` -- the interface being invoked +- `send_member` -- the specific method name +- `send_type` -- message type (`method_call`, `signal`, etc.) + +You can combine these. An `` with both `send_destination` and +`send_interface` only permits calls to that specific interface on that +specific service. + +### Deploying it + +For the system bus: + +```bash +sudo cp docs/examples/mcdbus.conf /etc/dbus-1/system.d/ +sudo systemctl reload dbus.service # dbus-daemon +# or: sudo systemctl reload dbus-broker.service +``` + +For the session bus, copy to `/etc/dbus-1/session.d/` instead. Session bus +policies apply to all users. If you only want to restrict a specific user, +use `user="mcdbus"` in the policy context. + +### Testing it + +Denied messages show up in the bus daemon's journal: + +```bash +# dbus-broker +journalctl -u dbus-broker.service --since "5 min ago" | grep -i deny + +# dbus-daemon +journalctl -u dbus.service --since "5 min ago" | grep -i deny +``` + +You can also use `dbus-monitor` to watch traffic in real time: + +```bash +dbus-monitor --system "destination='org.freedesktop.UPower'" +``` + + +## Layer 3: PolicyKit (polkit) + +polkit provides per-action authorization. Where D-Bus bus policy controls +which messages can be *sent*, polkit controls whether a specific *action* +is *authorized* for a specific user. Many system services (systemd, +NetworkManager, UPower, udisks2) check polkit before performing privileged +operations, regardless of what the D-Bus bus policy says. + +An example rules file is provided at [docs/examples/50-mcdbus.rules](examples/50-mcdbus.rules). + +### How it works + +polkit actions are identified by reverse-domain strings like +`org.freedesktop.systemd1.manage-units`. When a D-Bus service receives +a method call that requires authorization, it asks polkit whether the +calling user is allowed to perform that action. + +polkit rules are JavaScript files in `/etc/polkit-1/rules.d/`. They are +evaluated in filename order (hence the `50-` prefix). Each rule function +receives an `action` and a `subject` and returns one of: + +- `polkit.Result.YES` -- allow without prompting +- `polkit.Result.AUTH_ADMIN` -- require admin password +- `polkit.Result.NO` -- deny + +### What the example does + +The provided rules file checks if the calling user is in the `mcdbus` +Unix group. If so, it allows a curated set of read-only actions. Everything +else falls through to the system defaults (which typically require admin +authentication or deny outright). + +### Creating the group + +```bash +sudo groupadd mcdbus +sudo usermod -aG mcdbus $(whoami) +# Log out and back in for the group membership to take effect +``` + +If you are running mcdbus under systemd with `DynamicUser=yes`, the +dynamic user will not be in any supplementary groups by default. You can +either: + +- Use `SupplementaryGroups=mcdbus` in the unit file, or +- Use `User=mcdbus` with a real system user instead of `DynamicUser=yes` + +### Deploying it + +```bash +sudo cp docs/examples/50-mcdbus.rules /etc/polkit-1/rules.d/ +sudo systemctl restart polkit.service +``` + +### Testing it + +List all registered polkit actions: + +```bash +pkaction +``` + +Check whether a specific action would be allowed for a user: + +```bash +pkcheck --action-id org.freedesktop.systemd1.manage-units \ + --process $$ --allow-user-interaction +``` + +The `--allow-user-interaction` flag lets polkit prompt for a password if +the rules say `AUTH_ADMIN`. Without it, `AUTH_ADMIN` results are treated +as denial. + + +## Layer 4: xdg-dbus-proxy + +`xdg-dbus-proxy` is a Flatpak utility that creates a filtered D-Bus +socket. You point it at the real bus socket, tell it which services to +expose, and it creates a new socket that only passes through matching +messages. The client (mcdbus) connects to the proxy socket and never +sees the rest of the bus. + +An example wrapper script is provided at [docs/examples/mcdbus-proxy.sh](examples/mcdbus-proxy.sh). + +### Policy levels + +xdg-dbus-proxy supports three levels of access per service: + +- `--see=NAME` -- the service appears in `ListNames` and can be introspected, + but method calls are blocked. Read-only visibility. +- `--talk=NAME` -- full bidirectional communication with the service. + Method calls, property reads/writes, and signals all pass through. +- `--own=NAME` -- the client can register (own) the given bus name. + mcdbus does not need this. + +You can also filter at the interface and path level: + +```bash +--talk=org.freedesktop.Notifications +--call=org.freedesktop.UPower=org.freedesktop.DBus.Properties.GetAll@/org/freedesktop/UPower/* +``` + +### Installing it + +xdg-dbus-proxy ships with Flatpak: + +```bash +# Arch +sudo pacman -S xdg-dbus-proxy + +# Debian/Ubuntu +sudo apt install xdg-dbus-proxy + +# Fedora +sudo dnf install xdg-dbus-proxy +``` + +### How the wrapper works + +The script: + +1. Creates a temporary directory for the proxy socket. +2. Starts `xdg-dbus-proxy` in the background, pointing at `$DBUS_SESSION_BUS_ADDRESS`. +3. Waits for the proxy socket to appear. +4. Sets `DBUS_SESSION_BUS_ADDRESS` to the proxy socket. +5. `exec`s mcdbus, so mcdbus runs with PID 1 semantics and gets signals correctly. +6. Cleans up the proxy socket and process on exit via a trap. + +### Deploying it + +```bash +chmod +x docs/examples/mcdbus-proxy.sh +./docs/examples/mcdbus-proxy.sh +``` + +Or integrate it into the systemd unit file: + +```ini +[Service] +ExecStart=/usr/local/bin/mcdbus-proxy.sh +``` + + +## Choosing Layers + +All four layers are independent. You can use any combination. + +**Layer 1 (systemd sandboxing)** is always recommended. It costs nothing +to deploy and provides broad process-level isolation. Even if mcdbus or +dbus-fast had a vulnerability, the sandbox limits what an attacker could +do with it. + +**Layer 2 (D-Bus bus policy)** is useful when you want to restrict which +services mcdbus can talk to at the bus level. This is the coarsest filter +but also the most reliable -- it operates inside the bus daemon itself, +so there is no way for the client to bypass it. + +**Layer 3 (polkit)** matters when the services mcdbus talks to perform +their own polkit authorization checks. Most system services do this. +If you want mcdbus to be able to read battery status but not manage +systemd units, polkit rules are where you express that. + +**Layer 4 (xdg-dbus-proxy)** is the most granular option. It is useful +for session bus filtering (where D-Bus bus policy is less commonly +configured) and for deployments where you want per-interface or per-path +control. The trade-off is that it adds a proxy process. + +For most deployments, Layer 1 alone is sufficient. For high-security +environments or shared systems, stacking Layers 1 + 2 + 3 provides +defense in depth. Layer 4 is there when you need fine-grained session +bus filtering that the other layers cannot express. + +### Quick reference + +| Layer | Scope | Granularity | Bus | +|-------|-------|-------------|-----| +| systemd | Process isolation | Filesystem, network, syscalls | Both | +| D-Bus policy | Message filtering | Service, interface, method | Both | +| polkit | Action authorization | Per-action, per-user/group | System (mostly) | +| xdg-dbus-proxy | Socket filtering | Service, interface, path | Session (mostly) | diff --git a/src/mcdbus/_bus.py b/src/mcdbus/_bus.py index 70bb4d4..0516333 100644 --- a/src/mcdbus/_bus.py +++ b/src/mcdbus/_bus.py @@ -2,6 +2,7 @@ import asyncio import json +import os import re import sys from typing import Any @@ -11,7 +12,7 @@ from dbus_fast.aio import MessageBus from dbus_fast.unpack import unpack_variants from fastmcp import Context -DBUS_CALL_TIMEOUT = float(30) # seconds; override via env MCDBUS_TIMEOUT +DBUS_CALL_TIMEOUT = float(os.environ.get("MCDBUS_TIMEOUT", "30")) # D-Bus spec: https://dbus.freedesktop.org/doc/dbus-specification.html _DBUS_NAME_RE = re.compile(r"^[A-Za-z_][A-Za-z0-9_.]*(\.[A-Za-z_][A-Za-z0-9_]*)+$") @@ -36,6 +37,24 @@ def validate_object_path(path: str) -> None: ) +_DBUS_SIG_CHARS = re.compile(r"^[ybnqiuxtdsogav(){}\s]*$") +MAX_SIGNATURE_LEN = 255 + + +def validate_signature(sig: str) -> None: + """Validate a D-Bus type signature string.""" + if len(sig) > MAX_SIGNATURE_LEN: + raise ValueError(f"Signature too long ({len(sig)} > {MAX_SIGNATURE_LEN})") + if not _DBUS_SIG_CHARS.match(sig): + raise ValueError(f"Invalid D-Bus signature characters: {sig!r}") + from dbus_fast.signature import SignatureTree + + try: + SignatureTree(sig) + except Exception as exc: + raise ValueError(f"Malformed D-Bus signature: {exc}") from exc + + class BusManager: """Lazy-connecting, caching manager for session and system D-Bus connections.""" @@ -122,6 +141,7 @@ def deserialize_args(args_json: str, signature: str) -> list[Any]: if not signature: return args + validate_signature(signature) tree = SignatureTree(signature) expected = len(tree.types) if len(args) != expected: @@ -146,6 +166,7 @@ def _auto_wrap_variant(val: Any, variant_cls: type) -> Any: """ # Explicit variant specification if isinstance(val, dict) and "signature" in val and "value" in val and len(val) == 2: + validate_signature(val["signature"]) return variant_cls(val["signature"], val["value"]) # Auto-infer from Python type (bool must come before int — bool is a subclass of int) if isinstance(val, bool): @@ -190,7 +211,9 @@ async def call_bus_method( f"{destination} {interface}.{member} at {path}" ) from None if reply.message_type == MessageType.ERROR: - raise RuntimeError(f"D-Bus error: {reply.error_name}: {reply.body}") + print(f"mcdbus: D-Bus error: {reply.error_name}: {reply.body}", file=sys.stderr) + detail = reply.body[0] if reply.body and isinstance(reply.body[0], str) else "" + raise RuntimeError(f"D-Bus error ({reply.error_name}): {detail}") if reply.body: return serialize_variant(reply.body) return None diff --git a/src/mcdbus/_discovery.py b/src/mcdbus/_discovery.py index 46803fa..71621bf 100644 --- a/src/mcdbus/_discovery.py +++ b/src/mcdbus/_discovery.py @@ -1,14 +1,17 @@ """Discovery tools — list services, introspect objects, walk object trees.""" +import time from collections import deque from fastmcp import Context +from fastmcp.exceptions import ToolError from mcdbus._bus import call_bus_method, get_mgr, validate_bus_name, validate_object_path from mcdbus._state import mcp MAX_TREE_DEPTH = 20 MAX_TREE_NODES = 500 +TOTAL_WALK_TIMEOUT = 60 # seconds — monotonic clock deadline for tree walks @mcp.tool() @@ -27,13 +30,16 @@ async def list_services( await ctx.info(f"Connecting to {bus} bus...") connection = await mgr.get_bus(bus) - result = await call_bus_method( - connection, - destination="org.freedesktop.DBus", - path="/org/freedesktop/DBus", - interface="org.freedesktop.DBus", - member="ListNames", - ) + try: + result = await call_bus_method( + connection, + destination="org.freedesktop.DBus", + path="/org/freedesktop/DBus", + interface="org.freedesktop.DBus", + member="ListNames", + ) + except (RuntimeError, TimeoutError) as exc: + raise ToolError(f"Error listing services on {bus} bus: {exc}") from exc names = sorted(result[0]) if result else [] if not include_unique: @@ -158,8 +164,14 @@ async def list_objects( visited: set[str] = set() queue: deque[tuple[str, int]] = deque([(root_path, 0)]) truncated = False + deadline = time.monotonic() + TOTAL_WALK_TIMEOUT while queue: + if time.monotonic() > deadline: + truncated = True + await ctx.warning(f"Tree walk timed out after {TOTAL_WALK_TIMEOUT}s") + break + path, depth = queue.popleft() if path in visited: diff --git a/src/mcdbus/_interaction.py b/src/mcdbus/_interaction.py index c6f6c88..c2a427b 100644 --- a/src/mcdbus/_interaction.py +++ b/src/mcdbus/_interaction.py @@ -1,10 +1,16 @@ """Interaction tools — method calls, property get/set.""" import json +import os import sys from dbus_fast.signature import Variant from fastmcp import Context +from fastmcp.exceptions import ToolError +from fastmcp.server.elicitation import ( + AcceptedElicitation, + CancelledElicitation, +) from mcdbus._bus import ( call_bus_method, @@ -12,10 +18,32 @@ from mcdbus._bus import ( get_mgr, validate_bus_name, validate_object_path, + validate_signature, ) from mcdbus._state import mcp +async def _confirm_or_abort(ctx: Context, message: str, operation: str) -> None: + """Elicit user confirmation; raise ToolError or return silently to proceed.""" + result = await ctx.elicit( + message, + { + "confirm": {"title": "Yes, proceed"}, + "deny": {"title": "No, cancel"}, + }, + ) + if isinstance(result, AcceptedElicitation) and result.data == "confirm": + return + if isinstance(result, CancelledElicitation): + # Client doesn't support elicitation + if os.environ.get("MCDBUS_REQUIRE_ELICITATION"): + raise ToolError("Elicitation required but client does not support it") + print(f"mcdbus: elicitation unavailable, proceeding with {operation}", file=sys.stderr) + return + # User explicitly declined + raise ToolError("Operation cancelled by user.") + + @mcp.tool() async def call_method( bus: str, @@ -48,10 +76,19 @@ async def call_method( try: body = deserialize_args(args, signature) except (ValueError, json.JSONDecodeError) as exc: - return f"Error parsing arguments: {exc}" + raise ToolError(f"Error parsing arguments: {exc}") from exc - # Log system bus writes to stderr for audit trail + # Elicit confirmation for system bus calls (session bus = user-scope, no prompt) if bus == "system": + await _confirm_or_abort( + ctx, + f"Confirm system bus method call:\n" + f" Service: {service}\n" + f" Method: {interface}.{method}\n" + f" Path: {object_path}\n" + f" Args: {args}", + f"{interface}.{method}", + ) print( f"mcdbus: system bus call {service} {interface}.{method} " f"path={object_path} sig={signature!r}", @@ -71,8 +108,7 @@ async def call_method( body=body, ) except (RuntimeError, TimeoutError) as exc: - await ctx.error(str(exc)) - return f"Error: {exc}" + raise ToolError(str(exc)) from exc if result is None: return "Method returned no data (void)." @@ -120,7 +156,7 @@ async def get_property( body=[interface, property_name], ) except (RuntimeError, TimeoutError) as exc: - return f"Error reading {interface}.{property_name}: {exc}" + raise ToolError(f"Error reading {interface}.{property_name}: {exc}") from exc if result: value = result[0] @@ -160,10 +196,22 @@ async def set_property( try: parsed_value = json.loads(value) except json.JSONDecodeError as exc: - return f"Error parsing value JSON: {exc}" + raise ToolError(f"Error parsing value JSON: {exc}") from exc + validate_signature(signature) variant = Variant(signature, parsed_value) + # Elicit confirmation for all property mutations (always state-changing) + await _confirm_or_abort( + ctx, + f"Confirm property change:\n" + f" Service: {service}\n" + f" Property: {interface}.{property_name}\n" + f" New value: {value}\n" + f" Bus: {bus}", + f"set {interface}.{property_name}", + ) + # Audit log for all set_property calls print( f"mcdbus: set_property {bus} {service} {interface}.{property_name} " @@ -182,7 +230,7 @@ async def set_property( body=[interface, property_name, variant], ) except (RuntimeError, TimeoutError) as exc: - return f"Error setting {interface}.{property_name}: {exc}" + raise ToolError(f"Error setting {interface}.{property_name}: {exc}") from exc return f"Set {interface}.{property_name} = {value}" @@ -221,7 +269,7 @@ async def get_all_properties( body=[interface], ) except (RuntimeError, TimeoutError) as exc: - return f"Error reading properties on {interface}: {exc}" + raise ToolError(f"Error reading properties on {interface}: {exc}") from exc if not result or not result[0]: return "No properties found." diff --git a/src/mcdbus/_resources.py b/src/mcdbus/_resources.py index 683b5c3..575c77b 100644 --- a/src/mcdbus/_resources.py +++ b/src/mcdbus/_resources.py @@ -3,61 +3,56 @@ from collections import deque from urllib.parse import unquote -from mcdbus._bus import BusManager, call_bus_method +from mcdbus._bus import BusManager, call_bus_method, validate_bus_name, validate_object_path from mcdbus._state import mcp MAX_RESOURCE_NODES = 200 +_resource_mgr = BusManager() + @mcp.resource("dbus://{bus}/services") async def bus_services(bus: str) -> str: """Live list of well-known service names on a D-Bus bus.""" - mgr = BusManager() - try: - connection = await mgr.get_bus(bus) - result = await call_bus_method( - connection, - destination="org.freedesktop.DBus", - path="/org/freedesktop/DBus", - interface="org.freedesktop.DBus", - member="ListNames", - ) - names = sorted(n for n in (result[0] if result else []) if not n.startswith(":")) - return "\n".join(names) - finally: - await mgr.disconnect_all() + connection = await _resource_mgr.get_bus(bus) + result = await call_bus_method( + connection, + destination="org.freedesktop.DBus", + path="/org/freedesktop/DBus", + interface="org.freedesktop.DBus", + member="ListNames", + ) + names = sorted(n for n in (result[0] if result else []) if not n.startswith(":")) + return "\n".join(names) @mcp.resource("dbus://{bus}/{service}/objects") async def service_objects(bus: str, service: str) -> str: """Object tree for a D-Bus service (bounded BFS walk).""" - mgr = BusManager() - try: - connection = await mgr.get_bus(bus) - paths: list[str] = [] - visited: set[str] = set() - queue: deque[tuple[str, int]] = deque([("/", 0)]) + validate_bus_name(service) + connection = await _resource_mgr.get_bus(bus) + paths: list[str] = [] + visited: set[str] = set() + queue: deque[tuple[str, int]] = deque([("/", 0)]) - while queue and len(paths) < MAX_RESOURCE_NODES: - path, depth = queue.popleft() - if path in visited: - continue - visited.add(path) + while queue and len(paths) < MAX_RESOURCE_NODES: + path, depth = queue.popleft() + if path in visited: + continue + visited.add(path) - try: - node = await connection.introspect(service, path) - except (RuntimeError, OSError): - continue + try: + node = await connection.introspect(service, path) + except (RuntimeError, OSError): + continue - paths.append(path) - if depth < 15: - for child in node.nodes: - child_path = path.rstrip("/") + "/" + child.name - queue.append((child_path, depth + 1)) + paths.append(path) + if depth < 15: + for child in node.nodes: + child_path = path.rstrip("/") + "/" + child.name + queue.append((child_path, depth + 1)) - return "\n".join(sorted(paths)) - finally: - await mgr.disconnect_all() + return "\n".join(sorted(paths)) @mcp.resource("dbus://{bus}/{service}/{path}/interfaces") @@ -67,10 +62,9 @@ async def object_interfaces(bus: str, service: str, path: str) -> str: if not decoded_path.startswith("/"): decoded_path = "/" + decoded_path - mgr = BusManager() - try: - connection = await mgr.get_bus(bus) - node = await connection.introspect(service, decoded_path) - return "\n".join(i.name for i in node.interfaces) - finally: - await mgr.disconnect_all() + validate_bus_name(service) + validate_object_path(decoded_path) + + connection = await _resource_mgr.get_bus(bus) + node = await connection.introspect(service, decoded_path) + return "\n".join(i.name for i in node.interfaces) diff --git a/src/mcdbus/_shortcuts.py b/src/mcdbus/_shortcuts.py index a47491a..22127b3 100644 --- a/src/mcdbus/_shortcuts.py +++ b/src/mcdbus/_shortcuts.py @@ -3,6 +3,7 @@ import fnmatch from fastmcp import Context +from fastmcp.exceptions import ToolError from mcdbus._bus import call_bus_method, get_mgr from mcdbus._state import mcp @@ -50,7 +51,7 @@ async def send_notification( ], ) except (RuntimeError, TimeoutError) as exc: - return f"Error sending notification: {exc}" + raise ToolError(f"Error sending notification: {exc}") from exc nid = result[0] if result else "unknown" return f"Notification sent (id: {nid})" @@ -84,7 +85,7 @@ async def list_systemd_units( member="ListUnits", ) except (RuntimeError, TimeoutError) as exc: - return f"Error listing units: {exc}" + raise ToolError(f"Error listing units: {exc}") from exc if not result or not result[0]: return "No units found." @@ -168,7 +169,7 @@ async def media_player_control( member=dbus_method, ) except (RuntimeError, TimeoutError) as exc: - return f"Error controlling player: {exc}" + raise ToolError(f"Error controlling player: {exc}") from exc # Read current playback status try: @@ -245,7 +246,7 @@ async def network_status(ctx: Context) -> str: body=[_NM_IFACE], ) except (RuntimeError, TimeoutError) as exc: - return f"NetworkManager not available: {exc}" + raise ToolError(f"NetworkManager not available: {exc}") from exc if not state_result or not state_result[0]: return "NetworkManager returned no properties." @@ -318,7 +319,7 @@ async def battery_status(ctx: Context) -> str: member="EnumerateDevices", ) except (RuntimeError, TimeoutError) as exc: - return f"UPower not available: {exc}" + raise ToolError(f"UPower not available: {exc}") from exc if not devices_result or not devices_result[0]: return "No power devices found." @@ -403,7 +404,7 @@ async def bluetooth_devices(ctx: Context) -> str: member="GetManagedObjects", ) except (RuntimeError, TimeoutError) as exc: - return f"bluez not available: {exc}" + raise ToolError(f"bluez not available: {exc}") from exc if not objects_result or not objects_result[0]: return "No Bluetooth objects found." @@ -468,7 +469,7 @@ async def kwin_windows(ctx: Context) -> str: body=[""], ) except (RuntimeError, TimeoutError) as exc: - return f"KWin not available: {exc}" + raise ToolError(f"KWin not available: {exc}") from exc if not result or not result[0]: return "No windows found (KWin WindowsRunner returned empty)." diff --git a/tests/conftest.py b/tests/conftest.py index b53e3b4..5e24329 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -18,6 +18,13 @@ def _reset_mcp_singleton(): dead loop and test 2 would crash. Re-creating these objects keeps every test isolated. """ + _PRIVATE_ATTRS = ("_started", "_lifespan_result", "_lifespan_result_set") + for attr in _PRIVATE_ATTRS: + if not hasattr(mcp, attr): + raise AttributeError( + f"FastMCP removed {attr!r} — fixture needs update " + f"(fastmcp version may have changed)" + ) mcp._started = asyncio.Event() mcp._lifespan_result = None mcp._lifespan_result_set = False diff --git a/tests/test_elicitation.py b/tests/test_elicitation.py new file mode 100644 index 0000000..36f383d --- /dev/null +++ b/tests/test_elicitation.py @@ -0,0 +1,83 @@ +"""Tests for elicitation (human-in-the-loop confirmation) on mutation tools.""" + +import os +from unittest.mock import AsyncMock, patch + +import pytest +from fastmcp.exceptions import ToolError +from fastmcp.server.elicitation import ( + AcceptedElicitation, + CancelledElicitation, + DeclinedElicitation, +) + +from mcdbus._interaction import _confirm_or_abort + + +class TestConfirmOrAbort: + """Unit tests for the _confirm_or_abort helper.""" + + async def test_accepted_proceeds(self): + ctx = AsyncMock() + ctx.elicit = AsyncMock(return_value=AcceptedElicitation(data="confirm")) + # Should return without raising + await _confirm_or_abort(ctx, "Test message", "test_op") + + async def test_declined_raises_tool_error(self): + ctx = AsyncMock() + ctx.elicit = AsyncMock(return_value=DeclinedElicitation()) + with pytest.raises(ToolError, match="cancelled by user"): + await _confirm_or_abort(ctx, "Test message", "test_op") + + async def test_accepted_wrong_key_raises(self): + ctx = AsyncMock() + ctx.elicit = AsyncMock(return_value=AcceptedElicitation(data="deny")) + with pytest.raises(ToolError, match="cancelled by user"): + await _confirm_or_abort(ctx, "Test message", "test_op") + + async def test_cancelled_proceeds_by_default(self): + ctx = AsyncMock() + ctx.elicit = AsyncMock(return_value=CancelledElicitation()) + # Client doesn't support elicitation — should proceed silently + await _confirm_or_abort(ctx, "Test message", "test_op") + + async def test_cancelled_hard_fails_when_required(self): + ctx = AsyncMock() + ctx.elicit = AsyncMock(return_value=CancelledElicitation()) + with patch.dict(os.environ, {"MCDBUS_REQUIRE_ELICITATION": "1"}): + with pytest.raises(ToolError, match="Elicitation required"): + await _confirm_or_abort(ctx, "Test message", "test_op") + + +class TestCallMethodElicitation: + """Verify call_method triggers elicitation on system bus but not session bus.""" + + async def test_session_bus_no_elicitation(self, client): + """Session bus calls should not trigger elicitation.""" + # Ping on session bus — should succeed without any elicitation + result = await client.call_tool("call_method", { + "bus": "session", + "service": "org.freedesktop.DBus", + "object_path": "/org/freedesktop/DBus", + "interface": "org.freedesktop.DBus.Peer", + "method": "Ping", + }) + text = result.content[0].text + assert "void" in text.lower() + + +class TestSetPropertyElicitation: + """Verify set_property always triggers elicitation.""" + + async def test_invalid_property_still_validates_first(self, client): + """Validation errors should fire before elicitation.""" + with pytest.raises(ToolError, match="Invalid D-Bus"): + await client.call_tool("set_property", { + "bus": "session", + "service": "not-valid", + "object_path": "/foo", + "interface": "org.test.Iface", + "property_name": "Foo", + "value": '"bar"', + "signature": "s", + }) diff --git a/tests/test_tools.py b/tests/test_tools.py index 587537b..1ed82c0 100644 --- a/tests/test_tools.py +++ b/tests/test_tools.py @@ -86,16 +86,15 @@ class TestCallMethod: assert isinstance(bus_id, str) assert len(bus_id) > 10 - async def test_bad_method_returns_error(self, client: Client): - result = await client.call_tool("call_method", { - "bus": "session", - "service": "org.freedesktop.DBus", - "object_path": "/org/freedesktop/DBus", - "interface": "org.freedesktop.DBus", - "method": "NoSuchMethod", - }) - text = result.content[0].text - assert "Error" in text or "error" in text + async def test_bad_method_raises_tool_error(self, client: Client): + with pytest.raises(ToolError, match="D-Bus error"): + await client.call_tool("call_method", { + "bus": "session", + "service": "org.freedesktop.DBus", + "object_path": "/org/freedesktop/DBus", + "interface": "org.freedesktop.DBus", + "method": "NoSuchMethod", + }) class TestGetProperty: @@ -127,6 +126,48 @@ class TestGetAllProperties: assert "Features" in text +# --------------------------------------------------------------------------- +# Validation +# --------------------------------------------------------------------------- + +class TestSignatureValidation: + async def test_invalid_signature_rejected(self, client: Client): + with pytest.raises(ToolError, match="Invalid D-Bus signature"): + await client.call_tool("call_method", { + "bus": "session", + "service": "org.freedesktop.DBus", + "object_path": "/org/freedesktop/DBus", + "interface": "org.freedesktop.DBus", + "method": "GetId", + "signature": "INVALID!@#", + "args": '["x"]', + }) + + async def test_signature_too_long(self, client: Client): + with pytest.raises(ToolError, match="Signature too long"): + await client.call_tool("call_method", { + "bus": "session", + "service": "org.freedesktop.DBus", + "object_path": "/org/freedesktop/DBus", + "interface": "org.freedesktop.DBus", + "method": "GetId", + "signature": "s" * 256, + "args": '["x"]', + }) + + async def test_malformed_signature_rejected(self, client: Client): + with pytest.raises(ToolError, match="Malformed D-Bus signature"): + await client.call_tool("call_method", { + "bus": "session", + "service": "org.freedesktop.DBus", + "object_path": "/org/freedesktop/DBus", + "interface": "org.freedesktop.DBus", + "method": "GetId", + "signature": "((", + "args": '["x"]', + }) + + # --------------------------------------------------------------------------- # Shortcut tools — existing # --------------------------------------------------------------------------- @@ -179,60 +220,73 @@ class TestMediaPlayerControl: # --------------------------------------------------------------------------- -# Shortcut tools — new +# Shortcut tools — new (service may or may not be available) # --------------------------------------------------------------------------- class TestNetworkStatus: async def test_returns_status(self, client: Client): - result = await client.call_tool("network_status", {}) + try: + result = await client.call_tool("network_status", {}) + except ToolError: + return # NetworkManager not available on this system text = result.content[0].text - assert "Network Status" in text or "NetworkManager not available" in text + assert "Network Status" in text async def test_has_state(self, client: Client): - result = await client.call_tool("network_status", {}) + try: + result = await client.call_tool("network_status", {}) + except ToolError: + return # NetworkManager not available text = result.content[0].text - if "not available" not in text: - assert "State:" in text + assert "State:" in text class TestBatteryStatus: async def test_returns_result(self, client: Client): - result = await client.call_tool("battery_status", {}) + try: + result = await client.call_tool("battery_status", {}) + except ToolError: + return # UPower not available on this system text = result.content[0].text - # Desktop may not have battery — both outcomes are valid assert ( "Battery Status" in text or "No batteries found" in text or "No power devices" in text - or "UPower not available" in text ) class TestBluetoothDevices: async def test_returns_result(self, client: Client): - result = await client.call_tool("bluetooth_devices", {}) + try: + result = await client.call_tool("bluetooth_devices", {}) + except ToolError: + return # bluez not available on this system text = result.content[0].text assert ( "Bluetooth Devices" in text or "No Bluetooth devices" in text or "No Bluetooth objects" in text - or "bluez not available" in text ) class TestKwinWindows: async def test_returns_windows(self, client: Client): - result = await client.call_tool("kwin_windows", {}) + try: + result = await client.call_tool("kwin_windows", {}) + except ToolError: + return # KWin not available on this system text = result.content[0].text assert ( "KWin Windows" in text - or "KWin not available" in text or "No windows found" in text or "No open windows" in text ) async def test_has_table(self, client: Client): - result = await client.call_tool("kwin_windows", {}) + try: + result = await client.call_tool("kwin_windows", {}) + except ToolError: + return # KWin not available text = result.content[0].text if "KWin Windows" in text: assert "| Window |" in text