Repository navigation
bip93: Reject non-ASCII before normalization - #13
BenWestgate wants to merge 4 commits into
Conversation
![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: 364e3d5060
ℹ️ 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".
364e3d5 to
b064fb5
Compare
This comment has been minimized.
This comment has been minimized.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b064fb5aa2
ℹ️ 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".
BenWestgate
added
gate: adversarial review
1b17d8a to
d2f8589
Compare
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
AI-generated review, posted at the maintainer's request.
ACK d2f8589.
BenWestgate
added
the
correction
ChatGPT Codex Connector
|
Agent release-gate review at exact head |
d2f8589 to
4f24148
Compare
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
4f24148 to
49f5cdc
Compare
This comment has been minimized.
This comment has been minimized.
49f5cdc to
4f01561
Compare
This comment has been minimized.
This comment has been minimized.
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
Codex current-head re-review: ACK 4f01561.
The current diff rejects non-ASCII lookalikes before header/index/target normalization and uses ASCII-only case folding in the common card-confirmation path, so characters such as Kelvin sign cannot normalize into valid Bech32 input or falsely confirm a transcription. All prior review findings are resolved; the remaining helper duplication is intentionally owned by #53. Exact-head Python-package run 637 succeeded.
No code blocker found; ready for human integration review after #12.
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
Codex current-head review at 4f01561: no findings. Public header/index and confirmation paths now reject Unicode lookalikes before case folding while retaining ASCII case-insensitivity.
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
AI-assisted current-head review: ACK 4f01561. Public identifiers, indices, derivation targets, CLI headers, and card confirmation now reject/non-match non-ASCII lookalikes before lowercasing, so Unicode characters such as Kelvin sign cannot alias valid Bech32 ASCII. Ordinary ASCII case-insensitivity and whitespace-tolerant confirmation are preserved. Exact-head Python-package CI is green and all review threads are resolved. #53 can centralize the duplicated ASCII helper without changing this behavior.
|
@codex review |
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 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". |
Reject non-ASCII identifiers, indices, and raw CLI creation headers before lowercasing so Unicode lookalikes cannot normalize into valid Bech32 symbols. Make whole-card confirmation fold ASCII case only. Non-ASCII lookalikes now stay visibly mismatched and enter the normal retry flow for shared, unshared, and derived cards instead of being accepted or reaching the ceremony callback as an apparent match. Document the ASCII-only confirmation boundary in the API and security model. Validation: 867 tests passed normally and under python -O; Ruff check and format passed; strict mypy passed; git diff --check passed.
4f01561 to
6f3dac8
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
| def _ascii_lower(value: str) -> str: | ||
| """Lowercase ASCII letters without normalizing Unicode lookalikes.""" | ||
| return "".join(character.lower() if character.isascii() else character for character in value) |
There was a problem hiding this comment.
let's just reject non-ASCII input entirely. Do not accept Unicode anywhere to avoid the lookalikes.
There was a problem hiding this comment.
Addressed in 58ea30d: reject non-ASCII before CLI whitespace normalization, parsing, or card confirmation. Unicode whitespace cannot disappear silently. Focused tests: 67 passed.
There was a problem hiding this comment.
Why is this function still in here then? I'm suggesting the command fail closed if unicode is pasted/entered. We don't need to support or tolerate it even as a "mistake" at all. it's an uncorrectable mistake, we just quit so we don't have to deal with this stupid implausible finding. nobody will ever type uppercase Kelvin.
There was a problem hiding this comment.
Addressed in 32db210. Removed _ascii_lower and Unicode retry handling. Non-ASCII input now aborts before normalization; library confirmation raises an error. Net PR: +75/−8, including 27 added production lines. 356 affected tests pass normally and optimized; Ruff/mypy pass.
Reject non-ASCII input before whitespace splitting or lowercasing, including nonbreaking spaces. Preserve existing interactive read-back spacing and fail confirmation on Unicode rather than silently dropping it. Validation: 72 focused tests normally and under optimized Python, Ruff, mypy and diff checks. refs #13
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
cNACKing the PR until someone explains why it needs to be 100 lines to fail when unicode is entered.
|
@codex review |
Reject non-ASCII text before whitespace removal or case folding. End the command instead of retaining Unicode in transcription retry state. Remove ASCII-only folding and its special retry handling. The library confirmation boundary also rejects non-ASCII rather than returning group feedback for it. Ordinary ASCII grouping and case remain accepted. Validation: 356 affected tests pass normally and with Python -O; Ruff lint/format, mypy and diff checks pass. refs #13
BenWestgate
changed the title
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
AI-assisted static review: ACK 32db210. The diff adds 27 production lines across distinct CLI and library input boundaries; the rest is regression coverage and documentation. Non-ASCII input is rejected before case conversion, whitespace removal, or confirmation. The earlier Unicode retry path is gone. No remaining code blocker found.
Reject non-ASCII identifiers, indices, derivation targets and recovery input before whitespace removal or case folding. CLI input ends the command with an ASCII error; library confirmation raises an error. ASCII case and grouping remain supported.
This prevents Unicode characters from becoming valid Bech32 text during normalization. The ASCII-only folding helper and Unicode retry handling have been removed, as requested in review.
Validation at
32db210: all 954 tests pass normally and under Python-O; Ruff lint/format, mypy and whitespace checks pass. Current-head CI remains under review. Adapts Rob1Ham#9.