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

Merged

the_og merged 1 commits from fix/68-core-validation into main 2026-07-12 21:28:24 -04:00

Conversation 0 Commits 1 Files Changed 3

+661 -46

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

Owner

Copy Link

Copy Source

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 #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.

the_og added 1 commit 2026-07-12 21:27:39 -04:00

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

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 main 2026-07-12 21:28:24 -04:00

the_og deleted branch fix/68-core-validation 2026-07-12 21:28:24 -04:00

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

P0

P1

P2

blocked

Waiting on an external thing before it can move.

decision

A decision record (D-007 style) — settled call, referenceable by number from any repo.

needs-decision

Waiting on AJ's call.

next

Current focus / next actionable item.

status

The single pinned STATUS issue for this repo — live state, next action, open items, decisions needed.

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

the_og

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: the_og/better-claude-config#70