Repository navigation
Remove code for auditability (scan picks A, B1-B4, C7, C10-C13) - #146
BenWestgate wants to merge 3 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review Generated by Claude Code |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cfbba960e6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex security review Focus: correction ( Generated by Claude Code |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
77474cf to
ffebbbd
Compare
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
Security review and patch-risk assessment
Reviewed the immutable PR #146 comparison 9b99862eb14d..ffebbbd5fa5d (binary diff SHA-256 14752d8dd89c066ef8008dd92fee88c1a7f321e1a165de4527de06f8089cafa9). The Codex Security diff scan completed review of all 25 changed source/workflow/tooling items, investigated seven candidates, and found no standalone exploitable vulnerability under the repository's documented trust model. This is not an unconditional security sign-off: the separate patch-risk assessment recommends revise, not auto-merge.
Risk dimensions: impact if wrong high; regression likelihood high (source-visible/reproduced boundary changes); regression protection partial; recoverability managed; confidence moderate. Exact-head GitHub Actions Python package and Bitcoin Core fixtures both passed. They do not cover the removed construction/copy guards or the public required-work timeout contract.
Changes requested before merge
- Reconsider the unchecked exported artifact constructor (existing review thread). With two synthetic, unfunded seeds,
MasterSeed(first.text, first.header, first.hrp, second.payload_symbols)displays the first seed's text butmaster_xprvderives the second seed's key. This is conditional on an API consumer directly constructing artifacts, which the new guide calls unsupported, but it contradicts the validated-artifact security invariant. Restore the factory-only guard or validate at the sensitive consumers and align the contract. - Restore the public correction API's fail-closed behavior when required competitor work expires. The new
competitors=Truepath can return one candidate markedsearch_complete=Falseeven during required work, contrary todocs/security/invariants.md. I reproduced this with a synthetic card, an adjacent swap and a deterministic expiry after the fixed pass. The CLI's whole-card confirmation mitigates downstream risk but does not satisfy the public API contract. - Reword the recordless restore message: a fingerprint-derived identifier matches any internally consistent unshared card, including the wrong wallet's. “So this is not a mixed-up card” overstates what the check establishes and conflicts with the adjacent no-record warning.
- Resolve the Core chain policy and documentation together. The adapter now follows the network selected by
bitcoin-cliconfiguration, butdocs/security/model.md:227-229anddocs/developer/api.md:564-569still promise five-chain probing and explicit-chainon every call. Later calls are not pinned to the chain observed at preflight. Either bind the destination or explicitly revise the mandatory contract and test the intended config-change behavior. - Make an explicit decision on the open ceremony-copy guard thread.
deepcopyof a live synthetic ceremony now succeeds; no in-repository copy path exists, so this is an API safety/recoverability risk rather than a proven exploit.
The deliberate removal of BIP39/Core Lightning application rules is documented as generic-HRP behavior; it remains a public-contract migration consideration. The detailed, schema-validated risk assessment is included below for auditability.
Machine-readable patch-risk assessment
{
"schemaVersion": 1,
"patch": {
"repository": "BenWestgate/python-codex32",
"sourceType": "commit_range",
"base": "9b99862eb14d15d86977cec202a6d39abcac2a88",
"head": "ffebbbd5fa5d5ad15459bb798c9c12ce332c2bfd",
"changedFiles": [
".github/workflows/python-package.yml",
"AGENTS.md",
"README.md",
"docs/developer/api.md",
"docs/security/invariants.md",
"docs/security/model.md",
"docs/user/guide.md",
"src/codex32/__init__.py",
"src/codex32/_bitcoin_core.py",
"src/codex32/_cli_input.py",
"src/codex32/_cli_parser.py",
"src/codex32/bip93.py",
"src/codex32/cli.py",
"src/codex32/correction.py",
"src/codex32/errors.py",
"src/codex32/generation.py",
"src/codex32/indel.py",
"src/codex32/profiles/__init__.py",
"src/codex32/profiles/bip39.py",
"src/codex32/profiles/cl32.py",
"src/codex32/profiles/ms32.py",
"src/codex32/wallet.py",
"tests/data/README.md",
"tests/data/malformed_inputs.json",
"tests/data/sharing_vectors.py",
"tests/test_alignment.py",
"tests/test_bip39.py",
"tests/test_bip93.py",
"tests/test_bitcoin_core.py",
"tests/test_cli.py",
"tests/test_cli_lengths.py",
"tests/test_competitors.py",
"tests/test_correction_bch.py",
"tests/test_correction_disclosure.py",
"tests/test_correction_indel.py",
"tests/test_crc.py",
"tests/test_generation.py",
"tests/test_generic_hrp.py",
"tests/test_lengths.py",
"tests/test_malformed_corpus.py",
"tests/test_mixed_case_deadline.py",
"tests/test_public_api.py",
"tests/test_sharing.py",
"tools/alignment_benchmark.py",
"tools/bitcoin_core_main_smoke.py",
"tools/bitcoin_core_regtest.py",
"tools/correction_benchmark.py",
"tools/differential_correction.py",
"tools/fuzz_correction_context.py",
"tools/fuzz_untrusted_boundaries.py",
"tools/verify_installed_wheel.py",
"tools/verify_wheel_environment.py"
],
"sha256": "14752d8dd89c066ef8008dd92fee88c1a7f321e1a165de4527de06f8089cafa9"
},
"recommendation": "revise",
"workflowLabel": "revise",
"impact": {
"rating": "high",
"rationale": "This changes exported artifact trust, correction behavior, recovery identity messaging, Core network selection and public HRP contracts in wallet-backup software; wrong backup or wallet selection can cause costly recovery failure."
},
"regressionLikelihood": {
"rating": "high",
"rationale": "Exact-head source and bounded synthetic reproductions establish that unchecked MasterSeed construction and incomplete required-work correction now succeed; Core safety documentation contradicts implemented chain selection."
},
"regressionProtection": {
"rating": "partial",
"rationale": "Exact-head Python, wheel, cross-platform and Core-fixture jobs pass, but tests for constructor and ceremony guards were removed, the main-chain smoke and fuzz harnesses were deleted, and no test asserts public required-work fail-closed or chain pinning.",
"exactHeadChecksPassed": true
},
"recoverability": {
"rating": "managed",
"rationale": "Code can be reverted without schema migration, but users may have printed backup cards or imported into Core wallets under the changed behavior; recovery requires auditing those artifacts and wallet records."
},
"confidence": {
"rating": "moderate",
"rationale": "The immutable commit range, all 25 changed-source review items, supporting docs/tests, exact-head CI and bounded reproductions are inspected; external consumers and natural timeout frequency are not established."
},
"applicability": {
"status": "confirmed",
"rationale": "Both published CLI entrypoints and exported Python API reach the changed code; package and Bitcoin Core test workflows run at the exact head."
},
"statusQuoRisk": {
"rating": "unknown",
"rationale": "The PR identifies a substantial auditability cost from legacy code, but no current exploit or quantified operating risk of retaining it is demonstrated."
},
"autoMergeExclusions": [
"public_contract",
"privileged_boundary",
"architecture_specific_rollout"
],
"affectedRuntimeRoots": [
"codex32.cli:main",
"codex32.cli:ms_main",
"codex32.parse_codex32",
"codex32.correct",
"codex32.MasterSeed",
"codex32.CreationCeremony",
"codex32.wallet.master_xprv",
"codex32._bitcoin_core.BitcoinCore.connect/initialize"
],
"importantCallers": [
"CLI check/correct/create/share/wallet flows",
"External Python package consumers of exported artifact classes",
"Bitcoin Core subprocess adapter and wallet initialization"
],
"riskDrivers": [
"Direct MasterSeed constructor accepts inconsistent text and payload; synthetic reproduction derives the payload's xprv rather than the displayed text's key.",
"Public correction can return a sole candidate when required competitor search expires, contrary to docs/security/invariants.md:20-25.",
"Recordless restore says a self-consistent card is not mixed up, although no wallet record binds it to the intended wallet.",
"Core adapter no longer pins -chain and authoritative security/developer docs still claim five-chain explicit probing.",
"Ceremony deepcopy succeeds after pending share; noncopyability tests and other boundary fuzz/mainnet smoke checks were removed."
],
"protectiveFactors": [
"Parser/factories still validate/reparse normal CLI artifacts and output suggestions remain explicitly untrusted.",
"CLI requires exact-card correction confirmation and recordless restore still prints a broader warning.",
"Exact-head Python package and Bitcoin Core fixture workflows completed successfully across Linux, Windows and macOS."
],
"materialBoundaries": [
{
"id": "validated-artifact",
"invariant": "Only validated artifacts may authorize derivation and wallet operations.",
"runtimeRoot": "MasterSeed/Share Python API and wallet.master_xprv",
"counterexample": "Construct MasterSeed from one synthetic seed's text and another's payload; master_xprv derives the latter without parsing.",
"legitimateControl": "Normal parser/factory call sites validate; developer docs declare direct construction unsupported, but the exported constructor is callable.",
"result": "contradicted"
},
{
"id": "correction-required-work",
"invariant": "Public API returns no candidate if required correction search is incomplete.",
"runtimeRoot": "codex32.correct",
"counterexample": "Adjacent-swap synthetic input with competitor deadline expiry returns one incomplete candidate.",
"legitimateControl": "search_complete=False flags truncation and CLI requires exact-card confirmation; these do not enforce the documented public fail-closed rule.",
"result": "contradicted"
},
{
"id": "recordless-identity",
"invariant": "No-card-record message must not imply that internal identifier consistency proves wallet identity.",
"runtimeRoot": "ms32 wallet/create --existing",
"counterexample": "An unrelated valid unshared backup self-matches its fingerprint identifier and triggers 'not a mixed-up card'.",
"legitimateControl": "NO_RECORD_WARNING separately says nothing proves expected wallet and requires explicit confirmation.",
"result": "contradicted"
},
{
"id": "core-chain",
"invariant": "Core chain used for wallet mutation remains bound to preflight and documented contract is accurate.",
"runtimeRoot": "BitcoinCore.connect/_rpc/initialize",
"counterexample": "bitcoin-cli config can change after connect; later _rpc has no -chain and may target another local network.",
"legitimateControl": "The selected bitcoin-cli and config are trusted; chain is reported at connect, but not pinned and docs still promise explicit -chain.",
"result": "contradicted"
},
{
"id": "non-ms-application",
"invariant": "Generic HRPs must not be presented as application-validated by this library after profile removal.",
"runtimeRoot": "parse_codex32/codex32 check",
"counterexample": "Checksum-valid but application-invalid BIP39 or CL text now parses generically.",
"legitimateControl": "README and user guide explicitly delegate non-ms application semantics; removed profile exports make old typed consumers fail imports.",
"result": "supported"
},
{
"id": "secret-channel",
"invariant": "Private wallet key material remains off command arguments and normal diagnostics.",
"runtimeRoot": "BitcoinCore._rpc/initialize",
"counterexample": "A changed subprocess wrapper might put xprv in argv or print it.",
"legitimateControl": "Current _rpc uses child stdin and fixed command arguments; Core errors are suppressed; exact-head tests exercise wallet fixtures.",
"result": "supported"
}
],
"validation": [
{
"name": "Exact-head GitHub Actions Python package run 39560631259",
"status": "passed",
"protects": "Python 3.10-3.15 compatibility, Linux/Windows/macOS tests, type/lint, differential correction and installed-wheel test."
},
{
"name": "Exact-head GitHub Actions Core fixtures run 39560631342",
"status": "passed",
"protects": "Frozen wallet fingerprint agreement with pinned Bitcoin Core."
},
{
"name": "Synthetic isolated MasterSeed mismatch reproduction",
"status": "passed",
"protects": "Confirms direct constructor allows text/payload mismatch and master_xprv follows payload."
},
{
"name": "Synthetic isolated correction expiry reproduction",
"status": "passed",
"protects": "Confirms required-work timeout can still return one incomplete public candidate."
},
{
"name": "Main-chain smoke, removed construction/copy tests, and fuzz harnesses",
"status": "unavailable",
"protects": "Previously tested or exercised Core mainnet integration and hardening boundaries; removed in this patch."
}
],
"unknowns": [
{
"summary": "External package consumers that relied on removed BIP39/CL profile validation or direct artifact construction are not inventoried.",
"decisionCritical": false
},
{
"summary": "Natural ten-second correction expiry frequency for affected damaged-card shapes is not measured.",
"decisionCritical": false
}
],
"evidencePlan": []
}Security scan ID: 5b29126a-05e7-48d5-81be-98e895575148. No local integration suite was run; the bounded reproductions used synthetic, unfunded material in an isolated checkout.
Security review on #146: MasterSeed(first.text, first.header, "ms", second.payload_symbols) showed one seed's text while master_xprv derived another, and a live CreationCeremony could be deep-copied and confirmed twice. Bring back the construction token and the ceremony __init__ and __reduce_ex__ traps, with their tests. Claude-Session: https://claude.ai/code/session_01XGMpowzJwwgyHMEd63ijDe
Security review on #146: any valid unshared card matches its own fingerprint, so a match shows the card is consistent, not that it is the expected wallet. Claude-Session: https://claude.ai/code/session_01XGMpowzJwwgyHMEd63ijDe
This reverts "core: Use bitcoin-cli's configured chain". It saved 28 lines but made testers edit bitcoin.conf, and the security review on #146 found later RPC calls were no longer pinned to the chain seen at preflight, against docs/security/model.md and docs/developer/api.md. Claude-Session: https://claude.ai/code/session_01XGMpowzJwwgyHMEd63ijDe
Security review on #146: moving the CLI onto the library driver also moved the public correct() onto the competitor scheduler, which returns a best-so-far candidate when required work expires. That breaks docs/security/invariants.md item 5. correct() again uses the fail-closed search; only the CLI asks for the competitor scheduler. A deterministic test expires every layer after the BCH pass. Claude-Session: https://claude.ai/code/session_01XGMpowzJwwgyHMEd63ijDe
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
AI-assisted current-head triage.
Concept ACK, not code/security ACK, at 74b6858. The 27-commit cleanup changes parsing, correction, public-facing behavior, and generation boundaries together (52 files). Keep draft pending per-commit tests, BIP-93 and differential vectors, and a focused security review of shared correction and ceremony invariants. Earlier review fixes (including ceremony-copy protection) need confirmation against this head.
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
AI-assisted current-head triage.
Concept ACK, not code/security ACK, at d1714f7. The latest commit changes correction, parsing, generation and validation boundaries across 29 files; the full PR still spans 53 files and 28 commits. Keep draft pending per-commit test evidence, BIP-93/differential regression checks, and a complete security review of the immutable head. Python-package CI was still running at review time.
Port _from_unchecksummed_string() to string headers and Codex32String. Claude-Session: https://claude.ai/code/session_01C8Ur3dhqpcWU3gEYsweXNM
|
Per-commit evidence for the triage reviews: all 28 commits from 9b99862 to d1714f7 were checked in a clean worktree, each with The full suite contains the BIP-93 vectors.
The head also passes under Earlier review fixes on d1714f7:
The PR stays in draft until the security review of the head. Generated by Claude Code |
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
AI-assisted incremental review at bb52ac4 — not ACKing yet. CorrectionContext.from_shares() truncates accepted to 10 with islice; an incompatible 11th share is silently ignored, and its index is not excluded. Reject an oversized sequence instead of accepting a partial set. CI is green; the full cleanup still needs final security/integration review.
|
Correction to my |
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
Fresh exact-head review (bb52ac4). GitHub Actions Python package and Bitcoin Core wallet fixtures passed on this SHA. Two reproducible recovery-availability regressions in the new context factory block readiness; please keep this PR in draft until fixed and rerun CI/security review on the replacement head. Neither result by itself shows acceptance of an invalid wallet seed, but both remove previously available backup-correction behavior.
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
Additional exact-head parser robustness finding: malformed text with a long HRP can now raise an uncaught IndexError instead of a CodexError. Reproduced with synthetic input; the baseline's Header._from_symbols length check handled it. This is separate from the two correction regressions recorded in the earlier review. PR should remain draft.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb52ac45c3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Return to v0.6.1's single Codex32String(s) for every share and secret, with hrp, k, ident, share_idx, payload and checksum read as slices of s, as in v0.6.1 and rust-codex32. Share, Secret and the Header class go: shares and secrets differ only by share_idx, and a private _header() validates new headers. MasterSeed stays as the one subclass with extra rules (ms, index S) and the seed bytes (data) and from_seed. Every ms string still gets BIP93's length check when parsed. Claude-Session: https://claude.ai/code/session_01XGMpowzJwwgyHMEd63ijDe
bb52ac4 to
58f9487
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
A short body after a long HRP passed the 21-character check and then indexed past the header, raising IndexError instead of InvalidLength. Restore the six-symbol header check from reviewability-v1. Claude-Session: https://claude.ai/code/session_01XGMpowzJwwgyHMEd63ijDe
With S accepted in a share basis, its index went into the correction context's excluded indices, which accept only ordinary indices, so a damaged next share got no suggestion. Leave S out of the exclusions, as reviewability-v1 effectively did. Claude-Session: https://claude.ai/code/session_01XGMpowzJwwgyHMEd63ijDe
Requested by Ben · project thread
Before: the library was 4,894 logical lines. The CLI ran its own correction driver, card comparer and error wording, the library carried
clandbip39profiles, and several guards re-checked already-validated values.correctcould not be told a known prefix or length.After: the library is 4,399 logical lines (−495). The CLI and GUI share one correction driver, one shift-aware card comparer and one set of messages. Only BIP93's
msrules remain.correcttakes--prefix, andcodex32 correcttakes--length.14 atomic commits on
reviewability-v1(9b99862), each passing lint, strict mypy and the full suite on its own, then 4 commits answering the security review:cleanup: remove dead and unused code.bip93: keep only themsrules and fold the profile registry.clandbip39strings now parse as generic codex32, so theclthreshold-0 rejection from api: Remove unused public entry points #130 is gone.cli: the CLI calls the library's_correct_complete, which gains the CLI's set-validity filter and, for the CLI only, the competitor scheduler. This supersedes the_correction_planpart of cli: Remove unreachable recovery and search paths #105.generation:ConfirmationResultnow carries shift-awaremismatched_groupsandentered_groups, so a dropped character marks only its own group. Entries more than 16 characters longer than the card are marked wrong without alignment. The CLI keeps only its region-retry prompt.errors: user-facing messages now live in the library exceptions._FRIENDLY_SET_ERRORSand_prefixed_input_errorare removed.generation: ceremony thresholds and identifiers are validated throughHeader.errors: drop the never-raisedInvalidHeaderandInvalidPayload. The specific reasons now derive fromCodexError.bip93: drop the_artifact_tailre-decode and the defensive sequence copy (a share set is now read at most ten items deep).cli: confirmations need interactive stdin; the/dev/tty/CONIN$fallback is removed.verify_wheel_environment.py,verify_installed_wheel.pyandbitcoin_core_main_smoke.pyare removed.cli:correct --prefix ms12namekeeps a known start fixed.codex32 correct --length Nlimits the search to the original's length, which helps with inserted or dropped characters;ms32 correctkeeps--bytes. Both are rejected with--residue.Security review follow-ups (review 5608668817):
CreationCeremonycopy/pickle traps (backs out those two C12 cuts).-chain, matching the security model and API guide.correct()fail-closed on expired required work; only the CLI uses the competitor scheduler. A deterministic deadline test covers both paths.Still needed before merge: the GUI branches import
profiles.ms32,CoreLightningSecretandProfile, so they need a follow-up.Checks run locally (Python 3.13):
pytestandpython -O -m pytest(873 passed),ruff,mypy --strict,verify_correction_constants.pyanddifferential_correction.py --verify(57 cases).🤖 Generated with Claude Code
https://claude.ai/code/session_01XGMpowzJwwgyHMEd63ijDe
Generated by Claude Code