Skip to content

Refs #576: Reject control chars in JSON integer fields#614

Open
Squirbie wants to merge 1 commit into
ramimbo:mainfrom
Squirbie:codex/json-int-control-576
Open

Refs #576: Reject control chars in JSON integer fields#614
Squirbie wants to merge 1 commit into
ramimbo:mainfrom
Squirbie:codex/json-int-control-576

Conversation

@Squirbie
Copy link
Copy Markdown
Contributor

@Squirbie Squirbie commented May 29, 2026

Summary

  • reject raw control characters in shared JSON integer parsing before trimming string values
  • keep clean integer strings and JSON integers accepted
  • add a wallet transfer regression proving nonce="\t1" no longer succeeds even with a signature for nonce 1

Evidence

Before this fix, the public wallet transfer API accepted a control-character-padded integer string after strip(): POST /api/v1/transfers with nonce="\t1" and a valid signature for nonce 1 returned HTTP 200 and created a transfer. That makes the submitted body less strict than the signed canonical integer payload.

This PR rejects C0/DEL control characters in _parse_int() before whitespace normalization, so shared JSON body integer fields fail closed with a bounded 400 such as nonce must not contain control characters.

Validation

  • tests/test_wallet_api.py::test_wallet_transfer_api_rejects_control_character_nonce_string -> 1 passed, 1 warning
  • focused wallet transfer tests -> 6 passed, 1 warning
  • tests/test_wallet_api.py -> 34 passed, 1 warning
  • full pytest -> 483 passed, 1 warning
  • ruff check app/main.py tests/test_wallet_api.py -> passed
  • ruff format --check app/main.py tests/test_wallet_api.py -> 2 files already formatted
  • mypy app/main.py -> success
  • scripts/docs_smoke.py -> docs smoke ok
  • git diff --check -> clean

No private data, wallet private material, tokens, admin access, production mutation, price/liquidity/exchange/bridge/off-ramp claims, or fabricated payout claims are included.

Summary by CodeRabbit

  • Bug Fixes
    • Enhanced API input validation to reject strings containing ASCII control characters, returning a 400 error response to prevent invalid requests from being processed.

Review Change Stack

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 29, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 0d5c00ea-b576-438c-aebc-cb432e429e4c

📥 Commits

Reviewing files that changed from the base of the PR and between e00dc75 and de30d8b.

📒 Files selected for processing (2)
  • app/main.py
  • tests/test_wallet_api.py

📝 Walkthrough

Walkthrough

The _parse_int function adds a validation check to reject string inputs containing ASCII control characters (codepoints < 32 or == 127) before existing parsing logic, returning a 400 error. A test verifies the wallet transfer API rejects a nonce containing a tab character with a 400 response and no balance change.

Changes

Control character validation

Layer / File(s) Summary
Control character validation in _parse_int and transfer API test
app/main.py, tests/test_wallet_api.py
_parse_int adds a guard to scan string inputs for ASCII control characters and reject them with a 400 error. A new test submits a signed transfer with a control-character-containing nonce field and validates the request is rejected with the appropriate error message and no balance change occurs.

Possibly related PRs

  • ramimbo/mergework#312: Both PRs modify the shared _parse_int validation path in app/main.py to reject invalid string inputs with controlled 400 errors.
🚥 Pre-merge checks | ✅ 6
✅ Passed checks (6 passed)
Check name Status Explanation
Title check ✅ Passed The title directly names the changed surface and describes the main change: rejecting control characters in JSON integer fields.
Description check ✅ Passed The description covers the main change, evidence of the security issue, test validation, and confirms all required checks passed.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Mergework Public Artifact Hygiene ✅ Passed PR maintains artifact hygiene: MRWK described as native coin supporting future snapshots/bridges; no investment, price, cash-out, fabricated payout, or private security claims found.
Bounty Pr Focus ✅ Passed PR changes match stated scope: control character validation added to _parse_int() and a focused regression test in test_wallet_api.py. Only the two stated files modified.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@eliasx45 eliasx45 left a comment

Choose a reason for hiding this comment

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

Reviewed current head de30d8b0e7b0776d5bd582e9997239da88b5e512 for Refs #576.

Verdict: approve.

The shared _parse_int() guard now rejects ASCII control characters before whitespace normalization, so a body field like nonce=\t1 cannot be accepted as canonical integer 1 after the signature was computed for 1. The regression signs the canonical transfer payload, submits the control-character-padded nonce, verifies the bounded 400 response, and confirms the receiver balance does not change.

Validation on this checkout:

git diff --check origin/main...HEAD
# clean

git merge-tree --write-tree origin/main HEAD
# 924f4574d1954dcc5eca1347689ac6b2a74ed3c8

PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 ..\mergework\.venv\Scripts\python.exe -m pytest tests\test_wallet_api.py::test_wallet_transfer_api_rejects_control_character_nonce_string tests\test_wallet_api.py::test_wallet_transfer_api_rejects_replayed_signed_body tests\test_wallet_api.py::test_wallet_transfer_api_rejects_invalid_requests tests\test_wallet_api.py::test_wallet_transfer_api_returns_validation_error tests\test_wallet_api.py::test_wallet_transfer_api_rejects_memo_control_characters -q
# 8 passed

PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 ..\mergework\.venv\Scripts\python.exe -m pytest tests\test_wallet_api.py -q
# 34 passed

..\mergework\.venv\Scripts\python.exe -m ruff check app\main.py tests\test_wallet_api.py
# passed

..\mergework\.venv\Scripts\python.exe -m ruff format --check app\main.py tests\test_wallet_api.py
# 2 files already formatted

PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 ..\mergework\.venv\Scripts\python.exe -m mypy app\main.py
# success

PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 ..\mergework\.venv\Scripts\python.exe scripts\docs_smoke.py
# docs smoke ok

GitHub readback: PR is open/non-draft, merge state is clean, hosted CI is green, and CodeRabbit has no actionable comments. I used isolated local SQLite files for the pytest runs to avoid cross-process schema collisions in this workspace.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants