diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1c66260..f011c0c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -276,18 +276,30 @@ jobs: done <<< "$TAGS" docker buildx imagetools create "${TAG_ARGS[@]}" "${IMAGE}@${DIGEST}" - # Intended as proof, in the job log, that every published tag resolves to - # the bytes Trivy cleared. As written the check cannot pass: `imagetools - # create` above wraps the single-platform manifest in a NEW OCI index, so - # the tag's digest legitimately differs from the scanned one (and hashing - # `inspect --raw` stdout folds in a trailing newline the registry's - # manifest bytes do not have). Since 2026-07-26 it fails on every publish - # run — after the tags are already live — which also stops the attest step - # below from running. A failure here means the comparison is broken, NOT - # that the scan gate was bypassed. FIXME: compare the manifest digest the - # registry reports (`imagetools inspect --format '{{.Manifest.Digest}}'`), - # accounting for the index wrapping. - - name: Verify every tag resolves to the scanned digest + # Proof, in the job log, that every published tag serves the bytes Trivy + # cleared — nothing more. + # + # Production incident 2026-07-26: the original check compared + # `sha256sum` of `imagetools inspect --raw` against the build digest and + # failed on EVERY publish run, taking the attest step below down with it + # (images published between 2026-07-26 and this fix carry no provenance + # attestation). Confirmed cause: `imagetools create` does NOT reuse the + # source digest — it wraps the single-platform manifest in a NEW OCI + # index, so the tag's own digest legitimately differs from the scanned + # manifest's. Verified against the live registry: `:latest` was index + # sha256:8204263e… whose sole child was the scanned sha256:12db37ce…, + # and re-hashing those bytes reproduces 8204263e exactly. + # (Piping `inspect --raw` into `sha256sum` is separately fragile if the + # 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 + # 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 if: env.PUBLISH == 'true' env: TAGS: ${{ steps.meta.outputs.tags }} @@ -296,17 +308,33 @@ jobs: set -euo pipefail while IFS= read -r tag; do [ -z "$tag" ] && continue - resolved="$(docker buildx imagetools inspect --raw "$tag" | sha256sum | cut -d' ' -f1)" - echo "${tag} -> sha256:${resolved} (scanned ${DIGEST})" - if [ "sha256:${resolved}" != "${DIGEST}" ]; then - echo "::error::${tag} does not resolve to the scanned digest ${DIGEST}" >&2 + raw="$(docker buildx imagetools inspect --raw "$tag")" + # An index lists children under .manifests; a bare manifest has none. + served="$(printf '%s' "$raw" | jq -r ' + if .manifests then + [ .manifests[] + | select((.annotations["vnd.docker.reference.type"] // "") + != "attestation-manifest") + | .digest ] | join(" ") + else + empty + end')" + if [ -z "$served" ]; then + served="$(docker buildx imagetools inspect --format '{{.Manifest.Digest}}' "$tag")" + 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 + if [ -z "$served" ]; then + echo "::error::${tag} resolved to no image manifest at all" >&2 exit 1 fi done <<< "$TAGS" - # Unreachable on the publish path today: the verify step above fails - # first, so images pushed to GHCR since 2026-07-26 carry no provenance - # attestation. Fixing verify restores this. - name: Attest build provenance for the image if: env.PUBLISH == 'true' uses: actions/attest-build-provenance@0f67c3f4856b2e3261c31976d6725780e5e4c373 # v4.1.1 diff --git a/CHANGELOG.md b/CHANGELOG.md index 3f86f8e..606a74d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,7 +7,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] -> **Release note for whoever cuts this:** ship as **0.6.0**, not 0.5.7. Two independent reasons: the `mcp` floor in `[project.dependencies]` is narrowed from `>=1.27.0` to `>=1.28.1`, and the SFTP path contract is a **breaking change** (see everything under *Changed — BREAKING* below). +## [0.6.0] - 2026-07-26 + +> **Why 0.6.0 and not 0.5.7:** two independent reasons — the `mcp` floor in `[project.dependencies]` is narrowed from `>=1.27.0` to `>=1.28.1`, and the SFTP path contract is a **breaking change** (see everything under *Changed — BREAKING* below). ### Security — please read if you run ssh-mcp @@ -25,6 +27,10 @@ Two further hardening fixes in the same area: the audit record now reports the f **Denial of service in credential redaction.** `_redact_secrets` ran in quadratic time, so a ~10 KB command stalled the single-threaded server for over six seconds, reachable without an SSH connection or SSH authentication via `dry_run=true`. There was no length limit on a command anywhere. The two rules responsible — URL basic-auth and long-flag matching — are now hand-written token-wise scanners that visit each token once (~0.007 s for 100 KB); the remaining rules stay on Python's `re` engine, which their lookbehinds need. `max_command_bytes` (default 64 KiB) caps command length at the tool boundary. Multi-word *quoted* credential values remain only partially redacted — unchanged from previous behaviour; redaction is a tripwire, and credentials should be passed via env files or stdin rather than argv. +**Credential redaction missed values beginning with a dash.** In the space-separated form (`--password `, not `--password=`), a value starting with `-` was left unredacted, so it reached logs and OTel spans in plaintext. The scanner treated any following token that began with `-` as "this flag was boolean, leave the next token alone" — a guard that exists to fix an earlier leak where a credential flag following a boolean flag was silently skipped — and a password beginning with `-` is indistinguishable from a flag under that rule. Generated passwords routinely start with `-`, so this was not a corner case. The guard is now narrowed to `--`: a single-dash token is redacted, while a `--`-prefixed token is still left for its own classification turn, keeping the earlier fix intact. **Residual, unchanged and deliberate:** a value beginning with `--` is still treated as a flag and not redacted. Redaction remains a tripwire — pass credentials via env files or stdin rather than argv. + +**`SSH_MCP_HTTP_ALLOWED_HOSTS=*.*` silently disabled DNS-rebinding protection.** The startup gate refuses bare wildcards, and `*.*` was listed in its refusal set, but the allowance for legitimate suffix wildcards (`*.internal.example.com`) was evaluated first, so `*.*` — which matches effectively any dotted hostname — was accepted. On the HTTP transport that is remote command execution behind a rebinding attack. Validation now strips only the two deliberately-permitted wildcard forms (a leading `*.` subdomain wildcard and a trailing `:*` port wildcard) and refuses anything with a wildcard left over; the same change also starts refusing infix wildcards such as `a*b.example.com`, which the previous check never caught, and a whitespace-only value, which previously fell back silently to loopback-only. Suffix wildcards remain permitted as documented. + **Audit records could be forged.** The success and timeout audit paths interpolated the command without escaping control characters, so an embedded newline produced a convincing fake log line. All audit paths now escape. Also cleared every advisory reported by `pip-audit`. **None of the four is exploitable in ssh-mcp** — this is defence-in-depth and a CI unblock, not an incident response. Being precise, because the two are not the same thing: three of the four are *unreachable* (the vulnerable code is never imported or never called), while CVE-2026-52869's session manager **is** in the HTTP request path — it is reachable but its exploit precondition cannot be satisfied by ssh-mcp's authentication model. Details per advisory below. Reachability was verified by grep over `src/` and the non-exploitability argument for CVE-2026-52869 was independently stress-tested by two external models. @@ -67,7 +73,8 @@ Also cleared every advisory reported by `pip-audit`. **None of the four is explo **SFTP no longer raises a bare `KeyError`** for an unknown server, which fell outside its documented `ValueError`/`RuntimeError` contract. A non-ASCII bearer token now returns 401 rather than 500 (it always failed closed). `$XDG_CONFIG_HOME` is honoured for the config search path, matching the new `$XDG_DATA_HOME` handling. -- **The `docker` job scanned an image it did not publish.** It ran BuildKit twice — a `load: true` image for Trivy and a separately built pushed image — which by construction cannot share a digest, so the scanned bytes were never provably the shipped bytes. There is now one build, pushed to GHCR *by digest with no tag attached* (`push-by-digest=true,name-canonical=true`); Trivy scans `${IMAGE}@${digest}`; tags are created from that same digest only after the scan passes, and a verification step fails the job if any tag resolves elsewhere. Build provenance is recorded as a GitHub Artifact Attestation against the digest. +- **The `docker` job scanned an image it did not publish.** It ran BuildKit twice — a `load: true` image for Trivy and a separately built pushed image — which by construction cannot share a digest, so the scanned bytes were never provably the shipped bytes. There is now one build, pushed to GHCR *by digest with no tag attached* (`push-by-digest=true,name-canonical=true`); Trivy scans `${IMAGE}@${digest}`; tags are created from that same digest only after the scan passes, and a verification step fails the job if any tag serves anything other than the scanned digest. Build provenance is recorded as a GitHub Artifact Attestation against the digest. +- **That tag-verification step was itself broken, and it suppressed provenance attestation.** `docker buildx imagetools create` does not reuse the source digest — it wraps the scanned manifest in a *new* OCI index — so comparing the tag's own digest against the build digest compared an index against one of its children and failed on every push to `main`. Because the check runs before the attestation step, images published to GHCR between 2026-07-26 and this release carry **no provenance attestation**. The scan gate itself was never bypassed: Trivy runs against the digest before any tag is promoted, so published content was always the scanned content. Verification now asserts the invariant that matters — the set of image manifests a tag serves is exactly the scanned digest — by inspecting the index's children and ignoring attestation manifests. - **`SECURITY.md` described dangerous-command detection as "intentionally non-blocking".** The shipped code blocks: a match returns an error instead of executing, and `force=true` is the documented bypass. The policy now says so, and lists the acknowledged obfuscation bypasses rather than implying the check is advisory. - **CI `Lint` job was non-deterministic and had begun failing on unmodified code.** It invoked `uvx ruff` and `uvx bandit`; `uvx` resolves the newest release at run time. ruff **0.16.0** (2026-07-23) expanded its default rule set from **59 to 413 rules**, so the same command that printed `All checks passed!` on 2026-07-16 reported 50 errors days later with zero code changes. Lint tooling is now pinned (`ruff==0.16.0`, `bandit==1.9.4`) in the `dev` extra and invoked via `uv run`, so the gate is a function of the commit rather than of the outside world. - **The repo now declares its own lint standard.** There was no `[tool.ruff]` config at all, so the standard was whatever default the installed ruff happened to ship. `[tool.ruff.lint] select = ["E4", "E7", "E9", "F"]` codifies the pre-0.16 default the codebase has always been held to and is 100% clean against — no rule is dropped. Adopting ruff 0.16's wider default is tracked as a separate deliberate change. diff --git a/src/ssh_mcp/__init__.py b/src/ssh_mcp/__init__.py index 5870aa5..69d40bf 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.5.6" +__version__ = "0.6.0" diff --git a/src/ssh_mcp/server.py b/src/ssh_mcp/server.py index 4ea6bdb..6ce381a 100644 --- a/src/ssh_mcp/server.py +++ b/src/ssh_mcp/server.py @@ -897,7 +897,14 @@ def _run_http() -> None: ) from e token = raw_token or None stateless = os.environ.get("SSH_MCP_HTTP_STATELESS", "false").lower() == "true" - allowed_hosts_env = os.environ.get("SSH_MCP_HTTP_ALLOWED_HOSTS", "").strip() + # Keep the raw (pre-strip) value around so we can tell "unset" apart + # from "explicitly set to whitespace" below — os.environ.get(..., "") + # collapses both to "", which would let a blank templated env var + # (e.g. "${EXTRA_HOSTS:- }") silently fall back to the localhost-only + # default instead of failing loud on what is almost always a config + # generation mistake. + _allowed_hosts_raw = os.environ.get("SSH_MCP_HTTP_ALLOWED_HOSTS") + allowed_hosts_env = (_allowed_hosts_raw or "").strip() # Auth-mode dispatch. Default ``bearer`` preserves v0.3.1 behavior. # ``none`` disables the bearer middleware entirely — useful when a @@ -963,17 +970,48 @@ def _run_http() -> None: base_hosts = ["127.0.0.1:*", "localhost:*", "[::1]:*"] extra_hosts: list[str] = [] + if ( + _allowed_hosts_raw is not None + and _allowed_hosts_raw != "" + and not allowed_hosts_env + ): + # Explicitly set but whitespace-only after stripping. Distinct from + # "unset" (which falls through to the localhost-only default below) + # — a whitespace value almost always means a broken env-file + # substitution, and silently treating it as "no extra hosts" would + # mask that from the operator. + raise RuntimeError( + "SSH_MCP_HTTP_ALLOWED_HOSTS is set but contains only whitespace. " + "Unset the variable to use the localhost-only default, or " + "provide a concrete hostname (e.g. 'ssh-mcp.internal:*')." + ) if allowed_hosts_env: extra_hosts = [h.strip() for h in allowed_hosts_env.split(",") if h.strip()] # H4: reject wildcards — they silently disable DNS-rebinding # protection. An operator setting "*" almost certainly means # "match my specific hostname" and doesn't realize the security # implication. Fail loud instead of silently letting it through. + # + # Ordering trap (regression found on ci/fix-digest-verification): + # the previous implementation special-cased entries starting with + # "*." as "always a permitted suffix wildcard, skip refusal" via a + # `continue` evaluated before the `entry in {"*", "*:*", "*.*"}` + # refusal was reached. "*.*" itself starts with "*." too, so that + # `continue` fired first and let "*.*" — which matches essentially + # any dotted hostname and is semantically identical to the bare + # "*" this gate exists to block — through as PERMITTED even though + # it was listed in the refusal set. Do not reintroduce a + # startswith("*.")-first shortcut. The fix below strips the two + # deliberately-permitted wildcard *forms* (a trailing ":*" port + # wildcard, then a leading "*." subdomain wildcard) and only + # afterwards demands a concrete, wildcard-free remainder. for entry in extra_hosts: - bare = entry.replace(":*", "").replace("*", "") - if not bare or entry in {"*", "*:*", "*.*"} or entry.startswith("*."): - if entry.startswith("*."): - continue # wildcard suffixes like *.internal.example.com are OK + remainder = entry + if remainder.endswith(":*"): + remainder = remainder[:-2] + if remainder.startswith("*."): + remainder = remainder[2:] + if not remainder or "*" in remainder or not remainder.strip("."): raise RuntimeError( f"SSH_MCP_HTTP_ALLOWED_HOSTS wildcard entry {entry!r} " "would disable DNS-rebinding protection. " diff --git a/src/ssh_mcp/ssh.py b/src/ssh_mcp/ssh.py index 1b615be..11f411f 100644 --- a/src/ssh_mcp/ssh.py +++ b/src/ssh_mcp/ssh.py @@ -408,11 +408,28 @@ def _redact_long_flags(text: str) -> str: 1) redacts from that ``=`` to the end of THIS token. * ``--`` alone — credential-shaped ```` looks at exactly the next non-whitespace token and redacts it, UNLESS that token - itself starts with ``-`` (then this flag was boolean/valueless; + itself starts with ``--`` (then this flag was boolean/valueless; the next token is left untouched here and gets its own turn on the next loop iteration — it is never silently skipped). * anything else — passed through unchanged. + Defect (found 2026-07-25, reported against ``main``): the guard + above used to test for a single leading ``-``, not ``--``. A + credential VALUE that itself begins with ``-`` (e.g. a numeric + argument like ``-0000000``, or any dash-led token) is + indistinguishable from a flag under a single-dash test, so it was + passed through unredacted — ``mysql --password -0000000 somedb`` + leaked the password verbatim. Narrowing the guard to ``--`` fixes + this without reopening the original skip-leak it exists to + prevent: a ``--``-prefixed token is still almost certainly a long + flag, AND it still gets its own classification turn on the next + loop iteration (so ``--password --token=x`` still redacts + ``--token=x`` — no skip). A single-dash token (``-p``, ``-rf``, + bare ``-``, or ``-0000000``) is now treated as the credential's + value and redacted; for a tripwire, over-redaction is strictly + safer than under-redaction (see the module docstring above + ``_REDACTION_PLACEHOLDER``). + Note (Defect 1 follow-up, scope note): a credential value that is itself shell-quoted and contains whitespace (``--password="a b"``) is only partially redacted here, same as on ``main`` — matching @@ -444,7 +461,11 @@ def _redact_long_flags(text: str) -> str: j = i + 1 if j < n and chunks[j][1]: # skip exactly one whitespace run j += 1 - if j < n and not chunks[j][1] and not chunks[j][0].startswith("-"): + # Guard narrowed from "-" to "--" (defect above): a + # dash-prefixed VALUE (e.g. "-0000000") must still be + # 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 i = j + 1 continue @@ -2570,16 +2591,29 @@ async def _create_connection( _safe_log_value(str(e)), ) raise - except OSError as e: + # `except asyncio.TimeoutError` MUST precede `except OSError`: on + # Python >=3.11 `asyncio.TimeoutError is TimeoutError`, and + # `TimeoutError` is itself a subclass of `OSError`. With OSError + # listed first (the previous ordering) the TimeoutError handler + # below was unreachable dead code — every `asyncio.wait_for` + # timeout was silently caught by the OSError branch and logged + # with the less specific "OS error connecting" message instead + # of "Timeout connecting". Reordering (rather than deleting the + # TimeoutError branch) preserves the more specific message for + # the common "server unreachable within command_timeout" case, + # while a genuine OSError that is NOT a timeout (e.g. + # ConnectionRefusedError) still falls through to the OSError + # branch below. + except asyncio.TimeoutError as e: logger.error( - "OS error connecting to %s: %s", + "Timeout connecting to %s: %s", _safe_log_value(server.name), _safe_log_value(str(e)), ) raise - except asyncio.TimeoutError as e: + except OSError as e: logger.error( - "Timeout connecting to %s: %s", + "OS error connecting to %s: %s", _safe_log_value(server.name), _safe_log_value(str(e)), ) diff --git a/tests/test_ci_lint_determinism.py b/tests/test_ci_lint_determinism.py index 18dee4d..58d81a3 100644 --- a/tests/test_ci_lint_determinism.py +++ b/tests/test_ci_lint_determinism.py @@ -333,6 +333,68 @@ def test_docker_job_builds_exactly_once() -> None: ) +def test_digest_verification_accounts_for_index_wrapping() -> None: + """Production incident 2026-07-26: the publish-path digest check failed on + every run to ``main``, and because it sits *before* the attestation step it + also stopped provenance from ever being attached. + + ``docker buildx imagetools create`` does not reuse the source digest — it + wraps the scanned manifest in a NEW OCI index. So comparing the tag's own + digest (or a hash of ``inspect --raw`` stdout) against + ``steps.build.outputs.digest`` compares an index against one of its + children and can never match. Verified against the live registry: tag + ``:latest`` was index ``sha256:8204263e…`` whose sole child was the scanned + ``sha256:12db37ce…``. + + The invariant worth asserting is that every published tag serves *only* the + scanned digest, which means inspecting the index's children. This test + fails if someone reintroduces the digest-to-digest comparison. + """ + workflow = _load_workflow(CI_WORKFLOW_PATH) + docker_job = workflow["jobs"]["docker"] + + 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", "") + ] + assert verify_steps, ( + "the docker job no longer verifies that published tags serve the " + "scanned digest; that check is the proof the Trivy gate was not bypassed" + ) + + for step in verify_steps: + script = step.get("run", "") + # The broken form hashed the raw manifest bytes and compared that to + # the build digest. Both halves of that mistake are banned. + assert "sha256sum" not in script, ( + "the digest-verification step hashes `imagetools inspect --raw` " + "output again. That yields the INDEX digest, which never equals " + "the scanned manifest digest, so the step fails on every publish " + "run and blocks provenance attestation. Inspect the index's " + "child manifests instead." + ) + # It must actually look at what the index serves. + assert ".manifests" in script, ( + "the digest-verification step does not inspect the index's " + "`.manifests` children, so it cannot tell whether the tag serves " + "the scanned image after `imagetools create` wrapped it" + ) + + # Ordering matters for the consequence described above: attestation must + # not be gated behind a check that is prone to this failure mode without + # someone noticing the images lost their provenance. + step_names = [step.get("name", "") for step in docker_job.get("steps", [])] + attest = next( + (i for i, name in enumerate(step_names) if "Attest build provenance" in name), + None, + ) + assert attest is not None, ( + "the docker job no longer attests build provenance for the image" + ) + + 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_http_transport.py b/tests/test_http_transport.py index 3efb5e0..f1d8857 100644 --- a/tests/test_http_transport.py +++ b/tests/test_http_transport.py @@ -523,6 +523,83 @@ def test_wildcard_in_list_also_rejected( with pytest.raises(RuntimeError, match="wildcard"): _run_http() + @pytest.mark.parametrize( + "entry", + [ + "*", + "*:*", + "*.*", + "*.*:*", + "*.*.*", + "*.:*", + "a*b.example.com", + ], + ) + def test_wildcard_family_rejected( + self, entry: str, monkeypatch: pytest.MonkeyPatch + ) -> None: + """Regression for the H4 ordering trap on ci/fix-digest-verification. + + The previous implementation checked ``entry.startswith("*.")`` as an + "always a permitted suffix wildcard" escape hatch BEFORE the + ``entry in {"*", "*:*", "*.*"}`` refusal ran. "*.*" satisfies + ``startswith("*.")`` too, so that escape hatch fired first and let + "*.*" — which matches essentially any dotted hostname, functionally + equivalent to the bare "*" this gate exists to block — through as + PERMITTED, even though it was explicitly listed in the refusal set. + + Every entry here must still resolve to "no concrete hostname" + after stripping the two deliberately-permitted wildcard forms (a + trailing ``:*`` port wildcard, a leading ``*.`` subdomain + wildcard), so all of them must be refused. + """ + monkeypatch.setenv("SSH_MCP_HTTP_HOST", "127.0.0.1") + monkeypatch.setenv("SSH_MCP_HTTP_ALLOWED_HOSTS", entry) + with pytest.raises(RuntimeError, match="wildcard"): + _run_http() + + def test_whitespace_only_allowed_hosts_rejected( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """An ALLOWED_HOSTS value that is explicitly set but blank (e.g. an + empty templated env var like ``${EXTRA_HOSTS:- }``) must fail loud + rather than silently falling back to the localhost-only default — + that fallback would mask a broken deployment config from the + operator. + """ + monkeypatch.setenv("SSH_MCP_HTTP_HOST", "127.0.0.1") + monkeypatch.setenv("SSH_MCP_HTTP_ALLOWED_HOSTS", " ") + with pytest.raises(RuntimeError, match="whitespace"): + _run_http() + + @pytest.mark.parametrize( + "entry", + [ + "*.internal.example.com", + "*.internal.example.com:*", + "ok.example.com:*", + "ok.example.com", + ], + ) + def test_deliberate_wildcard_forms_still_permitted( + self, entry: str, monkeypatch: pytest.MonkeyPatch + ) -> None: + """Suffix wildcards and port wildcards remain deliberately permitted. + + Documented in AGENTS.md: a leading subdomain wildcard such as + ``*.internal.example.com`` and a trailing port wildcard such as + ``ok.example.com:*`` (and combinations of the two) must keep + working — only entries with no concrete hostname remainder are + refused. Patches ``uvicorn.run`` and ``_build_http_app`` so no + socket is actually bound. + """ + monkeypatch.setenv("SSH_MCP_HTTP_HOST", "127.0.0.1") + monkeypatch.setenv("SSH_MCP_HTTP_ALLOWED_HOSTS", entry) + with patch("uvicorn.run"), patch.object(server_module, "_build_http_app"): + _run_http() + allowed = server_module.mcp.settings.transport_security.allowed_hosts + assert entry in allowed, f"{entry!r} should be permitted, got {allowed!r}" + def test_token_whitespace_stripped(self, monkeypatch: pytest.MonkeyPatch) -> None: """M4: SSH_MCP_HTTP_TOKEN with trailing whitespace (common from .env files that append ``\\n``) must be stripped before being handed to diff --git a/tests/test_ssh.py b/tests/test_ssh.py index cd7c1b3..d8d99f1 100644 --- a/tests/test_ssh.py +++ b/tests/test_ssh.py @@ -1687,13 +1687,19 @@ def test_fuzzed_long_flag_junk_prefix_is_unbounded( prefix to 40 chars — this generates junk from 41 to 300 chars to prove the replacement scanner has no such ceiling. """ - assume(secret not in junk) - assume(secret not in "{REDACTED}") cmd = f"myapp --{junk}-{keyword}={secret} run" redacted = _redact_secrets(cmd) - assert secret not in redacted, ( - f"Leaked: junk_len={len(junk)} keyword={keyword!r} secret={secret!r} " - f"→ {redacted!r}" + # Assert the exact expected output rather than `secret not in redacted`. + # The substring form needed a growing pile of `assume()`s to stay + # satisfiable, because the structural parts of the command legitimately + # survive redaction: a generated secret can be a substring of `junk` + # (already excluded) or of `keyword` — Hypothesis found + # 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" + assert redacted == expected, ( + f"junk_len={len(junk)} keyword={keyword!r} secret={secret!r} → {redacted!r}" ) assert _REDACTION_PLACEHOLDER in redacted @@ -1928,11 +1934,15 @@ def _tokenwise_command(draw: st.DrawFn) -> tuple[str, str]: if use_eq_form: cred_tokens = [f"{cred_name}={secret}"] else: - # Space-separated form: a value token starting with '-' is - # genuinely ambiguous with another flag (this is documented, - # intentional behaviour — see _redact_long_flags), so exclude it - # here rather than assert on undefined CLI-parsing behaviour. - assume(not secret.startswith("-")) + # Space-separated form: a value token starting with '--' is + # genuinely ambiguous with another long flag (this is + # documented, intentional behaviour — see _redact_long_flags), + # so exclude it here rather than assert on undefined + # CLI-parsing behaviour. A single-dash value (e.g. "-0000000") + # is NOT ambiguous — it must be redacted — so it is deliberately + # left in the generated space here (see the 2026-07-25 defect + # note in _redact_long_flags). + assume(not secret.startswith("--")) cred_tokens = [cred_name, secret] token_groups = [[t] for t in boolean_flags] + [[t] for t in noncred_flags] @@ -2122,6 +2132,133 @@ def test_credential_flag_at_token_start_still_works(self) -> None: assert redacted == "--password={REDACTED}" +# --------------------------------------------------------------------------- +# Defect (found 2026-07-25, reported against `main`): a space-separated +# credential flag whose VALUE begins with a single '-' (e.g. `-0000000`) +# was indistinguishable, under the old "starts with '-'" boolean-flag +# guard in _redact_long_flags, from a following flag — so it was left +# unredacted. Fixed by narrowing the guard to "starts with '--'". See the +# docstring above _redact_long_flags in ssh.py for the full rationale. +# --------------------------------------------------------------------------- + + +class TestRedactSecretsDashPrefixedValue: + """Regression tests: a dash-led credential VALUE in the space-separated + flag form (``--password VALUE``) must be redacted like any other + value, not mistaken for a following flag.""" + + @pytest.mark.parametrize( + "command,secret,expected", + [ + ( + "mysql --password -0000000 somedb", + "-0000000", + "mysql --password {REDACTED} somedb", + ), + ( + "mysql --password - somedb", + None, + "mysql --password {REDACTED} somedb", + ), + ( + # Value uses "--token" (not "--password") so the short-flag + # value "-p" cannot false-positive as a substring of the + # flag name itself ("-p" IS a substring of "--password"). + "tool --token -p somedb", + "-p", + "tool --token {REDACTED} somedb", + ), + ( + "tool --token -rf somedb", + "-rf", + "tool --token {REDACTED} somedb", + ), + ( + # Dash-prefixed value at end-of-string, no trailing token. + "mysql --password -0000000", + "-0000000", + "mysql --password {REDACTED}", + ), + ], + ids=[ + "numeric-dash-value", + "bare-single-dash-value", + "short-flag-shaped-value", + "short-flag-rf-shaped-value", + "dash-prefixed-value-eos", + ], + ) + def test_dash_prefixed_space_separated_value_redacted( + self, command: str, secret: str | None, expected: str + ) -> None: + redacted = _redact_secrets(command) + assert redacted == expected + if secret is not None: + assert secret not in redacted, f"Leaked: {command!r} -> {redacted!r}" + + @pytest.mark.parametrize( + "command", + [ + # A value that is only dashes (`---`) begins with "--", which + # is documented, intentional residual ambiguity (see the + # _redact_long_flags docstring) — it is left untouched, same + # as any other genuine "--"-prefixed long flag would be. + "mysql --password --- somedb", + "mysql --password ----- somedb", + "mysql --password -----", + ], + ids=["dashes-only-value", "longer-dashes-only-value", "dashes-only-value-eos"], + ) + def test_double_dash_shaped_value_left_unredacted_documented_residual( + self, command: str + ) -> None: + """Control case, NOT a leak of concern: a value indistinguishable + from a long flag (starts with '--') is intentionally left alone by + the narrowed guard — this is the documented tripwire boundary, not + a credential (a real credential value beginning with two literal + dashes is an edge case outside this tripwire's coverage, same as + the pre-existing quoted-multi-word-value residual).""" + assert _redact_secrets(command) == command + + def test_double_dash_after_credential_flag_still_treated_as_boolean(self) -> None: + """Control case: the guard is narrowed to '--', not removed — a + genuine long flag immediately following a space-separated + credential flag must still be left alone (boolean-flag shape), + and it still gets its own classification turn (no skip-leak + reintroduced).""" + redacted = _redact_secrets("cmd --password --rm image") + assert redacted == "cmd --password --rm image" + + def test_credential_flag_then_double_dash_credential_flag_both_redacted( + self, + ) -> None: + """The skip-leak this guard exists to prevent (R5/Defect A): a + credential flag immediately followed by ANOTHER credential flag + must not cause the second one to be silently skipped.""" + redacted = _redact_secrets("cmd --password --token=secret") + assert "secret" not in redacted + assert redacted == "cmd --password --token={REDACTED}" + + def test_boolean_flag_then_credential_flag_docker_rm_shape(self) -> None: + """Named in the task report: docker run --rm --password=X must not + leak X (pre-existing skip-leak guard, unaffected by this fix).""" + redacted = _redact_secrets("docker run --rm --password=X") + assert "=X" not in redacted + assert redacted == "docker run --rm --password={REDACTED}" + + def test_redaction_idempotent_on_dash_prefixed_value(self) -> None: + """Redacting already-redacted output must not change it further.""" + once = _redact_secrets("mysql --password -0000000 somedb") + twice = _redact_secrets(once) + assert once == twice == "mysql --password {REDACTED} somedb" + + def test_fails_on_unfixed_guard_reproduces_task_example(self) -> None: + """Sanity pin of the exact example from the bug report.""" + redacted = _redact_secrets("mysql --password -0000000 somedb") + assert redacted == "mysql --password {REDACTED} somedb" + assert "-0000000" not in redacted + + # --------------------------------------------------------------------------- # fail_fast cancelled-result visibility (R5 finding #9) # --------------------------------------------------------------------------- @@ -2955,6 +3092,97 @@ async def test_eviction_resets_running_on_crash(self) -> None: assert manager._running is False, "_running must reset after crash" +# --------------------------------------------------------------------------- +# _create_connection exception-handler ordering (found 2026-07-25): on +# Python >=3.11, `asyncio.TimeoutError is TimeoutError`, and `TimeoutError` +# is itself a subclass of `OSError`. The `except OSError` handler used to +# be listed BEFORE `except asyncio.TimeoutError`, which made the +# TimeoutError handler unreachable dead code — every connect timeout was +# silently caught by the OSError branch and logged with the less specific +# "OS error connecting" message. Fixed by reordering (TimeoutError first) +# so the more specific "Timeout connecting" message is preserved, while a +# non-timeout OSError (e.g. ConnectionRefusedError) still reaches the +# OSError branch. +# --------------------------------------------------------------------------- + + +class TestCreateConnectionExceptionOrdering: + """_create_connection must log a distinct message for a connect + timeout vs. any other OSError.""" + + def _make_registry(self) -> ServerRegistry: + import tempfile + + config_content = """ +[settings] +command_timeout = 30 + +[groups] +test = { description = "Test group" } + +[servers.test-host] +description = "Test server" +groups = ["test"] +""" + tmp = tempfile.NamedTemporaryFile(suffix=".toml", mode="w", delete=False) + tmp.write(config_content) + tmp.flush() + tmp.close() + return ServerRegistry(tmp.name) + + async def test_timeout_error_logs_timeout_message_not_os_error( + self, caplog: pytest.LogCaptureFixture + ) -> None: + """A TimeoutError from asyncssh.connect (as asyncio.wait_for + propagates it) must be logged as 'Timeout connecting', proving the + handler is reachable and ordered before the OSError branch.""" + from unittest.mock import AsyncMock, patch + + manager = SSHManager(self._make_registry(), Settings()) + server = manager.registry.get_server("test-host") + + with patch( + "ssh_mcp.ssh.asyncssh.connect", + AsyncMock(side_effect=TimeoutError("connect timed out")), + ): + with caplog.at_level("ERROR", logger="ssh_mcp.ssh"): + with pytest.raises(TimeoutError): + await manager._create_connection(server) + + messages = [r.getMessage() for r in caplog.records if r.name == "ssh_mcp.ssh"] + assert any("Timeout connecting" in m for m in messages), ( + f"Expected 'Timeout connecting' log, got: {messages!r}" + ) + assert not any("OS error connecting" in m for m in messages), ( + f"TimeoutError must not fall through to the OSError branch: {messages!r}" + ) + + async def test_non_timeout_os_error_logs_os_error_message( + self, caplog: pytest.LogCaptureFixture + ) -> None: + """A genuine (non-timeout) OSError, e.g. connection refused, must + still be logged as 'OS error connecting' — the reorder must not + swallow this branch.""" + from unittest.mock import AsyncMock, patch + + manager = SSHManager(self._make_registry(), Settings()) + server = manager.registry.get_server("test-host") + + with patch( + "ssh_mcp.ssh.asyncssh.connect", + AsyncMock(side_effect=ConnectionRefusedError("refused")), + ): + with caplog.at_level("ERROR", logger="ssh_mcp.ssh"): + with pytest.raises(ConnectionRefusedError): + await manager._create_connection(server) + + messages = [r.getMessage() for r in caplog.records if r.name == "ssh_mcp.ssh"] + assert any("OS error connecting" in m for m in messages), ( + f"Expected 'OS error connecting' log, got: {messages!r}" + ) + assert not any("Timeout connecting" in m for m in messages) + + # --------------------------------------------------------------------------- # P8: SFTP size limit enforcement # ---------------------------------------------------------------------------