[codex] Bound MCP integer arguments#323
Conversation
📝 WalkthroughWalkthroughThis PR adds bounds validation to integer arguments in MCP tool calls, preventing oversized values from causing uncontrolled failures. The implementation tightens ChangesMCP Integer Argument Bounds Validation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_api_mcp.py`:
- Around line 847-855: The parametrized test matrix for oversized integers
misses negative overflow and raw-int paths for some tools; update the
pytest.mark.parametrize data in tests/test_api_mcp.py to add entries exercising
the negative overflow value (-2**63 - 1) and the raw 2**63 value for
get_ledger_entry and submit_work_proof in addition to the existing cases (i.e.,
extend the tuples for ("get_ledger_entry", {"sequence": ...}, 18) and both
("submit_work_proof", {"bounty_id": ...}, 19) and ("submit_work_proof",
{"issue_number": ...}, 20) so each changed bounds path is covered).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 34220a7f-8d29-45d1-8b83-714458332d2a
📒 Files selected for processing (2)
app/main.pytests/test_api_mcp.py
| @pytest.mark.parametrize( | ||
| ("tool_name", "arguments", "request_id"), | ||
| [ | ||
| ("get_bounty", {"id": "9" * 40}, 16), | ||
| ("get_bounty", {"id": 2**63}, 17), | ||
| ("get_ledger_entry", {"sequence": "9" * 40}, 18), | ||
| ("submit_work_proof", {"bounty_id": "9" * 40}, 19), | ||
| ("submit_work_proof", {"issue_number": "9" * 40}, 20), | ||
| ], |
There was a problem hiding this comment.
Expand oversized-integer matrix to cover lower-bound and raw-int paths.
This regression misses the negative overflow branch (< -2**63) and only validates raw oversized JSON int for one tool. Please add cases like -2**63 - 1 and raw 2**63 for get_ledger_entry/submit_work_proof too, so every changed bounds path is exercised.
As per coding guidelines, tests/**/*.py: Add or update tests for changed behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_api_mcp.py` around lines 847 - 855, The parametrized test matrix
for oversized integers misses negative overflow and raw-int paths for some
tools; update the pytest.mark.parametrize data in tests/test_api_mcp.py to add
entries exercising the negative overflow value (-2**63 - 1) and the raw 2**63
value for get_ledger_entry and submit_work_proof in addition to the existing
cases (i.e., extend the tuples for ("get_ledger_entry", {"sequence": ...}, 18)
and both ("submit_work_proof", {"bounty_id": ...}, 19) and ("submit_work_proof",
{"issue_number": ...}, 20) so each changed bounds path is covered).
ayskobtw-lil
left a comment
ayskobtw-lil
left a comment
There was a problem hiding this comment.
No blockers from my review.
Evidence checked:
- Inspected the PR diff for
app/main.pyandtests/test_api_mcp.py. - Verified
int_arg()now rejects bools, non-integer strings, raw integers above SQLite's signed 64-bit range, numeric strings above range, and values below-2**63before they reach SQLAlchemy/SQLite-backed lookups. - Checked the changed MCP paths that consume bounded integers:
get_bounty,get_ledger_entry,submit_work_proofbybounty_id, andsubmit_work_proofbyissue_number. - Confirmed
positive_int_arg()still applies the existing non-positive rejection after range validation, so negative in-range values remain invalid for these positive-only tool arguments. - Non-blocking coverage note: CodeRabbit's suggested matrix expansion would add useful explicit cases for raw
2**63and negative overflow on every tool, but I verified those paths manually and they are covered through the sharedint_arg()behavior.
Validation run on current PR head 130cb8d:
python -m pytest tests/test_api_mcp.py::test_mcp_rejects_oversized_integer_arguments_without_500 tests/test_api_mcp.py::test_mcp_rejects_invalid_string_arguments tests/test_api_mcp.py::test_mcp_get_bounty_rejects_non_positive_id -q->13 passed- Extra manual MCP probe for raw
2**63and-(2**63)-1onget_ledger_entry,submit_work_proof.bounty_id, andsubmit_work_proof.issue_number-> all returned JSON-RPC-32602without 500s. python -m ruff check app/main.py tests/test_api_mcp.py->All checks passed!python -m ruff format --check app/main.py tests/test_api_mcp.py->2 files already formattedgit diff --check HEAD~1..HEAD-> clean
weilixiong
left a comment
weilixiong
left a comment
There was a problem hiding this comment.
✅ Reviewed:
Files: app/main.py (int_arg → bounds check with SQLITE_INTEGER_MAX), tests/test_api_mcp.py (5 parametrized oversized argument tests)
Checked: int_arg now validates parsed value against [-SQLITE_INTEGER_MAX-1, SQLITE_INTEGER_MAX]. Covers int, string, and bool input paths. Consistent with PR#300/#312/#313 hardening pattern. Tests cover get_bounty, get_ledger_entry, submit_work_proof with oversized IDs/sequences.
CI: ✅ Quality/readiness/docs/image
Risk: LOW — 2 files, +53/−4, input validation hardening, consistent with prior PRs
rebel117
left a comment
rebel117
left a comment
There was a problem hiding this comment.
Reviewed the int_arg bounds change.
The core logic is sound: SQLITE_INTEGER_MAX = 2**63 - 1 and the check parsed < -SQLITE_INTEGER_MAX - 1 or parsed > SQLITE_INTEGER_MAX correctly limits accepted values to the SQLite 64-bit signed integer range [-2^63, 2^63-1].
Checked locally:
- pytest tests/test_api_mcp.py -> 68 passed
- ruff check app/main.py -> all checks passed
One observation: the test parametrize for get_bounty id: 2**63 sends a Python int that exceeds JavaScript safe integer range (2^53). This is fine for the Python test client and the JSON-RPC handler, but worth noting that a browser-based JSON-RPC caller would lose precision before the server ever sees the value. Not a blocker for this PR, just a note for anyone extending MCP test coverage later.
No secrets, wallet material, or price claims.
ramimbo
added
mrwk:accepted
5 participants
Summary
get_bounty,get_ledger_entry, andsubmit_work_proofinteger inputsBounty #292
Repro Before Fix
get_bountywithid = "9" * 40returned 500 Internal Server Errorget_ledger_entrywithsequence = "9" * 40returned 500 Internal Server ErrorValidation
uv run pytest tests/test_api_mcp.py::test_mcp_rejects_oversized_integer_arguments_without_500 tests/test_api_mcp.py::test_mcp_get_bounty_rejects_fractional_id tests/test_api_mcp.py::test_mcp_get_bounty_rejects_non_positive_id -q-> 8 passeduv run ruff check app/main.py tests/test_api_mcp.py-> passeduv run mypy app-> passeduv run ruff check .-> passeduv run pytest-> 279 passed, 2 warningsgit diff --check-> passedNo secrets, wallet material, deployment credentials, private vulnerability details, payout details, or MRWK price claims are included.
Summary by CodeRabbit
Bug Fixes
Tests