diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9c4ac20..bd93e7f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -173,6 +173,17 @@ jobs: with: persist-credentials: false + # arm64 is built under emulation. Measured tradeoff: the alternative — + # a second native `ubuntu-24.04-arm` runner plus a manifest-merge job — + # is faster but splits one digest across two jobs, which the promote and + # verify steps below are built around. The image installs prebuilt + # manylinux aarch64 wheels rather than compiling, so emulation only + # pays for UV_COMPILE_BYTECODE and `useradd`. + - name: Set up QEMU (arm64 emulation) + uses: docker/setup-qemu-action@1f40c72289eff860ee54a304f1438e3cff362e0a # v4.3.0 + with: + platforms: arm64 + - name: Set up Docker Buildx uses: docker/setup-buildx-action@37fe631027851001ddb9b187196cc803df7f5f0e # v4.3.0 @@ -222,6 +233,7 @@ jobs: uses: docker/build-push-action@53b7df96c91f9c12dcc8a07bcb9ccacbed38856a # v7.3.0 with: context: . + platforms: linux/amd64,linux/arm64 outputs: type=image,name=${{ env.IMAGE }},push-by-digest=true,name-canonical=true,push=true provenance: false sbom: false @@ -248,10 +260,31 @@ jobs: # Scope of the gate, deliberately narrow: `severity: CRITICAL` only, so # HIGH image findings never fail the build, and `ignore-unfixed` is unset. # Open posture decision, tracked in docs/fixes/05-followups.md §3. + # + # ONE STEP PER PLATFORM, and that is load-bearing since the build became + # multi-arch: the pushed digest is now an INDEX, and Trivy resolves an + # index to the runner's own platform. A single step would therefore have + # gated amd64 and published arm64 unscanned — the gate would still be + # green and half the image would be uninspected. `TRIVY_PLATFORM` is + # used because trivy-action v0.36.0 exposes no `platform` input; it + # passes environment variables through to Trivy, which reads this one. # ----------------------------------------------------------------------- - - name: Scan published digest with Trivy + - name: Scan published digest with Trivy (amd64) if: env.PUBLISH == 'true' uses: aquasecurity/trivy-action@ed142fd0673e97e23eac54620cfb913e5ce36c25 # v0.36.0 + env: + TRIVY_PLATFORM: linux/amd64 + with: + image-ref: ${{ env.IMAGE }}@${{ steps.build.outputs.digest }} + severity: CRITICAL + exit-code: 1 + trivyignores: .trivyignore + + - name: Scan published digest with Trivy (arm64) + if: env.PUBLISH == 'true' + uses: aquasecurity/trivy-action@ed142fd0673e97e23eac54620cfb913e5ce36c25 # v0.36.0 + env: + TRIVY_PLATFORM: linux/arm64 with: image-ref: ${{ env.IMAGE }}@${{ steps.build.outputs.digest }} severity: CRITICAL @@ -303,46 +336,72 @@ jobs: # CLI appends a newline — not the cause here, but a reason not to go back # to hashing stdout.) # - # So compare the right thing: the set of image manifests the tag actually - # serves must be exactly {scanned digest}. Attestation manifests that - # buildx/attest may attach are excluded — they are metadata about the - # image, not another runnable image. Do not "simplify" this back to + # So compare the right thing: the set of image manifests the tag serves + # must equal the set the SCANNED reference serves. Attestation manifests + # that buildx/attest may attach are excluded — they are metadata about + # the image, not another runnable image. Do not "simplify" this back to # comparing the tag's own digest; that asserts an invariant the promote # step deliberately does not hold. - - name: Verify every tag serves only the scanned digest + # + # The expected set is DERIVED from the scanned reference rather than + # assumed to be `{DIGEST}`, which the multi-arch build made necessary: + # `steps.build.outputs.digest` is now an index digest, and + # `imagetools create` copies that index's CHILDREN into the new tag. A + # check hardcoding `{DIGEST}` would have failed every publish run — the + # same shape as the 2026-07-26 incident above, one level down. Deriving + # also keeps the single-platform case working unchanged: a bare manifest + # has no children, so the expected set is just itself. + - name: Verify every tag serves only the scanned image manifests if: env.PUBLISH == 'true' env: TAGS: ${{ steps.meta.outputs.tags }} DIGEST: ${{ steps.build.outputs.digest }} run: | set -euo pipefail - while IFS= read -r tag; do - [ -z "$tag" ] && continue - raw="$(docker buildx imagetools inspect --raw "$tag")" - # An index lists children under .manifests; a bare manifest has none. + + # Image manifests a reference resolves to, one per line, sorted. + # An index lists children under .manifests; a bare manifest has none. + served_manifests() { + local ref="$1" raw served + raw="$(docker buildx imagetools inspect --raw "$ref")" served="$(printf '%s' "$raw" | jq -r ' if .manifests then - [ .manifests[] - | select((.annotations["vnd.docker.reference.type"] // "") - != "attestation-manifest") - | .digest ] | join(" ") + .manifests[] + | select((.annotations["vnd.docker.reference.type"] // "") + != "attestation-manifest") + | .digest else empty - end')" + end' | sort)" if [ -z "$served" ]; then - served="$(docker buildx imagetools inspect --format '{{.Manifest.Digest}}' "$tag")" + docker buildx imagetools inspect --format '{{.Manifest.Digest}}' "$ref" + else + printf '%s\n' "$served" fi - echo "${tag} serves: ${served} (scanned ${DIGEST})" - for d in $served; do - if [ "$d" != "${DIGEST}" ]; then - echo "::error::${tag} serves ${d}, which is not the scanned digest ${DIGEST}" >&2 - exit 1 - fi - done + } + + expected="$(served_manifests "${IMAGE}@${DIGEST}")" + if [ -z "$expected" ]; then + echo "::error::scanned ${DIGEST} resolved to no image manifest at all" >&2 + exit 1 + fi + echo "scanned ${DIGEST} serves:" + printf ' %s\n' $expected + + while IFS= read -r tag; do + [ -z "$tag" ] && continue + served="$(served_manifests "$tag")" if [ -z "$served" ]; then echo "::error::${tag} resolved to no image manifest at all" >&2 exit 1 fi + if [ "$served" != "$expected" ]; then + echo "::error::${tag} serves a different set of manifests than the scanned index" >&2 + echo " tag: $(printf '%s' "$served" | tr '\n' ' ')" >&2 + echo " scanned: $(printf '%s' "$expected" | tr '\n' ' ')" >&2 + exit 1 + fi + echo "${tag} OK — serves exactly the scanned manifests" done <<< "$TAGS" - name: Attest build provenance for the image diff --git a/AGENTS.md b/AGENTS.md index d756120..ff4d00e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -12,7 +12,7 @@ Command sources: `.github/workflows/ci.yml` (the gates), `.github/workflows/rele - **Type:** app — published as a PyPI wheel (`uvx blc-ssh-mcp`) *and* a container image (`ghcr.io/blackaxgit/ssh-mcp`). **The PyPI distribution is `blc-ssh-mcp`, not `ssh-mcp`** — that name belongs to an unrelated project this repo has never owned; see the comment above `name` in `pyproject.toml`. The import package is still `ssh_mcp` and the image is still `.../ssh-mcp`. - **Stacks:** Python 3.11-3.14, `uv` + hatchling, asyncssh, MCP SDK, Starlette/uvicorn (HTTP transport only) - **Deploy targets:** N/A — this is a tool operators run themselves, not a hosted service -- **Version:** `src/ssh_mcp/__init__.py` is the single source (hatchling reads it). Currently `0.8.0`. `0.7.0` shipped 2026-09-06 (the MCP SDK v2 migration) and is the first release published under a reviewer-gated `pypi` environment; `0.6.0`, `0.6.1` and `0.6.2` all shipped 2026-07-26. `CHANGELOG.md` `[Unreleased]` is where post-0.8.0 work accrues. +- **Version:** `src/ssh_mcp/__init__.py` is the single source (hatchling reads it). Currently `0.8.1`. `0.7.0` shipped 2026-09-06 (the MCP SDK v2 migration) and is the first release published under a reviewer-gated `pypi` environment; `0.6.0`, `0.6.1` and `0.6.2` all shipped 2026-07-26. `CHANGELOG.md` `[Unreleased]` is where post-0.8.0 work accrues. **What this tool does matters for how you treat it.** Every tool call runs a shell command or transfers a file on a remote host. A bug here is not a crashed request; it is an unintended command on someone's infrastructure, or a write to the operator's own machine. @@ -20,7 +20,7 @@ Command sources: `.github/workflows/ci.yml` (the gates), `.github/workflows/rele ```bash uv sync --locked --extra dev # setup. --locked is required; see Gotchas -uv run pytest # full suite (781 tests, seconds) +uv run pytest # full suite (848 tests, seconds) uv run pytest tests/test_ssh.py::TestRedactSecrets -v # one class uv run pytest 'tests/test_ssh.py::TestRedactSecrets::' -v # one test uv run pytest -k "confinement" -v # by keyword @@ -85,6 +85,8 @@ Four module-level tunables in `ssh.py` are *deliberately* not `Settings` fields Tests mirror modules (`test_ssh.py` ↔ `ssh.py`). `pytest-asyncio` runs in `asyncio_mode = "auto"`, so `async def test_*` needs no decorator. Hypothesis drives the redaction and dangerous-command property tests. +**Run `HYPOTHESIS_PROFILE=ci uv run pytest` before pushing any change to `_redact_secrets` or `_DANGEROUS_PATTERNS`.** A bare `uv run pytest` uses the `dev` profile — 50 examples — while CI runs 200 (`tests/conftest.py`). That gap is not theoretical: 0.8.1's redaction change passed the local suite and CI found two real defects in it on the first run, both needing a generated secret containing `&`. One was an inconsistent marker (a value flagged under `--password=` and not under `--token=`, because a different pipeline rule matched first); the other was a later rule *downgrading* a marker an earlier rule had set. The 200-example run costs about a second here. + Not every test file mirrors a module. `tests/test_input_limits.py` (18 tests) guards `max_command_bytes` at the tool boundary; `tests/test_logging.py` (7) and `tests/test_otel.py` (7) cover the observability wiring in `server.py`. Three test files assert *invariants* rather than behaviour, and are load-bearing — read them before changing what they guard: @@ -124,7 +126,11 @@ The full state matrix is documented on `ExecResult` in `models.py`. Do not "norm | `audit.yml` | daily cron + manual | `pip-audit` against `main`, so a new advisory surfaces on its own schedule instead of reddening an unrelated PR | | `release.yml` | tag `v*` | re-runs test/lint/audit → `build` (asserts tag == packaged version, emits CycloneDX SBOM) → `publish-pypi` → `github-release` | -**Every push to `main` publishes a container image.** The `docker` job's `PUBLISH` is `github.event_name == 'push' && (github.ref == 'refs/heads/main' || startsWith(github.ref, 'refs/tags/v'))` (`ci.yml:160`), so a merged PR pushes `latest` + `type=sha` tags to GHCR with **no environment gate and no human approval** — a tag is not the only path to a publish. PRs build and scan only. Two Trivy steps (CRITICAL severity) gate the promote, both reading `.trivyignore`. +**Every push to `main` publishes a container image.** The `docker` job's `PUBLISH` is `github.event_name == 'push' && (github.ref == 'refs/heads/main' || startsWith(github.ref, 'refs/tags/v'))`, so a merged PR pushes `latest` + `type=sha` tags to GHCR with **no environment gate and no human approval** — a tag is not the only path to a publish. PRs build and scan only. + +**The image is multi-arch (`linux/amd64,linux/arm64`) as of 0.8.1, and that makes the Trivy gate arity load-bearing.** A multi-arch push produces an *index*, and Trivy resolves an index to the runner's own platform, so **one** scan step gates amd64 and publishes arm64 uninspected — with a green gate. There is therefore one scan step per platform, selected via `TRIVY_PLATFORM` (trivy-action v0.36.0 exposes no `platform` input but passes environment through). For the same reason the digest-verification step **derives** its expected manifest set from the scanned reference instead of comparing against `steps.build.outputs.digest`, which is now an index digest whose children are what `imagetools create` copies into the tag — hardcoding it would fail every publish run, the same shape as the 2026-07-26 incident one level down. `test_published_image_is_multi_arch_and_every_platform_is_scanned` asserts platforms-built equals platforms-scanned. + +**The PR path builds amd64 only, so arm64 build breakage surfaces on `main`, not on the PR.** `load: true` cannot load a multi-platform result, so the PR build stays single-platform for fast feedback and Trivy has a local image to scan. The consequence is asymmetric and deliberate: a Dockerfile change that breaks the arm64 leg passes review and turns `main` red. That is loud and publishes nothing — the digest push happens in the same failed step — but it does mean an arch-sensitive change should be built locally first (`docker buildx build --platform linux/arm64 .`), which is how 0.8.1's arm64 leg was verified before merge. Both pinned bases were checked to be multi-arch indexes at the same time (`python:3.14-slim-trixie` and `ghcr.io/astral-sh/uv:0.12.10`); pinning a *single-platform* digest there is the failure this would otherwise find late. All third-party actions are pinned to a full commit SHA with a `# vX` comment, and `astral-sh/setup-uv` additionally pins a `version:` so `uv` itself cannot float. Keep both — an unpinned tool is how this repo previously had CI go red on commits that had passed days earlier. Never replace a SHA with a tag reference. None of this is prose-only convention: `tests/test_ci_lint_determinism.py` asserts it, so a workflow edit that drops a pin fails the suite. @@ -155,13 +161,15 @@ Most of these will break a build, a release, or production if ignored. The last - **Do not optimise `_redact_secrets` without running the adversarial tests.** It has had four implementations; the first was a denial of service and the next two leaked credentials. A quadratic regex was a DoS; bounding the quantifiers made it stop matching past the bound; a positional scanner skipped a credential flag that followed a boolean flag (`docker run --rm --password=X`). It is now a token-wise scan where every token is classified independently, which is what makes skipping impossible. Redaction covers the **command string only, never command output**, and multi-word quoted values are only partially redacted — it is a tripwire, and the documented mitigation is to pass credentials via env files or stdin rather than argv. +- **`{REDACTED+ELIDED}` in a log means the audit record is incomplete, and the false positives are deliberate.** Redaction replaces from the credential marker to the end of the whitespace-delimited token — which is what stops a partially quoted secret leaking — but shell separators are not whitespace, so `echo --password=X;reboot` logged as `echo --password={REDACTED}` and the `reboot` was **absent from the record while the command ran** (reproduced against a live host, 0.8.1). Narrowing the replacement to the separator was rejected: a secret containing `;` would then have its tail printed. So the value stays fully hidden and `_placeholder_for` flags the record instead. It errs toward flagging — a secret that merely *contains* a `;` is flagged too — because telling those apart needs shell parsing this module refuses to do. Do not "fix" the noise by narrowing the replacement, and do not add a separator to `_SHELL_SEPARATOR_RE` that cannot survive `_split_ws_preserving` (newline and CR were removed for exactly that reason). + - **Never lower the `asyncssh` log levels — that suppression *is* a credential control.** `server.py::_configure_logging` raises the `asyncssh`, `asyncssh.sftp` and `asyncssh.connection` loggers to WARNING because asyncssh logs the full raw command, credentials included, at INFO via its own channel logger. That is "Production incident 2026-04-11 (round 2)", and `_redact_secrets` did **not** fix it: redaction only sanitizes the ssh-mcp logger, so the password leaked anyway. Turning those loggers back down to INFO to debug an SSH problem reopens the leak. Note also that `_configure_logging()` is called at *import* time and clears root handlers, so anything configuring logging around it will be overwritten. - **`mcp>=2` removed the `mcp.settings` transport fields, and `_build_http_app`'s three keyword arguments have no defaults, deliberately.** v1's `Settings` carried `host`/`port`/`stateless_http`/`transport_security`; v2's does not, so transport config is passed straight to `streamable_http_app()` instead. `_build_transport_security(raw_allowed_hosts, host) -> TransportSecuritySettings` and `_build_http_app(token, *, host, stateless, transport_security)` in `server.py` both require every argument — no `transport_security=None` shortcut. `mcp/server/transport_security.py:48` turns a `None` settings object into `TransportSecuritySettings(enable_dns_rebinding_protection=False)`, so a default would silently fail open on a non-loopback bind. Do not add one to "simplify" a callsite. - **`mcp>=2` logs a tool-failure message at INFO before converting it to a client-visible error.** `mcp/server/mcpserver/server.py:438` does `logger.info("Tool %r failed: %r", params.name, str(exc))` on the `mcp.server.mcpserver.server` logger before turning a `ToolError` into `CallToolResult(is_error=True)`. `upload_file`/`download_file` are the tools that raise, and their messages carry local and remote paths. `_configure_logging` now raises that logger to WARNING for the same reason it already raises the `asyncssh` loggers above — "Production incident 2026-04-11 (round 2)" was a third-party library logging sensitive text at INFO, and this is the same class of leak on a different logger. -- **The dangerous-command list is a tripwire, not a security boundary.** `README.md` and `SECURITY.md` say so explicitly, and base64 / hex-escape / homoglyph / `$(...)` bypasses are acknowledged. Do not describe it as a security control, and do not widen it into a claim it cannot keep. +- **The dangerous-command list is a tripwire, not a security boundary.** `README.md` and `SECURITY.md` say so explicitly, and base64 / hex-escape / homoglyph / `$(...)` bypasses are acknowledged. Do not describe it as a security control, and do not widen it into a claim it cannot keep. Two things in it are load-bearing and easy to break by "simplifying". The host-availability entries added in 0.8.1 (`reboot`, `poweroff`, `init 0`, `systemctl poweroff`, `iptables -F`, `nft flush ruleset`, `userdel`, `passwd -l`) are anchored on `_CMD_START` — string start, a shell separator, or `sudo`/`doas` — because a bare `\breboot\b` blocks `last reboot` and `grep reboot /var/log/messages`, and blocking read-only diagnostics is how a tripwire teaches operators to route around it. And `_is_dangerous_command` searches **two renderings** of the command: control characters become a space (so `rm -rf\n/` still matches, needing the whitespace reading) *and* line breaks become `;` (so a second line reads as a command position, not as an argument — with the space rendering alone, `echo hi\nreboot` was measurably not caught). Neither rendering alone is sufficient; a test pins each. - **`ssh-mcp --version` and `--help` do not exist.** `main()` in `server.py` inspects only `sys.argv[1] == "healthcheck"`; anything else falls through and starts the MCP server on stdin. `CONTRIBUTING.md` no longer tells bug reporters to run it (it now points at `import ssh_mcp; ssh_mcp.__version__` or `pip show blc-ssh-mcp`), but `release.yml` still smoke-tests artifacts with `ssh-mcp --help` — that test exits 0 only because CI closes stdin, so it verifies the entry point imports and nothing more. Fix the flags or fix both callers; do not assume the smoke test is meaningful. diff --git a/CHANGELOG.md b/CHANGELOG.md index fdffd0d..04571a9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,46 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.8.1] - 2026-09-07 + +> **Why 0.8.1 and not 0.9.0:** nothing here changes a configuration that started successfully on 0.8.0 into one that refuses to start, which is the test 0.8.0 itself recorded for taking the minor. The one behaviour an operator could notice is that four more command shapes are now blocked by the dangerous-command tripwire — and that list has always been documented as advisory and subject to widening, `force=true` still bypasses it, and no *configuration* becomes invalid. Everything else is a hang becoming an error, a log line becoming honest, an image gaining an architecture, and prose corrections. + +### Security + +**The audit log can no longer silently omit an executed command.** Redaction replaces from the credential marker to the end of the whitespace-delimited token, which is what makes leaking a partially quoted secret impossible — but shell separators are not whitespace. So `echo --password=X;reboot` was logged as `echo --password={REDACTED}`: the `reboot` was **absent from the audit record while the command still ran on the host**. Reproduced end-to-end against a live SSH host on 2026-09-07, where the elided `touch /tmp/f` created its file. + +Narrowing the replacement to the separator was rejected — a secret containing `;` would then have its tail printed. The value stays fully hidden and the record stops being *silent* instead: text that could chain a second command is now replaced with `{REDACTED+ELIDED}`, so a reader knows the token carried something beyond the secret and the log is not a faithful transcript. The marker cannot say *what* was elided, only that something was, and it deliberately errs toward flagging — a secret that merely *contains* a `;` is flagged too, because telling those apart needs shell parsing this module refuses to do. A noisy marker is recoverable; a silently truncated audit record is not. + +The marker is applied by **every** rule that replaces to the end of a whitespace-delimited run, and not by the two that are bounded on both sides (URL basic-auth, terminated by `@`; the quoted `-p'…'` form, terminated by its closing quote) — those cannot swallow a following command, so flagging them would be a pure false positive. Both halves of that were found by CI's 200-example Hypothesis profile on the first push, not locally at 50: a value was flagged under `--password=` and not under `--token=` because the enumerated-env-var rule matched the second one first, and separately a later rule re-matched an already-redacted flag and *downgraded* `{REDACTED+ELIDED}` back to the plain placeholder. An inconsistent marker is worse than none, since a reader takes its absence as proof nothing was cut, so an existing marker is now sticky. `AGENTS.md` records the process fix: run `HYPOTHESIS_PROFILE=ci` locally before pushing a redaction change. + +**The dangerous-command tripwire now covers host availability, not just filesystem destruction.** End-to-end testing found the priorities inverted: `reboot`, `poweroff`, `init 0`, `systemctl poweroff`, `iptables -F`, `nft flush ruleset`, `userdel` and `passwd -l` all reached the host, while a routine `rm -rf /tmp/build-cache` was blocked. For a tool whose `execute_on_group` fans one command across a fleet, a single `reboot` takes the whole group down, and `iptables -F` on a host reached through those rules is unrecoverable without console access. + +These are anchored on **command position** rather than the bare word, so `last reboot`, `grep reboot /var/log/messages` and `journalctl | grep -i poweroff` still run — blocking read-only diagnostics is how a tripwire teaches operators to route around it. Cost of the anchor, accepted and documented: `sudo -n reboot` is not matched. Still a tripwire and not a security boundary; `$(printf 'reboo\164')` defeats every entry, exactly as documented for the rest of the table. + +Matching now runs over two renderings of the command, because collapsing a newline to a space is right for one class of pattern and wrong for the other: `rm -rf\n/` only matches while the line break reads as whitespace, and a second line `reboot` only matches while it reads as a command position. With the space rendering alone, `echo hi\nreboot` was measurably not caught. + +**Both architectures of the published image are now scanned.** A multi-arch push produces an index, and Trivy resolves an index to the runner's own platform, so the single CRITICAL gate would have covered amd64 and published arm64 uninspected — with the gate still green. `ci.yml` now runs one scan step per platform, and a regression test asserts the set of platforms built equals the set scanned. + +### Added + +**The container image is now `linux/amd64` *and* `linux/arm64`.** Until 0.8.1 it was amd64-only, so `docker run` on Apple Silicon or AWS Graviton failed outright with "no matching manifest" — verified against the published `0.8.0` manifest, whose only entry was `linux/amd64`. Reaching remote infrastructure is this tool's whole job and Graviton is a plausible host for it. arm64 is built under QEMU emulation; the alternative, a second native `ubuntu-24.04-arm` runner plus a manifest-merge job, is faster but splits one digest across two jobs, which the promote/scan/verify chain is built around. The image installs prebuilt manylinux aarch64 wheels rather than compiling, so emulation only pays for `UV_COMPILE_BYTECODE` and `useradd`. + +### Fixed + +**`upload_file` on a non-regular local file no longer hangs.** The local open beneath `transfer_root` now passes `O_NONBLOCK`. `_upload_impl` has always `fstat`-ed the descriptor and refused anything that is not a regular file, but for a **fifo** that refusal was unreachable: a plain `O_RDONLY` open blocks until a writer appears, so the check never ran for exactly the file type it exists to reject. Found by end-to-end testing against a live SSH host on 2026-09-07, not by the unit suite — the existing non-regular test uses a *directory*, which opens fine and therefore does reach the guard. + +The open runs in a worker thread, so the event loop survived; the thread did not. Sixteen concurrent fifo uploads exhausted the default executor (`min(32, cpu+4)`) and every later `asyncio.to_thread` call — all SFTP in the process — stalled until restart, while `execute` kept working. Reaching it requires a non-regular file beneath `transfer_root`, which is `0700` and owner-only, so this is a local availability bug rather than a remote one. This is the same defect class 0.8.0 fixed for `SSH_MCP_HTTP_TOKEN_FILE`; the SFTP path was missed. The flag is referenced bare rather than through `getattr` because SFTP is POSIX-only by design — `paths.py::ensure_root` fails closed without `O_NOFOLLOW`/`O_DIRECTORY` before the open is reached. + +**An unknown group is no longer reported as a group with one server.** Asking for a group that does not exist printed `Executing on group 'no-such-group' (1 servers)...`, which asserts two false things at once: that the group exists, and that it has a member. `execute_on_group` cannot raise (see `ExecResult` in `models.py`), so a registry failure arrives as a single result whose `server` *is* the group name; that shape now renders as the error alone. Single-member group headers are also pluralised correctly. + +**Two real Pyright errors from `_build_http_app`'s lifespan are gone.** `_lifespan` was annotated `-> Any`, which made both `asynccontextmanager` and Starlette's `lifespan` parameter unverifiable; it is now `-> AsyncGenerator[None]`. The three `-> Iterator[…]`/`-> AsyncIterator[…]` annotations under `@contextmanager`/`@asynccontextmanager` are deprecated forms and became `Generator`/`AsyncGenerator`. Fixing the last of them surfaced a genuine narrowing hole: `_transfer_root` yielded the `int | None` attribute rather than a checked local, so a checker could not tell the descriptor was set. It now reads a local, and `server.py`/`ssh.py` capture the OTel status helpers at import time instead of reaching through a name bound only inside the `try`. `ssh.py` reports zero Pyright errors; `server.py` is down from seven to the two unresolved-import artefacts of Pyright not using the project venv, which `mypy` resolves. + +**A symlink at a download destination now says so.** `open_beneath` adds `O_NOFOLLOW`, so `O_CREAT|O_EXCL` reports `EEXIST` for a symlink rather than following it — correct, but the message read as "a regular file is in the way", leaving an operator to wonder why `ls` shows a link pointing somewhere writable. + +### Documentation + +**Corrected the 0.7.0 claim that all four security floors protect `pip install` consumers.** Only two do: `mcp>=2.1.1` and `asyncssh>=2.24.0` are in `[project.dependencies]` and reach the published wheel's `METADATA` (verified against the 0.7.0 wheel's `requires_dist`). `cryptography>=50.0.1` and `click>=8.3.3` are `[tool.uv] constraint-dependencies` — uv-only, absent from `METADATA` by design, and binding this repo's lock rather than a downstream resolve. The placement was right; the sentence describing it was not. + ## [0.8.0] - 2026-09-06 > **Why 0.8.0 and not 0.7.1:** two of the changes below make a configuration that started successfully on 0.7.0 **refuse to start**, which is a breaking change to operator-visible behaviour rather than a bug fix. An empty or whitespace-only `SSH_MCP_HTTP_TOKEN_FILE`, a whitespace-only `SSH_MCP_HTTP_TOKEN`, and a token containing non-ASCII, a control character or an interior space all used to boot; all now abort. A token file with mode `0666` does too. Shipping that as a patch would let `~=0.7.0` pull it in silently and take a working deployment down on its next restart, so it takes the minor — the same reasoning recorded above for 0.6.0 over 0.5.7. The 0.6.1 precedent of a "BREAKING" patch does not apply: that one renamed the PyPI distribution while keeping an `ssh-mcp` alias, the import package and the image, so only the install command changed, never the behaviour of a running server. @@ -45,7 +85,7 @@ The two adopted groups earn their place. `I` is deterministic and fully autofixa ### Security -Cleared all four Dependabot alerts open against `uv.lock`, moving to the *latest stable* of each affected package rather than the minimum patched version Dependabot proposed, so one lock refresh supersedes three separate open PRs (#53, #54, #55) instead of leaving them to land piecemeal. **None of the four is exploitable in ssh-mcp today** — the floors exist to protect `pip install blc-ssh-mcp` consumers through the wheel's published METADATA, since `uv.lock` itself carries no guarantee for anyone installing from PyPI. +Cleared all four Dependabot alerts open against `uv.lock`, moving to the *latest stable* of each affected package rather than the minimum patched version Dependabot proposed, so one lock refresh supersedes three separate open PRs (#53, #54, #55) instead of leaving them to land piecemeal. **None of the four is exploitable in ssh-mcp today** — the floors exist because `uv.lock` carries no guarantee for anyone installing from PyPI. How far a floor actually reaches depends on where it is declared, and only two of these four reach a `pip install` consumer: `mcp>=2.1.1` and `asyncssh>=2.24.0` sit in `[project.dependencies]` and so appear in the published wheel's `METADATA` (verified against the 0.7.0 wheel's `requires_dist`). `cryptography>=50.0.1` and `click>=8.3.3` are `[tool.uv] constraint-dependencies`, which are uv-only and absent from `METADATA` by design — declaring them as project dependencies would assert import edges ssh-mcp does not have. They bind this repo's own lock, not a downstream resolve. - **CVE-2026-54591** (`GHSA-2wxc-x7rj-hg8f`, asyncssh, SCP-client path traversal to arbitrary file write via a server-supplied filename) — **not reachable**: ssh-mcp uses SFTP exclusively and never calls `asyncssh.scp`, and it is an SSH client, never a server. - **CVE-2026-54590** (`GHSA-qr67-gv47-xwwh`, asyncssh, `AuthorizedKeysFile %u` escape) — **not reachable**: this is server-side behavior and ssh-mcp is a client only. diff --git a/README.md b/README.md index 3c2a8e8..0657a34 100644 --- a/README.md +++ b/README.md @@ -44,7 +44,7 @@ Requires Python 3.11+. Install [uv](https://docs.astral.sh/uv/getting-started/in #### Docker -A prebuilt image is published to GitHub Container Registry: +A prebuilt image is published to GitHub Container Registry for **`linux/amd64` and `linux/arm64`** (0.8.1 and newer — 0.8.0 and earlier are amd64-only and will not run on Apple Silicon or AWS Graviton): ```bash docker pull ghcr.io/blackaxgit/ssh-mcp:latest @@ -387,10 +387,11 @@ Command strings longer than `max_command_bytes` (default 65536 encoded UTF-8 byt - `chmod 777 /` — flag before *or* after the mode (`chmod -R 777 /`, `chmod 777 -R /`) - Fork bombs (spaced and adjacent variants) - Payload-execution wrappers: `base64 -d | bash|sh|zsh|python|perl|ruby`, `eval "…"`, `python|python3|perl|ruby -c`, `bash -c` +- Host-availability destruction, matched only in **command position** (string start, after a shell separator, or after `sudo`/`doas`): `reboot`, `poweroff`, `halt`, `shutdown`, `init 0|6`, `telinit 0|6`, `systemctl reboot|poweroff|halt|kexec`, `iptables|ip6tables -F|--flush`, `nft flush ruleset`, `userdel`, `passwd -l` -> **Note the last bullet — it catches ordinary commands too.** `bash -c '…'`, `python3 -c '…'` and `eval …` are blocked by default even when entirely benign. Wrap them differently (a script file, a heredoc) or pass `force=true` for an audited call. +> **Note the payload-execution bullet — it catches ordinary commands too.** `bash -c '…'`, `python3 -c '…'` and `eval …` are blocked by default even when entirely benign. Wrap them differently (a script file, a heredoc) or pass `force=true` for an audited call. The host-availability bullet is anchored the other way on purpose, so read-only diagnostics like `last reboot` or `grep reboot /var/log/messages` are *not* blocked — the cost of that anchor is that `sudo -n reboot` slips past it. -ASCII control characters (null bytes, newlines, `\x01..\x1f`, `\x7f`) are normalized to spaces before matching, so `rm\x00-rf /` is caught just like `rm -rf /`. The regex is fuzz-tested with Hypothesis on every CI run. +ASCII control characters (null bytes, `\x01..\x1f`, `\x7f`) are normalized to spaces before matching, so `rm\x00-rf /` is caught just like `rm -rf /`. Line breaks are matched **both** ways — as whitespace and as the statement separator they are — because `rm -rf\n/` needs the first reading and a second line `reboot` needs the second, where it sits in command position rather than reading as an argument to line one. The regexes are fuzz-tested with Hypothesis on every CI run. > **This is a TRIPWIRE, not a security boundary.** The regex catches obvious accidents and shortcut destructive commands. It does NOT defend against a motivated attacker: > - The base64/`eval`/`-c` wrappers above are matched *literally*; any rewrite the regex does not spell out (a different decoder, a temp file, `sh <<'EOF'`) gets through @@ -406,6 +407,8 @@ ASCII control characters (null bytes, newlines, `\x01..\x1f`, `\x7f`) are normal > **Known limitation: command OUTPUT is NOT redacted.** If you run `cat /etc/mysql/my.cnf`, `env | grep PASSWORD`, or `kubectl get secret X -o yaml`, the stdout/stderr returned to the MCP client will contain plaintext secrets. The redaction pipeline only filters the COMMAND string (what you asked to run), not the OUTPUT (what it printed). Avoid running commands that print secrets via ssh-mcp — pass credentials through env vars, Docker/K8s secrets, or dedicated config files instead. +> **`{REDACTED+ELIDED}` means more than a secret was cut.** Redaction replaces from the credential marker to the end of the whitespace-delimited token, so a shell separator inside that token is consumed with it — before 0.8.1, `echo --password=X;reboot` was logged as `echo --password={REDACTED}` and the chained command did not appear in the record at all, though it ran. The secret is still never partially printed; the placeholder changes instead, so the log does not read as a faithful transcript. It errs toward flagging, so a password that merely *contains* `;`, `|`, `&`, a backtick or `$(` is flagged too. + **Local path confinement (new in 0.6.0; versions ≤ 0.5.6 are affected by the flaw it fixes — see [CHANGELOG.md](https://github.com/blackaxgit/ssh-mcp/blob/main/CHANGELOG.md)).** SFTP `upload_file` and `download_file` no longer accept arbitrary absolute local paths. Every local path is **relative to a configured transfer root** and is resolved one component at a time beneath it, refusing a symbolic link at *any* component: ```toml diff --git a/SECURITY.md b/SECURITY.md index 5750ea6..b344b2a 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -65,7 +65,7 @@ The `execute` and `execute_on_group` tools run arbitrary shell commands on remot ### Dangerous Command Detection -ssh-mcp **blocks** commands matching a list of commonly destructive patterns (e.g. `rm -rf /`, `mkfs`, disk wipes) and returns an error instead of executing them. A caller may bypass the block by passing `force=true`. This is a safety feature, not a vulnerability. +ssh-mcp **blocks** commands matching a list of commonly destructive patterns and returns an error instead of executing them. Two kinds are covered: destruction of *data* (`rm -rf /`, `mkfs`, disk wipes, `chmod 777 /`) and, since 0.8.1, destruction of *availability* — `reboot`, `poweroff`, `init 0`, `systemctl poweroff`, `iptables -F`, `nft flush ruleset`, `userdel`, `passwd -l`. The second group matters because `execute_on_group` fans one command across a fleet, so a single `reboot` takes the whole group down and an `iptables -F` on a host reached through those rules needs console access to recover. These are matched only in *command position* — after the string start, a shell separator, or `sudo`/`doas` — so read-only diagnostics such as `last reboot` and `grep reboot /var/log/messages` still run. A caller may bypass the block by passing `force=true`. This is a safety feature, not a vulnerability. **The bypass is not recorded in the audit log.** Audit records carry `server`, `command`, `exit_code` and `duration_ms` only; `force` is emitted solely as an OpenTelemetry span attribute (`ssh.force`), and therefore only when an OpenTelemetry SDK and exporter are configured. The tracing API itself now ships as a required dependency of the MCP SDK, so spans are always created — but with no SDK installed they are no-ops that record nothing. A block is logged as a warning on the operational logger, not the audit logger. If you need a paper trail for bypasses, export traces or withhold `force=true` at the MCP client. @@ -116,6 +116,8 @@ Known credential patterns in the **command string** (`-p`, `--password=`, This is a tripwire too, with two documented limits: multi-word **quoted** values are only partially redacted, and command **output** is never redacted at all — `env | grep PASSWORD` returns plaintext to the MCP client by design. Pass credentials via env files, secret stores, or stdin rather than argv. +**`{REDACTED+ELIDED}` means the record is incomplete.** Redaction replaces from the credential marker to the end of the whitespace-delimited token, so a shell separator inside that token — `--password=X;reboot` — is consumed with it, and before 0.8.1 the log read `--password={REDACTED}` while the chained command still ran. The value is still never partially printed; instead the placeholder changes so the record does not read as a faithful transcript. It errs toward flagging: a secret that merely *contains* `;`, `|`, `&`, a backtick or `$(` is flagged too, because distinguishing those requires shell parsing that this redactor deliberately does not attempt. Treat the marker as "something beyond the secret was cut here", never as proof a command was hidden — and never as proof one was not. + ### Resource Limits Several bounds exist for denial-of-service reasons and should not be raised casually: diff --git a/src/ssh_mcp/__init__.py b/src/ssh_mcp/__init__.py index cdc987a..ab68aae 100644 --- a/src/ssh_mcp/__init__.py +++ b/src/ssh_mcp/__init__.py @@ -1,3 +1,3 @@ """SSH MCP Server - Manage infrastructure via MCP clients like Claude Code.""" -__version__ = "0.8.0" +__version__ = "0.8.1" diff --git a/src/ssh_mcp/formatting.py b/src/ssh_mcp/formatting.py index 5a99cde..2e3a1de 100644 --- a/src/ssh_mcp/formatting.py +++ b/src/ssh_mcp/formatting.py @@ -228,8 +228,18 @@ def format_group_results(results: list[ExecResult], group_name: str) -> str: f"Executing on group '{group_name}' (0 servers)...\n\nNo servers in group." ) + # A registry-level failure is not a one-server group. `execute_on_group` + # cannot raise (see ExecResult in models.py), so an unknown group comes + # back as a single result whose `server` IS the group name — and framing + # that as "Executing on group 'x' (1 servers)..." states two false things + # at once: that the group exists, and that it has a member. Observed + # 2026-09-07 during end-to-end testing. + if len(results) == 1 and results[0].server == group_name and results[0].error: + return f"ERROR: {results[0].error}" + + plural = "server" if len(results) == 1 else "servers" lines = [ - f"Executing on group '{group_name}' ({len(results)} servers)...", + f"Executing on group '{group_name}' ({len(results)} {plural})...", "", ] diff --git a/src/ssh_mcp/server.py b/src/ssh_mcp/server.py index bda27d0..ca4902a 100644 --- a/src/ssh_mcp/server.py +++ b/src/ssh_mcp/server.py @@ -25,7 +25,7 @@ import stat import sys import traceback -from collections.abc import Awaitable, Callable, Iterator +from collections.abc import AsyncGenerator, Awaitable, Callable, Generator from pathlib import Path from typing import Any, TypeVar, cast @@ -68,12 +68,23 @@ from opentelemetry import trace as _otel_trace _tracer: Any = _otel_trace.get_tracer("ssh_mcp") + # Captured here rather than reached through `_otel_trace` in the error + # path below. The module name is only bound inside this `try`, and its + # use down there is guarded by `_tracer is not None` — a correlation no + # type checker can see, so it reported the name possibly-unbound on a + # branch that cannot execute. Binding the two symbols in BOTH branches + # removes the report without a suppression, and rebinding + # `_otel_trace` itself is not an option: that shadows an imported name. + _otel_status: Any = _otel_trace.Status + _otel_error_code: Any = _otel_trace.StatusCode.ERROR except ImportError: # pragma: no cover - unreachable; see comment above _tracer = None + _otel_status = None + _otel_error_code = None @contextlib.contextmanager -def _span(name: str, **attributes: Any) -> Iterator[Any]: +def _span(name: str, **attributes: Any) -> Generator[Any]: """Start an OTel span if the API is available, else a no-op. Usage:: @@ -97,7 +108,7 @@ def _span(name: str, **attributes: Any) -> Iterator[Any]: yield span except Exception as e: span.record_exception(e) - span.set_status(_otel_trace.Status(_otel_trace.StatusCode.ERROR)) + span.set_status(_otel_status(_otel_error_code)) raise @@ -752,7 +763,7 @@ def _build_http_app( ) from exc @asynccontextmanager - async def _lifespan(_app: Starlette) -> Any: + async def _lifespan(_app: Starlette) -> AsyncGenerator[None]: # Step 1: start the MCPServer session manager's task group via the # public session_manager.run() API (see note above). async with session_manager.run(): diff --git a/src/ssh_mcp/ssh.py b/src/ssh_mcp/ssh.py index 5f40943..9fc6275 100644 --- a/src/ssh_mcp/ssh.py +++ b/src/ssh_mcp/ssh.py @@ -24,7 +24,7 @@ import stat import time import uuid -from collections.abc import AsyncIterator, Callable +from collections.abc import AsyncGenerator, Callable from typing import Any, TypeVar import asyncssh @@ -48,8 +48,16 @@ from opentelemetry import trace as _otel_trace _ssh_tracer: Any = _otel_trace.get_tracer("ssh_mcp.ssh") + # Bound in both branches, same reason as server.py: the error paths + # below are guarded by `_ssh_tracer is not None`, which no checker can + # correlate with the import, and rebinding `_otel_trace` would shadow + # an imported name. + _otel_status: Any = _otel_trace.Status + _otel_error_code: Any = _otel_trace.StatusCode.ERROR except ImportError: # pragma: no cover - exercised by env without extras _ssh_tracer = None + _otel_status = None + _otel_error_code = None # --------------------------------------------------------------------------- # Tunable constants — promoted from inline literals for discoverability. @@ -154,6 +162,65 @@ def _safe_exc(e: BaseException) -> str: _REDACTION_PLACEHOLDER: str = "{REDACTED}" +# Distinct placeholder for the case where redaction consumed MORE than a +# credential value. +# +# Redaction replaces from the credential marker to the end of the +# whitespace-delimited token, which is what makes leaking a partially +# quoted secret impossible. Shell separators are not whitespace, so +# ``--password=X;touch /tmp/f`` redacted to the plain placeholder reads +# ``--password={REDACTED} /tmp/f`` — the ``touch`` verb is gone from the +# audit record, and ``--password=X;reboot`` loses ``reboot`` entirely +# while the command still RUNS. Reproduced end-to-end against a live host +# on 2026-09-07: the elided ``touch`` created its file. +# +# Narrowing the replacement to the separator was rejected: a secret +# containing ``;`` would then have its tail printed. So the value stays +# fully hidden and the RECORD stops being silent instead — an auditor +# reading ``{REDACTED+ELIDED}`` knows the token carried something beyond +# the secret and that the log is not a faithful transcript. This is a +# tripwire like the rest of this module: it cannot say WHAT was elided, +# only that something was. +_REDACTION_ELIDED_PLACEHOLDER: str = "{REDACTED+ELIDED}" + +# Shell separators that can chain a second command onto a credential +# token. ``$(`` is listed explicitly because ``(`` alone is common in +# legitimate quoted values. Newline and CR are deliberately absent: both +# callers operate on tokens produced by ``_split_ws_preserving``, so +# whitespace has already split the text and neither can appear inside a +# token being elided — listing them would be an unreachable branch in a +# security-relevant predicate. +_SHELL_SEPARATOR_RE = re.compile(r"[;|&`]|\$\(") + + +def _placeholder_for(elided: str) -> str: + """Pick the placeholder for text about to be replaced wholesale. + + The pipeline runs several rules over the SAME string, so a later rule + can be handed text an earlier one already redacted. Found by CI's + 200-example Hypothesis profile 2026-09-07: rule 3a flagged + ``--…-token=x&`` correctly and then ``_redact_long_flags`` re-matched + the same flag and replaced ``{REDACTED+ELIDED}`` — which holds no + separator — with the plain placeholder, silently DOWNGRADING the + record. A marker that a later rule can erase is worse than no marker, + so an existing one is preserved. The plain placeholder is not sticky: + it carries no claim to lose. + + Args: + elided: The exact text redaction is discarding. + + Returns: + ``{REDACTED+ELIDED}`` if that text could chain another command, or + already records that something was elided; otherwise the plain + ``{REDACTED}``. + """ + if _REDACTION_ELIDED_PLACEHOLDER in elided: + return _REDACTION_ELIDED_PLACEHOLDER + if _SHELL_SEPARATOR_RE.search(elided): + return _REDACTION_ELIDED_PLACEHOLDER + return _REDACTION_PLACEHOLDER + + # Environment variables that are known credential sinks. Matched # case-insensitively as ``=`` substrings. _SECRET_ENV_NAMES: tuple[str, ...] = ( @@ -415,7 +482,7 @@ def _redact_credential_in_token(token: str) -> str: for match in _LONG_FLAG_EQ_RE.finditer(token): name = match.group()[2:-1] # strip leading "--" and trailing "=" if _long_flag_is_credential(name): - return token[: match.end()] + _REDACTION_PLACEHOLDER + return token[: match.end()] + _placeholder_for(token[match.end() :]) return token @@ -491,7 +558,7 @@ def _redact_long_flags(text: str) -> str: # redacted; only a genuine "--"-prefixed long flag is # treated as this flag being boolean/valueless. if j < n and not chunks[j][1] and not chunks[j][0].startswith("--"): - out[j] = _REDACTION_PLACEHOLDER + out[j] = _placeholder_for(chunks[j][0]) i = j + 1 continue i += 1 @@ -523,6 +590,19 @@ def _regex_step(pattern: re.Pattern[str], repl: Any) -> Callable[[str], str]: return [ # 1. Basic auth credentials embedded in a URL: # ``scheme://user:password@host``. See _redact_url_basic_auth. + # + # Keeps the PLAIN placeholder: the replacement is bounded on both + # sides (``:`` … ``@``), so nothing beyond the secret is consumed + # and there is nothing to flag. Same for rule 4 below, whose value + # is delimited by its closing quote. Every OTHER rule here + # replaces to the end of a whitespace-delimited run and therefore + # goes through ``_placeholder_for`` — a rule that swallows + # ``;reboot`` must say so. Reported by CI's 200-example Hypothesis + # profile 2026-09-07: `--…-password=x&` was flagged while + # `--…-token=x&` was not, because rule 3a's enumerated env names + # matched the second one first. An inconsistent marker is worse + # than none, since a reader would take its absence as proof that + # nothing was cut. _redact_url_basic_auth, # 2. HTTP ``Authorization:`` header with Bearer/Basic/Digest/Token. _regex_step( @@ -530,7 +610,7 @@ def _regex_step(pattern: re.Pattern[str], repl: Any) -> Callable[[str], str]: r"(Authorization:\s*(?:Bearer|Basic|Digest|Token)\s+)(\S+)", re.IGNORECASE, ), - lambda m: f"{m.group(1)}{_REDACTION_PLACEHOLDER}", + lambda m: f"{m.group(1)}{_placeholder_for(m.group(2))}", ), # 3a. Known credential env vars (enumerated list, exact match). _regex_step( @@ -538,7 +618,7 @@ def _regex_step(pattern: re.Pattern[str], repl: Any) -> Callable[[str], str]: r"\b(" + env_alt + r")=(\S+)", re.IGNORECASE, ), - lambda m: f"{m.group(1)}={_REDACTION_PLACEHOLDER}", + lambda m: f"{m.group(1)}={_placeholder_for(m.group(2))}", ), # 3b. (v0.4.3 G2) Generic env var SUFFIX patterns: # ``*_PASSWORD=``, ``*_SECRET=``, ``*_TOKEN=``, ``*_KEY=``, @@ -554,9 +634,10 @@ def _regex_step(pattern: re.Pattern[str], repl: Any) -> Callable[[str], str]: r"\b(\w+(?:_PASSWORD|_SECRET|_TOKEN|_KEY|_CREDENTIAL|_PWD))=(\S+)", re.IGNORECASE, ), - lambda m: f"{m.group(1)}={_REDACTION_PLACEHOLDER}", + lambda m: f"{m.group(1)}={_placeholder_for(m.group(2))}", ), - # 4. MySQL/MariaDB short password flag QUOTED form. + # 4. MySQL/MariaDB short password flag QUOTED form. Plain + # placeholder: the closing quote bounds the replacement. _regex_step( re.compile(r"(? Callable[[str], str]: # 5. MySQL/MariaDB short password flag UNQUOTED form (≥3 chars). _regex_step( re.compile(r"(? Callable[[str], str]: # disambiguate from the MySQL rule. _regex_step( re.compile(r"(sshpass\s+-p\s+)(\S+)", re.IGNORECASE), - lambda m: f"{m.group(1)}{_REDACTION_PLACEHOLDER}", + lambda m: f"{m.group(1)}{_placeholder_for(m.group(2))}", ), # 8/9. Long flags with ``=`` or whitespace separator. See # _redact_long_flags — merges the old rules 8 and 9 into one @@ -632,6 +713,22 @@ def _redact_secrets(value: Any) -> Any: # fork-bomb pattern below matches no letters, so it needs no flag). The # rm-flag patterns use lookaheads instead of ordered character classes so # `-rfv`, `-vfr`, `-rfvi` (any order with extra flags) all match. + +# Prefix asserting a word sits where a COMMAND goes, not where an +# argument does: string start, after a shell separator (``;``, ``&&``, +# ``|``, newline), or after a privilege wrapper. Used by the +# host-availability entries below, whose verbs — unlike ``mkfs`` or +# ``dd if=`` — are ordinary English words that appear constantly as +# arguments to harmless commands. +# +# Deliberately not a shell parser. It cannot see through quoting, +# ``xargs``, or a backslash-newline continuation, and it is not meant to; +# see the tripwire caveat above. +# Newline is absent because it never reaches a pattern: both renderings +# in ``_is_dangerous_command`` rewrite line breaks first — to a space, or +# to the ``;`` this class already covers. +_CMD_START = r"(?:^|[;&|]\s*|\b(?:sudo|doas)\s+)" + _DANGEROUS_PATTERNS = [ # Filesystem root wipe via rm -rf — catches `/`, `~`, `$HOME`, `$USER` # forms. Flag cluster must contain BOTH `r` and `f` anywhere, plus @@ -706,6 +803,38 @@ def _redact_secrets(value: Any) -> Any: re.compile(r"\beval\s+[\"'\$\(]", re.IGNORECASE), re.compile(r"\b(python|python3|perl|ruby)\s+-(c|e)\s+", re.IGNORECASE), re.compile(r"\bbash\s+-c\s+", re.IGNORECASE), + # Host availability. Added 2026-09-07 after end-to-end testing against + # a live host showed this table covered filesystem and device + # destruction thoroughly and host availability not at all: `reboot`, + # `poweroff`, `init 0`, `iptables -F` and `userdel -r` all reached the + # host, while a routine `rm -rf /tmp/build-cache` was blocked. That is + # an inverted priority for a tool whose `execute_on_group` fans one + # command across a fleet — a single `reboot` takes the whole group + # down, and `iptables -F` on a host reached through those rules is + # unrecoverable without out-of-band console access. + # + # Anchored on COMMAND POSITION (``_CMD_START``), not on the bare word. + # A bare `\breboot\b` would block `last reboot`, `grep reboot + # /var/log/messages` and `journalctl | grep -i poweroff` — common + # read-only diagnostics, and blocking those is how a tripwire trains + # operators to route around it. Cost of the anchor: `sudo -n reboot` + # is not matched, because the flag breaks the prefix. Accepted — it is + # a tripwire, and the same class of gap is already documented for the + # rest of the table (`$(printf 'reboo\\164')` defeats every entry). + re.compile(_CMD_START + r"(?:reboot|poweroff|halt|shutdown)\b", re.IGNORECASE), + re.compile(_CMD_START + r"(?:tel)?init\s+[06]\b", re.IGNORECASE), + re.compile( + _CMD_START + r"systemctl\s+(?:\S+\s+)*?(?:reboot|poweroff|halt|kexec)\b", + re.IGNORECASE, + ), + # Firewall flush — severs access to the host it runs on. Case-sensitive + # on purpose: `-F` flushes, `-f` does not exist for iptables, and + # IGNORECASE would conflate them. + re.compile(_CMD_START + r"(?:ip6?tables)\b(?:\s+\S+)*?\s+(?:-F|--flush)\b"), + re.compile(_CMD_START + r"nft\s+flush\s+ruleset\b", re.IGNORECASE), + # Account destruction — locks the operator out of the host. + re.compile(_CMD_START + r"userdel\b", re.IGNORECASE), + re.compile(_CMD_START + r"passwd\s+(?:-l|--lock)\b", re.IGNORECASE), ] # Sensitive paths that should be blocked in SFTP operations. @@ -838,6 +967,14 @@ def _is_dangerous_command(command: str) -> bool: remain a known bypass, per the tripwire caveat above ``_DANGEROUS_PATTERNS``. + Matching runs over TWO renderings of the command, because collapsing a + newline to a space is right for one class of pattern and wrong for the + other. ``rm -rf\\n/`` only matches while the newline reads as + whitespace; a second line ``reboot`` only matches while it reads as a + command position rather than as an argument to the first line. Added + 2026-09-07 with the host-availability entries: with the space + rendering alone, ``echo hi\\nreboot`` was measurably not caught. + Args: command: Command string to check @@ -848,9 +985,15 @@ def _is_dangerous_command(command: str) -> bool: # space before matching. Deletion would collapse adjacent tokens (e.g. # "rm\x00-rf" → "rm-rf") and miss the pattern; replacement preserves # token boundaries while removing the bypass character. - sanitized = re.sub(r"[\x00-\x1f\x7f]", " ", command) + as_whitespace = re.sub(r"[\x00-\x1f\x7f]", " ", command) + # Same sanitization, but line breaks become the shell separator they + # actually are, so ``_CMD_START`` still sees a command position. + as_statements = re.sub(r"[\x00-\x08\x0b-\x1f\x7f]", " ", command) + as_statements = re.sub(r"[\n\r]+", ";", as_statements) for pattern in _DANGEROUS_PATTERNS: - if pattern.search(sanitized): + if pattern.search(as_whitespace) or ( + as_statements != as_whitespace and pattern.search(as_statements) + ): return True return False @@ -1246,7 +1389,7 @@ def __init__(self, registry: ServerRegistry, settings: Settings) -> None: # Eviction loop starts on first connection (deferred if no event loop) @contextlib.asynccontextmanager - async def _transfer_root(self) -> AsyncIterator[int]: + async def _transfer_root(self) -> AsyncGenerator[int]: """Yield the pinned transfer-root fd, safe against concurrent shutdown. See the ``__init__`` comment (Defect 5) for the races this closes. @@ -1313,9 +1456,20 @@ async def _transfer_root(self) -> AsyncIterator[int]: leaked_fd = await _await_shielded_until_done(ensure_task) os.close(leaked_fd) raise + fd = self._transfer_root_fd + if fd is None: # pragma: no cover - the block above just set it + raise RuntimeError( + "transfer root fd is unset after setup — this should be " + "unreachable; refusing to yield an invalid descriptor" + ) self._transfer_root_refcount += 1 try: - yield self._transfer_root_fd + # The local, not the attribute: `close_all()` may clear the + # attribute while this transfer holds a refcount, and the fd is + # kept open until the refcount drops. Reading the attribute here + # would also reintroduce an `int | None` that no checker can + # narrow across the lock re-entry. + yield fd finally: async with self._transfer_root_cond: self._transfer_root_refcount -= 1 @@ -1382,7 +1536,7 @@ async def execute( # Truncate error messages into spans — some operators ingest # traces into cost-sensitive backends. span.set_attribute("ssh.error", _redact_secrets(result.error[:200])) - span.set_status(_otel_trace.Status(_otel_trace.StatusCode.ERROR)) + span.set_status(_otel_status(_otel_error_code)) return result async def _execute_impl( @@ -1979,7 +2133,7 @@ async def upload(self, server_name: str, local_path: str, remote_path: str) -> s return await self._upload_impl(server_name, local_path, remote_path) except Exception as e: span.set_attribute("ssh.error_type", type(e).__name__) - span.set_status(_otel_trace.Status(_otel_trace.StatusCode.ERROR)) + span.set_status(_otel_status(_otel_error_code)) raise async def _upload_impl( @@ -2041,8 +2195,29 @@ async def _upload_impl( # open_beneath does blocking syscalls (openat per # component) — off the event loop thread, same as the # read loop below. + # + # O_NONBLOCK is load-bearing, not defensive: without it an + # O_RDONLY open of a fifo beneath transfer_root blocks + # forever waiting for a writer, so the S_ISREG refusal + # immediately below is UNREACHABLE for exactly the file + # type it exists to reject. The open runs in a worker + # thread, so the event loop survives — but the thread does + # not: measured 2026-09-07, 16 concurrent fifo uploads + # exhaust the default executor (min(32, cpu+4)) and every + # later to_thread call, i.e. ALL SFTP in the process, + # stalls until restart while `execute` keeps working. + # This is the same defect 0.8.0 fixed for + # SSH_MCP_HTTP_TOKEN_FILE (server.py::_read_token_file) and + # missed here. Referenced bare rather than via getattr + # because this subsystem is POSIX-only by design: + # _transfer_root() awaits paths.py::ensure_root first, + # which fails closed without O_NOFOLLOW/O_DIRECTORY, so + # this line is unreachable on a platform lacking the + # constant. O_NONBLOCK is a no-op on a regular file and is + # NOT a liveness guarantee — a regular file on a hostile + # FUSE mount can still block the read. local_fd = await asyncio.to_thread( - open_beneath, root_fd, local_path, os.O_RDONLY + open_beneath, root_fd, local_path, os.O_RDONLY | os.O_NONBLOCK ) # fstat the descriptor we are ABOUT TO READ, not a path — @@ -2193,7 +2368,7 @@ async def download( return await self._download_impl(server_name, remote_path, local_path) except Exception as e: span.set_attribute("ssh.error_type", type(e).__name__) - span.set_status(_otel_trace.Status(_otel_trace.StatusCode.ERROR)) + span.set_status(_otel_status(_otel_error_code)) raise async def _download_impl( @@ -2275,11 +2450,20 @@ async def _download_impl( os.O_WRONLY | os.O_CREAT | os.O_EXCL, ) except FileExistsError as e: + # A SYMLINK at this name lands here too, and that is + # worth naming: `open_beneath` adds O_NOFOLLOW, so + # `O_CREAT|O_EXCL` on a symlink reports EEXIST rather + # than following it. Without the second sentence the + # message reads as "a regular file is in the way", + # and an operator can spend a while wondering why + # `ls` shows a link pointing somewhere writable. raise ValueError( f"local destination already exists: {local_path!r} " "under transfer_root. Downloads never overwrite an " "existing file — remove or rename it first, or " - "choose a different destination name." + "choose a different destination name. A symlink at " + "this name reports the same error and is refused " + "rather than followed, whatever it points at." ) from e local_file_created = True # Capture identity while local_fd is still ours (before diff --git a/tests/test_ci_lint_determinism.py b/tests/test_ci_lint_determinism.py index 3bd04e3..2e3ceec 100644 --- a/tests/test_ci_lint_determinism.py +++ b/tests/test_ci_lint_determinism.py @@ -369,11 +369,14 @@ def test_digest_verification_accounts_for_index_wrapping() -> None: workflow = _load_workflow(CI_WORKFLOW_PATH) docker_job = workflow["jobs"]["docker"] + # Matched on what the step DOES, not on its exact title: the name has + # already been reworded once (0.8.1, "scanned digest" -> "scanned image + # manifests" when the build went multi-arch), and a title-only matcher + # turns this guard into a silent no-op the moment someone renames it. verify_steps = [ step for step in docker_job.get("steps", []) - if "serves only the scanned digest" in step.get("name", "") - or "resolves to the scanned digest" in step.get("name", "") + if "imagetools inspect" in step.get("run", "") ] assert verify_steps, ( "the docker job no longer verifies that published tags serve the " @@ -411,6 +414,60 @@ def test_digest_verification_accounts_for_index_wrapping() -> None: ) +def test_published_image_is_multi_arch_and_every_platform_is_scanned() -> None: + """0.8.1: the published image must cover arm64, and the CRITICAL gate + must cover every platform it publishes. + + Until 0.8.1 the image was amd64-only, so ``docker run`` on Apple Silicon + or AWS Graviton failed outright with "no matching manifest" — verified + against the published ``0.8.0`` manifest, whose only entry was + ``linux/amd64``. Reaching remote infrastructure is this tool's whole job, + and Graviton is a plausible host for it. + + The second half is the part that could regress silently. A multi-arch + push produces an INDEX, and Trivy resolves an index to the runner's own + platform, so ONE scan step gates amd64 and lets arm64 through + unscanned — with the gate still green. So the platform set built must + equal the platform set scanned. + """ + docker_job = _load_workflow(CI_WORKFLOW_PATH)["jobs"]["docker"] + steps = docker_job.get("steps", []) + + publish_builds = [ + step + for step in steps + if "build-push-action" in step.get("uses", "") + and "push=true" in str(step.get("with", {}).get("outputs", "")) + ] + assert len(publish_builds) == 1, ( + f"expected exactly one publish-path build step, found {len(publish_builds)}" + ) + + built = { + p.strip() + for p in str(publish_builds[0]["with"]["platforms"]).split(",") + if p.strip() + } + assert built == {"linux/amd64", "linux/arm64"}, ( + f"the publish build declares platforms {sorted(built)}; dropping one " + "makes the image unrunnable on that architecture" + ) + + scanned = { + str(step.get("env", {}).get("TRIVY_PLATFORM", "")).strip() + for step in steps + if "trivy-action" in step.get("uses", "") + and "@${{ steps.build.outputs.digest }}" + in str(step.get("with", {}).get("image-ref", "")) + } + assert scanned == built, ( + f"platforms built {sorted(built)} but scanned {sorted(scanned)}. Trivy " + "resolves a multi-arch index to the runner's own platform, so an " + "unscanned platform is published with a green CRITICAL gate. Add a " + "scan step with the matching TRIVY_PLATFORM." + ) + + def test_release_workflow_exists_and_is_tag_triggered() -> None: """S6: a release workflow must exist and be triggered by version tags.""" assert RELEASE_WORKFLOW_PATH.exists(), ( diff --git a/tests/test_formatting.py b/tests/test_formatting.py index eaefa6c..3fc7de3 100644 --- a/tests/test_formatting.py +++ b/tests/test_formatting.py @@ -424,3 +424,74 @@ def test_format_group_results_none_exit_code_no_error_counted_as_failed( output = format_group_results(results, "test") assert "Summary: 0 succeeded, 1 failed" in output assert "exit unknown" in output + + def test_registry_failure_is_not_framed_as_a_one_server_group(self) -> None: + """An unknown group must not be reported as a group with one member. + + Observed 2026-09-07 during end-to-end testing: asking for a group + that does not exist printed ``Executing on group 'no-such-group' + (1 servers)...``, which asserts two false things — that the group + exists, and that it has a member. ``execute_on_group`` cannot raise + (see ExecResult in models.py), so a registry failure arrives as a + single result whose ``server`` IS the group name. + """ + results = [ + ExecResult( + server="no-such-group", + command="hostname", + stdout="", + stderr="", + exit_code=None, + error="Group not found: 'no-such-group'", + ), + ] + output = format_group_results(results, "no-such-group") + assert "Executing on group" not in output, ( + "a nonexistent group must not get a group-run header" + ) + assert "servers" not in output, "no server count may be claimed" + assert "Group not found" in output, "the actual reason must survive" + + def test_a_real_single_host_group_still_gets_its_header(self) -> None: + """Guards the test above from over-reaching. + + A genuine one-member group differs from a registry failure only in + that its result names a SERVER, not the group. Suppressing the + header for both would hide a real run. + """ + results = [ + ExecResult( + server="web1", + command="hostname", + stdout="web1", + stderr="", + exit_code=0, + duration_ms=12, + ), + ] + output = format_group_results(results, "solo") + assert "Executing on group 'solo' (1 server)..." in output, ( + "a real single-host group needs its header, correctly pluralised" + ) + assert "1 servers" not in output + + def test_a_failed_host_named_like_its_group_still_runs(self) -> None: + """The registry-failure shape needs all three conditions. + + A host that happens to share its group's name and failed is a real + run, and must keep the header. Pins that the check is not just + "one result with an error". + """ + results = [ + ExecResult( + server="web1", + command="hostname", + stdout="", + stderr="", + exit_code=None, + error="SSH error: connection refused", + ), + ] + output = format_group_results(results, "web") + assert "Executing on group 'web' (1 server)..." in output + assert "Summary: 0 succeeded, 1 failed" in output diff --git a/tests/test_sftp_confinement.py b/tests/test_sftp_confinement.py index cf77507..d087c3f 100644 --- a/tests/test_sftp_confinement.py +++ b/tests/test_sftp_confinement.py @@ -23,6 +23,8 @@ from __future__ import annotations import os +import signal +import stat import tempfile from pathlib import Path from unittest.mock import AsyncMock, MagicMock @@ -31,8 +33,10 @@ import pytest from asyncssh.constants import FILEXFER_TYPE_REGULAR, FILEXFER_TYPE_SYMLINK +import ssh_mcp.ssh from ssh_mcp.config import ServerRegistry from ssh_mcp.models import Settings +from ssh_mcp.paths import open_beneath from ssh_mcp.ssh import SSHManager pytestmark = pytest.mark.asyncio @@ -277,6 +281,29 @@ async def test_download_no_clobber_existing_file(root: Path) -> None: fake_sftp.get.assert_not_called() +@pytest.mark.skipif(not hasattr(os, "symlink"), reason="symlinks are POSIX-only") +async def test_download_onto_a_symlink_is_refused_not_followed(root: Path) -> None: + """A symlink at the destination must not be written through. + + ``open_beneath`` adds ``O_NOFOLLOW``, so ``O_CREAT|O_EXCL`` reports + EEXIST for a symlink rather than following it — which is the whole + point, since the link may aim anywhere on the operator's machine. This + pins both halves: nothing is written to the target, and the error names + the symlink case so it does not read as "a regular file is in the way". + """ + outside = root.parent / "outside-transfer-root.txt" + outside.write_bytes(b"untouched") + os.symlink(outside, root / "link.txt") + fake_sftp = _FakeSFTP() + manager = _make_manager(root, fake_sftp) + + with pytest.raises(ValueError, match="symlink at this name"): + await manager.download("test-host", "/remote/src.txt", "link.txt") + + assert outside.read_bytes() == b"untouched", "wrote through the symlink" + fake_sftp.get.assert_not_called() + + # --------------------------------------------------------------------------- # Audit record reports the real transferred file and byte count (S13/TOCTOU) # --------------------------------------------------------------------------- @@ -342,6 +369,82 @@ async def test_upload_rejects_non_regular_local_file(root: Path) -> None: await manager.upload("test-host", "adir", "/remote/dest.txt") +async def test_upload_opens_the_local_file_non_blocking( + root: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Upload must pass ``O_NONBLOCK``, or a fifo makes ``S_ISREG`` unreachable. + + The sibling test above uses a directory, which opens fine and so does + reach the ``S_ISREG`` refusal. A fifo does not: a plain ``O_RDONLY`` + open blocks until a writer appears, so the refusal never runs for + precisely the file type it exists to reject. + + Measured 2026-09-07 against 0.8.0: the open runs in a worker thread, so + the event loop survives — but the thread does not. 16 concurrent fifo + uploads exhausted the default executor and every later ``to_thread`` + call, i.e. ALL SFTP in the process, stalled until restart while + ``execute`` kept working. Same defect class 0.8.0 fixed for + ``SSH_MCP_HTTP_TOKEN_FILE``. + + Asserting the flag rather than uploading a real fifo is deliberate and + was measured, not assumed. The obvious end-to-end version — mkfifo, + upload, ``asyncio.wait_for`` — does go red on mutation, and then HANGS + the run: the blocked executor thread is non-daemon, so pytest cannot + exit (observed 120 s, killed externally). A mutation that hangs CI is + strictly worse than one that fails it. The sibling test below proves + the flag is what actually makes a fifo open return. + """ + (root / "payload.txt").write_bytes(b"hi") + seen: list[int] = [] + real_open_beneath = ssh_mcp.ssh.open_beneath + + def spy(root_fd: int, path: str, flags: int, *args: object, **kw: object) -> int: + seen.append(flags) + return real_open_beneath(root_fd, path, flags, *args, **kw) # type: ignore[arg-type] + + monkeypatch.setattr(ssh_mcp.ssh, "open_beneath", spy) + manager = _make_manager(root, _FakeSFTP()) + + await manager.upload("test-host", "payload.txt", "/remote/dest.txt") + + assert seen, "open_beneath was never called — the spy is not wired up" + assert seen[0] & os.O_NONBLOCK, ( + f"upload opened the local file with flags {seen[0]:#o}, without " + "O_NONBLOCK: a fifo beneath transfer_root would block the open " + "forever and never reach the S_ISREG refusal" + ) + + +@pytest.mark.skipif(not hasattr(os, "mkfifo"), reason="fifos are POSIX-only") +def test_o_nonblock_is_what_makes_a_fifo_open_return(root: Path) -> None: + """Pins the assumption the test above depends on. + + Run in the MAIN thread on purpose: a blocked ``open`` here is + interrupted by ``SIGALRM`` instead of leaking a non-daemon executor + thread that would outlive the test. Without the deadline this would + hang rather than fail if the platform ever stopped honouring + ``O_NONBLOCK`` on a fifo. + """ + os.mkfifo(root / "afifo") + root_fd = os.open(root, os.O_RDONLY) + + def _deadline(signum: int, frame: object) -> None: + raise TimeoutError("O_RDONLY|O_NONBLOCK blocked on a writer-less fifo") + + previous = signal.signal(signal.SIGALRM, _deadline) + signal.setitimer(signal.ITIMER_REAL, 5.0) + try: + fd = open_beneath(root_fd, "afifo", os.O_RDONLY | os.O_NONBLOCK) + try: + assert not stat.S_ISREG(os.fstat(fd).st_mode) + finally: + os.close(fd) + finally: + signal.setitimer(signal.ITIMER_REAL, 0) + signal.signal(signal.SIGALRM, previous) + os.close(root_fd) + + # --------------------------------------------------------------------------- # N1: unknown server raises the documented ValueError, not a raw KeyError # --------------------------------------------------------------------------- diff --git a/tests/test_ssh.py b/tests/test_ssh.py index 0ed0c23..cac0f85 100644 --- a/tests/test_ssh.py +++ b/tests/test_ssh.py @@ -22,11 +22,13 @@ from ssh_mcp.ssh import ( _DANGEROUS_PATTERNS, _LONG_FLAG_KEYWORDS, + _REDACTION_ELIDED_PLACEHOLDER, _REDACTION_PLACEHOLDER, _SENSITIVE_PATHS, SSHManager, _is_dangerous_command, _make_connection_id, + _placeholder_for, _redact_secrets, _safe_exc, _unlink_beneath, @@ -1302,6 +1304,100 @@ def test_connection_ids_cleared_on_close_all( assert manager._connection_ids == {} +class TestDangerousCommandHostAvailability: + """Taking the host down is as destructive as wiping its disk. + + Found by end-to-end testing against a live host on 2026-09-07: the + table covered filesystem and device destruction thoroughly and host + availability not at all. ``reboot``, ``poweroff``, ``init 0``, + ``iptables -F`` and ``userdel -r`` all reached the host, while a + routine ``rm -rf /tmp/build-cache`` was blocked — an inverted priority + for a tool whose ``execute_on_group`` fans one command across a fleet. + """ + + @pytest.mark.parametrize( + "command", + [ + "reboot", + "poweroff", + "halt", + "shutdown -h now", + "sudo reboot", + "doas poweroff", + "uptime; reboot", + "true && reboot", + "init 0", + "init 6", + "telinit 6", + "systemctl poweroff", + "systemctl --force reboot", + "iptables -F", + "ip6tables --flush", + "sudo iptables -t nat -F", + "nft flush ruleset", + "userdel -r tester", + "passwd -l root", + ], + ) + def test_availability_destruction_is_blocked(self, command: str) -> None: + assert _is_dangerous_command(command), ( + f"{command!r} reached the host — it takes the host, or access to it, away" + ) + + @pytest.mark.parametrize( + "command", + [ + # Diagnostics. Blocking these is how a tripwire teaches + # operators to route around it, so a bare `\\breboot\\b` was + # rejected in favour of a command-position anchor. + "last reboot", + "grep reboot /var/log/messages", + "journalctl | grep -i poweroff", + "who -b", + "uptime", + "echo rebooted_at", + # systemctl verbs that are not availability destruction. + "systemctl status nginx", + "systemctl restart nginx", + # iptables read-only, and the lowercase flag that does not + # flush — proving the pattern is case-sensitive on purpose. + "iptables -L -n", + "iptables -f", + "passwd --status bob", + "apt-get install -y userdel-helper", + ], + ) + def test_diagnostics_and_lookalikes_are_allowed(self, command: str) -> None: + assert not _is_dangerous_command(command), ( + f"{command!r} was blocked; it does not take the host down" + ) + + @pytest.mark.parametrize( + ("command", "expected"), + [ + ("echo hi\nreboot", True), + ("set -e\nsystemctl poweroff", True), + # Pre-existing coverage that the newline rendering must not + # cost: this one only matches while a line break reads as + # WHITESPACE, which is why both renderings are searched. + ("rm -rf\n/", True), + ("echo hello\nls -la", False), + ], + ) + def test_a_second_line_is_a_command_position( + self, command: str, expected: bool + ) -> None: + """Both renderings of a newline must be searched. + + Sanitization replaces control characters with a space, so a second + line used to read as an ARGUMENT to the first — measured: plain + ``echo hi\\nreboot`` was not caught. Rewriting newlines to ``;`` + instead would have lost ``rm -rf\\n/``, which needs the whitespace + reading, so neither rendering alone is sufficient. + """ + assert _is_dangerous_command(command) is expected + + # --------------------------------------------------------------------------- # Property-based fuzz tests for _is_dangerous_command (B3) # @@ -1698,11 +1794,17 @@ def test_fuzzed_long_flag_junk_prefix_is_unbounded( # secret='credenti' inside keyword='credential', which reported a # "leak" via the FLAG NAME, not via any leaked value. Exact equality is # immune to that whole class and is a strictly stronger property. - expected = f"myapp --{junk}-{keyword}={_REDACTION_PLACEHOLDER} run" + # The placeholder is content-dependent: a secret containing a shell + # separator gets `{REDACTED+ELIDED}`, because redaction cannot tell a + # separator INSIDE a secret from one chaining a second command, and + # errs toward flagging (see `_placeholder_for`). Exact equality is + # preserved — the whole string is still pinned byte for byte. + placeholder = _placeholder_for(secret) + expected = f"myapp --{junk}-{keyword}={placeholder} run" assert redacted == expected, ( f"junk_len={len(junk)} keyword={keyword!r} secret={secret!r} → {redacted!r}" ) - assert _REDACTION_PLACEHOLDER in redacted + assert placeholder in redacted @given( userinfo=st.text( @@ -1777,6 +1879,146 @@ def test_no_leak_short_flag_within_old_bound(self) -> None: assert _REDACTION_PLACEHOLDER in redacted +class TestRedactionElidedMarker: + """A redacted token that swallowed a second command must say so. + + Found by end-to-end testing against a live SSH host on 2026-09-07, not + by this suite. Redaction replaces from the credential marker to the end + of the whitespace-delimited token — which is what stops a partially + quoted secret leaking — but shell separators are not whitespace. So + ``--password=X;reboot`` logged as ``--password={REDACTED}``: the + ``reboot`` was ABSENT from the audit record while the command still ran + on the host. The elided ``touch /tmp/f`` in that session created its + file, which is how the erasure was proven rather than inferred. + + Narrowing the replacement to the separator was rejected — a secret + containing ``;`` would then have its tail printed. The value stays + fully hidden and the record stops being SILENT instead. + """ + + def test_command_chained_after_an_eq_credential_is_flagged(self) -> None: + """The exact shape reproduced against a live host.""" + redacted = _redact_secrets("echo --password=PW1;touch /tmp/f") + assert "PW1" not in redacted + assert _REDACTION_ELIDED_PLACEHOLDER in redacted + + def test_command_chained_after_a_spaced_credential_is_flagged(self) -> None: + """``--flag value`` replaces the whole next token, same erasure.""" + redacted = _redact_secrets("docker login --password PW2;reboot") + assert "PW2" not in redacted + assert "reboot" not in redacted, "the value must still be fully hidden" + assert _REDACTION_ELIDED_PLACEHOLDER in redacted + + def test_ordinary_credential_is_not_flagged(self) -> None: + """The marker must stay meaningful: no separator, no marker. + + Without this the change could degrade into tagging every command + and telling an auditor nothing. + """ + redacted = _redact_secrets('mysql --password=plain -e "select 1"') + assert redacted == 'mysql --password={REDACTED} -e "select 1"' + + def test_separate_token_after_a_credential_is_not_flagged(self) -> None: + """Only text actually consumed counts. + + ``$(id)`` here is its own token and survives into the log, so + flagging would be a lie about what was lost. + """ + redacted = _redact_secrets("curl --token=T $(id)") + assert redacted == "curl --token={REDACTED} $(id)" + + @pytest.mark.parametrize("sep", [";", "|", "&", "`", "$("]) + def test_every_separator_that_can_chain_is_flagged(self, sep: str) -> None: + """Each separator in the set, so removing one from the regex fails. + + Newline and CR are absent on purpose: whitespace has already split + the text into tokens before either caller runs, so nothing can be + elided across one. Measured — asserting them here failed, which is + why they were removed from the regex rather than left as an + unreachable branch in a security-relevant predicate. + """ + redacted = _redact_secrets(f"cmd --password=PW{sep}rest") + assert "PW" not in redacted + assert _REDACTION_ELIDED_PLACEHOLDER in redacted, ( + f"separator {sep!r} elided text without flagging the record" + ) + + def test_a_secret_containing_a_separator_is_also_flagged(self) -> None: + """Documents the deliberate false-positive bias. + + Deciding whether a ``;`` inside a token separates a command or is + just a character in the password needs shell parsing, which this + module refuses to do on purpose. So the marker means "this record + MAY be incomplete", never "a command was definitely hidden", and it + errs toward flagging — a noisy marker is recoverable, a silently + truncated audit record is not. + """ + redacted = _redact_secrets("app --password=pa;ss run") + assert "pa;ss" not in redacted + assert _REDACTION_ELIDED_PLACEHOLDER in redacted + + @pytest.mark.parametrize( + "keyword", ["password", "token", "secret", "key", "credential", "pass"] + ) + def test_the_marker_does_not_depend_on_which_rule_matched( + self, keyword: str + ) -> None: + """Every rule that swallows text to end-of-token must flag it. + + Reported by CI's 200-example Hypothesis profile 2026-09-07, which + the 50-example dev profile had missed: `--…-password=x&` was + flagged and `--…-token=x&` was not, because rule 3a's enumerated + env names (`TOKEN`, `SECRET`, …) matched the second one first and + used the plain placeholder. An inconsistent marker is worse than + none — a reader takes its absence as proof nothing was cut. + """ + junk = "0" * 41 + redacted = _redact_secrets(f"myapp --{junk}-{keyword}=0000000& run") + assert _REDACTION_ELIDED_PLACEHOLDER in redacted, ( + f"keyword {keyword!r} lost the marker; which rule matched must " + "not change what the record claims" + ) + + def test_a_later_rule_cannot_downgrade_the_marker(self) -> None: + """The marker must survive re-redaction by a subsequent rule. + + Same CI finding, one layer down: rule 3a flagged the value + correctly and then `_redact_long_flags` re-matched the same flag + and replaced `{REDACTED+ELIDED}` — which contains no separator — + with the plain placeholder. A marker a later rule can erase is + worth nothing, so `_placeholder_for` treats an existing one as + sticky. + """ + assert ( + _placeholder_for(_REDACTION_ELIDED_PLACEHOLDER) + == _REDACTION_ELIDED_PLACEHOLDER + ) + # The plain placeholder carries no claim, so it must NOT be sticky — + # otherwise re-redacting an ordinary secret would invent a marker. + assert _placeholder_for(_REDACTION_PLACEHOLDER) == _REDACTION_PLACEHOLDER + + @pytest.mark.parametrize( + ("command", "flagged"), + [ + ("PGPASSWORD=a&b psql", True), + ("VAULT_TOKEN=x;id", True), + ("curl -u bob:pw;id http://x/", True), + ("sshpass -p pw;id ssh h", True), + ('curl -H "Authorization: Bearer tok;id" http://x/', True), + # Bounded replacements cannot swallow a following command, so + # flagging them would be a pure false positive: the closing + # quote and the `@` respectively terminate the value. + ("mysql -p'quoted;value' -e x", False), + ("https://u:p;q@host/", False), + ("mysql -pplain -e x", False), + ], + ) + def test_only_unbounded_rules_flag(self, command: str, flagged: bool) -> None: + """The distinction is whether the rule replaces to end-of-token.""" + redacted = _redact_secrets(command) + assert (_REDACTION_ELIDED_PLACEHOLDER in redacted) is flagged, redacted + + class TestRedactSecretsPerformance: """B2-regex: quadratic backtracking in rules 0, 8, 9 fixed via bounded (and, for rule 0, possessive) quantifiers.