Skip to content

Commit 31ff06b

Browse files
Maselkovclaude
andcommitted
Merge mcp-oauth's review fixes; write tools: review fixes
- request_status expires the queue before reading a request's row: a request the game never picks up is otherwise only marked failed when the game or the page next looks, and kept reading "waiting". - cancel_craft requires the job_id when the CPU reports what it's making: without it, a CPU that moved on to another job since the user agreed would have that one cancelled. - Stock rule amounts must be finite (on the page too): JSON's 1e400 is infinity, which a refill would then ask the game for. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2 parents 0f57233 + fe27e93 commit 31ff06b

10 files changed

Lines changed: 202 additions & 56 deletions

File tree

‎docs/CONTEXT.md‎

Lines changed: 17 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -801,19 +801,23 @@ routes/oauth.py, store/oauth.py) answers 401 with a pointer to its
801801
protected resource metadata (RFC 9728), which names this server as the
802802
authorization server (RFC 8414). Clients register themselves (RFC 7591,
803803
public clients only; Claude registers on every new connection, so unused
804-
registrations are dropped after a day and capped per hour). A Client ID
805-
Metadata Document isn't supported: Claude falls back to registering when
806-
the metadata doesn't advertise one. The consent page signs in with the
807-
user's existing access token, so there's no new credential; it's plain
808-
HTML with its own CSP, whose `form-action` allows the app's callback
809-
origin, as Chrome applies `form-action` to the redirect after a POST.
810-
PKCE is S256 only. Loopback redirect URIs match on any port (RFC 8252),
811-
as Claude Code picks a free port each time. Access tokens last an hour
812-
and refresh tokens 30 days; refresh tokens rotate, and one used twice
813-
revokes its grant (it was copied). A grant remembers the login token the
814-
user consented with, so revoking that token, **New token** or deleting
815-
the user ends it too. `PUBLIC_URL`, when set, is the issuer and resource
816-
URL: behind a proxy the request's own host may not be what clients use.
804+
registrations are dropped after a day, and capped per hour, per address
805+
and in all). A Client ID Metadata Document isn't supported: Claude falls
806+
back to registering when the metadata doesn't advertise one. The consent
807+
page signs in with the user's existing access token, so there's no new
808+
credential, and only takes POSTs from its own page (Origin,
809+
Sec-Fetch-Site), against login CSRF; it's plain HTML with its own CSP,
810+
whose `form-action` allows the app's callback origin, as Chrome applies
811+
`form-action` to the redirect after a POST. PKCE is S256 only. Loopback
812+
redirect URIs match on any port (RFC 8252), as Claude Code picks a free
813+
port each time. Access tokens last an hour and refresh tokens 30 days;
814+
refresh tokens rotate, and one used again more than 30 s after its first
815+
use revokes its grant (it was copied); sooner is a retry or two requests
816+
refreshing at once, and gets tokens. A grant remembers the login token
817+
the user consented with, so revoking that token, **New token** or
818+
deleting the user ends it too. `PUBLIC_URL`, when set, is the issuer and
819+
resource URL: behind a proxy the request's own host may not be what
820+
clients use.
817821

818822
**The MCP write tools have no amount cap; the plan is the guard.** A fixed
819823
cap can't be right for every item (100k mB of a fluid is little, 100k of an

‎server/gcm/db.py‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -643,7 +643,8 @@ def _app_oauth(conn):
643643
id TEXT PRIMARY KEY,
644644
name TEXT NOT NULL,
645645
redirect_uris TEXT NOT NULL,
646-
created_at REAL NOT NULL
646+
created_at REAL NOT NULL,
647+
created_by_ip TEXT
647648
)
648649
""")
649650
conn.execute("""

‎server/gcm/mcp/protocol.py‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -158,10 +158,9 @@ def handle(message, headers, user):
158158
name = params.get("name")
159159
if not isinstance(name, str):
160160
return 200, error(msg_id, INVALID_PARAMS, "params.name must be a tool name")
161-
try:
162-
answer, is_error = tools.call(name, params.get("arguments"), user)
163-
except KeyError:
161+
if not tools.exists(name):
164162
return 200, error(msg_id, INVALID_PARAMS, f"Unknown tool: {name}")
163+
answer, is_error = tools.call(name, params.get("arguments"), user)
165164
result = {"content": [{"type": "text", "text": _compact_json(answer)}], "isError": is_error}
166165
else:
167166
return (404 if modern else 200), error(msg_id, METHOD_NOT_FOUND, f"Method not found: {method}")

‎server/gcm/mcp/tools.py‎

Lines changed: 47 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -12,13 +12,17 @@
1212
of several items called alike."""
1313

1414
import datetime
15+
import logging
16+
import math
1517
import time
1618

1719
from gcm import (
1820
auth, bottlenecks, commands, craft_stats, inventory, item_info, patterns, planner, rates,
1921
mctext, search, state, stock, store,
2022
)
2123

24+
log = logging.getLogger(__name__)
25+
2226

2327
class ToolError(Exception):
2428
"""A tool call that can't be answered: its message goes back to the
@@ -136,13 +140,14 @@ def _arg(args, name, kind, default=None, choices=None, minimum=None, maximum=Non
136140
if required:
137141
raise ToolError(f"{name} is required")
138142
return None
139-
if kind == "integer":
140-
if isinstance(value, bool) or not isinstance(value, (int, float)) or value != int(value):
141-
raise ToolError(f"{name} must be a whole number")
142-
value = int(value)
143-
elif kind == "number":
144-
if isinstance(value, bool) or not isinstance(value, (int, float)):
143+
if kind in ("integer", "number"):
144+
# JSON allows 1e400, which Python reads as infinity.
145+
if isinstance(value, bool) or not isinstance(value, (int, float)) or not math.isfinite(value):
145146
raise ToolError(f"{name} must be a number")
147+
if kind == "integer":
148+
if value != int(value):
149+
raise ToolError(f"{name} must be a whole number")
150+
value = int(value)
146151
elif kind == "string":
147152
if not isinstance(value, str):
148153
raise ToolError(f"{name} must be a string")
@@ -177,7 +182,7 @@ def _range_arg(choices, default, what):
177182

178183

179184
def _rank(item, query):
180-
name = (item.get("name") or "").lower()
185+
name = mctext.plain(item.get("name") or "").lower()
181186
q = query.strip().lower()
182187
if name == q:
183188
return 0
@@ -211,7 +216,7 @@ def search_items(user, args):
211216
item_info.mod_name(item.get("mod")), info.get("formula"),
212217
):
213218
found.append(item)
214-
found.sort(key=lambda it: (_rank(it, query), len(it.get("name") or ""), -(it.get("size") or 0)))
219+
found.sort(key=lambda it: (_rank(it, query), len(mctext.plain(it.get("name") or "")), -(it.get("size") or 0)))
215220
return {
216221
"scanned_at": _iso(scanned_at),
217222
"matches": len(found),
@@ -283,11 +288,14 @@ def inventory_changes(user, args):
283288
sort = _arg(args, "sort", "string", "biggest", choices=CHANGE_SORTS)
284289
limit = _arg(args, "limit", "integer", 25, minimum=1, maximum=100)
285290
range_key, changes = inventory.changes(range_key)
286-
sizes, _, _ = inventory.stock_levels()
291+
# One pass over the snapshot: looking each item up would go through
292+
# all of it again for every changed item.
293+
with state.network_lock:
294+
by_key = {store.items.key_of(it): it for it in state.network["items"]}
287295
rows = []
288296
for key, change in changes.items():
289-
item = _network_item(key)
290-
now = sizes.get(key, 0)
297+
item = by_key.get(key)
298+
now = (item.get("size") or 0) if item else 0
291299
delta = now - change["from"]
292300
if (sort == "falling" and delta >= 0) or (sort == "rising" and delta <= 0):
293301
continue
@@ -688,6 +696,13 @@ def cancel_craft(user, args):
688696
except ValueError:
689697
raise ToolError("job_id must be the job_id current_crafts gave for this CPU") from None
690698
shown = {"mod": parts[0] or None, "internal": parts[1], "damage": damage}
699+
if shown is None:
700+
# Without it, a CPU that moved on to another job since the user
701+
# agreed would have that one cancelled instead.
702+
with state.crafts_lock:
703+
job = next((j for j in state.crafts["jobs"] if j.get("name") == cpu), None)
704+
if job is not None and job.get("busy") and _job_id(job):
705+
raise ToolError("Pass the job_id current_crafts gave for this CPU, so only that job is cancelled.")
691706
req_id, error = commands.request_cancel(user["id"], cpu, shown)
692707
if error:
693708
raise ToolError(error[0])
@@ -700,6 +715,10 @@ def cancel_craft(user, args):
700715
def request_status(user, args):
701716
req_id = _arg(args, "request_id", "integer", required=True)
702717
kind = _arg(args, "type", "string", "craft", choices=("craft", "cancel"))
718+
# A request the game never picked up is only marked failed when its
719+
# queue is next looked at (the game's poll, the page): look now.
720+
queue = commands.craft_requests if kind == "craft" else commands.cancel_requests
721+
queue.select(lambda r: False)
703722
found = store.requests.find(kind, req_id, user["id"])
704723
if found is None:
705724
raise ToolError(f"You have no {kind} request with that id.")
@@ -962,7 +981,10 @@ def request_status(user, args):
962981
),
963982
"inputSchema": _schema({
964983
"cpu": {"type": "string", "description": "A CPU name from current_crafts."},
965-
"job_id": {"type": "string", "description": "The job_id current_crafts gave for that CPU's job."},
984+
"job_id": {"type": "string", "description": (
985+
"The job_id current_crafts gave for that CPU's job. Required unless current_crafts "
986+
"gave none (the CPU has no Crafting Monitor)."
987+
)},
966988
}, ["cpu"]),
967989
"annotations": GAME_CHANGES,
968990
"operator": True,
@@ -983,12 +1005,15 @@ def listing(user):
9831005
]
9841006

9851007

1008+
def exists(name):
1009+
return name in _BY_NAME
1010+
1011+
9861012
def call(name, args, user):
987-
"""Runs a tool for a user. Returns (result, is_error): result is the
988-
tool's JSON-able answer, or an error message for the model."""
989-
tool = _BY_NAME.get(name)
990-
if tool is None:
991-
raise KeyError(name)
1013+
"""Runs a tool (exists(name)) for a user. Returns (result, is_error):
1014+
result is the tool's JSON-able answer, or an error message for the
1015+
model."""
1016+
tool = _BY_NAME[name]
9921017
if args is None:
9931018
args = {}
9941019
if not isinstance(args, dict):
@@ -1000,3 +1025,8 @@ def call(name, args, user):
10001025
return _compact_numbers(tool["handler"](user, args)), False
10011026
except ToolError as e:
10021027
return str(e), True
1028+
except Exception:
1029+
# A bug, not the model's mistake: logged, and the model told so
1030+
# rather than the client getting a 500 it can't read.
1031+
log.exception("MCP tool %s failed", name)
1032+
return "The server hit an error running this tool. Its log has the details.", True

‎server/gcm/routes/oauth.py‎

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,7 @@ def register():
9090
name = urlsplit(uris[0]).hostname or "An app"
9191
name = " ".join(name.split())[: oauth.MAX_CLIENT_NAME]
9292

93-
client_id = store.oauth.create_client(name, uris)
93+
client_id = store.oauth.create_client(name, uris, request.remote_addr)
9494
if client_id is None:
9595
return jsonify({"error": "temporarily_unavailable",
9696
"error_description": "too many registrations; try again later"}), 429
@@ -248,9 +248,22 @@ def authorize_get():
248248
return _consent_page(params, client, auth.session_user())
249249

250250

251+
def _from_this_site():
252+
"""Whether a form POST came from this server's own pages. Browsers
253+
send Origin on every cross-site POST (Sec-Fetch-Site too): without
254+
this, another site could post a sign-in with its own token and sign
255+
the visitor in as someone else."""
256+
origin = request.headers.get("Origin")
257+
if origin is not None and origin != request.host_url.rstrip("/"):
258+
return False
259+
return request.headers.get("Sec-Fetch-Site") in (None, "same-origin", "none")
260+
261+
251262
@bp.route("/oauth/authorize", methods=["POST"])
252-
@auth.custom_check # signing in needs no session; allowing checks it and the form value
263+
@auth.custom_check # same-site only; signing in needs no session, allowing checks it and the form value
253264
def authorize_post():
265+
if not _from_this_site():
266+
return _error_page("This form has to be sent from this server's own page.", 403)
254267
params, client, error = _authorize_request(request.form)
255268
if error and error[1] is None:
256269
return _error_page(error[0])

‎server/gcm/stock.py‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
before asking again."""
2424

2525
import logging
26+
import math
2627
import time
2728

2829
from gcm import charts, commands, config, icons, push, state, store, webhooks
@@ -141,7 +142,9 @@ def rule_key(item):
141142

142143

143144
def _positive(value):
144-
return not isinstance(value, bool) and isinstance(value, (int, float)) and value > 0
145+
# Finite too: JSON's 1e400 reads as infinity, which a refill would
146+
# then ask the game for.
147+
return not isinstance(value, bool) and isinstance(value, (int, float)) and math.isfinite(value) and value > 0
145148

146149

147150
def _network_item(key):

‎server/gcm/store/oauth.py‎

Lines changed: 24 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -19,33 +19,40 @@
1919

2020
# Clients are made by anyone who asks (RFC 7591: Claude registers on every
2121
# new connection), so ones never used for a grant are dropped after a day,
22-
# and only so many may be made in an hour.
22+
# and only so many may be made in an hour: per address, so one script
23+
# can't use up everyone's, and in all, as a backstop.
2324
UNUSED_CLIENT_SECONDS = 86400
24-
CLIENTS_PER_HOUR = 200
25+
CLIENTS_PER_HOUR_PER_IP = 30
26+
CLIENTS_PER_HOUR = 2000
27+
# A refresh token used again this soon after its first use is a client
28+
# retrying, or two requests refreshing at once, not a copy: it gets new
29+
# tokens instead of revoking the grant.
30+
REFRESH_REUSE_GRACE_SECONDS = 30
2531

2632
ACCESS_PREFIX = "gcmat_"
2733
REFRESH_PREFIX = "gcmrt_"
2834

2935

30-
def create_client(name, redirect_uris):
31-
"""Registers a client; returns its id, or None when too many were
32-
registered in the last hour."""
36+
def create_client(name, redirect_uris, ip=None):
37+
"""Registers a client for a request from ip; returns its id, or None
38+
when too many were registered in the last hour."""
3339
now = time.time()
3440
with db.transaction(db.app_db) as conn:
3541
conn.execute(
3642
"DELETE FROM oauth_clients WHERE created_at < ? "
3743
"AND id NOT IN (SELECT client_id FROM oauth_grants)",
3844
(now - UNUSED_CLIENT_SECONDS,),
3945
)
40-
recent = conn.execute(
41-
"SELECT COUNT(*) FROM oauth_clients WHERE created_at > ?", (now - 3600,)
42-
).fetchone()[0]
43-
if recent >= CLIENTS_PER_HOUR:
46+
recent, from_ip = conn.execute(
47+
"SELECT COUNT(*), COALESCE(SUM(created_by_ip IS ?), 0) FROM oauth_clients WHERE created_at > ?",
48+
(ip, now - 3600),
49+
).fetchone()
50+
if recent >= CLIENTS_PER_HOUR or from_ip >= CLIENTS_PER_HOUR_PER_IP:
4451
return None
4552
client_id = f"gcmc_{secrets.token_hex(16)}"
4653
conn.execute(
47-
"INSERT INTO oauth_clients (id, name, redirect_uris, created_at) VALUES (?, ?, ?, ?)",
48-
(client_id, name, json.dumps(redirect_uris), now),
54+
"INSERT INTO oauth_clients (id, name, redirect_uris, created_at, created_by_ip) VALUES (?, ?, ?, ?, ?)",
55+
(client_id, name, json.dumps(redirect_uris), now, ip),
4956
)
5057
return client_id
5158

@@ -137,8 +144,9 @@ def create_grant(code_details):
137144

138145
def refresh(refresh_token, client_id):
139146
"""New tokens for a refresh token, which is used up. A refresh token
140-
used a second time means it was copied: the whole grant is revoked.
141-
None when it can't be used."""
147+
used again later than REFRESH_REUSE_GRACE_SECONDS after its first
148+
use means it was copied: the whole grant is revoked. None when it
149+
can't be used."""
142150
if not isinstance(refresh_token, str) or not refresh_token.startswith(REFRESH_PREFIX):
143151
return None
144152
now = time.time()
@@ -154,14 +162,15 @@ def refresh(refresh_token, client_id):
154162
if not row or row[3] != client_id:
155163
return None
156164
grant_id, expires_at, used_at, _, revoked_at, disabled_at = row
157-
if used_at is not None:
165+
if used_at is not None and now - used_at > REFRESH_REUSE_GRACE_SECONDS:
158166
conn.execute(
159167
"UPDATE oauth_grants SET revoked_at = ? WHERE id = ? AND revoked_at IS NULL", (now, grant_id)
160168
)
161169
return None
162170
if expires_at <= now or revoked_at is not None or disabled_at is not None:
163171
return None
164-
conn.execute("UPDATE oauth_tokens SET used_at = ? WHERE token_hash = ?", (now, hash_secret(refresh_token)))
172+
if used_at is None:
173+
conn.execute("UPDATE oauth_tokens SET used_at = ? WHERE token_hash = ?", (now, hash_secret(refresh_token)))
165174
return _issue(conn, grant_id, now)
166175

167176

‎server/tests/test_mcp.py‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -216,6 +216,19 @@ def test_not_json_rpc(self, client, token):
216216
assert post(client, token, {"hello": 1}).status_code == 400
217217
assert post(client, token, [{"jsonrpc": "2.0", "id": 1, "method": "ping"}]).status_code == 400
218218

219+
def test_a_bug_in_a_tool_is_a_tool_error_not_a_500(self, client, token, monkeypatch):
220+
def broken(user, args):
221+
return {}["missing"]
222+
223+
monkeypatch.setitem(tools._BY_NAME["current_crafts"], "handler", broken)
224+
answer, is_error = call(client, token, "current_crafts")
225+
assert is_error and "server hit an error" in answer
226+
227+
def test_infinite_numbers_are_refused(self, client, token):
228+
# 1e400 in JSON is infinity to Python.
229+
answer, is_error = call(client, token, "search_items", query="x", limit=float("inf"))
230+
assert is_error and answer == "limit must be a number"
231+
219232
def test_bad_arguments_come_back_as_tool_errors(self, client, token):
220233
assert call(client, token, "search_items") == ("query is required", True)
221234
assert call(client, token, "search_items", query="x", bogus=1) == ("unknown argument(s): bogus", True)
@@ -246,6 +259,14 @@ def test_search_ranks_the_closest_names_first(self, client, token, network):
246259
"unit": "items", "craftable": True}
247260
assert ok(client, token, "search_items", query="iron plate")["items"][0]["name"] == "Iron Plate"
248261

262+
def test_search_ranks_by_the_name_without_colour_codes(self, client, token, api_headers):
263+
run_scan(client, api_headers, [[
264+
{"name": "Infinity Catalyst Dust", "mod": "gregtech", "internal": "dust", "damage": 1, "size": 1},
265+
{"name": "\u00a76Infinity Catalyst", "mod": "gregtech", "internal": "ingot", "damage": 1, "size": 1},
266+
]])
267+
found = ok(client, token, "search_items", query="infinity catalyst")
268+
assert found["items"][0]["name"] == "Infinity Catalyst"
269+
249270
def test_search_filters(self, client, token, network):
250271
assert ok(client, token, "search_items", query="ingot", craftable_only=True)["matches"] == 1
251272
assert ok(client, token, "search_items", query="@gregtech")["items"][0]["name"] == "Iron Plate"

0 commit comments

Comments
 (0)