Reject non-decimal MRWK amount notation#167
Conversation
Ckeplinger199
left a comment
There was a problem hiding this comment.
No blockers from my review.
I checked PR #167 at head 895dd016c539bf9ee696cba45ea9e9ee44c25655. The change is focused on the shared MRWK amount parser: it requires plain decimal notation before Decimal parsing, so scientific notation and leading-plus values no longer slip through while ordinary decimal values and existing error paths are preserved.
Local verification:
uv run --extra dev python -m pytest tests/test_security.py::test_amount_parser_rejects_non_finite_values tests/test_security.py::test_amount_parser_rejects_non_decimal_notation tests/test_security.py::test_amount_parser_rejects_values_above_fixed_supply tests/test_wallet_api.py::test_wallet_transfer_api_rejects_invalid_requests -q-> 7 passeduv run --extra dev python -m pytest tests/test_security.py tests/test_ledger.py tests/test_wallet_api.py tests/test_wallets.py -q-> 73 passeduv run --extra dev python -m pytest -q-> 168 passed, 2 warningsuv run --extra dev ruff format --check app/ledger/service.py tests/test_security.py-> 2 files already formatted;ruff check-> all checks passeduv run --extra dev python -m mypy app-> successuv run --extra dev python scripts/check_agents.py && uv run --extra dev python scripts/docs_smoke.py && git diff --check origin/main...HEAD-> passed
I also ran a direct parser probe: 1e3, 1E-3, and +1 now raise invalid MRWK amount; 1.5 and 0.000001 still convert correctly; -1 still raises amount must be positive. Hosted quality/readiness/docs/image checks are green.
TateLyman
left a comment
There was a problem hiding this comment.
No blockers found.
I reviewed PR #167 at head 895dd01, which rejects non-decimal-looking MRWK amount strings before shared Decimal parsing.
What I checked:
parse_mrwk_amount()now strips once, validates against a plain decimal pattern, then passes the same cleaned string intoDecimal.- Scientific notation (
1e3,1E-3) and leading plus notation (+1) now take theinvalid MRWK amountpath before they can normalize into accepted decimal values. - Normal decimal values like
1.5still parse, and negative plain decimals still reach the existingamount must be positiveerror. - This shared parser covers admin bounty creation and wallet transfer amount inputs, so the fix applies at the right layer.
- Hosted CI is green.
Local validation on the PR branch:
uv run --python 3.12 --extra dev pytest tests/test_security.py::test_amount_parser_rejects_non_decimal_notation tests/test_security.py::test_amount_parser_rejects_non_finite_values tests/test_security.py::test_amount_parser_rejects_values_above_fixed_supply tests/test_wallet_api.py::test_wallet_transfer_api_rejects_invalid_requests -q-> 7 passed.uv run --python 3.12 --extra dev pytest tests/test_security.py tests/test_ledger.py tests/test_wallet_api.py tests/test_wallets.py -q-> 73 passed.uv run --python 3.12 --extra dev ruff check app/ledger/service.py tests/test_security.py-> passed.uv run --python 3.12 --extra dev ruff format --check app/ledger/service.py tests/test_security.py-> passed.uv run --python 3.12 --extra dev mypy app-> success.python3 scripts/check_agents.pyandpython3 scripts/docs_smoke.py-> passed.git diff --check origin/main...HEAD-> no output.
One earlier parallel focused-test command hit a transient SQLite lock while another pytest process was already collecting the same app; rerunning after the broader test pass succeeded cleanly. No secrets, wallet material, private context, deployment values, or MRWK price claims involved.
|
Review for bounty #219: no blockers from my pass on PR #167. Evidence checked:
No secrets, wallet material, payout details, private deployment values, private context, private vulnerability details, or MRWK price claims were reviewed or disclosed. |
Refs #122.
Summary
1e3/1E-3and leading plus notation such as+1amount must be positiveRepro before fix
On current
origin/main,parse_mrwk_amount()accepted non-decimal-looking amount strings:Those values can flow through admin bounty creation and wallet transfer APIs because both call the shared parser.
Validation
python -m pytest tests/test_security.py::test_amount_parser_rejects_non_finite_values tests/test_security.py::test_amount_parser_rejects_non_decimal_notation tests/test_security.py::test_amount_parser_rejects_values_above_fixed_supply tests/test_wallet_api.py::test_wallet_transfer_api_rejects_invalid_requests -q-> 7 passedpython -m pytest tests/test_security.py tests/test_ledger.py tests/test_wallet_api.py tests/test_wallets.py -q-> 73 passedpython -m pytest -q-> 168 passed, 2 warningsruff check app/ledger/service.py tests/test_security.pyruff format --check app/ledger/service.py tests/test_security.pymypy apppython scripts/docs_smoke.pypython scripts/check_agents.pygit diff --checkNo secrets, wallet material, deployment values, private context, or MRWK price claims are included.