Merge remote-tracking branch 'origin/feat/93' into integ

2026-08-12 14:18:07 +00:00
parent d2c126a60c 603d24566d
commit ce82f7b5c3
3 changed files with 307 additions and 0 deletions
@@ -929,6 +929,21 @@ class ServerEditor(QFrame):
        self.sidecar_warn.setWordWrap(True)
        self.sidecar_warn.hide()
        v.addWidget(self.sidecar_warn)
+        # Shown when the sidecar config is group/other-accessible (#93): ssh-mcp
+        # refuses to start unless it's 0600 / its dir 0700. One-click chmod fix.
+        # POSIX only — hidden on Windows where modes don't apply.
+        self.perm_warn = QLabel("")
+        self.perm_warn.setStyleSheet(f"color: {WARN};")
+        self.perm_warn.setWordWrap(True)
+        self.perm_warn.hide()
+        self.perm_fix_btn = QPushButton("Fix permissions")
+        self.perm_fix_btn.setToolTip("chmod the config file to 0600 and its directory to 0700")
+        self.perm_fix_btn.clicked.connect(self._fix_permissions)
+        self.perm_fix_btn.hide()
+        perm_row = QHBoxLayout()
+        perm_row.addWidget(self.perm_warn, 1)
+        perm_row.addWidget(self.perm_fix_btn)
+        v.addLayout(perm_row)
        v.addWidget(self._lbl("Environment variables"))
        self.env = KeyValueTable(
            "Variable", "Value", on_change=self._emit, before_change=self._before_change
@@ -1031,6 +1046,8 @@ class ServerEditor(QFrame):
            self.removed_flag_warn.hide()
            self.removed_flag_fix_btn.hide()
            self.sidecar_warn.hide()
+            self.perm_warn.hide()
+            self.perm_fix_btn.hide()
            self._loading = False
            return
        self.setEnabled(True)
@@ -1121,6 +1138,8 @@ class ServerEditor(QFrame):
            self.removed_flag_warn.hide()
            self.removed_flag_fix_btn.hide()
            self.sidecar_warn.hide()
+            self.perm_warn.hide()
+            self.perm_fix_btn.hide()
            return
        _, notes = core.split_suspicious_args(self._current_arg_lines())
        if notes:
@@ -1161,6 +1180,15 @@ class ServerEditor(QFrame):
            self.sidecar_warn.show()
        else:
            self.sidecar_warn.hide()
+        # Sidecar filesystem permissions (#93). Real platform/fs; no-op on Windows.
+        perm_warnings = core.sidecar_permission_warnings(stdio)
+        if perm_warnings:
+            self.perm_warn.setText("⚠  " + "\n".join(perm_warnings))
+            self.perm_warn.show()
+            self.perm_fix_btn.show()
+        else:
+            self.perm_warn.hide()
+            self.perm_fix_btn.hide()

    def _fix_args(self):
        if self._before_change:
@@ -1181,6 +1209,23 @@ class ServerEditor(QFrame):
        self.env.load(migrated.get("env", {}))
        self.args.setPlainText("\n".join(migrated.get("args", [])))

+    def _fix_permissions(self):
+        """chmod the sidecar config to 0600 / its dir to 0700 (#93)."""
+        stdio = {
+            "command": self.command.text().strip(),
+            "args": self._current_arg_lines(),
+            "env": self.env.dump(),
+        }
+        target = core.sidecar_permission_fix_target(stdio)
+        if target is None:
+            return
+        changed, note = core.fix_permissions(target)
+        if not changed:
+            # Surface the failure in-place rather than silently doing nothing.
+            self.perm_warn.setText("⚠  " + (note or "could not change permissions"))
+            return
+        self._check_args()  # re-check; the warning clears when perms are now tight
+
    # --- dependency ------------------------------------------------------ #
    def refresh_dependency(self, auto_open=False):
        if not self.isEnabled():
@@ -2954,6 +2954,152 @@ def version_status(data: dict, *, resolved: str | None = _UNSET, **lookup) -> di
    }


+# --------------------------------------------------------------------------- #
+# Filesystem permission pre-flight (issue #93, epic #94)
+#
+# ssh-mcp refuses to start if its config is group/world-readable: it throws when
+# `mode & 0o077` is set, requiring dir 0700 / file 0600. A GUI user has no idea
+# what `chmod 600` means — they just get a dead server. Generalise: any config
+# file that carries credentials should be permission-checked. POSIX only — modes
+# don't apply on Windows, where this degrades to a clean no-op.
+#
+# Depends on #91's sidecar path resolution to know WHICH file to check.
+# --------------------------------------------------------------------------- #
+def _is_windows_platform(platform: str | None) -> bool:
+    p = platform if platform is not None else sys.platform
+    return p.startswith("win")
+
+
+def _permission_mode(path, stat_mode) -> int | None:
+    """The 0o777 permission bits of `path`, or None if it can't be stat-ed."""
+    if stat_mode is not None:
+        return stat_mode(path)
+    try:
+        return os.stat(path).st_mode & 0o777
+    except OSError:
+        return None
+
+
+def permission_status(
+    path: str | os.PathLike,
+    *,
+    platform: str | None = None,
+    stat_mode=None,
+) -> dict | None:
+    """Check a credential-bearing config file's mode (and its directory's). Pure.
+
+    Returns None on Windows (POSIX modes don't apply) or when the file doesn't
+    exist (nothing to check). Otherwise a dict:
+    ``{path, mode, file_ok, dir, dir_mode, dir_ok, ok, problems}``.
+
+    The rule mirrors ssh-mcp's own guard: any group/other bit set (``mode &
+    0o077``) is not-ok — the file must be 0600, its directory 0700. ``stat_mode``
+    is an injectable ``path -> int|None`` (the 0o777 bits) so tests never chmod a
+    real file; it defaults to a real ``os.stat``.
+    """
+    if _is_windows_platform(platform):
+        return None
+    p = Path(path)
+    fmode = _permission_mode(p, stat_mode)
+    if fmode is None:
+        return None  # file absent / unreadable -> nothing to pre-flight
+    dmode = _permission_mode(p.parent, stat_mode)
+    problems: list[str] = []
+    file_ok = not (fmode & 0o077)
+    if not file_ok:
+        problems.append(
+            f"the config file is readable by other users (mode {fmode:04o}); "
+            f"ssh-mcp requires 0600 and refuses to start otherwise"
+        )
+    # The directory is only judged when we could read its mode.
+    dir_ok = dmode is None or not (dmode & 0o077)
+    if dmode is not None and not dir_ok:
+        problems.append(
+            f"the containing directory is accessible to other users (mode {dmode:04o}); "
+            f"ssh-mcp requires 0700"
+        )
+    return {
+        "path": p,
+        "mode": fmode,
+        "file_ok": file_ok,
+        "dir": p.parent,
+        "dir_mode": dmode,
+        "dir_ok": dir_ok,
+        "ok": file_ok and dir_ok,
+        "problems": problems,
+    }
+
+
+def fix_permissions(
+    path: str | os.PathLike,
+    *,
+    platform: str | None = None,
+    chmod=None,
+) -> tuple[bool, str | None]:
+    """Tighten a config file to 0600 and its directory to 0700. POSIX only.
+
+    Returns ``(changed, note)``. On Windows: ``(False, None)`` — nothing to do.
+    ``chmod`` is an injectable ``(path, mode) -> None`` so tests don't touch real
+    files; it defaults to ``os.chmod``. Only the bits that are currently wrong are
+    reported, but both file and dir are set unconditionally (cheap and idempotent).
+    """
+    if _is_windows_platform(platform):
+        return False, None
+    p = Path(path)
+    chmod = chmod if chmod is not None else os.chmod
+    try:
+        chmod(p, 0o600)
+        chmod(p.parent, 0o700)
+    except OSError as e:
+        return False, f"could not change permissions: {e}"
+    return True, "set the config file to 0600 and its directory to 0700"
+
+
+def sidecar_permission_warnings(
+    data: dict,
+    *,
+    platform: str | None = None,
+    environ: dict | None = None,
+    home: str | os.PathLike | None = None,
+    stat_mode=None,
+) -> list[str]:
+    """Permission advisories for a server's sidecar config (#93 over #91's path).
+
+    Resolves the ServerSpec sidecar path, and — when that file exists and is
+    group/other-accessible — returns a plain-language warning per problem. Empty
+    list == fine, no sidecar, or Windows. Mirrors ``sidecar_warnings``' shape so
+    the GUI wiring is identical.
+    """
+    if _is_windows_platform(platform):
+        return []
+    spec = resolve_server_spec(data)
+    if spec is None:
+        return []
+    path = sidecar_path(spec, platform=platform, environ=environ, home=home)
+    if path is None:
+        return []
+    status = permission_status(path, platform=platform, stat_mode=stat_mode)
+    if status is None or status["ok"]:
+        return []
+    return [f"{spec.package}: {prob}" for prob in status["problems"]]
+
+
+def sidecar_permission_fix_target(
+    data: dict,
+    *,
+    platform: str | None = None,
+    environ: dict | None = None,
+    home: str | os.PathLike | None = None,
+) -> Path | None:
+    """The sidecar path a "fix permissions" action should chmod, or None."""
+    if _is_windows_platform(platform):
+        return None
+    spec = resolve_server_spec(data)
+    if spec is None:
+        return None
+    return sidecar_path(spec, platform=platform, environ=environ, home=home)
+
+
 # --------------------------------------------------------------------------- #
 # Validation
 # --------------------------------------------------------------------------- #
@@ -3527,6 +3527,122 @@ def test_version_status_none_for_non_npx():
    assert c.version_status({"url": "https://x"}) is None


+# --------------------------------------------------------------------------- #
+# Filesystem permission pre-flight (issue #93)
+# --------------------------------------------------------------------------- #
+def test_permission_status_ok_when_0600():
+    # NOTE: key the injected mode map on Path(p).as_posix() — on the Windows CI
+    # runner permission_status wraps the path in a WindowsPath, so str(p) would
+    # use backslashes and miss the lookup (the same portability trap as #91).
+    st = c.permission_status(
+        "/cfg/config.toml",
+        platform="darwin",
+        stat_mode=lambda p: {"/cfg/config.toml": 0o600, "/cfg": 0o700}.get(Path(p).as_posix()),
+    )
+    assert st["ok"] is True
+    assert st["mode"] == 0o600
+    assert st["problems"] == []
+
+
+def test_permission_status_blocks_group_world_readable_file():
+    st = c.permission_status(
+        "/cfg/config.toml",
+        platform="linux",
+        stat_mode=lambda p: {"/cfg/config.toml": 0o644, "/cfg": 0o755}.get(Path(p).as_posix()),
+    )
+    assert st["ok"] is False
+    assert st["file_ok"] is False
+    assert st["dir_ok"] is False
+    # Two plain-language problems, naming the offending octal modes.
+    text = " ".join(st["problems"])
+    assert "0644" in text and "0600" in text
+    assert "0755" in text and "0700" in text
+
+
+def test_permission_status_file_bad_dir_ok():
+    st = c.permission_status(
+        "/cfg/config.toml",
+        platform="linux",
+        stat_mode=lambda p: {"/cfg/config.toml": 0o640, "/cfg": 0o700}.get(Path(p).as_posix()),
+    )
+    assert st["file_ok"] is False
+    assert st["dir_ok"] is True
+    assert len(st["problems"]) == 1
+
+
+def test_permission_status_none_on_windows_and_missing_file():
+    # Windows: POSIX modes don't apply -> None (clean no-op).
+    assert c.permission_status("/cfg/config.toml", platform="win32") is None
+    # Absent file -> nothing to pre-flight.
+    assert c.permission_status("/cfg/gone.toml", platform="linux", stat_mode=lambda p: None) is None
+
+
+def test_fix_permissions_chmods_file_and_dir():
+    # as_posix() so the recorded paths compare equal on the Windows CI runner too.
+    calls = []
+    changed, note = c.fix_permissions(
+        "/cfg/config.toml",
+        platform="linux",
+        chmod=lambda p, m: calls.append((Path(p).as_posix(), m)),
+    )
+    assert changed is True
+    assert ("/cfg/config.toml", 0o600) in calls
+    assert ("/cfg", 0o700) in calls
+    assert note and "0600" in note
+
+
+def test_fix_permissions_noop_on_windows():
+    calls = []
+    changed, note = c.fix_permissions(
+        "/cfg/config.toml", platform="win32", chmod=lambda p, m: calls.append((p, m))
+    )
+    assert changed is False
+    assert note is None
+    assert calls == []
+
+
+def test_fix_permissions_reports_oserror():
+    def boom(p, m):
+        raise OSError("nope")
+
+    changed, note = c.fix_permissions("/cfg/config.toml", platform="linux", chmod=boom)
+    assert changed is False
+    assert "could not change permissions" in note
+
+
+def test_sidecar_permission_warnings_over_real_path():
+    data = {"command": "npx", "args": ["-y", "ssh-mcp", "--host=h"]}
+    real = c.sidecar_path(c.SERVER_SPECS["ssh-mcp"], platform="darwin", environ={}, home="/Users/t")
+
+    def stat_mode(p):
+        return 0o644 if Path(p) == real else 0o700
+
+    warns = c.sidecar_permission_warnings(
+        data, platform="darwin", environ={}, home="/Users/t", stat_mode=stat_mode
+    )
+    assert warns and warns[0].startswith("ssh-mcp:")
+    assert "0600" in warns[0]
+    # Windows / unknown package -> nothing.
+    assert c.sidecar_permission_warnings(data, platform="win32") == []
+    assert c.sidecar_permission_warnings({"command": "npx", "args": ["other"]}) == []
+
+
+def test_sidecar_permission_warnings_quiet_when_tight():
+    data = {"command": "npx", "args": ["-y", "ssh-mcp", "--host=h"]}
+    warns = c.sidecar_permission_warnings(
+        data, platform="darwin", environ={}, home="/Users/t", stat_mode=lambda p: 0o600
+    )
+    assert warns == []
+
+
+def test_sidecar_permission_fix_target():
+    data = {"command": "npx", "args": ["-y", "ssh-mcp"]}
+    tgt = c.sidecar_permission_fix_target(data, platform="darwin", environ={}, home="/Users/t")
+    assert tgt is not None and tgt.name == "config.toml"
+    assert c.sidecar_permission_fix_target(data, platform="win32") is None
+    assert c.sidecar_permission_fix_target({"command": "npx", "args": ["other"]}) is None
+
+
 # --------------------------------------------------------------------------- #
 # Move to environment variable (issue #83)
 # --------------------------------------------------------------------------- #