Skip to content

Reject padded MCP bounty filters#871

Open
xiefuzheng713-alt wants to merge 1 commit into
ramimbo:mainfrom
xiefuzheng713-alt:codex/mcp-list-bounties-padding
Open

Reject padded MCP bounty filters#871
xiefuzheng713-alt wants to merge 1 commit into
ramimbo:mainfrom
xiefuzheng713-alt:codex/mcp-list-bounties-padding

Conversation

@xiefuzheng713-alt
Copy link
Copy Markdown
Contributor

@xiefuzheng713-alt xiefuzheng713-alt commented Jun 4, 2026

Summary

  • reject non-empty leading/trailing whitespace in MCP list_bounties string filter arguments before normalization
  • apply the padding guard only to status, q, sort, and availability
  • keep clean case-insensitive status values like Paid working and keep blank/omitted filters on the existing default paths

Bounty #799
Source report: #798 (comment)

Production evidence before fix

Current production accepts whitespace-padded MCP list_bounties filters as hidden aliases for the clean values:

clean status args: {"status":"open","limit":3}
-> HTTP 200, JSON-RPC result, first ids [109,108,107]

padded status args: {"status":" open ","limit":3}
-> HTTP 200, JSON-RPC result, first ids [109,108,107]

invalid status args: {"status":"bogus","limit":3}
-> HTTP 200, JSON-RPC error -32602 / invalid tool arguments

clean q args: {"status":"open","q":"review","limit":3}
-> HTTP 200, JSON-RPC result, first ids [109,107,97]

padded q args: {"status":"open","q":" review ","limit":3}
-> HTTP 200, JSON-RPC result, first ids [109,107,97]

clean sort args: {"status":"open","sort":"reward","limit":3}
-> HTTP 200, JSON-RPC result, first ids [108,107,106]

padded sort args: {"status":"open","sort":" reward ","limit":3}
-> HTTP 200, JSON-RPC result, first ids [108,107,106]

Duplicate/current check

Validation

  • uv run --extra dev pytest tests/test_api_mcp.py::test_mcp_list_bounties_filters_status_query_and_limit tests/test_api_mcp.py::test_mcp_list_bounties_rejects_invalid_filters tests/test_mcp_tools.py::test_call_mcp_tool_rejects_padded_list_bounty_filters -q -> 16 passed, 1 warning
  • uv run --extra dev pytest tests/test_api_mcp.py tests/test_mcp_tools.py -q -> 120 passed, 1 warning
  • uv run --extra dev ruff check app/mcp_tools.py tests/test_api_mcp.py tests/test_mcp_tools.py -> passed
  • uv run --extra dev ruff format --check app/mcp_tools.py tests/test_api_mcp.py tests/test_mcp_tools.py -> 3 files already formatted
  • uv run --extra dev mypy app/mcp_tools.py -> success
  • uv run --extra dev python scripts/docs_smoke.py -> docs smoke ok
  • git diff --check -> clean

Scope

This PR is limited to read-only MCP list_bounties string filter argument validation. It does not change REST/HTML bounty list behavior, valid clean MCP query semantics, bounty creation, payout execution, treasury mutation, wallet behavior, ledger mutation, admin-token behavior, private data, secrets, bridge/exchange/cash-out behavior, or MRWK price behavior.

Summary by CodeRabbit

  • Bug Fixes

    • Enhanced input validation for MCP tool filters to reject parameters with leading or trailing whitespace.
  • Tests

    • Updated and added test cases to verify strict input validation for filter parameters.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jun 4, 2026

Review Change Stack

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: a03d5e54-dd1f-4acc-8e32-5952b2423128

📥 Commits

Reviewing files that changed from the base of the PR and between d4d0e48 and 7070cda.

📒 Files selected for processing (3)
  • app/mcp_tools.py
  • tests/test_api_mcp.py
  • tests/test_mcp_tools.py

📝 Walkthrough

Walkthrough

This PR tightens argument validation for the MCP list_bounties tool by introducing a reject_padding mode to the argument cleaner. Strings with leading or trailing whitespace are now rejected instead of stripped. The validation is applied to status, q, sort, and availability arguments, and tests are updated to verify the behavior.

Changes

MCP List Bounties Padding Validation

Layer / File(s) Summary
Reject padding validation in optional_clean_str_arg
app/mcp_tools.py, tests/test_mcp_tools.py
optional_clean_str_arg now supports reject_padding parameter that raises ValueError when a string has leading or trailing whitespace. New parametrized test validates this behavior across status, q, sort, and availability fields.
Apply reject_padding to list_bounties filter arguments
app/mcp_tools.py
status argument parsing and sort/availability arguments now use reject_padding=True to enforce strict whitespace validation before normalization.
Update list_bounties filter tests
tests/test_api_mcp.py
Existing test fixed to use canonical "Paid" instead of padded value. Invalid filter test cases extended with padded enum-like strings for all affected fields.

Possibly related issues

  • ramimbo/mergework#779: Proposes fail-closed rejection of leading/trailing whitespace for bounty-list string filters, directly addressed by implementing reject_padding for status, q, sort, and availability.

Possibly related PRs

  • ramimbo/mergework#398: Modifies call_mcp_tool argument validation and normalization for list_bounties at the same code paths.
  • ramimbo/mergework#659: Adds control-character rejection to optional_clean_str_arg before normalization, overlapping validation logic.
  • ramimbo/mergework#286: Introduced list_bounties filter argument handling for status and q fields that this PR further constrains.
🚥 Pre-merge checks | ✅ 6
✅ Passed checks (6 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Reject padded MCP bounty filters' is concrete and directly names the changed surface—a tightened validation for MCP list_bounties string filter arguments.
Description check ✅ Passed The description covers summary, evidence, production behavior, duplicates check, validation results, and scope; it meets the template structure with all critical sections populated.
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 introduces no investment, price, cash-out, off-ramp, fabricated payout, or private security claims. Changes are limited to MCP validation logic and tests.
Bounty Pr Focus ✅ Passed PR diff matches stated files; implementation aligns with summary; test coverage includes padding rejection for all four fields; scope confined to MCP list_bounties filtering.

✏️ 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

@tudorian95 tudorian95 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 7070cdaf3655338c2bf7e263cb904a302442e0ca as a non-author.

Scope reviewed:

  • app/mcp_tools.py now rejects raw leading/trailing whitespace only for non-empty list_bounties string filters: status, q, sort, and availability.
  • Other MCP string arguments continue using the existing trim/control-character behavior because reject_padding defaults to false.
  • Clean case-insensitive status/filter values still work through normalization, while padded aliases now fail before normalization.
  • Regression coverage checks both the direct call_mcp_tool() path and the JSON-RPC /mcp tools/call path for padded filter rejection.

Validation run in an isolated Docker container from the checked-out PR head:

  • uv run --extra dev pytest tests/test_api_mcp.py::test_mcp_list_bounties_filters_status_query_and_limit tests/test_api_mcp.py::test_mcp_list_bounties_rejects_invalid_filters tests/test_mcp_tools.py::test_call_mcp_tool_rejects_padded_list_bounty_filters -q -> 16 passed, 1 existing Starlette/httpx warning.
  • uv run --extra dev ruff check app/mcp_tools.py tests/test_api_mcp.py tests/test_mcp_tools.py -> passed.
  • uv run --extra dev ruff format --check app/mcp_tools.py tests/test_api_mcp.py tests/test_mcp_tools.py -> 3 files already formatted.
  • uv run --extra dev mypy app/mcp_tools.py -> success.
  • uv run --extra dev python scripts/docs_smoke.py -> docs smoke ok.
  • git diff --check origin/main...HEAD -> clean.
  • git merge-tree --write-tree origin/main HEAD -> clean tree c8d53d8e8d03eb566298ee1f0d4fb54bc1246ed3.

GitHub state checked before review: mergeStateStatus=CLEAN, mergeable=MERGEABLE, hosted Quality, readiness, docs, and image checks successful, and no prior reviews on this PR.

No issue found in the focused change. Scope stays limited to read-only MCP list_bounties filter validation and does not touch REST/HTML list behavior, bounty creation, proposal/payout execution, treasury mutation, wallet material, ledger mutation, private data, bridge/exchange/cash-out behavior, secrets, or MRWK price behavior.

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.

Reviewed current head 7070cdaf3655338c2bf7e263cb904a302442e0ca as a non-author.

Evidence checked:

  • inspected app/mcp_tools.py and confirmed reject_padding=True is applied only to the list_bounties MCP status, q, sort, and availability filters, while the helper still trims optional string args for callers that do not opt into padded-input rejection;
  • inspected tests/test_api_mcp.py and verified the existing happy-path status filter now uses canonical "Paid" so padded status is reserved for invalid-input coverage;
  • inspected tests/test_mcp_tools.py and confirmed direct tool coverage rejects leading/trailing whitespace before normalization for all four affected list-bounty filters;
  • ran uv sync --extra dev and PYTHONPATH=. uv run --extra dev pytest tests/test_mcp_tools.py::test_call_mcp_tool_rejects_padded_list_bounty_filters tests/test_api_mcp.py::test_mcp_list_bounties_rejects_invalid_filters tests/test_api_mcp.py::test_mcp_list_bounties_filters_status_query_and_limit -q locally: 16 passed, 1 warning.

No blockers found.

@asddda121
Copy link
Copy Markdown

Bounty #838 review evidence for current head 7070cdaf3655338c2bf7e263cb904a302442e0ca:

I reviewed the focused MCP list_bounties padding change across app/mcp_tools.py, tests/test_api_mcp.py, and tests/test_mcp_tools.py. The implementation rejects non-empty leading/trailing whitespace for status, q, sort, and availability before the normal normalizers run, while preserving omitted/blank defaults and leaving unrelated MCP fields such as repo unchanged. That matches the reported MCP-only scope and avoids changing REST/public page behavior.

Local verification on this PR branch:

  • python -m pytest tests/test_api_mcp.py tests/test_mcp_tools.py -q -> 120 passed, 1 Starlette/httpx deprecation warning
  • python -m ruff check app/mcp_tools.py tests/test_api_mcp.py tests/test_mcp_tools.py -> passed
  • python -m ruff format --check app/mcp_tools.py tests/test_api_mcp.py tests/test_mcp_tools.py -> already formatted
  • python scripts/docs_smoke.py -> docs smoke ok
  • git diff --check origin/main...HEAD -> clean
  • git merge-tree --write-tree origin/main HEAD -> clean tree c8d53d8e8d03eb566298ee1f0d4fb54bc1246ed3

Note: my local environment has Python 3.11 only, while the project declares Python >=3.12, so the runtime test evidence is useful but not a replacement for CI on the supported version. I did not find a blocking issue in this diff.

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.

4 participants