P0: the catalog review gate is vacuous, and three of the safeguards don't exist in code #68

New Issue

2026-07-12T19:11:47-04:00

the_og commented

2026-07-12 19:11:47 -04:00

Second adversarial review (Fable), this time against the as-built code rather than the design. Every finding below was independently reproduced before filing. The headline: the ceremony is intact and the load-bearing checks are missing.

🔴 1. The Console signs without reviewing anything. This already happened.

catalog_review.can_sign() returns ok=True with zero entries reviewed:

>>> s = start_review("abc123", cat, cat)   # main vs main
>>> can_sign(s, "abc123")
changes: 0 | acknowledged: set()
can_sign with ZERO review -> True

Two bugs compound:

catalog_console.ReviewWindow._on_sign hardcodes main as the TOCTOU comparison ref: blob_sha_at(..., fetch_ref(self.repo_dir, "main"), ...). But _on_load pins the blob of the PR head when a PR is selected. For any PR that changes catalog.json — i.e. every PR the Console exists for — those SHAs differ by definition, so can_sign() always refuses, and the retry path re-pins the same SHA and loops forever. The PR review path cannot sign at all.
So the only path that works is "main (current tip)", where _on_load sets old_catalog = new_catalog. diff_catalogs(x, x) → [] → zero cards rendered → zero risk predicates run → zero registry lookups → all_entries_acknowledged() is set() <= set() → True → Sign enabled instantly.

The workflow this forces is exactly the one the Console was built to prevent: merge the PR, open the Console on main, click Sign. And that is what happened — commit b08cf21 signed all 19 entries through this path. The signature on our catalog right now attests to a review that never occurred.

The per-entry acknowledgement gate is decorative on the only path that reaches sign_catalog_bytes().

Fix: re-fetch the TOCTOU pin from the ref that was reviewed (self._loaded_ref), never hardcoded main. For source=main, diff against the last signed catalog (the blob covered by the current .sig), never against itself — an empty diff must mean "nothing to sign," not "sign unlocked." can_sign() must refuse an empty changeset, and must itself check has_blocking_risk() rather than trusting the GUI to have disabled a checkbox.

🔴 2. `config.env` is completely unvalidated and flows straight into the executed subprocess

Reproduced:

validate_catalog(NODE_OPTIONS=--require /tmp/payload.js) -> ACCEPTED, no problems
catalog_entry_to_paste_json(...) -> {'command': 'npx', 'args': [...], 'env': {'NODE_OPTIONS': '--require /tmp/payload.js'}}

_validate_catalog_config checks env for shape only (is it a dict of strings), while args gets the allowlist, deny-rules, ASCII check and secret check. NODE_OPTIONS, PYTHONSTARTUP, PYTHONPATH, LD_PRELOAD, DYLD_INSERT_LIBRARIES — with an allowlisted command: npx, any of these walks straight past the command allowlist and past the -e/--eval/-c deny-rule that exists to stop precisely this. catalog_review.py has no risk predicate for config.env either, so the Console wouldn't even annotate it.

This is the single most valuable field a malicious PR could touch, and it is the one field nobody looks at.

Fix: deny-list dangerous env keys in _validate_catalog_config, mirror it as a blocking risk predicate, apply the ASCII + secret-looking checks to env keys and values, and require config.env values to be empty or a <PLACEHOLDER>.

🔴 3. Version pinning is not enforced anywhere that runs

validate_catalog(npx -y @scope/pkg)   # no @version
-> ACCEPTED — pinning NOT enforced at runtime

Pinning lives only in catalog_review.risk_unpinned_package() — the Console's risk predicates. But resolve_catalog and the CI gate both call validate_catalog, which doesn't check it. Combined with #1 (the signing path never evaluates the risk predicates at all), nothing in the shipped system enforces pinning. Today's catalog happens to be fully pinned because I pinned it by hand — the next merged PR is where this bites.

Fix: move the pinning check into _validate_catalog_config.

🔴 4. The CI signature gate trusts the public key from the PR it is reviewing

This one is mine — I wrote it. .github/workflows/ci.yml does import bcc_core as c from the checked-out PR branch and verifies against c.CATALOG_PUBKEYS. A PR that changes data/catalog.json and bcc_core.CATALOG_PUBKEYS to an attacker key, signed with the matching private key, produces a green build. The gate's stated threat model is "the maintainer merges a friendly-looking PR without really reading it" — and this is a two-file diff. The placeholder-key check does not help.

Fix: hardcode the expected base64 pubkey in ci.yml and assert CATALOG_PUBKEYS == [expected] before verifying.

🟠 5. The catalog key and the release key are the same key — and it is now in CI

release.yml's smoke test verifies RELEASE_SIGNING_KEY against bcc_core.CATALOG_PUBKEYS[0]. So the offline, passphrase-encrypted, maintainer-held root of trust for the catalog is also sitting in a Gitea Actions secret. Anything that exfiltrates a repo secret (a workflow edit in a PR, a runner compromise) can sign catalogs that every user's binary trusts. catalog_review.py's careful scrypt+AES-GCM at-rest encryption is defeated by show-seed-b64 → paste into CI.

Fix: two separate keys. Release checksums get their own CI-resident key; the catalog key never leaves the Console. The current key must be treated as CI-resident and rotated for catalog use.

🟠 6. `resolve_catalog` — the version cap doesn't apply to the first candidate, and rollback protection is pass-local

Both guards sit behind if best_version >= 0, so the first verified candidate is accepted unconditionally — a signed catalog claiming version: 999999999 sails through if it's evaluated first. And there is no persisted floor, so anti-rollback protects nothing across app restarts. On a version tie, the last candidate wins, so remote silently beats bundled at equal version.

Fix: persist a last_accepted_version floor; apply the cap against it; prefer bundled on ties.

🟡 7. Catalog `id` is unconstrained

Only "non-empty ASCII string". It becomes the mcpServers JSON key and is interpolated into Qt.AutoText widgets (status bar, QMessageBox) in PR #67. <b>Verified</b> renders as markup. UI spoofing, not RCE. Fix: ^[a-z0-9][a-z0-9._-]{0,63}$.

The tests that manufactured confidence

Worth reading as a group, because this is the real lesson:

test_no_acknowledge_all_function_exists — asserts only that no function is named acknowledge_all. It passes today while the acknowledgement gate is entirely bypassable. Pure theatre. I asked for this test.
Every test_can_sign_* passes SHAs in directly, so they all pass while _on_sign feeds the wrong SHA. The bug lives precisely in the untested GUI seam.
test_load_catalog_valid_round_trip uses an unpinned package in its fixture and asserts it validates clean — enshrining #3.
test_catalog_entry_to_paste_json_includes_env_when_present asserts config.env passes through verbatim — locking in #2.
test_resolve_catalog_rejects_absurd_version_jump puts the good catalog first, so it never exercises the uncapped first-candidate path.

374 green tests, and the three checks that decide what actually executes were all missing.

What was actually sound

Worth stating, since the above is grim: verify_catalog_signature is correct (domain-separated, fails closed on garbage, never raises, any-key-matches for rotation). load_catalog is strict-JSON-only with no repair-pipeline contamination anywhere in the catalog path. PR #67's rendering is genuinely clean — Qt.PlainText throughout, and there is no path where an unverified catalog populates the list.

The crypto is fine. The ceremony around the crypto is where the holes are.

Second adversarial review (Fable), this time against the **as-built code** rather than the design. Every finding below was independently reproduced before filing. The headline: **the ceremony is intact and the load-bearing checks are missing.** --- ## 🔴 1. The Console signs without reviewing anything. This already happened. `catalog_review.can_sign()` returns **`ok=True` with zero entries reviewed**: ``` >>> s = start_review("abc123", cat, cat) # main vs main >>> can_sign(s, "abc123") changes: 0 | acknowledged: set() can_sign with ZERO review -> True ``` Two bugs compound: 1. **`catalog_console.ReviewWindow._on_sign` hardcodes `main`** as the TOCTOU comparison ref: `blob_sha_at(..., fetch_ref(self.repo_dir, "main"), ...)`. But `_on_load` pins the blob of the **PR head** when a PR is selected. For any PR that changes `catalog.json` — i.e. every PR the Console exists for — those SHAs differ *by definition*, so `can_sign()` always refuses, and the retry path re-pins the same SHA and loops forever. **The PR review path cannot sign at all.** 2. So the only path that works is "main (current tip)", where `_on_load` sets `old_catalog = new_catalog`. `diff_catalogs(x, x)` → `[]` → zero cards rendered → zero risk predicates run → zero registry lookups → `all_entries_acknowledged()` is `set() <= set()` → **True** → Sign enabled instantly. The workflow this forces is exactly the one the Console was built to prevent: **merge the PR, open the Console on main, click Sign.** And that is what happened — commit `b08cf21` signed all 19 entries through this path. The signature on our catalog right now attests to a review that never occurred. The per-entry acknowledgement gate is **decorative on the only path that reaches `sign_catalog_bytes()`**. **Fix:** re-fetch the TOCTOU pin from *the ref that was reviewed* (`self._loaded_ref`), never hardcoded `main`. For source=main, diff against the last **signed** catalog (the blob covered by the current `.sig`), never against itself — an empty diff must mean "nothing to sign," not "sign unlocked." `can_sign()` must refuse an empty changeset, and must itself check `has_blocking_risk()` rather than trusting the GUI to have disabled a checkbox. ## 🔴 2. `config.env` is completely unvalidated and flows straight into the executed subprocess Reproduced: ``` validate_catalog(NODE_OPTIONS=--require /tmp/payload.js) -> ACCEPTED, no problems catalog_entry_to_paste_json(...) -> {'command': 'npx', 'args': [...], 'env': {'NODE_OPTIONS': '--require /tmp/payload.js'}} ``` `_validate_catalog_config` checks `env` for *shape only* (is it a dict of strings), while `args` gets the allowlist, deny-rules, ASCII check and secret check. `NODE_OPTIONS`, `PYTHONSTARTUP`, `PYTHONPATH`, `LD_PRELOAD`, `DYLD_INSERT_LIBRARIES` — with an allowlisted `command: npx`, any of these walks straight past the command allowlist *and* past the `-e`/`--eval`/`-c` deny-rule that exists to stop precisely this. `catalog_review.py` has no risk predicate for `config.env` either, so the Console wouldn't even annotate it. **This is the single most valuable field a malicious PR could touch, and it is the one field nobody looks at.** **Fix:** deny-list dangerous env keys in `_validate_catalog_config`, mirror it as a blocking risk predicate, apply the ASCII + secret-looking checks to env keys and values, and require `config.env` values to be empty or a `<PLACEHOLDER>`. ## 🔴 3. Version pinning is not enforced anywhere that runs ``` validate_catalog(npx -y @scope/pkg) # no @version -> ACCEPTED — pinning NOT enforced at runtime ``` Pinning lives **only** in `catalog_review.risk_unpinned_package()` — the Console's risk predicates. But `resolve_catalog` and the CI gate both call `validate_catalog`, which doesn't check it. Combined with #1 (the signing path never evaluates the risk predicates at all), **nothing in the shipped system enforces pinning.** Today's catalog happens to be fully pinned because I pinned it by hand — the next merged PR is where this bites. **Fix:** move the pinning check into `_validate_catalog_config`. ## 🔴 4. The CI signature gate trusts the public key from the PR it is reviewing This one is mine — I wrote it. `.github/workflows/ci.yml` does `import bcc_core as c` **from the checked-out PR branch** and verifies against `c.CATALOG_PUBKEYS`. A PR that changes `data/catalog.json` *and* `bcc_core.CATALOG_PUBKEYS` to an attacker key, signed with the matching private key, produces a **green build**. The gate's stated threat model is "the maintainer merges a friendly-looking PR without really reading it" — and this is a two-file diff. The placeholder-key check does not help. **Fix:** hardcode the expected base64 pubkey in `ci.yml` and assert `CATALOG_PUBKEYS == [expected]` before verifying. ## 🟠 5. The catalog key and the release key are the same key — and it is now in CI `release.yml`'s smoke test verifies `RELEASE_SIGNING_KEY` against `bcc_core.CATALOG_PUBKEYS[0]`. So the offline, passphrase-encrypted, maintainer-held **root of trust for the catalog is also sitting in a Gitea Actions secret.** Anything that exfiltrates a repo secret (a workflow edit in a PR, a runner compromise) can sign catalogs that every user's binary trusts. `catalog_review.py`'s careful scrypt+AES-GCM at-rest encryption is defeated by `show-seed-b64` → paste into CI. **Fix:** two separate keys. Release checksums get their own CI-resident key; the catalog key never leaves the Console. **The current key must be treated as CI-resident and rotated for catalog use.** ## 🟠 6. `resolve_catalog` — the version cap doesn't apply to the first candidate, and rollback protection is pass-local Both guards sit behind `if best_version >= 0`, so the **first** verified candidate is accepted unconditionally — a signed catalog claiming `version: 999999999` sails through if it's evaluated first. And there is no persisted floor, so anti-rollback protects nothing across app restarts. On a version tie, the *last* candidate wins, so remote silently beats bundled at equal version. **Fix:** persist a `last_accepted_version` floor; apply the cap against it; prefer bundled on ties. ## 🟡 7. Catalog `id` is unconstrained Only "non-empty ASCII string". It becomes the `mcpServers` JSON key *and* is interpolated into `Qt.AutoText` widgets (status bar, QMessageBox) in PR #67. `<b>Verified</b>` renders as markup. UI spoofing, not RCE. Fix: `^[a-z0-9][a-z0-9._-]{0,63}$`. --- ## The tests that manufactured confidence Worth reading as a group, because this is the real lesson: - **`test_no_acknowledge_all_function_exists`** — asserts only that no function is *named* `acknowledge_all`. It passes today while the acknowledgement gate is entirely bypassable. **Pure theatre.** I asked for this test. - Every `test_can_sign_*` passes SHAs in directly, so they all pass while `_on_sign` feeds the wrong SHA. The bug lives precisely in the untested GUI seam. - `test_load_catalog_valid_round_trip` uses an **unpinned** package in its fixture and asserts it validates clean — enshrining #3. - `test_catalog_entry_to_paste_json_includes_env_when_present` asserts `config.env` passes through verbatim — locking in #2. - `test_resolve_catalog_rejects_absurd_version_jump` puts the good catalog first, so it never exercises the uncapped first-candidate path. 374 green tests, and the three checks that decide what actually executes were all missing. ## What was actually sound Worth stating, since the above is grim: `verify_catalog_signature` is correct (domain-separated, fails closed on garbage, never raises, any-key-matches for rotation). `load_catalog` is strict-JSON-only with **no** repair-pipeline contamination anywhere in the catalog path. PR #67's rendering is genuinely clean — `Qt.PlainText` throughout, and there is no path where an unverified catalog populates the list. The crypto is fine. **The ceremony around the crypto is where the holes are.**

the_og added the P0 label 2026-07-12 19:11:47 -04:00

the_og referenced this issue from a commit

2026-07-12 21:25:40 -04:00

fix(catalog-console): close the vacuous review gate; split catalog/release signing keys

the_og referenced a pull request that will close this issue

2026-07-12 21:26:12 -04:00

fix(catalog-console): close the vacuous review gate; split catalog/release signing keys (#68 findings 1, 5) #69

the_og referenced this issue from a commit

2026-07-12 21:27:13 -04:00

fix(core): validate config.env, enforce version pinning, fix CI trust anchor and resolve_catalog guards (#68)

the_og referenced this issue

2026-07-12 21:27:39 -04:00