Merge branch 'main' into feat/10-browse-dialog

tests/test_catalog_review.py

@@ -334,13 +334,29 @@ def test_acknowledge_gating_requires_every_entry():
     assert r.all_entries_acknowledged(session) is True
 
 
-def test_no_acknowledge_all_function_exists():
-    """Deliberate: there must be no shortcut to acknowledge every entry at
-    once. See the comment in catalog_review.py above SignDecision."""
+def test_no_acknowledge_all_shortcut_and_gate_is_real():
+    """Two things, both load-bearing (issue #68: the original version of
+    this test asserted ONLY the first half, and passed the entire time the
+    gate below it was vacuously satisfiable -- 'no function named
+    acknowledge_all' is worthless if signing doesn't actually require
+    acknowledgement in practice).
+
+    1. No bulk-acknowledge shortcut exists (see the comment in
+       catalog_review.py above SignDecision -- deliberate friction).
+    2. The gate that friction protects is actually enforced: with entries
+       still unacknowledged, can_sign() must refuse, not just "some GUI
+       checkbox happens to be unticked".
+    """
     names = [n for n in dir(r) if "acknowledge" in n.lower()]
     assert "acknowledge_all" not in names
     assert "acknowledge_all_entries" not in names
 
+    session = r.start_review("sha1", _catalog(), _catalog(_entry(id="a"), _entry(id="b")))
+    r.acknowledge_entry(session, "a")  # only one of two -- not a bulk call
+    decision = r.can_sign(session, "sha1")
+    assert decision.ok is False
+    assert "acknowledged" in decision.reason.lower()
+
 
 # --------------------------------------------------------------------------- #
 # can_sign: TOCTOU blob pinning + acknowledge gating combined
@@ -377,6 +393,130 @@ def test_can_sign_blob_mismatch_takes_priority_message():
     assert "blob" in decision.reason.lower() or "changed" in decision.reason.lower()
 
 
+def test_can_sign_false_on_empty_changeset():
+    """The exact bug behind issue #68 finding 1: 'main' loaded against
+    itself diffs to [], and an empty changeset used to leave can_sign()
+    with nothing to refuse on (set() <= set() is vacuously True). Commit
+    b08cf21 signed 19 entries through precisely this path -- zero of them
+    were ever reviewed. An empty diff must mean 'nothing to sign', never
+    'sign unlocked'."""
+    same_catalog = _catalog(_entry())
+    session = r.start_review("sha1", same_catalog, same_catalog)
+    assert session.changes == []  # diff_catalogs(x, x) -> []
+    assert r.all_entries_acknowledged(session) is True  # vacuously -- this is the trap
+    decision = r.can_sign(session, "sha1")  # blob matches, "everything" acknowledged
+    assert decision.ok is False
+    assert "nothing to sign" in decision.reason.lower()
+
+
+def test_can_sign_false_with_outstanding_blocking_risk_even_if_acknowledged():
+    """can_sign() must itself refuse a blocking risk finding -- today a
+    blocking finding only disables the GUI checkbox, so the pure gate must
+    not simply trust that the caller never acknowledged a blocking entry.
+    Acknowledge it directly here (bypassing any GUI checkbox-disable logic
+    entirely) to prove the gate catches it independently of the GUI."""
+    session = r.start_review(
+        "sha1",
+        _catalog(),
+        _catalog(_entry(config={"command": "bash", "args": ["-c", "evil"]})),
+    )
+    r.acknowledge_entry(session, "filesystem")
+    assert r.all_entries_acknowledged(session) is True
+    decision = r.can_sign(session, "sha1")
+    assert decision.ok is False
+    assert "blocking" in decision.reason.lower()
+
+
+# --------------------------------------------------------------------------- #
+# sign_precondition: the ref-resolution seam that used to hardcode "main"
+# --------------------------------------------------------------------------- #
+def test_sign_precondition_resolves_against_loaded_ref_not_hardcoded_main():
+    """The regression test for issue #68 finding 1's first bug:
+    ReviewWindow._on_sign used to hardcode fetch_ref(repo, "main") as the
+    TOCTOU comparison ref. For a PR review, _on_load pins the PR HEAD's
+    blob SHA, so comparing against main's SHA differs by definition and
+    Sign could never fire on the PR path.
+
+    The fake resolver below returns a DIFFERENT (deliberately wrong) SHA for
+    "main" than for the PR ref that was actually loaded. If
+    sign_precondition ever resolves against "main" instead of
+    session.loaded_ref, this test fails -- both via the recorded `calls`
+    list and via decision.ok flipping to False.
+    """
+    pr_ref = "refs/pull/42/head"
+    session = r.start_review("pr-blob-sha", _catalog(), _catalog(_entry()), loaded_ref=pr_ref)
+    r.acknowledge_entry(session, "filesystem")
+
+    calls: list[str] = []
+
+    def fake_resolver(ref: str) -> str:
+        calls.append(ref)
+        return {"main": "main-blob-sha-WRONG", pr_ref: "pr-blob-sha"}[ref]
+
+    decision = r.sign_precondition(session, fake_resolver)
+    assert calls == [pr_ref]  # never asked the resolver for "main"
+    assert decision.ok is True
+    assert decision.reason is None
+
+
+def test_sign_precondition_refuses_when_loaded_ref_blob_moved():
+    """Same seam, the negative case: if the loaded ref's blob SHA has moved
+    since review began (a new commit landed on the reviewed PR/branch), the
+    resolver reflects that and sign_precondition must refuse -- proving this
+    isn't just a hardcoded pass-through."""
+    pr_ref = "refs/pull/42/head"
+    session = r.start_review("pr-blob-sha", _catalog(), _catalog(_entry()), loaded_ref=pr_ref)
+    r.acknowledge_entry(session, "filesystem")
+
+    def fake_resolver(_ref: str) -> str:
+        return "pr-blob-sha-AFTER-A-NEW-PUSH"
+
+    decision = r.sign_precondition(session, fake_resolver)
+    assert decision.ok is False
+    assert "mismatch" in decision.reason.lower() or "changed" in decision.reason.lower()
+
+
+def test_sign_precondition_defaults_to_main_when_loaded_ref_unset():
+    """start_review()'s loaded_ref defaults to 'main' for source=main
+    reviews (and backward-compat with callers that don't pass it)."""
+    session = r.start_review("sha1", _catalog(), _catalog(_entry()))
+    assert session.loaded_ref == "main"
+    r.acknowledge_entry(session, "filesystem")
+
+    def fake_resolver(ref: str) -> str:
+        assert ref == "main"
+        return "sha1"
+
+    decision = r.sign_precondition(session, fake_resolver)
+    assert decision.ok is True
+
+
+# --------------------------------------------------------------------------- #
+# find_last_signed_catalog_raw: what source=main diffs against
+# --------------------------------------------------------------------------- #
+def test_find_last_signed_catalog_raw_returns_matching_candidate():
+    """Simulates walking catalog.json's git history: the CURRENT signature
+    covers an OLDER version of the bytes (a later commit changed
+    catalog.json without re-signing -- the exact bypass that produced
+    commit b08cf21). The first candidate that verifies against that
+    signature is 'the last catalog a maintainer actually signed'."""
+    seed, pubkey = r.generate_keypair()
+    old_raw = b'{"schema":1,"version":1,"servers":[]}'
+    new_raw = b'{"schema":1,"version":2,"servers":[]}'
+    sig = r.sign_catalog_bytes(old_raw, seed)  # signature covers the OLD bytes
+    found = r.find_last_signed_catalog_raw([new_raw, old_raw], sig, [pubkey])
+    assert found == old_raw
+
+
+def test_find_last_signed_catalog_raw_none_when_nothing_verifies():
+    seed, _pubkey = r.generate_keypair()
+    _other_seed, other_pubkey = r.generate_keypair()
+    raw = b'{"schema":1,"version":1,"servers":[]}'
+    sig = r.sign_catalog_bytes(raw, seed)
+    # Check against a pubkey list that does NOT include the signer's key.
+    assert r.find_last_signed_catalog_raw([raw], sig, [other_pubkey]) is None
+
+
 # --------------------------------------------------------------------------- #
 # catalog_signing_message: domain separation must match bcc_core exactly
 # --------------------------------------------------------------------------- #

tests/test_core.py

@@ -1690,8 +1690,12 @@ def _minimal_catalog(version: int = 1) -> dict:
                 "official": True,
                 "setup": "basic",
                 "config": {
+                    # Pinned on purpose (issue #68 finding 3): an earlier
+                    # version of this fixture used an unpinned package and
+                    # asserted it validated clean, which enshrined the bug
+                    # instead of catching it.
                     "command": "npx",
-                    "args": ["-y", "widget-mcp"],
+                    "args": ["-y", "widget-mcp@1.0.0"],
                 },
                 "placeholders": {},
                 "env_required": {},
@@ -1952,6 +1956,213 @@ def test_validate_catalog_rejects_duplicate_ids():
     assert any("duplicate id" in p for p in problems)
 
 
+# --- validate_catalog: config.env (issue #68 finding 2) -------------------- #
+def test_validate_catalog_rejects_each_denied_env_key():
+    for key in sorted(c.CATALOG_DENIED_ENV_KEYS):
+        data = _catalog_with(
+            {"config": {"command": "npx", "args": ["-y", "widget-mcp@1.0.0"], "env": {key: ""}}}
+        )
+        problems = c.validate_catalog(data)
+        assert any("deny-list" in p for p in problems), (key, problems)
+
+
+def test_validate_catalog_rejects_denied_env_key_case_insensitively():
+    data = _catalog_with(
+        {
+            "config": {
+                "command": "npx",
+                "args": ["-y", "widget-mcp@1.0.0"],
+                "env": {"node_options": ""},
+            }
+        }
+    )
+    problems = c.validate_catalog(data)
+    assert any("deny-list" in p for p in problems)
+
+
+def test_validate_catalog_rejects_nonempty_nonplaceholder_env_value():
+    data = _catalog_with(
+        {
+            "config": {
+                "command": "npx",
+                "args": ["-y", "widget-mcp@1.0.0"],
+                "env": {"FOO_URL": "https://example.com"},
+            }
+        }
+    )
+    problems = c.validate_catalog(data)
+    assert any("empty string or a single <PLACEHOLDER>" in p for p in problems)
+
+
+def test_validate_catalog_accepts_empty_and_placeholder_env_values():
+    data = _catalog_with(
+        {
+            "config": {
+                "command": "npx",
+                "args": ["-y", "widget-mcp@1.0.0"],
+                "env": {"FOO": "", "BAR_URL": "<BAR_URL>"},
+            }
+        }
+    )
+    assert c.validate_catalog(data) == []
+
+
+def test_validate_catalog_rejects_non_ascii_env_key():
+    data = _catalog_with(
+        {
+            "config": {
+                "command": "npx",
+                "args": ["-y", "widget-mcp@1.0.0"],
+                "env": {"FÖO": ""},
+            }
+        }
+    )
+    problems = c.validate_catalog(data)
+    assert any("config.env key" in p and "ASCII" in p for p in problems)
+
+
+def test_validate_catalog_rejects_non_ascii_env_value():
+    data = _catalog_with(
+        {
+            "config": {
+                "command": "npx",
+                "args": ["-y", "widget-mcp@1.0.0"],
+                "env": {"FOO": "<Bäd>"},
+            }
+        }
+    )
+    problems = c.validate_catalog(data)
+    assert any("config.env value" in p and "ASCII" in p for p in problems)
+
+
+def test_validate_catalog_rejects_secret_looking_env_value():
+    data = _catalog_with(
+        {
+            "config": {
+                "command": "npx",
+                "args": ["-y", "widget-mcp@1.0.0"],
+                "env": {"SOME_TOKEN": "ghp_abcdef1234567890"},
+            }
+        }
+    )
+    problems = c.validate_catalog(data)
+    assert any("real secret value" in p for p in problems)
+
+
+def test_validate_catalog_rejects_node_options_env_walking_past_allowlist():
+    # The exact reproduction from issue #68 finding 2: an allowlisted
+    # `npx` command carrying NODE_OPTIONS in env, which previously passed
+    # validation and would have flowed straight into the executed
+    # subprocess via catalog_entry_to_paste_json().
+    data = _catalog_with(
+        {
+            "config": {
+                "command": "npx",
+                "args": ["-y", "widget-mcp@1.0.0"],
+                "env": {"NODE_OPTIONS": "--require /tmp/payload.js"},
+            }
+        }
+    )
+    problems = c.validate_catalog(data)
+    assert problems != []
+
+
+# --- validate_catalog: version pinning (issue #68 finding 3) --------------- #
+def test_validate_catalog_rejects_unpinned_npx_package():
+    data = _catalog_with({"config": {"command": "npx", "args": ["-y", "widget-mcp"]}})
+    problems = c.validate_catalog(data)
+    assert any("not version-pinned" in p for p in problems)
+
+
+def test_validate_catalog_rejects_unpinned_scoped_npx_package():
+    data = _catalog_with({"config": {"command": "npx", "args": ["-y", "@scope/pkg"]}})
+    problems = c.validate_catalog(data)
+    assert any("not version-pinned" in p for p in problems)
+
+
+def test_validate_catalog_accepts_pinned_scoped_npx_package():
+    data = _catalog_with({"config": {"command": "npx", "args": ["-y", "@scope/pkg@1.2.3"]}})
+    assert c.validate_catalog(data) == []
+
+
+def test_validate_catalog_rejects_unpinned_uvx_package():
+    data = _catalog_with({"config": {"command": "uvx", "args": ["some-tool"]}})
+    problems = c.validate_catalog(data)
+    assert any("not version-pinned" in p for p in problems)
+
+
+def test_validate_catalog_accepts_uvx_at_version_pin():
+    data = _catalog_with({"config": {"command": "uvx", "args": ["some-tool@1.0.0"]}})
+    assert c.validate_catalog(data) == []
+
+
+def test_validate_catalog_accepts_uvx_double_equals_pin():
+    data = _catalog_with({"config": {"command": "uvx", "args": ["some-tool==1.0.0"]}})
+    assert c.validate_catalog(data) == []
+
+
+def test_validate_catalog_rejects_docker_latest_tag():
+    data = _catalog_with({"config": {"command": "docker", "args": ["run", "some/image:latest"]}})
+    problems = c.validate_catalog(data)
+    assert any("'latest'" in p for p in problems)
+
+
+def test_validate_catalog_rejects_docker_untagged_image():
+    data = _catalog_with({"config": {"command": "docker", "args": ["run", "some/image"]}})
+    problems = c.validate_catalog(data)
+    assert any("no explicit tag" in p for p in problems)
+
+
+def test_validate_catalog_accepts_pinned_docker_image_with_flags():
+    data = _catalog_with(
+        {
+            "config": {
+                "command": "docker",
+                "args": ["run", "-i", "--rm", "-e", "SOME_TOKEN", "some/image:1.2.3"],
+            }
+        }
+    )
+    assert c.validate_catalog(data) == []
+
+
+def test_validate_catalog_does_not_pin_check_placeholders_flags_or_subcommand():
+    # A pinned uvx spec followed by flags and a <PLACEHOLDER> positional
+    # must not itself get mistaken for an unpinned package.
+    data = _catalog_with(
+        {
+            "config": {
+                "command": "uvx",
+                "args": ["mcp-server-git@2026.7.10", "--repository", "<REPO_PATH>"],
+            }
+        }
+    )
+    assert c.validate_catalog(data) == []
+
+
+# --- validate_catalog: id constraint (issue #68 finding 7) ----------------- #
+def test_validate_catalog_rejects_id_with_markup():
+    data = _catalog_with({"id": "<b>Verified</b>"})
+    problems = c.validate_catalog(data)
+    assert any("must match" in p for p in problems)
+
+
+def test_validate_catalog_rejects_id_with_uppercase():
+    data = _catalog_with({"id": "Widget"})
+    problems = c.validate_catalog(data)
+    assert any("must match" in p for p in problems)
+
+
+def test_validate_catalog_rejects_id_starting_with_dash():
+    data = _catalog_with({"id": "-widget"})
+    problems = c.validate_catalog(data)
+    assert any("must match" in p for p in problems)
+
+
+def test_validate_catalog_accepts_valid_slug_id():
+    data = _catalog_with({"id": "widget-2.thing-ok"})
+    assert c.validate_catalog(data) == []
+
+
 # --- resolve_catalog -------------------------------------------------------- #
 def test_resolve_catalog_nothing_available_returns_empty_dict():
     assert c.resolve_catalog(None, None, None) == {}
@@ -2003,13 +2214,93 @@ def test_resolve_catalog_rejects_absurd_version_jump(monkeypatch):
     pub = priv.public_key().public_bytes_raw()
     monkeypatch.setattr(c, "CATALOG_PUBKEYS", [pub])
 
-    cached = _signed(_minimal_catalog(version=5), priv)
+    # The freeze attempt goes FIRST (as `cached`), the legitimate catalog
+    # SECOND (as `remote`) -- on purpose. Putting the good catalog first
+    # (as an earlier version of this test did) never exercises the
+    # vulnerable path: the old implementation only guarded a candidate
+    # against "the best accepted so far," so whichever candidate was
+    # evaluated FIRST got in unconditionally, uncapped. Ordering the freeze
+    # attempt first is what actually proves the cap holds regardless of
+    # evaluation order.
     freeze_attempt = _signed(_minimal_catalog(version=999999), priv)
+    good = _signed(_minimal_catalog(version=5), priv)
 
-    result = c.resolve_catalog(None, cached, freeze_attempt)
+    result = c.resolve_catalog(None, freeze_attempt, good)
     assert c.catalog_version(result) == 5
 
 
+def test_resolve_catalog_caps_first_and_only_candidate(monkeypatch):
+    # issue #68 finding 6: with no bundled catalog to anchor against, a
+    # signed catalog claiming an absurd version must still be capped even
+    # when it is the ONLY candidate resolve_catalog() ever sees -- there is
+    # no "best so far" for it to be compared against, so the cap has to
+    # apply unconditionally, not "once something else has already landed."
+    priv = Ed25519PrivateKey.generate()
+    pub = priv.public_key().public_bytes_raw()
+    monkeypatch.setattr(c, "CATALOG_PUBKEYS", [pub])
+
+    freeze_attempt = _signed(_minimal_catalog(version=999999999), priv)
+
+    result = c.resolve_catalog(None, None, freeze_attempt)
+    assert result == {}
+
+
+def test_resolve_catalog_anchors_cap_to_bundled_not_a_chained_best(monkeypatch):
+    # Anti-freeze must be measured against the BUNDLED version specifically,
+    # not against "whatever the best-so-far happens to be after each
+    # candidate is accepted" -- a chained anchor lets each accepted
+    # candidate ratchet the allowed ceiling upward, so a legitimate
+    # moderate bump (cached) plus a second, much larger jump (remote) can
+    # each individually look "within _CATALOG_MAX_VERSION_JUMP of the
+    # previous one" while remote is nowhere near bundled's version.
+    priv = Ed25519PrivateKey.generate()
+    pub = priv.public_key().public_bytes_raw()
+    monkeypatch.setattr(c, "CATALOG_PUBKEYS", [pub])
+
+    bundled = _signed(_minimal_catalog(version=2), priv)
+    cached = _signed(_minimal_catalog(version=1000), priv)  # within 1000 of bundled
+    remote = _signed(_minimal_catalog(version=1900), priv)  # within 1000 of cached,
+    #                                                          NOT of bundled
+
+    result = c.resolve_catalog(bundled, cached, remote)
+    assert c.catalog_version(result) == 1000
+
+
+def test_resolve_catalog_prefers_bundled_on_version_tie(monkeypatch):
+    # issue #68 finding 6: on a tie the LAST candidate evaluated used to
+    # win, so remote silently beat bundled at equal version. Bundled --
+    # the copy frozen into the binary -- must win ties.
+    priv = Ed25519PrivateKey.generate()
+    pub = priv.public_key().public_bytes_raw()
+    monkeypatch.setattr(c, "CATALOG_PUBKEYS", [pub])
+
+    bundled_data = _minimal_catalog(version=5)
+    bundled = _signed(bundled_data, priv)
+
+    remote_data = _minimal_catalog(version=5)
+    remote_data["servers"][0]["display"] = "Remote Impostor"
+    remote = _signed(remote_data, priv)
+
+    result = c.resolve_catalog(bundled, None, remote)
+    assert c.catalog_version(result) == 5
+    assert result["servers"][0]["display"] == "Widget"
+
+
+def test_resolve_catalog_floor_rejects_below_persisted_version(monkeypatch):
+    # `floor` is a pure parameter: the caller (eventually the GUI, from
+    # persisted storage) can pass a previously-accepted version, and
+    # nothing below it may be accepted even with no bundled catalog to
+    # anchor against.
+    priv = Ed25519PrivateKey.generate()
+    pub = priv.public_key().public_bytes_raw()
+    monkeypatch.setattr(c, "CATALOG_PUBKEYS", [pub])
+
+    stale = _signed(_minimal_catalog(version=3), priv)
+
+    result = c.resolve_catalog(None, None, stale, floor=10)
+    assert result == {}
+
+
 def test_resolve_catalog_malformed_candidate_does_not_raise(monkeypatch):
     priv = Ed25519PrivateKey.generate()
     pub = priv.public_key().public_bytes_raw()
@@ -2039,15 +2330,32 @@ def test_resolve_catalog_invalid_but_signed_candidate_is_skipped(monkeypatch):
 def test_catalog_entry_to_paste_json_basic_shape():
     entry = _minimal_catalog()["servers"][0]
     result = c.catalog_entry_to_paste_json(entry)
-    assert result == {"widget": {"command": "npx", "args": ["-y", "widget-mcp"]}}
+    assert result == {"widget": {"command": "npx", "args": ["-y", "widget-mcp@1.0.0"]}}
 
 
 def test_catalog_entry_to_paste_json_includes_env_when_present():
+    # NOTE: catalog_entry_to_paste_json() is a pure shape-converter for an
+    # entry that has ALREADY passed validate_catalog() -- it is correct for
+    # it to carry env through verbatim. The bug (issue #68 finding 2) was
+    # never in this function; it was that validate_catalog() let entries
+    # with dangerous/non-placeholder env values reach this function in the
+    # first place. This test now proves that boundary explicitly: a
+    # validation-legal env value (a <PLACEHOLDER> token) survives the
+    # conversion, and a value validate_catalog() would have rejected is
+    # confirmed rejected before it ever gets here.
     entry = _minimal_catalog()["servers"][0]
     entry["config"]["env"] = {"GRAFANA_URL": "<GRAFANA_URL>"}
     result = c.catalog_entry_to_paste_json(entry)
     assert result["widget"]["env"] == {"GRAFANA_URL": "<GRAFANA_URL>"}
 
+    catalog = _minimal_catalog()
+    catalog["servers"][0]["config"]["env"] = {"GRAFANA_URL": "<GRAFANA_URL>"}
+    assert c.validate_catalog(catalog) == []
+
+    malicious = _minimal_catalog()
+    malicious["servers"][0]["config"]["env"] = {"NODE_OPTIONS": "--require /tmp/payload.js"}
+    assert c.validate_catalog(malicious) != []
+
 
 def test_catalog_entry_to_paste_json_seeds_env_required_keys():
     # Regression: env_required is where the seed data actually keeps its