Skip to content

Reject non-integer wallet nonces#244

Merged
ramimbo merged 1 commit into
ramimbo:mainfrom
nicolatrozzi:reject-noninteger-wallet-nonces
May 25, 2026
Merged

Reject non-integer wallet nonces#244
ramimbo merged 1 commit into
ramimbo:mainfrom
nicolatrozzi:reject-noninteger-wallet-nonces

Conversation

@nicolatrozzi
Copy link
Copy Markdown
Contributor

Refs #228

Summary

  • Reject boolean and non-integer wallet nonces inside the shared ledger-service signature verification path.
  • Apply the same service-boundary guard to signed wallet transfers, wallet-to-GitHub linking, and GitHub balance claims.
  • Add regression coverage showing boolean nonces are rejected before they can be accepted as nonce 1 through Python's bool/int equality behavior.

Repro Before Fix

A direct ledger service caller could sign a wallet payload with nonce: true; because True == 1 in Python, _verify_wallet_payload() accepted it for a fresh wallet whose next nonce was 1. 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 ok
  • git diff --check -> clean

No secrets, wallet material, private vulnerability details, deployment values, payout details, or MRWK price claims are included.

Copy link
Copy Markdown
Contributor

@TUPM96 TUPM96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 rejects bool and non-int nonce values before the nonce != wallet.nonce + 1 comparison.
  • Inspected tests/test_wallets.py: the regression signs boolean-nonce payloads for transfer, wallet-link, and GitHub-claim flows, verifies each raises nonce 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 checks is 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 passed
  • uv run --python 3.12 --extra dev python -m pytest tests/test_wallets.py tests/test_wallet_api.py -q -> 32 passed
  • uv run --python 3.12 --extra dev ruff check app/ledger/service.py tests/test_wallets.py -> passed
  • uv run --python 3.12 --extra dev ruff format --check app/ledger/service.py tests/test_wallets.py -> already formatted
  • git diff --check origin/main...HEAD -> clean

No secrets, wallet material, private deployment values, private vulnerability details, PayPal details, or MRWK price claims are included.

Copy link
Copy Markdown
Contributor

@MolhamHamwi MolhamHamwi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blockers found.

Evidence checked:

  • Inspected the shared _verify_wallet_payload() guard in app/ledger/service.py; it rejects both bool and non-int nonce values before the wallet.nonce + 1 comparison, so Python cannot treat True as integer 1 on 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 for submit_wallet_transfer(), link_wallet_to_github(), and submit_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, and ruff format --check app/ledger/service.py tests/test_wallets.py.

Copy link
Copy Markdown
Contributor

@ayskobtw-lil ayskobtw-lil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blockers from my review.

Evidence checked:

  • Inspected app/ledger/service.py; _verify_wallet_payload() now rejects bool and other non-int nonce values before comparing against wallet.nonce + 1, so Python cannot accept True as nonce 1.
  • Traced the shared verifier call sites in submit_wallet_transfer(), link_wallet_to_github(), and submit_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 raises LedgerError("nonce must be an integer"). It also performs a valid link at nonce 1 so 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 passed
  • python -m ruff check app/ledger/service.py tests/test_wallets.py -> All checks passed!
  • git diff --check origin/main...HEAD -> clean

@ramimbo ramimbo merged commit cc9af20 into ramimbo:main May 25, 2026
1 check passed
@ramimbo ramimbo added mrwk:accepted Maintainer accepted for payout mrwk:paid Ledger payment recorded labels May 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

mrwk:accepted Maintainer accepted for payout mrwk:paid Ledger payment recorded

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants