Fixes findings 2, 3, 4, 6, 7 from #68 (the second adversarial review). Scope limited to bcc_core.py, tests/test_core.py, .github/workflows/ci.yml per the issue — findings 1 and 5 (Console signing flow, key separation) are out of scope for this PR and are being handled separately.
CATALOG_DENIED_ENV_KEYS (module-level frozenset, importable by the Console): NODE_OPTIONS, PYTHONSTARTUP, PYTHONPATH, PYTHONHOME, LD_PRELOAD, LD_LIBRARY_PATH, DYLD_INSERT_LIBRARIES, DYLD_LIBRARY_PATH, BROWSER, PATH, NODE_REPL_EXTERNAL_MODULE. Matched case-insensitively.
Same ASCII check args gets, applied to env keys and values.
Existing _is_secret_value check applied to env values.
config.env values must be "" or a single <PLACEHOLDER> token — blocking otherwise.
Reproduced and confirmed blocked: {"command": "npx", "env": {"NODE_OPTIONS": "--require /tmp/x.js"}} now fails validate_catalog.
Finding 3 — version pinning not enforced at runtime
Moved enforcement from catalog_review.py (maintainer-only, never runs per finding 1) into _validate_catalog_config, blocking:
npm specs need exact @version (scoped @scope/pkg@1.2.3 handled correctly — right-to-left parse so the scope's leading @ isn't mistaken for the version separator).
uvx/PyPI specs need @version or ==version.
docker images need an explicit tag, and it must not be latest.
Only the first plausible package-spec token is checked (flags, <PLACEHOLDER>s, and docker's run/-i/--rm/-e KEY are correctly skipped). All 19 entries in the real data/catalog.json still validate clean (test_shipped_catalog_json_passes_validation — unedited, still green).
Finding 4 — CI gate trusted the pubkey from the PR it reviewed
ci.yml's catalog-signature job now hardcodes EXPECTED_CATALOG_PUBKEY_B64 and asserts bcc_core.CATALOG_PUBKEYS (as loaded from the PR branch) matches it, before verifying the signature. Comment on that line calls out that it's a trust anchor and must be reviewed on its own, never bundled with a catalog content change.
Verified the actual attack in a sandbox: cloned the repo, had an "attacker" generate their own Ed25519 keypair, re-signed data/catalog.json with it, and swapped bcc_core.CATALOG_PUBKEYS to the attacker's key (the exact two-file diff from the finding) — the extracted CI check now exits 1 with a clear message instead of going green.
Note for the maintainer: per the issue, the current key (082NOwVB7uURkvfyS3+knJ+40Fk6C9unsF47+2uPKo4=) is about to be rotated. EXPECTED_CATALOG_PUBKEY_B64 in ci.yml is a single obvious line specifically so that update is trivial — please review it as its own commit when you rotate.
Finding 6 — resolve_catalog guards skipped the first candidate
Both guards sat behind if best_version >= 0, so the first verified candidate (bundled, cached, or remote — whichever landed first) was accepted unconditionally, and the anti-freeze ceiling drifted upward with each accepted candidate instead of staying fixed (a legitimate moderate bump could "carry" a much larger illegitimate jump right behind it).
The anti-freeze cap is now measured against the bundled catalog's verified version specifically — the trust anchor baked into the binary — independent of iteration order.
Bundled now wins version ties (previously the last-evaluated candidate won ties unconditionally).
Added a pure floor: int = 0 parameter so a future caller can pass a persisted accepted-version floor; no storage wired up in this PR, as scoped.
Finding 7 — catalog id unconstrained
Constrained to ^[a-z0-9][a-z0-9._-]{0,63}$.
Tests
Fixed _minimal_catalog/test_load_catalog_valid_round_trip — the fixture used an unpinned package and asserted it validated clean (enshrined finding 3). Now pinned; added a separate test asserting the unpinned form is rejected.
Rewrote test_catalog_entry_to_paste_json_includes_env_when_present — it asserted env passes through verbatim without checking validate_catalog first rejects the dangerous case (enshrined finding 2).
Reordered test_resolve_catalog_rejects_absurd_version_jump so the freeze attempt is evaluated first, which is what actually exercises the vulnerable path; added test_resolve_catalog_caps_first_and_only_candidate, test_resolve_catalog_anchors_cap_to_bundled_not_a_chained_best, test_resolve_catalog_prefers_bundled_on_version_tie, test_resolve_catalog_floor_rejects_below_persisted_version.
Added positive/negative tests for every new rule (each denied env key, case-insensitive match, non-ASCII env key/value, secret-looking env value, unpinned npm/scoped-npm/uvx, :latest/untagged docker, bad id forms).
Verified every new check by commenting it out and confirming the guarding test goes red, then restoring it — done individually for: env deny-list, env ASCII (key+value), env secret check, env placeholder/empty check, npm/uvx/docker pin enforcement, id regex, resolve_catalog anti-freeze anchor, resolve_catalog tie-break, resolve_catalog floor param.
Fixes findings **2, 3, 4, 6, 7** from #68 (the second adversarial review). Scope limited to `bcc_core.py`, `tests/test_core.py`, `.github/workflows/ci.yml` per the issue — findings 1 and 5 (Console signing flow, key separation) are out of scope for this PR and are being handled separately.
## Finding 2 — `config.env` was unvalidated
`_validate_catalog_config` type-checked `env` only. Added:
- `CATALOG_DENIED_ENV_KEYS` (module-level frozenset, importable by the Console): `NODE_OPTIONS`, `PYTHONSTARTUP`, `PYTHONPATH`, `PYTHONHOME`, `LD_PRELOAD`, `LD_LIBRARY_PATH`, `DYLD_INSERT_LIBRARIES`, `DYLD_LIBRARY_PATH`, `BROWSER`, `PATH`, `NODE_REPL_EXTERNAL_MODULE`. Matched case-insensitively.
- Same ASCII check `args` gets, applied to env keys and values.
- Existing `_is_secret_value` check applied to env values.
- `config.env` values must be `""` or a single `<PLACEHOLDER>` token — blocking otherwise.
Reproduced and confirmed blocked: `{"command": "npx", "env": {"NODE_OPTIONS": "--require /tmp/x.js"}}` now fails `validate_catalog`.
## Finding 3 — version pinning not enforced at runtime
Moved enforcement from `catalog_review.py` (maintainer-only, never runs per finding 1) into `_validate_catalog_config`, blocking:
- npm specs need exact `@version` (scoped `@scope/pkg@1.2.3` handled correctly — right-to-left parse so the scope's leading `@` isn't mistaken for the version separator).
- uvx/PyPI specs need `@version` or `==version`.
- docker images need an explicit tag, and it must not be `latest`.
Only the first plausible package-spec token is checked (flags, `<PLACEHOLDER>`s, and docker's `run`/`-i`/`--rm`/`-e KEY` are correctly skipped). **All 19 entries in the real `data/catalog.json` still validate clean** (`test_shipped_catalog_json_passes_validation` — unedited, still green).
## Finding 4 — CI gate trusted the pubkey from the PR it reviewed
`ci.yml`'s `catalog-signature` job now hardcodes `EXPECTED_CATALOG_PUBKEY_B64` and asserts `bcc_core.CATALOG_PUBKEYS` (as loaded from the PR branch) matches it, **before** verifying the signature. Comment on that line calls out that it's a trust anchor and must be reviewed on its own, never bundled with a catalog content change.
**Verified the actual attack in a sandbox**: cloned the repo, had an "attacker" generate their own Ed25519 keypair, re-signed `data/catalog.json` with it, and swapped `bcc_core.CATALOG_PUBKEYS` to the attacker's key (the exact two-file diff from the finding) — the extracted CI check now exits 1 with a clear message instead of going green.
**Note for the maintainer**: per the issue, the current key (`082NOwVB7uURkvfyS3+knJ+40Fk6C9unsF47+2uPKo4=`) is about to be rotated. `EXPECTED_CATALOG_PUBKEY_B64` in `ci.yml` is a single obvious line specifically so that update is trivial — please review it as its own commit when you rotate.
## Finding 6 — `resolve_catalog` guards skipped the first candidate
Both guards sat behind `if best_version >= 0`, so the first verified candidate (bundled, cached, or remote — whichever landed first) was accepted unconditionally, and the anti-freeze ceiling drifted upward with each accepted candidate instead of staying fixed (a legitimate moderate bump could "carry" a much larger illegitimate jump right behind it).
- The anti-freeze cap is now measured against the **bundled** catalog's verified version specifically — the trust anchor baked into the binary — independent of iteration order.
- Bundled now wins version ties (previously the last-evaluated candidate won ties unconditionally).
- Added a pure `floor: int = 0` parameter so a future caller can pass a persisted accepted-version floor; no storage wired up in this PR, as scoped.
## Finding 7 — catalog `id` unconstrained
Constrained to `^[a-z0-9][a-z0-9._-]{0,63}$`.
## Tests
- Fixed `_minimal_catalog`/`test_load_catalog_valid_round_trip` — the fixture used an unpinned package and asserted it validated clean (enshrined finding 3). Now pinned; added a separate test asserting the unpinned form is rejected.
- Rewrote `test_catalog_entry_to_paste_json_includes_env_when_present` — it asserted `env` passes through verbatim without checking `validate_catalog` first rejects the dangerous case (enshrined finding 2).
- Reordered `test_resolve_catalog_rejects_absurd_version_jump` so the freeze attempt is evaluated *first*, which is what actually exercises the vulnerable path; added `test_resolve_catalog_caps_first_and_only_candidate`, `test_resolve_catalog_anchors_cap_to_bundled_not_a_chained_best`, `test_resolve_catalog_prefers_bundled_on_version_tie`, `test_resolve_catalog_floor_rejects_below_persisted_version`.
- Added positive/negative tests for every new rule (each denied env key, case-insensitive match, non-ASCII env key/value, secret-looking env value, unpinned npm/scoped-npm/uvx, `:latest`/untagged docker, bad `id` forms).
- **Verified every new check by commenting it out and confirming the guarding test goes red, then restoring it** — done individually for: env deny-list, env ASCII (key+value), env secret check, env placeholder/empty check, npm/uvx/docker pin enforcement, id regex, resolve_catalog anti-freeze anchor, resolve_catalog tie-break, resolve_catalog floor param.
## Status
- `ruff check .` — clean
- `ruff format --check .` — clean
- `pytest` — 347 passed, 1 skipped (pre-existing skip, unrelated)
- `data/catalog.json` untouched; `test_shipped_catalog_json_passes_validation` still green with the new rules
Closes findings 2, 3, 4, 6, 7 of #68.
Fixes findings 2, 3, 4, 6, 7 from the issue #68 adversarial review.
- Finding 2: config.env was type-checked only. Add CATALOG_DENIED_ENV_KEYS
(case-insensitive) for interpreter/loader-override keys (NODE_OPTIONS,
PYTHONPATH, LD_PRELOAD, ...), apply the ASCII check and the existing
secret-value check to env keys/values, and require env values to be
empty or a single <PLACEHOLDER> token.
- Finding 3: version pinning was only checked by catalog_review.py (which
never runs on the signing path per finding 1). Move enforcement into
_validate_catalog_config: npm/uvx specs must carry @version or ==version
(scoped names handled), docker images must have an explicit non-latest
tag. Only the first plausible package-spec token is checked, so flags,
<PLACEHOLDER>s, and docker subcommands/flags don't trip it. All 19 real
catalog entries still validate clean.
- Finding 4: the CI catalog-signature gate imported bcc_core from the PR
branch and trusted whatever CATALOG_PUBKEYS said there, so a PR changing
both catalog.json and CATALOG_PUBKEYS (with a matching signature) went
green. ci.yml now hardcodes the expected base64 pubkey and asserts
bcc_core.CATALOG_PUBKEYS matches it before verifying the signature.
NOTE: the maintainer is planning to rotate this key -- update
EXPECTED_CATALOG_PUBKEY_B64 in ci.yml as its own reviewed change when
that happens, never bundled with a catalog content change.
- Finding 6: resolve_catalog's anti-rollback/anti-freeze guards sat behind
`if best_version >= 0`, so the first verified candidate was accepted
unconditionally and the anti-freeze anchor drifted with each accepted
candidate instead of staying fixed. The cap is now measured against the
bundled catalog's version specifically (the trust anchor baked into the
binary), regardless of evaluation order; bundled wins version ties; and
a new pure `floor` parameter lets a future caller pass a persisted
accepted-version floor.
- Finding 7: catalog id is now constrained to ^[a-z0-9][a-z0-9._-]{0,63}$.
Tests: fixed _minimal_catalog to use a pinned package (was enshrining
finding 3), rewrote the env-passthrough test to prove the validation
boundary instead of asserting env passes through unchecked, and
reordered test_resolve_catalog_rejects_absurd_version_jump so it
actually exercises the first-candidate path. Added positive/negative
tests for every new rule. Manually verified each new check by commenting
it out and confirming the guarding test goes red, then restoring it.
the_og
merged commit 86139100eb into main2026-07-12 21:28:24 -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.
Fixes findings 2, 3, 4, 6, 7 from #68 (the second adversarial review). Scope limited to
bcc_core.py,tests/test_core.py,.github/workflows/ci.ymlper the issue — findings 1 and 5 (Console signing flow, key separation) are out of scope for this PR and are being handled separately.Finding 2 —
config.envwas unvalidated_validate_catalog_configtype-checkedenvonly. Added:CATALOG_DENIED_ENV_KEYS(module-level frozenset, importable by the Console):NODE_OPTIONS,PYTHONSTARTUP,PYTHONPATH,PYTHONHOME,LD_PRELOAD,LD_LIBRARY_PATH,DYLD_INSERT_LIBRARIES,DYLD_LIBRARY_PATH,BROWSER,PATH,NODE_REPL_EXTERNAL_MODULE. Matched case-insensitively.argsgets, applied to env keys and values._is_secret_valuecheck applied to env values.config.envvalues must be""or a single<PLACEHOLDER>token — blocking otherwise.Reproduced and confirmed blocked:
{"command": "npx", "env": {"NODE_OPTIONS": "--require /tmp/x.js"}}now failsvalidate_catalog.Finding 3 — version pinning not enforced at runtime
Moved enforcement from
catalog_review.py(maintainer-only, never runs per finding 1) into_validate_catalog_config, blocking:@version(scoped@scope/pkg@1.2.3handled correctly — right-to-left parse so the scope's leading@isn't mistaken for the version separator).@versionor==version.latest.Only the first plausible package-spec token is checked (flags,
<PLACEHOLDER>s, and docker'srun/-i/--rm/-e KEYare correctly skipped). All 19 entries in the realdata/catalog.jsonstill validate clean (test_shipped_catalog_json_passes_validation— unedited, still green).Finding 4 — CI gate trusted the pubkey from the PR it reviewed
ci.yml'scatalog-signaturejob now hardcodesEXPECTED_CATALOG_PUBKEY_B64and assertsbcc_core.CATALOG_PUBKEYS(as loaded from the PR branch) matches it, before verifying the signature. Comment on that line calls out that it's a trust anchor and must be reviewed on its own, never bundled with a catalog content change.Verified the actual attack in a sandbox: cloned the repo, had an "attacker" generate their own Ed25519 keypair, re-signed
data/catalog.jsonwith it, and swappedbcc_core.CATALOG_PUBKEYSto the attacker's key (the exact two-file diff from the finding) — the extracted CI check now exits 1 with a clear message instead of going green.Note for the maintainer: per the issue, the current key (
082NOwVB7uURkvfyS3+knJ+40Fk6C9unsF47+2uPKo4=) is about to be rotated.EXPECTED_CATALOG_PUBKEY_B64inci.ymlis a single obvious line specifically so that update is trivial — please review it as its own commit when you rotate.Finding 6 —
resolve_catalogguards skipped the first candidateBoth guards sat behind
if best_version >= 0, so the first verified candidate (bundled, cached, or remote — whichever landed first) was accepted unconditionally, and the anti-freeze ceiling drifted upward with each accepted candidate instead of staying fixed (a legitimate moderate bump could "carry" a much larger illegitimate jump right behind it).floor: int = 0parameter so a future caller can pass a persisted accepted-version floor; no storage wired up in this PR, as scoped.Finding 7 — catalog
idunconstrainedConstrained to
^[a-z0-9][a-z0-9._-]{0,63}$.Tests
_minimal_catalog/test_load_catalog_valid_round_trip— the fixture used an unpinned package and asserted it validated clean (enshrined finding 3). Now pinned; added a separate test asserting the unpinned form is rejected.test_catalog_entry_to_paste_json_includes_env_when_present— it assertedenvpasses through verbatim without checkingvalidate_catalogfirst rejects the dangerous case (enshrined finding 2).test_resolve_catalog_rejects_absurd_version_jumpso the freeze attempt is evaluated first, which is what actually exercises the vulnerable path; addedtest_resolve_catalog_caps_first_and_only_candidate,test_resolve_catalog_anchors_cap_to_bundled_not_a_chained_best,test_resolve_catalog_prefers_bundled_on_version_tie,test_resolve_catalog_floor_rejects_below_persisted_version.:latest/untagged docker, badidforms).Status
ruff check .— cleanruff format --check .— cleanpytest— 347 passed, 1 skipped (pre-existing skip, unrelated)data/catalog.jsonuntouched;test_shipped_catalog_json_passes_validationstill green with the new rulesCloses findings 2, 3, 4, 6, 7 of #68.