From d16094c4e23e9a9dd2e54c77db98884201833a69 Mon Sep 17 00:00:00 2001 From: Tu Pham Date: Tue, 26 May 2026 08:22:41 +0700 Subject: [PATCH 1/7] Scope MCP work proof by repo Refs #315 --- app/main.py | 23 ++++++++++++----- app/mcp.py | 5 +++- tests/test_api_mcp.py | 57 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 78 insertions(+), 7 deletions(-) diff --git a/app/main.py b/app/main.py index 7e7c4f9..3160af9 100644 --- a/app/main.py +++ b/app/main.py @@ -1530,6 +1530,14 @@ def output_format_arg() -> str: raise ValueError("format must be text or json") return normalized + def optional_repo_selector_arg() -> str | None: + repo = optional_clean_str_arg("repo") + if repo is None: + return None + if len(repo) > 200: + raise ValueError("repo is too long") + return repo.lower() + def mcp_issue_number_search_value(query_text: str) -> int | None: if not query_text.isdigit(): return None @@ -1756,8 +1764,11 @@ def optional_bool_arg(field: str, default: bool = False) -> bool: output_format = output_format_arg() has_bounty_id = "bounty_id" in args and args.get("bounty_id") is not None has_issue_number = "issue_number" in args and args.get("issue_number") is not None + repo_selector = optional_repo_selector_arg() if has_bounty_id and has_issue_number: raise ValueError("use bounty_id or issue_number, not both") + if repo_selector is not None and not has_issue_number: + raise ValueError("repo can only be used with issue_number") if has_bounty_id: bounty = session.get(Bounty, positive_int_arg("bounty_id")) if bounty is None: @@ -1768,12 +1779,12 @@ def optional_bool_arg(field: str, default: bool = False) -> bool: else work_proof_guidance(bounty) ) if has_issue_number: - bounties = session.scalars( - select(Bounty) - .where(Bounty.issue_number == positive_int_arg("issue_number")) - .order_by(Bounty.id.desc()) - .limit(2) - ).all() + issue_query = select(Bounty).where( + Bounty.issue_number == positive_int_arg("issue_number") + ) + if repo_selector is not None: + issue_query = issue_query.where(func.lower(Bounty.repo) == repo_selector) + bounties = session.scalars(issue_query.order_by(Bounty.id.desc()).limit(2)).all() if not bounties: return "bounty not found" if len(bounties) > 1: diff --git a/app/mcp.py b/app/mcp.py index 92567a8..5d1c028 100644 --- a/app/mcp.py +++ b/app/mcp.py @@ -38,7 +38,10 @@ {"name": "get_proof", "description": "Get a public proof by hash"}, { "name": "submit_work_proof", - "description": "Return submission instructions, optionally for a bounty_id or issue_number", + "description": ( + "Return submission instructions, optionally for a bounty_id or issue_number " + "and repo, with text or json format" + ), }, ] diff --git a/tests/test_api_mcp.py b/tests/test_api_mcp.py index adf2a36..dfa4fef 100644 --- a/tests/test_api_mcp.py +++ b/tests/test_api_mcp.py @@ -1542,6 +1542,59 @@ def test_mcp_submit_work_proof_reports_unknown_bounty(sqlite_url: str) -> None: assert result["result"]["content"][0]["text"] == "bounty not found" +def test_mcp_submit_work_proof_scopes_issue_number_by_repo(sqlite_url: str) -> None: + create_schema(sqlite_url) + with session_scope(sqlite_url) as session: + ensure_genesis(session) + create_bounty( + session, + repo="ramimbo/mergework", + issue_number=284, + issue_url="https://github.com/ramimbo/mergework/issues/284", + title="First bounty", + reward_mrwk="100", + acceptance="First acceptance.", + ) + target = create_bounty( + session, + repo="example/mergework", + issue_number=284, + issue_url="https://github.com/example/mergework/issues/284", + title="Second bounty", + reward_mrwk="250", + acceptance="Second acceptance.", + ) + target_id = target.id + + client = TestClient(create_app(database_url=sqlite_url, webhook_secret="secret")) + + response = client.post( + "/mcp", + json={ + "jsonrpc": "2.0", + "id": 28, + "method": "tools/call", + "params": { + "name": "submit_work_proof", + "arguments": { + "issue_number": 284, + "repo": "Example/MergeWork", + "format": "json", + }, + }, + }, + ) + + result = response.json()["result"] + structured = result["structuredContent"] + assert json.loads(result["content"][0]["text"]) == structured + assert structured["bounty_id"] == target_id + assert structured["repository"] == "example/mergework" + assert structured["title"] == "Second bounty" + assert structured["reward_mrwk"] == "250" + assert structured["acceptance"] == "Second acceptance." + + @pytest.mark.parametrize( ("arguments", "request_id"), [ @@ -1552,6 +1605,10 @@ def test_mcp_submit_work_proof_reports_unknown_bounty(sqlite_url: str) -> None: ({"bounty_id": 1, "issue_number": 1}, 25), ({"format": "xml"}, 26), ({"format": 1}, 27), + ({"repo": "ramimbo/mergework"}, 29), + ({"bounty_id": 1, "repo": "ramimbo/mergework"}, 30), + ({"issue_number": 1, "repo": 1}, 31), + ({"issue_number": 1, "repo": "a" * 201}, 32), ], ) def test_mcp_submit_work_proof_rejects_invalid_bounty_selectors( From 3fa5e0f3ec751c652c4afe372692d02fd36e9ded Mon Sep 17 00:00:00 2001 From: Tu Pham Date: Tue, 26 May 2026 08:25:09 +0700 Subject: [PATCH 2/7] Chan OAuth next da ma hoa --- app/main.py | 6 +++++- tests/test_security.py | 3 +++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/app/main.py b/app/main.py index 3160af9..ef34a74 100644 --- a/app/main.py +++ b/app/main.py @@ -9,7 +9,7 @@ from datetime import UTC, datetime, timedelta from pathlib import Path from typing import Annotated, Any -from urllib.parse import urlencode, urlsplit, urlunsplit +from urllib.parse import unquote, urlencode, urlsplit, urlunsplit import httpx from fastapi import Depends, FastAPI, Form, HTTPException, Query, Request @@ -278,13 +278,17 @@ def _oauth_configured(settings: Settings) -> bool: def _safe_next_path(next_path: str | None) -> str: + decoded_next_path = unquote(next_path) if next_path else "" if ( not next_path or not next_path.startswith("/") or next_path.startswith("//") or len(next_path) > 2048 or "\\" in next_path + or decoded_next_path.startswith("//") + or "\\" in decoded_next_path or any(ord(char) < 32 or 127 <= ord(char) < 160 for char in next_path) + or any(ord(char) < 32 or 127 <= ord(char) < 160 for char in decoded_next_path) ): return "/me" return next_path diff --git a/tests/test_security.py b/tests/test_security.py index a1288f0..875ca48 100644 --- a/tests/test_security.py +++ b/tests/test_security.py @@ -856,7 +856,10 @@ def test_admin_bounty_api_accepts_multi_award_count( ("https://evil.example/me", "/me"), ("//evil.example/me", "/me"), ("/\\evil.example/me", "/me"), + ("/%2f%2fevil.example/me", "/me"), + ("/%5cevil.example/me", "/me"), ("/me\nLocation: https://evil.example", "/me"), + ("/me%0d%0aLocation:%20https://evil.example", "/me"), ("/me" + chr(0x85), "/me"), ("/me\x7f", "/me"), ("/" + ("a" * 2048), "/me"), From 78f39be72961a1520f10d8883a3f74d1efeb39bb Mon Sep 17 00:00:00 2001 From: Tu Pham Date: Tue, 26 May 2026 08:26:42 +0700 Subject: [PATCH 3/7] Sua type check ledger --- app/ledger/service.py | 37 +++++++++++++++---------------------- 1 file changed, 15 insertions(+), 22 deletions(-) diff --git a/app/ledger/service.py b/app/ledger/service.py index 3812285..295ae46 100644 --- a/app/ledger/service.py +++ b/app/ledger/service.py @@ -6,11 +6,10 @@ import re from datetime import UTC, datetime from decimal import Decimal, InvalidOperation -from typing import Any, cast +from typing import Any from urllib.parse import urlparse from sqlalchemy import case, func, select, update -from sqlalchemy.engine import CursorResult from sqlalchemy.exc import IntegrityError from sqlalchemy.orm import Session @@ -657,19 +656,16 @@ def pay_bounty( reserve_account = reserve_account_for_bounty(bounty.id) if get_balance(session, reserve_account) < bounty.reward_microunits: raise LedgerError("bounty reserve balance too low") - claimed = cast( - CursorResult[Any], - session.execute( - update(Bounty) - .where(Bounty.id == bounty.id, Bounty.awards_paid < Bounty.max_awards) - .values( - awards_paid=Bounty.awards_paid + 1, - status=case( - (Bounty.awards_paid + 1 >= Bounty.max_awards, "paid"), - else_="open", - ), - ) - ), + claimed = session.execute( + update(Bounty) + .where(Bounty.id == bounty.id, Bounty.awards_paid < Bounty.max_awards) + .values( + awards_paid=Bounty.awards_paid + 1, + status=case( + (Bounty.awards_paid + 1 >= Bounty.max_awards, "paid"), + else_="open", + ), + ) ) if claimed.rowcount != 1: raise LedgerError("bounty already paid") @@ -737,13 +733,10 @@ def close_bounty( raise LedgerError("bounty is not open") _clean_required_text(closed_by, "closed_by", 80) clean_reference = validate_public_url(reference or bounty.issue_url) - claimed = cast( - CursorResult[Any], - session.execute( - update(Bounty) - .where(Bounty.id == bounty.id, Bounty.status == "open") - .values(status="closed") - ), + claimed = session.execute( + update(Bounty) + .where(Bounty.id == bounty.id, Bounty.status == "open") + .values(status="closed") ) if claimed.rowcount != 1: raise LedgerError("bounty is not open") From 2a0037a34917675fe4750067c9172b3e51716e55 Mon Sep 17 00:00:00 2001 From: Tu Pham Date: Tue, 26 May 2026 08:26:42 +0700 Subject: [PATCH 4/7] Sua type check ledger --- app/ledger/service.py | 37 +++++++++++++++---------------------- app/main.py | 17 ++++++++++++----- app/mcp.py | 4 ++-- tests/test_api_mcp.py | 2 +- 4 files changed, 30 insertions(+), 30 deletions(-) diff --git a/app/ledger/service.py b/app/ledger/service.py index 3812285..295ae46 100644 --- a/app/ledger/service.py +++ b/app/ledger/service.py @@ -6,11 +6,10 @@ import re from datetime import UTC, datetime from decimal import Decimal, InvalidOperation -from typing import Any, cast +from typing import Any from urllib.parse import urlparse from sqlalchemy import case, func, select, update -from sqlalchemy.engine import CursorResult from sqlalchemy.exc import IntegrityError from sqlalchemy.orm import Session @@ -657,19 +656,16 @@ def pay_bounty( reserve_account = reserve_account_for_bounty(bounty.id) if get_balance(session, reserve_account) < bounty.reward_microunits: raise LedgerError("bounty reserve balance too low") - claimed = cast( - CursorResult[Any], - session.execute( - update(Bounty) - .where(Bounty.id == bounty.id, Bounty.awards_paid < Bounty.max_awards) - .values( - awards_paid=Bounty.awards_paid + 1, - status=case( - (Bounty.awards_paid + 1 >= Bounty.max_awards, "paid"), - else_="open", - ), - ) - ), + claimed = session.execute( + update(Bounty) + .where(Bounty.id == bounty.id, Bounty.awards_paid < Bounty.max_awards) + .values( + awards_paid=Bounty.awards_paid + 1, + status=case( + (Bounty.awards_paid + 1 >= Bounty.max_awards, "paid"), + else_="open", + ), + ) ) if claimed.rowcount != 1: raise LedgerError("bounty already paid") @@ -737,13 +733,10 @@ def close_bounty( raise LedgerError("bounty is not open") _clean_required_text(closed_by, "closed_by", 80) clean_reference = validate_public_url(reference or bounty.issue_url) - claimed = cast( - CursorResult[Any], - session.execute( - update(Bounty) - .where(Bounty.id == bounty.id, Bounty.status == "open") - .values(status="closed") - ), + claimed = session.execute( + update(Bounty) + .where(Bounty.id == bounty.id, Bounty.status == "open") + .values(status="closed") ) if claimed.rowcount != 1: raise LedgerError("bounty is not open") diff --git a/app/main.py b/app/main.py index ef34a74..3e638db 100644 --- a/app/main.py +++ b/app/main.py @@ -1539,7 +1539,7 @@ def optional_repo_selector_arg() -> str | None: if repo is None: return None if len(repo) > 200: - raise ValueError("repo is too long") + raise ValueError("repo is too long (max 200 characters)") return repo.lower() def mcp_issue_number_search_value(query_text: str) -> int | None: @@ -1783,12 +1783,19 @@ def optional_bool_arg(field: str, default: bool = False) -> bool: else work_proof_guidance(bounty) ) if has_issue_number: - issue_query = select(Bounty).where( - Bounty.issue_number == positive_int_arg("issue_number") - ) + issue_number = positive_int_arg("issue_number") + issue_query = select(Bounty).where(Bounty.issue_number == issue_number) if repo_selector is not None: - issue_query = issue_query.where(func.lower(Bounty.repo) == repo_selector) + issue_query = issue_query.where(Bounty.repo == repo_selector) bounties = session.scalars(issue_query.order_by(Bounty.id.desc()).limit(2)).all() + if repo_selector is not None and not bounties: + legacy_issue_query = select(Bounty).where( + Bounty.issue_number == issue_number, + func.lower(Bounty.repo) == repo_selector, + ) + bounties = session.scalars( + legacy_issue_query.order_by(Bounty.id.desc()).limit(2) + ).all() if not bounties: return "bounty not found" if len(bounties) > 1: diff --git a/app/mcp.py b/app/mcp.py index 5d1c028..390d563 100644 --- a/app/mcp.py +++ b/app/mcp.py @@ -39,8 +39,8 @@ { "name": "submit_work_proof", "description": ( - "Return submission instructions, optionally for a bounty_id or issue_number " - "and repo, with text or json format" + "Return submission instructions for a bounty_id, or for an issue_number " + "optionally scoped by repo; supports text or json format" ), }, ] diff --git a/tests/test_api_mcp.py b/tests/test_api_mcp.py index dfa4fef..429705c 100644 --- a/tests/test_api_mcp.py +++ b/tests/test_api_mcp.py @@ -429,7 +429,7 @@ def test_mcp_tools_list_and_call(sqlite_url: str) -> None: submit_tool = next( tool for tool in tools["result"]["tools"] if tool["name"] == "submit_work_proof" ) - assert "bounty_id or issue_number" in submit_tool["description"] + assert "issue_number optionally scoped by repo" in submit_tool["description"] bounty_tool = next(tool for tool in tools["result"]["tools"] if tool["name"] == "get_bounty") assert "accepted awards" in bounty_tool["description"] attempt_tool = next( From 8fb911350be3fee947c537d6cd9168fe045a2204 Mon Sep 17 00:00:00 2001 From: Tu Pham Date: Tue, 26 May 2026 08:33:31 +0700 Subject: [PATCH 5/7] Sua MCP guidance va type check --- app/ledger/service.py | 37 ++++++++++++++++++++++--------------- app/mcp.py | 4 ++-- 2 files changed, 24 insertions(+), 17 deletions(-) diff --git a/app/ledger/service.py b/app/ledger/service.py index 295ae46..3812285 100644 --- a/app/ledger/service.py +++ b/app/ledger/service.py @@ -6,10 +6,11 @@ import re from datetime import UTC, datetime from decimal import Decimal, InvalidOperation -from typing import Any +from typing import Any, cast from urllib.parse import urlparse from sqlalchemy import case, func, select, update +from sqlalchemy.engine import CursorResult from sqlalchemy.exc import IntegrityError from sqlalchemy.orm import Session @@ -656,16 +657,19 @@ def pay_bounty( reserve_account = reserve_account_for_bounty(bounty.id) if get_balance(session, reserve_account) < bounty.reward_microunits: raise LedgerError("bounty reserve balance too low") - claimed = session.execute( - update(Bounty) - .where(Bounty.id == bounty.id, Bounty.awards_paid < Bounty.max_awards) - .values( - awards_paid=Bounty.awards_paid + 1, - status=case( - (Bounty.awards_paid + 1 >= Bounty.max_awards, "paid"), - else_="open", - ), - ) + claimed = cast( + CursorResult[Any], + session.execute( + update(Bounty) + .where(Bounty.id == bounty.id, Bounty.awards_paid < Bounty.max_awards) + .values( + awards_paid=Bounty.awards_paid + 1, + status=case( + (Bounty.awards_paid + 1 >= Bounty.max_awards, "paid"), + else_="open", + ), + ) + ), ) if claimed.rowcount != 1: raise LedgerError("bounty already paid") @@ -733,10 +737,13 @@ def close_bounty( raise LedgerError("bounty is not open") _clean_required_text(closed_by, "closed_by", 80) clean_reference = validate_public_url(reference or bounty.issue_url) - claimed = session.execute( - update(Bounty) - .where(Bounty.id == bounty.id, Bounty.status == "open") - .values(status="closed") + claimed = cast( + CursorResult[Any], + session.execute( + update(Bounty) + .where(Bounty.id == bounty.id, Bounty.status == "open") + .values(status="closed") + ), ) if claimed.rowcount != 1: raise LedgerError("bounty is not open") diff --git a/app/mcp.py b/app/mcp.py index 390d563..2e819cd 100644 --- a/app/mcp.py +++ b/app/mcp.py @@ -39,8 +39,8 @@ { "name": "submit_work_proof", "description": ( - "Return submission instructions for a bounty_id, or for an issue_number " - "optionally scoped by repo; supports text or json format" + "Return submission instructions for bounty_id or issue_number optionally " + "scoped by repo; supports text or json format" ), }, ] From de8d72f96358e69cec2e38c5d68dc76c4a3d58d4 Mon Sep 17 00:00:00 2001 From: Tu Pham Date: Tue, 26 May 2026 08:42:06 +0700 Subject: [PATCH 6/7] Them test MCP repo legacy --- tests/test_api_mcp.py | 48 ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 47 insertions(+), 1 deletion(-) diff --git a/tests/test_api_mcp.py b/tests/test_api_mcp.py index 429705c..33c46c5 100644 --- a/tests/test_api_mcp.py +++ b/tests/test_api_mcp.py @@ -9,7 +9,7 @@ from app.db import create_schema, session_scope from app.ledger.service import close_bounty, create_bounty, ensure_genesis, pay_bounty from app.main import create_app -from app.models import BountyAttempt, Proof +from app.models import Bounty, BountyAttempt, Proof def test_health_status_and_bounty_api(sqlite_url: str) -> None: @@ -1595,6 +1595,52 @@ def test_mcp_submit_work_proof_scopes_issue_number_by_repo(sqlite_url: str) -> N assert structured["acceptance"] == "Second acceptance." +def test_mcp_submit_work_proof_scopes_legacy_mixed_case_repo(sqlite_url: str) -> None: + create_schema(sqlite_url) + with session_scope(sqlite_url) as session: + ensure_genesis(session) + bounty = Bounty( + repo="Ramimbo/MergeWork", + issue_number=284, + issue_url="https://github.com/ramimbo/mergework/issues/284", + title="Legacy bounty", + reward_microunits=100_000_000, + reserved_microunits=100_000_000, + max_awards=1, + awards_paid=0, + status="open", + acceptance="Legacy rows should still match repo-scoped MCP guidance.", + ) + session.add(bounty) + session.flush() + bounty_id = bounty.id + + client = TestClient(create_app(database_url=sqlite_url, webhook_secret="secret")) + + response = client.post( + "/mcp", + json={ + "jsonrpc": "2.0", + "id": 33, + "method": "tools/call", + "params": { + "name": "submit_work_proof", + "arguments": { + "issue_number": 284, + "repo": "ramimbo/mergework", + "format": "json", + }, + }, + }, + ) + + result = response.json()["result"] + structured = result["structuredContent"] + assert structured["bounty_id"] == bounty_id + assert structured["repository"] == "Ramimbo/MergeWork" + assert structured["title"] == "Legacy bounty" + + @pytest.mark.parametrize( ("arguments", "request_id"), [ From 40a790440f2c9d6a61ac68d516ca2ab3dc0def37 Mon Sep 17 00:00:00 2001 From: Tu Pham Date: Tue, 26 May 2026 08:46:03 +0700 Subject: [PATCH 7/7] Them test OAuth next ma hoa --- tests/test_security.py | 29 ++++++++++++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) diff --git a/tests/test_security.py b/tests/test_security.py index 875ca48..1ce30cc 100644 --- a/tests/test_security.py +++ b/tests/test_security.py @@ -4,6 +4,7 @@ import hmac import json import re +from urllib.parse import parse_qs, urlparse import pytest from fastapi.testclient import TestClient @@ -26,7 +27,7 @@ register_wallet, validate_public_url, ) -from app.main import _safe_next_path, _signed_value, create_app +from app.main import _safe_next_path, _signed_value, _verified_value, create_app from app.models import Bounty, LedgerEntry, Proof, Submission, WebhookEvent from app.webhooks.github import handle_github_webhook @@ -873,6 +874,32 @@ def test_oauth_next_path_rejects_external_or_headerlike_paths( assert _safe_next_path(next_path) == expected +@pytest.mark.parametrize( + "next_path", + ( + "/%255cevil.example/me", + "/me%250d%250aLocation:%20https://evil.example", + ), +) +def test_github_login_stores_safe_default_for_encoded_next_route( + sqlite_url: str, monkeypatch: pytest.MonkeyPatch, next_path: str +) -> None: + monkeypatch.setenv("MERGEWORK_GITHUB_OAUTH_CLIENT_ID", "client-id") + monkeypatch.setenv("MERGEWORK_GITHUB_OAUTH_CLIENT_SECRET", "client-secret") + monkeypatch.setenv("MERGEWORK_COOKIE_SECRET", "test-cookie-secret") + monkeypatch.setenv("MERGEWORK_PUBLIC_BASE_URL", "https://mrwk.example.test") + client = TestClient(create_app(database_url=sqlite_url, webhook_secret="secret")) + + response = client.get(f"/auth/github/login?next={next_path}", follow_redirects=False) + + assert response.status_code == 302 + query = parse_qs(urlparse(response.headers["location"]).query) + state_value = _verified_value(query["state"][0], "test-cookie-secret", 600) + assert state_value is not None + _nonce, stored_next_path = state_value.split(",", 1) + assert stored_next_path == "/me" + + def test_amount_parser_rejects_non_finite_values() -> None: for amount in ("NaN", "Infinity", "-Infinity"): with pytest.raises(LedgerError, match="invalid MRWK amount"):