Refs #406: Reject control chars in public search queries#572
Refs #406: Reject control chars in public search queries#572xingxi0614-cpu wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThree search endpoints now validate and normalize the ChangesControl Character Validation Across Search Endpoints
Possibly related PRs
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Actionable comments posted: 0 |
eliasx45
left a comment
eliasx45
left a comment
There was a problem hiding this comment.
Reviewed current head 82b6fabe330944c01ab99419cb3b48395b82c4bb as a non-author.
No blocker found. The patch is scoped to rejecting C0/DEL control characters in public search inputs before they are reflected or used for filtering, across bounty list/summary, activity API/page, and MCP list_bounties. The existing empty/whitespace search behavior is preserved, while embedded control characters now fail closed with HTTP 400 or the MCP invalid-argument path.
Validation performed:
git fetch origin main; PR diff confirmed focused to 7 filesgit diff --check origin/main...HEADclean- Focused regression set: 40 passed
python -m ruff check app/activity.py app/bounty_api.py app/mcp_tools.py tests/test_activity.py tests/test_api_mcp.py tests/test_bounty_api_routes.py tests/test_bounty_pages.pypython -m ruff format --check app/activity.py app/bounty_api.py app/mcp_tools.py tests/test_activity.py tests/test_api_mcp.py tests/test_bounty_api_routes.py tests/test_bounty_pages.pypython -m mypy app-> successpython scripts/docs_smoke.py-> docs smoke ok- Full suite:
python -m pytest -q-> 429 passed - Hosted Quality/readiness/docs/image checks are passing
No private data, credentials, wallet material, signatures, production mutation, MRWK price/off-ramp, exchange/liquidity claims, bridge claims, or fabricated payout claims were used.
eliasx45
left a comment
eliasx45
left a comment
There was a problem hiding this comment.
I found one blocker on current head 82b6fabe330944c01ab99419cb3b48395b82c4bb.
The new control-character validation runs after strip() on each search path, so leading/trailing whitespace control characters are silently removed instead of rejected. That means q=%09 (tab) is accepted as an empty search on the public/API/MCP paths even though the new contract says q must not contain control characters.
Affected paths I reproduced locally:
GET /api/v1/bounties?q=%09-> 200 with the normal bounty listGET /api/v1/bounties/summary?q=%09-> 200 with summary dataGET /bounties?q=%09-> 200 page renderGET /api/v1/activity?q=%09-> 200 withquery: ""GET /activity?q=%09-> 200 page render- MCP
list_bountieswith{ "q": "\t" }-> returns normal bounty JSON
Why it happens:
app/bounty_api.py::_normalized_bounty_search_query()callsquery_text.strip()before checking for control characters.app/activity.py::_normalized_activity_search_query()does the same.app/mcp_tools.pygetsqthroughoptional_clean_str_arg(), which strips beforereject_control_chars("q", query_text)is called.
Expected fix: validate the raw query string for control characters before trimming, then apply the existing whitespace normalization. Please add regression coverage for at least a leading/trailing whitespace control character such as q=%09 in REST/page/MCP paths, not only embedded/NUL input.
Validation I ran:
PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 .\.venv\Scripts\python.exe -m pytest tests\test_activity.py::test_activity_rejects_control_character_search_query tests\test_bounty_api_routes.py::test_bounty_api_rejects_control_character_search_queries tests\test_bounty_pages.py::test_bounties_page_and_api_search_by_text_and_issue_number tests\test_api_mcp.py::test_mcp_list_bounties_rejects_invalid_filters -q-> 10 passed- ad hoc TestClient/MCP reproduction above -> tab-only queries accepted on all five HTTP paths plus MCP
.\.venv\Scripts\python.exe -m ruff check app\activity.py app\bounty_api.py app\mcp_tools.py tests\test_activity.py tests\test_api_mcp.py tests\test_bounty_api_routes.py tests\test_bounty_pages.py-> passed.\.venv\Scripts\python.exe -m ruff format --check app\activity.py app\bounty_api.py app\mcp_tools.py tests\test_activity.py tests\test_api_mcp.py tests\test_bounty_api_routes.py tests\test_bounty_pages.py-> 7 files already formattedPYTEST_DISABLE_PLUGIN_AUTOLOAD=1 .\.venv\Scripts\python.exe -m mypy app\activity.py app\bounty_api.py app\mcp_tools.py-> successgit diff --check origin/main...HEAD-> clean
|
Holding with |
barnacleagent-svg
left a comment
barnacleagent-svg
left a comment
There was a problem hiding this comment.
Verdict: APPROVED
Scope: Adds control character validation to activity search query. Uses _normalized_activity_search_query helper to strip/validate before passing to activity_context.
Checklist:
- Diff: focused validation, consistent with existing CONTROL_CHAR patterns
- Tests pass
- Follows same pattern as prior PRs (#609, #608, etc.)
Conclusion: Clean input validation. Ready to merge.
|
Thanks, good catch. I pushed a follow-up that validates the raw Follow-up commit: 7893c23 Updated coverage:
Validation:
No private data, secrets, wallet material, production mutation, price/off-ramp, liquidity, exchange, bridge-promise, or private security details are included. |
eliasx45
left a comment
eliasx45
left a comment
There was a problem hiding this comment.
Re-reviewed follow-up head 7893c231f08093e460f2ef5402cd8e03fb5d555e.
Verdict: approve implementation, with one merge-readiness caveat.
The raw-control-character blocker from my previous review is addressed. The follow-up now validates raw q before trimming on the HTTP search paths and validates raw MCP string input before accepting list_bounties filters. The added/updated tests cover leading/trailing control characters such as tab/newline as requested.
Validation:
git diff --check origin/main...HEAD-> clean.- Focused regression slice:
PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 ..\mergework\.venv\Scripts\python.exe -m pytest tests\test_activity.py::test_activity_rejects_control_character_search_query tests\test_bounty_api_routes.py::test_bounty_api_rejects_control_character_search_queries tests\test_bounty_pages.py::test_bounties_page_and_api_search_by_text_and_issue_number tests\test_api_mcp.py::test_mcp_list_bounties_rejects_invalid_filters -q --tb=short-> 12 passed. ..\mergework\.venv\Scripts\python.exe -m ruff check app\activity.py app\bounty_api.py app\mcp_tools.py tests\test_activity.py tests\test_api_mcp.py tests\test_bounty_api_routes.py tests\test_bounty_pages.py-> passed...\mergework\.venv\Scripts\python.exe -m ruff format --check ...-> 7 files already formatted...\mergework\.venv\Scripts\python.exe -m mypy app\activity.py app\bounty_api.py app\mcp_tools.py-> success.- GitHub checks currently shown green/skipped as non-blocking.
Merge-readiness caveat: this branch is currently reported as CONFLICTING, and local git merge-tree --write-tree origin/main HEAD reports a content conflict in app/bounty_api.py. Please resolve/rebase before maintainer merge. The code issue I previously raised is fixed.
xingxi0614-cpu
force-pushed
the
codex/b406-bounty-search-control-query
branch
from
7893c23 to
50d4daa
Compare
|
Rebased this branch onto the latest Current head: What I kept during the conflict resolution:
Validation after rebase:
GitHub checks have restarted on the rebased head. No private data, secrets, wallet material, production mutation, price/off-ramp, liquidity, exchange, bridge-promise, or private security details are included. |
![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
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ce877525-9535-47a7-9376-713805290094
📒 Files selected for processing (7)
app/activity.pyapp/bounty_api.pyapp/mcp_tools.pytests/test_activity.pytests/test_api_mcp.pytests/test_bounty_api_routes.pytests/test_bounty_pages.py
eliasx45
left a comment
eliasx45
left a comment
There was a problem hiding this comment.
Re-reviewed rebased head 50d4daab3b365083bafe8316c26fb4d59e504d31.
Verdict: approve.
The previous merge-conflict caveat is cleared after rebasing onto current main. I verified the raw-control-character fix still validates before trimming across the HTTP search paths and MCP list_bounties, and the focused regressions still cover leading/trailing control characters.
Validation on this checkout:
git fetch origin main
git merge-tree --write-tree origin/main refs/remotes/pr/572
# 8594b49eb2531c6a489b301734e51e24fcdde4c5
git diff --check origin/main...refs/remotes/pr/572
# clean
PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 ..\mergework\.venv\Scripts\python.exe -m pytest tests\test_activity.py::test_activity_rejects_control_character_search_query tests\test_bounty_api_routes.py::test_bounty_api_rejects_control_character_search_queries tests\test_bounty_pages.py::test_bounties_page_and_api_search_by_text_and_issue_number tests\test_api_mcp.py::test_mcp_list_bounties_rejects_invalid_filters -q --tb=short
# 12 passed
..\mergework\.venv\Scripts\python.exe -m ruff check app\activity.py app\bounty_api.py app\mcp_tools.py tests\test_activity.py tests\test_api_mcp.py tests\test_bounty_api_routes.py tests\test_bounty_pages.py
# passed
..\mergework\.venv\Scripts\python.exe -m ruff format --check app\activity.py app\bounty_api.py app\mcp_tools.py tests\test_activity.py tests\test_api_mcp.py tests\test_bounty_api_routes.py tests\test_bounty_pages.py
# 7 files already formatted
..\mergework\.venv\Scripts\python.exe -m mypy app\activity.py app\bounty_api.py app\mcp_tools.py
# success
GitHub checks are green on the rebased head. I do not see a remaining blocker.
|
Addressed the CodeRabbit DEL-boundary test suggestion. Follow-up commit: 1165d5f What changed:
Validation:
No private data, secrets, wallet material, production mutation, price/off-ramp, liquidity, exchange, bridge-promise, or private security details are included. |
eliasx45
left a comment
eliasx45
left a comment
There was a problem hiding this comment.
Re-reviewed latest head 1165d5fc624e0711761ad00a7733f3308264e26c after the DEL-boundary follow-up.
Verdict: approve.
The new commit is test-only and adds \x7f coverage for both /api/v1/bounties?q=... and /api/v1/bounties/summary?q=..., which addresses CodeRabbit's remaining boundary-test suggestion without changing production code.
Validation on this checkout:
git diff --check origin/main...HEAD
# clean
git merge-tree --write-tree origin/main HEAD
# 49cb272575087106434379a0c848918abc8cea57
PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 ..\mergework\.venv\Scripts\python.exe -m pytest tests\test_bounty_api_routes.py::test_bounty_api_rejects_control_character_search_queries -q --tb=short
# 1 passed
..\mergework\.venv\Scripts\python.exe -m ruff check tests\test_bounty_api_routes.py
# All checks passed
..\mergework\.venv\Scripts\python.exe -m ruff format --check tests\test_bounty_api_routes.py
# 1 file already formatted
No blocker found.
|
Thanks again for the review and maintainer cleanup note. I believe the earlier Current state:
Could you please re-check and clear |
|
Quick follow-up: this PR still has the mrwk:needs-info label, but the raw-query validation blocker appears to be addressed at current head 1165d5f. Current state:
Could you please re-check and clear mrwk:needs-info if this now satisfies the requested change? |
|
Re-checked current head 1165d5f. The raw-control-character search blocker is cleared: raw q is validated before trimming across the HTTP search paths and MCP list_bounties, with tab/newline/DEL coverage. Waiting for a second useful current-head review before merge. |
4 participants
Summary
qsearch parameters before they are reflected or used for filteringlist_bounties%00so control-character searches fail closed with bounded400responses on API/HTML routes and invalid-argument JSON-RPC errors on MCPEvidence
Live public preflight before this fix showed control-character searches were accepted on public search endpoints:
GET https://api.mrwk.ltclab.site/api/v1/bounties?q=%00-> HTTP 200 and returned current bounty rowsGET https://api.mrwk.ltclab.site/api/v1/bounties/summary?q=%00-> HTTP 200 withbounties_shown=77GET https://api.mrwk.ltclab.site/api/v1/activity?q=%00-> HTTP 200 withquery:\u0000That is misleading because non-empty search parameters should not silently widen to all bounty rows or reflect control characters in public JSON. The fix now rejects C0/DEL control characters with bounded
400responses.Validation
uv run --extra dev python -m pytest tests/test_activity.py tests/test_bounty_api_routes.py tests/test_bounty_api.py tests/test_bounty_pages.py 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 -q-> 40 passeduv run --extra dev python -m pytest -q-> 429 passeduv run --extra dev ruff check app/activity.py app/bounty_api.py app/mcp_tools.py tests/test_activity.py tests/test_api_mcp.py tests/test_bounty_api_routes.py tests/test_bounty_pages.py-> passeduv run --extra dev ruff format --check app/activity.py app/bounty_api.py app/mcp_tools.py tests/test_activity.py tests/test_api_mcp.py tests/test_bounty_api_routes.py tests/test_bounty_pages.py-> 7 files already formatteduv run --extra dev python -m mypy app-> successuv run --extra dev python scripts/docs_smoke.py-> docs smoke okgit diff --check HEAD~1..HEAD-> cleanSafety
No private data, cookies, tokens, wallet material, signatures, production mutation, price claims, liquidity claims, exchange claims, bridge promises, off-ramp promises, or private security details are included.
Summary by CodeRabbit