From ea54f6bc068442547ccf9c886354c5df10bf64d7 Mon Sep 17 00:00:00 2001 From: blackax Date: Sat, 25 Jul 2026 22:33:00 -0700 Subject: [PATCH 1/2] ci: fix publish-path digest verification and restore provenance attestation `docker buildx imagetools create` wraps the scanned manifest in a new OCI index rather than reusing its digest, so comparing the tag's own digest against steps.build.outputs.digest compared an index to one of its children and failed on every push to main. Because the check sits before the attestation step, images published since 2026-07-26 carry no provenance. - Verify the set of image manifests a tag serves is exactly {scanned digest}, ignoring attestation manifests, instead of hashing raw manifest bytes. - Confirmed against the live registry: :latest was index sha256:8204263e whose sole child was the scanned sha256:12db37ce. - Regression test fails on the previous workflow and passes on this one. The Trivy gate was never bypassed: the scan runs against the digest before any tag is promoted. --- .github/workflows/ci.yml | 66 ++++++++++++++++++++++--------- tests/test_ci_lint_determinism.py | 62 +++++++++++++++++++++++++++++ 2 files changed, 109 insertions(+), 19 deletions(-) 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/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(), ( From cabe9dae601f46d2a0bfd5cf3b50a69d0491286d Mon Sep 17 00:00:00 2001 From: blackax Date: Sat, 25 Jul 2026 22:50:46 -0700 Subject: [PATCH 2/2] fix(security): redact dash-prefixed credentials, refuse *.* allowed_hosts, cut 0.6.0 Two credential/network security defects found while preparing the release, both pre-existing on main: - _redact_secrets left a space-separated credential value unredacted when it began with "-" (mysql --password -Xy9$kL2mQp leaked verbatim). The guard treating a following dash token as "this flag was boolean" is narrowed from "-" to "--", which keeps the earlier skip-leak fix intact. A value beginning with "--" remains a documented residual. - SSH_MCP_HTTP_ALLOWED_HOSTS=*.* was accepted despite being in the refusal set: the suffix-wildcard allowance was evaluated first. Validation now strips only the two permitted wildcard forms and demands a concrete remainder, which also refuses infix wildcards and whitespace-only values. Also: reorder except asyncio.TimeoutError before except OSError, where it was unreachable dead code on Python >=3.11, restoring the specific timeout message; make test_fuzzed_long_flag_junk_prefix_is_unbounded assert exact output so a secret that is a substring of the flag name no longer reports a false leak; bump to 0.6.0 and close the CHANGELOG entry. --- CHANGELOG.md | 11 +- src/ssh_mcp/__init__.py | 2 +- src/ssh_mcp/server.py | 48 ++++++- src/ssh_mcp/ssh.py | 46 ++++++- tests/test_http_transport.py | 77 +++++++++++ tests/test_ssh.py | 248 +++++++++++++++++++++++++++++++++-- 6 files changed, 408 insertions(+), 24 deletions(-) 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_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 # ---------------------------------------------------------------------------