Repository navigation
cli: Restore ms32 checksum as a separate generation command - #144
BenWestgate wants to merge 1 commit into
Conversation
55b8d5f to
dcda141
Compare
|
@codex security-review Focus: the input gate in Generated by Claude Code |
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. |
|
@codex security review Generated by Claude Code |
|
Codex Review: Didn't find any major issues. Nice work! 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". |
![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: dcda141178
ℹ️ 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".
| hrp, body = bech32_decode(text) | ||
| # The Book's checksum worksheet has 32 non-pink squares. | ||
| if hrp != Profile.MS.value or len(text) + 13 != _ms_text_length(16): | ||
| raise ValueError | ||
| artifact = _from_parts( |
There was a problem hiding this comment.
Move this worksheet decoding and artifact construction into a dedicated domain helper or factory. The CLI now selects the profile and payload length, decodes Bech32 symbols, splits the header from the payload, and invokes the private _from_parts constructor itself; that duplicates domain logic at the presentation boundary and makes future checksum, profile-validation, or worksheet-format changes require synchronized CLI updates.
AGENTS.md reference: AGENTS.md:L22-L23
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
cACK, especially since there is a tracked issue to add this to the GUI if requested.
There was a problem hiding this comment.
Updated in f1aec05.
Moved worksheet decoding and construction into _from_book_worksheet() in profiles/ms32.py. It owns the fixed Book shape, ms profile, header/payload split and validated artifact construction. The CLI now handles prompts and output. The helper stays private, so this adds no public completion API; it is also available for the GUI work tracked in #153.
There was a problem hiding this comment.
Why no public completion API? Just curious.
There was a problem hiding this comment.
1a14000 removed complete_checksum() to narrow the reviewable interface: under the pre-1.0 "remove unused public API" policy nothing in the repo used it, and a public function makes re-checksumming an existing backup one call away for any app. f1aec05 kept the helper private for the same reason.
Your cNACK changes that, so b3bfe59 restores complete_checksum() as public, generic API (any HRP, registered profile payload rules still apply, result reparsed). Its docstring and docs/security/model.md say it has none of the CLI's safeguards and that apps exposing it must add their own. ms32 checksum still accepts only ms Book worksheet data.
Generated by Claude Code
There was a problem hiding this comment.
Current head 1887cf6 has the domain helper _from_unchecksummed_string, so the CLI/domain split is fixed. But the earlier reply promised a public complete_checksum() API; this head exposes only a private helper. Please confirm the deliberate API decision and update the thread/description. AI-assisted current-head check.
There was a problem hiding this comment.
Confirmed, private is deliberate. My 2026-10-20 reply above is superseded: on 2026-10-21 you said "it can be private API, but we definitely don't need to duplicate it between CLI and GUI", so the rebuild dropped the public complete_checksum().
The one generic helper is _from_unchecksummed_string() in bip93.py (any HRP, not exported). The CLI uses it now, and the GUI (#153) will import the same function. Keeping it private means no app gets re-checksumming of an existing backup in one call, and it can go public later without a break if a real user asks for it. The description now states this.
Generated by Claude Code
| "Consult the Codex32 Book and check the non-pink bold squares." | ||
| ) from error | ||
| clear = "\x1b[3J\x1b[2J\x1b[H" if sys.stderr.isatty() else "" | ||
| if _worksheet_text(clear + "Re-enter the worksheet non-pink bold squares").lower() != text.lower(): |
There was a problem hiding this comment.
When the worksheet text contains k, a second entry using Unicode Kelvin sign K in its place passes this comparison because Python evaluates "K".lower() == "k", even though K is not a valid Bech32 character. The command then emits the completed secret despite the two normalized worksheet entries being different, weakening the independent re-entry check; validate that the second entry is ASCII/Bech32-compatible before performing the case-insensitive comparison.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I don't know why the entries are being normalized at all, they should require both to be all upper case or all lower case like codex32 does and the output should keep the input case if --plain otherwise be pretty if that's the default behavior.
Also should we display and confirm the new string like share and create? or just the checksum? (since they already had to type the payload and prefix twice..)
There was a problem hiding this comment.
Updated in f1aec05.
Both entries now pass ASCII and single-case validation before comparison, so Unicode Kelvin signs and mixed-case input are rejected. --plain keeps the first input's case; default terminal output keeps the usual uppercase card layout.
Per your follow-up, the non-pink data is entered once. The full string is shown, you copy the checksum into the pink squares and press Enter, then the screen clears and you type back only the 13 pink squares. Successful confirmation releases the full string on stdout; mismatch, EOF or interruption does not. This confirms the checksum copy, not the non-pink transcription. The warning and docs say so.
Regression tests cover these cases, including all-uppercase input containing a Kelvin sign.
There was a problem hiding this comment.
Did you remove the double entry with terminal blanking of the non-pink bold squares? That was intentional to detect errors and should be kept. cACK "The full string is shown, you copy the checksum into the pink squares and press Enter, then the screen clears and you type back only the 13 pink squares."
There was a problem hiding this comment.
Yes, f1aec05 removed it by misreading your follow-up. Restored in b3bfe59. The flow is now:
- Enter the non-pink squares.
- Screen clears; re-enter them. A mismatch stops before anything is computed or shown.
- The full string is shown; copy the checksum into the pink squares and press Enter.
- Screen clears; type back only the 13 pink squares.
- Only then does the string go to stdout.
Both comparisons require single-case ASCII. The warning and docs now say the repeated entries catch typing errors, not errors already on the worksheet.
Generated by Claude Code
Move the fixed Book worksheet format, profile/header decoding and validated artifact construction into the ms profile. Keep the CLI responsible for prompts and presentation; expose no public completion API. Validation: 25 checksum and size-budget tests pass; Ruff and strict mypy pass. Refs #144.
Enter the non-pink Book data once, preview the completed string, clear the terminal after copying, then require re-entry of only the pink checksum. Gate stdout on successful confirmation even with --plain or redirected output. Validate ASCII and single case before comparison, preserve first-entry case in plain output, and make command names explicit in diagnostics. Document that checksum confirmation cannot verify non-pink transcription or entropy. Validation: 938 normal tests pass; Ruff check/format, strict mypy and git diff --check pass. A real-TTY smoke test confirms stdout gating and Unicode rejection. Refs #144.
|
Updated to f1aec05 in two focused commits: worksheet construction now belongs to the ms profile, and confirmation asks for only the 13 pink checksum squares after entering the non-pink data once. The full completed string is previewed before copying and emitted on stdout only after confirmation. 938 tests pass normally and 938 under The current target-branch AI policy is the tool-neutral policy merged in #152; this follow-up uses that policy. @codex security review Please review current head f1aec05, particularly the fixed Book worksheet input boundary, ASCII/single-case checks before comparison, pink-square-only confirmation, and preview-versus-confirmed-output behavior. |
|
Codex Review: Didn't find any major issues. You're on a roll. 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". |
|
cNACK: worksheet construction now belongs to the ms profile The secretcodex32.com website generates worksheets for any HRP and a few lengths plus there are worksheets for NOT RECOMMENDED profiles/hrp. So worksheet construction is generic however our CLI and possibly GUI would only do it for "ms" since no other applications are recommended but we shouldn't make it impossible for users of our library to expose this for their applications. |
Old topic branches can retain superseded contribution rules, as happened while addressing PR #144 after #152 merged. Require agents to consult the current target-branch contributor guide and AI policy before acting on a PR. State that the tool-neutral policy permits AI-written descriptions and review replies while keeping publication within the user-authorized scope. The maintainer explicitly approved this AGENTS.md patch. Validation: reviewed the exact approved patch, checked referenced local paths, and ran git diff --check. Documentation only; application tests are not applicable. Refs #144 and #152.
|
Re the cNACK on worksheet construction in the ms profile: addressed in b3bfe59.
Checks: ruff, ruff format, mypy, Per CONTRIBUTING.md this is a fixup commit for now. Squashing into atomic commits on the current reviewability-v1 needs a force-push, which I'll ask about in the project thread. Generated by Claude Code |
|
@codex security review Focus: the restored public Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b3bfe59c64
ℹ️ 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".
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1 similar comment
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1105472 to
62f293e
Compare
|
@codex security review Rebuilt as two atomic commits on #146 (62f293e). Focus: the private Generated by Claude Code |
|
Codex Review: Didn't find any major issues. Keep them coming! 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". |
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
Security review of PR #144 at base ffebbbd5, head 8a04a420.
Result: No confirmed security vulnerability in the changed source. I reviewed the three changed runtime files, their parser/input/output dependencies, the updated security model, tests, and malformed-input corpus.
ms32 checksumrejects non-TTY stdin, all intact completemslengths, non-35-character input, and non-msHRPs. The private helper validates container/header syntax and reparses the generated artifact.- The non-pink data must match a second entry, and the pink checksum must match before the completed string reaches stdout. The completed preview intentionally reaches stderr earlier; terminal clearing is best effort.
- Residual operator risk: A damaged existing backup shortened or lengthened to the accepted 35-character shape cannot be distinguished from new worksheet data. The latest security-model edit now says this explicitly. The warning to use only new Book data remains essential.
- Documentation correction (non-exploitable):
docs/security/model.mdsays nothing is “computed or displayed” before the second entry._complete_checksum()runs before that entry; only display/output is gated. Please align the wording when convenient.
Patch risk: hold_for_evidence for merge readiness: impact high, regression likelihood moderate, protection partial, recoverability hard, confidence moderate. The code-identical previous head passed cross-platform CI and CodeQL. At this head, two Ubuntu 3.13 jobs are still running; confirm all required checks pass before merging. Because this PR is stacked on #146, retarget it after #146 merges and recheck the final comparison. This review is a comment, not an approval.
8a04a42 to
1887cf6
Compare
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
AI-assisted current-head code/security review.
Concept ACK 1887cf6. ms32 checksum is generation-only, interactive, checks worksheet data twice and the resulting checksum once, and leaves existing check/correct paths unchanged. It cannot authenticate worksheet entropy or prevent incorrectly recorded non-pink data; those limitations are documented. A real-terminal workflow test and security gate remain required before merge.
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
AI-assisted current-head review.
Approach ACK 7fcffa7; not a final code/security ACK. ms32 checksum is isolated from repair, refuses nonterminal input, rechecks worksheet input and confirms the new checksum. The private shared helper matches the maintainer's API decision. Blocked: GitHub reports merge conflicts against its #146 base (dirty), and no current-head CI result is visible. Resolve the stack, rerun checks, and exercise a real-terminal worksheet ceremony before human review.
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
AI-assisted review at maintainer request.
Code ACK 96f6493 (static, scoped to #144). The generic checksum helper reparses its result; ms32 checksum limits input to the Book worksheet, requires double entry and pink-square confirmation, and emits to stdout only after both checks. Malformed-input cases and CI pass. No new finding in this diff.
Before merge: land/review parent #146 and exercise the worksheet flow in a real terminal on supported Tails. The command cannot detect errors already written on the worksheet.
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 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". |
96f6493 to
dff3bb8
Compare
bb52ac4 to
58f9487
Compare
Before: there is no way to checksum new Codex32 Book worksheet data.
ms32 checksumand v0.6.1'sCodex32String.from_unchecksummed_stringwere removed in 1a14000.After:
ms32 checksumchecksums new Book worksheet data as a separate generation command fromcheckandcorrect, following codex32 discussion #78. One private, generic helper builds the checksum, so a future GUI page (#153) reuses it instead of duplicating it.API decision: private, not public. Ben (2026-10-21): "it can be private API, but we definitely don't need to duplicate it between CLI and GUI." The public
complete_checksum()described in an earlier review reply is superseded._from_unchecksummed_string()inbip93.pyis not exported; it can become public later without a break if a real user needs it.Stacked on #146 (base is its branch; retarget to
reviewability-v1once #146 merges). Two atomic commits, plus merges of #146's newer commits (7fcffa7, 96f6493):bip93: Add a private _from_unchecksummed_string: v0.6.1's operation for any HRP, now with header andmspayload validation and a full reparse. It is not exported. Invariant 1 names it; the malformed-input corpus covers it.cli: Restore ms32 checksum for Book worksheets: the command, help, tests, user guide and security model.Flow
Every entry must be ASCII and entirely uppercase or lowercase.
--plainkeeps the input's case; default output uses the usual uppercase card layout. Mismatch, EOF or interruption puts nothing on stdout (the preview has already appeared on stderr after step 3).Misuse risks
A checksum makes any data look like a valid backup. From discussion #78:
How the CLI answers them:
ms32only, interactive only, errors give no format hints.msHRP (checked on the decoded string, so a stray1cannot change it) and refuses every completemsstring length, pointing to'ms32 check'. This narrows the missing-vs-wrong ambiguity but cannot remove it: a damaged backup that lost or gained characters can still reach the accepted length.The helper itself has none of these safeguards; the security model says every façade that uses it must provide them.
Not included: GUI (#153 asks Book users to request it).
Security review: Ben's review of 8a04a42 found no vulnerabilities. Its one documentation note (the model said nothing is "computed" before the second entry, though only display is gated) is fixed in 1887cf6, alongside the rename from
_complete_checksumand the commit-message wording. Ben's approach ACK of 7fcffa7 still requires a real-terminal worksheet ceremony and the security gate before merge.Merges since the review: 7fcffa7's only conflict was the
docs/developer/api.mdboundary line, which takes #146's wording. 96f6493 brings in #146's flattenedCodex32String, so the helper now builds the six-character header string and validates it with_header()(same threshold, identifier and index checks as before), returnsCodex32String, and the CLI compares the pink squares againstartifact.checksum. Invariant 1 now says construction validates and names the helper as one of the two builders.Validation on 96f6493: 917 tests pass normally and under
python -O; ruff, ruff format and mypy pass. A pseudo-terminal run with BIP-93 vector 1 completed the flow (lowercase and uppercase) and rejected a wrong pink square.🤖 Generated with Claude Code
https://claude.ai/code/session_01C8Ur3dhqpcWU3gEYsweXNM