Reject non-integer wallet nonces#244
Merged
ramimbo merged 1 commit intoMay 25, 2026
Merged
Conversation
TUPM96
reviewed
May 25, 2026
Contributor
TUPM96
left a comment
There was a problem hiding this comment.
No blockers found. The new guard is placed at the shared wallet-payload verification boundary, so it covers signed transfers, GitHub linking, and GitHub claims before Python can treat True as nonce 1.
Evidence checked:
- Inspected
app/ledger/service.py:_verify_wallet_payload()now rejectsbooland non-intnonce values before thenonce != wallet.nonce + 1comparison. - Inspected
tests/test_wallets.py: the regression signs boolean-nonce payloads for transfer, wallet-link, and GitHub-claim flows, verifies each raisesnonce must be an integer, then confirms a normal integer link still succeeds before the claim check. - Checked hosted GitHub CI:
Quality, readiness, docs, and image checksis green and merge state is clean.
Local validation on PR head 4066040:
uv run --python 3.12 --extra dev python -m pytest tests/test_wallets.py::test_wallet_operations_reject_boolean_nonces -q->1 passeduv run --python 3.12 --extra dev python -m pytest tests/test_wallets.py tests/test_wallet_api.py -q->32 passeduv run --python 3.12 --extra dev ruff check app/ledger/service.py tests/test_wallets.py-> passeduv run --python 3.12 --extra dev ruff format --check app/ledger/service.py tests/test_wallets.py-> already formattedgit diff --check origin/main...HEAD-> clean
No secrets, wallet material, private deployment values, private vulnerability details, PayPal details, or MRWK price claims are included.
MolhamHamwi
reviewed
May 25, 2026
Contributor
MolhamHamwi
left a comment
There was a problem hiding this comment.
No blockers found.
Evidence checked:
- Inspected the shared
_verify_wallet_payload()guard inapp/ledger/service.py; it rejects bothbooland non-intnonce values before thewallet.nonce + 1comparison, so Python cannot treatTrueas integer1on the signed wallet paths. - Confirmed the guard is shared by wallet transfers, GitHub wallet linking, and GitHub balance claims through the existing call sites, without changing canonical payload serialization, signature verification, wallet balances, or nonce increment behavior for valid integer nonces.
- Inspected
tests/test_wallets.py; the new regression covers boolean nonce rejection forsubmit_wallet_transfer(),link_wallet_to_github(), andsubmit_github_claim(), then verifies a normal integer nonce link still succeeds before testing the claim path. - Ran local checks:
pytest tests/test_wallets.py -q(8 passed),ruff check app/ledger/service.py tests/test_wallets.py, andruff format --check app/ledger/service.py tests/test_wallets.py.
ayskobtw-lil
approved these changes
May 25, 2026
Contributor
ayskobtw-lil
left a comment
There was a problem hiding this comment.
No blockers from my review.
Evidence checked:
- Inspected
app/ledger/service.py;_verify_wallet_payload()now rejectsbooland other non-intnonce values before comparing againstwallet.nonce + 1, so Python cannot acceptTrueas nonce1. - Traced the shared verifier call sites in
submit_wallet_transfer(),link_wallet_to_github(), andsubmit_github_claim(), confirming the guard applies to all three signed wallet operations without changing canonical payload construction or signature verification. - Reviewed
tests/test_wallets.py; the regression signs boolean-nonce payloads for transfer, wallet link, and GitHub claim, then verifies each path raisesLedgerError("nonce must be an integer"). It also performs a valid link at nonce1so the claim path reaches the boolean-nonce check at the next operation.
Validation run on current head 4066040:
python -m pytest tests/test_wallets.py::test_wallet_operations_reject_boolean_nonces -q->1 passedpython -m ruff check app/ledger/service.py tests/test_wallets.py->All checks passed!git diff --check origin/main...HEAD-> clean
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs #228
Summary
1through Python'sbool/intequality behavior.Repro Before Fix
A direct ledger service caller could sign a wallet payload with
nonce: true; becauseTrue == 1in Python,_verify_wallet_payload()accepted it for a fresh wallet whose next nonce was1. The operation could then persist a boolean nonce value through transfer, link, or claim paths instead of requiring an integer nonce.Verification
./.venv/bin/python -m pytest tests/test_wallets.py::test_wallet_operations_reject_boolean_nonces -q-> 1 passed./.venv/bin/python -m pytest tests/test_wallets.py tests/test_wallet_api.py -q-> 32 passed./.venv/bin/python -m pytest -q-> 206 passed, 2 warnings./.venv/bin/python -m ruff format --check .-> 37 files already formatted./.venv/bin/python -m ruff check .-> all checks passed./.venv/bin/python -m mypy app-> success./.venv/bin/python scripts/docs_smoke.py-> docs smoke ok./.venv/bin/python scripts/check_agents.py-> AGENTS.md okgit diff --check-> cleanNo secrets, wallet material, private vulnerability details, deployment values, payout details, or MRWK price claims are included.