Guard MCP bounty search oversized numeric query#313
Conversation
📝 WalkthroughWalkthroughBoth the public ChangesIssue-number parsing robustness
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 `@app/main.py`:
- Line 1723: Replace the hardcoded literal 2**63 - 1 with the named constant
SQLITE_INTEGER_MAX to match the other helper `_issue_number_search_value`;
locate the return in the function that currently does `return issue_number if
issue_number <= 2**63 - 1 else None` and change the comparison to use
`SQLITE_INTEGER_MAX` so the code is consistent and uses the shared constant
defined earlier.
🪄 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: fad1160d-d177-4e20-b72c-86eb5c03805d
📒 Files selected for processing (2)
app/main.pytests/test_api_mcp.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/main.py (1)
123-129: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winApply the same ValueError protection for consistency.
The function
_issue_number_search_valuehas the same vulnerability that was fixed inmcp_issue_number_search_value: callingint(query)on an oversized digit string (e.g., 5000 digits) will raise aValueErrorbefore theSQLITE_INTEGER_MAXcheck. This affects the regular API endpoint/api/v1/bountieswhen filtering by query.For consistency and robustness, wrap the
int()call in a try/except block matching the pattern used in the MCP variant.♻️ Proposed fix
def _issue_number_search_value(query: str) -> int | None: if not query.isdigit(): return None - issue_number = int(query) - if issue_number > SQLITE_INTEGER_MAX: + try: + issue_number = int(query) + except ValueError: return None - return issue_number + return issue_number if issue_number <= SQLITE_INTEGER_MAX else None🤖 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 `@app/main.py` around lines 123 - 129, The _issue_number_search_value function can raise ValueError when int(query) is called on extremely long digit strings; update _issue_number_search_value to wrap the int(query) conversion in a try/except ValueError block (matching the pattern used in mcp_issue_number_search_value) so that on ValueError it returns None, then proceed to check issue_number against SQLITE_INTEGER_MAX and return the integer otherwise.
🤖 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.
Outside diff comments:
In `@app/main.py`:
- Around line 123-129: The _issue_number_search_value function can raise
ValueError when int(query) is called on extremely long digit strings; update
_issue_number_search_value to wrap the int(query) conversion in a try/except
ValueError block (matching the pattern used in mcp_issue_number_search_value) so
that on ValueError it returns None, then proceed to check issue_number against
SQLITE_INTEGER_MAX and return the integer otherwise.
weilixiong
left a comment
weilixiong
left a comment
There was a problem hiding this comment.
✅ Reviewed:
Files: app/main.py (try/except int() in mcp_issue_number_search_value, using SQLITE_INTEGER_MAX), tests/test_api_mcp.py (oversized query regression test)
Checked: 9*5000 → ValueError → None → empty [] MCP response. Uses existing SQLITE_INTEGER_MAX constant from PR#300. Consistent with PR#312 approach.
CI: ✅ quality/readiness/docs/image
Risk: LOW — 2 files, MCP search hardening, +20/−2
|
Follow-up pushed in 34de778 for the CodeRabbit outside-diff finding. What changed:
Validation:
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/main.py (1)
1717-1724: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider eliminating code duplication.
The nested
mcp_issue_number_search_valueduplicates the logic of the module-level_issue_number_search_value(lines 123-130). Both functions have identical parsing logic and constraints.Consider refactoring to reuse the shared implementation:
♻️ Proposed refactor to eliminate duplication
def mcp_issue_number_search_value(query_text: str) -> int | None: - if not query_text.isdigit(): - return None - try: - issue_number = int(query_text) - except ValueError: - return None - return issue_number if issue_number <= SQLITE_INTEGER_MAX else None + return _issue_number_search_value(query_text)🤖 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 `@app/main.py` around lines 1717 - 1724, The mcp_issue_number_search_value function duplicates parsing/validation logic already implemented in the module-level _issue_number_search_value; update mcp_issue_number_search_value to reuse that shared implementation (either by calling _issue_number_search_value(query_text) or extracting the common logic into a new helper used by both) so the int conversion, digit-check and SQLITE_INTEGER_MAX bound are maintained via the single shared routine; ensure the function signature and return type remain int | None and adjust any references accordingly.
🤖 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.
Outside diff comments:
In `@app/main.py`:
- Around line 1717-1724: The mcp_issue_number_search_value function duplicates
parsing/validation logic already implemented in the module-level
_issue_number_search_value; update mcp_issue_number_search_value to reuse that
shared implementation (either by calling _issue_number_search_value(query_text)
or extracting the common logic into a new helper used by both) so the int
conversion, digit-check and SQLITE_INTEGER_MAX bound are maintained via the
single shared routine; ensure the function signature and return type remain int
| None and adjust any references accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1663bd4a-8033-4bf1-9869-54c7d3134114
📒 Files selected for processing (2)
app/main.pytests/test_bounty_pages.py
ramimbo
added
mrwk:accepted
3 participants
Bounty #292
Summary
list_bountiesissue-number search parsing against Python integer digit-limit failures.qvalues on the same text-search/no-match path as already oversized SQLite integers instead of returning an MCP invalid-arguments error.Repro before fix
Calling the MCP
list_bountiestool withq = "9" * 5000returned a JSON-RPC invalid tool arguments error becausemcp_issue_number_search_value()attemptedint(query_text)and Python raised a digit-limitValueError.Expected behavior: treat that query as text-only search, like the existing oversized numeric query case, and return an empty result set when nothing matches.
Validation
uv run --with pytest pytest tests/test_api_mcp.py::test_mcp_list_bounties_filters_status_query_and_limit -q-> 1 passed.uv run --with ruff ruff check app/main.py tests/test_api_mcp.py-> passed.uv run --with ruff ruff format --check app/main.py tests/test_api_mcp.py-> 2 files already formatted.uv run --with mypy mypy app-> success.git diff --check-> clean.No secrets, wallet material, deployment credentials, private vulnerability details, payout details, or MRWK price claims are included.
Summary by CodeRabbit
Bug Fixes
Tests