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
_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 cfrom the checked-out PR branch and verifies against c.CATALOG_PUBKEYS. A PR that changes data/catalog.jsonandbcc_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 namedacknowledge_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
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
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()returnsok=Truewith zero entries reviewed:Two bugs compound:
catalog_console.ReviewWindow._on_signhardcodesmainas the TOCTOU comparison ref:blob_sha_at(..., fetch_ref(self.repo_dir, "main"), ...). But_on_loadpins the blob of the PR head when a PR is selected. For any PR that changescatalog.json— i.e. every PR the Console exists for — those SHAs differ by definition, socan_sign()always refuses, and the retry path re-pins the same SHA and loops forever. The PR review path cannot sign at all._on_loadsetsold_catalog = new_catalog.diff_catalogs(x, x)→[]→ zero cards rendered → zero risk predicates run → zero registry lookups →all_entries_acknowledged()isset() <= 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
b08cf21signed 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 hardcodedmain. 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 checkhas_blocking_risk()rather than trusting the GUI to have disabled a checkbox.🔴 2.
config.envis completely unvalidated and flows straight into the executed subprocessReproduced:
_validate_catalog_configchecksenvfor shape only (is it a dict of strings), whileargsgets the allowlist, deny-rules, ASCII check and secret check.NODE_OPTIONS,PYTHONSTARTUP,PYTHONPATH,LD_PRELOAD,DYLD_INSERT_LIBRARIES— with an allowlistedcommand: npx, any of these walks straight past the command allowlist and past the-e/--eval/-cdeny-rule that exists to stop precisely this.catalog_review.pyhas no risk predicate forconfig.enveither, 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 requireconfig.envvalues to be empty or a<PLACEHOLDER>.🔴 3. Version pinning is not enforced anywhere that runs
Pinning lives only in
catalog_review.risk_unpinned_package()— the Console's risk predicates. Butresolve_catalogand the CI gate both callvalidate_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.ymldoesimport bcc_core as cfrom the checked-out PR branch and verifies againstc.CATALOG_PUBKEYS. A PR that changesdata/catalog.jsonandbcc_core.CATALOG_PUBKEYSto 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.ymland assertCATALOG_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 verifiesRELEASE_SIGNING_KEYagainstbcc_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 byshow-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-localBoth guards sit behind
if best_version >= 0, so the first verified candidate is accepted unconditionally — a signed catalog claimingversion: 999999999sails 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_versionfloor; apply the cap against it; prefer bundled on ties.🟡 7. Catalog
idis unconstrainedOnly "non-empty ASCII string". It becomes the
mcpServersJSON key and is interpolated intoQt.AutoTextwidgets (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 namedacknowledge_all. It passes today while the acknowledgement gate is entirely bypassable. Pure theatre. I asked for this test.test_can_sign_*passes SHAs in directly, so they all pass while_on_signfeeds the wrong SHA. The bug lives precisely in the untested GUI seam.test_load_catalog_valid_round_tripuses an unpinned package in its fixture and asserts it validates clean — enshrining #3.test_catalog_entry_to_paste_json_includes_env_when_presentassertsconfig.envpasses through verbatim — locking in #2.test_resolve_catalog_rejects_absurd_version_jumpputs 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_signatureis correct (domain-separated, fails closed on garbage, never raises, any-key-matches for rotation).load_catalogis strict-JSON-only with no repair-pipeline contamination anywhere in the catalog path. PR #67's rendering is genuinely clean —Qt.PlainTextthroughout, 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.